[PATCH 11/11] read-cache: roll back lock on error with `COMMIT_LOCK`

as-is on error is appropriate, since that is how `close_lockfile_gently()` behaves. (There are not many in-tree users and they all `die()` on error.) Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- builtin/difftool.c | 1 - cache.h| 1 + merge.c| 4 +--- read-c

[PATCH 00/11] various lockfile-leaks and -fixes

nd like we do. Martin [1] https://public-inbox.org/git/20170905121353.62zg3mtextmq5...@sigill.intra.peff.net/ Martin Ågren (11): sha1_file: do not leak `lock_file` treewide: prefer lockfiles on the stack lockfile: fix documentation on `close_lock_file_gently()` tempfile: fix doc

[PATCH 01/11] sha1_file: do not leak `lock_file`

with `LOCK_DIE_ON_ERROR`, it evaluates to false exactly when we have called `rollback_lock_file()`. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- sha1_file.c | 16 +++- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 5a2014811..c50

[PATCH 06/11] apply: move lockfile into `apply_state`

in the next patch.) Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- apply.c | 14 +- apply.h | 5 ++--- builtin/am.c| 3 +-- builtin/apply.c | 4 +--- 4 files changed, 9 insertions(+), 17 deletions(-) diff --git a/apply.c b/apply.c index c02

[PATCH 02/11] treewide: prefer lockfiles on the stack

