Re: [PATCH v3 1/3] interactive -p: add new `--rework-with-file` flag to interactive machinery

[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 because the session automatically
> advances to the next file or ends if we have just one file, immediately all hunks
> in a file are decided on.

The last part of the sentence after the last comma does not read
very well, at least to me.

We recommend to fold lines in such a way that after a few e-mail
exchange and quoting it will still stay within 80-column, so a
practical fill-column value lies somewhere around ~70.  Your lines a
slightly longer.

> Introduce the flag "--rework-with-file" when interactively selecting patches with the
> '--patch' option, which does not auto advance, thereby allowing users the option
> to rework with files.
> This ensures the current auto-advance method stays as the default method.

OK.  There may be suggestions for better option names from others; I
do not think of any right now.

> diff --git a/add-interactive.h b/add-interactive.h
> index da49502b76..aef2feca56 100644
> --- a/add-interactive.h
> +++ b/add-interactive.h
> @@ -6,9 +6,10 @@
>  struct add_p_opt {
>  	int context;
>  	int interhunkcontext;
> +	int no_auto_advance;
>  };

We add a new risk of double-negation confusion, e.g.,

    if (!opt->no_auto_advance)
	... do the auto-advance thing ...

where it may be easier to follow if it were written

    if (opt->auto_advance)
	... do the auto-advance thing ...

Would it make it harder to arrange the code if we made this member
"auto_advance" that defaults to "true"?  We have ADD_P_OPT_INIT that
everybody is supposed to call already, like this

> -#define ADD_P_OPT_INIT { .context = -1, .interhunkcontext = -1 }
> +#define ADD_P_OPT_INIT { .context = -1, .interhunkcontext = -1, .no_auto_advance = 0 }

so I do not imagine it would be too much hassle.

> @@ -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, no_auto_advance;
>  };

Likewise.

> diff --git a/builtin/add.c b/builtin/add.c
> index 32709794b3..408827cf54 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -256,6 +256,8 @@ static struct option builtin_add_options[] = {
>  	OPT_GROUP(""),
>  	OPT_BOOL('i', "interactive", &add_interactive, N_("interactive picking")),
>  	OPT_BOOL('p', "patch", &patch_interactive, N_("select hunks interactively")),
> +	OPT_BOOL(0, "rework-with-file", &add_p_opt.no_auto_advance,
> +		 N_("rework with files when selecting hunks interactively")),

Likewise.