Hi Elijah,
On Tue, 17 May 2022, Elijah Newren wrote:
> On Fri, May 13, 2022 at 3:21 AM Johannes Schindelin
> <Johannes.Schindelin@xxxxxx> wrote:
> >
> > On Thu, 10 Mar 2022, Johannes Schindelin wrote:
> >
> > > On Tue, 8 Mar 2022, Elijah Newren wrote:
> > >
> > > > So, this is one series where even if everyone else says to merge it
> > > > already, I'd like to wait a bit longer on it until I feel confident we
> > > > have a solution that handles at least the current usecases.
> > >
> > > Fair enough, you're in charge of this series, and I really like what you
> > > came up with.
> > >
> > > My thinking was driven more by the users' side, as I am relatively eager
> > > to integrate this into production, but am loathe to do that with an early
> > > iteration of `en/merge-tree` that might be substantially revamped, still.
> >
> > I've been bogged down with things elsewhere, but should now have time to
> > help on this end.
> >
> > Elijah, _is_ there anything I can help with?
>
> Yeah, I've been bogged down with other things too; the little Git time
> I've had has been spent responding to review requests or other things
> folks manually were asking for my input on.
>
> 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.
> Some notes:
>
> * A big "WIP" commit that needs to be broken up
I did not yet start on that.
> * 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 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.
> * 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)",
[...]
> If you want to take a stab at the above, or even see if my changes
> make sense (sorry for it all being squashed into one big commit and
> not having good commit messages, but, you know...you did ask), that'd
> be great.
Yes, I did ask, and I did receive ;-)
Thank you so much! 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.
Thanks!
Dscho
