patch, 3 partsA couple of CI fixes regarding the built-in add --patch

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

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(-)

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

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).

OK, that warrants a casting to signed, surely.

Good finding.

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(-)

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

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

Hi Peff,

On Tue, 30 Aug 2022, Jeff King wrote:

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/

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

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

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(-)

Hi,

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

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

Hi Dscho

On 30/08/2022 15:19, Johannes Schindelin wrote:

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

Hi Phillip,

On Tue, 30 Aug 2022, Phillip Wood wrote:

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

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

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

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

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

Yes, indeed.

Thanks.

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

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

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: