Re: [PATCH v6] submodule merge: update conflict error message

[Calvin Wan <calvinwan@google.com>]

> Well explained.  One very minor suggestion: perhaps change "id of the
> commit" to "id of the submodule commit" just to make it slightly
> clearer that this information would take work for the user to discover
> on their own?  (When I first read it, I was thinking, "but they have
> the commit, it's what they passed to merge", before I realized my
> error.)

ack

> Sorry for not catching this in an earlier round, but merge_submodule()
> has four "return 0" cases, for particular types of conflicts.  Those
> should probably be switched to "goto cleanup" or something like that,
> so that these messages you are adding are also provided if one of
> those conflict cases are hit.

I didn't send these four "return 0" cases to cleanup because I thought
the error message wouldn't accurately reflect the resolution steps. Is
merging or updating the submodule still the correct resolution? The
first three cases are for a null o/a/b, and the fourth case is for a missing
local submodule. Also in cleanup, the subrepo is cleared but the
subrepo hasn't been initialized/failed to initialize in these four cases.

> >         object_array_clear(&merges);
> >  cleanup:
> > +       if (!ret) {
>
> And here's another item I have to apologize for not catching in an
> earlier round.  We should also require !opt->priv->call_depth as well
> in this if-condition.  If merging of merge bases produced a submodule
> conflict, but the outer merge involves two sides that resolved the
> inner conflict the exact same way, then there's no conflict at the
> outer level and nothing for the user to resolve.  If users don't have
> any conflicts to resolve, we don't want to print messages telling them
> how to resolve their non-existent conflicts.  And if there is still a
> conflict in the submodule for the outer merge as well as in the
> recursive merge(s), we don't want to list the module twice (or more)
> when we tell the user to fix conflicts in their submodules (especially
> since that means we'd be telling them to merge multiple different
> commits for the single submodule, which could get confusing).

ack.

> > +               for_each_string_list_item(item, csub) {
> > +            const char *abbrev= item->util;
>
> Messed up indent here?

Looks like going from my editor to `git format-patch` messed
something up here.

> > +               for_each_string_list_item(item, csub)
> > +                       /*
> > +                        * TRANSLATORS: This is a line of a recommended `git add` command
> > +                        * with multiple lines of submodule folders.
> > +                        * E.g.:     git add sub \
> > +                        *                   sub2 \
> > +                        *                   sub3
>
> Why does such a message need to be translated?  It's literal text the
> user should type, right?  I'm not sure what a translator would do with
> the message other than regurgitate it.

It doesn't. My point was to let the translator know that the only text
in this print is for a git command. I should probably add that context
to the comment though.

> > +                        */
> > +                       if (first) {
> > +                               printf("       git add %s", item->string);
>
> But if you did mean for there to be a translation and a TRANSLATORS
> note, then did you forget to translate it by calling _()?

Same reasoning as above.

> > +                               first = 0;
> > +                       } else {
> > +                               printf(" \\\n               %s", item->string);
> > +                       }
>
> Can we put braces around this for_each_string_list_item() block?  Or,
> as an alternative to the whole block, do you want to consider:
>
>    strub strbuf tmp = STRBUF_INIT;
>    strbuf_add_separated_string_list(&tmp, ' ', csub);
>    printf(_("    git add %s"), tmp.buf);   /* or maybe remove the
> translation; not sure what the point is */
>    strbuf_release(&tmp);
> ?  It is likely easier to copy & paste, and might be understood by
> more users (I'm not sure how many are aware that command lines can use
> backslashes for line continuation), but on the negative side, if you
> have a lot of submodules it might make it harder to read.  Even if you
> don't like space separated, though, you could still use this strategy
> by changing the second line to
>
>     strbuf_add_separated_string_list(&tmp, " \\\n               ", csub);

This is a much cleaner implementation, thanks! If my goal is to make
submodule merging easier for newer submodule users, then I think it's a
good assumption to remove any additional possible points of confusion,
aka with the "command lines can use backslashes for line continuation",
so I'll swap over to spaces.

> > +       print_submodule_conflict_suggestion(&opti->conflicted_submodules);
> > +       string_list_clear(&opti->conflicted_submodules, 1);
> > +
>
> It would be more consistent to have things allocated in merge_start()
> continue to be cleared out in clear_or_reinit_internal_opts().  This
> kind of breaks that pairing, and you're already making sure to clear
> it there, so I'd rather remove this duplicate string_list_clear()
> call.

ack.

> >         /* Also include needed rename limit adjustment now */
> >         diff_warn_rename_limit("merge.renamelimit",
> >                                opti->renames.needed_limit, 0);
> > @@ -4657,6 +4717,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
> >         trace2_region_enter("merge", "allocate/init", opt->repo);
> >         if (opt->priv) {
> >                 clear_or_reinit_internal_opts(opt->priv, 1);
> > +               string_list_init_dup(&opt->priv->conflicted_submodules);
>
> This works, but there is a minor optimization available here if you're
> interested (I understand if you're not since you're already at v6).
> Assuming you make the important opt->priv->call_depth fix, you can
> replace string_list_init_dup() with string_list_init_nodup() here.
> The paths aren't freed until clear_or_reinit_internal_opts() which
> (under the assumption previously stated) isn't called until
> merge_finalize(), which comes after the call to
> merge_display_update_messages(), which is where you use the data.
>
> (As repository paths are used all over merge-ort.c, the optimization
> to store them in one place (opt->priv->paths) and avoid duplicating
> them is used pretty heavily.  It's more important for a lot of the
> other strmaps since they'll have a lot more paths in them, but it is
> kind of nice to use this optimization where possible.)

Thanks for all the context of this one. I agree it's minor, but any place
we can provide a better example to future contributors seems like a
more than worthy reason to make this change :)