Re: [PATCH 2/4] sequencer: do not translate parameters to error_resolve_conflict()

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

[Michael, I do not consider what I wrote below relevant for your patch
series, you may ignore it if you want]

On Fri, 19 Aug 2022, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> Perhaps we should have the error_resolve_conflict() function take a
> >> "enum replay_action" instead?
> >
> > We could do that. We could also just delete the sequencer code. It's just
> > that both are a bad idea.
>
> Sorry, but I do not quite understand this comment.

I expected a seasoned reviewer to offer such a suggestion only after
looking up (or remembering) how `error_resolve_conflict()` is defined, and
where, and where its callers are.

After all, many suggestions that come to mind during a review turn out to
be a bad idea when considering them carefully, and if that can be
determined before the mail is sent, everybody wins back some time.

In this instance, `error_resolve_conflict()` is declared in `advice.h`.
The suggestion to use a sequencer-specific data type there sounds...
controversial. But okay, maybe there are good reasons to suggest that.

Let's look at the callers. Two callers in `sequencer.c`. Okay, maybe it
makes a bit more sense. But one caller in `advice.c`? Let's dig deeper.

That caller in `advice.c` is `die_resolve_conflict()`, which is called in
the built-ins `commit`, `merge-recursive`, `merge` and `pull`.

Those callers have nothing to do with the sequencer, therefore it is a bad
idea to suggest using a sequencer-specific data type in that call chain.

From my perspective, that is enough to retire the suggestion.

When I wrote what I wrote, I thought that it was a pretty quick thing to
determine, so quick that I really expected to not see such a suggestion on
the mailing list in the first place.

In hindsight, I understand that you would have had to look at the code,
and not just at the patch, to see this. And therefore it is probably not
quite as obvious as I thought. I did not expect new contributors to be
able to analyze this quickly, but a Git mailing list regular, yes.

For my flippant response, I apologize.

As for the suggestion I criticized: I stand by my assessment. It is not a
good idea, and it was not necessary to send it out before doing a cursory
sanity check. We want code contribution to have a high quality, and the
code reviews should meet at least the same bar.

Ciao,
Dscho