Add `range-diff`, a `tbdiff` lookalike #1

dscho · 2018-06-05T14:49:24Z

The incredibly useful git-tbdiff tool to compare patch series (say, to see what changed between two iterations sent to the Git mailing list) is slightly less useful for this developer due to the fact that it requires the hungarian and numpy Python packages which are for some reason really hard to build in MSYS2. So hard that I even had to give up, because it was simply easier to re-implement the whole shebang as a builtin command.

The project at https://github.com/trast/tbdiff seems to be dormant, anyway. Funny (and true) story: I looked at the open Pull Requests to see how active that project is, only to find to my surprise that I had submitted one in August 2015, and that it was still unanswered let alone merged.

While at it, I forward-ported AEvar's patch to force --decorate=no because git -p tbdiff would fail otherwise.

Side note: I work on implementing range-diff not only to make life easier for reviewers who have to suffer through v2, v3, ... of my patch series, but also to verify my changes before submitting a new iteration. And also, maybe even more importantly, I plan to use it to verify my merging-rebases of Git for
Windows (for which I previously used to redirect the pre-rebase/post-rebase diffs vs upstream and then compare them using git diff --no-index). And of course any interested person can see what changes were necessary e.g. in the merging-rebase of Git for Windows onto v2.17.0 by running a command like:

        base=^{/Start.the.merging-rebase}
        tag=v2.17.0.windows.1
        pre=$tag$base^2
        git range-diff $pre$base..$pre $tag$base..$tag

The command uses what it calls the "dual color mode" (can be disabled via --no-dual-color) which helps identifying what actually changed: it prefixes lines with a - (and red background) that correspond to the first commit range, and with a + (and green background) that correspond to the second range. The rest of the lines will be colored according to the original diffs.

Changes since v4:

Fixed a typo in the commit message of "range-diff: add tests" that was introduced in v4.
White-space fixes.
Fixed the length of the first header underline in the man page.
Changed the preprocessor guard in linear-assignment.h to reflect the new name (instead of the old name, which was hungarian.h).
Likewise, changed the preprocessor guards in range-diff.h to hide the history of the thrice-renamed command.
Fixed indentation in the completion.
Instead of trying to paper over white-space error handling that does not apply to "diffs of diffs", dual color mode now simply disables all white-space warnings.
When showing the "single arg must be symmetric range" error message, git range-diff now also shows the usage.
Adjusted the commit message of "range-diff: adjust the output of the commit pairs" to avoid the surprise of the reviewer when onelines are printed all of a sudden, too.
"range-diff: adjust the output of the commit pairs" is now using a simpler way to print onelines.
We are now sandwiching the diff_opt_parse() loop between two parse_options(), to make sure that we caught all options, and that the -- separator is handled.
Adjusted the lookup_commit_reference() call to the newest master (it now takes a the_repository parameter).

Changes since v3:

The cover letter was adjusted to reflect the new reality (the command is called range-diff now, not branch-diff, and --dual-color is the default).
The documentation was adjusted a bit more in the patch that makes --dual-color the default.
Clarified the calculation of the cost matrix, as per Stefan Beller's request.
The man page now spells out that merge commits are ignored in the commit ranges (not merges per se).
The code in linear-assignment.c was adjusted to use the SWAP() macro.
The commit message of the patch introducing the first rudimentary implementation no longer talks about the "Hungarian" algorithm, but about the "linear assignment algorithm" instead.
A bogus indentation change was backed out from the patch introducing the first rudimentary implementation.
Instead of merely warning about missing .. in the 2-parameter invocation, we now exit with the error message.
The diff_opt_parse() function is allowed to return a value larger than 1, indicating that more than just one command-line parameter was parsed. We now advance by the indicated value instead of always advancing exactly 1 (which is still correct much of the time).
A lengthy if...else if...else if...else was simplified (from a logical point of view) by reordering it.
The unnecessarily static variable dashes was turned into a local variable of the caller.
The commit message talking about the new man page still referred to git branch --diff, which has been fixed.
A forgotten t7910 reference was changed to t3206.
An unbalanced double-tick was fixed in the man page.
Fixed grammar both of the commit message and the description of the --no-dual-color option.
To fix the build, a blank man page is now introduced together with the new range-diff command, even if it is populated for real only at a later patch (i.e. at the same time as before).
The headaches Junio fears would be incurred by that simple workaround to avoid bogus white-space error reporting are fended off: a more complex patch is now in place that adds (and uses) a new white-space flag. Sadly, as is all too common when Junio "encourages" me to replace a simple workaround by something "proper", it caused all kinds of headaches to get this right, so I am rather less certain that the "proper" fix will cause us less headaches than the simple workaround would have done. But whatever.
The dual color mode now also dims the changes that are exclusively in the first specified commit range, and uses bold face on the changes exclusively in the second one. This matches the intuition when using range-diff to compare an older iteration of a patch series to a newer one: the changes from the previous iteration that were replaced by new ones "fade", while the changes that replace them are "shiny new".

Changes since v2:

