patch, 7 partsAdd a new --remerge-diff capability to show & log

> From: Elijah Newren <newren@gmail.com>
>
> 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.

>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > 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

> 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):
>
> diff --git a/dir/my.fil e.conflict_msg b/dir/my.fil e.conflict_msg
> deleted file mode 100644
> index 2bd215a32f06..000000000000
> --- a/dir/my.fil e.conflict_msg
> +++ /dev/null
> @@ -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.

>
> On Tue, Aug 31, 2021 at 02:26:35AM +0000, Elijah Newren via GitGitGadget wrote:
>
> > 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.
>
> 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.

> 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.

> So, I'm not so sure about this solution; it feels like it introduces
> bigger holes than the ugly space character it is fixing.
>
>>  - 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, ...

>
> Elijah Newren <newren@gmail.com> writes:
>
> > 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.

> > So, I'm not so sure about this solution; it feels like it introduces
> > bigger holes than the ugly space character it is fixing.
> >
> >>  - 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, ...
>
> 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").

> > 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.

> >  - 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).

>
> On Tue, Sep 28, 2021 at 11:25:20PM -0700, Elijah Newren wrote:
>
> > > 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.
>
> 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).

> > 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.
>
> > >  - 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

> > 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.

> From: Elijah Newren <newren@gmail.com>
> 
> 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.

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

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

> 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.

>
> On Tue, Aug 31, 2021 at 02:26:36AM +0000, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > 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).
>
> > @@ -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.

> > @@ -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 <newren@gmail.com>
> 
> 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.)

> @@ -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 &&
> -	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
>  '

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

>
>
> On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:
>
> > @@ -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 &&
> > -     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.
>
> [...]
>
> > -     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.

> On Thu, Sep 30, 2021 at 9:53 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>>
>> On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:
>>
>> > @@ -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 &&
>> > -     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
>
> Do you mean
>    other cmd >/dev/null 2>actual
> ?

> diff --git a/tmp-objdir.c b/tmp-objdir.c
> index b8d880e3626..9f75a75d1c0 100644
> --- a/tmp-objdir.c
> +++ b/tmp-objdir.c
> @@ -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;
> +}

>
> 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/87v92lxhh4.fsf@evledraar.gmail.com/; Just
> comments on this patch here:
>
> > diff --git a/tmp-objdir.c b/tmp-objdir.c
> > index b8d880e3626..9f75a75d1c0 100644
> > --- a/tmp-objdir.c
> > +++ b/tmp-objdir.c
> > @@ -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.

> > 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.

> On Tue, Sep 28, 2021 at 1:00 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> 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/87v92lxhh4.fsf@evledraar.gmail.com/; Just
>> comments on this patch here:
>>
>> > diff --git a/tmp-objdir.c b/tmp-objdir.c
>> > index b8d880e3626..9f75a75d1c0 100644
>> > --- a/tmp-objdir.c
>> > +++ b/tmp-objdir.c
>> > @@ -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.
>
> 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.

> [...]
>> 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/7d1237c7-5a83-d766-7d93-5f0d59166067@web.de/
> https://lore.kernel.org/git/20180720180427.GE22486@sigill.intra.peff.net/

>
> On Tue, Sep 28 2021, Elijah Newren wrote:
>
> > On Tue, Sep 28, 2021 at 1:00 AM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >>
> >> 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/87v92lxhh4.fsf@evledraar.gmail.com/; Just
> >> comments on this patch here:
> >>
> >> > diff --git a/tmp-objdir.c b/tmp-objdir.c
> >> > index b8d880e3626..9f75a75d1c0 100644
> >> > --- a/tmp-objdir.c
> >> > +++ b/tmp-objdir.c
> >> > @@ -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.
> >
> > 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.
>
> [This is somewhat of a continuation of
> https://lore.kernel.org/git/87lf3etaih.fsf@evledraar.gmail.com/].
>
> 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.

> > [...]
> >> 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/7d1237c7-5a83-d766-7d93-5f0d59166067@web.de/
> > https://lore.kernel.org/git/20180720180427.GE22486@sigill.intra.peff.net/
>
> 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 <newren@gmail.com>
> 
> 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.

> Jeff King <peff@peff.net> writes:
> 
> >   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.
> 
> 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 ;-)

