diff-tree: fix crash when used with --remerge-diff

PATCH Superseded blanet via GitGitGadget 4 participants 7 messages Aug 8, 2024 – Aug 9, 2024

lore

Share & tools

lore

blanet via GitGitGadget

author

2024-08-08 13:20 UTC #0 lore raw

PATCH 1/1 diff-tree: fix crash when used with --remerge-diff

From: Xing Xin <[email protected]>

When using "git-diff-tree" to get the tree diff for merge commits with the diff format set to remerge, a bug is triggered as shown below:

    $ git diff-tree -r --remerge-diff 363337e6eb
    363337e6eb812d0c0d785ed4261544f35559ff8b
    BUG: log-tree.c:1006: did a remerge diff without remerge_objdir?!?

This bug is reported by log-tree.c:do_remerge_diff, where a bug check added in commit 7b90ab467a (log: clean unneeded objects during log --remerge-diff, 2022-02-02) detects the absence of remerge_objdir when attempting to clean up temporary objects generated during the remerge process.

After some further digging, I find that the remerge-related diff options were introduced in db757e8b8d (show, log: provide a --remerge-diff capability, 2022-02-02), which also affect the setup of rev_info for "git-diff-tree", but were not accounted for in the original implementation (inferred from the commit message).

This commit fixes the bug by adding initialization logic for remerge_objdir in builtin/diff-tree.c, mirroring the logic in builtin/log.c:cmd_log_walk_no_free. A final cleanup for remerge_objdir is also included.

Signed-off-by Xing Xin <[email protected]>

---
diff-tree: fix crash when used with --remerge-diff

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1771%2Fblanet%2Fxx%2Ffix-diff-tree-crash-on-remerge-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1771/blanet/xx/fix-diff-tree-crash-on-remerge-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1771

 builtin/diff-tree.c     | 13 +++++++++++++
 t/t4069-remerge-diff.sh | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

builtin/diff-tree.c +13 -0

index 0d3c611aac0..813be486dad 100644
@@ -9,6 +9,7 @@
 #include "repository.h"
 #include "revision.h"
 #include "tree.h"
+#include "tmp-objdir.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);
 }

t/t4069-remerge-diff.sh +35 -0

index 07323ebafe0..ca8f999caba 100755
@@ -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 &&

base-commit: 406f326d271e0bacecdb00425422c5fa3f314930

Signature · 1 line

gitgitgadget

Elijah Newren

2024-08-08 16:04 UTC #1 lore raw

Quoted 9 lines · blanet via GitGitGadget

From: Xing Xin <[email protected]>
When using "git-diff-tree" to get the tree diff for merge commits with the diff format set to remerge, a bug is triggered as shown below:
    $ git diff-tree -r --remerge-diff 363337e6eb
    363337e6eb812d0c0d785ed4261544f35559ff8b
    BUG: log-tree.c:1006: did a remerge diff without remerge_objdir?!?

Wow, this bug is around for 2.5 years, and then we both independently notice and fix it within 3 weeks of each other:
https://github.com/git/git/commit/e5890667c7598e813edee0ac4e76d6e3cdd525ec

My patch is incomplete as it's missing a testcase, and you submitted first, so let's stick with your fix, though.

Quoted 12 lines

This bug is reported by log-tree.c:do_remerge_diff, where a bug check added in commit 7b90ab467a (log: clean unneeded objects during log --remerge-diff, 2022-02-02) detects the absence of remerge_objdir when attempting to clean up temporary objects generated during the remerge process.
After some further digging, I find that the remerge-related diff options were introduced in db757e8b8d (show, log: provide a --remerge-diff capability, 2022-02-02), which also affect the setup of rev_info for "git-diff-tree", but were not accounted for in the original implementation (inferred from the commit message).

Quoted 4 lines

This commit fixes the bug by adding initialization logic for remerge_objdir in builtin/diff-tree.c, mirroring the logic in builtin/log.c:cmd_log_walk_no_free. A final cleanup for remerge_objdir is also included.

The commit message from my patch also included an explanation for why diff-tree was the only caller that was missing the necessary logic (see the last paragraph, which kind of references the one before it as well).

Quoted 17 lines

Signed-off-by Xing Xin <[email protected]>
---
diff-tree: fix crash when used with --remerge-diff
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1771%2Fblanet%2Fxx%2Ffix-diff-tree-crash-on-remerge-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1771/blanet/xx/fix-diff-tree-crash-on-remerge-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1771
 builtin/diff-tree.c     | 13 +++++++++++++
 t/t4069-remerge-diff.sh | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)
builtin/diff-tree.c +0 -0
index 0d3c611aac0..813be486dad 100644

builtin/diff-tree.c line 12

Quoted 4 lines

builtin/diff-tree.c +1 -0

@@ -9,6 +9,7 @@
 #include "repository.h"
 #include "revision.h"
 #include "tree.h"
