Add a new --remerge-diff capability to show & log

From: Elijah Newren <[email protected]>

path_msg() has the ability to mark messages as omittable when recording conflict messages in an in-tree file. This conflict message file will then be shown by diffing the merge-created tree to the actual merge commit that is created. While all the messages touched in this commit are very useful when trying to create a merge initially, early use with the --remerge-diff feature (the only user of this omittable conflict message capability), suggests that the particular messages marked in this commit are just noise when trying to see what changes users made to create a merge commit. Mark them as omittable.

Quoted 11 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

path_msg() has the ability to mark messages as omittable when recording conflict messages in an in-tree file. This conflict message file will then be shown by diffing the merge-created tree to the actual merge commit that is created. While all the messages touched in this commit are very useful when trying to create a merge initially, early use with the --remerge-diff feature (the only user of this omittable conflict message capability), suggests that the particular messages marked in this commit are just noise when trying to see what changes users made to create a merge commit. Mark them as omittable.

Sorry for asking something that may be obvious

but if you recreate
an automated and potentially conflicting merge in-core, in order to then compare the recorded merge result, is there *any* message that is worth showing while doing the first step,

and where in the output do users see them?

Creating this file means you see something like this in the remerge-diff (note there are diffs for two files, with the synthetic file appearing just before the file it has messages about):

dir/my.fil e.conflict_msg deleted +0 -2

deleted file mode 100644
index 2bd215a32f06..000000000000
@@ -1,2 +0,0 @@
-== Conflict notices for my.file ==
-CONFLICT (modify/delete): dir/my.file deleted in HASH1 (SHORT

SUMMARY1) and modified in HASH2 (SHORT SUMMARY 2). Version HASH2 (SHORT SUMMARY2) of dir/my.file left in tree.

There are several considerations here:
* We have to pick file(s) where we write these conflict messages too
* We want to make it as clear as possible that it's not a real file but a set of messages about another file
* We want conflict messages about a file to appear near the file in question in a diff, preferably immediately preceding the file in question
* Related to the above, per-file conflict messages are preferred over lumping all conflict messages into one big file

To achive the above:
* We put the conflict messages for $filename in $filename[0:-1] + " " + $filename[-1] + ".conflict_msg" or, in words, we insert a space before the final character of the filename and then also add ".conflict_msg" at the end.

Quoted 10 lines · Elijah Newren via GitGitGadget

There are several considerations here:
* We have to pick file(s) where we write these conflict messages too
* We want to make it as clear as possible that it's not a real file but a set of messages about another file
* We want conflict messages about a file to appear near the file in question in a diff, preferably immediately preceding the file in question
* Related to the above, per-file conflict messages are preferred over lumping all conflict messages into one big file

Quoted 5 lines

To achive the above:
* We put the conflict messages for $filename in $filename[0:-1] + " " + $filename[-1] + ".conflict_msg" or, in words, we insert a space before the final character of the filename and then also add ".conflict_msg" at the end.

