Re: [PATCH v3 5/5] add -p: ignore dirty submodules

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Dscho

On 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Thanks to alwyas running `diff-index` and `diff-files` with the
> `--numstat` option (the latter with `--ignore-submodules=dirty`) before
> even generating any real diff to parse, the Perl version of `git add -p`
> simply ignored dirty submodules and does not even offer them up for
> staging.

I had a bit of a hard time understanding this paragraph. To me the fact 
that the perl version is using --numstat is not that important here, 
what is important is that it is using --ignore-submodules=dirty when it 
generates its list of files to show and that information is consigned to 
a parenthesized aside.

The fix itself looks good.

Best Wishes

Phillip

> However, the built-in variant did not use that flag because it tries to
> run only one `diff` command, skipping the unneeded
> `diff-index`/`diff-files` invocation of the Perl variant and therefore
> only faithfully recapitulates what the Perl code does once it _does_
> generate and parse the real diff.
> 
> This causes a problem when running the built-in `add -p` with
> `diff-so-fancy` because that diff colorizer always inserts an empty line
> before the diff header to ensure that it produces 4 lines as expected by
> `git add -p` (the equivalent of the non-colorized `diff`, `index`, `---`
> and `+++` lines). But `git diff-files` does not produce any `index` line
> for dirty submodules.
> 
> The underlying problem is not even the discrepancy in lines, but that
> `git add -p` presents diffs for dirty submodules: there is nothing that
> _can_ be staged for those.
> 
> Let's fix that bug, and teach the built-in `add -p` to ignore dirty
> submodules, too. This _incidentally_ also fixes the `diff-so-fancy`
> problem ;-)
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   add-patch.c                |  3 ++-
>   t/t3701-add-interactive.sh | 12 ++++++++++++
>   2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index 0217cdd7c4a..ee6a3d3b712 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -426,7 +426,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>   	}
>   	color_arg_index = args.nr;
>   	/* Use `--no-color` explicitly, just in case `diff.color = always`. */
> -	strvec_pushl(&args, "--no-color", "-p", "--", NULL);
> +	strvec_pushl(&args, "--no-color", "--ignore-submodules=dirty", "-p",
> +		     "--", NULL);
>   	for (i = 0; i < ps->nr; i++)
>   		strvec_push(&args, ps->items[i].original);
>   
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 39e68b6d066..a4f45fc48a0 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -965,6 +965,18 @@ test_expect_success 'status ignores dirty submodules (except HEAD)' '
>   	! grep dirty-otherwise output
>   '
>   
> +test_expect_success 'handle submodules' '
> +	echo 123 >>for-submodules/dirty-otherwise/initial.t &&
> +
> +	force_color git -C for-submodules add -p dirty-otherwise >output 2>&1 &&
> +	grep "No changes" output &&
> +
> +	force_color git -C for-submodules add -p dirty-head >output 2>&1 <y &&
> +	git -C for-submodules ls-files --stage dirty-head >actual &&
> +	rev="$(git -C for-submodules/dirty-head rev-parse HEAD)" &&
> +	grep "$rev" actual
> +'
> +
>   test_expect_success 'set up pathological context' '
>   	git reset --hard &&
>   	test_write_lines a a a a a a a a a a a >a &&