Re: [PATCH v8 00/11] Fix scissors bug during conflict

From: Denton Liu <liu.denton@gmail.com>
Date: Mar 18, 2019, 03:02 UTC
Message-ID: <20190318030220.GA14098@archbookpro.localdomain>
In-Reply-To: <20190317130539.GA23160@szeder.dev>

Hi Szeder,

On Sun, Mar 17, 2019 at 02:05:39PM +0100, SZEDER Gábor wrote:

Show 32 quoted lines

> On Sun, Mar 17, 2019 at 03:15:50AM -0700, Denton Liu wrote:
> > Sorry for taking so long to do a reroll, I've been pretty busy this week
> > and I've only been able to find the time now.
> 
> No problem, and thank you for sticking with it!
> 
> 
> > diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
> > index ca4a740da0..f035e4a507 100755
> > --- a/t/t7502-commit-porcelain.sh
> > +++ b/t/t7502-commit-porcelain.sh
> > @@ -16,7 +16,9 @@ commit_msg_is () {
> >  # Arguments: [<prefix] [<commit message>] [<commit options>]
> >  check_summary_oneline() {
> >  	test_tick &&
> > -	git commit ${3+"$3"} -m "$2" | head -1 > act &&
> > +	git commit ${3+"$3"} -m "$2" >act &&
> > +	head -1 <act >tmp &&
> > +	mv tmp act &&
> 
> Here you could swap the order of when you write to 'act' and 'tmp',
> and thus make the 'mv' unnecessary, like this:
> 
>   git commit [...] >tmp &&
>   head -1 act >tmp &&
>   [...rest of the test...]
> 
> Note also that 'head' (or 'sed' in later tests) can open its input
> file on its own, there's no need to redirect its standard input.
> 
> This is a recurring pattern in patches 1, 4, 8, and 9.
>

The reason I did it this way was because earlier, Junio said:

Show 6 quoted lines

> > -     git cat-file commit HEAD | sed -e '1,/^$/d' >actual &&
> > +     git cat-file commit HEAD >tmp &&
> > +     sed -e '1,/^$/d' <tmp >actual &&
> 
> The intermediary file may want a name better than 'tmp', if it is to
> be left behind, but this will do for now.

So I opted to write the tests in a way where a tmp file won't be produced. This pattern was shamelessly stolen from 'set up mod-256 conflict scenario' in t7600 where it does the following:

	# 256 near-identical stanzas...
	for i in $(test_seq 1 256); do
		for j in 1 2 3 4 5; do
			echo $i-$j
		done
	done >file &&
	git add file &&
	git commit -m base &&

	# one side changes the first line of each to "master"
	sed s/-1/-master/ <file >tmp &&
	mv tmp file &&
	git commit -am master &&

Good point about the heads and seds, though. I completely forgot that they accept a file argument.

Show 20 quoted lines

> > @@ -142,8 +144,8 @@ test_expect_success 'sign off' '
> >  	>positive &&
> >  	git add positive &&
> >  	git commit -s -m "thank you" &&
> > -	actual=$(git cat-file commit HEAD | sed -ne "s/Signed-off-by: //p") &&
> > -	expected=$(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/") &&
> > +	actual=$(git cat-file commit HEAD >tmp && sed -ne "s/Signed-off-by: //p" <tmp && rm tmp) &&
> > +	expected=$(git var GIT_COMMITTER_IDENT >tmp && sed -e "s/>.*/>/" <tmp && rm tmp) &&
> >  	test "z$actual" = "z$expected"
> 
> May I ask you to go one step further in restructuring this and the
> following tests? :)  Instead of using 'test' to compare the contents
> of the $actual and $expected variables, use 'test_cmp' to compare the
> 'actual' and 'expected' files, something like:
> 
>   git cat-file commit HEAD >tmp &&
>   sed -ne "s/Signed-off-by: //p" tmp >actual &&
>   git var GIT_COMMITTER_IDENT >tmp &&
>   sed -e "s/>.*/>/" >expected &&
>   test_cmp expected actual

Will do.

Thanks,

Denton