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

Commit Message

Johannes Schindelin Aug. 29, 2022, 3:11 p.m. UTC

From: Johannes Schindelin <[e-mail]>

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.

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 <[e-mail]>
---
 add-patch.c                |  3 ++-
 t/t3701-add-interactive.sh | 12 ++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

Phillip Wood Aug. 30, 2022, 1:26 p.m. UTC | #1

Hi Dscho

On 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>   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 &&

Johannes Schindelin Aug. 31, 2022, 8:05 p.m. UTC | #2

Hi Phillip,

On Tue, 30 Aug 2022, Phillip Wood wrote:

> On 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <[e-mail]>
> >
> > 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.

Yes, the `--ignore-submodules=dirty` part is important, but so is the
`--numstat` part.

It is important because the Perl script would parse that numstat part and
only offer files up for further processing for which it saw those numbers.
Then it would go ahead and run a full diff on those files, which basically
doubled the work.

And it is that doubling of work that I tried to avoid when implementing
the built-in version. The bug came about because the full diff call wasn't
using the `--ignore-submodules=dirty` option, and that's what I missed.

This is maybe more interesting a story for the cover letter, to be able to
understand how this bug was introduced, and maybe to offer an opportunity
for others (in addition to myself) to learn from my mistake.

> The fix itself looks good.

Thanks!
Dscho

Junio C Hamano Aug. 31, 2022, 8:19 p.m. UTC | #3

Johannes Schindelin <[e-mail]> writes:

> And it is that doubling of work that I tried to avoid when implementing
> the built-in version. The bug came about because the full diff call wasn't
> using the `--ignore-submodules=dirty` option, and that's what I missed.
>
> This is maybe more interesting a story for the cover letter, to be able to
> understand how this bug was introduced, and maybe to offer an opportunity
> for others (in addition to myself) to learn from my mistake.

Yup, the moral of the story is premature optimization bites because
we are not always careful ;-)

Anyhow, I am getting an impression that the behaviour of the next
iteration would be much closer to the original, which is excellent.

We have seen too many "ah, this is broken and here is a fix that is
appropriate in the context of how the new C version does it", not
"ok, let's make the whole thing behave more like what we used to
have".

Thanks.

Johannes Schindelin Aug. 31, 2022, 8:38 p.m. UTC | #4

Hi Junio,

On Wed, 31 Aug 2022, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
>
> > And it is that doubling of work that I tried to avoid when implementing
> > the built-in version. The bug came about because the full diff call wasn't
> > using the `--ignore-submodules=dirty` option, and that's what I missed.
> >
> > This is maybe more interesting a story for the cover letter, to be able to
> > understand how this bug was introduced, and maybe to offer an opportunity
> > for others (in addition to myself) to learn from my mistake.
>
> Yup, the moral of the story is premature optimization bites because
> we are not always careful ;-)

Yes. Maybe even more generally: trying to do too many things at once is
prone to introduce bugs.

> Anyhow, I am getting an impression that the behaviour of the next
> iteration would be much closer to the original, which is excellent.

Yep!

> We have seen too many "ah, this is broken and here is a fix that is
> appropriate in the context of how the new C version does it", not
> "ok, let's make the whole thing behave more like what we used to
> have".

Unfortunately so.

It is a fine line, and not always possible to tread safely, between trying
to stay close to the original and trying to learn the lesson from Elijah
that converting scripts to C without playing to C's strengths causes a lot
of hiccups later on. There are probably better alternatives for
prototyping Git functionality than using either an untyped language that
cannot hold complex data structures in memory or a language to which
everything looks like a map ;-)

Ciao,
Dscho

Patch

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 &&