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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 21, 2020, 15:43 UTC
Message-ID: <CAPig+cQNiB8nw-JwwFweHzeFYcozGA06pPKr=0N9Metp8PBbSQ@mail.gmail.com>
In-Reply-To: <7f851e7c20aafdae5d5ae46ee1083b32ecc82c84.1600695050.git.gitgitgadget@gmail.com>

On Mon, Sep 21, 2020 at 9:31 AM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 13 quoted lines

> +--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.

Show 9 quoted lines

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