Re: [PATCH v4 01/10] show, log: provide a --remerge-diff capability

On Tue, Feb 1, 2022 at 1:35 AM Ævar Arnfjörð Bjarmason [off-list ref] wrote:

On Fri, Jan 21 2022, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>
[...]
 ifdef::git-log[]
---diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc)::
+--diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r)::
 --no-diff-merges::
      Specify diff format to be used for merge commits. Default is
      {diff-merges-default} unless `--first-parent` is in use, in which case

@@ -64,6 +64,14 @@ ifdef::git-log[]
      each of the parents. Separate log entry and diff is generated
      for each parent.
 +
+--diff-merges=remerge:::
+--diff-merges=r:::
+--remerge-diff:::
+     With this option, two-parent merge commits are remerged to
+     create a temporary tree object -- potentially containing files
+     with conflict markers and such.  A diff is then shown between
+     that temporary tree and the actual merge commit.
++

Re some previous discussion. I really think we should add something like
this paragraph to this:

    The output emitted when this option is used is subject to change, and so
    is its interaction with other options (unless explicitly
    documented). I.e. many of the same caveats as the "OUTPUT STABILITY" in
    the linkgit:git-range-diff[1] documentation describes apply here. In
    particular other diff filtering options, pathspec limitations etc. may
    not produce the expected results, as some of those may apply to the
    "real" diff of the merge, and not on the generated "remerge-diff".

I think that would nicely give us permission to develop this further
without having to think about all the option interaction etc.

This is really useful right now, but I'd hate for it to get merged with
some bug/behavior that's not obvious to us now, and it being hard to fix
that because we'd have to consider the implicitly promised backwards
compatibility.

Sure I can add something.  I think the first sentence should be
sufficient though.

quoted

      int saved_dcctc = 0;
+     struct tmp_objdir *remerge_objdir = NULL;
+
+     if (rev->remerge_diff) {
+             remerge_objdir = tmp_objdir_create("remerge-diff");
+             if (!remerge_objdir)
+                     die(_("unable to create temporary object directory"));

I guess the s/die_errno/die/ here is better for now as we won't report
the wrong errno, but also lose the common case of errno being right. But
that can be fixed up with some other series to the tmp-objdir API.

quoted

[...]
+# This test is ort-specific
+test "${GIT_TEST_MERGE_ALGORITHM:-ort}" = ort || {
+     skip_all="GIT_TEST_MERGE_ALGORITHM != ort"
+     test_done
+}

FWIW this is still on a more complex pattern that it needs to be, see
this v1 discussion (which you seemed to ack):

https://lore.kernel.org/git/CABPp-BE+4rZNP-5mT2MNOWR6y6BgEG6mt1r_qcrZtarom6aGsw@mail.gmail.com/ (local)

Um, I thought I made this change.  How did I lose it?

Thanks for catching; will fix.