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@xxxxxx> 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