Right-aligned the patch numbers in the commit pairs.
Used ALLOC_ARRAY() in hungarian.c instead of xmalloc(sizeof()*size).
Changed compute_assignment()s return type from int to void, as it always succeeds.
Changed the Hungarian Algorithm to use an integer cost matrix.
Changed the --creation-weight option to --creation-factor where is an integer.
Retitled 1/19 and 2/19 to better conform with the current conventions, as pointed out (and suggested) by Junio.
Shut up Coverity, and at the same time avoided passing the unnecessary i and j parameters to output_pair_header().
Removed support for the --no-patches option: we inherit diff_options' support for -s already (and much more).
Removed the ugly _INV enum values, and introduced a beautiful GIT_COLOR_REVERSE instead. This way, whatever the user configured as color.diff.new (or .old) will be used in reverse in the dual color mode.
Instead of overriding the fragment header color, the dual color mode will now reverse the "outer" fragment headers, too.
Turned the stand-alone branch-diff command into the --diff option of git branch. Adjusted pretty much all commit messages to account for this. This change should no longer be visible: see below.
Pretty much re-wrote the completion, to support the new --diff mode of git-branch. See below: it was reverted for range-diff.
Renamed t7910 to t3206, to be closer to the git-branch tests.
Ensured that git_diff_ui_config() gets called, and therefore color.diff.* respected.
Avoided leaking four_spaces.
Fixed a declaration in a for (;;) statement (which Junio had as a fixup! that I almost missed).
Renamed branch --diff, which had been renamed from branch-diff (which was picked to avoid re-using tbdiff) to range-diff.
Renamed hungarian.c and its header to linear-assignment.c
Made --dual-color the default, and changed it to still auto-detect whether color should be used rather than forcing it

gitgitgadget · 2018-06-07T20:44:55Z

An error occurred while submitting:

Error: Branch f8c4e30c63bbfa7efec44ff0f6d0404326723d35 is not rebased to upstream/master

Change "fetch" to treat "+" in refspecs (aka --force) to mean we should clobber a local tag of the same name. This changes the long-standing behavior of "fetch" added in 853a369 ("[PATCH] Multi-head fetch.", 2005-08-20), before this change all tag fetches effectively had --force enabled. The original rationale in that change was: > Tags need not be pointing at commits so there is no way to > guarantee "fast-forward" anyway. That comment and the rest of the history of "fetch" shows that the "+" (--force) part of refpecs was only conceived for branch updates, while tags have accepted any changes from upstream unconditionally and clobbered the local tag object. Changing this behavior has been discussed as early as 2011[1]. I the current behavior doesn't make sense, it easily results in local tags accidentally being clobbered. Ideally we'd namespace our tags per-remote, but as with my 97716d2 ("fetch: add a --prune-tags option and fetch.pruneTags config", 2018-02-09) it's easier to work around the current implementation than to fix the root cause, so this implements suggestion #1 from [1], "fetch" now only clobbers the tag if either "+" is provided as part of the refspec, or if "--force" is provided on the command-line. This also makes it nicely symmetrical with how "tag" itself works. We'll now refuse to clobber any existing tags unless "--force" is supplied, whether that clobbering would happen by clobbering a local tag with "tag", or by fetching it from the remote with "fetch". It's still not at all nicely symmetrical with how "git push" works, as discussed in the updated pull-fetch-param.txt documentation, but this change brings them more into line with one another. I don't think there's any reason "fetch" couldn't fully converge with the behavior used by "push", but that's a topic for another change. One of the tests added in 31b808a ("clone --single: limit the fetch refspec to fetched branch", 2012-09-20) is being changed to use --force where a clone would clobber a tag. This changes nothing about the existing behavior of the test. 1. https://public-inbox.org/git/20111123221658.GA22313@sigill.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

dscho · 2018-07-03T11:14:32Z

/submit

gitgitgadget · 2018-07-03T11:16:49Z

An error occurred while submitting:

Error: git tag -F - -a pr-1/dscho/branch-diff-v3 4a68b95 failed: 128,
fatal: tag 'pr-1/dscho/branch-diff-v3' already exists

dscho · 2018-07-03T11:25:52Z

/submit

gitgitgadget · 2018-07-03T11:27:37Z

Submitted as pull.1.v3.git.gitgitgadget@gmail.com

dscho · 2018-07-21T22:02:25Z

/submit

gitgitgadget · 2018-07-21T22:06:32Z

Submitted as pull.1.v4.git.gitgitgadget@gmail.com