> On Tue, Sep 28, 2021 at 09:08:00PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> >   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.
>> 
>> 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".

>
> On Thu, Sep 30 2021, Jeff King wrote:
>
> > On Tue, Sep 28, 2021 at 09:08:00PM -0700, Junio C Hamano wrote:
> >
> >> Jeff King <peff@peff.net> writes:
> >>
> >> >   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.
> >>
> >> 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 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

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

> On Thu, Sep 30, 2021 at 6:31 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> On Thu, Sep 30 2021, Jeff King wrote:
>>
>> > On Tue, Sep 28, 2021 at 09:08:00PM -0700, Junio C Hamano wrote:
>> >
>> >> Jeff King <peff@peff.net> writes:
>> >>
>> >> >   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.
>> >>
>> >> 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 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
>
> 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.

>> 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?

>> 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?

>
> On Thu, Sep 30 2021, Elijah Newren wrote:
>
> > On Thu, Sep 30, 2021 at 6:31 AM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >>
> >> On Thu, Sep 30 2021, Jeff King wrote:
> >>
> >> > On Tue, Sep 28, 2021 at 09:08:00PM -0700, Junio C Hamano wrote:
> >> >
> >> >> Jeff King <peff@peff.net> writes:
> >> >>
> >> >> >   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.
> >> >>
> >> >> 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 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
> >
> > 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.
>
> ...
>
> >> 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.
>
> >> 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?
>
> 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.

>
> On Tue, Sep 28, 2021 at 09:08:00PM -0700, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > >   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.
> >
> > 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.

> >  - 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.

> >    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?

> On Thu, Sep 30, 2021 at 09:26:37PM -0700, Elijah Newren wrote:
>
>> >  - 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.
>
> 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.

>
> On Tue, Aug 31, 2021 at 02:26:38AM +0000, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > 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.
>
> 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 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.

>   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).

> > 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).

> 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.

> > 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.

> > 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.
> 
> 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?

> > 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.

> > > 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. :)

>
> Jeff King <peff@peff.net> writes:
>
> >   - 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?

> On Thu, Sep 30, 2021 at 1:06 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Jeff King <peff@peff.net> writes:
>>
>> >   - 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?
>
> 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.

>
> On Thu, Sep 30, 2021 at 03:26:42AM -0400, Jeff King wrote:
>
> > > > 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.

> > 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.)

> >   - 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.

> On Tue, Sep 28, 2021 at 10:05:31PM -0700, Elijah Newren wrote:
> 
> > 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.
> 
> > 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.
> 

> > > 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. :)
> 
> > > 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.
> > 
> > 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).
> 
> > > 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.
> 
> 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".
> 
> > 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.
> 
> > (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 :).

>> > 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.
>> 
>> ...
>
> 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).

>
> On Tue, Sep 28, 2021 at 10:05:31PM -0700, Elijah Newren wrote:
>
> > 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.
>
> > 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.
>
> > > 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. :)
>
> > > 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.
> >
> > 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).
>
> > > 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.
>
> 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".
>
> > 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.
>
> > (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).

> diff --git a/diff-merges.c b/diff-merges.c
> index d897fd8a293..3a24c45b8e5 100644
> --- a/diff-merges.c
> +++ b/diff-merges.c
> @@ -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;
> @@ -113,6 +122,9 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
>  	} else if (!strcmp(arg, "--cc")) {
>  		set_dense_combined(revs);
>  		revs->merges_imply_patch = 1;
> +	} else if (!strcmp(arg, "--remerge-diff")) {
> +		set_remerge_diff(revs);
> +		revs->merges_imply_patch = 1;
>  	} else if (!strcmp(arg, "--no-diff-merges")) {
>  		suppress(revs);
>  	} else if (!strcmp(arg, "--combined-all-paths")) {

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

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

>
> On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:
>
> >  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...
>
> > @@ -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);
> >
> > +     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?

> 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/87v92lxhh4.fsf@evledraar.gmail.com/
> 2. https://lore.kernel.org/git/87r1d9xh71.fsf@evledraar.gmail.com/

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

>
> On 31/08/21 09.26, Elijah Newren via GitGitGadget wrote:
> > 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
> >
> <snip>...
> > 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
> >
>
> 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

>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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 ;-)
>
> >  * 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.
>
> >  * 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?

>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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.