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

[Elijah Newren <newren@gmail.com>]

Hi,

On Wed, Jul 27, 2022 at 3:00 PM Calvin Wan <calvinwan@google.com> wrote:
[...]
> > 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.

Ah, I remember we partially discussed this earlier in this thread;
sorry for forgetting.

For the failed to initialize case, yes we also need a merge -- the
submodule is conflicted due to the lack of one.  The steps the user
needs to take are probably even more involved, though (they also need
to initialize the submodule), so perhaps that one should be special
cased.

The 'a' or 'b' being a null oid is actually dead code, as discussed
earlier in the thread.  Perhaps we should change those two code paths
from "return 0" to 'BUG("submodule deleted on one side; this should be
handled outside of merge_submodule()")', and possibly with a commit
message linking to
https://lore.kernel.org/git/CABPp-BE0qGwUy80dmVszkJQ+tcpfLRW0OZyErymzhZ9+HWY1mw@mail.gmail.com/
(and mentioning the "a and b being null oids within merge_submodule
will never trigger" portion of that email).

The 'o' being a null oid is not dead code.  That particular case means
that there was no submodule in the merge base, but both sides of the
merge introduced the submodule and have it checked out to different
oids.  (At least, hopefully it's the same submodule.)  In that case,
yes we do need some kind of merge.  So I think your message should
probably be included in this case, as-is.  Since the cleanup thing you
mention is an issue, perhaps you need to refactor the code a bit so
that you can make this case somehow get the same message printed for
users?

All that said, if you want to defer any or all of this to a follow-on
series, that's fine...but it would be nice to have it mentioned in the
commit message.

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

Um...if the string doesn't need to be marked for translation, and
isn't marked for translation, why would translators go looking for a
comment to help explain how to translate something that doesn't appear
in their list of strings they need to translate?

Using
    printf("    git add %s", ...)
means that the string "    git add %s" will not appear in the po/*.po
files.  If it had been
    printf(_("    git add %s"), ...)
then it would appear in those files with filename(s) and line
number(s) stating where the string had come from in case translators
needed to look for clues about the context in order to know how to
translate the string.

So, I think you can just drop the comment.  Or am I still not
understanding some nuance here?