[v4,3/3] add -p: ignore dirty submodules

Commit Message

Johannes Schindelin Aug. 31, 2022, 8:31 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

Jeff King Sept. 1, 2022, 3:45 p.m. UTC | #1

On Wed, Aug 31, 2022 at 08:31:17PM +0000, Johannes Schindelin via GitGitGadget wrote:

> Thanks to alwyas running `diff-index` and `diff-files` with the

Not a big deal, but since it sounds like you're going to re-roll anyway:

  s/alwyas/always/

I hadn't been closely following the earlier iterations of this series,
but the approach of this version definitely matches what I'd expected to
see based on our previous discussions about filters.

Modulo the coloring thing Phillip brought up in patch 2, it all looks
good to me. Thanks for working on it. I know you said earlier that the
bugs we've found in the conversion make you hesitant to remove the perl
version for now. But to me, your responsiveness in fixing those bugs
gives me more confidence in dumping the perl one. ;) (eventually, at
least)

-Peff

Jeff King Sept. 1, 2022, 3:49 p.m. UTC | #2

On Thu, Sep 01, 2022 at 11:45:49AM -0400, Jeff King wrote:

> On Wed, Aug 31, 2022 at 08:31:17PM +0000, Johannes Schindelin via GitGitGadget wrote:
> 
> > Thanks to alwyas running `diff-index` and `diff-files` with the
> 
> Not a big deal, but since it sounds like you're going to re-roll anyway:
> 
>   s/alwyas/always/

Doh, looks like I raced you sending the new version. :)

FWIW, the new iteration looks good to me.

-Peff

Junio C Hamano Sept. 1, 2022, 4:17 p.m. UTC | #3

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> From: Johannes Schindelin <[e-mail]>
>
> Thanks to alwyas running `diff-index` and `diff-files` with the

"always".

I notice that this round (like the previous one, v3) is made not to
apply on 'maint'.  As I said in the earlier review, in general I
prefer to fork a bugfix topic out of 'maint' unless there is a
strong reason not to.

It is a different matter if all these bugfix topics are actually
merged down to 'maint' and tagged a new maintenance release by me.
But I'd prefer to make it easier for motivated distro packagers to
adopt fixes that proves good in our 'master' front to their maint
releases, and it would be much easier to just merge a topic based on
'maint' than having to cherry-pick a topic based on 'master'.

Thanks.

Johannes Schindelin Sept. 2, 2022, 8:53 a.m. UTC | #4

Hi Junio,

On Thu, 1 Sep 2022, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> writes:
>
> > From: Johannes Schindelin <[e-mail]>
> >
> > Thanks to alwyas running `diff-index` and `diff-files` with the
>
> "always".
>
> I notice that this round (like the previous one, v3) is made not to
> apply on 'maint'.  As I said in the earlier review, in general I
> prefer to fork a bugfix topic out of 'maint' unless there is a
> strong reason not to.

Sorry, I thought that your comment was more about changing the base commit
too often, not meant as "please change it back for your next iteration".

I have changed it back, fixed the typo and force-pushed. In case a new
iteration is still required, that iteration will have these changes.

Ciao,
Dscho

> It is a different matter if all these bugfix topics are actually
> merged down to 'maint' and tagged a new maintenance release by me.
> But I'd prefer to make it easier for motivated distro packagers to
> adopt fixes that proves good in our 'master' front to their maint
> releases, and it would be much easier to just merge a topic based on
> 'maint' than having to cherry-pick a topic based on 'master'.
>
> Thanks.
>
>

Patch

diff --git a/add-patch.c b/add-patch.c
index 3699ca1fcaf..c7eebe9de89 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -419,7 +419,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 47ed6698943..2d710645719 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -955,6 +955,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 &&