When performing multi-pack reuse, reuse_partial_packfile_from_bitmap() is responsible for generating an array of bitmapped_pack structs from which to perform reuse. In the multi-pack case, we loop over the MIDXs packs and copy the result of calling `nth_bitmapped_pack()` to construct the list of reusable paths. But we may also want to do pack-reuse over a single pack, either because we only had one pack to perform reuse over (in the case of single-pack bitmaps), or because we explicitly asked to do single pack reuse even with a MIDX[^1]. When this is the case, the array we generate of reusable packs contains only a single element, which is either (a) the pack attached to the single-pack bitmap, or (b) the MIDX's preferred pack. In 795006f (pack-bitmap: gracefully handle missing BTMP chunks, 2024-04-15), we refactored the reuse_partial_packfile_from_bitmap() function and stopped assigning the pack_int_id field when reusing only the MIDX's preferred pack. This results in an uninitialized read down in try_partial_reuse() like so: ==7474==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x55c5cd191dde in try_partial_reuse pack-bitmap.c:1887:8 #1 0x55c5cd191dde in reuse_partial_packfile_from_bitmap_1 pack-bitmap.c:2001:8 #2 0x55c5cd191dde in reuse_partial_packfile_from_bitmap pack-bitmap.c:2105:3 #3 0x55c5cce0bd0e in get_object_list_from_bitmap builtin/pack-objects.c:4043:3 #4 0x55c5cce0bd0e in get_object_list builtin/pack-objects.c:4156:27 #5 0x55c5cce0bd0e in cmd_pack_objects builtin/pack-objects.c:4596:3 #6 0x55c5ccc8fac8 in run_builtin git.c:474:11 which happens when try_partial_reuse() tries to call midx_pair_to_pack_pos() when it tries to reject cross-pack deltas. Avoid the uninitialized read by ensuring that the pack_int_id field is set in the single-pack reuse case by setting it to either the MIDX preferred pack's pack_int_id, or '0', in the case of single-pack bitmaps. In the latter case, we never read the pack_int_id field, so the choice of '0' is arbitrary. [^1]: This can happen for a couple of reasons, either because the repository is configured with 'pack.allowPackReuse=(true|single)', or because the MIDX was generated prior to the introduction of the BTMP chunk, which contains information necessary to perform multi-pack reuse. Reported-by: Kyle Lippincott <spectral@google.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Commit d6a8c58 (midx-write.c: support reading an existing MIDX with `packs_to_include`, 2024-05-29) changed the MIDX generation machinery to support reading from an existing MIDX when writing a new one. Unfortunately, the rest of the MIDX generation machinery is not prepared to deal with such a change. For instance, the function responsible for adding to the object ID fanout table from a MIDX source (midx_fanout_add_midx_fanout()) will gladly add objects from an existing MIDX for some fanout level regardless of whether or not those objects came from packs that are to be included in the subsequent MIDX write. This results in broken pseudo-pack object order (leading to incorrect object traversal results) and segmentation faults, like so (generated by running the added test prior to the changes in midx-write.c): #0 0x000055ee31393f47 in midx_pack_order (ctx=0x7ffdde205c70) at midx-write.c:590 #1 0x000055ee31395a69 in write_midx_internal (object_dir=0x55ee32570440 ".git/objects", packs_to_include=0x7ffdde205e20, packs_to_drop=0x0, preferred_pack_name=0x0, refs_snapshot=0x0, flags=15) at midx-write.c:1171 #2 0x000055ee31395f38 in write_midx_file_only (object_dir=0x55ee32570440 ".git/objects", packs_to_include=0x7ffdde205e20, preferred_pack_name=0x0, refs_snapshot=0x0, flags=15) at midx-write.c:1274 [...] In stack frame #0, the code on midx-write.c:590 is using the new pack ID corresponding to some object which was added from the existing MIDX. Importantly, the pack from which that object was selected in the existing MIDX does not appear in the new MIDX as it was excluded via `--stdin-packs`. In this instance, the pack in question had pack ID "1" in the existing MIDX, but since it was excluded from the new MIDX, we never filled in that entry in the pack_perm table, resulting in: (gdb) p *ctx->pack_perm@2 $1 = {0, 1515870810} Which is what causes the segfault above when we try and read: struct pack_info *pack = &ctx->info[ctx->pack_perm[i]]; if (pack->bitmap_pos == BITMAP_POS_UNKNOWN) pack->bitmap_pos = 0; Fundamentally, we should be able to read information from an existing MIDX when generating a new one. But in practice the midx-write.c code assumes that we won't run into issues like the above with incongruent pack IDs, and often makes those assumptions in extremely subtle and fragile ways. Instead, let's avoid reading from an existing MIDX altogether, and stick with the pre-d6a8c58675 implementation. Harden against any regressions in this area by adding a test which demonstrates these issues. Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

When performing multi-pack reuse, reuse_partial_packfile_from_bitmap() is responsible for generating an array of bitmapped_pack structs from which to perform reuse. In the multi-pack case, we loop over the MIDXs packs and copy the result of calling `nth_bitmapped_pack()` to construct the list of reusable paths. But we may also want to do pack-reuse over a single pack, either because we only had one pack to perform reuse over (in the case of single-pack bitmaps), or because we explicitly asked to do single pack reuse even with a MIDX[^1]. When this is the case, the array we generate of reusable packs contains only a single element, which is either (a) the pack attached to the single-pack bitmap, or (b) the MIDX's preferred pack. In 795006f (pack-bitmap: gracefully handle missing BTMP chunks, 2024-04-15), we refactored the reuse_partial_packfile_from_bitmap() function and stopped assigning the pack_int_id field when reusing only the MIDX's preferred pack. This results in an uninitialized read down in try_partial_reuse() like so: ==7474==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x55c5cd191dde in try_partial_reuse pack-bitmap.c:1887:8 #1 0x55c5cd191dde in reuse_partial_packfile_from_bitmap_1 pack-bitmap.c:2001:8 #2 0x55c5cd191dde in reuse_partial_packfile_from_bitmap pack-bitmap.c:2105:3 #3 0x55c5cce0bd0e in get_object_list_from_bitmap builtin/pack-objects.c:4043:3 #4 0x55c5cce0bd0e in get_object_list builtin/pack-objects.c:4156:27 #5 0x55c5cce0bd0e in cmd_pack_objects builtin/pack-objects.c:4596:3 #6 0x55c5ccc8fac8 in run_builtin git.c:474:11 which happens when try_partial_reuse() tries to call midx_pair_to_pack_pos() when it tries to reject cross-pack deltas. Avoid the uninitialized read by ensuring that the pack_int_id field is set in the single-pack reuse case by setting it to either the MIDX preferred pack's pack_int_id, or '0', in the case of single-pack bitmaps. In the latter case, we never read the pack_int_id field, so the choice of '0' is arbitrary. Guard against further regressions in this area by adding a test which ensures that we do not throw out deltas from the preferred pack as "cross-pack" due to an uninitialized pack_int_id. [^1]: This can happen for a couple of reasons, either because the repository is configured with 'pack.allowPackReuse=(true|single)', or because the MIDX was generated prior to the introduction of the BTMP chunk, which contains information necessary to perform multi-pack reuse. Reported-by: Kyle Lippincott <spectral@google.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Commit d6a8c58 (midx-write.c: support reading an existing MIDX with `packs_to_include`, 2024-05-29) changed the MIDX generation machinery to support reading from an existing MIDX when writing a new one. Unfortunately, the rest of the MIDX generation machinery is not prepared to deal with such a change. For instance, the function responsible for adding to the object ID fanout table from a MIDX source (midx_fanout_add_midx_fanout()) will gladly add objects from an existing MIDX for some fanout level regardless of whether or not those objects came from packs that are to be included in the subsequent MIDX write. This results in broken pseudo-pack object order (leading to incorrect object traversal results) and segmentation faults, like so (generated by running the added test prior to the changes in midx-write.c): #0 0x000055ee31393f47 in midx_pack_order (ctx=0x7ffdde205c70) at midx-write.c:590 #1 0x000055ee31395a69 in write_midx_internal (object_dir=0x55ee32570440 ".git/objects", packs_to_include=0x7ffdde205e20, packs_to_drop=0x0, preferred_pack_name=0x0, refs_snapshot=0x0, flags=15) at midx-write.c:1171 #2 0x000055ee31395f38 in write_midx_file_only (object_dir=0x55ee32570440 ".git/objects", packs_to_include=0x7ffdde205e20, preferred_pack_name=0x0, refs_snapshot=0x0, flags=15) at midx-write.c:1274 [...] In stack frame #0, the code on midx-write.c:590 is using the new pack ID corresponding to some object which was added from the existing MIDX. Importantly, the pack from which that object was selected in the existing MIDX does not appear in the new MIDX as it was excluded via `--stdin-packs`. In this instance, the pack in question had pack ID "1" in the existing MIDX, but since it was excluded from the new MIDX, we never filled in that entry in the pack_perm table, resulting in: (gdb) p *ctx->pack_perm@2 $1 = {0, 1515870810} Which is what causes the segfault above when we try and read: struct pack_info *pack = &ctx->info[ctx->pack_perm[i]]; if (pack->bitmap_pos == BITMAP_POS_UNKNOWN) pack->bitmap_pos = 0; Fundamentally, we should be able to read information from an existing MIDX when generating a new one. But in practice the midx-write.c code assumes that we won't run into issues like the above with incongruent pack IDs, and often makes those assumptions in extremely subtle and fragile ways. Instead, let's avoid reading from an existing MIDX altogether, and stick with the pre-d6a8c58675 implementation. Harden against any regressions in this area by adding a test which demonstrates these issues. Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

