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

From: Calvin Wan <calvinwan@google.com>
Date: Jul 28, 2022, 19:06 UTC
Message-ID: <CAFySSZAfvjHR9SJEwyZ4JB1=uHp+XAxnoxnvekQ=pZ6To46DXg@mail.gmail.com>
In-Reply-To: <CABPp-BF2O9mT4tX-1PuNmNvQwN8S_OmMz-Y8_yCu7tmVO_ZJmA@mail.gmail.com>

Show 5 quoted lines

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

Adding a needswork bit to this one. Don't quite have the bandwidth to figure out what would be a useful message in this case.

Show 22 quoted lines

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

BUGging out for 'a' and 'b' sounds reasonable to me. And I also do believe my error message applies to the 'o' case. I'll add a test to confirm.

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

Ahhhhhhhhh I see...facepalm

Show 12 quoted lines

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

You're not missing anything. Wasn't aware of how translators worked with the project.