Hi Elijah,

On Sat, 4 Jun 2022, Johannes Schindelin wrote:

On Tue, 17 May 2022, Elijah Newren wrote:

quoted

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

quoted

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.

quoted

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

quoted

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

quoted

  * 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