When performing multi-pack reuse, reuse_partial_packfile_from_bitmap() is responsible for generating an array of bitmapped_pack structs from which to perform reuse. In the multi-pack case, we loop over the MIDXs packs and copy the result of calling `nth_bitmapped_pack()` to construct the list of reusable paths. But we may also want to do pack-reuse over a single pack, either because we only had one pack to perform reuse over (in the case of single-pack bitmaps), or because we explicitly asked to do single pack reuse even with a MIDX[^1]. When this is the case, the array we generate of reusable packs contains only a single element, which is either (a) the pack attached to the single-pack bitmap, or (b) the MIDX's preferred pack. In 795006f (pack-bitmap: gracefully handle missing BTMP chunks, 2024-04-15), we refactored the reuse_partial_packfile_from_bitmap() function and stopped assigning the pack_int_id field when reusing only the MIDX's preferred pack. This results in an uninitialized read down in try_partial_reuse() like so: ==7474==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x55c5cd191dde in try_partial_reuse pack-bitmap.c:1887:8 #1 0x55c5cd191dde in reuse_partial_packfile_from_bitmap_1 pack-bitmap.c:2001:8 #2 0x55c5cd191dde in reuse_partial_packfile_from_bitmap pack-bitmap.c:2105:3 #3 0x55c5cce0bd0e in get_object_list_from_bitmap builtin/pack-objects.c:4043:3 #4 0x55c5cce0bd0e in get_object_list builtin/pack-objects.c:4156:27 #5 0x55c5cce0bd0e in cmd_pack_objects builtin/pack-objects.c:4596:3 #6 0x55c5ccc8fac8 in run_builtin git.c:474:11 which happens when try_partial_reuse() tries to call midx_pair_to_pack_pos() when it tries to reject cross-pack deltas. Avoid the uninitialized read by ensuring that the pack_int_id field is set in the single-pack reuse case by setting it to either the MIDX preferred pack's pack_int_id, or '-1', in the case of single-pack bitmaps. In the latter case, we never read the pack_int_id field, so the choice of '-1' is intentional as a "garbage in, garbage out" measure. Guard against further regressions in this area by adding a test which ensures that we do not throw out deltas from the preferred pack as "cross-pack" due to an uninitialized pack_int_id. [^1]: This can happen for a couple of reasons, either because the repository is configured with 'pack.allowPackReuse=(true|single)', or because the MIDX was generated prior to the introduction of the BTMP chunk, which contains information necessary to perform multi-pack reuse. Reported-by: Kyle Lippincott <spectral@google.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Memory sanitizer (msan) is detecting a use of an uninitialized variable (`size`) in `read_attr_from_index`: ==2268==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x5651f3416504 in read_attr_from_index git/attr.c:868:11 #1 0x5651f3415530 in read_attr git/attr.c #2 0x5651f3413d74 in bootstrap_attr_stack git/attr.c:968:6 #3 0x5651f3413d74 in prepare_attr_stack git/attr.c:1004:2 #4 0x5651f3413d74 in collect_some_attrs git/attr.c:1199:2 #5 0x5651f3413144 in git_check_attr git/attr.c:1345:2 #6 0x5651f34728da in convert_attrs git/convert.c:1320:2 #7 0x5651f3473425 in would_convert_to_git_filter_fd git/convert.c:1373:2 #8 0x5651f357a35e in index_fd git/object-file.c:2630:34 #9 0x5651f357aa15 in index_path git/object-file.c:2657:7 #10 0x5651f35db9d9 in add_to_index git/read-cache.c:766:7 #11 0x5651f35dc170 in add_file_to_index git/read-cache.c:799:9 #12 0x5651f321f9b2 in add_files git/builtin/add.c:346:7 #13 0x5651f321f9b2 in cmd_add git/builtin/add.c:565:18 #14 0x5651f321d327 in run_builtin git/git.c:474:11 #15 0x5651f321bc9e in handle_builtin git/git.c:729:3 #16 0x5651f321a792 in run_argv git/git.c:793:4 #17 0x5651f321a792 in cmd_main git/git.c:928:19 #18 0x5651f33dde1f in main git/common-main.c:62:11 The issue exists because `size` is an output parameter from `read_blob_data_from_index`, but it's only modified if `read_blob_data_from_index` returns non-NULL. The read of `size` when calling `read_attr_from_buf` unconditionally may read from an uninitialized value. `read_attr_from_buf` checks that `buf` is non-NULL before reading from `size`, but by then it's already too late: the uninitialized read will have happened already. Furthermore, there's no guarantee that the compiler won't reorder things so that it checks `size` before checking `!buf`. Make the call to `read_attr_from_buf` conditional on `buf` being non-NULL, ensuring that `size` is not read if it's never set. Signed-off-by: Kyle Lippincott <spectral@google.com>

