From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Sun, 17 Mar 2019 13:05:39 GMT
Subject: Re: [PATCH v8 00/11] Fix scissors bug during conflict
Message-ID: <20190317130539.GA23160@szeder.dev>
In-Reply-To: <cover.1552817044.git.liu.denton@gmail.com>

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.

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