Valid points, I don't think I have any objections to anything listed.
Would it be recommended to update to test_grep (and test_config from
previous message) in the same test files whilst I'm at it?
Thanks for the review :)
On Tue, 6 May 2025 at 01:02, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>
> On Mon, May 5, 2025 at 5:19 AM Leon Michalak via GitGitGadget
> <gitgitgadget@xxxxxxxxx> wrote:
> > 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@xxxxxxxxx>
> > ---
> > 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"));
>
> > @@ -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"));
>
> > 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`.