Memory sanitizer (msan) is detecting a use of an uninitialized variable (`size`) in `read_attr_from_index`: ==2268==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x5651f3416504 in read_attr_from_index git/attr.c:868:11 #1 0x5651f3415530 in read_attr git/attr.c #2 0x5651f3413d74 in bootstrap_attr_stack git/attr.c:968:6 #3 0x5651f3413d74 in prepare_attr_stack git/attr.c:1004:2 #4 0x5651f3413d74 in collect_some_attrs git/attr.c:1199:2 #5 0x5651f3413144 in git_check_attr git/attr.c:1345:2 #6 0x5651f34728da in convert_attrs git/convert.c:1320:2 #7 0x5651f3473425 in would_convert_to_git_filter_fd git/convert.c:1373:2 #8 0x5651f357a35e in index_fd git/object-file.c:2630:34 #9 0x5651f357aa15 in index_path git/object-file.c:2657:7 #10 0x5651f35db9d9 in add_to_index git/read-cache.c:766:7 #11 0x5651f35dc170 in add_file_to_index git/read-cache.c:799:9 #12 0x5651f321f9b2 in add_files git/builtin/add.c:346:7 #13 0x5651f321f9b2 in cmd_add git/builtin/add.c:565:18 #14 0x5651f321d327 in run_builtin git/git.c:474:11 #15 0x5651f321bc9e in handle_builtin git/git.c:729:3 #16 0x5651f321a792 in run_argv git/git.c:793:4 #17 0x5651f321a792 in cmd_main git/git.c:928:19 #18 0x5651f33dde1f in main git/common-main.c:62:11 The issue exists because `size` is an output parameter from `read_blob_data_from_index`, but it's only modified if `read_blob_data_from_index` returns non-NULL. The read of `size` when calling `read_attr_from_buf` unconditionally may read from an uninitialized value. `read_attr_from_buf` checks that `buf` is non-NULL before reading from `size`, but by then it's already too late: the uninitialized read will have happened already. Furthermore, there's no guarantee that the compiler won't reorder things so that it checks `size` before checking `!buf`. Make the call to `read_attr_from_buf` conditional on `buf` being non-NULL, ensuring that `size` is not read if it's never set. Signed-off-by: Kyle Lippincott <spectral@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Signed-off-by: Junio C Hamano <gitster@pobox.com>

When using `--fixed-value` with a key whose value is left empty (implied as being "true"), 'git config' may crash when invoked like either of: $ git config set --file=config --value=value --fixed-value \ section.key pattern $ git config --file=config --fixed-value section.key value pattern The original bugreport[1] bisects to 00bbdde (builtin/config: introduce "set" subcommand, 2024-05-06), which is a red-herring, since the original bugreport uses the new 'git config set' invocation. The behavior likely bisects back to c90702a (config: plumb --fixed-value into config API, 2020-11-25), which introduces the new --fixed-value option in the first place. Looking at the relevant frame from a failed process's coredump, the crash appears in config.c::matches() like so: (gdb) up #1 0x000055b3e8b06022 in matches (key=0x55b3ea894360 "section.key", value=0x0, store=0x7ffe99076eb0) at config.c:2884 2884 return !strcmp(store->fixed_value, value); where we are trying to compare the `--fixed-value` argument to `value`, which is NULL. Avoid attempting to match `--fixed-value` for configuration keys with no explicit value. A future patch could consider the empty value to mean "true", "yes", "on", etc. when invoked with `--type=bool`, but let's punt on that for now in the name of avoiding the segfault. [1]: https://lore.kernel.org/git/CANrWfmTek1xErBLrnoyhHN+gWU+rw14y6SQ+abZyzGoaBjmiKA@mail.gmail.com/ Reported-by: Han Jiang <jhcarl0814@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

