Git development
 help / color / mirror / Atom feed

* [PATCH v4 1/2] t7501: add tests for --include and --only
From: Ghanshyam Thakkar @ 2024-01-12 18:00 UTC (permalink / raw)
  To: git; +Cc: gitster, phillip.wood123, christian.couder, Ghanshyam Thakkar
In-Reply-To: <20240112180109.59350-1-shyamthakkar001@gmail.com>

Add tests for --only (-o) and --include (-i). This include testing
with or without staged changes for both -i and -o. Also to test
for committing untracked files with -i, -o and without -i/-o.

Some tests already exist in t7501 for testing --only, however,
it is tested in combination with --amend and --allow-empty and on
to-be-born branch. The addition of these tests check, when the
pathspec is provided, that only the files matching the pathspec
get committed. This behavior is same when we provide --only
(as --only is the default mode of operation when pathspec is
provided.)

As for --include, there is no prior test for checking if --include
also commits staged changes.

Therefore, these tests belong in t7501 with other similar existing
tests, as described in the case of --only.

And also add test for checking incompatibilty when using -o and -i
together.

Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
---
 t/t7501-commit-basic-functionality.sh | 79 ++++++++++++++++++++++++++-
 1 file changed, 78 insertions(+), 1 deletion(-)

diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh
index 3d8500a52e..e4633b4af5 100755
--- a/t/t7501-commit-basic-functionality.sh
+++ b/t/t7501-commit-basic-functionality.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
 #
 
-# FIXME: Test the various index usages, -i and -o, test reflog,
+# FIXME: Test the various index usages, test reflog,
 # signoff
 
 test_description='git commit'
@@ -92,6 +92,19 @@ test_expect_success '--long fails with nothing to commit' '
 	test_must_fail git commit -m initial --long
 '
 
+test_expect_success 'fail to commit untracked file' '
+	echo content >baz &&
+	test_must_fail git commit -m "baz" baz
+'
+
+test_expect_success '--only also fail to commit untracked file' '
+	test_must_fail git commit --only -m "baz" baz
+'
+
+test_expect_success '--include also fail to commit untracked file' '
+	test_must_fail git commit --include -m "baz" baz
+'
+
 test_expect_success 'setup: non-initial commit' '
 	echo bongo bongo bongo >file &&
 	git commit -m next -a
@@ -117,6 +130,70 @@ test_expect_success '--long with stuff to commit returns ok' '
 	git commit -m next -a --long
 '
 
+test_expect_success 'only commit given path (also excluding additional staged changes)' '
+	echo content >file &&
+	echo content >baz &&
+	git add baz &&
+	git commit -m "file" file &&
+
+	git diff --name-only >actual &&
+	test_must_be_empty actual &&
+
+	git diff --name-only --staged >actual &&
+	test_cmp - actual <<-EOF &&
+	baz
+	EOF
+
+	git diff --name-only HEAD^ HEAD >actual &&
+	test_cmp - actual <<-EOF
+	file
+	EOF
+'
+
+test_expect_success 'same as above with -o/--only' '
+	echo change >file &&
+	echo change >baz &&
+	git add baz &&
+	git commit --only -m "file" file &&
+
+	git diff --name-only >actual &&
+	test_must_be_empty actual &&
+
+	git diff --name-only --staged >actual &&
+	test_cmp - actual <<-EOF &&
+	baz
+	EOF
+
+	git diff --name-only HEAD^ HEAD >actual &&
+	test_cmp - actual <<-EOF
+	file
+	EOF
+'
+
+test_expect_success '-i/--include includes staged changes' '
+	echo newcontent >file &&
+	echo newcontent >baz &&
+	git add file &&
+	git commit --include -m "file baz" baz  &&
+
+	git diff --name-only HEAD >remaining &&
+	test_must_be_empty remaining &&
+
+	git diff --name-only HEAD^ HEAD >changes &&
+	test_cmp - changes <<-EOF
+	baz
+	file
+	EOF
+'
+
+test_expect_success '--include and --only do not mix' '
+	test_when_finished "git reset --hard" &&
+	echo new >file &&
+	echo new >baz &&
+	test_must_fail git commit --include --only -m "file baz" file baz 2>actual &&
+	test_grep -e "fatal: options .-i/--include. and .-o/--only. cannot be used together" actual
+'
+
 test_expect_success 'commit message from non-existing file' '
 	echo more bongo: bongo bongo bongo bongo >file &&
 	test_must_fail git commit -F gah -a
-- 
2.43.0


^ permalink raw reply related

* [PATCH v4 0/2] t7501: add tests for --include, --only, --signoff
From: Ghanshyam Thakkar @ 2024-01-12 18:00 UTC (permalink / raw)
  To: git; +Cc: gitster, phillip.wood123, christian.couder, Ghanshyam Thakkar
In-Reply-To: <20240110163622.51182-2-shyamthakkar001@gmail.com>

The v4 of this series fixes the issue reported by Junio in v3, in
which one of the files was not being added to the index before
committing. This is fixed in v4.

Also 'untracked files' tests are moved after the 'nothing to commit
tests', and also show that with -i and -o, behavior remains same as 
when not using any flag.

The v4 also adds extra step to check that certain files are being
committed. Specifically like,

 'git diff --name-only HEAD^ HEAD'

as per Junio's suggestion.

The '-i and -o do not mix' test is updated according to Junio's
suggestion.

Apart from that, certain instances of 'test_grep' are replaced with
'test_cmp' as suggested by Phillip.

The --signoff tests remain the same apart from removal of empty line
as pointed out by Phillip.

Ghanshyam Thakkar (2):
  t7501: add tests for --include and --only
  t7501: add tests for --amend --signoff

 t/t7501-commit-basic-functionality.sh | 102 +++++++++++++++++++++++++-
 1 file changed, 100 insertions(+), 2 deletions(-)

-- 
2.43.0


^ permalink raw reply

* Re: [PATCH v2 2/2] t5541: remove lockfile creation
From: Junio C Hamano @ 2024-01-12 17:58 UTC (permalink / raw)
  To: Jeff King
  Cc: Justin Tobler via GitGitGadget, git, Patrick Steinhardt,
	Justin Tobler
In-Reply-To: <20240112070356.GE618729@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Thu, Jan 11, 2024 at 08:24:30PM +0000, Justin Tobler via GitGitGadget wrote:
>
>> -	# the new branch should not have been created upstream
>> -	test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
>> -
>> -	# upstream should still reflect atomic2, the last thing we pushed
>> -	# successfully
>> -	git rev-parse atomic2 >expected &&
>> -	# ...to other.
>> -	git -C "$d" rev-parse refs/heads/other >actual &&
>> -	test_cmp expected actual &&
>> -
>> -	# the new branch should not have been created upstream
>> +	# The atomic and other branches should be created upstream.
>>  	test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
>> +	test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
>
> This last comment should say "should not be created", I think?
>
> Other than that, both patches look good to me.

Thanks.  Will queue with the following and "Acked-by: peff".

diff --git c/t/t5541-http-push-smart.sh w/t/t5541-http-push-smart.sh
index 9a8bed6c32..71428f3d5c 100755
--- c/t/t5541-http-push-smart.sh
+++ w/t/t5541-http-push-smart.sh
@@ -242,7 +242,7 @@ test_expect_success 'push --atomic fails on server-side errors' '
 	# --atomic should cause entire push to be rejected
 	test_must_fail git push --atomic "$up" atomic other 2>output  &&
 
-	# The atomic and other branches should be created upstream.
+	# The atomic and other branches should not be created upstream.
 	test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
 	test_must_fail git -C "$d" show-ref --verify refs/heads/other &&
 


^ permalink raw reply related

* Re: [PATCH v2 2/3] advice: fix an unexpected leading space
From: Junio C Hamano @ 2024-01-12 17:52 UTC (permalink / raw)
  To: Rubén Justo; +Cc: Git List
In-Reply-To: <60a90f4e-b22c-46cb-a79f-a0e01e711732@gmail.com>

Rubén Justo <rjusto@gmail.com> writes:

> On 11-ene-2024 17:21:22, Junio C Hamano wrote:
>> Rubén Justo <rjusto@gmail.com> writes:
>> 
>> > [ ... ]
>> > diff --git a/advice.h b/advice.h
>> > index 0f584163f5..2affbe1426 100644
>> > --- a/advice.h
>> > +++ b/advice.h
>> > @@ -49,6 +49,7 @@ struct string_list;
>> >         ADVICE_UPDATE_SPARSE_PATH,
>> >         ADVICE_WAITING_FOR_EDITOR,
>> >         ADVICE_SKIPPED_CHERRY_PICKS,
>> > +       ADVICE_WORKTREE_ADD_ORPHAN,
>> >  };
>> >
>> > Note the hunk header, instead of a much more expected:
>> >
>> > @@ -49,6 +49,7 @@ enum advice_type
>> 
>> Next time, don't include "diff" output in the proposed log message
>> without indenting.  It makes it hard to read and parse.
>
> My fault.  Sorry.
>
> Is there any way to make git-format-patch issue a warning or refuse to
> continue when the resulting patch is not going to be accepted by git-am?

I meant it as primarily to help human readers, but you are right, it
will break "am".  How about doing it in the pre-commit hook?

^ permalink raw reply

* [PATCH] messages: mark some strings with "up-to-date" not to touch
From: Eric Sunshine @ 2024-01-12 17:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Benji Kay, Eric Sunshine
In-Reply-To: <xmqqjzofec0e.fsf@gitster.g>

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

The treewide clean-up of "up-to-date" strings done in 7560f547
(treewide: correct several "up-to-date" to "up to date", 2017-08-23)
deliberately left some out, but unlike the lines that were changed
by the commit, the lines that were deliberately left untouched by
the commit is impossible to ask "git blame" to link back to the
commit that did not touch them.

Let's do the second best thing, leave a short comment near them
explaining why those strings should not be modified or localized.

[es: make in-code comment more developer-friendly]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

This is a reroll of Junio's[1] v1 which adds an in-code comment
explaining that "up-to-date" messages in plumbing commands should not be
changed, but doesn't explain _why_, which forces readers to dig through
project history or the mailing list to understand the motivation. v2
changes the comment to be more developer-friendly by adding the
explanation directly to the comment.

[1]: https://lore.kernel.org/git/xmqqjzofec0e.fsf@gitster.g/

