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