It took me a minute to understand the space thing. I thought at first it was about avoiding conflicts with existing names (and while it might help in practice, it's not a guarantee). But I think it's about the "appear preceding the file" goal. The space sorts before any other printable character in the final position.

That's...simultaneously clever and gross. My biggest complaint is that the space looks like a bug in the output.

Using another character like "." might not be too bad, as it's also fairly early in the ascii table. But it's really this "do it before the last character" thing that is key to getting the ordering right.

Just brainstorming some alternatives:

- we have diff.orderFile, etc. Could we stuff this data into a less confusing name (even just "$filename.conflict_msg"), and then provide a custom ordering to the diff code? I think it could be done by generating a static ordering ahead of time, but it might even just be possible to tell diffcore_order() to take the ".conflict_msg" extension into account in its comparison function.

- there can be other non-diff data between the individual segments. For example, "patch" will skip over non-diff lines. And certainly in Git we have our own custom headers. I'm wondering if we could attach these annotations to the diff-pair somehow, and then show something like:

     diff --git a/foo.c b/foo.c
     index 1234abcd..5678cdef 100644
     conflict modify/delete foo.c

     --- a/foo.c
     +++ b/foo.c
     @@ some actual diff starts here @@

Obviously such a thing can't really be applied. But then you wouldn't want to apply the addition of "my.file e.conflict_msg" either.

I dunno. The latter especially is definitely more work, and requires a bit more cooperation between the merge and diff code. In particular, you can't just feed a straight tree to the diff anymore. We have to hold back the annotations, and then apply them to the resulting diff. But I think the output is much more pleasing to the eye.

I also don't like how diff.orderFile provides a global ordering of the files listed, rather than providing some scheme for relative orderings. That'd either force me to precompute the diff to determine all the files that were different so I can list _all_ of them,...

So, I'm not so sure about this solution; it feels like it introduces bigger holes than the ugly space character it is fixing.

Quoted 9 lines

- there can be other non-diff data between the individual segments. For example, "patch" will skip over non-diff lines. And certainly in Git we have our own custom headers. I'm wondering if we could attach these annotations to the diff-pair somehow, and then show something like:

     diff --git a/foo.c b/foo.c
     index 1234abcd..5678cdef 100644
     conflict modify/delete foo.c

A couple things here...

First, I'm not so sure I like the abbreviation here. Just knowing "modify/delete" might be enough in some cases, but I'd rather have the full messages that would have been printed to the console...

Second, what about when there are multiple ...

Third, what about the cases where there is no diff, ...

Quoted 4 lines · Elijah Newren

I also don't like how diff.orderFile provides a global ordering of the files listed, rather than providing some scheme for relative orderings. That'd either force me to precompute the diff to determine all the files that were different so I can list _all_ of them,...

Don't we determine all the files that would be listed in the diff anyway? The diffcore pipeline collects all the different filepairs, matches them up for rename/copy detection, and finally do the output formatting for each individual filepair.

Quoted 21 lines

So, I'm not so sure about this solution; it feels like it introduces bigger holes than the ugly space character it is fixing.

Quoted 9 lines

- there can be other non-diff data between the individual segments. For example, "patch" will skip over non-diff lines. And certainly in Git we have our own custom headers. I'm wondering if we could attach these annotations to the diff-pair somehow, and then show something like:

     diff --git a/foo.c b/foo.c
     index 1234abcd..5678cdef 100644
     conflict modify/delete foo.c

A couple things here...

First, I'm not so sure I like the abbreviation here. Just knowing "modify/delete" might be enough in some cases, but I'd rather have the full messages that would have been printed to the console...

Second, what about when there are multiple ...

Quoted 1 line

Third, what about the cases where there is no diff, ...

None of the above seems like a problem to me at least from the presentation and consumption sides. There is no rule that extended diff headers has to fit on a 72-char line, cannot use line folding by inserting LF-SP in the middle of a logical line, and there already is at least one case we give an extended diff header without a single line of content change (namely, "chmod +x README").

Quoted 8 lines

Just brainstorming some alternatives:

- we have diff.orderFile, etc. Could we stuff this data into a less confusing name (even just "$filename.conflict_msg"), and then provide a custom ordering to the diff code? I think it could be done by generating a static ordering ahead of time, but it might even just be possible to tell diffcore_order() to take the ".conflict_msg" extension into account in its comparison function.

I can't just go on the ".conflict_msg" extension. As you noted above, this scheme is not sufficient for avoiding collisions. So I need to append extra "cruft" to the name in the case of collisions -- meaning we can't special case on just that extension.

I also don't like how diff.orderFile provides a global ordering of the files listed, rather than providing some scheme for relative orderings. That'd either force me to precompute the diff to determine all the files that were different so I can list _all_ of them, or put up with the fact that the files with non-content conflicts will be listed very first in the output, even if their name is 'zee-last-file.c' -- surprising users at the output ordering.

This also means that if the user had their own ordering defined, then I'm overriding it and messing up their ordering, which might be problematic.

Quoted 9 lines

- there can be other non-diff data between the individual segments. For example, "patch" will skip over non-diff lines. And certainly in Git we have our own custom headers. I'm wondering if we could attach these annotations to the diff-pair somehow, and then show something like:

     diff --git a/foo.c b/foo.c
     index 1234abcd..5678cdef 100644
     conflict modify/delete foo.c

A couple things here...

[...]
That said, I think it could also make sense to annotate the conflict files by giving them an unusual set of mode bits. That would be easier to recognize (and while real trees _could_ have silly modes, we do complain about them in fsck, so it shouldn't happen in practice).

Quoted 9 lines · Elijah Newren

Quoted 8 lines

Just brainstorming some alternatives:

- we have diff.orderFile, etc. Could we stuff this data into a less confusing name (even just "$filename.conflict_msg"), and then provide a custom ordering to the diff code? I think it could be done by generating a static ordering ahead of time, but it might even just be possible to tell diffcore_order() to take the ".conflict_msg" extension into account in its comparison function.

Quoted 4 lines

I can't just go on the ".conflict_msg" extension. As you noted above, this scheme is not sufficient for avoiding collisions. So I need to append extra "cruft" to the name in the case of collisions -- meaning we can't special case on just that extension.

Sure, but we can call it filename.conflict_msg.1, etc, and the sort code can pattern-match. It can never be fully robust (if you really did have a foo.conflict_msg, we'd sort it differently), but I think it's OK if the worst-case is that pathological trees get ordered slightly sub-optimally).

That said, I think it could also make sense to annotate the conflict files by giving them an unusual set of mode bits. That would be easier to recognize (and while real trees _could_ have silly modes, we do complain about them in fsck, so it shouldn't happen in practice).

Quoted 11 lines

I also don't like how diff.orderFile provides a global ordering of the files listed, rather than providing some scheme for relative orderings. That'd either force me to precompute the diff to determine all the files that were different so I can list _all_ of them, or put up with the fact that the files with non-content conflicts will be listed very first in the output, even if their name is 'zee-last-file.c' -- surprising users at the output ordering.

This also means that if the user had their own ordering defined, then I'm overriding it and messing up their ordering, which might be problematic.

Agreed. I do think it may be too horrible unless you teach diffcore_order() to actually understand your annotations in code. But that wouldn't be too hard if it's done in the mode bits.

But I do think anything that avoids these pseudo-files is going to be a lot cleaner overall.

Quoted 11 lines

Quoted 9 lines

- there can be other non-diff data between the individual segments. For example, "patch" will skip over non-diff lines. And certainly in Git we have our own custom headers. I'm wondering if we could attach these annotations to the diff-pair somehow, and then show something like:

     diff --git a/foo.c b/foo.c
     index 1234abcd..5678cdef 100644
     conflict modify/delete foo.c

A couple things here...

I think Junio already indicated that we can pretty much make this look like whatever we want. As soon as any reader sees "conflict", it should happily ignore the rest as something it doesn't know about. And my short example here was just meant to be illustrative. I agree it probably needs more details (and the whole CONFLICT line that usually goes to stderr is probably the best thing).

-Peff

Quoted 4 lines

That said, I think it could also make sense to annotate the conflict files by giving them an unusual set of mode bits. That would be easier to recognize (and while real trees _could_ have silly modes, we do complain about them in fsck, so it shouldn't happen in practice).

I tried giving things unusual mode bits in early versions of merge-ort for other reasons. It doesn't work: canon_mode() will "fix" the unusualness so there's no way for the reader to know that they had unusual bits.

But, as you said later, avoiding these pseudo-files is going to be cleaner anyway, so let's just do that.

From: Elijah Newren <[email protected]>

Whenever ll-merge encounters a binary file, it prints a warning to stderr. However, for the --remerge-diff option we want to add, we need to capture all conflict messages and show them in a diff instead of dumping them straight to stdout or stderr. Add some new API that will allow us to capture this output and display it in our preferred method.

I do wonder if the ll_merge() code should avoid calling warning() in the first place. It is after all, meant to be "low-level". We already return an error code from the function. I wonder if returning a more detailed code instead, like:

  enum LL_MERGE_RESULT {
	LL_MERGE_OK = 0,
	LL_MERGE_CONFLICT,
	LL_MERGE_BINARY_CONFLICT,

};

would let the caller do the sensible thing.

Quoted 7 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Whenever ll-merge encounters a binary file, it prints a warning to stderr. However, for the --remerge-diff option we want to add, we need to capture all conflict messages and show them in a diff instead of dumping them straight to stdout or stderr. Add some new API that will allow us to capture this output and display it in our preferred method.

This is a reasonable strategy for error-handling in general (though some more thoughts below).

ll-merge.c static int ll_binary_merge(const struct ll_merge_driver *drv_unused, · line 82

Quoted 11 lines

ll-merge.c +5 -2

@@ -71,8 +75,11 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
      } else {
              switch (opts->variant) {
              default:
-                     warning("Cannot merge binary files: %s (%s vs. %s)",
-                             path, name1, name2);
+                     if (warnings) {
+                             strbuf_addstr(warnings, "Warning: ");
+                             strbuf_addf(warnings, msg, path, name1, name2);
+                     } else
+                             warning(msg, path, name1, name2);

The usual warn_builtin() has a lowercase "warning: " prefix, but your strbuf variant uses uppercase.

ll-merge.c static int ll_binary_merge(const struct ll_merge_driver *drv_unused, · line 111

Quoted 8 lines

ll-merge.c +1 -0

@@ -98,6 +105,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
 
 static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
                      mmbuffer_t *result,
+                     struct strbuf *warnings,
                      const char *path,
                      mmfile_t *orig, const char *orig_name,
                      mmfile_t *src1, const char *name1,

There's a lot of plumbing this variable through. This is probably too gross, but another option would be to call set_warn_routine() to override it temporarily. It's gross because it affects everything, not just this call stack (and I also think we'd need to beef up the warn routine code to handle some of the rough edges).

I do wonder if the ll_merge() code should avoid calling warning() in the first place. It is after all, meant to be "low-level". We already return an error code from the function. I wonder if returning a more detailed code instead, like:

  enum LL_MERGE_RESULT {
        LL_MERGE_OK = 0,
        LL_MERGE_CONFLICT,
        LL_MERGE_BINARY_CONFLICT,
  };

would let the caller do the sensible thing.

From: Elijah Newren <[email protected]>

Instead of immediately printing ll-merge warnings to stderr, we save them in our output strbuf. Besides allowing us to move these warnings to a special file for --remerge-diff, this has two other benefits for regular merges done by merge-ort:

* The deferral of messages ensures we can print all messages about any given path together (merge-recursive was known to sometimes intersperse messages about other paths, particularly when renames were involved).

* The deferral of messages means we can avoid printing spurious conflict messages when we just end up aborting due to local user modifications in the way. (In contrast to merge-recursive.c which prematurely checks for local modifications in the way via unpack_trees() and gets the check wrong both in terms of false positives and false negatives relative to renames, merge-ort does not perform the local modifications in the way check until the checkout() step after the full merge has been computed.)

t/t6404-recursive-merge.sh +0 -1

@@ -108,8 +108,14 @@ test_expect_success 'refuse to merge binary files' '
 	printf "\0\0" >binary-file &&
 	git add binary-file &&
 	git commit -m binary2 &&
-	test_must_fail git merge F >merge.out 2>merge.err &&

Quoted 5 lines · Elijah Newren via GitGitGadget

t/t6404-recursive-merge.sh +0 -1

@@ -108,8 +108,14 @@ test_expect_success 'refuse to merge binary files' '
      printf "\0\0" >binary-file &&
      git add binary-file &&
      git commit -m binary2 &&
-     test_must_fail git merge F >merge.out 2>merge.err &&

t/t6404-recursive-merge.sh test_expect_success 'refuse to merge binary files' ' · line 119

Quoted 10 lines

t/t6404-recursive-merge.sh +8 -1

-     grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err
+     if test "$GIT_TEST_MERGE_ALGORITHM" = ort
+     then
+             test_must_fail git merge F >merge.out &&
+             grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.out
+     else
+             test_must_fail git merge F >merge.out 2>merge.err &&
+             grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err
+     fi
 '

To save readers from eyeballing if a single character has changed here, which I don't think it has, just do:

if ...
then
cmd >actual
else
other cmd >actual

fi &&
grep [...]

I.e. no need to duplicate the "grep" here just because of merge.out v.s. merge.err.

[...]

t/t6406-merge-attr.sh test_expect_success 'binary files with union attribute' ' · line 231

Quoted 10 lines

t/t6406-merge-attr.sh +8 -2

-     test_must_fail git merge bin-main 2>stderr &&
-     grep -i "warning.*cannot merge.*HEAD vs. bin-main" stderr
+     if test "$GIT_TEST_MERGE_ALGORITHM" = ort
+     then
+             test_must_fail git merge bin-main >stdout &&
+             grep -i "warning.*cannot merge.*HEAD vs. bin-main" stdout
+     else
+             test_must_fail git merge bin-main 2>stderr &&
+             grep -i "warning.*cannot merge.*HEAD vs. bin-main" stderr
+     fi

ditto.

Quoted 23 lines · Ævar Arnfjörð Bjarmason

Quoted 5 lines · Elijah Newren via GitGitGadget

t/t6404-recursive-merge.sh +0 -1

@@ -108,8 +108,14 @@ test_expect_success 'refuse to merge binary files' '
      printf "\0\0" >binary-file &&
      git add binary-file &&
      git commit -m binary2 &&
-     test_must_fail git merge F >merge.out 2>merge.err &&

t/t6404-recursive-merge.sh test_expect_success 'refuse to merge binary files' ' · line 119

Quoted 10 lines

t/t6404-recursive-merge.sh +8 -1

-     grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err
+     if test "$GIT_TEST_MERGE_ALGORITHM" = ort
+     then
+             test_must_fail git merge F >merge.out &&
+             grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.out
+     else
+             test_must_fail git merge F >merge.out 2>merge.err &&
+             grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err
+     fi
 '

To save readers from eyeballing if a single character has changed here, which I don't think it has, just do:

Quoted 5 lines

if ...
then
cmd >actual
else
other cmd >actual

Do you mean
other cmd >/dev/null 2>actual
?

On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:

I commented just now on how this API is duplicated between here & another in-flight series in
https://lore.kernel.org/git/[email protected]/; Just comments on this patch here:

tmp-objdir.c const char **tmp_objdir_env(const struct tmp_objdir *t) · line 294

Quoted 12 lines

tmp-objdir.c +4 -0

index b8d880e3626..9f75a75d1c0 100644
@@ -288,7 +288,36 @@ const char **tmp_objdir_env(const struct tmp_objdir *t)
      return t->env.v;
 }
 
+const char *tmp_objdir_path(struct tmp_objdir *t)
+{
+     return t->path.buf;
+}

Not your fault & this pre-dates your patch, but FWIW I prefer our APIs that don't have these "hidden struct" shenanigans (like say "struct string_list") so a caller could just access this, we can just declare it "const" appropriately.

We're also all in-tree friends here, so having various accessors for no reason other than to access struct members seems a bit too much.

All of which is to say if you + Neeraj come up with some common API I for one wouldn't mind moving the struct deceleration to tmp-objdir.h, but it looks like in his version it can stay "private" more easily.

In this particular case I hadn't understood on a previous reading of tmp-objdir.c why this "path" isn't a statically allocated "char path[PATH_MAX]", or a least we that hardcoded "1024" should be a PATH_MAX, as it is in some other cases.

Quoted 7 lines

Not your fault & this pre-dates your patch, but FWIW I prefer our APIs that don't have these "hidden struct" shenanigans (like say "struct string_list") so a caller could just access this, we can just declare it "const" appropriately.

We're also all in-tree friends here, so having various accessors for no reason other than to access struct members seems a bit too much.

That's a fair point, but just to play counterpoint for a minute...

FWIW, I dislike when our public facing APIs are polluted with all kinds of internal details. merge-recursive being a case in point. When writing merge-ort, although I had a struct with public fields, that struct also contained an opaque struct (pointer) within it to hide several private fields. (I would have liked to hide or remove a few more fields, but couldn't do so while the merge_recursive_options struct was shared between both merge-ort and merge-recursive.)

That said, I agree it can certainly be overdone, and tmp_objdir is pretty simple. However, sometimes even in simple cases I like when folks make use of an opaque struct because it means folks put some effort into thinking more about the API that should be exposed. That's something we as a project have often overlooked in the past, as evidenced both by our one-shot usage mentality, and the existence of external projects like libgit2 attempting to correct this design shortcoming. I'd like git to move more towards being structured as a reusable library as well as a useful command-line tool.

Quoted 26 lines · Ævar Arnfjörð Bjarmason

On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:

I commented just now on how this API is duplicated between here & another in-flight series in
https://lore.kernel.org/git/[email protected]/; Just comments on this patch here:

tmp-objdir.c const char **tmp_objdir_env(const struct tmp_objdir *t) · line 294

Quoted 12 lines

tmp-objdir.c +4 -0

index b8d880e3626..9f75a75d1c0 100644
@@ -288,7 +288,36 @@ const char **tmp_objdir_env(const struct tmp_objdir *t)
      return t->env.v;
 }
 
+const char *tmp_objdir_path(struct tmp_objdir *t)
+{
+     return t->path.buf;
+}

Not your fault & this pre-dates your patch, but FWIW I prefer our APIs that don't have these "hidden struct" shenanigans (like say "struct string_list") so a caller could just access this, we can just declare it "const" appropriately.

Quoted 2 lines

We're also all in-tree friends here, so having various accessors for no reason other than to access struct members seems a bit too much.

That's a fair point, but just to play counterpoint for a minute...

FWIW, I dislike when our public facing APIs are polluted with all kinds of internal details. merge-recursive being a case in point. When writing merge-ort, although I had a struct with public fields, that struct also contained an opaque struct (pointer) within it to hide several private fields. (I would have liked to hide or remove a few more fields, but couldn't do so while the merge_recursive_options struct was shared between both merge-ort and merge-recursive.)

That said, I agree it can certainly be overdone, and tmp_objdir is pretty simple. However, sometimes even in simple cases I like when folks make use of an opaque struct because it means folks put some effort into thinking more about the API that should be exposed. That's something we as a project have often overlooked in the past, as evidenced both by our one-shot usage mentality, and the existence of external projects like libgit2 attempting to correct this design shortcoming. I'd like git to move more towards being structured as a reusable library as well as a useful command-line tool.

[...]

Quoted 4 lines

In this particular case I hadn't understood on a previous reading of tmp-objdir.c why this "path" isn't a statically allocated "char path[PATH_MAX]", or a least we that hardcoded "1024" should be a PATH_MAX, as it is in some other cases.

Actually, PATH_MAX shouldn't be used:

https://lore.kernel.org/git/[email protected]/
https://lore.kernel.org/git/[email protected]/

Quoted 41 lines · Elijah Newren

Quoted 26 lines · Ævar Arnfjörð Bjarmason

On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:

I commented just now on how this API is duplicated between here & another in-flight series in
https://lore.kernel.org/git/[email protected]/; Just comments on this patch here:

tmp-objdir.c const char **tmp_objdir_env(const struct tmp_objdir *t) · line 294

Quoted 12 lines

tmp-objdir.c +4 -0

index b8d880e3626..9f75a75d1c0 100644
@@ -288,7 +288,36 @@ const char **tmp_objdir_env(const struct tmp_objdir *t)
      return t->env.v;
 }
 
+const char *tmp_objdir_path(struct tmp_objdir *t)
+{
+     return t->path.buf;
+}

Not your fault & this pre-dates your patch, but FWIW I prefer our APIs that don't have these "hidden struct" shenanigans (like say "struct string_list") so a caller could just access this, we can just declare it "const" appropriately.

Quoted 2 lines

We're also all in-tree friends here, so having various accessors for no reason other than to access struct members seems a bit too much.

That's a fair point, but just to play counterpoint for a minute...

FWIW, I dislike when our public facing APIs are polluted with all kinds of internal details. merge-recursive being a case in point. When writing merge-ort, although I had a struct with public fields, that struct also contained an opaque struct (pointer) within it to hide several private fields. (I would have liked to hide or remove a few more fields, but couldn't do so while the merge_recursive_options struct was shared between both merge-ort and merge-recursive.)

Quoted 9 lines

That said, I agree it can certainly be overdone, and tmp_objdir is pretty simple. However, sometimes even in simple cases I like when folks make use of an opaque struct because it means folks put some effort into thinking more about the API that should be exposed. That's something we as a project have often overlooked in the past, as evidenced both by our one-shot usage mentality, and the existence of external projects like libgit2 attempting to correct this design shortcoming. I'd like git to move more towards being structured as a reusable library as well as a useful command-line tool.

[This is somewhat of a continuation of
https://lore.kernel.org/git/[email protected]/].

I like our APIs being usable, but right now we're not promising any maintenance of APIs that work the same as git v2.30.0 or whatever.

I think some external users directly link to libgit.a, but they're tracking whatever churn we have from release to release, just like someone maintaining out-of-tree linux kernel drivers.

For maintenance of git.git itself I think it's going to stay like that for any foreseeable future[1].

And that's before we even get into the practicalities of git's license likely not winning over many (if any) libgit2 users. I think those users are if anything migrating from libgit2 to git's plumbing CLI, or in some cases folding what they needed into git.git itself (or just staying with libgit2).

So assuming all of that I really don't see the point in general of having a "foo" field that has the equivalent of get_foo() and set_foo() accessors in the context of git.git.

That's something that's really useful if you're maintaining API and ABI compatibility, but for us it's usually just verbosity. All the users are in-tree, so just add "const" as appropriate, or perhaps we should give the field a "priv" name or whatever.

Obviously not everything should be done that way, e.g. being able to have trace2 start/end points for something you'd otherwise push/clear from a "struct string_list" or whatever directly *is* usable, so it's not black and white.

I'm just saying thaht using convoluted patterns *just* to get in the way of some hypothetical code that's going to mess with your internals can make things harder in other areas. E.g. the init/release pattern via macros I mentioned in the linked E-Mail above.

Quoted 10 lines

[...]

Quoted 4 lines

In this particular case I hadn't understood on a previous reading of tmp-objdir.c why this "path" isn't a statically allocated "char path[PATH_MAX]", or a least we that hardcoded "1024" should be a PATH_MAX, as it is in some other cases.

Actually, PATH_MAX shouldn't be used:

https://lore.kernel.org/git/[email protected]/
https://lore.kernel.org/git/[email protected]/

Thanks. I didn't know about that. I've fixed (some of that) up in some semi-related WIP work I've got.

1. Aside: I *do* think it would be really useful for us to expose a stable C API eventually, using some of the same license workarounds/shenanigans (depending on your view) as the Linux kernel did. I.e. to say that "we're GPLv2, but this small API is the equivalent of our syscall interface".

Rather than that API being something that would look anything like internal git.git users it would basically be a run_command() + Perl's unpack/pack + output formatter on steroids.

I.e. we'd be able to invoke say a 'git cat-file --batch' in the same process as the caller by essentially calling cmd_cat_file() directly. Just with some callback replacements for "provide this on stdin" and "this line accepts stdout" without actually involving different processes or I/O.

You'd then have the equivalent of a --format output that would just so happen to be in your OS's / C ABI's understood struct format. Similar to "perldoc -f pack".

The maintenance burden of such an API would be trivial. It would basically be a handful of functions just to setup input/output callbacks.

Most of the interface would be a rather convoluted construction of command-line parameters (like our own run_command() use), not ideal, but most users are after structured data and eliminating process overhead, and they'd get that.

We would need to replace fprintf() and the like with some structured formatting mechanism, but that would be useful for non-API users as new commands learning a shiny new --format parameter. You could even use such a --format to have a non-C-API user get the same struct output, perhaps for direct mapping into memory, say if you didn't trust the built-in/command in question to not leak memory yet.

I don't know if this has been suggested before, just my 0.02. It's also partially why I'm interested in making even-built-ins not leak memory with some of my parallel SANITIZE=leak work.

Once you've got that you can usually call the cmd_whatever() in a loop without much effort, and once you've got that, well, see above :)

From: Elijah Newren <[email protected]>

The tmp_objdir API provides the ability to create temporary object directories, but was designed with the goal of having subprocesses access these object stores, followed by the main process migrating objects from it to the main object store or just deleting it. The subprocesses would view it as their primary datastore and write to it.

For the --remerge-diff option we want to add to show & log, we want all writes of intermediate merge results (both blobs and trees) to go to this alternate object store; since those writes will be done by the main process, we need this "alternate" object store to actually be the primary object store. When show & log are done, they'll simply remove this temporary object store.

We also need one more thing -- git log --remerge-diff can cause the temporary object store to fill up with loose objects. Rather than trying to gc that are known garbage anyway, we simply want to know the location of the temporary object store so we can purge the loose objects after each merge.

Side note: The pretend_object_file() approach is actually even better, because we know the object is fake. So it does not confuse write_object_file()'s "do we already have this object" freshening check.

I suspect it could even be made faster than the tmp_objdir approach. From our perspective, these objects really are tempfiles. So we could write them as such, not worrying about things like fsyncing them, naming them into place, etc. We could just write them out, then mmap the results, and put the pointers into cached_objects (currently it insists on malloc-ing a copy of the input buffer, but that seems like an easy extension to add).

In fact, I think you could get away with just _one_ tempfile per merge. Open up one tempfile. Write out all of the objects you want to "store" into it in sequence, and record the lseek() offsets before and after for each object. Then mmap the whole result, and stuff the appropriate pointers (based on arithmetic with the offsets) into the cached_objects list.

Quoted 13 lines · Jeff King

Side note: The pretend_object_file() approach is actually even better, because we know the object is fake. So it does not confuse write_object_file()'s "do we already have this object" freshening check.

I suspect it could even be made faster than the tmp_objdir approach. From our perspective, these objects really are tempfiles. So we could write them as such, not worrying about things like fsyncing them, naming them into place, etc. We could just write them out, then mmap the results, and put the pointers into cached_objects (currently it insists on malloc-ing a copy of the input buffer, but that seems like an easy extension to add).

Quoted 6 lines

In fact, I think you could get away with just _one_ tempfile per merge. Open up one tempfile. Write out all of the objects you want to "store" into it in sequence, and record the lseek() offsets before and after for each object. Then mmap the whole result, and stuff the appropriate pointers (based on arithmetic with the offsets) into the cached_objects list.

Cute. The remerge diff code path creates a full tree that records the mechanical merge result. By hooking into the lowest layer of write_object() interface, we'd serialize all objects in such a tree in the order they are computed (bottom up from the leaf level, I'd presume) into a single flat file ;-)

Quoted 22 lines · Junio C Hamano

Quoted 13 lines · Jeff King

Side note: The pretend_object_file() approach is actually even better, because we know the object is fake. So it does not confuse write_object_file()'s "do we already have this object" freshening check.

I suspect it could even be made faster than the tmp_objdir approach. From our perspective, these objects really are tempfiles. So we could write them as such, not worrying about things like fsyncing them, naming them into place, etc. We could just write them out, then mmap the results, and put the pointers into cached_objects (currently it insists on malloc-ing a copy of the input buffer, but that seems like an easy extension to add).

Quoted 6 lines

In fact, I think you could get away with just _one_ tempfile per merge. Open up one tempfile. Write out all of the objects you want to "store" into it in sequence, and record the lseek() offsets before and after for each object. Then mmap the whole result, and stuff the appropriate pointers (based on arithmetic with the offsets) into the cached_objects list.

Quoted 5 lines

Cute. The remerge diff code path creates a full tree that records the mechanical merge result. By hooking into the lowest layer of write_object() interface, we'd serialize all objects in such a tree in the order they are computed (bottom up from the leaf level, I'd presume) into a single flat file ;-)

I do still like this approach, but just two possible gotchas I was thinking of:

- This side-steps all of our usual code for getting object data into memory. In general, I'd expect this content to not be too enormous, but it _could_ be if there are many / large blobs in the result. So we may end up with large maps. Probably not a big deal on modern 64-bit systems. Maybe an issue on 32-bit systems, just because of virtual address space.

Likewise, we do support systems with NO_MMAP. They'd work here, but it would probably mean putting all that object data into the heap. I could live with that, given how rare such systems are these days, and that it only matters if you're using --remerge-diff with big blobs.

- I wonder to what degree --remerge-diff benefits from omitting writes for objects we already have. I.e., if you are writing out a whole tree representing the conflicted state, then you don't want to write all of the trees that aren't interesting. Hopefully the code is already figuring out which paths the merge even touched, and ignoring the rest. It probably benefits performance-wise from write_object_file() deciding to skip some object writes, as well (e.g., for resolutions which the final tree already took, as they'd be in the merge commit). The whole pretend-we-have-this-object thing may want to likewise make sure we don't write out objects that we already have in the real odb.

I also wonder how much if anything writing out the one file v.s. lots of loose objects is worthwhile on systems where we could write out those loose objects on a ramdisk, which is commonly available on e.g. Linux distros these days out of the box. If you care about performance but not about your transitory data using a ramdisk is generally much better than any other potential I/O optimization.

Finally, and I don't mean to throw a monkey wrench into this whole discussion, so take this as a random musing: I wonder how much faster this thing could be on its second run if instead of avoiding writing to the store & cleaning up, it just wrote to the store, and then wrote another object keyed on the git version and any revision paramaters etc., and then pretty much just had to do a "git cat-file -p <that-obj>" to present the result to the user :)

I suppose that would be throwing a lot more work at an eventual "git gc" than we ever do now, so maybe it's a bit crazy, but I think it might be an interesting direction in general to (ab)use either the primary or some secondary store in the .git dir as a semi-permanent cache of resolved queries from the likes of "git log".

Quoted 45 lines · Jeff King

Quoted 22 lines · Junio C Hamano

Quoted 13 lines · Jeff King

Side note: The pretend_object_file() approach is actually even better, because we know the object is fake. So it does not confuse write_object_file()'s "do we already have this object" freshening check.

I suspect it could even be made faster than the tmp_objdir approach. From our perspective, these objects really are tempfiles. So we could write them as such, not worrying about things like fsyncing them, naming them into place, etc. We could just write them out, then mmap the results, and put the pointers into cached_objects (currently it insists on malloc-ing a copy of the input buffer, but that seems like an easy extension to add).

Quoted 6 lines

In fact, I think you could get away with just _one_ tempfile per merge. Open up one tempfile. Write out all of the objects you want to "store" into it in sequence, and record the lseek() offsets before and after for each object. Then mmap the whole result, and stuff the appropriate pointers (based on arithmetic with the offsets) into the cached_objects list.

Quoted 5 lines

Cute. The remerge diff code path creates a full tree that records the mechanical merge result. By hooking into the lowest layer of write_object() interface, we'd serialize all objects in such a tree in the order they are computed (bottom up from the leaf level, I'd presume) into a single flat file ;-)

I do still like this approach, but just two possible gotchas I was thinking of:

- This side-steps all of our usual code for getting object data into memory. In general, I'd expect this content to not be too enormous, but it _could_ be if there are many / large blobs in the result. So we may end up with large maps. Probably not a big deal on modern 64-bit systems. Maybe an issue on 32-bit systems, just because of virtual address space.

Likewise, we do support systems with NO_MMAP. They'd work here, but it would probably mean putting all that object data into the heap. I could live with that, given how rare such systems are these days, and that it only matters if you're using --remerge-diff with big blobs.

Quoted 11 lines

- I wonder to what degree --remerge-diff benefits from omitting writes for objects we already have. I.e., if you are writing out a whole tree representing the conflicted state, then you don't want to write all of the trees that aren't interesting. Hopefully the code is already figuring out which paths the merge even touched, and ignoring the rest. It probably benefits performance-wise from write_object_file() deciding to skip some object writes, as well (e.g., for resolutions which the final tree already took, as they'd be in the merge commit). The whole pretend-we-have-this-object thing may want to likewise make sure we don't write out objects that we already have in the real odb.

I haven't benchmarked since my core.checkCollisions RFC patch[1] resulted in the somewhat related loose object cache patch from you, and not with something like the midx, but just a note that on some setups just writing things out is faster than exhaustively checking if we absolutely need to write things out.

I also wonder how much if anything writing out the one file v.s. lots of loose objects is worthwhile on systems where we could write out those loose objects on a ramdisk, which is commonly available on e.g. Linux distros these days out of the box. If you care about performance but not about your transitory data using a ramdisk is generally much better than any other potential I/O optimization.

Finally, and I don't mean to throw a monkey wrench into this whole discussion, so take this as a random musing: I wonder how much faster this thing could be on its second run if instead of avoiding writing to the store & cleaning up, it just wrote to the store, and then wrote

another object keyed on the git version and any revision paramaters etc., and then pretty much just had to do a "git cat-file -p <that-obj>" to present the result to the user :)

I suppose that would be throwing a lot more work at an eventual "git gc" than we ever do now, so maybe it's a bit crazy, but I think it might be an interesting direction in general to (ab)use either the primary or some secondary store in the .git dir as a semi-permanent cache of resolved queries from the likes of "git log".

Quoted 73 lines · Ævar Arnfjörð Bjarmason

Quoted 45 lines · Jeff King

Quoted 22 lines · Junio C Hamano

Quoted 13 lines · Jeff King

Side note: The pretend_object_file() approach is actually even better, because we know the object is fake. So it does not confuse write_object_file()'s "do we already have this object" freshening check.

I suspect it could even be made faster than the tmp_objdir approach. From our perspective, these objects really are tempfiles. So we could write them as such, not worrying about things like fsyncing them, naming them into place, etc. We could just write them out, then mmap the results, and put the pointers into cached_objects (currently it insists on malloc-ing a copy of the input buffer, but that seems like an easy extension to add).

Quoted 6 lines

In fact, I think you could get away with just _one_ tempfile per merge. Open up one tempfile. Write out all of the objects you want to "store" into it in sequence, and record the lseek() offsets before and after for each object. Then mmap the whole result, and stuff the appropriate pointers (based on arithmetic with the offsets) into the cached_objects list.

Quoted 5 lines

Cute. The remerge diff code path creates a full tree that records the mechanical merge result. By hooking into the lowest layer of write_object() interface, we'd serialize all objects in such a tree in the order they are computed (bottom up from the leaf level, I'd presume) into a single flat file ;-)

I do still like this approach, but just two possible gotchas I was thinking of:

- This side-steps all of our usual code for getting object data into memory. In general, I'd expect this content to not be too enormous, but it _could_ be if there are many / large blobs in the result. So we may end up with large maps. Probably not a big deal on modern 64-bit systems. Maybe an issue on 32-bit systems, just because of virtual address space.

Likewise, we do support systems with NO_MMAP. They'd work here, but it would probably mean putting all that object data into the heap. I could live with that, given how rare such systems are these days, and that it only matters if you're using --remerge-diff with big blobs.

Quoted 11 lines

- I wonder to what degree --remerge-diff benefits from omitting writes for objects we already have. I.e., if you are writing out a whole tree representing the conflicted state, then you don't want to write all of the trees that aren't interesting. Hopefully the code is already figuring out which paths the merge even touched, and ignoring the rest. It probably benefits performance-wise from write_object_file() deciding to skip some object writes, as well (e.g., for resolutions which the final tree already took, as they'd be in the merge commit). The whole pretend-we-have-this-object thing may want to likewise make sure we don't write out objects that we already have in the real odb.

I haven't benchmarked since my core.checkCollisions RFC patch[1] resulted in the somewhat related loose object cache patch from you, and not with something like the midx, but just a note that on some setups just writing things out is faster than exhaustively checking if we absolutely need to write things out.

I also wonder how much if anything writing out the one file v.s. lots of loose objects is worthwhile on systems where we could write out those loose objects on a ramdisk, which is commonly available on e.g. Linux distros these days out of the box. If you care about performance but not about your transitory data using a ramdisk is generally much better than any other potential I/O optimization.

Quoted 4 lines

Finally, and I don't mean to throw a monkey wrench into this whole discussion, so take this as a random musing: I wonder how much faster this thing could be on its second run if instead of avoiding writing to the store & cleaning up, it just wrote to the store, and then wrote

It'd be _much_ slower. My first implementation in fact did that; it just wrote objects to the store, left them there, and didn't bother to do any auto-gcs. It slowed down quite a bit as it ran. Adding auto-gc's during the run were really slow too. But stepping back, gc'ing objects that I already knew were garbage seemed like a waste; why not just prune them pre-emptively? To do so, though, I'd have to track all the individual objects I added to make sure I didn't prune something else. Following that idea and a few different attempts eventually led me to the discovery of tmp_objdir.

In case it's not clear to you why just writing all the objects to the normal store and leaving them there slows things down so much...

Let's say 1 in 10 merges had originally needed some kind of conflict resolution (either in the outer merge or in the inner merge for the virtual merge bases), meaning that 9 out of 10 merges traversed by --remerge-diff don't write any objects. Now for each merge for which --remerge-diff does need to create conflicted blobs and new trees, let's say it writes on average 3 blobs and 7 trees. (I don't know the real average numbers, it could well be ~5 total, but ~10 seems a realistic first order approximation and it makes the math easy.) Then, if we keep all objects we write, then git log --remerge-diff on a history with 100,000 merge commits, will have added 100,000 loose objects by the time it finishes. That means that all diff and merge operations slow down considerably as it runs due to all the extra loose objects.

Quoted 3 lines

another object keyed on the git version and any revision paramaters etc., and then pretty much just had to do a "git cat-file -p <that-obj>" to present the result to the user :)

