Re: [PATCH 3/3] commit: add an option the reword HEAD

From: Phillip Wood <phillip.wood123@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>,
	Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Cc: Git List <git@vger.kernel.org>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH 3/3] commit: add an option the reword HEAD
Date: Mon, 21 Sep 2020 19:05:08 +0100	[thread overview]
Message-ID: <6da3752f-5e45-1381-b54c-64a81d642b72@gmail.com> (raw)
In-Reply-To: <CAPig+cQNiB8nw-JwwFweHzeFYcozGA06pPKr=0N9Metp8PBbSQ@mail.gmail.com>

Hi Eric

On 21/09/2020 16:43, Eric Sunshine wrote:
> On Mon, Sep 21, 2020 at 9:31 AM Phillip Wood via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> +--reword::
>> +       Reword the commit message of the tip of the current branch by
>> +       replacing it with a new commit. The commit contents will be
>> +       unchanged even if there are staged changes. This is equivalent
>> +       to specifying `--amend --only --allow-empty` with no paths.
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> @@ -1152,6 +1153,41 @@ static void finalize_deferred_config(struct wt_status *s)
>> +static void validate_reword_options(int argc, struct commit *current_head)
>> +{
>> +       if (amend)
>> +               die(_("cannot combine --reword with --amend"));
>> +       if (only)
>> +               die(_("cannot combine --reword with --only"));
> 
> Nit: It feels a bit odd (though not outright wrong) to disallow
> --reword in combination with --amend and --only after the
> documentation states that --reword is equivalent to using those
> options.

Yeah I decided to be quite strict, I'm in two minds about the 
documentation, I think it might be better to remove that line.

>> diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
>> @@ -713,4 +713,60 @@ test_expect_success '--dry-run --short' '
>> +test_reword_opt () {
>> +       test_expect_success C_LOCALE_OUTPUT "--reword incompatible with $1" "
>> +               echo 'fatal: cannot combine --reword with $1' >expect &&
>> +               test_must_fail git commit --reword $1 2>actual &&
>> +               test_cmp expect actual
>> +       "
>> +}
> 
> These error messages are subject to localization, so you'd want to use
> test_i18ncmp() here, I think.
> 
> Same comment for other new tests.

I decided to use the C_LOCALE_OUTPUT prerequisite and test_cmp rather 
than grep so I could check the exact output. I'm slightly suspicious of 
tests that just grep for an error message when that is all we should be 
showing. I should probably check that nothing is printed to stdout in 
these tests

Best Wishes

Phillip