+#include "tmp-objdir.h"

The includes other than this one are in alphabetical order; can you move this a line before?

Also, as an aside, folks in this project often just put includes at the end, but I think it's a bad practice. Whenever someone needs to backport fixes or merge separate patch topics into seen/next/etc. or even merge not-yet-upstream topics with newer upstream versions, this practice increases the odds of unnecessary conflicts. And it makes it harder for the next person who comes along to spot whether a header is already included (and sometimes leaves us including headers twice). While each case is a small amount of toil so we tend to overlook it, it's totally unnecessary toil in many cases. Putting includes in alphabetical order (other than the one include required to be first, git-compat-util.h or its documented stand-ins) can often remove this unnecessary toil. Anyway, thanks for letting me vent. :-)

Quoted 17 lines

builtin/diff-tree.c +7 -0

 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",

builtin/diff-tree.c int cmd_diff_tree(int argc, const char **argv, const char *prefix) · line 247

Quoted 10 lines

builtin/diff-tree.c +5 -0

@@ -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);
 }

Your fix exactly matches mine, other than the header include location.

Quoted 35 lines

t/t4069-remerge-diff.sh +27 -0

index 07323ebafe0..ca8f999caba 100755
@@ -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

t/t4069-remerge-diff.sh test_expect_success 'can filter out additional headers with pickaxe' ' · line 149

Quoted 10 lines

t/t4069-remerge-diff.sh +8 -0

+        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 &&

Test looks good too.

I'll be happy to add my Reviewed-by if you fix the header include order.

Junio C Hamano

2024-08-08 17:24 UTC #2 lore raw

Quoted 4 lines · Elijah Newren

The commit message from my patch also included an explanation for why diff-tree was the only caller that was missing the necessary logic (see the last paragraph, which kind of references the one before it as well).

... which we may want to resurrect.

Quoted 3 lines

Test looks good too.
I'll be happy to add my Reviewed-by if you fix the header include order.

Thanks for a review.

Xing Xin

2024-08-09 07:25 UTC #3 lore raw

In reply to Elijah Newren

Quoted 12 lines · Elijah Newren

Quoted 9 lines · blanet via GitGitGadget
From: Xing Xin <[email protected]>
When using "git-diff-tree" to get the tree diff for merge commits with the diff format set to remerge, a bug is triggered as shown below:
    $ git diff-tree -r --remerge-diff 363337e6eb
    363337e6eb812d0c0d785ed4261544f35559ff8b
    BUG: log-tree.c:1006: did a remerge diff without remerge_objdir?!?

Quoted 6 lines

Wow, this bug is around for 2.5 years, and then we both independently notice and fix it within 3 weeks of each other:
https://github.com/git/git/commit/e5890667c7598e813edee0ac4e76d6e3cdd525ec
My patch is incomplete as it's missing a testcase, and you submitted first, so let's stick with your fix, though.

Wow, such an interesting coincidence! And thanks for your quick reply!

Quoted 17 lines

Quoted 12 lines
This bug is reported by log-tree.c:do_remerge_diff, where a bug check added in commit 7b90ab467a (log: clean unneeded objects during log --remerge-diff, 2022-02-02) detects the absence of remerge_objdir when attempting to clean up temporary objects generated during the remerge process.
After some further digging, I find that the remerge-related diff options were introduced in db757e8b8d (show, log: provide a --remerge-diff capability, 2022-02-02), which also affect the setup of rev_info for "git-diff-tree", but were not accounted for in the original implementation (inferred from the commit message).
Quoted 4 lines
This commit fixes the bug by adding initialization logic for remerge_objdir in builtin/diff-tree.c, mirroring the logic in builtin/log.c:cmd_log_walk_no_free. A final cleanup for remerge_objdir is also included.

Quoted 4 lines

The commit message from my patch also included an explanation for why diff-tree was the only caller that was missing the necessary logic (see the last paragraph, which kind of references the one before it as well).

Your explanations better illustrate the impact of this bug, I've quoted them in the new patch commit message.

[snip]

Quoted 13 lines

builtin/diff-tree.c line 12
Quoted 9 lines
builtin/diff-tree.c +1 -0
index 0d3c611aac0..813be486dad 100644
@@ -9,6 +9,7 @@
 #include "repository.h"
 #include "revision.h"
 #include "tree.h"
+#include "tmp-objdir.h"
The includes other than this one are in alphabetical order; can you move this a line before?

Quoted 12 lines

Also, as an aside, folks in this project often just put includes at the end, but I think it's a bad practice. Whenever someone needs to backport fixes or merge separate patch topics into seen/next/etc. or even merge not-yet-upstream topics with newer upstream versions, this practice increases the odds of unnecessary conflicts. And it makes it harder for the next person who comes along to spot whether a header is already included (and sometimes leaves us including headers twice). While each case is a small amount of toil so we tend to overlook it, it's totally unnecessary toil in many cases. Putting includes in alphabetical order (other than the one include required to be first, git-compat-util.h or its documented stand-ins) can often remove this unnecessary toil. Anyway, thanks for letting me vent. :-)