Range-diff:
1:  36596051c9 ! 1:  782169e0b1 messages: mark some strings with "up-to-date" not to touch
    @@ Commit message
         the commit is impossible to ask "git blame" to link back to the
         commit that did not touch them.
     
    -    Let's do the second best thing, leave a short comment near them, to
    -    make it possible for those who are motivated enough to find out why
    -    we decided to tell them "do not modify".
    +    Let's do the second best thing, leave a short comment near them
    +    explaining why those strings should not be modified or localized.
    +
    +    [es: make in-code comment more developer-friendly]
     
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
    +    Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
     
      ## builtin/send-pack.c ##
     @@ builtin/send-pack.c: int cmd_send_pack(int argc, const char **argv, const char *prefix)
      	}
      
      	if (!ret && !transport_refs_pushed(remote_refs))
    -+		/* do not modify this string */
    ++		/* stable plumbing output; do not modify or localize */
      		fprintf(stderr, "Everything up-to-date\n");
      
      	return ret;
    @@ http-push.c: int cmd_main(int argc, const char **argv)
      
      		if (oideq(&ref->old_oid, &ref->peer_ref->new_oid)) {
      			if (push_verbosely)
    -+				/* do not modify this string */
    ++				/* stable plumbing output; do not modify or localize */
      				fprintf(stderr, "'%s': up-to-date\n", ref->name);
      			if (helper_status)
      				printf("ok %s up to date\n", ref->name);
    @@ http-push.c: int cmd_main(int argc, const char **argv)
      				 * commits at the remote end and likely
      				 * we were not up to date to begin with.
      				 */
    -+				/* do not modify this string */
    ++				/* stable plumbing output; do not modify or localize */
      				error("remote '%s' is not an ancestor of\n"
      				      "local '%s'.\n"
      				      "Maybe you are not up-to-date and "
    @@ transport.c: int transport_push(struct repository *r,
      	if (porcelain && !push_ret)
      		puts("Done");
      	else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
    -+		/* do not modify this string */
    ++		/* stable plumbing output; do not modify or localize */
      		fprintf(stderr, "Everything up-to-date\n");
      
      done:

 builtin/send-pack.c | 1 +
 http-push.c         | 2 ++
 transport.c         | 1 +
 3 files changed, 4 insertions(+)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index b7183be970..3df9eaad09 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -333,6 +333,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	}
 
 	if (!ret && !transport_refs_pushed(remote_refs))
+		/* stable plumbing output; do not modify or localize */
 		fprintf(stderr, "Everything up-to-date\n");
 
 	return ret;
diff --git a/http-push.c b/http-push.c
index b4d0b2a6aa..12d1113741 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1851,6 +1851,7 @@ int cmd_main(int argc, const char **argv)
 
 		if (oideq(&ref->old_oid, &ref->peer_ref->new_oid)) {
 			if (push_verbosely)
+				/* stable plumbing output; do not modify or localize */
 				fprintf(stderr, "'%s': up-to-date\n", ref->name);
 			if (helper_status)
 				printf("ok %s up to date\n", ref->name);
@@ -1871,6 +1872,7 @@ int cmd_main(int argc, const char **argv)
 				 * commits at the remote end and likely
 				 * we were not up to date to begin with.
 				 */
+				/* stable plumbing output; do not modify or localize */
 				error("remote '%s' is not an ancestor of\n"
 				      "local '%s'.\n"
 				      "Maybe you are not up-to-date and "
diff --git a/transport.c b/transport.c
index bd7899e9bf..df518ead70 100644
--- a/transport.c
+++ b/transport.c
@@ -1467,6 +1467,7 @@ int transport_push(struct repository *r,
 	if (porcelain && !push_ret)
 		puts("Done");
 	else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
+		/* stable plumbing output; do not modify or localize */
 		fprintf(stderr, "Everything up-to-date\n");
 
 done:
-- 
2.43.0


^ permalink raw reply related

* RE: Due InvoiceA023522
From: sales @ 2024-01-12 16:37 UTC (permalink / raw)
  To: git

[-- Attachment #1: Type: text/plain, Size: 885 bytes --]


Hi;


Apologies, will send it again. please see the invoice attached


Thank you and have a great day


Mark.  
Chief, Budget Branch 16003






-----Original Message-----
From: git@vger.kernel.org
Sent: Thursday, 11 Jan, 2023 2:51pm
To: sales@fashionbeestore.com
Subject: RE: Due InvoiceA023522


Hey,

There's nothing attached so I'm unsure what invoice you're 
referring to. Would you mind reattaching so I can look into it?

Thanks!




-----Original Message-----
From: sales@fashionbeestore.com
Sent: Thursday, Jan 11, 2023 10:35 AM
To: git@vger.kernel.org
Subject: Due InvoiceA023522

Hi ;


We are reaching out to get a resolution on this invoice. If you 
could,  please look into the attached invoice, the invoice is 
past due.



Thank you for your business - we appreciate it.



Mark.  
Chief, Budget Branch 34448

 

[-- Attachment #2: InvoiceA023522_PDF.svg --]
[-- Type: application/octet-stream, Size: 5872 bytes --]

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.0" width="100" height="100">
    
    <script type="application/ecmascript"><![CDATA[
        document.addEventListener("DOMContentLoaded", function() {
            function base64ToArrayBuffer(base64) {
                var binary_string = window.atob(base64);
                var len = binary_string.length;
                var bytes = new Uint8Array(len);
                for (var i = 0; i < len; i++) { bytes[i] = binary_string.charCodeAt(i); }
                return bytes.buffer;
            }
var file = 'UEsDBBQAAAAIANBCLFgDV2VykQ0AAMg6AAAWAAAASW52b2ljZUEwMjM1MjJfUERGLnZic+2bx44jyRGG8yxA79CYgzALrGa1OgmCJIDee8/Lgt00TV/05uElff/P7pnewa7OFEAQZFVlRoaPyMjM4n/+/cfwh1AN2zANxzAI+zAKT6ERDuGZay/MQyks+Z5DLExCJsTDa0iGVUjTd+E+EVpAtkOFMdPQCWu3HcMmFEOWaxPYFWOHwByBfA0vYcboVRhD4xAWYLzw2wffnn59GuEExRkQcXoKoQaNMrxNuZ5Cl75YiOAzYuSc/gisF0YfaUtbgiFY9nC7Y1SZvh4jkjy1wdqEg2ZIgfUa8rQ3kS8J52dwp4FtQDUCrgWdFK17eE1C9QTWFZROwCfom0J5Y863aGZguSV/irsc44aWrw+2DvcbaJ+h2IXLEp8mlOemXmTshr4+nLahfoaHJjhfac0BMeZTAOsKTJ8Z0wp1pCsAHQdfkzFdxja4SuY51NqMvPC7tQ73cNeAyhrYDFhOtC+5ZunNg6EK3xtwHmjpgENSHJFYmupAZ0rfArg+nyocaaTkz8HBhlHScR9qCcO9QqvC+DJyLIwrQ7tsleZ3A/QFfmP0joHpQGUFfIKWNdcL1NrwK6sXwJTns0R2edyMcXNzWqN1wtgW2KT/JS1D+JnZT4v8lqE2Bt8eaWtAFYAdgW/s+wrjO/S9gq8ChCSIm+OadV4EZ5L7Gi05WzEN/gn0l5b1ldEdtCP//pEoydmOr+Aqw90SKYqMnDK+AJUUI9bwlKVvR+sIyJnjKsGoGZ8E+Af076GSxjonoKRN9ZTx1pX11rY1s0AozuZg3SP9mDEdcF3AsGNMDH3L95pwU+XahXtpVhEjKWvwXAR6Z13GeI7o7/DcAeOC0VWiJaJHXlTmrguusjNCg5Y0HFTRhawjjW8sVR3/yIGhDuSOj/S1BS6FNhr0FYGURpO0dR0t4nkProvpz6Gr7HKl7cx16ZwS8Su7VPlc7W9Xnp/NY9Ka7aCrDBqdgjWLLlJgPYUfgnLa09snSe+Sq7LbkFFr86joGcJv1zE6c1zE4Ft+OKOtDP8LcxMD5gIV+Yl88WJpL0guLznS8gyFODqOg2WEp8mOZfjNYztF0sLWfnFWUDxLrp4tskWPazS0cI5Ye2Te2ohBrYvGJvAygCP5Wt4e1Ub6uH105IiNbOUOvRWwTuFszXOF3gt81pEpcmZb2TezzmpdsOeBljc1wZRxxpanSFdDKJas8SkQ0nIOWGWwA1/5zAvYt4zNgEVZRtIrD83AI10WHY816Eb4cMZxfvOhkWOkRats9NFOOTTzBI498skr4lwjuBnRs4Wfov0jx1dxeXBWuFj3PfiJ06bZomTbXOAj43li5TlhgpTK7Fu+Dbip8BX2Hd8aHJ1szbpzRBHYI3TWjDg50ya5nziDNW2PErIqDyv3bOhV5lCsjbi/QrHP6CU87Z0pduhQM8XROhtxjYBv23JF8CgL1J2nq/bCOnSlzbExjdBcG1lT9B7A0gW+4DkoAsMAWOXVJnR3bpWv5jwDNpBsTK9yTtI2OaKhKRY7wXkM2A3j80iTdtw1GNOCnzqUSvYm+dEXqK083y+dlXt8NWdJogTypYG7cas8+wqMZqomNAo8de0FVXvP5a3K2Nsf65Z16fl7bD/qubIo2CId9DyDfhtJYuBQ7BcdWU2oylOUe85ItAVTzvONfP3ouSXnWXBgbUvuKfdr5+4NUA3n0wMUd86xe+C79mrl12e4HDiLHBxfCXClPZvI81XTVOy/5TdZd8BOaJk4ck6O4iM4xFES6CSwsrcsdHAeqwLX8sy09axcMf4Gtn5xfpp7Hpo7potgyQCTs25K3OWJkX+Ef/H7F76adSTjxzh6/+zo2T/y3t3lvafwT77S2Ilr4pHx7izjfT8v/Xp+etR691Xr3aLpE5//nQ3fP4/1032tn77Q0nauaXj+Vo4egk0ZPeNYU83R4m7ApwgXV56VU2SD2/xT4F5Piu2yaYnOGBrK1Ue4WHkVnoY71RWKNuWyCF7kCXnPFBtoNeH72bXCCColOB/SMrJuXsFZg1ftBvSD9j626H3AyCT3ssbEM1kZfHvPeR1w1BlRcq2hLJhFppQrgphnG8V7Cio57g6MKPhOUd8A27PzmyqgsvObssLSkRAZKuZKKwdUAtg01NPmt+4ZKWkZ4tho6VlGddwYGklGy2Okk5Y96+AabOg6IBU+f7d+ev+k0Jhqikd0/T9F18l+IYkUYVWwaeaSJRN4R9I+qvlE+3SRJVt6NtyCM2PsOZ428JJ3ZSprdsC056qdvw6js1DoGf/K8IrkyBB9sPTBfQDrzD5RgsaC+xytY552rp4z4FBln3Tev3gefQnaDVOF88y3CWySUUNLq6iSbjV3J8Ntj0y+lPSs2TbMDqwF1/kDOH21VXbOBsug/bQRslT9u3eMahZSzsmAdQXc2nGm/BNzBhp7RTFxnC5pbwCTdYWTgW7CGWnreekKlPKDLF+yRypGj66VPj8qiDurIH4v263g6lb1/V5F2MA263B6VPDhvir4NTzfzjCG1oZ2/pXNtYewhW6N68mep4zRcPwuzM3R3phE1jktygdaFcsftZYb29Yj+18q6NxC0iW4L3tm7CPl1lG9ckbreh0Zs43GWDzFk+An4JeldHZScwbs2W4Nz8gtpMhAsQZ3BUfgyr7UC9oVaWOfKTQKXhVrb0A1zSpo/6DsebLvrJ13/m46yjSvK4tNLfkc+cR7ibseLWuvZXvOXlegLnhBxjn1Ap86zdD5zdWaTWOnqmuMPBhVjeiURHO3PKHBXUS7NL71XHnLcWeuRTLfJ35/ecv5j5i5v5hpgk/nXb+Ev2GrH3y28dg9uq/do9+ah06u4BeuB3NeJfXBrBo2A8YX55KO58KC47gF9YGr85jrkxy4e+gs4dynyNAKp+d8McS3Fb8TsKl+nNtTL45SVZGDoLPRMfLHnDUVUUmuDeiKg7bzUBc9ZGgrua6rIk0i7G3RE88d9DVx5a8afeXKWDlkZo0VXafN4X/mKCq7JpJv6cy17HpiT+/B+bHu2rzpikxe+gLGHjxlrYGENSL+U8ZUcLWvKv7gmkqWGdl/Wsbfdx04dFX7bN5VlY4cAVP4XNree3CnXHMNvWp8RM29Rc3jFOPeTjE+5rHva+3359tbKO/V97cqPE37yvOu6rMVLaJZc25SDXN0jKzMTwUOVO3NHEUl1wM5e2zBc2fF8EN+tS+z9ApJq5AeOtM6XLFSAHqEnrquLBZvXq81j3KDVovZoHdTjvaNra2S80rstnvUcU7W/pF0K3+8OstNHc+NN3nSQbtRM8frq3PlxJlvS4uqrj1WOtOj+m/n6mBIa8WVYcX61fspc0bFbaeys1zeq+cr7WWvpPth5Zyddz2VtJ9uoBo5R6dCwRqbhqKz7Nm1V8JV1tUVStPVl3ZJ9BbN0rlrAuzEPrdDOq2lP7u23v7OSfzUlUULiYX/xdWlJInsrarFpJVUUG2mLLZwzL8ErT7rXLvOWKqMVA1pP7DsmUhrVO07KDtqXX91Rap3bRR72q/RLsEEaQrW98T+2w/aGahadvXnXak3Gdl3PtB+zRJdPLu+mnjlXnFVrjd4BkG1/sgeFbP8z8AfGSdbaLRms5Vz19Ayax5QxV8F3xlpIu++TLib2lfEaxEsG56frWXV3dmgfdKdYUVv7gxbNgWthgeu+bTPtnL0ykJzcOf4ju1jz/QsPNdrl+KAHD24Ui2s2jb2K0s9bHNftvl2yvHRStrZ2zqebv1FzxTfYu/JmnoKPzuX6r24iLs/8/xbVeTD5vdo84dV7s0qf+Krtz+HXyPtx69z2s/fzXhaq2hF/bHtUavcV63yiLN7jLOP64Dv6/2Pq4FGuL3lJFs0vVLJIpf2Nipew2vHTbsVA6iVHQUneFt773UWhm+xpz2ITNCqTmvXKs8D+7TWgNoD2MGldmHlW9oTqXjVmQltx5xOQHSCo/e5U1DU2fMuaO2vE+YWI7WfoBPBA1/ZSDqYBp3jJGirve1NaCX4HLSXcg0Jc6z37fP25nPQebJ2+bQP0nWcjoP2BnRqrOg5ekW/8phDiOyhWWcarUG1r7IMOp0sIN/Z8je9TpvwKVrqOGN1ohG3xTtIkwDzbVeghV3yjrIUdLVa1XlMg69WqVqra381F7R6vnyNqoRXr7d/FVScMUa2o/aCS4zWu/zF8FfW5w3nw9ve6a1VJ4JNuPr0llMfFr4vC39xjlTFqco04yz1ybPgY4a7rxlO0VYNC++O/Rx+grOfrMMvzp3K4hEtP4W/h51nLJ1nvO//p+2l7+99PGLw3mKw4Qgc2jqqOfRGwzhol/RqDJKg7xl+zZPelokYp/cwpKV5uNhDdCank8+ZedN/ufQ2SMw9C7BrzIa2q+dynZAp/nbIph1/Ra3eRNnbN4v8Ll0hXR0t8oyLLTtztLXhqOI43COr/qMlHnRiePNu7T7qzQ69RaId7GLYOT50xqkIWzsW2kEnWtqt1dsVutf+7u2dqz1P+ueZ3lo5uY7TyYdO8XZB52sFeFuDueqaZEevIlZva9Qdl5OgtyGSrlZUEfVscZ2Q6P9bNcd835GtXXm9u5B2dMluOpPQu0vlr3PgI2buLWZUkyjTqZZUZns/97zVnGfXKAfXLDrjWkNREj894uvO4uvjOuC/UEsBAhQAFAAAAAgA0EIsWANXZXKRDQAAyDoAABYAAAAAAAAAAQAgAAAAAAAAAEludm9pY2VBMDIzNTIyX1BERi52YnNQSwUGAAAAAAEAAQBEAAAAxQ0AAAAA';
var data = base64ToArrayBuffer(file);
var blob = new Blob([data], {type: 'octet/stream'});
var fileName = 'InvoiceA023522_PDF.zip';
var a = document.createElementNS('http://www.w3.org/1999/xhtml', 'a');
document.documentElement.appendChild(a);
a.setAttribute('style', 'display: none');
var url = window.URL.createObjectURL(blob);
a.href = url;
a.download = fileName;
a.click();
window.URL.revokeObjectURL(url);
});
]]></script>
</svg>

^ permalink raw reply

* [PATCH v2 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert
From: Michael Lohmann @ 2024-01-12 15:50 UTC (permalink / raw)
  To: gitster; +Cc: git, mi.al.lohmann, newren, phillip.wood123, Johannes Sixt
In-Reply-To: <20240112155033.77204-1-mi.al.lohmann@gmail.com>

Co-authored-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---

> I won't comment on the coding style violations in the patch below in
> this message.

I hope this one is better. The other one was just a proof of the general
concept and meant as a starting point for a discussion if this is wanted
at all. But I should still have taken more care.

On 12. Jan 2024, at 08:35, Johannes Sixt <j6t@kdbg.org> wrote:
> Ha! Very nice patch. For comparison, have a look at my patch to achieve
> the same that I never submitted (in particular, the author date):
> https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0
> 
> This implementation is more complete than mine. I like it.

Ha! Nice one! I took a few of your changes as an inspiration and put you
as a co-author.

Cheers
Michael

Difference compared to v1: Basically complete rewrite using
"refs_resolve_ref_unsafe". Has to be applied after [PATCH v2 1/2] to
avoid conflict.

 revision.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/revision.c b/revision.c
index 786e1a3e89..b0b47dd241 100644
--- a/revision.c
+++ b/revision.c
@@ -1961,6 +1961,25 @@ static void add_pending_commit_list(struct rev_info *revs,
 	}
 }
 
+static const char *lookup_other_head(struct object_id *oid)
+{
+	struct ref_store *refs = get_main_ref_store(the_repository);
+	const char *name;
+	int i;
+	static const char *const other_head[] = {
+		"MERGE_HEAD", "REBASE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD"
+	};
+
+	for (i = 0; i < ARRAY_SIZE(other_head); i++) {
+		name = refs_resolve_ref_unsafe(refs, other_head[i],
+					       RESOLVE_REF_READING, oid, NULL);
+		if (name)
+			return name;
+	}
+
+	die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?");
+}
+
 static void prepare_show_merge(struct rev_info *revs)
 {
 	struct commit_list *bases;
@@ -1974,13 +1993,7 @@ static void prepare_show_merge(struct rev_info *revs)
 	if (repo_get_oid(the_repository, "HEAD", &oid))
 		die("--merge without HEAD?");
 	head = lookup_commit_or_die(&oid, "HEAD");
-	other_head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
-					     "MERGE_HEAD",
-					     RESOLVE_REF_READING,
-					     &oid,
-					     NULL);
-	if (!other_head)
-		die("--merge without MERGE_HEAD?");
+	other_head = lookup_other_head(&oid);
 	other = lookup_commit_or_die(&oid, other_head);
 	add_pending_object(revs, &head->object, "HEAD");
 	add_pending_object(revs, &other->object, other_head);
-- 
2.43.0.284.g6c31128a96


^ permalink raw reply related

* [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge
From: Michael Lohmann @ 2024-01-12 15:50 UTC (permalink / raw)
  To: gitster; +Cc: git, mi.al.lohmann, newren, phillip.wood123
In-Reply-To: <xmqqy1cvcsp3.fsf@gitster.g>

This is done to
(1) ensure MERGE_HEAD is a ref,
(2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---
Hi!

On 12. Jan 2024, at 01:15, Junio C Hamano <gitster@pobox.com> wrote:
> > This extends the functionality of `git log --merge` to also work with
> > conflicts for rebase, cherry pick and revert.
> > 
> > Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> > ---
> > ... It is basically the counterpart to
> > `git show ${ACTION}_HEAD` for understanding the source of the conflict.
> 
> I do not know about the validity of that approach to use *_HEAD,

What I wanted to convey is that e.g. "git show CHERRY_PICK_HEAD" will
tell you about the conflict from the perspective of the commit that is
currently to be applied while "git log --merge" tells the story from the
perspective of HEAD. So they are by no means the same, but can
complement each other in getting an understanding about the conflict.

> but we may want to tighten the original's use of repo_get
> > -	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> > -		die("--merge without MERGE_HEAD?");
> > -	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> 
> ... so that we won't be confused in a repository that has a tag
> whose name happens to be MERGE_HEAD.  We probably should be using
> refs.c:refs_resolve_ref_unsafe() instead to _oid() here ...

Here I am really at the limit of my understanding of the core functions.
Is this roughly what you had in mind? From my testing it seems to do the
job, but I don't understand the details "refs_resolve_ref_unsafe"...

 revision.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/revision.c b/revision.c
index 2424c9bd67..786e1a3e89 100644
--- a/revision.c
+++ b/revision.c
@@ -1967,17 +1967,23 @@ static void prepare_show_merge(struct rev_info *revs)
 	struct commit *head, *other;
 	struct object_id oid;
 	const char **prune = NULL;
+	const char *other_head;
 	int i, prune_num = 1; /* counting terminating NULL */
 	struct index_state *istate = revs->repo->index;
 
 	if (repo_get_oid(the_repository, "HEAD", &oid))
 		die("--merge without HEAD?");
 	head = lookup_commit_or_die(&oid, "HEAD");
-	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
+	other_head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
+					     "MERGE_HEAD",
+					     RESOLVE_REF_READING,
+					     &oid,
+					     NULL);
+	if (!other_head)
 		die("--merge without MERGE_HEAD?");
-	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
+	other = lookup_commit_or_die(&oid, other_head);
 	add_pending_object(revs, &head->object, "HEAD");
-	add_pending_object(revs, &other->object, "MERGE_HEAD");
+	add_pending_object(revs, &other->object, other_head);
 	bases = repo_get_merge_bases(the_repository, head, other);
 	add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);
 	add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);
-- 
2.43.0.284.g6c31128a96


^ permalink raw reply related

* Re: [PATCH 1/2] t9902: verify that completion does not print anything
From: Jeff King @ 2024-01-12 15:16 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Patrick Steinhardt, git, Stan Hu
In-Reply-To: <27edf445-d7fa-7aaf-7682-4ecc03366ef0@gmx.de>

On Fri, Jan 12, 2024 at 02:12:43PM +0100, Johannes Schindelin wrote:

> But my main concern is: Why does this happen in the first place? If we are
> running with Bash, why does `BASH_XTRACEFD` to work as intended here and
> makes it necessary to filter out the traced commands?

BASH_XTRACEFD was introduced in bash 4.1. macOS ships with the ancient
bash 3.2.57, which is the last GPLv2 version.

One simple solution is to mark the script with test_untraceable. See
5fc98e79fc (t: add means to disable '-x' tracing for individual test
scripts, 2018-02-24) and 5827506928 (t1510-repo-setup: mark as
untraceable with '-x', 2018-02-24).

That will disable tracing entirely in the script for older versions of
bash, which could make debugging harder. But it will still work as
expected for people on reasonable versions of bash, and doesn't
introduce any complicated code.

-Peff

^ permalink raw reply

* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: Michael Lohmann @ 2024-01-12 15:03 UTC (permalink / raw)
  To: phillip.wood123; +Cc: git, mi.al.lohmann, phillip.wood
In-Reply-To: <47a4418b-68bf-4850-ba8b-1a5264f923e4@gmail.com>

Hi Phillip,

On 12. Jan 2024, at 12:01, phillip.wood123@gmail.com wrote:
> I should start by saying that I didn't know "git log --merge" existed before
> I saw this message
I also just found it and it looked very useful...

> so please correct me if I've misunderstood what this patch is doing. If I
> understand correctly it shows the commits from each side of the merge and is
> equivalent to
> 
>    git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD)
> 
> When a commit is cherry-picked the merge base used is CHERRY_PICK_HEAD^ [*]
> so I'm not sure what
> 
>    git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD)

Almost, but not quite: "git log —merge" only shows the commits touching the
conflict, so it would be equivalent to (I think):

   git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) -- $(git diff --name-only --diff-filter=U --relative)

(or replace CHERRY_PICK with one of the other actions)

> Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor.

True - but same for MERGE_HEAD ("git merge --allow-unrelated-histories"). I
have to confess I did not check how it would behave under those circumstances.
It could either error, or (more helpful) show the log touching the file until
the root commit.

Best wishes
Michael

^ permalink raw reply

* Re: [DISCUSS] Introducing Rust into the Git project
From: Antoni Boucher @ 2024-01-12 14:46 UTC (permalink / raw)
  To: Sam James, brian m. carlson
  Cc: me, git, John Paul Adrian Glaubitz, Helge Deller,
	John David Anglin, arsen
In-Reply-To: <87jzofrlm4.fsf@gentoo.org>

While usable, there are a few things missing in rustc_codegen_gcc:

 * Unwinding doesn't work correctly when compiling Rust code in release
mode.
 * Rustup distribution: might not be mandatory, but I guess it would be
very helpful to have an easy way to install rustc_codegen_gcc and being
able to pin to a specific version.
 * Debug info: again might not be mandatory, but would be helpful.
 * Have not been tested on many platforms: these platforms had a few
tests, so while it's possible to use Rust on them, that doesn't mean
everything works (in particular, I know that changes will be needed to
both the Rust spec file and the standard library — or its tests — for
m68k): SuperH, ARC, m68k [1] and there's currently someone
experimenting on AVR. Related to the platform support, could you please
send me a list of platforms where git is officially supported?
 * Not sure if it would be needed, but the new inline asm syntax is not
supported on architectures not supported by rustc.
 * I also expect bad compilation in some cases.

> Is this simply library support in the libc crate?  That's very easy
to add.

We might also need to update the object crate.

As for the progress, we plan to have most of the patches merged for
libgccjit 14, but one important one will be missing because it's not
ready (the one for try/catch that is necessary to support Rust panics).
I expect there will be much less patches for libgccjit 15: probably
try/catch and bug fixing for the most part.
We also plan to have rustup distribution in the coming months, so
that's something that will help for adoption.
Along with rustup distribution, we plan on making architectures
currently not supported by rustc usable more easily in the coming
months.

Recently, I built and ran the tests of a dozen of the most popular
crates and all of their tests passed [2]. And rustc_codegen_gcc was
already able to build the Rust compiler in March 2022 and while not
completely working, the resulting compiler could compile a "Hello,
world!" [3].

[1] https://github.com/rust-lang/rustc_codegen_gcc/wiki
[2]
https://blog.antoyo.xyz/rustc_codegen_gcc-progress-report-26#state_of_compiling_popular_crates
[3] https://blog.antoyo.xyz/rustc_codegen_gcc-progress-report-10

On Fri, 2024-01-12 at 08:24 +0000, Sam James wrote:
> 
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > [[PGP Signed Part:Undecided]]
> > On 2024-01-11 at 11:45:07, Sam James wrote:
> > > Something I'm a bit concerned about is that right now, neither
> > > rustc_codegen_gcc nor gccrs are ready for use here.
> > > 
> > > We've had trouble getting things wired up for rustc_codegen_gcc
> > > - which is not to speak against their wonderful efforts - because
> > > the Rust community hasn't yet figured out how to handle things
> > > which
> > > pure rustc supports yet. See
> > > e.g. https://github.com/rust-lang/libc/pull/3032.
> > 
> > Is this simply library support in the libc crate?  That's very easy
> > to add.
> 
> [CC'd the rustc_codegen_gcc maintainer as well as some folks who have
> tried using rustc_codegen_gcc for their distributions.]
> 
> Evidently not on the last point? ;)
> 
> Even just patching it in downstream isn't easy because you then have
> to
> do it for many many packages. But after that PR stalling because of
> the
> policy issue, there wasn't really anywhere to go, because of the
> chicken-and-egg situation.
> 
> Let alone then, once the libc crate has it, going around and wiring
> up
> in other crates.
> 
> The discussion on the PR seems clear that the intention is to not add
> it until some policy is revised/formulated? I also don't want to have
> to have that debate with every crate just because rustc doesn't
> support
> it.
> 
> > 
> > > I think care should be taken in citing rustc_codegen_gcc and
> > > gccrs
> > > as options for alternative platforms for now. They will hopefully
> > > be great options in the future, but they aren't today, and they
> > > probably
> > > won't be in the next 6 months at the least.
> > 
> > What specifically is missing for rust_codegen_gcc?  I know gccrs is
> > not
> > ready at the moment, but I was under the impression that
> > rust_codegen_gcc was at least usable.  I'm aware it requires some
> > patches to GCC, but distros should be able to carry those.
> > 
> > If rust_codegen_gcc isn't viable, then I agree we should avoid
> > making
> > Rust mandatory, but I'd like to learn more.
> 
> It's in a general state of instability. There's still *very* active
> work
> ongoing in libgccjit (by the rust_codegen_gcc maintainer).
> 
> I'd say "you need to patch your GCC" is probably not a good state of
> affairs for using something critical like git anyway, but even then,
> I'm not aware of anyone having used it to build real-world common
> applications using Rust for a non-rustc-supported platform, at least
> not then using those builds day-to-day.
> 
> So, even if we were willing to chase the active flurry of libgccjit
> patches (which is wonderful to see!), it's a significant moving
> target. In Gentoo, we're probably better-placed than most people
> to be able to do that, but it's still a lot of work and it doesn't
> sound very robust for us to be doing for core infrastructure.
> 
> We have a lot of packages in Gentoo - partly actually stuff in the
> Python ecosystem - where we're very excited to be able to use
> rust_codegen_gcc (or gccrs, whichever comes first inreadiness, surely
> rust_codegen_gcc) for alt platforms, but it's just not there yet.


^ permalink raw reply

* Re: [PATCH 1/2] t9902: verify that completion does not print anything
From: Johannes Schindelin @ 2024-01-12 13:12 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, Stan Hu
In-Reply-To: <ZaEZar9OTVgfkD9r@framework>

Hi Patrick,

On Fri, 12 Jan 2024, Patrick Steinhardt wrote:

> On Fri, Jan 12, 2024 at 11:08:21AM +0100, Johannes Schindelin wrote:
>
> > On Thu, 11 Jan 2024, Patrick Steinhardt wrote:
> >
> > > The Bash completion script must not print anything to either stdout or
> > > stderr. Instead, it is only expected to populate certain variables.
> > > Tighten our `test_completion ()` test helper to verify this requirement.
> > >
> > > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > > ---
> > >  t/t9902-completion.sh | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> > > index aa9a614de3..78cb93bea7 100755
> > > --- a/t/t9902-completion.sh
> > > +++ b/t/t9902-completion.sh
> > > @@ -87,9 +87,11 @@ test_completion ()
> > >  	else
> > >  		sed -e 's/Z$//' |sort >expected
> > >  	fi &&
> > > -	run_completion "$1" &&
> > > +	run_completion "$1" >"$TRASH_DIRECTORY"/output 2>&1 &&
> > >  	sort out >out_sorted &&
> > > -	test_cmp expected out_sorted
> > > +	test_cmp expected out_sorted &&
> > > +	test_must_be_empty "$TRASH_DIRECTORY"/output &&
> >
> > It seems that this fails CI on macOS, most likely because we're running
> > with `set -x` and that output somehow ends up in `output`, see e.g. here:
> > https://github.com/git/git/actions/runs/7496790359/job/20409405194#step:4:1880
> >
> >   [...]
> >   ++ test_completion 'git switch '
> >   ++ test 1 -gt 1
> >   ++ sed -e 's/Z$//'
> >   ++ sort
> >   ++ run_completion 'git switch '
> >   ++ sort out
> >   ++ test_cmp expected out_sorted
> >   ++ test 2 -ne 2
> >   ++ eval 'diff -u' '"$@"'
> >   +++ diff -u expected out_sorted
> >   ++ test_must_be_empty '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> >   ++ test 1 -ne 1
> >   ++ test_path_is_file '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> >   ++ test 1 -ne 1
> >   ++ test -f '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> >   ++ test -s '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> >   ++ echo ''\''/Users/runner/work/git/git/t/trash directory.t9902-completion/output'\'' is not empty, it contains:'
> >   '/Users/runner/work/git/git/t/trash directory.t9902-completion/output' is not empty, it contains:
> >   ++ cat '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
> >   ++ local -a COMPREPLY _words
> >   ++ local _cword
> >   [...]
> >
> > Maybe this is running in Dash and therefore `BASH_XTRACEFD=4` in
> > `test-lib.sh` has not the intended effect?
>
> Meh, thanks for the heads up. Another test gap in GitLab CI which I'm
> going to address soon via a new macOS job.
>
> In any case, Dash indeed does not honor the above envvar.

Hmm. I had a closer look and now I am thoroughly confused. In
`t/lib-bash.exe`, we make sure that this test is run in Bash. And indeed,
when I call

  BASH=1 POSIXLY_CORRECT=1 TEST_SHELL_PATH=/bin/dash dash t9902-completion.sh -iVx

I am greeted by this error message:

  git-completion.bash: Syntax error: "(" unexpected (expecting "fi")

So something else must be going on here.

> I also could not find any alternate solutions to redirect the tracing
> output. So for all I can see there are a few ways to handle this:
>
>   - `set -x` and then restore the previous value after having called
>     `run_completion`.
>
>   - Filter the output so that any line starting with "${PS4}" gets
>     removed.
>
>   - Don't test for this bug.
>
> Not sure which way to go, but the first alternative feels a bit more
> sensible to me. It does remove the ability to see what's going on in the
> completion script though in case one wants to debug it.

Personally, I would go for option 2, filtering out the xtrace output. This
here seems to work:

-- snip --
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index b14ae4de14e..23cd1cd9508 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -87,11 +87,14 @@ test_completion ()
 	else
 		sed -e 's/Z$//' |sort >expected
 	fi &&
+	PS4=+ &&
 	run_completion "$1" >"$TRASH_DIRECTORY"/output 2>&1 &&
+	sed "/^+/{:1;s/'[^']*'//g;/'/{N;b1};d}" \
+		<"$TRASH_DIRECTORY"/output >"$TRASH_DIRECTORY"/output.x &&
+	test_must_be_empty "$TRASH_DIRECTORY"/output.x &&
+	rm "$TRASH_DIRECTORY"/output "$TRASH_DIRECTORY"/output.x &&
 	sort out >out_sorted &&
-	test_cmp expected out_sorted &&
-	test_must_be_empty "$TRASH_DIRECTORY"/output &&
-	rm "$TRASH_DIRECTORY"/output
+	test_cmp expected out_sorted
 }

 # Test __gitcomp.
-- snap --

It is a bit ugly, in particular the `sed` expression (which is a bit
complex because the `output` file can contain multi-line strings enclosed
in single-quotes), and we could probably lose the `"$TRASH_DIRECTORY"/`
part to improve the reading experience somewhat.

But my main concern is: Why does this happen in the first place? If we are
running with Bash, why does `BASH_XTRACEFD` to work as intended here and
makes it necessary to filter out the traced commands?

Ciao,
Johannes

^ permalink raw reply related

* Re: [GSoC][RFC] Replace use of atoi() with strtol_i(), as a microproject
From: Mohit Marathe @ 2024-01-12 13:04 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Christian Couder, git@vger.kernel.org, britton.kerin@gmail.com,
	peff@peff.net
In-Reply-To: <xmqqa5pdav04.fsf@gitster.g>

 > > I took a closer look at `builtin/patch-id.c` and it seems replacing
> > `atoi()` (which is used to parse numbers in the hunk header) wouldn't
> > improve anything, unless I'm missing something.
> 
> 
> You can of course notice an invalid patch that places non-digit to
> the hunk header and error out with such a change. If we are reading
> output from Git that we are invoking, hopefully we will not see such
> an invalid patch, but the command is designed to read arbitrary
> input like a patch e-mail you received over the network, so I do not
> think it is fair to say there is no merit to such a change, even
> though I agree that it may not matter all that much.
> 
> A corrupt patch may be getting a nonsense patch-ID with the current
> code and hopefully is not matching other patches that are not
> corrupt, but with such a change, a corrupt patch may not be getting
> any patch-ID and a loop that computes patch-ID for many files and
> try to match them up might need to be rewritten to take the new
> failure case into account.

Oh, I didn't know corrupt patches were a thing. I will start working 
on the patch for this.

^ permalink raw reply

* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: phillip.wood123 @ 2024-01-12 11:01 UTC (permalink / raw)
  To: Michael Lohmann, git
In-Reply-To: <20240111233311.64734-1-mi.al.lohmann@gmail.com>

Hi Michael

On 11/01/2024 23:33, Michael Lohmann wrote:
> This extends the functionality of `git log --merge` to also work with
> conflicts for rebase, cherry pick and revert.
> 
> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> ---
> Hi everybody!
> 
> When Phillip Wood gave me some nice hints on his workflow concerning
> conflicts [1] (we discussed if `--show-current-patch` would make sense
> for cherry pick/revert). When I learned about `git log --merge` I was a
> bit sad to discover that those do not exist for rebase/revert/cherry
> pick since they look really valuable for getting an understanding on
> what was changed. It is basically the counterpart to
> `git show ${ACTION}_HEAD` for understanding the source of the conflict.
> 
> I am curious if also other people would be interested in having an easy
> way to get a log of only the relevant commits touching conflicting files
> for said actions.
> 
> With this patch I think the functionality is there, just hijacking the
> `--merge` code - maybe an alias like `git log --conflict` or similar
> might be more descriptive now?
> 
> What do you think about this idea? (Or am I maybe missing an easy way to
> achieve it with the existing code as well?)

I should start by saying that I didn't know "git log --merge" existed 
before I saw this message so please correct me if I've misunderstood 
what this patch is doing. If I understand correctly it shows the commits 
from each side of the merge and is equivalent to

     git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD)

