[PATCH 0/3] A couple of CI fixes regarding the built-in add --patch

[PATCH 0/3] A couple of CI fixes regarding the built-in add --patch

[Johannes Schindelin via GitGitGadget]

This is an attempt to address the concern Junio raised in
https://lore.kernel.org/git/xmqq7d3gm1bl.fsf@gitster.g/: originally
motivated by a test suite failure when running Git's test suite in Visual
Studio, this pulls out the signed vs unsigned fix whose implications are
potentially much wider than Visual C.

While in that space, I spent the time (which took almost as long as I
expected
[https://lore.kernel.org/git/nrr2312s-q256-61n7-2843-7r0s817rp432@tzk.qr/])
to craft a semantic patch to scrutinize Git's source code for similar issues
(narrator's voice: there were no other instances, what did ya expect?).

To verify the fix, I then worked on a patch to exercise the built-in git add
-p in the test suite even when NO_PERL is set, and while developing this
patch and validating it, I got really puzzled that the add -p test case in
t6132 did not need to be guarded behind a PERL prereq. So this patch series
also includes a fix for that.

The story arc that binds all of these patches together is that they all
revolve around NO_PERL and CI issues that involve git add --patch.

Note: This patch series is based on ds/github-actions-use-newer-ubuntu (but
probably applies cleanly even on maint) because I tried to develop a
semantic patch to fix similar issues in the code base. However, I've since
run into what looks like a bug in Coccinelle
[https://github.com/coccinelle/coccinelle/issues/284]. My latest version of
that semantic patch looks like this, but I stopped when running it on Git's
source code triggered the bug for 66 of Git's .c files:

@@
type T = { unsigned int };
T:n b;
type S != { unsigned int, size_t };
S s;
binary operator o != { &&, || };
@@
-s o b
+s o (S)b

@@
type T = { unsigned int };
T:n b;
type S != { unsigned int, size_t };
S s;
binary operator o != { &&, || };
@@
-b o s
+(S)b o s


Johannes Schindelin (3):
  add -p: avoid ambiguous signed/unsigned comparison
  t3701: test the built-in `add -i` regardless of NO_PERL
  t6132(NO_PERL): do not run the scripted `add -p`

 add-patch.c                 | 2 +-
 t/t3701-add-interactive.sh  | 4 ++--
 t/t6132-pathspec-exclude.sh | 6 +++++-
 3 files changed, 8 insertions(+), 4 deletions(-)


base-commit: ef46584831268a83591412a33783caf866867482
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1340%2Fdscho%2Fbuilt-in-add-i-does-not-need-perl-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1340/dscho/built-in-add-i-does-not-need-perl-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1340
-- 
gitgitgadget

[PATCH 1/3] add -p: avoid ambiguous signed/unsigned comparison

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In the interactive `add` operation, users can choose to jump to specific
hunks, and Git will present the hunk list in that case. To avoid showing
too many lines at once, only a maximum of 21 hunks are shown, skipping
the "mode change" pseudo hunk.

The comparison performed to skip the "mode change" pseudo hunk (if any)
compares a signed integer `i` to the unsigned value `mode_change` (which
can be 0 or 1 because it is a 1-bit type).

According to section 6.3.1.8 of the C99 standard (see e.g.
https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should
happen is an automatic conversion of the "lesser" type to the "greater"
type, but since the types differ in signedness, it is ill-defined what
is the correct "usual arithmetic conversion".

Which means that Visual C's behavior can (and does) differ from GCC's:
When compiling Git using the latter, `add -p`'s `goto` command shows no
hunks by default because it casts a negative start offset to a pretty
large unsigned value, breaking the "goto hunk" test case in
`t3701-add-interactive.sh`.

Let's avoid that by converting the unsigned bit explicitly to a signed
integer.

Note: This is a long-standing bug in the Visual C build of Git, but it
has never been caught because t3701 is skipped when `NO_PERL` is set,
which is the case in the `vs-test` jobs of Git's CI runs.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/add-patch.c b/add-patch.c
index 509ca04456b..3524555e2b0 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1547,7 +1547,7 @@ soft_increment:
 			strbuf_remove(&s->answer, 0, 1);
 			strbuf_trim(&s->answer);
 			i = hunk_index - DISPLAY_HUNKS_LINES / 2;
-			if (i < file_diff->mode_change)
+			if (i < (int)file_diff->mode_change)
 				i = file_diff->mode_change;
 			while (s->answer.len == 0) {
 				i = display_hunks(s, file_diff, i);
-- 
gitgitgadget

[PATCH 2/3] t3701: test the built-in `add -i` regardless of NO_PERL

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The built-in `git add --interactive` does not require Perl, therefore we
can safely run these tests even when building with `NO_PERL=LetsDoThat`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3701-add-interactive.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index b354fb39de8..8d16cd45821 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -7,9 +7,9 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
-if ! test_have_prereq PERL
+if test_have_prereq !ADD_I_USE_BUILTIN,!PERL
 then
-	skip_all='skipping add -i tests, perl not available'
+	skip_all='skipping add -i (scripted) tests, perl not available'
 	test_done
 fi
 
-- 
gitgitgadget

[PATCH 3/3] t6132(NO_PERL): do not run the scripted `add -p`

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When using the non-built-in version of `git add -p` in a `NO_PERL`
build, we expect that invocation to fail.

However, when b02fdbc80a4 (pathspec: correct an empty string used as a
pathspec element, 2022-05-29) added a test case to t6132 to exercise
`git add -p`, it did not add appropriate prereqs (which admittedly did
not exist back then).

Let's specify the appropriate prereqs.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t6132-pathspec-exclude.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
index 9fdafeb1e90..cada952f9ae 100755
--- a/t/t6132-pathspec-exclude.sh
+++ b/t/t6132-pathspec-exclude.sh
@@ -293,7 +293,11 @@ test_expect_success 'add with all negative' '
 	test_cmp expect actual
 '
 
-test_expect_success 'add -p with all negative' '
+test_lazy_prereq ADD_I_USE_BUILTIN_OR_PERL '
+	test_have_prereq ADD_I_USE_BUILTIN || test_have_prereq PERL
+'
+
+test_expect_success ADD_I_USE_BUILTIN_OR_PERL 'add -p with all negative' '
 	H=$(git rev-parse HEAD) &&
 	git reset --hard $H &&
 	git clean -f &&
-- 
gitgitgadget

validating signed/unsigned comparisons with Coccinelle, was Re: [PATCH 0/3] A couple of CI fixes regarding the built-in add --patch

[Johannes Schindelin]

Hi,

On Tue, 30 Aug 2022, Johannes Schindelin via GitGitGadget wrote:

> Note: This patch series is based on ds/github-actions-use-newer-ubuntu (but
> probably applies cleanly even on maint) because I tried to develop a
> semantic patch to fix similar issues in the code base. However, I've since
> run into what looks like a bug in Coccinelle
> [https://github.com/coccinelle/coccinelle/issues/284]. My latest version of
> that semantic patch looks like this, but I stopped when running it on Git's
> source code triggered the bug for 66 of Git's .c files:
>
> @@
> type T = { unsigned int };
> T:n b;
> type S != { unsigned int, size_t };
> S s;
> binary operator o != { &&, || };
> @@
> -s o b
> +s o (S)b
>
> @@
> type T = { unsigned int };
> T:n b;
> type S != { unsigned int, size_t };
> S s;
> binary operator o != { &&, || };
> @@
> -b o s
> +(S)b o s

The bug in Coccinelle is already fixed (are you impressed? I certainly
am at the incredible speed and at the wonderful conversation I had!), and
I verified with this semantic patch that our code is clean:

-- snip --
@@
type T = { unsigned int };
T:n b;
type S != { unsigned int, size_t, float, double };
S s;
binary operator o != { &&, || };
@@
 s o
-b
+(S)b

@@
type T = { unsigned int };
T:n b;
type S != { unsigned int, size_t, float, double };
S s;
binary operator o != { &&, || };
@@
-b
+(S)b
 o s
-- snap --

I do not currently plan on integrating this into `contrib/coccinelle/`,
though, because it will take a while until we can benefit from the fix in
Git's CI/PR runs.

Ciao,
Dscho

Re: [PATCH 0/3] A couple of CI fixes regarding the built-in add --patch

[Phillip Wood]

Hi Dscho

All three patches look good to me, thanks for working on them.

Phillip

On 30/08/2022 14:54, Johannes Schindelin via GitGitGadget wrote:
> This is an attempt to address the concern Junio raised in
> https://lore.kernel.org/git/xmqq7d3gm1bl.fsf@gitster.g/: originally
> motivated by a test suite failure when running Git's test suite in Visual
> Studio, this pulls out the signed vs unsigned fix whose implications are
> potentially much wider than Visual C.
> 
> While in that space, I spent the time (which took almost as long as I
> expected
> [https://lore.kernel.org/git/nrr2312s-q256-61n7-2843-7r0s817rp432@tzk.qr/])
> to craft a semantic patch to scrutinize Git's source code for similar issues
> (narrator's voice: there were no other instances, what did ya expect?).
> 
> To verify the fix, I then worked on a patch to exercise the built-in git add
> -p in the test suite even when NO_PERL is set, and while developing this
> patch and validating it, I got really puzzled that the add -p test case in
> t6132 did not need to be guarded behind a PERL prereq. So this patch series
> also includes a fix for that.
> 
> The story arc that binds all of these patches together is that they all
> revolve around NO_PERL and CI issues that involve git add --patch.
> 
> Note: This patch series is based on ds/github-actions-use-newer-ubuntu (but
> probably applies cleanly even on maint) because I tried to develop a
> semantic patch to fix similar issues in the code base. However, I've since
> run into what looks like a bug in Coccinelle
> [https://github.com/coccinelle/coccinelle/issues/284]. My latest version of
> that semantic patch looks like this, but I stopped when running it on Git's
> source code triggered the bug for 66 of Git's .c files:
> 
> @@
> type T = { unsigned int };
> T:n b;
> type S != { unsigned int, size_t };
> S s;
> binary operator o != { &&, || };
> @@
> -s o b
> +s o (S)b
> 
> @@
> type T = { unsigned int };
> T:n b;
> type S != { unsigned int, size_t };
> S s;
> binary operator o != { &&, || };
> @@
> -b o s
> +(S)b o s
> 
> 
> Johannes Schindelin (3):
>    add -p: avoid ambiguous signed/unsigned comparison
>    t3701: test the built-in `add -i` regardless of NO_PERL
>    t6132(NO_PERL): do not run the scripted `add -p`
> 
>   add-patch.c                 | 2 +-
>   t/t3701-add-interactive.sh  | 4 ++--
>   t/t6132-pathspec-exclude.sh | 6 +++++-
>   3 files changed, 8 insertions(+), 4 deletions(-)
> 
> 
> base-commit: ef46584831268a83591412a33783caf866867482
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1340%2Fdscho%2Fbuilt-in-add-i-does-not-need-perl-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1340/dscho/built-in-add-i-does-not-need-perl-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1340

Re: validating signed/unsigned comparisons with Coccinelle, was Re: [PATCH 0/3] A couple of CI fixes regarding the built-in add --patch

[Phillip Wood]

Hi Dscho

On 30/08/2022 15:19, Johannes Schindelin wrote:
> Hi,
> 
> On Tue, 30 Aug 2022, Johannes Schindelin via GitGitGadget wrote:
> 
>> Note: This patch series is based on ds/github-actions-use-newer-ubuntu (but
>> probably applies cleanly even on maint) because I tried to develop a
>> semantic patch to fix similar issues in the code base. However, I've since
>> run into what looks like a bug in Coccinelle
>> [https://github.com/coccinelle/coccinelle/issues/284]. My latest version of
>> that semantic patch looks like this, but I stopped when running it on Git's
>> source code triggered the bug for 66 of Git's .c files:
>>
>> @@
>> type T = { unsigned int };
>> T:n b;
>> type S != { unsigned int, size_t };
>> S s;
>> binary operator o != { &&, || };
>> @@
>> -s o b
>> +s o (S)b
>>
>> @@
>> type T = { unsigned int };
>> T:n b;
>> type S != { unsigned int, size_t };
>> S s;
>> binary operator o != { &&, || };
>> @@
>> -b o s
>> +(S)b o s
> 
> The bug in Coccinelle is already fixed (are you impressed? I certainly
> am at the incredible speed and at the wonderful conversation I had!), and
> I verified with this semantic patch that our code is clean:

Wow that's fast, I wonder if they would be interested in fixing the 
parsing bug we found with Peff's UNUSED() series. It's good news that 
the patch does not find any other problems.

Best Wishes

Phillip

> 
> -- snip --
> @@
> type T = { unsigned int };
> T:n b;
> type S != { unsigned int, size_t, float, double };
> S s;
> binary operator o != { &&, || };
> @@
>   s o
> -b
> +(S)b
> 
> @@
> type T = { unsigned int };
> T:n b;
> type S != { unsigned int, size_t, float, double };
> S s;
> binary operator o != { &&, || };
> @@
> -b
> +(S)b
>   o s
> -- snap --
> 
> I do not currently plan on integrating this into `contrib/coccinelle/`,
> though, because it will take a while until we can benefit from the fix in
> Git's CI/PR runs.
> 
> Ciao,
> Dscho

Re: [PATCH 2/3] t3701: test the built-in `add -i` regardless of NO_PERL

[Jeff King]

On Tue, Aug 30, 2022 at 01:54:23PM +0000, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> The built-in `git add --interactive` does not require Perl, therefore we
> can safely run these tests even when building with `NO_PERL=LetsDoThat`.

Make sense. The patch is small enough that it is certainly worth doing
in the meantime, but is it time to start thinking about dropping the
perl implementation (and hence this prereq) entirely?

-Peff

Re: [PATCH 1/3] add -p: avoid ambiguous signed/unsigned comparison

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In the interactive `add` operation, users can choose to jump to specific
> hunks, and Git will present the hunk list in that case. To avoid showing
> too many lines at once, only a maximum of 21 hunks are shown, skipping
> the "mode change" pseudo hunk.

Wow.

While the whole "add -i" was what I started, I admit that everything
more recent than the support of the "edit" action in "add -p" is
foreign to me.  I didn't remember the existence of, and the
justification for, the 20-line limit.  It seems that this came from
3f6aff68 (Add subroutine to display one-line summary of hunks,
2008-12-04).

> The comparison performed to skip the "mode change" pseudo hunk (if any)
> compares a signed integer `i` to the unsigned value `mode_change` (which
> can be 0 or 1 because it is a 1-bit type).

OK, that warrants a casting to signed, surely.

> Note: This is a long-standing bug in the Visual C build of Git, but it
> has never been caught because t3701 is skipped when `NO_PERL` is set,
> which is the case in the `vs-test` jobs of Git's CI runs.

Good finding.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  add-patch.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index 509ca04456b..3524555e2b0 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1547,7 +1547,7 @@ soft_increment:
>  			strbuf_remove(&s->answer, 0, 1);
>  			strbuf_trim(&s->answer);
>  			i = hunk_index - DISPLAY_HUNKS_LINES / 2;
> -			if (i < file_diff->mode_change)
> +			if (i < (int)file_diff->mode_change)
>  				i = file_diff->mode_change;
>  			while (s->answer.len == 0) {
>  				i = display_hunks(s, file_diff, i);

Re: [PATCH 2/3] t3701: test the built-in `add -i` regardless of NO_PERL

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The built-in `git add --interactive` does not require Perl, therefore we
> can safely run these tests even when building with `NO_PERL=LetsDoThat`.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t3701-add-interactive.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

A companion to the previous step that is obvious and very much to
the point.  Looking good.

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index b354fb39de8..8d16cd45821 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -7,9 +7,9 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>  . ./test-lib.sh
>  . "$TEST_DIRECTORY"/lib-terminal.sh
>  
> -if ! test_have_prereq PERL
> +if test_have_prereq !ADD_I_USE_BUILTIN,!PERL
>  then
> -	skip_all='skipping add -i tests, perl not available'
> +	skip_all='skipping add -i (scripted) tests, perl not available'
>  	test_done
>  fi

Re: [PATCH 2/3] t3701: test the built-in `add -i` regardless of NO_PERL

[Johannes Schindelin]

Hi Peff,

On Tue, 30 Aug 2022, Jeff King wrote:

> On Tue, Aug 30, 2022 at 01:54:23PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The built-in `git add --interactive` does not require Perl, therefore we
> > can safely run these tests even when building with `NO_PERL=LetsDoThat`.
>
> Make sense. The patch is small enough that it is certainly worth doing
> in the meantime, but is it time to start thinking about dropping the
> perl implementation (and hence this prereq) entirely?

A couple of months ago, I would have said the same (and you seemed to
suggest something similar in [*1*], too), but since we flipped the default
to the built-in version in 0527ccb1b55 (add -i: default to the built-in
implementation, 2021-11-30), we discovered the need for several fixes:

- pw/add-p-hunk-split-fix
- js/add-i-delete
- js/add-p-diff-parsing-fix

Therefore I consider it premature to drop `git-add--interactive.perl` just
yet.

Ciao,
Dscho

Footnote *1*:
https://lore.kernel.org/git/YaaP%2Fg74KA63MCmx@coredump.intra.peff.net/

Re: validating signed/unsigned comparisons with Coccinelle, was Re: [PATCH 0/3] A couple of CI fixes regarding the built-in add --patch

[Johannes Schindelin]

Hi Phillip,

On Tue, 30 Aug 2022, Phillip Wood wrote:

> I wonder if they would be interested in fixing the parsing bug we found
> with Peff's UNUSED() series.

Could you point me to the relevant mail? I am sure that I can come up with
an MCVE that will help them pinpoint the bug and fix it as quickly as they
did with this here bug.

Still amazed at the turn-around time,
Dscho

Re: validating signed/unsigned comparisons with Coccinelle, was Re: [PATCH 0/3] A couple of CI fixes regarding the built-in add --patch

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Phillip,
>
> On Tue, 30 Aug 2022, Phillip Wood wrote:
>
>> I wonder if they would be interested in fixing the parsing bug we found
>> with Peff's UNUSED() series.
>
> Could you point me to the relevant mail? I am sure that I can come up with
> an MCVE that will help them pinpoint the bug and fix it as quickly as they
> did with this here bug.
>
> Still amazed at the turn-around time,
> Dscho

https://lore.kernel.org/git/xmqqwnax438x.fsf@gitster.g

Re: validating signed/unsigned comparisons with Coccinelle, was Re: [PATCH 0/3] A couple of CI fixes regarding the built-in add --patch

[Jeff King]

On Tue, Aug 30, 2022 at 11:12:24PM +0200, Johannes Schindelin wrote:

> On Tue, 30 Aug 2022, Phillip Wood wrote:
> 
> > I wonder if they would be interested in fixing the parsing bug we found
> > with Peff's UNUSED() series.
> 
> Could you point me to the relevant mail? I am sure that I can come up with
> an MCVE that will help them pinpoint the bug and fix it as quickly as they
> did with this here bug.

It's the sub-thread starting here:

  https://lore.kernel.org/git/220825.86ilmg4mil.gmgdl@evledraar.gmail.com/

I tried to put together a minimal example, which would be something
like:

  $ cat foo.c
  void old_function(int);
  void new_function(int);
  
  #if defined(__GNUC__)
  #define UNUSED(var) UNUSED_##var __attribute__((unused))
  #else
  #define UNUSED(var) UNUSED_##var
  #endif
  
  void foo(int UNUSED(bar), int x)
  {
  	old_function(x);
  }


  $ cat foo.cocci
  @@
  expression E;
  @@
  -old_function(E)
  +new_function(E)

but to my frustration, it actually works! If you add --verbose-parsing
to the spatch command, you can see it complaining about some of the
parsing. And it produces wrong outputs in the real-world with our actual
unused.cocci file. I suspect the example semantic patch might just need
to be a little more complicated.

-Peff

Re: validating signed/unsigned comparisons with Coccinelle, was Re: [PATCH 0/3] A couple of CI fixes regarding the built-in add --patch

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> Hi Phillip,
>>
>> On Tue, 30 Aug 2022, Phillip Wood wrote:
>>
>>> I wonder if they would be interested in fixing the parsing bug we found
>>> with Peff's UNUSED() series.
>>
>> Could you point me to the relevant mail? I am sure that I can come up with
>> an MCVE that will help them pinpoint the bug and fix it as quickly as they
>> did with this here bug.

I think the reference Peff posted is much closer to the description
of the "parsing bug".  Sorry for  the noise.

>> Still amazed at the turn-around time,
>> Dscho

Yes, indeed.

Thanks.