From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 11 Mar 2019 10:14:46 GMT
Subject: Re: [PATCH v7 5/8] merge: cleanup messages like commit
Message-ID: <2e10d78a-d92c-76c1-abc3-49ad1be55779@gmail.com>
In-Reply-To: <CAPig+cS5YsjxuLGNAtfFguQvy2p2bJECSsHksx356+WsO1ct5w@mail.gmail.com>

Hi Denton/Eric

On 11/03/2019 05:49, Eric Sunshine wrote:
> On Sun, Mar 10, 2019 at 11:42 PM Denton Liu <liu.denton@gmail.com> wrote:
>> This change allows git-merge messages to be cleaned up with the
>> commit.cleanup configuration or --cleanup option, just like how
>> git-commit does it.
>>
>> We also give git-pull the passthrough option of --cleanup so that it can
>> also take advantage of this change.
>>
>> Finally, add testing to ensure that messages are properly cleaned up.
>> Note that some newlines that were added to the commit message were
>> removed so that if a file were read via -F, it would be copied
>> faithfully.
>>
>> Signed-off-by: Denton Liu <liu.denton@gmail.com>
>> ---
>> diff --git a/t/t7604-merge-custom-message.sh b/t/t7604-merge-custom-message.sh
>> @@ -47,4 +47,65 @@ test_expect_success 'merge --log appends to custom message' '
>> +test_expect_success 'cleanup commit messages (verbatim option)' '
>> +       git reset --hard c1 &&
>> +       git merge --cleanup=verbatim -F expect c2 &&
>> +       git cat-file -p HEAD |sed -e "1,/^\$/d" >actual &&
> 
> An earlier patch in this series "fixed" a test with a Git command
> upstream of a pipe. Yet, this test adds such an instance.

Is it worth worrying about? We're not trying to test cat-file here and 
if it does fail the test will fail as actual will be empty. You could use
git log --pretty=%B >actual
which is clearer and saves forking sed.

Best Wishes

Phillip

> (Also, style: add space after '|'.)
> 
>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'cleanup commit messages (whitespace option)' '
>> +       git reset --hard c1 &&
>> +       { echo;echo "# text";echo; } >text &&
> 
> Style: add space after semicolon or use &&-chaining inside {...}.
> 
> Alternately, less ugly:
> 
>      test_write_lines "" "# text" "" >text &&
> 
> (Or even a here-doc, though the leading and trailing blank lines make
> the here-doc ugly.)
> 
>> +       echo "# text" >expect &&
>> +       git merge --cleanup=whitespace -F text c2 &&
>> +       git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
> 
> Git upstream pipe.
> 
>> +       test_cmp expect actual
>> +
>> +'
> 
> Style: drop the blank line before the closing quote.
> 
>> +test_expect_success 'cleanup merge messages (scissors option)' '
>> +       git reset --hard c1 &&
>> +       cat >text <<EOF &&
> 
> This here-doc probably ought to be using '-' and '\', so:
> 
>      cat >text <<-\EOF &&
> 
> and indent the here-doc body.
> 
>> +# to be kept
>> +
>> +  # ------------------------ >8 ------------------------
>> +# to be kept, too
>> +# ------------------------ >8 ------------------------
>> +to be removed
>> +# ------------------------ >8 ------------------------
>> +to be removed, too
>> +EOF
>> +
>> +       cat >expect <<EOF &&
> 
> Ditto: <<-\EOF and indent body.
> 
>> +# to be kept
>> +
>> +  # ------------------------ >8 ------------------------
>> +# to be kept, too
>> +EOF
>> +       git merge --cleanup=scissors -e -F text c2 &&
>> +       git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
> 
> Git upstream pipe.
> 
>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'cleanup commit messages (strip option)' '
>> +       git reset --hard c1 &&
>> +       { echo;echo "# text";echo sample;echo; } >text &&
> 
> test_write_lines "" "# text" sample "" >text &&
> 
>> +       echo sample >expect &&
>> +       git merge --cleanup=strip -F text c2 &&
>> +       git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
> 
> Git upstream pipe.
> 
>> +       test_cmp expect actual
>> +
>> +'
> 
> Drop blank line before closing quote.
>