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

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

Hi Elijah,

[sorry for sending this flurry of mails, I just wasn't sure how
consecutively I could work on the `merge-tree` patches, and therefore sent
mails whenever I had to take a break in case I wouldn't be able to get
back to this project for a couple of days.]

On Sun, 5 Jun 2022, Johannes Schindelin wrote:

> On Sat, 4 Jun 2022, Johannes Schindelin wrote:
>
> > On Tue, 17 May 2022, Elijah Newren wrote:
> >
> > > * The previous "output" member of merge_result, containing a strmap
> > > of conflict and informational messages (basically a mapping of
> > > filename -> strbuf) now needs to be replaced by a strmap
> > > "conflicts", which is now a mapping of primary_filename ->
> > > logical_conflicts, and logical_conflicts is an array of
> > > logical_conflict, and logical_conflict has a type, array of paths,
> > > and message.

I massaged this a bit further: since we no longer actually need a `strbuf`
there (we no longer append to the `strbuf` but instead to the list of
logical conflicts), I replaced `struct logical_conflicts` with a
`string_list` where each item contains the conflict message and its `util`
points to a `struct logical_merge_info` that contains the `type` and the
involved paths.

This lets me...

> > >   * Since "output" is no longer part of merge_result, the new
> > > remerge-diff functionality is going to need to be modified since it
> > > used that field, and instead iterate on "conflicts" to get the same
> > > information
> >
> > I punted on that for now, recreating an `output`-style strmap and storing
> > it as `path_messages` attribute.
>
> I still punted on that because I wanted to see whether I could address the
> test suite failures first (narrator's voice: he could).

... address this issue without resorting to declaring the `logical_merge`
struct in `merge-ort.h` (which would get a bit messy, not only because we
would have to `#include <strvec.h>` in that header because that struct
contains a `struct strvec paths` and therefore must know the storage size
of `strvec`, but also because `merge-ort.h` is not included in `diff.c`,
and it would be a bit iffy to do that).

Since the per-path conflicts are now stored as a `string_list`, and since
we only need the messages in remerge, we can continue to simply pass the
pointer to `strmap conflicts` to the remerge machinery (in the common
case, if pathspecs are involved, we continue to lightly copy-filter that
`strmap`).

> > >   * The new enums and structs I added to merge-ort.c really have to be
> > > added to merge-ort.h and become part of the API.  Feels a little
> > > unfortunate since it'll make the API _much_ more involved, but I don't
> > > see any other way to solve your usecase.
> >
> > I agree, but I did not do that yet ;-)

As mentioned above, I think the better way is to _not_ declare that enum
and structs in `merge-ort.h` and instead store the per-path conflicts as a
`string_list` whose strings contain the conflict message and whose `util`
contains the type and the list of involved paths.

This simplifies the API rather dramatically, since the current user
(remerge) is only interested in the conflict message, but not in the type
nor in the full list of involved paths.

Should we ever need to access the type or the paths outside of
`merge-ort.c`, it is easy enough to add a simple function to access that
information via the `string_list`'s `util`.

> > Another thing I noticed is that we can probably ensure consistency between
> > the `conflict_and_info_types` enum and the `type_short_descriptions` array
> > by using the same C99 construct we're already using in the
> > `advice_setting` array in advice.c:
> >
> > 	static const char *type_short_descriptions[NB_CONFLICT_TYPES] = {
> > 		/*** "Simple" conflicts and informational messages ***/
> > 		[INFO_AUTO_MERGING] = "Auto-merging",
> > 		[CONFLICT_CONTENTS] = "CONFLICT (contents)",
> > 	[...]
>
> Still haven't done that, either, as it is merely syntactic sugar, really,
> and not really an interesting change. I think I'll leave that to a time
> after I managed to modify the remerge-diff machinery to accept the
> new-style `conflicts` map (instead of recreating the old-style `output`
> map, as I am doing for now).

Since I addressed that `output` issue, I now also C99-ified the
`type_short_descriptions` array.

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

Ciao,
Dscho