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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 22, 2022, 13:53 UTC
Message-ID: <oqq42q11-3031-91or-no50-p68q85po1492@tzk.qr>
In-Reply-To: <xmqqfshsm8z1.fsf@gitster.g>

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:

Show 9 quoted lines

> 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