From: Phillip Wood Date: Tue, 30 Aug 2022 13:26:28 GMT Subject: Re: [PATCH v3 5/5] add -p: ignore dirty submodules Message-ID: In-Reply-To: Hi Dscho On 29/08/2022 16:11, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin > > 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 > --- > 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 + 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 &&