It was recently reported that concurrent reads and writes may cause the reftable backend to segfault. The root cause of this is that we do not properly keep track of reftable readers across reloads. Suppose that you have a reftable iterator and then decide to reload the stack while iterating through the iterator. When the stack has been rewritten since we have created the iterator, then we would end up discarding a subset of readers that may still be in use by the iterator. The consequence is that we now try to reference deallocated memory, which of course segfaults. One way to trigger this is in t5616, where some background maintenance jobs have been leaking from one test into another. This leads to stack traces like the following one: + git -c protocol.version=0 -C pc1 fetch --filter=blob:limit=29999 --refetch origin AddressSanitizer:DEADLYSIGNAL ================================================================= ==657994==ERROR: AddressSanitizer: SEGV on unknown address 0x7fa0f0ec6089 (pc 0x55f23e52ddf9 bp 0x7ffe7bfa1700 sp 0x7ffe7bfa1700 T0) ==657994==The signal is caused by a READ memory access. #0 0x55f23e52ddf9 in get_var_int reftable/record.c:29 #1 0x55f23e53295e in reftable_decode_keylen reftable/record.c:170 #2 0x55f23e532cc0 in reftable_decode_key reftable/record.c:194 #3 0x55f23e54e72e in block_iter_next reftable/block.c:398 #4 0x55f23e5573dc in table_iter_next_in_block reftable/reader.c:240 #5 0x55f23e5573dc in table_iter_next reftable/reader.c:355 #6 0x55f23e5573dc in table_iter_next reftable/reader.c:339 #7 0x55f23e551283 in merged_iter_advance_subiter reftable/merged.c:69 #8 0x55f23e55169e in merged_iter_next_entry reftable/merged.c:123 #9 0x55f23e55169e in merged_iter_next_void reftable/merged.c:172 #10 0x55f23e537625 in reftable_iterator_next_ref reftable/generic.c:175 #11 0x55f23e2cf9c6 in reftable_ref_iterator_advance refs/reftable-backend.c:464 #12 0x55f23e2d996e in ref_iterator_advance refs/iterator.c:13 #13 0x55f23e2d996e in do_for_each_ref_iterator refs/iterator.c:452 #14 0x55f23dca6767 in get_ref_map builtin/fetch.c:623 #15 0x55f23dca6767 in do_fetch builtin/fetch.c:1659 #16 0x55f23dca6767 in fetch_one builtin/fetch.c:2133 #17 0x55f23dca6767 in cmd_fetch builtin/fetch.c:2432 #18 0x55f23dba7764 in run_builtin git.c:484 #19 0x55f23dba7764 in handle_builtin git.c:741 #20 0x55f23dbab61e in run_argv git.c:805 #21 0x55f23dbab61e in cmd_main git.c:1000 #22 0x55f23dba4781 in main common-main.c:64 #23 0x7fa0f063fc89 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 #24 0x7fa0f063fd44 in __libc_start_main_impl ../csu/libc-start.c:360 #25 0x55f23dba6ad0 in _start (git+0xadfad0) (BuildId: 803b2b7f59beb03d7849fb8294a8e2145dd4aa27) While it is somewhat awkward that the maintenance processes survive tests in the first place, it is totally expected that reftables should work alright with concurrent writers. Seemingly they don't. The only underlying resource that we need to care about in this context is the reftable reader, which is responsible for reading a single table from disk. These readers get discarded immediately (unless reused) when calling `reftable_stack_reload()`, which is wrong. We can only close them once we know that there are no iterators using them anymore. Prepare for a fix by converting the reftable readers to be refcounted. Reported-by: Jeff King <peff@peff.net> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

The incremental MIDX bitmap work was done prior to 9d4855e (midx-write: fix leaking buffer, 2024-09-30), and causes test failures in t5334 in a post-9d4855eef3 world. The leak looks like: Direct leak of 264 byte(s) in 1 object(s) allocated from: #0 0x7f6bcd87eaca in calloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:90 #1 0x55ad1428e8a4 in xcalloc wrapper.c:151 #2 0x55ad14199e16 in prepare_midx_bitmap_git pack-bitmap.c:742 #3 0x55ad14199447 in open_midx_bitmap_1 pack-bitmap.c:507 #4 0x55ad14199cca in open_midx_bitmap pack-bitmap.c:704 #5 0x55ad14199d44 in open_bitmap pack-bitmap.c:717 #6 0x55ad14199dc2 in prepare_bitmap_git pack-bitmap.c:733 #7 0x55ad1419e496 in test_bitmap_walk pack-bitmap.c:2698 #8 0x55ad14047b0b in cmd_rev_list builtin/rev-list.c:629 #9 0x55ad13f71cd6 in run_builtin git.c:487 #10 0x55ad13f72132 in handle_builtin git.c:756 #11 0x55ad13f72380 in run_argv git.c:826 #12 0x55ad13f728f4 in cmd_main git.c:961 #13 0x55ad1407d3ae in main common-main.c:64 #14 0x7f6bcd5f0c89 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 #15 0x7f6bcd5f0d44 in __libc_start_main_impl ../csu/libc-start.c:360 #16 0x55ad13f6ff90 in _start (git+0x1ef90) (BuildId: 3e63cdd415f1d185b21da3035cb48332510dddce) , and is a result of us not freeing the resources corresponding to the bitmap's base layer, if one was present. Rectify that leak by calling the newly-introduced free_bitmap_index() function on the base layer to ensure that its resources are also freed. Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

This one is a little bit more curious. In t6112, we have a test that exercises the `git rev-list --filter` option with invalid filters. We execute git-rev-list(1) via `test_must_fail`, which means that we check for leaks even though Git exits with an error code. This causes the following leak: Direct leak of 27 byte(s) in 1 object(s) allocated from: #0 0x5555555e6946 in realloc.part.0 lsan_interceptors.cpp.o #1 0x5555558fb4b6 in xrealloc wrapper.c:137:8 #2 0x5555558b6e06 in strbuf_grow strbuf.c:112:2 #3 0x5555558b7550 in strbuf_add strbuf.c:311:2 #4 0x5555557c1a88 in strbuf_addstr strbuf.h:310:2 #5 0x5555557c1d4c in parse_list_objects_filter list-objects-filter-options.c:261:3 #6 0x555555885ead in handle_revision_pseudo_opt revision.c:2899:3 #7 0x555555884e20 in setup_revisions revision.c:3014:11 #8 0x5555556c4b42 in cmd_rev_list builtin/rev-list.c:588:9 #9 0x5555555ec5e3 in run_builtin git.c:483:11 #10 0x5555555eb1e4 in handle_builtin git.c:749:13 #11 0x5555555ec001 in run_argv git.c:819:4 #12 0x5555555eaf94 in cmd_main git.c:954:19 #13 0x5555556fd569 in main common-main.c:64:11 #14 0x7ffff7ca714d in __libc_start_call_main (.../lib/libc.so.6+0x2a14d) #15 0x7ffff7ca7208 in __libc_start_main@GLIBC_2.2.5 (.../libc.so.6+0x2a208) #16 0x5555555ad064 in _start (git+0x59064) This leak is valid, as we call `die()` and do not clean up the memory at all. But what's curious is that this is the only leak reported, because we don't clean up any other allocated memory, either, and I have no idea why the leak sanitizer treats this buffer specially. In any case, we can work around the leak by shuffling things around a bit. Instead of calling `gently_parse_list_objects_filter()` and dying after we have modified the filter spec, we simply do so beforehand. Like this we don't allocate the buffer in the error case, which makes the reported leak go away. It's not pretty, but it manages to make t6112 leak free. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Taylor Blau <me@ttaylorr.com>

