A couple of CI fixes regarding the built-in add --patch

From: Johannes Schindelin <[email protected]>

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

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 <[email protected]>

---
 add-patch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

add-patch.c +1 -1

index 509ca04456b..3524555e2b0 100644
@@ -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);

From: Johannes Schindelin <[email protected]>

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

Quoted 4 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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?

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 t/t3701-add-interactive.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

t/t3701-add-interactive.sh +2 -2

index b354fb39de8..8d16cd45821 100755
@@ -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

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 != { &&, || };
@@

(quoted diff) +1 -1

-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 != { &&, || };
@@

Hi,

Quoted 18 lines · Johannes Schindelin via GitGitGadget

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 != { &&, || };
@@

(quoted diff) +1 -1

-s o b
+s o (S)b

Review

Quoted 9 lines

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

(quoted diff) +1 -1

-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

(quoted diff) +1 -1

-b
+(S)b

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

(quoted diff) +1 -2

-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

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

Hi Phillip,

Quoted 2 lines · Phillip Wood

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

Quoted 2 lines · Johannes Schindelin

Hi Phillip,

Quoted 8 lines · Phillip Wood

Quoted 3 lines

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.

Quoted 2 lines

Still amazed at the turn-around time,
Dscho

Quoted 2 lines · Phillip Wood

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.

This is an attempt to address the concern Junio raised in
https://lore.kernel.org/git/[email protected]/: 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/[email protected]/])
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 != { &&, || };
@@

(quoted diff) +1 -1

-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 != { &&, || };
@@

(quoted diff) +1 -1

-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