Re: [PATCH 3/3] add-interactive: add new "context" subcommand

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 6, 2025, 00:02 UTC
Message-ID: <CAPig+cQmnAiHo8su6UBaKnZ=UZwgwMbDFA6ewMAfvCRR0RFzbw@mail.gmail.com>
In-Reply-To: <b4b7854f330af7588b12e3361bed40723febddad.1746436719.git.gitgitgadget@gmail.com>

On Mon, May 5, 2025 at 5:19 AM Leon Michalak via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 15 quoted lines

> This teaches `add/commit --interactive` a new "context" subcommand, which
> changes the amount of context lines subsequent subcommands like "patch"
> or "diff" generate in their diffs.
>
> Signed-off-by: Leon Michalak <leonmichalak6@gmail.com>
> ---
> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
> @@ -265,14 +265,15 @@ and type return, like this:
>  ------------
>      *** Commands ***
>        1: status       2: update       3: revert       4: add untracked
> -      5: patch        6: diff         7: quit         8: help
> +      5: patch        6: diff         7: context      8: quit
> +      9: help
>      What now> 1

I'm not a `git add/commit --interactive' user, but I can imagine that inserting "context" at 7 and bumping "quit" and "help" to 8 and 9, respectively, is going to play havoc with muscle memory people have built up over the years. To make this more friendly for existing users, I'd suggest adding this new command at the end of the list without changing the existing command numbers.

Also, looking at this list, I can't help but think that "context" feels out of place among the other action-oriented commands. Moreover, if --interactive mode grows more configuration/setting-like commands in the future, do we really want to keep extending this menu for them? Specifically, I'm wondering if it would instead make sense to introduce a new item "9: settings" which takes the user to a "Settings" submenu from which the number of context lines can be set.

> -The main command loop has 6 subcommands (plus help and quit).
> +The main command loop has 7 subcommands (plus help and quit).

Since you're touching this anyhow, let's fix this maintenance burden once and for all by writing more it generically, perhaps like this:

   The main command loop has several subcommands (plus help and quit).

> +context::
> +
> +  This lets you change the amount of context lines shown in diffs that
> +  the 'patch' and 'diff' subcommands generate.

s/amount/number/

> diff --git a/add-interactive.c b/add-interactive.c
> @@ -1061,6 +1118,8 @@ static int run_help(struct add_i_state *s, const struct pathspec *ps UNUSED,
> +       color_fprintf_ln(stdout, s->help_color, "context       - %s",
> +                        _("change how many context lines diffs are generated with"));

Perhaps:

    _("change the number of diff context lines"));

Show 7 quoted lines

> @@ -1087,6 +1146,16 @@ static void choose_prompt_help(struct add_i_state *s)
> +static void choose_prompt_help_context(struct add_i_state *s)
> +{
> +       color_fprintf_ln(stdout, s->help_color, "%s",
> +                        _("Prompt help:"));
> +       color_fprintf_ln(stdout, s->help_color, "<n>        - %s",
> +                        _("specify new context lines amount"));

Likewise:

    _("change number of diff context lines"));

> +       color_fprintf_ln(stdout, s->help_color, "           - %s",
> +                        _("(empty) finish selecting"));

"finish selecting" looks like a copy/paste error from elsewhere in this source file. Perhaps you meant something like:

    _("(empty) don't change number of context lines"));

Show 20 quoted lines

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> @@ -1230,4 +1237,23 @@ test_expect_success 'hunk splitting works with diff.suppressBlankEmpty' '
> +test_expect_success 'change context works' '
> +       git reset --hard &&
> +       cat >template <<-\EOF &&
> +       firstline
> +       preline
> +       TARGET
> +       postline
> +       lastline
> +       EOF
> +       sed "/TARGET/d" >x <template &&
> +       git update-index --add x &&
> +       git commit -m initial &&
> +       sed "s/TARGET/ADDED/" >x <template &&
> +       test_write_lines p 1 | git add -i >output &&
> +       grep firstline output &&
> +       test_write_lines c 0 p 1 | git add -i >output &&
> +       ! grep firstline output
> +'

This script does have its share of bare `grep` invocations, but these days we prefer `test_grep`, which also appears often in this script, so the following would be more appropriate:

    test_grep firstline output &&
    ...
    test_grep ! firstline output

Note the placement of "!" when used with `test_grep`.