This one is a little bit more curious. In t6112, we have a test that exercises the `git rev-list --filter` option with invalid filters. We execute git-rev-list(1) via `test_must_fail`, which means that we check for leaks even though Git exits with an error code. This causes the following leak: Direct leak of 27 byte(s) in 1 object(s) allocated from: #0 0x5555555e6946 in realloc.part.0 lsan_interceptors.cpp.o #1 0x5555558fb4b6 in xrealloc wrapper.c:137:8 #2 0x5555558b6e06 in strbuf_grow strbuf.c:112:2 #3 0x5555558b7550 in strbuf_add strbuf.c:311:2 #4 0x5555557c1a88 in strbuf_addstr strbuf.h:310:2 #5 0x5555557c1d4c in parse_list_objects_filter list-objects-filter-options.c:261:3 #6 0x555555885ead in handle_revision_pseudo_opt revision.c:2899:3 #7 0x555555884e20 in setup_revisions revision.c:3014:11 #8 0x5555556c4b42 in cmd_rev_list builtin/rev-list.c:588:9 #9 0x5555555ec5e3 in run_builtin git.c:483:11 #10 0x5555555eb1e4 in handle_builtin git.c:749:13 #11 0x5555555ec001 in run_argv git.c:819:4 #12 0x5555555eaf94 in cmd_main git.c:954:19 #13 0x5555556fd569 in main common-main.c:64:11 #14 0x7ffff7ca714d in __libc_start_call_main (.../lib/libc.so.6+0x2a14d) #15 0x7ffff7ca7208 in __libc_start_main@GLIBC_2.2.5 (.../libc.so.6+0x2a208) #16 0x5555555ad064 in _start (git+0x59064) This leak is valid, as we call `die()` and do not clean up the memory at all. But what's curious is that this is the only leak reported, because we don't clean up any other allocated memory, either, and I have no idea why the leak sanitizer treats this buffer specially. In any case, we can work around the leak by shuffling things around a bit. Instead of calling `gently_parse_list_objects_filter()` and dying after we have modified the filter spec, we simply do so beforehand. Like this we don't allocate the buffer in the error case, which makes the reported leak go away. It's not pretty, but it manages to make t6112 leak free. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

When running t5601 with the leak checker enabled we can see a hang in our CI systems. This hang seems to be system-specific, as I cannot reproduce it on my own machine. As it turns out, the issue is in those testcases that exercise cloning of `~repo`-style paths. All of the testcases that hang eventually end up interpreting "repo" as the username and will call getpwnam(3p) with that username. That should of course be fine, and getpwnam(3p) should just return an error. But instead, the leak sanitizer seems to be recursing while handling a call to `free()` in the NSS modules: #0 0x00007ffff7fd98d5 in _dl_update_slotinfo (req_modid=1, new_gen=2) at ../elf/dl-tls.c:720 #1 0x00007ffff7fd9ac4 in update_get_addr (ti=0x7ffff7a91d80, gen=<optimized out>) at ../elf/dl-tls.c:916 #2 0x00007ffff7fdc85c in __tls_get_addr () at ../sysdeps/x86_64/tls_get_addr.S:55 #3 0x00007ffff7a27e04 in __lsan::GetAllocatorCache () at ../../../../src/libsanitizer/lsan/lsan_linux.cpp:27 #4 0x00007ffff7a2b33a in __lsan::Deallocate (p=0x0) at ../../../../src/libsanitizer/lsan/lsan_allocator.cpp:127 #5 __lsan::lsan_free (p=0x0) at ../../../../src/libsanitizer/lsan/lsan_allocator.cpp:220 ... #261505 0x00007ffff7fd99f2 in free (ptr=<optimized out>) at ../include/rtld-malloc.h:50 #261506 _dl_update_slotinfo (req_modid=1, new_gen=2) at ../elf/dl-tls.c:822 #261507 0x00007ffff7fd9ac4 in update_get_addr (ti=0x7ffff7a91d80, gen=<optimized out>) at ../elf/dl-tls.c:916 #261508 0x00007ffff7fdc85c in __tls_get_addr () at ../sysdeps/x86_64/tls_get_addr.S:55 #261509 0x00007ffff7a27e04 in __lsan::GetAllocatorCache () at ../../../../src/libsanitizer/lsan/lsan_linux.cpp:27 #261510 0x00007ffff7a2b33a in __lsan::Deallocate (p=0x5020000001e0) at ../../../../src/libsanitizer/lsan/lsan_allocator.cpp:127 #261511 __lsan::lsan_free (p=0x5020000001e0) at ../../../../src/libsanitizer/lsan/lsan_allocator.cpp:220 #261512 0x00007ffff793da25 in module_load (module=0x515000000280) at ./nss/nss_module.c:188 #261513 0x00007ffff793dee5 in __nss_module_load (module=0x515000000280) at ./nss/nss_module.c:302 #261514 __nss_module_get_function (module=0x515000000280, name=name@entry=0x7ffff79b9128 "getpwnam_r") at ./nss/nss_module.c:328 #261515 0x00007ffff793e741 in __GI___nss_lookup_function (fct_name=<optimized out>, ni=<optimized out>) at ./nss/nsswitch.c:137 #261516 __GI___nss_next2 (ni=ni@entry=0x7fffffffa458, fct_name=fct_name@entry=0x7ffff79b9128 "getpwnam_r", fct2_name=fct2_name@entry=0x0, fctp=fctp@entry=0x7fffffffa460, status=status@entry=0, all_values=all_values@entry=0) at ./nss/nsswitch.c:120 #261517 0x00007ffff794c6a7 in __getpwnam_r (name=name@entry=0x501000000060 "repo", resbuf=resbuf@entry=0x7ffff79fb320 <resbuf>, buffer=<optimized out>, buflen=buflen@entry=1024, result=result@entry=0x7fffffffa4b0) at ../nss/getXXbyYY_r.c:343 #261518 0x00007ffff794c4d8 in getpwnam (name=0x501000000060 "repo") at ../nss/getXXbyYY.c:140 #261519 0x00005555557e37ff in getpw_str (username=0x5020000001a1 "repo", len=4) at path.c:613 #261520 0x00005555557e3937 in interpolate_path (path=0x5020000001a0 "~repo", real_home=0) at path.c:654 #261521 0x00005555557e3aea in enter_repo (path=0x501000000040 "~repo", strict=0) at path.c:718 #261522 0x000055555568f0ba in cmd_upload_pack (argc=1, argv=0x502000000100, prefix=0x0, repo=0x0) at builtin/upload-pack.c:57 #261523 0x0000555555575ba8 in run_builtin (p=0x555555a20c98 <commands+3192>, argc=2, argv=0x502000000100, repo=0x555555a53b20 <the_repo>) at git.c:481 #261524 0x0000555555576067 in handle_builtin (args=0x7fffffffaab0) at git.c:742 #261525 0x000055555557678d in cmd_main (argc=2, argv=0x7fffffffac58) at git.c:912 #261526 0x00005555556963cd in main (argc=2, argv=0x7fffffffac58) at common-main.c:64 Note that this stack is more than 260000 function calls deep. Run under the debugger this will eventually segfault, but in our CI systems it seems like this just hangs forever. I assume that this is a bug either in the leak sanitizer or in glibc, as I cannot reproduce it on my machine. In any case, let's work around the bug for now by marking those tests with the "!SANITIZE_LEAK" prereq. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

