Re: [PATCH v4 1/4] interactive -p: add new `--auto-advance` flag

[Junio C Hamano <gitster@pobox.com>]

Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:

> When using the interactive add, reset, stash or checkout machinery,
> we do not have the option of reworking with a file when selecting
> hunks, because the session automatically advances to the next file
> or ends if we have just one file.
>
> Introduce the flag `--auto-advance` which auto advances by default,
> when interactively selecting patches with the '--patch' option.
> However, the `--no-auto-advance` option does not auto advance, thereby
> allowing users the option to rework with files.
>
> Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> ---
>  add-interactive.c     | 4 ++++
>  add-interactive.h     | 5 +++--
>  builtin/add.c         | 4 ++++
>  builtin/checkout.c    | 7 +++++++
>  builtin/reset.c       | 4 ++++
>  builtin/stash.c       | 8 ++++++++
>  t/t9902-completion.sh | 1 +
>  7 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/add-interactive.c b/add-interactive.c
> index 95ec5a89f8..c3a36cd11f 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -64,6 +64,7 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,
>  	s->r = r;
>  	s->context = -1;
>  	s->interhunkcontext = -1;
> +	s->auto_advance = 1;
>  
>  	s->use_color_interactive = check_color_config(r, "color.interactive");
>  
> @@ -124,6 +125,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,
>  			die(_("%s cannot be negative"), "--inter-hunk-context");
>  		s->interhunkcontext = add_p_opt->interhunkcontext;
>  	}
> +	if (!add_p_opt->auto_advance)
> +		s->auto_advance = 0;
>  }

I am confused.  Why do we need above two hunks in this function?
Wouldn't it suffice to do

	s->auto_advance = add_p_opt->auto_advance;

in the first hunk, instead of assigning 1 to it?

>  struct add_i_state {
>  	struct repository *r;
> @@ -28,7 +29,7 @@ struct add_i_state {
>  
>  	int use_single_key;
>  	char *interactive_diff_filter, *interactive_diff_algorithm;
> -	int context, interhunkcontext;
> +	int context, interhunkcontext, auto_advance;

Please don't do this.

The original is already bad to have two members on the same line,
but is tolerated as they represent somewhat related concepts.  The
auto_advance member has nothing to do with these two.