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

From: Elijah Newren <newren@gmail.com>
Date: Jul 28, 2022, 00:41 UTC
Message-ID: <CABPp-BF2O9mT4tX-1PuNmNvQwN8S_OmMz-Y8_yCu7tmVO_ZJmA@mail.gmail.com>
In-Reply-To: <CAFySSZB6bB0qqCv5EPmJBJY9RbDRFv8JDYj89W+ND_Jw6Ys1kA@mail.gmail.com>

Hi,

On Wed, Jul 27, 2022 at 3:00 PM Calvin Wan <calvinwan@google.com> wrote: [...]

Show 12 quoted lines

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

Show 15 quoted lines

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