So caching the full git log ... output, based on a hash of the command line flags, and then merely re-showing it later? And having that output be invalidated as soon as any head advances? Or are you thinking of caching the output per-commit based on a hash of the other command line flags...potentially slowing non-log operations down with the huge number of loose objects?

Quoted 5 lines

I suppose that would be throwing a lot more work at an eventual "git gc" than we ever do now, so maybe it's a bit crazy, but I think it might be an interesting direction in general to (ab)use either the primary or some secondary store in the .git dir as a semi-permanent cache of resolved queries from the likes of "git log".

If you do per-commit caching, and the user scrolls through enough output (not hard to do, just searching the output for some string often is enough), that "eventual" git-gc will be the very next git operation. If you cache the entire output, it'll be invalidated pretty quickly. So I don't see how this works. Or am I misunderstanding something you're suggesting here?

Quoted 106 lines · Elijah Newren

Quoted 73 lines · Ævar Arnfjörð Bjarmason

Quoted 45 lines · Jeff King

Quoted 22 lines · Junio C Hamano

Quoted 13 lines · Jeff King

Side note: The pretend_object_file() approach is actually even better, because we know the object is fake. So it does not confuse write_object_file()'s "do we already have this object" freshening check.

I suspect it could even be made faster than the tmp_objdir approach. From our perspective, these objects really are tempfiles. So we could write them as such, not worrying about things like fsyncing them, naming them into place, etc. We could just write them out, then mmap the results, and put the pointers into cached_objects (currently it insists on malloc-ing a copy of the input buffer, but that seems like an easy extension to add).

