Re: [PATCH v2 1/3] advice: handle "rebase" in error_resolve_conflict()

[Junio C Hamano <gitster@pobox.com>]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> On Fri, Apr 28, 2023 at 12:01:02PM -0700, Junio C Hamano wrote:
>>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>>
>>> This makes sure that we get a properly translated message rather than
>>> inserting the command (which we failed to translate) into a generic
>>> fallback message.
>>
>>Hmph, can this be accompanied with a change to add a test to an
>>existing test script to demonstrate that the function can be called
>>with me set to "rebase" and results in a generic message?
>>
> i suppose it could, but see next paragraph.
>
>>> We now also BUG() out when encountering an unexpected command.
>>
>>This needs to be reviewed by somebody who is more familiar with the
>>rebase/chrry-pick/revert/sequencer codepaths so that they can give a
>>definitive "good--I know that we never call this function with any
>>other value in 'me'" and that person would not be me.
>>
> assuming we care only about in-tree code, i'm just about as confident
> about this as one can reasonably be - because i grepped through the
> code, recursively looking for entry points. there are several calls
> via die_resolve_conflict() which have hard-coded `me`s (none of which
> is rebase), and two from the sequencer, where `me` comes from
> action_name(), which in turn returns one of three hard-coded strings
> (one of which is rebase). the latter is also kinda the test case,
> because it is obvious that this will be actually invoked when the
> situation occurs. it's probably also how i actually ran into the
> problem in the first place (i surely wasn't *looking* for it ...).
>
>>> Arguably, it would be cleaner to pass the command as an enum in the
>>> first place ...
>>
>>True, but that can be left to a different topic, I would think.
>>
> yes, otherwise i'd have already done it. ^^
> i can make it more explicit if you prefer that.
>
> if you agree with the reasoning, i'll prepare an update to the commit
> message and leave the patch as-is.

Yeah, how you arrived the conclusion that just covering "rebase" in
addition to what the code already covers would make the if/elseif
cascade complete is a very helpful addition to the log message.