Re: [PATCH v3 5/9] t3404: relax rebase.missingCommitsCheck tests

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

Hi Junio,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > These tests were a bit anal about the *exact* warning/error message
> > printed by git rebase. But those messages are intended for the *end
> > user*, therefore it does not make sense to test so rigidly for the
> > *exact* wording.
> >
> > In the following, we will reimplement the missing commits check in
> > the sequencer, with slightly different words.
> 
> Up to this point I thought your strategy was to mimic the original
> as closely as possible, and changes to update (and/or improve) the
> end user experience (like rewording messages) are left as a separate
> step.  Changes to the test can be used to demonstrate the improved
> end-user experience that way, and I found it a good way to structure
> your series.  Is there a technical reason why you deviate from that
> pattern here?  
> 
> Just being curious, as I do not particularly mind seeing things done
> differently (especially if there is a good reason).

Yes, I remember the qualms I had about this patch. It was simply too
cumbersome to keep the exact error message, as it would have meant to
deviate purposefully from the way we do things in C.

In our C code, we error out using the error() function, with translateable
messages. That is what the todo list parsing code in sequencer.c does:

	error: invalid line 4: badcmd 0547e3f1350d D

It even prints out the line number, which I think is a nice touch.

Compare that to the non-standard way the rebase -i *shell* code reported
the failure:

	Warning: the command isn't recognized in the following line:
	 - badcmd 0547e3f1350d D

First of all, it reported it as a "Warning". Which is wrong, as it is a
fatal error, and it was always treated as such.

Second, it uses a capitalized prefix, which not even our warning()
function does.

Third, it uses two lines, and then indents the offending line with a
leading dash (as if we were in the middle of an enumeration).

I would have added that the previous message also imitated Lore in that it
uses a contraction, but I was shocked to learn through a simple git grep
that plenty of our error messages use that style, too.

I could have simply re-written the error() as an `fprintf(stderr,
"Warning: ...");` but that was just too inconsistent for my taste.

Hence I settled for relaxing the error message, which I had already done
much earlier so that running the test with a `set -x` inserted somewhere
into git-rebase--interacive.sh would still work.

Ciao,
Dscho