Quoted 6 lines

In fact, I think you could get away with just _one_ tempfile per merge. Open up one tempfile. Write out all of the objects you want to "store" into it in sequence, and record the lseek() offsets before and after for each object. Then mmap the whole result, and stuff the appropriate pointers (based on arithmetic with the offsets) into the cached_objects list.

Quoted 5 lines

Cute. The remerge diff code path creates a full tree that records the mechanical merge result. By hooking into the lowest layer of write_object() interface, we'd serialize all objects in such a tree in the order they are computed (bottom up from the leaf level, I'd presume) into a single flat file ;-)

I do still like this approach, but just two possible gotchas I was thinking of:

- This side-steps all of our usual code for getting object data into memory. In general, I'd expect this content to not be too enormous, but it _could_ be if there are many / large blobs in the result. So we may end up with large maps. Probably not a big deal on modern 64-bit systems. Maybe an issue on 32-bit systems, just because of virtual address space.

Likewise, we do support systems with NO_MMAP. They'd work here, but it would probably mean putting all that object data into the heap. I could live with that, given how rare such systems are these days, and that it only matters if you're using --remerge-diff with big blobs.

Quoted 11 lines

- I wonder to what degree --remerge-diff benefits from omitting writes for objects we already have. I.e., if you are writing out a whole tree representing the conflicted state, then you don't want to write all of the trees that aren't interesting. Hopefully the code is already figuring out which paths the merge even touched, and ignoring the rest. It probably benefits performance-wise from write_object_file() deciding to skip some object writes, as well (e.g., for resolutions which the final tree already took, as they'd be in the merge commit). The whole pretend-we-have-this-object thing may want to likewise make sure we don't write out objects that we already have in the real odb.

I haven't benchmarked since my core.checkCollisions RFC patch[1] resulted in the somewhat related loose object cache patch from you, and not with something like the midx, but just a note that on some setups just writing things out is faster than exhaustively checking if we absolutely need to write things out.

I also wonder how much if anything writing out the one file v.s. lots of loose objects is worthwhile on systems where we could write out those loose objects on a ramdisk, which is commonly available on e.g. Linux distros these days out of the box. If you care about performance but not about your transitory data using a ramdisk is generally much better than any other potential I/O optimization.

Quoted 4 lines

Finally, and I don't mean to throw a monkey wrench into this whole discussion, so take this as a random musing: I wonder how much faster this thing could be on its second run if instead of avoiding writing to the store & cleaning up, it just wrote to the store, and then wrote

It'd be _much_ slower. My first implementation in fact did that; it just wrote objects to the store, left them there, and didn't bother to do any auto-gcs. It slowed down quite a bit as it ran. Adding auto-gc's during the run were really slow too. But stepping back, gc'ing objects that I already knew were garbage seemed like a waste; why not just prune them pre-emptively? To do so, though, I'd have to track all the individual objects I added to make sure I didn't prune something else. Following that idea and a few different attempts eventually led me to the discovery of tmp_objdir.

In case it's not clear to you why just writing all the objects to the normal store and leaving them there slows things down so much...

Let's say 1 in 10 merges had originally needed some kind of conflict resolution (either in the outer merge or in the inner merge for the virtual merge bases), meaning that 9 out of 10 merges traversed by --remerge-diff don't write any objects. Now for each merge for which --remerge-diff does need to create conflicted blobs and new trees, let's say it writes on average 3 blobs and 7 trees. (I don't know the real average numbers, it could well be ~5 total, but ~10 seems a realistic first order approximation and it makes the math easy.) Then, if we keep all objects we write, then git log --remerge-diff on a history with 100,000 merge commits, will have added 100,000 loose objects by the time it finishes. That means that all diff and merge operations slow down considerably as it runs due to all the extra loose objects.