gitgitgadget deleted a comment from gitgitgadget bot Jun 5, 2018

gitgitgadget deleted a comment from gitgitgadget bot Jun 7, 2018

dscho force-pushed the branch-diff branch from f8c4e30 to 1304201 Compare June 7, 2018 20:58

gitgitgadget deleted a comment from gitgitgadget bot Jun 8, 2018

dscho force-pushed the branch-diff branch 2 times, most recently from 79ce337 to 4a68b95 Compare June 30, 2018 21:36

dscho force-pushed the branch-diff branch 3 times, most recently from ebf3fea to d4e27c6 Compare July 9, 2018 20:39

dscho force-pushed the branch-diff branch 4 times, most recently from 5a2cf0c to d8498fb Compare July 21, 2018 21:40

stefanbeller mentioned this pull request Jul 23, 2018

Cover letters are hard to read on the mailing list (break lines?) gitgitgadget/gitgitgadget#26

Closed

gitgitgadget bot pushed a commit that referenced this pull request Feb 3, 2024

What's cooking (2024/02 #1)

00e4c21

gitgitgadget bot pushed a commit that referenced this pull request Mar 4, 2024

What's cooking (2024/03 #1)

1b60a6f

gitgitgadget bot pushed a commit that referenced this pull request Apr 1, 2024

What's cooking (2024/04 #1)

4b384fc

gitgitgadget bot pushed a commit that referenced this pull request May 1, 2024

What's cooking (2024/05 #1)

a95c444

gitgitgadget bot pushed a commit that referenced this pull request Jun 4, 2024

What's cooking (2024/06 #1)

c14ca44

gitgitgadget bot pushed a commit that referenced this pull request Jul 2, 2024

What's cooking (2024/07 #1)

22684c2

gitgitgadget bot pushed a commit that referenced this pull request Jul 15, 2024

Post 2.46-rc0 batch #1

ad850ef

Signed-off-by: Junio C Hamano <gitster@pobox.com>

gitgitgadget bot pushed a commit that referenced this pull request Aug 5, 2024

What's cooking (2024/08 #1)

5d775b5

gitgitgadget bot pushed a commit that referenced this pull request Sep 3, 2024

What's cooking (2024/09 #1)

73168e0

gitgitgadget bot pushed a commit that referenced this pull request Oct 1, 2024

What's cooking (2024/10 #1)

36ce426

gitgitgadget bot pushed a commit that referenced this pull request Nov 1, 2024

What's cooking (2024/11 #1)

16a7369

gitgitgadget bot pushed a commit that referenced this pull request Dec 3, 2024

What's cooking (2024/12 #1)

ce1cda5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `range-diff`, a `tbdiff` lookalike #1

Add `range-diff`, a `tbdiff` lookalike #1

dscho commented Jun 5, 2018 •

edited

Loading

gitgitgadget bot commented Jun 7, 2018

dscho commented Jul 3, 2018

gitgitgadget bot commented Jul 3, 2018

dscho commented Jul 3, 2018

gitgitgadget bot commented Jul 3, 2018

dscho commented Jul 21, 2018

gitgitgadget bot commented Jul 21, 2018

Add range-diff, a tbdiff lookalike #1

Add range-diff, a tbdiff lookalike #1

Conversation

dscho commented Jun 5, 2018 • edited Loading

gitgitgadget bot commented Jun 7, 2018

dscho commented Jul 3, 2018

gitgitgadget bot commented Jul 3, 2018

dscho commented Jul 3, 2018

gitgitgadget bot commented Jul 3, 2018

dscho commented Jul 21, 2018

gitgitgadget bot commented Jul 21, 2018

Add `range-diff`, a `tbdiff` lookalike #1

Add `range-diff`, a `tbdiff` lookalike #1

dscho commented Jun 5, 2018 •

edited

Loading