Noted. I'll move the new include to the correct position. Thanks for the guidance!

Quoted 20 lines

builtin/diff-tree.c +7 -0

 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);
        }

Quoted 9 lines

builtin/diff-tree.c int cmd_diff_tree(int argc, const char **argv, const char *prefix) · line 247
Quoted 7 lines
builtin/diff-tree.c +5 -0
+       if (opt->remerge_diff) {
+               tmp_objdir_destroy(opt->remerge_objdir);
+               opt->remerge_objdir = NULL;
+       }
+
        return diff_result_code(&opt->diffopt);
 }
Your fix exactly matches mine, other than the header include location.

High five! :-)

[snip]

Xing Xin

blanet via GitGitGadget

author

2024-08-09 07:24 UTC #4 lore raw

PATCH v2 1/1 diff-tree: fix crash when used with --remerge-diff

In reply to blanet via GitGitGadget

From: Xing Xin <[email protected]>

When using "git-diff-tree" to get the tree diff for merge commits with the diff format set to remerge, a bug is triggered as shown below:

  $ git diff-tree -r --remerge-diff 363337e6eb
  363337e6eb812d0c0d785ed4261544f35559ff8b
  BUG: log-tree.c:1006: did a remerge diff without remerge_objdir?!?

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.

This commit resolves the crash by adding remerge_objdir setup logic to builtin/diff-tree.c, mirroring builtin/log.c:cmd_log_walk_no_free. It also includes the necessary cleanup for remerge_objdir.

Reviewed-by Elijah Newren <[email protected]>

Signed-off-by Xing Xin <[email protected]>

---
diff-tree: fix crash when used with --remerge-diff

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1771%2Fblanet%2Fxx%2Ffix-diff-tree-crash-on-remerge-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1771/blanet/xx/fix-diff-tree-crash-on-remerge-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1771

Range-diff vs v1:

1: f0b86faa275 ! 1: 57f0b1247d8 diff-tree: fix crash when used with --remerge-diff
@@ Commit message
When using "git-diff-tree" to get the tree diff for merge commits with the diff format set to remerge, a bug is triggered as shown below:

- $ git diff-tree -r --remerge-diff 363337e6eb
- 363337e6eb812d0c0d785ed4261544f35559ff8b
- BUG: log-tree.c:1006: did a remerge diff without remerge_objdir?!?
+ $ git diff-tree -r --remerge-diff 363337e6eb
+ 363337e6eb812d0c0d785ed4261544f35559ff8b
+ BUG: log-tree.c:1006: did a remerge diff without remerge_objdir?!?

This bug is reported by log-tree.c:do_remerge_diff, where a bug check added in commit 7b90ab467a (log: clean unneeded objects during log
@@ Commit message
"git-diff-tree", but were not accounted for in the original implementation (inferred from the commit message).

- This commit fixes the bug by adding initialization logic for
- remerge_objdir in builtin/diff-tree.c, mirroring the logic in
- builtin/log.c:cmd_log_walk_no_free. A final cleanup for
- remerge_objdir is also included.
+ 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.
+
+ This commit resolves the crash by adding remerge_objdir setup logic to
+ builtin/diff-tree.c, mirroring builtin/log.c:cmd_log_walk_no_free.
+ It also includes the necessary cleanup for remerge_objdir.
+
+ Reviewed-by: Elijah Newren <[email protected]>

Signed-off-by Xing Xin <[email protected]>

## builtin/diff-tree.c ##
@@
+ #include "read-cache-ll.h"
#include "repository.h"
#include "revision.h"
- #include "tree.h"
+#include "tmp-objdir.h"
+ #include "tree.h"

static struct rev_info log_tree_opt;
-
@@ builtin/diff-tree.c: int cmd_diff_tree(int argc, const char **argv, const char *prefix)

opt->diffopt.rotate_to_strict = 1;

 builtin/diff-tree.c     | 13 +++++++++++++
 t/t4069-remerge-diff.sh | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

builtin/diff-tree.c +13 -0

index 0d3c611aac0..b8df1d4b79b 100644
@@ -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);
 }

t/t4069-remerge-diff.sh +35 -0

index 07323ebafe0..ca8f999caba 100755
@@ -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 &&

base-commit: 406f326d271e0bacecdb00425422c5fa3f314930

Signature · 1 line

gitgitgadget

Junio C Hamano

2024-08-09 15:32 UTC #5 lore raw

Quoted 9 lines · blanet via GitGitGadget

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

2024-08-09 15:46 UTC #6 lore raw

Quoted 2 lines · Junio C Hamano

but I have to wonder if we do even better in a future follow-up patch.

"if we do" -> "if we can do".

Quoted 7 lines

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.