...

Quoted 10 lines

Quoted 3 lines

another object keyed on the git version and any revision paramaters etc., and then pretty much just had to do a "git cat-file -p <that-obj>" to present the result to the user :)

So caching the full git log ... output, based on a hash of the command line flags, and then merely re-showing it later? And having that output be invalidated as soon as any head advances? Or are you thinking of caching the output per-commit based on a hash of the other command line flags...potentially slowing non-log operations down with the huge number of loose objects?

Yeah I meant caching the full 'git log' output. Anyway, I think what you said above about number of loose objects clearly makes that a stupid suggestion on my part.

FWIW I meant that more as a "maybe in the future we can ..." musing than anything for this series. I.e. in general I've often wanted say the second run of a 'git log -G<rx>"' to be faster than it is, which could use such on-the-fly caching.

But if we had such caching then something like --remerge-diff would need to (from my understanding of what you're saying) need both this temporary staging area for objects and perhaps to cache the output in the main (or some cache) store. I.e. they're orthagonal.

Quoted 6 lines

Quoted 5 lines

I suppose that would be throwing a lot more work at an eventual "git gc" than we ever do now, so maybe it's a bit crazy, but I think it might be an interesting direction in general to (ab)use either the primary or some secondary store in the .git dir as a semi-permanent cache of resolved queries from the likes of "git log".

Quoted 6 lines

If you do per-commit caching, and the user scrolls through enough output (not hard to do, just searching the output for some string often is enough), that "eventual" git-gc will be the very next git operation. If you cache the entire output, it'll be invalidated pretty quickly. So I don't see how this works. Or am I misunderstanding something you're suggesting here?

With the caveat above that I think this is all a pretty stupid suggestion on my part:

Just on the narrow aspect of how "git gc" behaves in that scenario: We'd keep those objects around for 2 weeks by default, or whatever you have gc.pruneExpire set to.

So such a setup would certainly cause lots of pathological issues (see my [1] for one), but I don't think over-eager expiry of loose objects would be one.

I'm not sure but are you referring to "invalidated pretty quickly" because it would go over the gc.auto limit? If so that's now how it works in this scenario, see [2]. I.e. it's moderated by the gc.pruneExpire setting.

1. https://lore.kernel.org/git/[email protected]/
2. https://lore.kernel.org/git/[email protected]/

Quoted 22 lines · Junio C Hamano

Quoted 13 lines · Jeff King

Side note: The pretend_object_file() approach is actually even better, because we know the object is fake. So it does not confuse write_object_file()'s "do we already have this object" freshening check.

I suspect it could even be made faster than the tmp_objdir approach. From our perspective, these objects really are tempfiles. So we could write them as such, not worrying about things like fsyncing them, naming them into place, etc. We could just write them out, then mmap the results, and put the pointers into cached_objects (currently it insists on malloc-ing a copy of the input buffer, but that seems like an easy extension to add).

Quoted 6 lines

In fact, I think you could get away with just _one_ tempfile per merge. Open up one tempfile. Write out all of the objects you want to "store" into it in sequence, and record the lseek() offsets before and after for each object. Then mmap the whole result, and stuff the appropriate pointers (based on arithmetic with the offsets) into the cached_objects list.

Quoted 5 lines

Cute. The remerge diff code path creates a full tree that records the mechanical merge result. By hooking into the lowest layer of write_object() interface, we'd serialize all objects in such a tree in the order they are computed (bottom up from the leaf level, I'd presume) into a single flat file ;-)

I do still like this approach, but just two possible gotchas I was thinking of:

- This side-steps all of our usual code for getting object data into memory. In general, I'd expect this content to not be too enormous, but it _could_ be if there are many / large blobs in the result. So we may end up with large maps. Probably not a big deal on modern 64-bit systems. Maybe an issue on 32-bit systems, just because of virtual address space.

Likewise, we do support systems with NO_MMAP. They'd work here, but it would probably mean putting all that object data into the heap. I could live with that, given how rare such systems are these days, and that it only matters if you're using --remerge-diff with big blobs.

- I wonder to what degree --remerge-diff benefits from omitting writes for objects we already have. I.e., if you are writing out a whole tree representing the conflicted state, then you don't want to write all of the trees that aren't interesting. Hopefully the code is already figuring out which paths the merge even touched, and ignoring the rest.