the `struct lock_file` on the stack instead. These instances were identified by running `git grep "^\s*struct lock_file\s*\*"`. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- The changes to config.c conflict on pu (52d59cc64 (branch: add a --copy (-c) option to go with --move

[PATCH 05/11] cache-tree: simplify locking logic

` so that we have one variable less to reason about. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- cache-tree.c | 12 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/cache-tree.c b/cache-tree.c index 71d092ed5..f646f5673 100644 --- a/cache-tree.c +++ b

[PATCH 03/11] lockfile: fix documentation on `close_lock_file_gently()`

Commit 83a3069a3 (lockfile: do not rollback lock on failed close, 2017-09-05) forgot to update the documentation by the function definition to reflect that the lock is not rolled back in case closing fails. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- lockfile.h | 4 ++--

[PATCH 04/11] tempfile: fix documentation on `delete_tempfile()`

The function has always been documented as returning 0 or -1. It is in fact `void`. Correct that. As part of the rearrangements we lose the mention that `delete_tempfile()` might set `errno`. Because there is no return value, the user can't really know whether it did anyway. Signed-off-by: Martin

Re: [PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes

On 24 September 2017 at 09:01, Junio C Hamano <gits...@pobox.com> wrote: > Martin Ågren <martin.ag...@gmail.com> writes: > >> Since Junio collected my "independent" patches into ma/leakplugs, this >> is a re-roll of that whole topic. I got the first tw

Re: [PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes

On 23 September 2017 at 18:38, Jeff King wrote: > On Sat, Sep 23, 2017 at 12:13:16PM -0400, Jeff King wrote: > >> In theory you should be able to just add "log_path=/tmp/lsan/output" to >> that, which should put all the logs in a convenient place (and stop the >> extra output from

Re: [PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes

On 23 September 2017 at 06:37, Jeff King <p...@peff.net> wrote: > On Sat, Sep 23, 2017 at 01:34:48AM +0200, Martin Ågren wrote: > >> Martin Ågren (6): >> builtin/commit: fix memory leak in `prepare_index()` >> commit: fix memory leak in `reduce_h

Re: [PATCH v2 5/6] object_array: add and use `object_array_pop()`

On 23 September 2017 at 06:27, Jeff King <p...@peff.net> wrote: > On Sat, Sep 23, 2017 at 01:34:53AM +0200, Martin Ågren wrote: > >> Introduce and use `object_array_pop()` instead. Release memory in the >> new function. Document that popping an object leaves the associa

Re: [PATCH v2 4/6] object_array: use `object_array_clear()`, not `free()`

On 23 September 2017 at 06:04, Jeff King <p...@peff.net> wrote: > On Sat, Sep 23, 2017 at 01:34:52AM +0200, Martin Ågren wrote: > >> The way we handle `study` in builting/reflog.c still looks like it might >> leak. That will be addressed in the next commit. > > This c

Re: [PATCH 1/4] git-merge: Honor pre-merge hook

On 22 September 2017 at 14:04, Michael J Gruber wrote: > From: Michael J Gruber > > git-merge does not honor the pre-commit hook when doing automatic merge > commits, and for compatibility reasons this is going to stay. > > Introduce a pre-merge hook

Re: [PATCH v8 08/12] fsmonitor: add a test tool to dump the index extension

On 22 September 2017 at 18:35, Ben Peart wrote: > Add a test utility (test-dump-fsmonitor) that will dump the fsmonitor > index extension. > > Signed-off-by: Ben Peart > --- > Makefile | 1 + > t/helper/test-dump-fsmonitor.c

Re: [PATCH v8 01/12] bswap: add 64 bit endianness helper get_be64

On 22 September 2017 at 18:35, Ben Peart wrote: > Add a new get_be64 macro to enable 64 bit endian conversions on memory > that may or may not be aligned. I needed this to compile and pass the tests with NO_UNALIGNED_LOADS. Martin diff --git a/compat/bswap.h

[PATCH v2 3/6] leak_pending: use `object_array_clear()`, not `free()`

users get this right. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- bisect.c | 3 ++- builtin/checkout.c | 9 - bundle.c | 9 - revision.h | 11 +++ 4 files changed, 29 insertions(+), 3 deletions(-) diff --git a/bise

[PATCH v2 6/6] pack-bitmap[-write]: use `object_array_clear()`, don't leak

really needed and just make it harder than necessary to understand the code. While we're here, remove the aliases to make the code easier to follow. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- pack-bitmap-write.c | 4 +--- pack-bitmap.c | 10 +++--- 2 files changed,

[PATCH v2 5/6] object_array: add and use `object_array_pop()`

)`. That is unfortunate. On the other hand, it matches `object_array_clear()`. Arguably it's `add_...` that is the odd one out, since it reads like it's used to "add" an "object array". For that reason, side with `object_array_clear()`. Signed-off-by: Martin Ågren <martin.ag

[PATCH v2 4/6] object_array: use `object_array_clear()`, not `free()`

` in builting/reflog.c still looks like it might leak. That will be addressed in the next commit. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- builtin/reflog.c | 4 ++-- diff-lib.c | 3 +-- submodule.c | 4 ++-- upload-pack.c| 2 +- 4 files changed, 6 insertions

[PATCH v2 1/6] builtin/commit: fix memory leak in `prepare_index()`

ase it. That introduces some unnecessary overhead in various code paths, but means there is one and only one way out of the function. If we ever accumulate more things we need to free, it should be straightforward to do so. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> Reviewed-by: Jef

[PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes

On 20 September 2017 at 22:02, Jeff King <p...@peff.net> wrote: > On Wed, Sep 20, 2017 at 09:47:24PM +0200, Martin Ågren wrote: > >> Instead of conditionally freeing `rev.pending.objects`, just call >> `object_array_clear()` on `rev.pending`. This means

[PATCH v2 2/6] commit: fix memory leak in `reduce_heads()`

We don't free the temporary scratch space we use with `remove_redundant()`. Free it similar to how we do it in `get_merge_bases_many_0()`. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> Reviewed-by: Jeff King <p...@peff.net> Signed-off-by: Junio C Hamano <gits...@pobox.com

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

On 20 September 2017 at 22:36, Jeff King wrote: > On Wed, Sep 20, 2017 at 04:25:52PM -0400, Jeff King wrote: > >> Isn't this whole thing just an argv_array, and this is argv_array_pushv? >> We even NULL-terminate it manually later on! >> >> So rather than increasing the line count

Re: [PATCH] diff-lib: clear `pending` object-array in `index_differs_from()`

On 20 September 2017 at 22:02, Jeff King <p...@peff.net> wrote: > On Wed, Sep 20, 2017 at 09:47:24PM +0200, Martin Ågren wrote: > >> Instead of conditionally freeing `rev.pending.objects`, just call >> `object_array_clear()` on `rev.pending`. This means

[PATCH] diff-lib: clear `pending` object-array in `index_differs_from()`

-by: Martin Ågren <martin.ag...@gmail.com> --- diff-lib.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/diff-lib.c b/diff-lib.c index 2a52b07..4e0980c 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -549,7 +549,6 @@ int index_differs_from(const char *def, int diff

[PATCH] revision: fix memory leaks with `struct cmdline_pathspec`

it is clear who owns the strings and that we can stop leaking those that we do allocate. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- revision.c | 15 ++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/revision.c b/revision.c index f9a90d7..dfb6a6c

[PATCH] commit: fix memory leak in `reduce_heads()`

We don't free the temporary scratch space we use with `remove_redundant()`. Free it similar to how we do it in `get_merge_bases_many_0()`. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- commit.c | 1 + 1 file changed, 1 insertion(+) diff --git a/commit.c b/commit.c index 9

[PATCH] commit: fix memory leak in `prepare_index()`

ase it. That introduces some unnecessary overhead in various code paths, but means there is one and only one way out of the function. If we ever accumulate more things we need to free, it should be straightforward to do so. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- builtin/comm

Re: [PATCH v7 05/12] fsmonitor: add documentation for the fsmonitor extension.

On 20 September 2017 at 19:02, Ben Peart wrote: >>> +--[no-]fsmonitor-valid:: >>> + When one of these flags is specified, the object name recorded >>> + for the paths are not updated. Instead, these options >>> + set and unset the "fsmonitor valid" bit for

Re: [PATCH v7 05/12] fsmonitor: add documentation for the fsmonitor extension.

On 19 September 2017 at 21:27, Ben Peart wrote: > diff --git a/Documentation/git-update-index.txt > b/Documentation/git-update-index.txt > index e19eba62cd..95231dbfcb 100644 > --- a/Documentation/git-update-index.txt > +++ b/Documentation/git-update-index.txt > @@ -16,9

[PATCH v4 0/4] Rerolling ma/split-symref-update-fix

efname` instead of "HEAD" into the list of affected refnames. Thanks all for comments, suggestions and help along the way. Martin Martin Ågren (4): refs/files-backend: add longer-scoped copy of string to list refs/files-backend: fix memory leak in lock_ref_for_update refs/files-backend

[PATCH v4 4/4] refs/files-backend: add `refname`, not "HEAD", to list

The downside is a call to `string_list_has_string()`, which should be relatively cheap. Signed-off-by: Jeff King <p...@peff.net> Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- refs/files-backend.c | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --g

[PATCH v4 3/4] refs/files-backend: correct return value in lock_ref_for_update

hag...@alum.mit.edu> Reviewed-by: Michael Haggerty <mhag...@alum.mit.edu> Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 3d6363966..03df00275 10064

[PATCH v4 2/4] refs/files-backend: fix memory leak in lock_ref_for_update

After the previous patch, none of the functions we call hold on to `referent.buf`, so we can safely release the string buffer before returning. Reviewed-by: Michael Haggerty <mhag...@alum.mit.edu> Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- refs/files-ba

[PATCH v4 1/4] refs/files-backend: add longer-scoped copy of string to list

tly to `affected_refnames`. Instead, first just check whether `referent` exists in the string list, and later add `new_update->refname`. Helped-by: Michael Haggerty <mhag...@alum.mit.edu> Reviewed-by: Michael Haggerty <mhag...@alum.mit.edu> Signed-off-by: Martin Ågren <martin.ag...@gmail

Re: Git diff --no-index and file descriptor inputs

On 7 September 2017 at 18:00, Jason Pyeron wrote: > > I was hoping to leverage --word-diff-regex, but the --no-index option does > not seem to work with <(...) notation. > > I am I doing something wrong or is this a bug? There were some patches floating around half a year ago,

Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list

On 5 September 2017 at 23:26, Junio C Hamano wrote: > Jeff King writes: > >> I noticed the HEAD funniness, too, when looking at this earlier. I agree >> with Junio that it's not quite consistent with the general rule of >> "string list items point to their

Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives

On 5 September 2017 at 15:05, Jeff King wrote: > 1. It can be compiled conditionally. There's no need in > normal runs to do this free(), and it just wastes time. > By using a macro, we can get the benefit for leak-check > builds with zero cost for normal builds

Re: [PATCH 0/10] towards clean leak-checker output

On 5 September 2017 at 21:02, Jeff King <p...@peff.net> wrote: > On Tue, Sep 05, 2017 at 07:50:10PM +0200, Martin Ågren wrote: > >> > That line is the setting of argv0_path, which is a global (and thus >> > shouldn't be marked as leaking). Interestingly, it

[PATCH] config: remove git_config_maybe_bool

The function was deprecated in commit 89576613 ("treewide: deprecate git_config_maybe_bool, use git_parse_maybe_bool", 2017-08-07) and has no users. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- ma/parse-maybe-bool has been in master for some time and I cannot find any

Re: [PATCH 0/10] towards clean leak-checker output

On 5 September 2017 at 15:01, Jeff King wrote: > Using leak-checking tools like valgrind or LSAN is usually not all that > productive with Git, because the output is far from clean. And _most_ of > these are just not interesting, as they're either: > > 1. Real leaks, but ones

Re: [PATCH v3 2/3] refs/files-backend: fix memory leak in lock_ref_for_update

On 5 September 2017 at 10:47, Jeff King <p...@peff.net> wrote: > On Tue, Aug 29, 2017 at 07:18:23PM +0200, Martin Ågren wrote: > >> After the previous patch, none of the functions we call hold on to >> `referent.buf`, so we can safely release the string buffer before >

Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list

On 5 September 2017 at 12:02, Junio C Hamano <gits...@pobox.com> wrote: > Martin Ågren <martin.ag...@gmail.com> writes: > >> On 30 August 2017 at 04:52, Michael Haggerty <mhag...@alum.mit.edu> wrote: >>> v3 looks good to me. Thanks! >>> >>

Re: [PATCH] hashmap: add API to disable item counting when threaded

On 5 September 2017 at 18:39, Jeff Hostetler wrote: > > > On 9/1/2017 7:50 PM, Jonathan Nieder wrote: >> >> Hi, >> >> Johannes Schindelin wrote: >>> >>> On Wed, 30 Aug 2017, Jeff Hostetler wrote: >> >> This is to address concerns raised by ThreadSanitizer on the

Re: [PATCH 17/34] mailinfo: release strbuf on error return in handle_boundary()

On 31 August 2017 at 19:21, René Scharfe <l@web.de> wrote: > Am 30.08.2017 um 20:23 schrieb Martin Ågren: >> On 30 August 2017 at 19:49, Rene Scharfe <l@web.de> wrote: >>> Signed-off-by: Rene Scharfe <l@web.de> >>> --- >>>

Re: [PATCH 17/34] mailinfo: release strbuf on error return in handle_boundary()

On 30 August 2017 at 19:49, Rene Scharfe wrote: > Signed-off-by: Rene Scharfe > --- > mailinfo.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mailinfo.c b/mailinfo.c > index b1f5159546..f2387a3267 100644 > --- a/mailinfo.c > +++ b/mailinfo.c > @@ -911,48

Re: Commit dropped when swapping commits with rebase -i -p

On 30 August 2017 at 12:11, Sebastian Schuberth wrote: > Hi, > > I believe stumbled upon a nasty bug in Git: It looks like a commits gets > dropped during interactive rebase when asking to preserve merges. Steps: > > $ git clone -b git-bug --single-branch >

Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list

On 30 August 2017 at 04:52, Michael Haggerty wrote: > v3 looks good to me. Thanks! > > Reviewed-by: Michael Haggerty Thank _you_ for very helpful feedback on the earlier versions. Martin

Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()

On 29 August 2017 at 20:53, Jeff King wrote: > On Tue, Aug 29, 2017 at 06:51:52PM +0100, Lars Schneider wrote: >> What if we run a few selected tests with valgrind and count all files that >> valgrind mentions (a single leak has multiple file mentions because of >> the stack trace

[PATCH v3 3/3] refs/files-backend: correct return value in lock_ref_for_update

hag...@alum.mit.edu> Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- v3: this is a new patch, as suggested by Michael refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index a2b3df21b..ad05d1d5f 100644 ---

[PATCH v3 2/3] refs/files-backend: fix memory leak in lock_ref_for_update

After the previous patch, none of the functions we call hold on to `referent.buf`, so we can safely release the string buffer before returning. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- v3: identical to v2 refs/files-backend.c | 31 --- 1 file c

[PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list

tly to `affected_refnames`. Instead, first just check whether `referent` exists in the string list, and later add `new_update->refname`. Helped-by: Michael Haggerty <mhag...@alum.mit.edu> Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- v3: "O(lg N)" in comment; if-BUG() inst

Re: [PATCH v2 2/2] refs/files-backend: fix memory leak in lock_ref_for_update

On 29 August 2017 at 10:39, Michael Haggerty <mhag...@alum.mit.edu> wrote: > On 08/28/2017 10:32 PM, Martin Ågren wrote: >> After the previous patch, none of the functions we call hold on to >> `referent.buf`, so we can safely release the string buffer before >> retu

[PATCH v2 2/2] refs/files-backend: fix memory leak in lock_ref_for_update

After the previous patch, none of the functions we call hold on to `referent.buf`, so we can safely release the string buffer before returning. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- refs/files-backend.c | 31 --- 1 file changed, 20 insertions(

[PATCH v2 1/2] refs/files-backend: add longer-scoped copy of string to list

eck whether `referent` exists in the string list, and later add `new_update->refname`. Helped-by: Michael Haggerty <mhag...@alum.mit.edu> Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- Thanks Junio and Michael for your comments on the first version. This first patch is now co

Re: [PATCH 1/2] refs/files-backend: duplicate strings added to affected_refnames

On 28 August 2017 at 10:06, Michael Haggerty <mhag...@alum.mit.edu> wrote: > On Sat, Aug 26, 2017 at 12:16 PM, Martin Ågren <martin.ag...@gmail.com> wrote: >> On 25 August 2017 at 23:00, Junio C Hamano <gits...@pobox.com> wrote: >>> Martin Å

Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()

On 28 August 2017 at 01:23, Jeff King wrote: > On Sun, Aug 27, 2017 at 10:04:55PM +0200, Lars Schneider wrote: > >> I did run all tests under valgrind using "make valgrind" and I found >> the following files with potential issues: >> >> cat valgrind.out | perl -nE 'say

Re: [PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()

On 27 August 2017 at 20:21, Lars Schneider <larsxschnei...@gmail.com> wrote: > >> On 27 Aug 2017, at 09:37, Martin Ågren <martin.ag...@gmail.com> wrote: >> >> The static-ness was silently dropped in commit 70428d1a5 ("pkt-line: add >> packet_write_fmt_g

[PATCH] pkt-line: re-'static'-ify buffer in packet_write_fmt_1()

returning (but not before we die, so that we don't touch errno). That would also prepare us for threaded use. But until that needs to happen, let's just restore the static-ness so that we get back to a situation where we (eventually) do not continuosly keep allocating memory. Signed-off-by: Martin Ågren

Re: [PATCH 1/2] refs/files-backend: duplicate strings added to affected_refnames

On 25 August 2017 at 23:00, Junio C Hamano <gits...@pobox.com> wrote: > Martin Ågren <martin.ag...@gmail.com> writes: > >> files_transaction_prepare() and the functions it calls add strings to a >> string list without duplicating them, i.e., we keep the original

[PATCH 2/2] refs/files-backend: fix memory leak in lock_ref_for_update

fer before returning. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- I appreciate Git's stance on memory leaks ("if we're about to exit or die, why bother?") but this one feels different since lock_ref_for_update is called in a loop rather deep down the call stack. Feel free to te

[PATCH 1/2] refs/files-backend: duplicate strings added to affected_refnames

o it is ok for string_list_clear to free them. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 5cca55510..22daca2ba 100644 --- a/refs/files-backend.c

Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf

On 24 August 2017 at 20:29, Brandon Casey wrote: > On Thu, Aug 24, 2017 at 9:52 AM, Junio C Hamano wrote: >> Brandon Casey writes: >> >>> Ah, you probably meant something like this: >>> >>>const char strbuf_slopbuf = '\0'; >>> >>>

Re: [PATCH 2/2] treewide: correct several "up-to-date" to "up to date"

On 24 August 2017 at 05:51, STEVEN WHITE <stevencharleswhitevoi...@gmail.com> wrote: > On Wed, Aug 23, 2017 at 10:49 AM, Martin Ågren <martin.ag...@gmail.com> wrote: >> Follow the Oxford style, which says to use "up-to-date" before the noun, >> but "u

[PATCH 2/2] treewide: correct several "up-to-date" to "up to date"

to date". It turned out we only had to edit in one direction, removing the hyphens. Fix a typo in Documentation/git-diff-index.txt while we're there. Reported-by: Jeffrey Manian <jeffrey.man...@gmail.com> Reported-by: STEVEN WHITE <stevencharleswhitevoi...@gmail.com> Signed-off-by

[PATCH 1/2] Documentation/user-manual: update outdated example output

Since commit f7673490 ("more terse push output", 2007-11-05), git push has a completely different output format than the one shown in the user manual for a non-fast-forward push. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- I'd say it's "not very many read this

Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf

On 23 August 2017 at 19:24, Junio C Hamano <gits...@pobox.com> wrote: > Martin Ågren <martin.ag...@gmail.com> writes: > >> strbuf_setlen(., 0) writes '\0' to sb.buf[0], where buf is either >> allocated and unique to sb, or the global slopbuf. The slopbuf is meant >

Re: [PATCH v2 1/3] Documentation/git-merge: explain --continue

On 22 August 2017 at 11:26, Michael J Gruber <g...@grubix.eu> wrote: > Martin Ågren venit, vidit, dixit 21.08.2017 18:43: >> On 21 August 2017 at 14:53, Michael J Gruber <g...@grubix.eu> wrote: >>> Currently, 'git merge --continue' is mentioned but not explained. >

[PATCH v2 4/4] ThreadSanitizer: add suppressions

ile is used by setting the environment variable TSAN_OPTIONS to, e.g., "suppressions=$(pwd)/.tsan-suppressions". Observe that relative paths such as ".tsan-suppressions" might not work. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- v2: added NEEDSWORK; reworded

[PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf

1510528 bytes large (another 0.1% increase). Suggested-by: Junio C Hamano <gits...@pobox.com> Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- v2: no "ifdef TSAN"; moved check from strbuf_reset into strbuf_setlen strbuf.h | 5 - 1 file changed, 4 insertions(+), 1 de

[PATCH v2 2/4] pack-objects: take lock before accessing `remaining`

y again. We could tweak the contract so that the lock should be taken before calling find_deltas, but let's defer that until someone can actually show that "unlock+lock" has a measurable negative impact. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- v2: unchanged from v1

[PATCH v2 1/4] convert: always initialize attr_action in convert_attrs

no problem right now. But convert_attrs is obviously trying not to rely on such an implementation-detail of another component. Make the initialization of attr_action after the if-else. Remove the earlier assignments. Suggested-by: Torsten Bögershausen <tbo...@web.de> Signed-off-by: Martin Ågre

[PATCH v2 0/4] Some ThreadSanitizer-results

feel like I have any idea which of them is better. 2) hashmap_add, which I could try my hands on if Jeff doesn't beat me to it -- his proposed change should fix it and I doubt I could come up with anything "better", considering he knows the code. Martin Martin Ågren (4): conver

Re: [PATCH v2 1/3] Documentation/git-merge: explain --continue

On 21 August 2017 at 14:53, Michael J Gruber wrote: > Currently, 'git merge --continue' is mentioned but not explained. > > Explain it. > > Signed-off-by: Michael J Gruber > --- > Documentation/git-merge.txt | 5 - > 1 file changed, 4 insertions(+), 1

Re: "Your branch is up-to-date ..." is incorrect English grammar

The code base contains a few instances of "up-to-date" and "up to date". A tree wide sweep could be made to update user-visible strings in the code and in the documentation. Fixing source code comments seems like overkill. Could I count on you to review any changes I'd propose? (With respect to

Re: What's cooking in git.git (Aug 2017, #04; Fri, 18)

On 20 August 2017 at 11:40, Jeff King wrote: > -- >8 -- > From: =?UTF-8?q?Martin=20=C3=85gren?= > Subject: [PATCH] doc/interpret-trailers: fix "the this" typo > > Signed-off-by: Jeff King > --- > I put Martin as the author since he noticed

Re: [PATCH/RFC 0/5] Some ThreadSanitizer-results

On 20 August 2017 at 12:06, Jeff King <p...@peff.net> wrote: > On Tue, Aug 15, 2017 at 02:53:00PM +0200, Martin Ågren wrote: > >> I tried running the test suite on Git compiled with ThreadSanitizer >> (SANITIZE=thread). Maybe this series could be useful for someone else >

Re: [PATCH v4 2/3] builtin/branch: stop supporting the use of --set-upstream option

On 18 August 2017 at 18:30, Junio C Hamano <gits...@pobox.com> wrote: > Kaartic Sivaraam <kaarticsivaraam91...@gmail.com> writes: > >> On Friday 18 August 2017 01:25 AM, Junio C Hamano wrote: >>> Martin Ågren <martin.ag...@gmail.com> writes: >>> &

Re: [PATCH v4 2/3] builtin/branch: stop supporting the use of --set-upstream option

On 17 August 2017 at 04:54, Kaartic Sivaraam <kaarticsivaraam91...@gmail.com> wrote: > Helped-by: Martin Ågren <martin.ag...@gmail.com>, Junio C Hamano > <gits...@pobox.com> > Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com> I didn't expect a &

Re: [PATCH v4 5/8] interpret-trailers: add --parse convenience option

On 16 August 2017 at 10:20, Jeff King <p...@peff.net> wrote: > On Tue, Aug 15, 2017 at 01:26:53PM +0200, Martin Ågren wrote: > >> > This command reads some patches or commit messages from either the >> > - arguments or the standard input if no is specified. T

Re: tsan: t3008: hashmap_add touches size from multiple threads

On 15 August 2017 at 20:48, Stefan Beller wrote: /* total number of entries (0 means the hashmap is empty) */ - unsigned int size; + /* -1 means size is unknown for threading reasons */ + int size; >>> >>> This double-encodes the

Re: [PATCH 4/5] strbuf_reset: don't write to slopbuf with ThreadSanitizer

On 15 August 2017 at 20:43, Junio C Hamano <gits...@pobox.com> wrote: > Martin Ågren <martin.ag...@gmail.com> writes: > >> If two threads have one freshly initialized string buffer each and call >> strbuf_reset on them at roughly the same time, both th

Re: tsan: t5400: set_try_to_free_routine

On 15 August 2017 at 19:35, Stefan Beller <sbel...@google.com> wrote: > On Tue, Aug 15, 2017 at 5:53 AM, Martin Ågren <martin.ag...@gmail.com> wrote: >> Using SANITIZE=thread made t5400-send-pack.sh hit the potential race >> below. >> >> This is set_try

Re: tsan: t3008: hashmap_add touches size from multiple threads

On 15 August 2017 at 20:17, Stefan Beller <sbel...@google.com> wrote: > On Tue, Aug 15, 2017 at 10:59 AM, Jeff Hostetler <g...@jeffhostetler.com> > wrote: >> >> >> On 8/15/2017 8:53 AM, Martin Ågren wrote: >>> >>> Using SANITIZE

Re: [PATCH 1/5] convert: initialize attr_action in convert_attrs

On 15 August 2017 at 16:17, Torsten Bögershausen <tbo...@web.de> wrote: > On Tue, Aug 15, 2017 at 02:53:01PM +0200, Martin Ågren wrote: >> convert_attrs populates a struct conv_attrs. The field attr_action is >> not set in all code paths, but still one caller unconditional

[PATCH 3/5] Makefile: define GIT_THREAD_SANITIZER

and define GIT_THREAD_SANITIZER when we are compiling for tsan. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- Makefile | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Makefile b/Makefile index 461c845d3..e77a60d1f 100644 --- a/Makefile +++ b/Makefile @@ -1033,6 +1033,9 @@ BASIC_

[PATCH 4/5] strbuf_reset: don't write to slopbuf with ThreadSanitizer

n more bad if we stop covering it up in strbuf_reset.) Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- strbuf.h | 12 1 file changed, 12 insertions(+) diff --git a/strbuf.h b/strbuf.h index e705b94db..295654d39 100644 --- a/strbuf.h +++ b/strbuf.h @@ -153,7 +153,19 @@ st

[PATCH/RFC 0/5] Some ThreadSanitizer-results

of little to no (or even negative) value... I'll follow up with the two remaining issues that I found but which I do not try to address in this series. Martin Martin Ågren (5): convert: initialize attr_action in convert_attrs pack-objects: take lock before accessing `remaining` Makefi

[PATCH 1/5] convert: initialize attr_action in convert_attrs

that first assignment while we're here. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- I hit a warning about attr_action possibly being uninitialized when building with SANITIZE=thread. I guess it's some random interaction between code added by tsan, the optimizer (-O3) and the w

tsan: t5400: set_try_to_free_routine

Using SANITIZE=thread made t5400-send-pack.sh hit the potential race below. This is set_try_to_free_routine in wrapper.c. The race relates to the reading of the "old" value. The caller doesn't care about the "old" value, so this should be harmless right now. But it seems that using this mechanism

tsan: t3008: hashmap_add touches size from multiple threads

Using SANITIZE=thread made t3008-ls-files-lazy-init-name-hash.sh hit the potential race below. What seems to happen is, threaded_lazy_init_name_hash ends up using hashmap_add on the_index.dir_hash from two threads in a way that tsan considers racy. While the buckets each have their own mutex, the

[PATCH 5/5] ThreadSanitizer: add suppressions

quot; might not work. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- I am no memory-model expert. Maybe (aligned) stores and loads of int are not actually atomic on all the various hardware that Git wants to run on. Or maybe the compiler is allowed to compile them into 4 1-byte

[PATCH 2/5] pack-objects: take lock before accessing `remaining`

y again. We could tweak the contract so that the lock should be taken before calling find_deltas, but let's defer that until someone can actually show that "unlock+lock" has a measurable negative impact. Signed-off-by: Martin Ågren <martin.ag...@gmail.com> --- I don't think thi

Re: [PATCH v4 5/8] interpret-trailers: add --parse convenience option

On 15 August 2017 at 12:23, Jeff King wrote: > SYNOPSIS > > [verse] > -'git interpret-trailers' [--in-place] [--trim-empty] [(--trailer > [(=|:)])...] [...] > +'git interpret-trailers' [options] [(--trailer [(=|:)])...] > [...] > +'git interpret-trailers' [options]

Re: [PATCH v3 1/2 / RFC] builtin/branch: stop supporting the use of --set-upstream option

On 14 August 2017 at 10:54, Kaartic Sivaraam wrote: > The '--set-upstream' option of branch was deprecated in, > > b347d06bf branch: deprecate --set-upstream and show help if we > detect possible mistaken use (Thu, 30 Aug 2012 19:23:13 +0200) > > It was

Re: [PATCH 3/9] Convert unpack-objects to size_t

On 12 August 2017 at 10:47, Martin Koegler wrote: > From: Martin Koegler > > Signed-off-by: Martin Koegler > --- > builtin/unpack-objects.c | 20 ++-- > 1 file changed, 10 insertions(+), 10

Re: [PATCH 5/9] Convert various things to size_t

On 12 August 2017 at 10:47, Martin Koegler wrote: > From: Martin Koegler > > --- > bisect.c| 2 +- > blame.c | 2 +- > builtin/fmt-merge-msg.c | 2 +- > builtin/mktag.c | 2 +- > dir.c

Re: [PATCH v2 1/2 / RFC] builtin/branch: stop supporting the use of --set-upstream option

On 8 August 2017 at 19:11, Kaartic Sivaraam wrote: > The '--set-upstream' option of branch was deprecated in, > > b347d06bf branch: deprecate --set-upstream and show help if we > detect possible mistaken use (Thu, 30 Aug 2012 19:23:13 +0200) > > It was

Re: [PATCH 0/6] clean up parsing of maybe_bool

On 8 August 2017 at 19:04, Junio C Hamano <gits...@pobox.com> wrote: > Martin Ågren <martin.ag...@gmail.com> writes: > >> Thanks, both of you. I could wait a couple of days to see if there are >> other things to address, then send a v2 with a more aggressive p

<    1   2   3   4   5   6   >