Re: [PATCH v2 3/7] Add a test for `git replace --convert-graft-file`

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 20, 2018, 15:31 UTC
Message-ID: <nycvar.QRO.7.76.6.1804201728210.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
In-Reply-To: <xmqq7ep2d665.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Fri, 20 Apr 2018, Junio C Hamano wrote:

Show 11 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > The proof, as the saying goes, lies in the pudding. So here is a
> > regression test that not only demonstrates what the option is supposed to
> > accomplish, but also demonstrates that it does accomplish it.
> 
> The above spreads the misconception that the value of the test is
> "what I wrote works, look here".  It is not.  "Here is how this
> thing is supposed to work.  You are free to improve it, but do not
> break the basic promises these tests outline" to protect the
> resulting system is.

I am but a lousy foreigner, but to me, basically we said the same.

Show 21 quoted lines

> > diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
> > index c630aba657e..77ded6df653 100755
> > --- a/t/t6050-replace.sh
> > +++ b/t/t6050-replace.sh
> > @@ -444,4 +444,24 @@ test_expect_success GPG '--graft on a commit with a mergetag' '
> >  	git replace -d $HASH10
> >  '
> >  
> > +test_expect_success '--convert-graft-file' '
> > +	: add and convert graft file &&
> > +	printf "%s\n%s %s\n%s\n" \
> > +		$(git rev-parse HEAD^^ HEAD^ HEAD^^ HEAD^2) \
> > +		>.git/info/grafts &&
> 
> I find the above much less readbale than something like
> 
> 	{
> 		git rev-parse HEAD^^
> 		git rev-parse HEAD^ HEAD^^
> 		git rev-parse HEAD^2
> 	} >.git/info/grafts

Well, don't you know, that is how my first version looked. It failed, though, as `git rev-parse HEAD^ HEAD^^` outputs two *lines*.

And the version with a here-doc and inlined `$(git rev-parse ...)` really looks even uglier.

Show 9 quoted lines

> because printf formatting string must be deciphered and then matched
> against the order and number of rev-parse arguments (and printf's
> ability to happily accept more args than the placeholders does not
> help in readablity---the reader needs to verify that the code is not
> doing anything overly clever exploiting that ability) before I can
> figure out who's parent of whom.
> 
> Of course, it saves a few spawning of subprocesses; I am not sure if
> that is worth the loss of readability in this case, though.

I disagree that it is so horrible to read. If a regression occurs, you will have the .git/info/grafts file as a reference anyway, so there is little you need to decipher.

Ciao, Dscho