It probably benefits performance-wise from write_object_file() deciding to skip some object writes, as well (e.g., for resolutions which the final tree already took, as they'd be in the merge commit).

The whole pretend-we-have-this-object thing
may want to likewise make sure we don't write out objects that we already have in the real odb.

Quoted 11 lines

- This side-steps all of our usual code for getting object data into memory. In general, I'd expect this content to not be too enormous, but it _could_ be if there are many / large blobs in the result. So we may end up with large maps. Probably not a big deal on modern 64-bit systems. Maybe an issue on 32-bit systems, just because of virtual address space.

Likewise, we do support systems with NO_MMAP. They'd work here, but it would probably mean putting all that object data into the heap. I could live with that, given how rare such systems are these days, and that it only matters if you're using --remerge-diff with big blobs.

Um, I'm starting to get uncomfortable with this pretend_object stuff. Part of the reason that merge-ort isn't truly "in memory" despite attempting to do exactly that, was because for large enough repos with enough files modified on both sides, I wasn't comfortable assuming that all new files from three-way content merges and all new trees fit into memory. I'm sure we'd be fine with current-day linux kernel sized repos. No big deal. In fact, most merges probably don't add more than a few dozen new files. But for microsoft-sized repos, and with repos tending to grow over time, more so when the tools themselves scale nicely (which we've all been working on enabling), makes me worry there might be enough new objects within a single merge (especially given the recursive inner merges) that we might need to worry about this.

Quoted 3 lines

The whole pretend-we-have-this-object thing
may want to likewise make sure we don't write out objects that we already have in the real odb.

Right, so I'd have to copy the relevant logic from write_object_file() -- I think that means instead of write_object_file()'s calls to freshen_packed_object() and freshen_loose_object() that I instead call find_pack_entry() and make has_loose_object() in object-file.c not be static and then call it. Does that sound right?

Quoted 12 lines · Elijah Newren

Quoted 11 lines

- This side-steps all of our usual code for getting object data into memory. In general, I'd expect this content to not be too enormous, but it _could_ be if there are many / large blobs in the result. So we may end up with large maps. Probably not a big deal on modern 64-bit systems. Maybe an issue on 32-bit systems, just because of virtual address space.

Likewise, we do support systems with NO_MMAP. They'd work here, but it would probably mean putting all that object data into the heap. I could live with that, given how rare such systems are these days, and that it only matters if you're using --remerge-diff with big blobs.

Quoted 13 lines

Um, I'm starting to get uncomfortable with this pretend_object stuff. Part of the reason that merge-ort isn't truly "in memory" despite attempting to do exactly that, was because for large enough repos with enough files modified on both sides, I wasn't comfortable assuming that all new files from three-way content merges and all new trees fit into memory. I'm sure we'd be fine with current-day linux kernel sized repos. No big deal. In fact, most merges probably don't add more than a few dozen new files. But for microsoft-sized repos, and with repos tending to grow over time, more so when the tools themselves scale nicely (which we've all been working on enabling), makes me worry there might be enough new objects within a single merge (especially given the recursive inner merges) that we might need to worry about this.

I do think we need to consider that the content might be larger than will comfortably fit in memory. But the point of using mmap is that we don't have to care. The OS is taking care of it for us (just like it would in regular object files).

The question is just whether we're comfortable assuming that mmap exists if you're working on such a large repository. I'd guess that big repos are pretty painful with out it (and again, I'm not even clear which systems Git runs on even lack mmap these days). So IMHO this isn't really a blocker for going in this direction.

Quoted 8 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

The tmp_objdir API provides the ability to create temporary object directories, but was designed with the goal of having subprocesses access these object stores, followed by the main process migrating objects from it to the main object store or just deleting it. The subprocesses would view it as their primary datastore and write to it.

Quoted 6 lines

For the --remerge-diff option we want to add to show & log, we want all writes of intermediate merge results (both blobs and trees) to go to this alternate object store; since those writes will be done by the main process, we need this "alternate" object store to actually be the primary object store. When show & log are done, they'll simply remove this temporary object store.

I think this is consistent with the original design of tmp_objdir. I just never needed to do anything in-process before, so overriding the environment of sub-processes was sufficient.

I do think these are dangerous and may cause bugs, though. Anything you write while the tmp_objdir is marked as "primary" is going to go away when you remove it. So any object names you reference outside of that, either:

- by another object that you create after the tmp_objdir is removed; or

- in a ref

are going to turn into repository corruption. Of course that's true for the existing sub-processes, too, but here we're touching a wider variety of code.

Obviously the objects we write as part of remerge-diff are meant to be temporary, and we'll never reference them in any other way. And usually we would not expect a diff to write any other objects. But one thing that comes to mind if textconv caching.

If you do a remerge diff on a blob that uses textconv, and the caching feature is turned on, then we'll write out a new blob with the cached value, and eventually a new tree and refs/notes/ pointer referencing it. I'm not sure of the timing of all of that, but it seems likely to me that at least some of that will end up in your tmp_objdir.

If you remove the tmp_objdir as the primary as soon as you're done with the merge, but before you run the diff, you might be OK, though.

If not, then I think the solution is probably not to install this as the "primary", but rather:

- do the specific remerge-diff writes we want using a special "write to this object dir" variant of write_object_file()

- install the tmp_objdir as an alternate, so the reading side (which is diff code that doesn't otherwise know about our remerge-diff trickery) can find them

And that lets other writers avoid writing into the temporary area accidentally.

In that sense this is kind of like the pretend_object_file() interface, except that it's storing the objects on disk instead of in memory. Of course another way of doing that would be to stuff the object data into tempfiles and just put pointers into the in-memory cached_objects array.

It's also not entirely foolproof (nor is the existing pretend_object_file()). Any operation which is fooled into thinking we have object X because it's in the fake-object list or the tmp_objdir may reference that object erroneously, creating a corruption. But it's still safer than allowing arbitrary writes into the tmp_objdir.

Side note: The pretend_object_file() approach is actually even better, because we know the object is fake. So it does not confuse write_object_file()'s "do we already have this object" freshening check.

I suspect it could even be made faster than the tmp_objdir approach. From our perspective, these objects really are tempfiles. So we could write them as such, not worrying about things like fsyncing them, naming them into place, etc. We could just write them out, then mmap the results, and put the pointers into cached_objects (currently it insists on malloc-ing a copy of the input buffer, but that seems like an easy extension to add).

In fact, I think you could get away with just _one_ tempfile per merge. Open up one tempfile. Write out all of the objects you want to "store" into it in sequence, and record the lseek() offsets before and after for each object. Then mmap the whole result, and stuff the appropriate pointers (based on arithmetic with the offsets) into the cached_objects list.

As a bonus, this tempfile can easily be in $TMPDIR, meaning remerge-diff could work on a read-only repository (and the OS can perhaps handle the data better, especially if $TMPDIR isn't a regular filesystem).

Quoted 5 lines

We also need one more thing -- git log --remerge-diff can cause the temporary object store to fill up with loose objects. Rather than trying to gc that are known garbage anyway, we simply want to know the location of the temporary object store so we can purge the loose objects after each merge.

This paragraph confused me. At first I thought you were talking about how to avoid calling "gc --auto" because we don't want to waste time thinking all those loose objects were worth gc-ing. But we wouldn't do that anyway (git-log does not expect to make objects so doesn't call it at all, and anyway you'd expect it to happen at the end of a process, after we've already removed the tmp_objdir).

But I think you just mean: we can build up a bunch of loose objects, which is inefficient. We don't want to gc them, because that's even less efficient. So we want to clean them out between merges.

But I don't see any code to do that here. I guess that's maybe why you've added tmp_objdir_path(), to find them later. But IMHO this would be much better encapsulated as tmp_objdir_clear_objects() or something.

But simpler still: is there any reason not to just drop and re-create the tmp-objdir for each merge? It's not very expensive to do so (and certainly not compared to the cost of actually writing out the objects).

So there are other parts of running git log that are not read-only, besides what I'm doing. That's unfortunate. Can we just disable those things in this section of the code?

Also, I think it makes sense that textconv caching causes a problem via writing new blobs and trees *and* refs that reference these blobs and trees. However, I'm not sure I'm really grasping it, because I don't see how your solutions are safe either. I'll mention more after each one.

Quoted 2 lines

If you remove the tmp_objdir as the primary as soon as you're done with the merge, but before you run the diff, you might be OK, though.

It has to be after I run the diff, because the diff needs access to the temporary files to diff against them.

Quoted 6 lines

If not, then I think the solution is probably not to install this as the "primary", but rather:

- do the specific remerge-diff writes we want using a special "write to this object dir" variant of write_object_file()

Quoted 6 lines

- install the tmp_objdir as an alternate, so the reading side (which is diff code that doesn't otherwise know about our remerge-diff trickery) can find them

And that lets other writers avoid writing into the temporary area accidentally.

Doesn't this have the same problem? If something similar to textconv caching were to create new trees that referenced the existing blobs and then added a ref that referred to that tree, then you have the exact same problem, right? The new tree and the new ref would be corrupt as soon as the tmp-objdir went away. Whether or not other callers write to the temporary area isn't the issue, it's whether they write refs that can refer to anything from the temporary area. Or am I missing something?

Quoted 10 lines

In that sense this is kind of like the pretend_object_file() interface, except that it's storing the objects on disk instead of in memory. Of course another way of doing that would be to stuff the object data into tempfiles and just put pointers into the in-memory cached_objects array.

It's also not entirely foolproof (nor is the existing pretend_object_file()). Any operation which is fooled into thinking we have object X because it's in the fake-object list or the tmp_objdir may reference that object erroneously, creating a corruption. But it's still safer than allowing arbitrary writes into the tmp_objdir.

Why is the pretend_object_file() interface safer? Does it disable the textconv caching somehow? I don't see why it helps avoid this problem.

Interesting. I'm not familiar with the pretend_object_file() code, but I think I catch what you're saying. I don't see anything in object-file.c that allows removing items from the cache, as I would want to do, but I'm sure I could add something.

(I'm still not sure how this avoids the problem of things like textconv caching trying to write an object that references one of these, though. Should we just manually disable textconv caching during a remerge-diff?)

Quoted 5 lines

Quoted 2 lines

If you remove the tmp_objdir as the primary as soon as you're done with the merge, but before you run the diff, you might be OK, though.

It has to be after I run the diff, because the diff needs access to the temporary files to diff against them.

Right, of course. I was too fixated on the object-write part, forgetting that the whole point of the exercise is to later read them back. :)

- is it possible for the merge code to ever write an object? I kind of wonder if we'd ever do any cache-able transformations as part of a content-level merge. I don't think we do now, though.

Quoted 3 lines · Jeff King

- is it possible for the merge code to ever write an object? I kind of wonder if we'd ever do any cache-able transformations as part of a content-level merge. I don't think we do now, though.

How is virtual merge base, result of an inner merge that recursively merging two merge bases, fed to an outer merge as the ancestor? Isn't it written as a tree object by the inner merge as a tree with full of blob objects, so the outer merge does not have to treat a merge base tree that is virtual any specially from a sole merge base?

Quoted 7 lines · Junio C Hamano

Quoted 3 lines · Jeff King

- is it possible for the merge code to ever write an object? I kind of wonder if we'd ever do any cache-able transformations as part of a content-level merge. I don't think we do now, though.

Quoted 6 lines

How is virtual merge base, result of an inner merge that recursively merging two merge bases, fed to an outer merge as the ancestor? Isn't it written as a tree object by the inner merge as a tree with full of blob objects, so the outer merge does not have to treat a merge base tree that is virtual any specially from a sole merge base?

Yes. And that's not unique to the inner merges; it also writes new blob and tree objects for the outer merge, and then passes the resulting toplevel tree id out as the result.

Quoted 8 lines · Jeff King

Quoted 5 lines

Quoted 2 lines

If you remove the tmp_objdir as the primary as soon as you're done with the merge, but before you run the diff, you might be OK, though.

It has to be after I run the diff, because the diff needs access to the temporary files to diff against them.

Right, of course. I was too fixated on the object-write part, forgetting that the whole point of the exercise is to later read them back. :)

Ah, no, I remember what I was trying to say here. The distinction is between "remove the tmp_objdir" and "remove it as the primary".

I.e., if you do this:

1. create tmp_objdir

2. make tmp_objdir primary for writes

3. run the "merge" half of remerge-diff, writing objects into the temporary space

4. stop having tmp_objdir as the primary; instead make it an alternate

5. run the diff

6. remove tmp_objdir totally

Then step 5 can't accidentally write objects into the temporary space, but it can still read them. So it's not entirely safe, but it's safer, and it would be a much smaller change.

Some ways it could go wrong:

- is it possible for the merge code to ever write an object? I kind of wonder if we'd ever do any cache-able transformations as part of a content-level merge. I don't think we do now, though.

- in step 5, write_object_file() may still be confused by the presence of the to-be-thrown-away objects in the alternate. This is pretty unlikely, as it implies that the remerge-diff wrote a blob or tree that is byte-identical to something that the diff wants to write.

Quoted 5 lines

Some ways it could go wrong:

- is it possible for the merge code to ever write an object? I kind of wonder if we'd ever do any cache-able transformations as part of a content-level merge. I don't think we do now, though.

Yes, of course -- otherwise there would have been no need for the tmp_objdir in the first place. In particular, it needs to write three-way-content merges of individual files, and it needs to write new tree objects. (And it needs to do this both for creating the virtual merge bases if the merge is recursive, as well as doing it for the outer merge.)

Quoted 4 lines

- in step 5, write_object_file() may still be confused by the presence of the to-be-thrown-away objects in the alternate. This is pretty unlikely, as it implies that the remerge-diff wrote a blob or tree that is byte-identical to something that the diff wants to write.

That's one reason it could be confused. The textconv filtering in particular was creating a new object based on an existing one, and a tree, and a ref. What if there was some other form of caching or statistic gathering that didn't write a new object based on an existing one, but did add trees and especially refs that referenced the existing object? It's not that the diff wanted to write something byte-identical to what the merge wrote, it's just that the diff wants to reference the object somehow.

Quoted 3 lines · Elijah Newren

So there are other parts of running git log that are not read-only, besides what I'm doing. That's unfortunate. Can we just disable those things in this section of the code?

It might be possible for the cache (we'd still generate entries, but just not write them). It's "just" an optimization, though it's possible there are cases where it's really annoying to do so. I'd imagine those are a stretch. I do use textconv to decrypt gpg blobs with a key on a hardware token, for example. I don't require a touch for each one, though, nor would I use textconv caching anyway (since the point is not to store the unencrypted contents).

It wouldn't necessarily be OK to disable object writes in general. Though I'd think since a diff is _conceptually_ a read-only operation that any writes it wants to do would be in the same caching / optimization boat. I'd also worry about the maintenance cost of having to annotate any such case.

Quoted 5 lines

Also, I think it makes sense that textconv caching causes a problem via writing new blobs and trees *and* refs that reference these blobs and trees. However, I'm not sure I'm really grasping it, because I don't see how your solutions are safe either. I'll mention more after each one.

Heh. So I originally wrote more on this in my earlier email, but worried I was getting too into the weeds and would confuse you, so I pared it down. But I managed to be confusing anyway. :)

Here's one mental model. If you have a temporary object store, then these are the bad things that can happen:

1. Some code may write an object to it, thinking the data is saved, but we later throw it away.

2. Some code may _skip_ writing an object, thinking that we already have that object available (when in fact we'll later throw it away).

3. Some code may record the fact that we have an object in memory, and then later (after the temporary object store is not in use), may point to it when writing an object or reference.

Pointing the primary object-dir at a temporary directory (like your patch does) means we're subject to all three.

Problem (1) happens because unrelated code may not realize we've swapped out the primary object writing code for this throw-away directory. But we could require writers to explicitly indicate that they want to write to the temporary area, while allowing other writes to go to the regular object store. This could be done with a flag or variant of write_object_file(). Or if the temporary area isn't a regular object directory at all (like the whole tempfile / pretend thing), then this happens automatically, because the regular object-write paths don't even know about our temporary area.

Problem (2) is mostly handled inside write_object_file(). It will optimize out writes of objects we already have. So it needs to know when we have an object "for real", and when we just have it in our temporary store. If we make the tmp_objdir the primary, then it doesn't know this (though we could teach it to check. In the pretend_object_file() system, it already knows this (it uses the "freshen" code paths which actually want to find the entry in the filesystem).

Problem (3) is the hardest one, because we don't really distinguish between readers and writers. So if I call has_object_file(), is it because I want to access the object? Or is it because I want to generate a new reference to it, which must ensure that it exists? One of those is OK to look at the tmp_objdir (and indeed, is the whole point of making it in the first place), and the other is the first step to corrupting the repository. ;)

With the environment variables we set up for running external commands (like hooks) in the receive-pack quarantine, one bit of safety we have is to prevent ref writes entirely in those commands. That works reasonably well. Even if they may write new objects that might get thrown away (if the push is rejected), they'd ultimately need to point to those objects with a ref. And because those hooks spend their _entire_ run-time in the quarantine state, they can't ever write a ref.

Whereas doing it in-process is a little dicier. Right now the textconv cache writes out a new tree for every entry we add, so it happens to do the ref write while the tmp-objdir is still in place. But as this comment notes:

  $ git grep -B6 notes_cache_write diff.c
  diff.c-         /*
  diff.c-          * we could save up changes and flush them all at the end,
  diff.c-          * but we would need an extra call after all diffing is done.
  diff.c-          * Since generating a cache entry is the slow path anyway,
  diff.c-          * this extra overhead probably isn't a big deal.
  diff.c-          */
  diff.c:         notes_cache_write(textconv->cache);

this is slow, and it would be perfectly reasonable to flush the cache at the end of the process (e.g., using an atexit() handler). In which case we'd hit this problem (3) exactly: we'd generate and remember objects during the tmp-objdir period, but only later actually reference them. This is more likely than the receive-pack hook case, because we're doing it all in process.

Quoted 5 lines

Quoted 2 lines

If you remove the tmp_objdir as the primary as soon as you're done with the merge, but before you run the diff, you might be OK, though.

It has to be after I run the diff, because the diff needs access to the temporary files to diff against them.

Right, of course. I was too fixated on the object-write part, forgetting that the whole point of the exercise is to later read them back. :)

Quoted 13 lines

Quoted 6 lines

If not, then I think the solution is probably not to install this as the "primary", but rather:

- do the specific remerge-diff writes we want using a special "write to this object dir" variant of write_object_file()

Quoted 6 lines

- install the tmp_objdir as an alternate, so the reading side (which is diff code that doesn't otherwise know about our remerge-diff trickery) can find them

And that lets other writers avoid writing into the temporary area accidentally.

Quoted 8 lines

Doesn't this have the same problem? If something similar to textconv caching were to create new trees that referenced the existing blobs and then added a ref that referred to that tree, then you have the exact same problem, right? The new tree and the new ref would be corrupt as soon as the tmp-objdir went away. Whether or not other callers write to the temporary area isn't the issue, it's whether they write refs that can refer to anything from the temporary area. Or am I missing something?

The key thing here is in the first step, where remerge-diff is explicitly saying "I want to write to this temporary object-dir". But the textconv-cache code does not; it gets to write to the normal spot. So it avoids problem (1).

You're right that it does not avoid problem (3) exactly. But triggering that would require some code not just writing other objects or references while the tmp-objdir is in place, but specifically referencing the objects that remerge-diff did put into the tmp-objdir. That seems a lot less likely to me (because the thing we're most worried about is unrelated code that just happens to write while the tmp-objdir is in place).

Quoted 11 lines

Quoted 10 lines

In that sense this is kind of like the pretend_object_file() interface, except that it's storing the objects on disk instead of in memory. Of course another way of doing that would be to stuff the object data into tempfiles and just put pointers into the in-memory cached_objects array.

It's also not entirely foolproof (nor is the existing pretend_object_file()). Any operation which is fooled into thinking we have object X because it's in the fake-object list or the tmp_objdir may reference that object erroneously, creating a corruption. But it's still safer than allowing arbitrary writes into the tmp_objdir.

Quoted 3 lines

Why is the pretend_object_file() interface safer? Does it disable the textconv caching somehow? I don't see why it helps avoid this problem.

So hopefully it's clearer now from what I wrote above, but just connecting the dots:

1. Unrelated code calling write_object_file() would still write real, durable objects, as usual.

2. The write_object_file() "skip this write" optimization already ignores the pretend_object_file() objects while checking "do we have this object".

Quoted 4 lines

Interesting. I'm not familiar with the pretend_object_file() code, but I think I catch what you're saying. I don't see anything in object-file.c that allows removing items from the cache, as I would want to do, but I'm sure I could add something.

Right, it's pretty limited now, and there's only one caller. And in fact I've wanted to get rid of it, exactly because it can create the danger we're discussing here. But I still think it's _less_ dangerous than fully replacing the object directory.

Quoted 4 lines

(I'm still not sure how this avoids the problem of things like textconv caching trying to write an object that references one of these, though. Should we just manually disable textconv caching during a remerge-diff?)

It can't avoid it completely, but in general, the textconv cache should be writing and referencing its own objects. Even if they happen to be the same as something remerge-diff generated, it won't know that until it has tried to write it (and so that's why having write_object_file() continue to really write the object is important).

Hopefully that clears up my line of thinking. I do think a lot of this is kind of theoretical, in the sense that:

- textconv caching is an obscure feature that we _could_ just disable during remerge-diffs

- there's a reasonable chance that there isn't any other code that wants to write during a diff

But this is all scary and error-prone enough that I'd prefer an approach that has the "least surprise". So any solution where random code calling write_object_file() _can't_ accidentally write to a throw-away directory seems like a safer less surprising thing.

It does mean that the remerge-diff code needs to be explicit in its object writes to say "this needs to go to the temporary area" (whether it's a flag, or calling into a totally different function or subsystem). I'm hoping that's not hard to do (because its writes are done explicitly by the remerge-diff code), but I worry that it may be (because you are relying on more generic tree code to the write under the hood).

Is there state we care about besides objects and refs? Maybe we need to introduce a transactional system for the entire repository like we have for the refs-db. In-process code that cares about oids that might be only transactionally present can register a callback for when the transaction commits or rollsback. I'm not volunteering to write this, btw :).

We can make the tmp object directory stuff more prevalent by using it in git-apply and the merge stategy, and by using bulk-checkin while writing out the cached tree and commits. This should flush out any issues and keep other parts of the codebase honest.

Quoted 6 lines

Quoted 6 lines

If not, then I think the solution is probably not to install this as the "primary", but rather:

- do the specific remerge-diff writes we want using a special "write to this object dir" variant of write_object_file()

Quoted 8 lines

Quoted 6 lines

- install the tmp_objdir as an alternate, so the reading side (which is diff code that doesn't otherwise know about our remerge-diff trickery) can find them

And that lets other writers avoid writing into the temporary area accidentally.

...

The key thing here is in the first step, where remerge-diff is explicitly saying "I want to write to this temporary object-dir". But the textconv-cache code does not; it gets to write to the normal spot. So it avoids problem (1).

You're right that it does not avoid problem (3) exactly. But triggering that would require some code not just writing other objects or references while the tmp-objdir is in place, but specifically referencing the objects that remerge-diff did put into the tmp-objdir. That seems a lot less likely to me (because the thing we're most worried about is unrelated code that just happens to write while the tmp-objdir is in place).

So hopefully it's clearer now from what I wrote above, but just connecting the dots:

1. Unrelated code calling write_object_file() would still write real, durable objects, as usual.

2. The write_object_file() "skip this write" optimization already ignores the pretend_object_file() objects while checking "do we have this object".

But this is all scary and error-prone enough that I'd prefer an approach that has the "least surprise". So any solution where random code calling write_object_file() _can't_ accidentally write to a throw-away directory seems like a safer less surprising thing.

It does mean that the remerge-diff code needs to be explicit in its object writes to say "this needs to go to the temporary area" (whether it's a flag, or calling into a totally different function or subsystem).

I'm hoping that's not hard to do (because its writes are done explicitly by the remerge-diff code), but I worry that it may be (because you are relying on more generic tree code to the write under the hood).

Quoted 3 lines · Elijah Newren

So there are other parts of running git log that are not read-only, besides what I'm doing. That's unfortunate. Can we just disable those things in this section of the code?

It might be possible for the cache (we'd still generate entries, but just not write them). It's "just" an optimization, though it's possible there are cases where it's really annoying to do so. I'd imagine those are a stretch. I do use textconv to decrypt gpg blobs with a key on a hardware token, for example. I don't require a touch for each one, though, nor would I use textconv caching anyway (since the point is not to store the unencrypted contents).

It wouldn't necessarily be OK to disable object writes in general. Though I'd think since a diff is _conceptually_ a read-only operation that any writes it wants to do would be in the same caching / optimization boat. I'd also worry about the maintenance cost of having to annotate any such case.

Quoted 5 lines

Also, I think it makes sense that textconv caching causes a problem via writing new blobs and trees *and* refs that reference these blobs and trees. However, I'm not sure I'm really grasping it, because I don't see how your solutions are safe either. I'll mention more after each one.

Heh. So I originally wrote more on this in my earlier email, but worried I was getting too into the weeds and would confuse you, so I pared it down. But I managed to be confusing anyway. :)

Here's one mental model. If you have a temporary object store, then these are the bad things that can happen:

1. Some code may write an object to it, thinking the data is saved, but we later throw it away.

2. Some code may _skip_ writing an object, thinking that we already have that object available (when in fact we'll later throw it away).

3. Some code may record the fact that we have an object in memory, and then later (after the temporary object store is not in use), may point to it when writing an object or reference.

Pointing the primary object-dir at a temporary directory (like your patch does) means we're subject to all three.

Problem (1) happens because unrelated code may not realize we've swapped out the primary object writing code for this throw-away directory. But we could require writers to explicitly indicate that they want to write to the temporary area, while allowing other writes to go to the regular object store. This could be done with a flag or variant of write_object_file(). Or if the temporary area isn't a regular object directory at all (like the whole tempfile / pretend thing), then this happens automatically, because the regular object-write paths don't even know about our temporary area.

Problem (2) is mostly handled inside write_object_file(). It will optimize out writes of objects we already have. So it needs to know when we have an object "for real", and when we just have it in our temporary store. If we make the tmp_objdir the primary, then it doesn't know this (though we could teach it to check. In the pretend_object_file() system, it already knows this (it uses the "freshen" code paths which actually want to find the entry in the filesystem).

Problem (3) is the hardest one, because we don't really distinguish between readers and writers. So if I call has_object_file(), is it because I want to access the object? Or is it because I want to generate a new reference to it, which must ensure that it exists? One of those is OK to look at the tmp_objdir (and indeed, is the whole point of making it in the first place), and the other is the first step to corrupting the repository. ;)

With the environment variables we set up for running external commands (like hooks) in the receive-pack quarantine, one bit of safety we have is to prevent ref writes entirely in those commands. That works reasonably well. Even if they may write new objects that might get thrown away (if the push is rejected), they'd ultimately need to point to those objects with a ref. And because those hooks spend their _entire_ run-time in the quarantine state, they can't ever write a ref.

Whereas doing it in-process is a little dicier. Right now the textconv cache writes out a new tree for every entry we add, so it happens to do the ref write while the tmp-objdir is still in place. But as this comment notes:

  $ git grep -B6 notes_cache_write diff.c
  diff.c-         /*
  diff.c-          * we could save up changes and flush them all at the end,
  diff.c-          * but we would need an extra call after all diffing is done.
  diff.c-          * Since generating a cache entry is the slow path anyway,
  diff.c-          * this extra overhead probably isn't a big deal.
  diff.c-          */
  diff.c:         notes_cache_write(textconv->cache);

this is slow, and it would be perfectly reasonable to flush the cache at the end of the process (e.g., using an atexit() handler). In which case we'd hit this problem (3) exactly: we'd generate and remember objects during the tmp-objdir period, but only later actually reference them. This is more likely than the receive-pack hook case, because we're doing it all in process.

Quoted 5 lines

Quoted 2 lines

If you remove the tmp_objdir as the primary as soon as you're done with the merge, but before you run the diff, you might be OK, though.

It has to be after I run the diff, because the diff needs access to the temporary files to diff against them.

Right, of course. I was too fixated on the object-write part, forgetting that the whole point of the exercise is to later read them back. :)

Quoted 13 lines

Quoted 6 lines

If not, then I think the solution is probably not to install this as the "primary", but rather:

- do the specific remerge-diff writes we want using a special "write to this object dir" variant of write_object_file()

Quoted 6 lines

- install the tmp_objdir as an alternate, so the reading side (which is diff code that doesn't otherwise know about our remerge-diff trickery) can find them

And that lets other writers avoid writing into the temporary area accidentally.

Quoted 8 lines

Doesn't this have the same problem? If something similar to textconv caching were to create new trees that referenced the existing blobs and then added a ref that referred to that tree, then you have the exact same problem, right? The new tree and the new ref would be corrupt as soon as the tmp-objdir went away. Whether or not other callers write to the temporary area isn't the issue, it's whether they write refs that can refer to anything from the temporary area. Or am I missing something?

The key thing here is in the first step, where remerge-diff is explicitly saying "I want to write to this temporary object-dir". But the textconv-cache code does not; it gets to write to the normal spot. So it avoids problem (1).

You're right that it does not avoid problem (3) exactly. But triggering that would require some code not just writing other objects or references while the tmp-objdir is in place, but specifically referencing the objects that remerge-diff did put into the tmp-objdir. That seems a lot less likely to me (because the thing we're most worried about is unrelated code that just happens to write while the tmp-objdir is in place).

Quoted 11 lines

Quoted 10 lines

In that sense this is kind of like the pretend_object_file() interface, except that it's storing the objects on disk instead of in memory. Of course another way of doing that would be to stuff the object data into tempfiles and just put pointers into the in-memory cached_objects array.

It's also not entirely foolproof (nor is the existing pretend_object_file()). Any operation which is fooled into thinking we have object X because it's in the fake-object list or the tmp_objdir may reference that object erroneously, creating a corruption. But it's still safer than allowing arbitrary writes into the tmp_objdir.

Quoted 3 lines

Why is the pretend_object_file() interface safer? Does it disable the textconv caching somehow? I don't see why it helps avoid this problem.

So hopefully it's clearer now from what I wrote above, but just connecting the dots:

1. Unrelated code calling write_object_file() would still write real, durable objects, as usual.

2. The write_object_file() "skip this write" optimization already ignores the pretend_object_file() objects while checking "do we have this object".

Quoted 4 lines

Interesting. I'm not familiar with the pretend_object_file() code, but I think I catch what you're saying. I don't see anything in object-file.c that allows removing items from the cache, as I would want to do, but I'm sure I could add something.

Right, it's pretty limited now, and there's only one caller. And in fact I've wanted to get rid of it, exactly because it can create the danger we're discussing here. But I still think it's _less_ dangerous than fully replacing the object directory.

Quoted 4 lines

(I'm still not sure how this avoids the problem of things like textconv caching trying to write an object that references one of these, though. Should we just manually disable textconv caching during a remerge-diff?)

It can't avoid it completely, but in general, the textconv cache should be writing and referencing its own objects. Even if they happen to be the same as something remerge-diff generated, it won't know that until it has tried to write it (and so that's why having write_object_file() continue to really write the object is important).

Hopefully that clears up my line of thinking. I do think a lot of this is kind of theoretical, in the sense that:

- textconv caching is an obscure feature that we _could_ just disable during remerge-diffs

- there's a reasonable chance that there isn't any other code that wants to write during a diff

But this is all scary and error-prone enough that I'd prefer an approach that has the "least surprise". So any solution where random code calling write_object_file() _can't_ accidentally write to a throw-away directory seems like a safer less surprising thing.

It does mean that the remerge-diff code needs to be explicit in its object writes to say "this needs to go to the temporary area" (whether it's a flag, or calling into a totally different function or subsystem). I'm hoping that's not hard to do (because its writes are done explicitly by the remerge-diff code), but I worry that it may be (because you are relying on more generic tree code to the write under the hood).

From: Elijah Newren <[email protected]>

diff-merges.c +9 -0

index d897fd8a293..3a24c45b8e5 100644
@@ -17,6 +17,7 @@ static void suppress(struct rev_info *revs)
 	revs->combined_all_paths = 0;
 	revs->merges_imply_patch = 0;
 	revs->merges_need_diff = 0;
+	revs->remerge_diff = 0;
 }
 
 static void set_separate(struct rev_info *revs)

@@ -45,6 +46,12 @@ static void set_dense_combined(struct rev_info *revs)
 	revs->dense_combined_merges = 1;
 }
 
+static void set_remerge_diff(struct rev_info *revs)
+{
+	suppress(revs);
+	revs->remerge_diff = 1;
+}
+
 static diff_merges_setup_func_t func_by_opt(const char *optarg)
 {
 	if (!strcmp(optarg, "off") || !strcmp(optarg, "none"))

@@ -57,6 +64,8 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)
 		return set_combined;
 	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
 		return set_dense_combined;
+	else if (!strcmp(optarg, "r") || !strcmp(optarg, "remerge"))
+		return set_remerge_diff;
 	else if (!strcmp(optarg, "m") || !strcmp(optarg, "on"))
 		return set_to_default;
 	return NULL;

static int decoration_given;
static int use_mailmap_config = 1;
+static struct tmp_objdir *tmp_objdir;
static const char *fmt_patch_subject_prefix = "PATCH"; static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT; static const char *fmt_pretty;

builtin/log.c +11 -0

@@ -407,6 +410,17 @@ static int cmd_log_walk(struct rev_info *rev)
 	int saved_nrl = 0;
 	int saved_dcctc = 0;
 
+	if (rev->remerge_diff) {
+		tmp_objdir = tmp_objdir_create();
+		if (!tmp_objdir)
+			die(_("unable to create temporary object directory"));
+		tmp_objdir_make_primary(the_repository, tmp_objdir);
+
+		strbuf_init(&rev->remerge_objdir_location, 0);
+		strbuf_addstr(&rev->remerge_objdir_location,
+			      tmp_objdir_path(tmp_objdir));
+	}
+
 	if (rev->early_output)
 		setup_early_output();
 

Quoted 6 lines · Elijah Newren via GitGitGadget

static int decoration_given;
static int use_mailmap_config = 1;
+static struct tmp_objdir *tmp_objdir;
static const char *fmt_patch_subject_prefix = "PATCH"; static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT; static const char *fmt_pretty;

So here we make this static file-level etc...

Quoted 22 lines

builtin/log.c +11 -0

@@ -407,6 +410,17 @@ static int cmd_log_walk(struct rev_info *rev)
      int saved_nrl = 0;
      int saved_dcctc = 0;
 
+     if (rev->remerge_diff) {
+             tmp_objdir = tmp_objdir_create();
+             if (!tmp_objdir)
+                     die(_("unable to create temporary object directory"));
+             tmp_objdir_make_primary(the_repository, tmp_objdir);
+
+             strbuf_init(&rev->remerge_objdir_location, 0);
+             strbuf_addstr(&rev->remerge_objdir_location,
+                           tmp_objdir_path(tmp_objdir));
+     }
+
      if (rev->early_output)
              setup_early_output();
 

@@ -449,6 +463,13 @@ static int cmd_log_walk(struct rev_info *rev)
      rev->diffopt.no_free = 0;
      diff_free(&rev->diffopt);
 

builtin/log.c static int cmd_log_walk(struct rev_info *rev) · line 470

Quoted 5 lines

builtin/log.c +5 -0

+     if (rev->remerge_diff) {
+             strbuf_release(&rev->remerge_objdir_location);
+             tmp_objdir_remove_as_primary(the_repository, tmp_objdir);
+             tmp_objdir_destroy(tmp_objdir);
+             tmp_objdir = NULL;

...but all of the "tmp_objdir" usage is in one function, can't the variable be declared here instead?

We need to hand the "remerge_objdir_location" off to the "rev_info" struct, but that seems separate from its lifetime.

Re my [1] & [2] I like Neeraj's "atexit cleanup" approach better, perhaps that makes your cleanup in log-tree.c redundant or easier?

Per [2] it looks like you need to "hand off" the "remerge_objdir_location", so having the struct live in tmp-objdir.h as I suggested in [2] might make that work...

1. https://lore.kernel.org/git/[email protected]/
2. https://lore.kernel.org/git/[email protected]/

Here are some patches to add a --remerge-diff capability to show & log, which works by comparing merge commits to an automatic remerge (note that the automatic remerge tree can contain files with conflict markers).

Here are some example commits you can try this out on (with git show --remerge-diff $COMMIT):

* git.git conflicted merge: 07601b5b36
* git.git non-conflicted change: bf04590ecd
* linux.git conflicted merge: eab3540562fb
* linux.git non-conflicted change: 223cea6a4f05

In regards to the performance point above, the timing for running the following command:

time git log --min-parents=2 --max-parents=2 $DIFF_FLAG | wc -l

in linux.git (with v5.4 checked out, since my copy of linux is very out of date) is as follows:

DIFF_FLAG=--cc:            71m 31.536s
DIFF_FLAG=--remerge-diff:  31m  3.170s

Note that there are 62476 merges in this history. Also, output size is:

DIFF_FLAG=--cc:            2169111 lines
DIFF_FLAG=--remerge-diff:  2458020 lines

Quoted 4 lines · Elijah Newren via GitGitGadget

Here are some patches to add a --remerge-diff capability to show & log, which works by comparing merge commits to an automatic remerge (note that the automatic remerge tree can contain files with conflict markers).

Quoted 8 lines

Here are some example commits you can try this out on (with git show --remerge-diff $COMMIT):

* git.git conflicted merge: 07601b5b36
* git.git non-conflicted change: bf04590ecd
* linux.git conflicted merge: eab3540562fb
* linux.git non-conflicted change: 223cea6a4f05

<snip>...

Quoted 9 lines

In regards to the performance point above, the timing for running the following command:

time git log --min-parents=2 --max-parents=2 $DIFF_FLAG | wc -l

in linux.git (with v5.4 checked out, since my copy of linux is very out of date) is as follows:

Quoted 9 lines

DIFF_FLAG=--cc:            71m 31.536s
DIFF_FLAG=--remerge-diff:  31m  3.170s

Note that there are 62476 merges in this history. Also, output size is:

DIFF_FLAG=--cc:            2169111 lines
DIFF_FLAG=--remerge-diff:  2458020 lines

Which repo did you mean by linux.git? Kernel developers often work against Linus' mainline tree [1], while end-users (including myself) prefer stable tree (which is mainline + stable release branches and tags) [2].

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git

Here are some patches to add a --remerge-diff capability to show & log, which works by comparing merge commits to an automatic remerge (note that the automatic remerge tree can contain files with conflict markers).

* This new option does not (currently) work for octopus merges, since merge-ort is specific to two-parent merges[1].

* This option will not work on a read-only or full filesystem[2].

Quoted 3 lines · Elijah Newren via GitGitGadget

Here are some patches to add a --remerge-diff capability to show & log, which works by comparing merge commits to an automatic remerge (note that the automatic remerge tree can contain files with conflict markers).

Excited ;-)

Quoted 2 lines

* This new option does not (currently) work for octopus merges, since merge-ort is specific to two-parent merges[1].

Unless you do so manually, the native "octopus" backend does not let you create non-trivial merges anyway, so punting on them should not be a big loss. Falling back to --cc might be a usable alternative.

Quoted 1 line

* This option will not work on a read-only or full filesystem[2].

OK. I am not sure if it is worth doing the "temporary objects" trick, though---would it risk repository corruption if somebody is creating a new blob that happens to be identical to the one that is involved in the remerge operation at the same time, or there is no visibility of the temporary area to these "somebody" outside so there is no risk?

Here are some patches to add a --remerge-diff capability to show & log,

Quoted 1 line · Elijah Newren via GitGitGadget

Here are some patches to add a --remerge-diff capability to show & log,

One sad omission from the maintainer usecase is that we do not seem to know "git diff --remerge-diff" yet during a conflicted merge.

"git diff [-- <path>]" before recording the resolution for the path with "git add <path>" shows combined patch to give a final sanity check before committing it to the rerere database. I am wondering if viewing it in the --remrege-diff format instead would help this step even more.

We do have git diff AUTO_MERGE, though. It's not quite the same as it doesn't include all the "CONFLICT" messages shown in the terminal like --remerge-diff does with log/show, but otherwise it's the same.

Perhaps we could even alias git diff --remerge-diff to git diff AUTO_MERGE?