| Message ID | pull.1771.v2.git.1723188292498.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a77554ea0972e4ec3518fb8fb0b2582c76cf3105 |
| Headers | show |
| Series | [v2] diff-tree: fix crash when used with --remerge-diff | expand |
"blanet via GitGitGadget" <gitgitgadget@gmail.com> writes: > Elijah Newren, the author of the remerge diff feature, notes that other > callers of `log-tree.c:log_tree_commit` (the only caller of > `log-tree.c:do_remerge_diff`) also exist, but: > > `builtin/am.c`: manually sets all flags; remerge_diff is not among them > `sequencer.c`: manually sets all flags; remerge_diff is not among them > > so `builtin/diff-tree.c` really is the only caller that was overlooked > when remerge-diff functionality was added. That is more than OK as a band-aid, and I'll take the patch as-is, but I have to wonder if we do even better in a future follow-up patch. Any time do_remerge_diff() is entered, we know that either the end user (from the command line) or the hard-coded caller (like am/sequencer cited above) wants us to do the remerge-diff, which in turn requires us to have the temporary object directory rotated into the status of the primary object store. And there is nothing in that object directory rotation code that requires caller-specific customization---it is the same "create remerge-diff directory as tmp-objdir, rotate it into the alt object store chain as the primary" regardless of the actual caller). So wouldn't it work well if we (1) at the beginning of do_remerge_diff(), only once for a rev_info structure: (1-a) lazily do the "object directory rotation" (1-b) set up an atexit handler to clear the temporary object store (2) remove all the "ah, we need to prepare and tear down the temporary object store for _this_ operation" we have sprinkled in different code paths (including the one added by the fix we are looking at). That way, we won't have to worry about adding future remerge_diff users, including existing hard-coded callers. ANyway, thanks for the fix. It is very pleasing to see contributors working well together.
Junio C Hamano <gitster@pobox.com> writes: > but I have to wonder if we do even better in a future follow-up > patch. "if we do" -> "if we can do". > So wouldn't it work well if we > > (1) at the beginning of do_remerge_diff(), only once for a rev_info > structure: > (1-a) lazily do the "object directory rotation" > (1-b) set up an atexit handler to clear the temporary object > store An atexit handler may not be enough, when a program wants to start creating a real object after we did a remerge-diff but before exiting. So we'd probably need to allow an explicit "ok, we are done" clean-up call for such a program, too. And the atexit handler can call the same clean-up function if the program hasn't called it explicitly. For logically read-only operations like diff-tree, they do not have to worry about rotating the real object store back to the primary status as soon as possible.
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c index 0d3c611aac0..b8df1d4b79b 100644 --- a/builtin/diff-tree.c +++ b/builtin/diff-tree.c @@ -8,6 +8,7 @@ #include "read-cache-ll.h" #include "repository.h" #include "revision.h" +#include "tmp-objdir.h" #include "tree.h" static struct rev_info log_tree_opt; @@ -166,6 +167,13 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix) opt->diffopt.rotate_to_strict = 1; + if (opt->remerge_diff) { + opt->remerge_objdir = tmp_objdir_create("remerge-diff"); + if (!opt->remerge_objdir) + die(_("unable to create temporary object directory")); + tmp_objdir_replace_primary_odb(opt->remerge_objdir, 1); + } + /* * NOTE! We expect "a..b" to expand to "^a b" but it is * perfectly valid for revision range parser to yield "b ^a", @@ -230,5 +238,10 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix) diff_free(&opt->diffopt); } + if (opt->remerge_diff) { + tmp_objdir_destroy(opt->remerge_objdir); + opt->remerge_objdir = NULL; + } + return diff_result_code(&opt->diffopt); } diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh index 07323ebafe0..ca8f999caba 100755 --- a/t/t4069-remerge-diff.sh +++ b/t/t4069-remerge-diff.sh @@ -110,6 +110,41 @@ test_expect_success 'can filter out additional headers with pickaxe' ' test_must_be_empty actual ' +test_expect_success 'remerge-diff also works for git-diff-tree' ' + # With a clean merge + git diff-tree -r -p --remerge-diff --no-commit-id bc_resolution >actual && + test_must_be_empty actual && + + # With both a resolved conflict and an unrelated change + cat <<-EOF >tmp && + diff --git a/numbers b/numbers + remerge CONFLICT (content): Merge conflict in numbers + index a1fb731..6875544 100644 + --- a/numbers + +++ b/numbers + @@ -1,13 +1,9 @@ + 1 + 2 + -<<<<<<< b0ed5cb (change_a) + -three + -======= + -tres + ->>>>>>> 6cd3f82 (change_b) + +drei + 4 + 5 + 6 + 7 + -eight + +acht + 9 + EOF + sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >expect && + git diff-tree -r -p --remerge-diff --no-commit-id ab_resolution >tmp && + sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >actual && + test_cmp expect actual +' + test_expect_success 'setup non-content conflicts' ' git switch --orphan base &&