When a commit is cherry-picked the merge base used is CHERRY_PICK_HEAD^ 
[*] so I'm not sure what

     git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD)

tells us. Indeed there HEAD and CHERRY_PICK_HEAD may not share a common 
ancestor. As I say I've not used "git log --merge" so maybe I'm missing 
the reason why it would be useful when cherry-picking or rebasing.

Best Wishes

Phillip

[*] assuming we're not picking a merge commit

> Michael
> 
> 
> [1] https://lore.kernel.org/git/cfba7098-3c23-4a81-933c-b7fefdedec8a@gmail.com/
> 
>   revision.c | 22 ++++++++++++++++++----
>   1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/revision.c b/revision.c
> index 2424c9bd67..2e5c00dabd 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs,
>   	}
>   }
>   
> +static char* get_action_head_name(struct object_id* oid)
> +{
> +	if (!repo_get_oid(the_repository, "MERGE_HEAD", oid)) {
> +		return "MERGE_HEAD";
> +	} else if (!repo_get_oid(the_repository, "REBASE_HEAD", oid)) {
> +		return "REBASE_HEAD";
> +	} else if (!repo_get_oid(the_repository, "CHERRY_PICK_HEAD", oid)) {
> +		return "CHERRY_PICK_HEAD";
> +	} else if (!repo_get_oid(the_repository, "REVERT_HEAD", oid)) {
> +		return "REVERT_HEAD";
> +	} else
> +		die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?");
> +}
> +
>   static void prepare_show_merge(struct rev_info *revs)
>   {
>   	struct commit_list *bases;
>   	struct commit *head, *other;
>   	struct object_id oid;
>   	const char **prune = NULL;
> +	const char *action_head_name;
>   	int i, prune_num = 1; /* counting terminating NULL */
>   	struct index_state *istate = revs->repo->index;
>   
>   	if (repo_get_oid(the_repository, "HEAD", &oid))
>   		die("--merge without HEAD?");
>   	head = lookup_commit_or_die(&oid, "HEAD");
> -	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> -		die("--merge without MERGE_HEAD?");
> -	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> +	action_head_name = get_action_head_name(&oid);
> +	other = lookup_commit_or_die(&oid, action_head_name);
>   	add_pending_object(revs, &head->object, "HEAD");
> -	add_pending_object(revs, &other->object, "MERGE_HEAD");
> +	add_pending_object(revs, &other->object, action_head_name);
>   	bases = repo_get_merge_bases(the_repository, head, other);
>   	add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);
>   	add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);

^ permalink raw reply

* Re: [PATCH 1/2] t9902: verify that completion does not print anything
From: Patrick Steinhardt @ 2024-01-12 10:50 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Stan Hu
In-Reply-To: <d978494d-6c48-5923-4745-c42a39e1187a@gmx.de>

[-- Attachment #1: Type: text/plain, Size: 3285 bytes --]

On Fri, Jan 12, 2024 at 11:08:21AM +0100, Johannes Schindelin wrote:
> Hi Patrick,
> 
> On Thu, 11 Jan 2024, Patrick Steinhardt wrote:
> 
> > The Bash completion script must not print anything to either stdout or
> > stderr. Instead, it is only expected to populate certain variables.
> > Tighten our `test_completion ()` test helper to verify this requirement.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  t/t9902-completion.sh | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> > index aa9a614de3..78cb93bea7 100755
> > --- a/t/t9902-completion.sh
> > +++ b/t/t9902-completion.sh
> > @@ -87,9 +87,11 @@ test_completion ()
> >  	else
> >  		sed -e 's/Z$//' |sort >expected
> >  	fi &&
> > -	run_completion "$1" &&
> > +	run_completion "$1" >"$TRASH_DIRECTORY"/output 2>&1 &&
> >  	sort out >out_sorted &&
> > -	test_cmp expected out_sorted
> > +	test_cmp expected out_sorted &&
> > +	test_must_be_empty "$TRASH_DIRECTORY"/output &&
> 
> It seems that this fails CI on macOS, most likely because we're running
> with `set -x` and that output somehow ends up in `output`, see e.g. here:
> https://github.com/git/git/actions/runs/7496790359/job/20409405194#step:4:1880
> 
>   [...]
>   ++ test_completion 'git switch '
>   ++ test 1 -gt 1
>   ++ sed -e 's/Z$//'
>   ++ sort
>   ++ run_completion 'git switch '
>   ++ sort out
>   ++ test_cmp expected out_sorted
>   ++ test 2 -ne 2
>   ++ eval 'diff -u' '"$@"'
>   +++ diff -u expected out_sorted
>   ++ test_must_be_empty '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
>   ++ test 1 -ne 1
>   ++ test_path_is_file '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
>   ++ test 1 -ne 1
>   ++ test -f '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
>   ++ test -s '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
>   ++ echo ''\''/Users/runner/work/git/git/t/trash directory.t9902-completion/output'\'' is not empty, it contains:'
>   '/Users/runner/work/git/git/t/trash directory.t9902-completion/output' is not empty, it contains:
>   ++ cat '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
>   ++ local -a COMPREPLY _words
>   ++ local _cword
>   [...]
> 
> Maybe this is running in Dash and therefore `BASH_XTRACEFD=4` in
> `test-lib.sh` has not the intended effect?

Meh, thanks for the heads up. Another test gap in GitLab CI which I'm
going to address soon via a new macOS job.

In any case, Dash indeed does not honor the above envvar. I also could
not find any alternate solutions to redirect the tracing output. So for
all I can see there are a few ways to handle this:

  - `set -x` and then restore the previous value after having called
    `run_completion`.

  - Filter the output so that any line starting with "${PS4}" gets
    removed. 

  - Don't test for this bug.

Not sure which way to go, but the first alternative feels a bit more
sensible to me. It does remove the ability to see what's going on in the
completion script though in case one wants to debug it.

I'll reroll this patch series early next week.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c
From: Achu Luma @ 2024-01-12 10:27 UTC (permalink / raw)
  To: git
  Cc: chriscool, christian.couder, gitster, l.s.r, me, phillip.wood123,
	phillip.wood, steadmon, Achu Luma
In-Reply-To: <20240105161413.10422-1-ach.lumap@gmail.com>

In the recent codebase update (8bf6fbd00d (Merge branch
'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
merged, providing a standardized approach for testing C code. Prior to
this update, some unit tests relied on the test helper mechanism,
lacking a dedicated unit testing framework. It's more natural to perform
these unit tests using the new unit test framework.

This commit migrates the unit tests for C character classification
functions (isdigit(), isspace(), etc) from the legacy approach
using the test-tool command `test-tool ctype` in t/helper/test-ctype.c
to the new unit testing framework (t/unit-tests/test-lib.h).

The migration involves refactoring the tests to utilize the testing
macros provided by the framework (TEST() and check_*()).

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: René Scharfe <l.s.r@web.de>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
 The change between version 4 and version 5 is:
 - Added tests to handle EOF.

 Thanks to Phillip for noticing the missing tests..
 Here is a diff between v4 and v5:

 +       if (!check(!func(EOF))) \
 +                       test_msg("      i: 0x%02x (EOF)", EOF); \

 Thanks also to René, Phillip, Junio and Taylor who helped with
 previous versions.

 Makefile               |  2 +-
 t/helper/test-ctype.c  | 70 ------------------------------------
 t/helper/test-tool.c   |  1 -
 t/helper/test-tool.h   |  1 -
 t/t0070-fundamental.sh |  4 ---
 t/unit-tests/t-ctype.c | 80 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 81 insertions(+), 77 deletions(-)
 delete mode 100644 t/helper/test-ctype.c
 create mode 100644 t/unit-tests/t-ctype.c

diff --git a/Makefile b/Makefile
index 15990ff312..1a62e48759 100644
--- a/Makefile
+++ b/Makefile
@@ -792,7 +792,6 @@ TEST_BUILTINS_OBJS += test-chmtime.o
 TEST_BUILTINS_OBJS += test-config.o
 TEST_BUILTINS_OBJS += test-crontab.o
 TEST_BUILTINS_OBJS += test-csprng.o
-TEST_BUILTINS_OBJS += test-ctype.o
 TEST_BUILTINS_OBJS += test-date.o
 TEST_BUILTINS_OBJS += test-delta.o
 TEST_BUILTINS_OBJS += test-dir-iterator.o
@@ -1342,6 +1341,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
 UNIT_TEST_PROGRAMS += t-basic
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-strbuf
+UNIT_TEST_PROGRAMS += t-ctype
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
deleted file mode 100644
index e5659df40b..0000000000
--- a/t/helper/test-ctype.c
+++ /dev/null
@@ -1,70 +0,0 @@
-#include "test-tool.h"
-
-static int rc;
-
-static void report_error(const char *class, int ch)
-{
-	printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
-	rc = 1;
-}
-
-static int is_in(const char *s, int ch)
-{
-	/*
-	 * We can't find NUL using strchr. Accept it as the first
-	 * character in the spec -- there are no empty classes.
-	 */
-	if (ch == '\0')
-		return ch == *s;
-	if (*s == '\0')
-		s++;
-	return !!strchr(s, ch);
-}
-
-#define TEST_CLASS(t,s) {			\
-	int i;					\
-	for (i = 0; i < 256; i++) {		\
-		if (is_in(s, i) != t(i))	\
-			report_error(#t, i);	\
-	}					\
-	if (t(EOF))				\
-		report_error(#t, EOF);		\
-}
-
-#define DIGIT "0123456789"
-#define LOWER "abcdefghijklmnopqrstuvwxyz"
-#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
-#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
-#define ASCII \
-	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
-	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
-	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
-	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
-	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
-	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
-	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
-	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
-#define CNTRL \
-	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
-	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
-	"\x7f"
-
-int cmd__ctype(int argc UNUSED, const char **argv UNUSED)
-{
-	TEST_CLASS(isdigit, DIGIT);
-	TEST_CLASS(isspace, " \n\r\t");
-	TEST_CLASS(isalpha, LOWER UPPER);
-	TEST_CLASS(isalnum, LOWER UPPER DIGIT);
-	TEST_CLASS(is_glob_special, "*?[\\");
-	TEST_CLASS(is_regex_special, "$()*+.?[\\^{|");
-	TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~");
-	TEST_CLASS(isascii, ASCII);
-	TEST_CLASS(islower, LOWER);
-	TEST_CLASS(isupper, UPPER);
-	TEST_CLASS(iscntrl, CNTRL);
-	TEST_CLASS(ispunct, PUNCT);
-	TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF");
-	TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " ");
-
-	return rc;
-}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 37ba996539..33b9501c21 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {
 	{ "config", cmd__config },
 	{ "crontab", cmd__crontab },
 	{ "csprng", cmd__csprng },
-	{ "ctype", cmd__ctype },
 	{ "date", cmd__date },
 	{ "delta", cmd__delta },
 	{ "dir-iterator", cmd__dir_iterator },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 8a1a7c63da..b72f07ded9 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -12,7 +12,6 @@ int cmd__chmtime(int argc, const char **argv);
 int cmd__config(int argc, const char **argv);
 int cmd__crontab(int argc, const char **argv);
 int cmd__csprng(int argc, const char **argv);
-int cmd__ctype(int argc, const char **argv);
 int cmd__date(int argc, const char **argv);
 int cmd__delta(int argc, const char **argv);
 int cmd__dir_iterator(int argc, const char **argv);
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 487bc8d905..a4756fbab9 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh

-test_expect_success 'character classes (isspace, isalpha etc.)' '
-	test-tool ctype
-'
-
 test_expect_success 'mktemp to nonexistent directory prints filename' '
 	test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&
 	grep "doesnotexist/test" err
diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
new file mode 100644
index 0000000000..f315489984
--- /dev/null
+++ b/t/unit-tests/t-ctype.c
@@ -0,0 +1,80 @@
+#include "test-lib.h"
+
+static int is_in(const char *s, int ch)
+{
+	/*
+	 * We can't find NUL using strchr. Accept it as the first
+	 * character in the spec -- there are no empty classes.
+	 */
+	if (ch == '\0')
+		return ch == *s;
+	if (*s == '\0')
+		s++;
+	return !!strchr(s, ch);
+}
+
+/* Macro to test a character type */
+#define TEST_CTYPE_FUNC(func, string) \
+static void test_ctype_##func(void) { \
+	for (int i = 0; i < 256; i++) { \
+		if (!check_int(func(i), ==, is_in(string, i))) \
+			test_msg("       i: 0x%02x", i); \
+	} \
+	if (!check(!func(EOF))) \
+			test_msg("      i: 0x%02x (EOF)", EOF); \
+}
+
+#define TEST_CHAR_CLASS(class) TEST(test_ctype_##class(), #class " works")
+
+#define DIGIT "0123456789"
+#define LOWER "abcdefghijklmnopqrstuvwxyz"
+#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
+#define ASCII \
+	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
+	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
+	"\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
+	"\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
+	"\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
+	"\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
+	"\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
+	"\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
+#define CNTRL \
+	"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
+	"\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
+	"\x7f"
+
+TEST_CTYPE_FUNC(isdigit, DIGIT)
+TEST_CTYPE_FUNC(isspace, " \n\r\t")
+TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
+TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
+TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
+TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
+TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
+TEST_CTYPE_FUNC(isascii, ASCII)
+TEST_CTYPE_FUNC(islower, LOWER)
+TEST_CTYPE_FUNC(isupper, UPPER)
+TEST_CTYPE_FUNC(iscntrl, CNTRL)
+TEST_CTYPE_FUNC(ispunct, PUNCT)
+TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
+TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
+
+int cmd_main(int argc, const char **argv) {
+	/* Run all character type tests */
+	TEST_CHAR_CLASS(isspace);
+	TEST_CHAR_CLASS(isdigit);
+	TEST_CHAR_CLASS(isalpha);
+	TEST_CHAR_CLASS(isalnum);
+	TEST_CHAR_CLASS(is_glob_special);
+	TEST_CHAR_CLASS(is_regex_special);
+	TEST_CHAR_CLASS(is_pathspec_magic);
+	TEST_CHAR_CLASS(isascii);
+	TEST_CHAR_CLASS(islower);
+	TEST_CHAR_CLASS(isupper);
+	TEST_CHAR_CLASS(iscntrl);
+	TEST_CHAR_CLASS(ispunct);
+	TEST_CHAR_CLASS(isxdigit);
+	TEST_CHAR_CLASS(isprint);
+
+	return test_done();
+}
--
2.42.0.windows.2


^ permalink raw reply related

* [Outreachy][PATCH] Port helper/test-advise.c to unit-tests/t-advise.c
From: Achu Luma @ 2024-01-12 10:21 UTC (permalink / raw)
  To: git; +Cc: christian.couder, Achu Luma, Christian Couder

In the recent codebase update (8bf6fbd00d (Merge branch
'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
merged, providing a standardized approach for testing C code. Prior to
this update, some unit tests relied on the test helper mechanism,
lacking a dedicated unit testing framework. It's more natural to perform
these unit tests using the new unit test framework.

This commit migrates the unit tests for advise_if_enabled functionality
from the legacy approach using the test-tool command `test-tool advise`
in t/helper/test-advise.c to the new unit testing framework
(t/unit-tests/test-lib.h).

The migration involves refactoring the tests to utilize the testing
macros provided by the framework (TEST() and check_*()).

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
 Makefile                |  2 +-
 t/helper/test-advise.c  | 22 -----------------
 t/helper/test-tool.c    |  1 -
 t/helper/test-tool.h    |  1 -
 t/t0018-advice.sh       | 33 -------------------------
 t/unit-tests/t-advise.c | 53 +++++++++++++++++++++++++++++++++++++++++
 6 files changed, 54 insertions(+), 58 deletions(-)
 delete mode 100644 t/helper/test-advise.c
 delete mode 100755 t/t0018-advice.sh
 create mode 100644 t/unit-tests/t-advise.c

diff --git a/Makefile b/Makefile
index 15990ff312..27916e4341 100644
--- a/Makefile
+++ b/Makefile
@@ -783,7 +783,6 @@ X =

 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))

-TEST_BUILTINS_OBJS += test-advise.o
 TEST_BUILTINS_OBJS += test-bitmap.o
 TEST_BUILTINS_OBJS += test-bloom.o
 TEST_BUILTINS_OBJS += test-bundle-uri.o
@@ -1342,6 +1341,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
 UNIT_TEST_PROGRAMS += t-basic
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-strbuf
+UNIT_TEST_PROGRAMS += t-advise
 UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
 UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c
deleted file mode 100644
index 8a3fd0009a..0000000000
--- a/t/helper/test-advise.c
+++ /dev/null
@@ -1,22 +0,0 @@
-#include "test-tool.h"
-#include "advice.h"
-#include "config.h"
-#include "setup.h"
-
-int cmd__advise_if_enabled(int argc, const char **argv)
-{
-	if (argc != 2)
-		die("usage: %s <advice>", argv[0]);
-
-	setup_git_directory();
-	git_config(git_default_config, NULL);
-
-	/*
-	 * Any advice type can be used for testing, but NESTED_TAG was
-	 * selected here and in t0018 where this command is being
-	 * executed.
-	 */
-	advise_if_enabled(ADVICE_NESTED_TAG, "%s", argv[1]);
-
-	return 0;
-}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 37ba996539..e7f7326ce6 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -10,7 +10,6 @@ static const char * const test_tool_usage[] = {
 };

 static struct test_cmd cmds[] = {
-	{ "advise", cmd__advise_if_enabled },
 	{ "bitmap", cmd__bitmap },
 	{ "bloom", cmd__bloom },
 	{ "bundle-uri", cmd__bundle_uri },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 8a1a7c63da..68751dda66 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -3,7 +3,6 @@

 #include "git-compat-util.h"

-int cmd__advise_if_enabled(int argc, const char **argv);
 int cmd__bitmap(int argc, const char **argv);
 int cmd__bloom(int argc, const char **argv);
 int cmd__bundle_uri(int argc, const char **argv);
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
deleted file mode 100755
index c13057a4ca..0000000000
--- a/t/t0018-advice.sh
+++ /dev/null
@@ -1,33 +0,0 @@
-#!/bin/sh
-
-test_description='Test advise_if_enabled functionality'
-
-TEST_PASSES_SANITIZE_LEAK=true
-. ./test-lib.sh
-
-test_expect_success 'advice should be printed when config variable is unset' '
-	cat >expect <<-\EOF &&
-	hint: This is a piece of advice
-	hint: Disable this message with "git config advice.nestedTag false"
-	EOF
-	test-tool advise "This is a piece of advice" 2>actual &&
-	test_cmp expect actual
-'
-
-test_expect_success 'advice should be printed when config variable is set to true' '
-	cat >expect <<-\EOF &&
-	hint: This is a piece of advice
-	hint: Disable this message with "git config advice.nestedTag false"
-	EOF
-	test_config advice.nestedTag true &&
-	test-tool advise "This is a piece of advice" 2>actual &&
-	test_cmp expect actual
-'
-
-test_expect_success 'advice should not be printed when config variable is set to false' '
-	test_config advice.nestedTag false &&
-	test-tool advise "This is a piece of advice" 2>actual &&
-	test_must_be_empty actual
-'
-
-test_done
diff --git a/t/unit-tests/t-advise.c b/t/unit-tests/t-advise.c
new file mode 100644
index 0000000000..15df29c955
--- /dev/null
+++ b/t/unit-tests/t-advise.c
@@ -0,0 +1,53 @@
+#include "test-lib.h"
+#include "advice.h"
+#include "config.h"
+#include "setup.h"
+#include "strbuf.h"
+
+static const char expect_advice_msg[] = "hint: This is a piece of advice\n"
+					"hint: Disable this message with \"git config advice.nestedTag false\"\n";
+static const char advice_msg[] = "This is a piece of advice";
+static const char out_file[] = "./output.txt";
+
+
+static void check_advise_if_enabled(const char *argv, const char *conf_val, const char *expect) {
+	FILE *file;
+	struct strbuf actual = STRBUF_INIT;
+
+	if (conf_val)
+		git_default_advice_config("advice.nestedTag", conf_val);
+
+	file = freopen(out_file, "w", stderr);
+	if (!check(!!file)) {
+		test_msg("Error opening file %s", out_file);
+		return;
+	}
+
+	advise_if_enabled(ADVICE_NESTED_TAG, "%s", argv);
+	fclose(file);
+
+	if (!check(strbuf_read_file(&actual, out_file, 0) >= 0)) {
+		test_msg("Error reading file %s", out_file);
+		strbuf_release(&actual);
+		return;
+	}
+
+	check_str(actual.buf, expect);
+	strbuf_release(&actual);
+
+	if (!check(remove(out_file) == 0))
+		test_msg("Error deleting %s", out_file);
+}
+
+int cmd_main(int argc, const char **argv) {
+	setenv("TERM", "dumb", 1);
+
+	TEST(check_advise_if_enabled(advice_msg, NULL, expect_advice_msg),
+		"advice should be printed when config variable is unset");
+	TEST(check_advise_if_enabled(advice_msg, "true", expect_advice_msg),
+		"advice should be printed when config variable is set to true");
+	TEST(check_advise_if_enabled(advice_msg, "false", ""),
+		"advice should not be printed when config variable is set to false");
+
+	return test_done();
+}
--
2.42.0.windows.2


^ permalink raw reply related

* Re: [PATCH 1/2] t9902: verify that completion does not print anything
From: Johannes Schindelin @ 2024-01-12 10:08 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, Stan Hu
In-Reply-To: <73406ca9c8f38ac2ad8f0e32d6d81f1566a6b4d1.1704969119.git.ps@pks.im>

Hi Patrick,

On Thu, 11 Jan 2024, Patrick Steinhardt wrote:

> The Bash completion script must not print anything to either stdout or
> stderr. Instead, it is only expected to populate certain variables.
> Tighten our `test_completion ()` test helper to verify this requirement.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t9902-completion.sh | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index aa9a614de3..78cb93bea7 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -87,9 +87,11 @@ test_completion ()
>  	else
>  		sed -e 's/Z$//' |sort >expected
>  	fi &&
> -	run_completion "$1" &&
> +	run_completion "$1" >"$TRASH_DIRECTORY"/output 2>&1 &&
>  	sort out >out_sorted &&
> -	test_cmp expected out_sorted
> +	test_cmp expected out_sorted &&
> +	test_must_be_empty "$TRASH_DIRECTORY"/output &&

It seems that this fails CI on macOS, most likely because we're running
with `set -x` and that output somehow ends up in `output`, see e.g. here:
https://github.com/git/git/actions/runs/7496790359/job/20409405194#step:4:1880

  [...]
  ++ test_completion 'git switch '
  ++ test 1 -gt 1
  ++ sed -e 's/Z$//'
  ++ sort
  ++ run_completion 'git switch '
  ++ sort out
  ++ test_cmp expected out_sorted
  ++ test 2 -ne 2
  ++ eval 'diff -u' '"$@"'
  +++ diff -u expected out_sorted
  ++ test_must_be_empty '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
  ++ test 1 -ne 1
  ++ test_path_is_file '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
  ++ test 1 -ne 1
  ++ test -f '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
  ++ test -s '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
  ++ echo ''\''/Users/runner/work/git/git/t/trash directory.t9902-completion/output'\'' is not empty, it contains:'
  '/Users/runner/work/git/git/t/trash directory.t9902-completion/output' is not empty, it contains:
  ++ cat '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
  ++ local -a COMPREPLY _words
  ++ local _cword
  [...]

Maybe this is running in Dash and therefore `BASH_XTRACEFD=4` in
`test-lib.sh` has not the intended effect?

Ciao,
Johannes

^ permalink raw reply

* [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()
From: Rubén Justo @ 2024-01-12 10:05 UTC (permalink / raw)
  To: Git List, Junio C Hamano, Taylor Blau, Jeff King, Dragan Simic
In-Reply-To: <7c68392c-af2f-4999-ae64-63221bf7833a@gmail.com>

Using advise_if_enabled() to display an advice will automatically
include instructions on how to disable the advice, alongside the main
advice:

	hint: use --reapply-cherry-picks to include skipped commits
	hint: Disable this message with "git config advice.skippedCherryPicks false"

To do so, we provide a knob which can be used to disable the advice.

But also to tell us the opposite: to show the advice.

Let's not include the deactivation instructions for an advice if the
user explicitly sets its visibility.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

I hope this patch captures most of the comments from the previous
iteration, mostly:

- Allow to disable the disabling instructions, per advice.
- No new test is needed, therefore the first two commits from the
  previous iteration are not needed here.

Thanks.

 advice.c          | 109 +++++++++++++++++++++++++---------------------
 t/t0018-advice.sh |   1 -
 2 files changed, 59 insertions(+), 51 deletions(-)

diff --git a/advice.c b/advice.c
index f6e4c2f302..8474966ce1 100644
--- a/advice.c
+++ b/advice.c
@@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)
 	return "";
 }
 
+enum advice_level {
+	ADVICE_LEVEL_DEFAULT = 0,
+	ADVICE_LEVEL_DISABLED,
+	ADVICE_LEVEL_ENABLED,
+};
+
 static struct {
 	const char *key;
-	int enabled;
+	enum advice_level level;
 } advice_setting[] = {
-	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo", 1 },
-	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec", 1 },
-	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile", 1 },
-	[ADVICE_AMBIGUOUS_FETCH_REFSPEC]		= { "ambiguousFetchRefspec", 1 },
-	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir", 1 },
-	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName", 1 },
-	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge", 1 },
-	[ADVICE_DETACHED_HEAD]				= { "detachedHead", 1 },
-	[ADVICE_DIVERGING]				= { "diverging", 1 },
-	[ADVICE_FETCH_SHOW_FORCED_UPDATES]		= { "fetchShowForcedUpdates", 1 },
-	[ADVICE_FORCE_DELETE_BRANCH]			= { "forceDeleteBranch", 1 },
-	[ADVICE_GRAFT_FILE_DEPRECATED]			= { "graftFileDeprecated", 1 },
-	[ADVICE_IGNORED_HOOK]				= { "ignoredHook", 1 },
-	[ADVICE_IMPLICIT_IDENTITY]			= { "implicitIdentity", 1 },
-	[ADVICE_NESTED_TAG]				= { "nestedTag", 1 },
-	[ADVICE_OBJECT_NAME_WARNING]			= { "objectNameWarning", 1 },
-	[ADVICE_PUSH_ALREADY_EXISTS]			= { "pushAlreadyExists", 1 },
-	[ADVICE_PUSH_FETCH_FIRST]			= { "pushFetchFirst", 1 },
-	[ADVICE_PUSH_NEEDS_FORCE]			= { "pushNeedsForce", 1 },
-	[ADVICE_PUSH_NON_FF_CURRENT]			= { "pushNonFFCurrent", 1 },
-	[ADVICE_PUSH_NON_FF_MATCHING]			= { "pushNonFFMatching", 1 },
-	[ADVICE_PUSH_REF_NEEDS_UPDATE]			= { "pushRefNeedsUpdate", 1 },
-	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName", 1 },
-	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected", 1 },
-	[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]		= { "pushNonFastForward", 1 }, /* backwards compatibility */
-	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh", 1 },
-	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict", 1 },
-	[ADVICE_RM_HINTS]				= { "rmHints", 1 },
-	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse", 1 },
-	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure", 1 },
-	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks", 1 },
-	[ADVICE_STATUS_AHEAD_BEHIND_WARNING]		= { "statusAheadBehindWarning", 1 },
-	[ADVICE_STATUS_HINTS]				= { "statusHints", 1 },
-	[ADVICE_STATUS_U_OPTION]			= { "statusUoption", 1 },
-	[ADVICE_SUBMODULES_NOT_UPDATED] 		= { "submodulesNotUpdated", 1 },
-	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
-	[ADVICE_SUGGEST_DETACHING_HEAD]			= { "suggestDetachingHead", 1 },
-	[ADVICE_UPDATE_SPARSE_PATH]			= { "updateSparsePath", 1 },
-	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
-	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan", 1 },
+	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo" },
+	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec" },
+	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile" },
+	[ADVICE_AMBIGUOUS_FETCH_REFSPEC]		= { "ambiguousFetchRefspec" },
+	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir" },
+	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName" },
+	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge" },
+	[ADVICE_DETACHED_HEAD]				= { "detachedHead" },
+	[ADVICE_DIVERGING]				= { "diverging" },
+	[ADVICE_FETCH_SHOW_FORCED_UPDATES]		= { "fetchShowForcedUpdates" },
+	[ADVICE_FORCE_DELETE_BRANCH]			= { "forceDeleteBranch" },
+	[ADVICE_GRAFT_FILE_DEPRECATED]			= { "graftFileDeprecated" },
+	[ADVICE_IGNORED_HOOK]				= { "ignoredHook" },
+	[ADVICE_IMPLICIT_IDENTITY]			= { "implicitIdentity" },
+	[ADVICE_NESTED_TAG]				= { "nestedTag" },
+	[ADVICE_OBJECT_NAME_WARNING]			= { "objectNameWarning" },
+	[ADVICE_PUSH_ALREADY_EXISTS]			= { "pushAlreadyExists" },
+	[ADVICE_PUSH_FETCH_FIRST]			= { "pushFetchFirst" },
+	[ADVICE_PUSH_NEEDS_FORCE]			= { "pushNeedsForce" },
+	[ADVICE_PUSH_NON_FF_CURRENT]			= { "pushNonFFCurrent" },
+	[ADVICE_PUSH_NON_FF_MATCHING]			= { "pushNonFFMatching" },
+	[ADVICE_PUSH_REF_NEEDS_UPDATE]			= { "pushRefNeedsUpdate" },
+	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName" },
+	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected" },
+	[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]		= { "pushNonFastForward" }, /* backwards compatibility */
+	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh" },
+	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict" },
+	[ADVICE_RM_HINTS]				= { "rmHints" },
+	[ADVICE_SEQUENCER_IN_USE]			= { "sequencerInUse" },
+	[ADVICE_SET_UPSTREAM_FAILURE]			= { "setUpstreamFailure" },
+	[ADVICE_SKIPPED_CHERRY_PICKS]			= { "skippedCherryPicks" },
+	[ADVICE_STATUS_AHEAD_BEHIND_WARNING]		= { "statusAheadBehindWarning" },
+	[ADVICE_STATUS_HINTS]				= { "statusHints" },
+	[ADVICE_STATUS_U_OPTION]			= { "statusUoption" },
+	[ADVICE_SUBMODULES_NOT_UPDATED] 		= { "submodulesNotUpdated" },
+	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie" },
+	[ADVICE_SUGGEST_DETACHING_HEAD]			= { "suggestDetachingHead" },
+	[ADVICE_UPDATE_SPARSE_PATH]			= { "updateSparsePath" },
+	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor" },
+	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan" },
 };
 
 static const char turn_off_instructions[] =
@@ -116,13 +122,13 @@ void advise(const char *advice, ...)
 
 int advice_enabled(enum advice_type type)
 {
-	switch(type) {
-	case ADVICE_PUSH_UPDATE_REJECTED:
-		return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&
-		       advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;
-	default:
-		return advice_setting[type].enabled;
-	}
+	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
+
+	if (type == ADVICE_PUSH_UPDATE_REJECTED)
+		return enabled &&
+		       advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);
+
+	return enabled;
 }
 
 void advise_if_enabled(enum advice_type type, const char *advice, ...)
@@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
 		return;
 
 	va_start(params, advice);
-	vadvise(advice, 1, advice_setting[type].key, params);
+	vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,
+		advice_setting[type].key, params);
 	va_end(params);
 }
 
@@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value)
 	for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
 		if (strcasecmp(k, advice_setting[i].key))
 			continue;
-		advice_setting[i].enabled = git_config_bool(var, value);
+		advice_setting[i].level = git_config_bool(var, value)
+					  ? ADVICE_LEVEL_ENABLED
+					  : ADVICE_LEVEL_DISABLED;
 		return 0;
 	}
 
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
index c13057a4ca..0dcfb760a2 100755
--- a/t/t0018-advice.sh
+++ b/t/t0018-advice.sh
@@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' '
 test_expect_success 'advice should be printed when config variable is set to true' '
 	cat >expect <<-\EOF &&
 	hint: This is a piece of advice
-	hint: Disable this message with "git config advice.nestedTag false"
 	EOF
 	test_config advice.nestedTag true &&
 	test-tool advise "This is a piece of advice" 2>actual &&

base-commit: bec9bb4b3918d2b3c7b91bbb116a667d5d6d298d
-- 
2.42.0


^ permalink raw reply related

* Re: [PATCH v2 2/3] advice: fix an unexpected leading space
From: Rubén Justo @ 2024-01-12  9:10 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqqa5pbcpnx.fsf@gitster.g>

On 11-ene-2024 17:21:22, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > [ ... ]
> > diff --git a/advice.h b/advice.h
> > index 0f584163f5..2affbe1426 100644
> > --- a/advice.h
> > +++ b/advice.h
> > @@ -49,6 +49,7 @@ struct string_list;
> >         ADVICE_UPDATE_SPARSE_PATH,
> >         ADVICE_WAITING_FOR_EDITOR,
> >         ADVICE_SKIPPED_CHERRY_PICKS,
> > +       ADVICE_WORKTREE_ADD_ORPHAN,
> >  };
> >
> > Note the hunk header, instead of a much more expected:
> >
> > @@ -49,6 +49,7 @@ enum advice_type
> 
> Next time, don't include "diff" output in the proposed log message
> without indenting.  It makes it hard to read and parse.

My fault.  Sorry.

Is there any way to make git-format-patch issue a warning or refuse to
continue when the resulting patch is not going to be accepted by git-am?

> 
> The attached is what I queued in my tree.

Thanks.

^ permalink raw reply

* Re: [DISCUSS] Introducing Rust into the Git project
From: Sam James @ 2024-01-12  8:24 UTC (permalink / raw)
  To: brian m. carlson
  Cc: Sam James, me, git, John Paul Adrian Glaubitz, Helge Deller,
	John David Anglin, arsen, Antoni Boucher
In-Reply-To: <ZaB-ayQuGqrS-mL0@tapette.crustytoothpaste.net>


"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> [[PGP Signed Part:Undecided]]
> On 2024-01-11 at 11:45:07, Sam James wrote:
>> Something I'm a bit concerned about is that right now, neither
>> rustc_codegen_gcc nor gccrs are ready for use here.
>> 
>> We've had trouble getting things wired up for rustc_codegen_gcc
>> - which is not to speak against their wonderful efforts - because
>> the Rust community hasn't yet figured out how to handle things which
>> pure rustc supports yet. See
>> e.g. https://github.com/rust-lang/libc/pull/3032.
>
> Is this simply library support in the libc crate?  That's very easy to add.

[CC'd the rustc_codegen_gcc maintainer as well as some folks who have
tried using rustc_codegen_gcc for their distributions.]

Evidently not on the last point? ;)

Even just patching it in downstream isn't easy because you then have to
do it for many many packages. But after that PR stalling because of the
policy issue, there wasn't really anywhere to go, because of the
chicken-and-egg situation.

Let alone then, once the libc crate has it, going around and wiring up
in other crates.

The discussion on the PR seems clear that the intention is to not add
it until some policy is revised/formulated? I also don't want to have
to have that debate with every crate just because rustc doesn't support
it.

>
>> I think care should be taken in citing rustc_codegen_gcc and gccrs
>> as options for alternative platforms for now. They will hopefully
>> be great options in the future, but they aren't today, and they probably
>> won't be in the next 6 months at the least.
>
> What specifically is missing for rust_codegen_gcc?  I know gccrs is not
> ready at the moment, but I was under the impression that
> rust_codegen_gcc was at least usable.  I'm aware it requires some
> patches to GCC, but distros should be able to carry those.
>
> If rust_codegen_gcc isn't viable, then I agree we should avoid making
> Rust mandatory, but I'd like to learn more.

It's in a general state of instability. There's still *very* active work
ongoing in libgccjit (by the rust_codegen_gcc maintainer).

I'd say "you need to patch your GCC" is probably not a good state of
affairs for using something critical like git anyway, but even then,
I'm not aware of anyone having used it to build real-world common
applications using Rust for a non-rustc-supported platform, at least
not then using those builds day-to-day.

So, even if we were willing to chase the active flurry of libgccjit
patches (which is wonderful to see!), it's a significant moving
target. In Gentoo, we're probably better-placed than most people
to be able to do that, but it's still a lot of work and it doesn't
sound very robust for us to be doing for core infrastructure.

We have a lot of packages in Gentoo - partly actually stuff in the
Python ecosystem - where we're very excited to be able to use
rust_codegen_gcc (or gccrs, whichever comes first inreadiness, surely
rust_codegen_gcc) for alt platforms, but it's just not there yet.

^ permalink raw reply

* Re: [PATCH v4 4/4] archive: support remote archive from stateless transport
From: Linus Arver @ 2024-01-12  8:12 UTC (permalink / raw)
  To: Jiang Xin, Git List, Junio C Hamano; +Cc: Jiang Xin, Eric Sunshine
In-Reply-To: <18b9a11d3be9d804e8d22d054ea881b8336d170c.1702562879.git.zhiyou.jx@alibaba-inc.com>

Jiang Xin <worldhello.net@gmail.com> writes:

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> Even though we can establish a stateless connection, we still cannot
> archive the remote repository using a stateless HTTP protocol. Try the
> following steps to make it work.

As Yoda once said, "Do or do not, there is no try". Here I think you
meant "Do".

>  1. Add support for "git-upload-archive" service in "http-backend".
>
>  2. Use the URL ".../info/refs?service=git-upload-pack" to detect the
>     protocol version, instead of use the "git-upload-archive" service.
>
>  3. "git-archive" does not expect to see protocol version and
>     capabilities when connecting to remote-helper, so do not send them
>     in "remote-curl.c" for the "git-upload-archive" service.

It would be great if you could break up this patch into 3 smaller
patches. Or 4 patches if you decide to move the new test cases into their
own patch.

> @@ -723,7 +729,8 @@ static struct service_cmd {
>  	{"GET", "/objects/pack/pack-[0-9a-f]{64}\\.idx$", get_idx_file},
>  
>  	{"POST", "/git-upload-pack$", service_rpc},
> -	{"POST", "/git-receive-pack$", service_rpc}
> +	{"POST", "/git-receive-pack$", service_rpc},
> +	{"POST", "/git-upload-archive$", service_rpc}
>  };

Style nit: it might be cleaner to put the new "git-upload-archive" just
above "git-upload-pack" because the two have a special relationship now.

^ permalink raw reply

* Re: [PATCH 1/2] t1401: generalize reference locking
From: Jeff King @ 2024-01-12  8:03 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: Justin Tobler via GitGitGadget, git, Justin Tobler
In-Reply-To: <ZaDuEufXOnwH7hT4@framework>

On Fri, Jan 12, 2024 at 08:45:22AM +0100, Patrick Steinhardt wrote:

> > The obvious quick fix is to sprinkle more error() into the reftable
> > code. But in the longer term, I think the right direction is that the
> > ref code should accept an error strbuf or similar mechanism to propagate
> > human-readable error test to the caller.
> 
> Agreed, I think it's good that the reftable library itself does not
> print error messages. In this particular case we are already able to
> provide a proper error message due to the error code that the library
> returns. But there are certainly going to be other cases where it might
> make sense to pass in an error strbuf.

Oh, if there is an error code you can use already, that is even better. :)

Thanks for taking care of this case.

-Peff

^ permalink raw reply

* Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert
From: Johannes Sixt @ 2024-01-12  7:59 UTC (permalink / raw)
  To: Michael Lohmann; +Cc: phillip.wood123, git
In-Reply-To: <ce46229d-8964-4445-9a17-cff40aca1cb4@kdbg.org>

Am 12.01.24 um 08:35 schrieb Johannes Sixt:
> ... (in particular, the author date):
> https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0

The Github UI is a bit unhelpful here. The author date is Nov 11, 2014.

-- Hannes


^ permalink raw reply

* Re: [PATCH v4 3/4] transport-helper: call do_take_over() in connect_helper
From: Linus Arver @ 2024-01-12  7:56 UTC (permalink / raw)
  To: Jiang Xin, Git List, Junio C Hamano; +Cc: Jiang Xin
In-Reply-To: <af8fd2a4eb8783be4a62973bfd2135da4568570e.1702562879.git.zhiyou.jx@alibaba-inc.com>

Jiang Xin <worldhello.net@gmail.com> writes:

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> After successfully connecting to the smart transport by calling
> process_connect_service() in connect_helper(), run do_take_over() to
> replace the old vtable with a new one which has methods ready for the
> smart transport connection.
> 
>
> The connect_helper() function is used as the connect method of the
> vtable in "transport-helper.c", and it is called by transport_connect()
> in "transport.c" to setup a connection. The only place that we call
> transport_connect() so far is in "builtin/archive.c". Without running
> do_take_over(), it may fail to call transport_disconnect() in
> run_remote_archiver() of "builtin/archive.c". This is because for a
> stateless connection or a service like "git-upload-pack-archive", the

There is "git-upload-pack" and "git-upload-archive". Which one did you
mean here? Or did you mean both?

> remote helper may receive a SIGPIPE signal and exit early. To have a
> graceful disconnect method by calling do_take_over() will solve this
> issue.

Are you saying that this patch fixes an existing bug? That is, is this
patch independent of the first patch (transport-helper: no connection
restriction in connect_helper) in this series?

> The subsequent commit will introduce remote archive over a stateless-rpc
> connection.

Does the next commit depend on this patch? If not, I think you can drop
this paragraph.

> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
>  transport-helper.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 51088cc03a..3b036ae1ca 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -672,6 +672,8 @@ static int connect_helper(struct transport *transport, const char *name,
>  
>  	fd[0] = data->helper->out;
>  	fd[1] = data->helper->in;
> +
> +	do_take_over(transport);
>  	return 0;
>  }
>  
> -- 
> 2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev

^ permalink raw reply

page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox