Hi Junio, On Wed, 26 Apr 2017, Junio C Hamano wrote: > Johannes Schindelin <johannes.schindelin@xxxxxx> 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