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