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

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

Hi Elijah,

On Sat, 4 Jun 2022, Johannes Schindelin wrote:

> On Tue, 17 May 2022, Elijah Newren wrote:
>
> > I think I got a fair amount of this implemented about a month or so
> > ago.  I just pushed up what I have to the wip-for-in-core-merge-tree
> > branch of newren/git.
>
> Thank you so much!
>
> I worked a few hours on this and pushed up my changes under the same
> branch name to dscho/git.

And I worked on it some more, and pushed up the result (which passes the
CI build except for some problem caused by changes outside of Git's source
code) ;-)

> > Some notes:
> >
> >   * A big "WIP" commit that needs to be broken up
>
> I did not yet start on that.

Still no start on that yet.

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

> >   * I have some FIXME comments in a couple places where I need to
> > figure out how I want to pass the variable number of arguments (in a
> > function already accepting a variable number of arguments for other
> > reasons, making the function in a way have to variable length lists of
> > arguments)
>
> In my WIP fixups, I refactored this into a version that takes varargs and
> another version that takes a string_list.
>
> However, after getting all this to compile and t4301 to pass, I think we
> actually only need a version that takes up to two "other" paths, and a
> version that takes a string_list with those "other" paths, where the
> former constructs a temporary string_list and then calls the latter.

I ended up refactoring the refactor. The `path_msg()` function now takes
the "other paths" in two different forms: it accepts two (optional) `const
char*` parameters and an (also optional) `struct string_list *`. Either of
them, if non-NULL, will be added to the `struct strvec` field.

This looks _slightly_ ugly to me, but from an implementation point is the
cleanest solution.

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

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

Ciao,
Dscho