Re: [PATCH 08/12] merge-ort: provide a merge_get_conflicted_files() helper function

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 11064 bytes --]

Hi Elijah,

On Fri, 17 Jun 2022, Elijah Newren wrote:

> On Tue, Jun 7, 2022 at 12:38 AM Elijah Newren <newren@gmail.com> wrote:
> >
> > On Mon, Jun 6, 2022 at 2:37 PM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > >
> > > [after this update, I will shut up until you chime in, promise!]
> > >
> > > On Mon, 6 Jun 2022, Johannes Schindelin wrote:
> > >
> > > > On Sun, 5 Jun 2022, Johannes Schindelin wrote:
> > > >
> > > > > On Sat, 4 Jun 2022, Johannes Schindelin wrote:
> > > > >
> > > > > > It would be great if you could have a quick look over the commits I
> > > > > > added on top of your branch, to see whether things make more or less
> > > > > > sense to you. But if you're too busy elsewhere, I am one of the best
> > > > > > persons to understand that, too.
> > > > >
> > > > > Hopefully I will get this into a reviewable shape before you get to
> > > > > looking at it, so that your time is spent more wisely than what I
> > > > > asked for ;-)
> > > >
> > > > I hope to find some time to work on this more tomorrow; If not, I will
> > > > get back to the project on Wednesday and push it further.
> > >
> > > I did get a chance, and polished the patch series a bit. I also rebased it
> > > onto the current tip of Git's main branch, mainly to address some merge
> > > conflicts preemptively. The result is pushed up to
> > > https://github.com/dscho/git/tree/js/in-core-merge-tree. This is the
> > > range-diff relative to `newren/wip-for-in-core-merge-tree`:
> >
> > This is so awesome; thanks for working on this.  Sorry I haven't had
> > time to review yet, but I'm hoping to be able to near the end of this
> > week.  I'm excited to see how it looks.  :-)
> >
> > > -- snip --
> > >  1:  cccb3888070 <  -:  ----------- tmp-objdir: new API for creating temporary writable databases
> > >  2:  4e44121c2d7 <  -:  ----------- tmp-objdir: disable ref updates when replacing the primary odb
> > >  3:  0b94724311d <  -:  ----------- show, log: provide a --remerge-diff capability
> > >  4:  f06de6c1b2f <  -:  ----------- log: clean unneeded objects during `log --remerge-diff`
> > >  5:  8d6c3d48f0e <  -:  ----------- ll-merge: make callers responsible for showing warnings
> > >  6:  de8e8f88fa4 <  -:  ----------- merge-ort: capture and print ll-merge warnings in our preferred fashion
> > >  7:  6b535a4d55a <  -:  ----------- merge-ort: mark a few more conflict messages as omittable
> > >  8:  e2441608c63 <  -:  ----------- merge-ort: format messages slightly different for use in headers
> > >  9:  62734beb693 <  -:  ----------- diff: add ability to insert additional headers for paths
> > > 10:  17eccf7e0d6 <  -:  ----------- show, log: include conflict/warning messages in --remerge-diff headers
> > > 11:  b3e7656cfc6 <  -:  ----------- merge-ort: mark conflict/warning messages from inner merges as omittable
> > > 12:  ea5df61cf35 <  -:  ----------- diff-merges: avoid history simplifications when diffing merges
> > > 13:  4a7cd5542bb =  1:  8fb51817ed4 merge-tree: rename merge_trees() to trivial_merge_trees()
> > > 14:  4780ff6784d =  2:  8e0a79fa1ad merge-tree: move logic for existing merge into new function
> > > 15:  60253745f5c =  3:  baf0950bcb6 merge-tree: add option parsing and initial shell for real merge function
> > > 16:  f8266d39c1b =  4:  697470e50ae merge-tree: implement real merges
> > > 17:  6629af14919 =  5:  069af1ecc30 merge-ort: split out a separate display_update_messages() function
> > > 18:  17b57efb714 =  6:  53c92a5d8d9 merge-tree: support including merge messages in output
> > > 19:  4c8f42372dd =  7:  67a728d35f0 merge-ort: provide a merge_get_conflicted_files() helper function
> > > 25:  8fe5be07cd0 !  8:  6419487e26b merge-ort: remove command-line-centric submodule message from merge-ort
> > >     @@ merge-ort.c: static int merge_submodule(struct merge_options *opt,
> > >                 strbuf_release(&sb);
> > >                 break;
> > >         default:
> > >     +
> > >     + ## t/t6437-submodule-merge.sh ##
> > >     +@@ t/t6437-submodule-merge.sh: test_expect_success 'merging should conflict for non fast-forward' '
> > >     +   (cd merge-search &&
> > >     +    git checkout -b test-nonforward b &&
> > >     +    (cd sub &&
> > >     +-    git rev-parse sub-d > ../expect) &&
> > >     ++    git rev-parse --short sub-d > ../expect) &&
> > >     +     if test "$GIT_TEST_MERGE_ALGORITHM" = ort
> > >     +     then
> > >     +           test_must_fail git merge c >actual
> > > 20:  7b1ee417f3d !  9:  c92b81e7366 merge-tree: provide a list of which files have conflicts
> > >     @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
> > >      +          string_list_clear(&conflicted_files, 1);
> > >      +  }
> > >         if (o->show_messages) {
> > >     -           printf("\n");
> > >     +-          printf("\n");
> > >     ++          putchar(line_termination);
> > >                 merge_display_update_messages(&opt, &result);
> > >     +   }
> > >     +   merge_finalize(&opt, &result);
> > >      @@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> > >
> > >         /* Do the relevant type of merge */
> > > 21:  f1231a8fbc8 = 10:  d7360f58f16 merge-tree: provide easy access to `ls-files -u` style info
> > >  -:  ----------- > 11:  b53ef9c2116 merge-ort: store messages in a list, not in a single strbuf
> > >  -:  ----------- > 12:  b16d570d248 merge-ort: make `path_messages` a strmap to a string_list
> > >  -:  ----------- > 13:  b575a6b5f8a merge-ort: store more specific conflict information
> > >  -:  ----------- > 14:  4f245cc28ae merge-ort: optionally produce machine-readable output
> > > 22:  22297e6ce75 ! 15:  6a369f837be merge-tree: allow `ls-files -u` style info to be NUL terminated
> > >     @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
> > >         if (!result.clean) {
> > >                 struct string_list conflicted_files = STRING_LIST_INIT_NODUP;
> > >                 const char *last = NULL;
> > >     -@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
> > >     -           string_list_clear(&conflicted_files, 1);
> > >     -   }
> > >     -   if (o->show_messages) {
> > >     --          printf("\n");
> > >     -+          putchar(line_termination);
> > >     -           merge_display_update_messages(&opt, &result);
> > >     -   }
> > >     -   merge_finalize(&opt, &result);
> > >      @@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> > >                             N_("do a trivial merge only"), MODE_TRIVIAL),
> > >                 OPT_BOOL(0, "messages", &o.show_messages,
> > >     @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Check conflicted oids and
> > >      +
> > >      +  test_expect_code 1 git merge-tree --write-tree -z tweak1 side2 >out &&
> > >      +  anonymize_hash out >actual &&
> > >     ++  printf "\\n" >>actual &&
> > >      +
> > >      +  # Expected results:
> > >      +  #   "greeting" should merge with conflicts
> > >     @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Check conflicted oids and
> > >      +
> > >      +  EOF
> > >      +
> > >     -+  cat <<-EOF >>expect &&
> > >     -+  Auto-merging greeting
> > >     -+  CONFLICT (content): Merge conflict in greeting
> > >     -+  CONFLICT (file/directory): directory in the way of whatever from tweak1; moving it to whatever~tweak1 instead.
> > >     -+  CONFLICT (modify/delete): whatever~tweak1 deleted in side2 and modified in tweak1.  Version tweak1 of whatever~tweak1 left in tree.
> > >     -+  Auto-merging Αυτά μου φαίνονται κινέζικα
> > >     -+  CONFLICT (content): Merge conflict in Αυτά μου φαίνονται κινέζικα
> > >     ++  q_to_nul <<-EOF >>expect &&
> > >     ++  1QgreetingQAuto-mergingQAuto-merging greeting
> > >     ++  Q1QgreetingQCONFLICT (contents)QCONFLICT (content): Merge conflict in greeting
> > >     ++  Q2Qwhatever~tweak1QwhateverQCONFLICT (file/directory)QCONFLICT (file/directory): directory in the way of whatever from tweak1; moving it to whatever~tweak1 instead.
> > >     ++  Q1Qwhatever~tweak1QCONFLICT (modify/delete)QCONFLICT (modify/delete): whatever~tweak1 deleted in side2 and modified in tweak1.  Version tweak1 of whatever~tweak1 left in tree.
> > >     ++  Q1QΑυτά μου φαίνονται κινέζικαQAuto-mergingQAuto-merging Αυτά μου φαίνονται κινέζικα
> > >     ++  Q1QΑυτά μου φαίνονται κινέζικαQCONFLICT (contents)QCONFLICT (content): Merge conflict in Αυτά μου φαίνονται κινέζικα
> > >     ++  Q
> > >      +  EOF
> > >      +
> > >      +  test_cmp expect actual
> > > 23:  db73c6dd823 = 16:  47146dd59dd merge-tree: add a --allow-unrelated-histories flag
> > > 24:  d58a7c7a9f6 ! 17:  3ce28f6fd97 git-merge-tree.txt: add a section on potentional usage mistakes
> > >     @@ Documentation/git-merge-tree.txt: with linkgit:git-merge[1]:
> > >      +<<IM,Informational messages>> section has the necessary info, though it
> > >      +is not designed to be machine parseable.
> > >      +
> > >     ++Do NOT assume that each paths from <<CFI,Conflicted file info>>, and
> > >     ++the logical conflicts in the <<IM,Informational messages>> have a
> > >     ++one-to-one mapping, nor that there is a one-to-many mapping, nor a
> > >     ++many-to-one mapping.  Many-to-many mappings exist, meaning that each
> > >     ++path can have many logical conflict types in a single merge, and each
> > >     ++logical conflict type can affect many paths.
> > >     ++
> > >      +Do NOT assume all filenames listed in the <<IM,Informational messages>>
> > >      +section had conflicts.  Messages can be included for files that have no
> > >      +conflicts, such as "Auto-merging <file>".
> > > 26:  78e1243eca1 <  -:  ----------- WIP
> > > -- snap --
> > >
> > > I am pretty happy with the current state of the patches, and hope that we
> > > can push this patch series over the finish line.
> > >
> > > If you can think of anything I can do to help with this, please do let me
> > > know, I am _very_ interested in getting this done, and finally am in a
> > > position to help.
> >
> > Very much appreciated.  Looks like you're now blocking on my review,
> > so I'll try to make some time by end of week to look over things.
>
> Sorry for the delay.  However, I have looked over the changes in
> detail.  Well done!  I very much like this version.  Thanks for taking
> the time to break up my WIP patches, fix my bugs, address all the
> FIXMEs, and then going and adding some nice cleanups and improvements
> on top!  I'll add my Signed-off-by on a couple patches and have
> gitgitgadget submit this round.

That's great news, thank you so much!

Ciao,
Dscho