* Re: Strange behaviour when pushing a commit object to remote's refs/HEAD
From: Pratyush Yadav @ 2024-01-16 11:33 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Pratyush Yadav, git
In-Reply-To: <CAOLa=ZS8YBhzaYx=9016KxErsMsazsF09rcuPs=-WpEGjV+ruw@mail.gmail.com>
On Tue, Jan 16 2024, Karthik Nayak wrote:
> Pratyush Yadav <me@yadavpratyush.com> writes:
>
>> Hi,
>>
>
> Hello,
>
>> I ran into a strange Magit bug, where when I ran magit-show-refs on a
>> particular repo it threw an error. The details of the Magit bug are not
>> very interesting, but when attempting to reproduce it, I also saw git
>> misbehaving for such repos.
>>
>> The strange behaviour happens when you push a commit object to remote's
>> refs/HEAD instead of pushing a symbolic ref. Such a repository can be
>> found at https://github.com/prati0100/magit-reproducer. I roughly used
>> the below steps to create such a repo:
>>
>> $ git init
>> $ echo 1 > foo && git add foo && git commit
>> $ echo 2 > bar && git add bar && git commit
>> $ git push
>> $ git checkout 79264c3
>> $ echo 2.1 > bar && git add bar && git commit
>> $ git push origin 707a3d5:refs/heads/HEAD
>>
>
> Just to note here that pushing to "refs/heads/HEAD" is not actually
> updating the remote repositories $GIT_DIR/HEAD file, rather it creates a
> new reference $GIT_DIR/refs/heads/HEAD.
Yes, that is what I would also expect. I checked one of the Git servers
we have and this is exactly what happens. $GIT_DIR/HEAD is a symref
pointing to refs/heads/main and $GIT_DIR/refs/heads/HEAD points to the
commit. But behaviour from client side is not consistent.
>
> With this understanding you'll see that this is not a bug, because the
> remote HEAD was never updated, but only a new branch called HEAD was
> created [0].
GitHub thinks so but try opening the branch. It won't show you the
commit (707a3d5, "2.1") but instead shows you 86e1c97 ("2"). So
something is wrong _at least_ with Github.
>
>> Now with such a repo, if you do `git log --all --oneline` it would look
>> something like:
>>
>> 707a3d5 (origin/HEAD) 2.1
>> 86e1c97 (HEAD -> main, origin/main) 2
>> 79264c3 1
>>
>> And running `git for-each-ref --format='%(symref:short),%(refname:short),%(refname),%(subject)' refs/remotes/origin` gives:
>>
>> ,origin,refs/remotes/origin/HEAD,2.1
>> ,origin/main,refs/remotes/origin/main,2
>>
>> All well and good so far. Now delete the repo and attempt to clone it.
>> This time `git log --all --oneline` gives:
>>
>> 86e1c97 (HEAD -> main, origin/main, origin/HEAD) 2
>> 79264c3 1
>>
>
> This is expected since you cloned the repository and you got the default
> branch 'main'.
No.
First, if I clone a repo with multiple branches (say
https://github.com/prati0100/git-gui) I get _all_ the remote branches.
Yet here I clearly don't get the so called "HEAD" branch. This is not
expected behaviour.
Second, git really does misunderstand refs/remotes/origin/HEAD. For
example, when running git for-each-ref command with the clone method, I
get:
origin/main,origin,refs/remotes/origin/HEAD,2
So it clearly thinks refs/remotes/origin/HEAD is at 86e1c97 ("2"). Or,
to be more specific, it thinks the ref points to origin/main which is at
86e1c97 ("2"). But we set it at (707a3d5, "2.1"). So it tells me the
wrong thing. Now if I do the git remote add && git remote update method,
git for-each-ref says:
,origin,refs/remotes/origin/HEAD,2.1
So now it thinks refs/remotes/origin/HEAD points at (707a3d5, "2.1"). I
do not see it as expected behaviour.
We can also see this when inspecting the contents of
.git/refs/remotes/origin/HEAD. With clone it says:
ref: refs/remotes/origin/main
With git remote add && git remote update it says:
707a3d587c61c089710e3924eb63a51763b5a4c8
The same ref points to different places based on how you pull the repo.
Looking deeper, if you clone a repo that does not have a branch called
"HEAD" (like git-gui), git creates a file in
.git/refs/remotes/origin/HEAD that says:
ref: refs/remotes/origin/master
So it certainly seems to use refs/remotes/origin/HEAD to point to the
remote's HEAD, and not as a regular branch.
I find this to be inconsistent behaviour on git's part and do not think
it is (or should be) expected behaviour.
>
>> And running `git for-each-ref --format='%(symref:short),%(refname:short),%(refname),%(subject)' refs/remotes/origin` gives:
>>
>> origin/main,origin,refs/remotes/origin/HEAD,2
>> ,origin/main,refs/remotes/origin/main,2
>>
>> So suddenly the remote's HEAD becomes origin/main (symbolic ref) and the
>> commit (707a3d5, "2.1") is nowhere to be found. It neither shows up in
>> `git rev-list --all` nor in `git log --all`. The files and trees
>> associated with it also do not show up in `git rev-list --all --object`.
>
>
> Because rev-list's `--all`, iterates over all refs. Since you only
> cloned, the HEAD branch is not pulled.
Why not? When you clone all branches should get pulled.
>
> Everything else is a consequence of the subtle but important difference
> between updating $GIT_DIR/HEAD vs creating $GIT_DIR/refs/heads/HEAD.
>
> [0]: https://github.com/prati0100/magit-reproducer/branches/all
--
Regards,
Pratyush Yadav
^ permalink raw reply
* Re: Strange behaviour when pushing a commit object to remote's refs/HEAD
From: Karthik Nayak @ 2024-01-16 13:24 UTC (permalink / raw)
To: Pratyush Yadav; +Cc: git
In-Reply-To: <mafs0a5p5pl6y.fsf@yadavpratyush.com>
[-- Attachment #1: Type: text/plain, Size: 5062 bytes --]
Pratyush Yadav <me@yadavpratyush.com> writes:
>> Just to note here that pushing to "refs/heads/HEAD" is not actually
>> updating the remote repositories $GIT_DIR/HEAD file, rather it creates a
>> new reference $GIT_DIR/refs/heads/HEAD.
>
> Yes, that is what I would also expect. I checked one of the Git servers
> we have and this is exactly what happens. $GIT_DIR/HEAD is a symref
> pointing to refs/heads/main and $GIT_DIR/refs/heads/HEAD points to the
> commit. But behaviour from client side is not consistent.
>
What is the non _consistent_ part?
>>
>> With this understanding you'll see that this is not a bug, because the
>> remote HEAD was never updated, but only a new branch called HEAD was
>> created [0].
>
> GitHub thinks so but try opening the branch. It won't show you the
> commit (707a3d5, "2.1") but instead shows you 86e1c97 ("2"). So
> something is wrong _at least_ with Github.
>
I don't know how GitHub operates, but I'm guessing because there is
ambiguity between a branch called HEAD and the actual HEAD. So this is
probably the reason.
>>
>>> Now with such a repo, if you do `git log --all --oneline` it would look
>>> something like:
>>>
>>> 707a3d5 (origin/HEAD) 2.1
>>> 86e1c97 (HEAD -> main, origin/main) 2
>>> 79264c3 1
>>>
>>> And running `git for-each-ref --format='%(symref:short),%(refname:short),%(refname),%(subject)' refs/remotes/origin` gives:
>>>
>>> ,origin,refs/remotes/origin/HEAD,2.1
>>> ,origin/main,refs/remotes/origin/main,2
>>>
>>> All well and good so far. Now delete the repo and attempt to clone it.
>>> This time `git log --all --oneline` gives:
>>>
>>> 86e1c97 (HEAD -> main, origin/main, origin/HEAD) 2
>>> 79264c3 1
>>>
>>
>> This is expected since you cloned the repository and you got the default
>> branch 'main'.
>
> No.
>
> First, if I clone a repo with multiple branches (say
> https://github.com/prati0100/git-gui) I get _all_ the remote branches.
> Yet here I clearly don't get the so called "HEAD" branch. This is not
> expected behaviour.
>
You're right, I meant to say that the remote branches don't have the
corresponding local branches. But that does not matter here.
I'm not saying that there is a path for git to work properly when
creating a branch called "HEAD". It's just that "HEAD" is more of a
reserved word for git and creating a branch with the same name has
unintended effects.
> Second, git really does misunderstand refs/remotes/origin/HEAD. For
> example, when running git for-each-ref command with the clone method, I
> get:
>
> origin/main,origin,refs/remotes/origin/HEAD,2
>
> So it clearly thinks refs/remotes/origin/HEAD is at 86e1c97 ("2"). Or,
> to be more specific, it thinks the ref points to origin/main which is at
> 86e1c97 ("2"). But we set it at (707a3d5, "2.1"). So it tells me the
> wrong thing. Now if I do the git remote add && git remote update method,
> git for-each-ref says:
>
> ,origin,refs/remotes/origin/HEAD,2.1
>
This is one of those ambiguities, we store HEAD for remotes as
$GIT_DIR/refs/remotes/<remote>/HEAD
and remote branches as
$GIT_DIR/refs/remotes/<remote>/<branch>
So what happens if there is a branch named HEAD? This is the problem
you're facing...
> So now it thinks refs/remotes/origin/HEAD points at (707a3d5, "2.1"). I
> do not see it as expected behaviour.
>
> We can also see this when inspecting the contents of
> .git/refs/remotes/origin/HEAD. With clone it says:
>
> ref: refs/remotes/origin/main
>
> With git remote add && git remote update it says:
>
> 707a3d587c61c089710e3924eb63a51763b5a4c8
>
> The same ref points to different places based on how you pull the repo.
>
> Looking deeper, if you clone a repo that does not have a branch called
> "HEAD" (like git-gui), git creates a file in
> .git/refs/remotes/origin/HEAD that says:
>
> ref: refs/remotes/origin/master
>
> So it certainly seems to use refs/remotes/origin/HEAD to point to the
> remote's HEAD, and not as a regular branch.
>
> I find this to be inconsistent behaviour on git's part and do not think
> it is (or should be) expected behaviour.
>
Maybe we should explicitly mention that using HEAD as the branch name
has unintended effects and should be avoided.
>>
>>> And running `git for-each-ref --format='%(symref:short),%(refname:short),%(refname),%(subject)' refs/remotes/origin` gives:
>>>
>>> origin/main,origin,refs/remotes/origin/HEAD,2
>>> ,origin/main,refs/remotes/origin/main,2
>>>
>>> So suddenly the remote's HEAD becomes origin/main (symbolic ref) and the
>>> commit (707a3d5, "2.1") is nowhere to be found. It neither shows up in
>>> `git rev-list --all` nor in `git log --all`. The files and trees
>>> associated with it also do not show up in `git rev-list --all --object`.
>>
>>
>> Because rev-list's `--all`, iterates over all refs. Since you only
>> cloned, the HEAD branch is not pulled.
>
> Why not? When you clone all branches should get pulled.
>
I think I jumped too quick here, it is because the branch HEAD is never
realized locally as I explained above.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* [PATCH v5 0/6] support remote archive via stateless transport
From: Jiang Xin @ 2024-01-16 13:39 UTC (permalink / raw)
To: Git List, Junio C Hamano, Linus Arver; +Cc: Jiang Xin
In-Reply-To: <cover.1702562879.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
"git archive --remote=<remote>" learned to talk over the smart
http (aka stateless) transport.
# Changes since v4
1. Change commit messages and order of commits.
2. Split the last commit of v4 into three seperate commits.
# range-diff v4...v5
1: da80391037 ! 1: f3fef46c05 transport-helper: no connection restriction in connect_helper
@@ Commit message
was for transport that supports the ".connect" method. The
"connect_helper()" function protected itself from getting called for a
transport without the method before calling process_connect_service(),
- which did not work with such a transport.
+ which only worked with the ".connect" method.
Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
2018-03-15) added a way for a transport without the ".connect" method
- to establish a "stateless" connection in protocol-v2, which
- process_connect_service() was taught to handle the "stateless"
- connection, making the old safety valve in its caller that insisted
- that ".connect" method must be defined too strict, and forgot to loosen
- it.
+ to establish a "stateless" connection in protocol-v2, where
+ process_connect_service() was taught to handle the ".stateless_connect"
+ method, making the old protection too strict. But commit edc9caf7 forgot
+ to adjust this protection accordingly.
Remove the restriction in the "connect_helper()" function and give the
function "process_connect_service()" the opportunity to establish a
@@ Commit message
protocol.
Helped-by: Junio C Hamano <gitster@pobox.com>
+ Helped-by: Linus Arver <linusa@google.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
## transport-helper.c ##
-: ---------- > 2: 6be331b22d remote-curl: supports git-upload-archive service
-: ---------- > 3: aabc8e1a2a transport-helper: protocol-v2 supports upload-archive
4: a21a80dae9 ! 4: fdab4abb43 archive: support remote archive from stateless transport
@@ Metadata
Author: Jiang Xin <zhiyou.jx@alibaba-inc.com>
## Commit message ##
- archive: support remote archive from stateless transport
+ http-backend: new rpc-service for git-upload-archive
- 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.
+ Add new rpc-service "upload-archive" in http-backend to add server side
+ support for remote archive over HTTP/HTTPS protocols.
- 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.
+ Also add new test cases in t5003. In the test case "archive remote http
+ repository", git-archive exits with a non-0 exit code even though we
+ create the archive correctly. It will be fixed in a later commit.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
if (ret)
... (omitted) ...
3: 870dc5fd21 ! 5: 6ac0c8e105 transport-helper: call do_take_over() in connect_helper
@@ Commit message
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.
+ smart transport connection. This will fix the exit code of git-archive
+ in test case "archive remote http repository" of t5003.
The connect_helper() function is used as the connect method of the
vtable in "transport-helper.c", and it is called by transport_connect()
@@ Commit message
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
+ stateless connection and a service like "git-upload-archive", the
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.
- The subsequent commit will introduce remote archive over a stateless-rpc
- connection.
-
+ Helped-by: Linus Arver <linusa@google.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
+ ## t/t5003-archive-zip.sh ##
+@@ t/t5003-archive-zip.sh: test_expect_success 'remote archive does not work with protocol v1' '
+ '
+
+ test_expect_success 'archive remote http repository' '
+- test_must_fail git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
++ git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
+ --output=remote-http.zip HEAD &&
+ test_cmp_bin d.zip remote-http.zip
+ '
+
## transport-helper.c ##
@@ transport-helper.c: static int connect_helper(struct transport *transport, const char *name,
2: 2f7060f7c5 = 6: 423a89c593 transport-helper: call do_take_over() in process_connect
Jiang Xin (6):
transport-helper: no connection restriction in connect_helper
remote-curl: supports git-upload-archive service
transport-helper: protocol-v2 supports upload-archive
http-backend: new rpc-service for git-upload-archive
transport-helper: call do_take_over() in connect_helper
transport-helper: call do_take_over() in process_connect
http-backend.c | 13 ++++++++++---
remote-curl.c | 14 +++++++++++---
t/t5003-archive-zip.sh | 34 ++++++++++++++++++++++++++++++++++
transport-helper.c | 29 +++++++++++++----------------
4 files changed, 68 insertions(+), 22 deletions(-)
--
2.43.0
^ permalink raw reply
* [PATCH v5 1/6] transport-helper: no connection restriction in connect_helper
From: Jiang Xin @ 2024-01-16 13:39 UTC (permalink / raw)
To: Git List, Junio C Hamano, Linus Arver; +Cc: Jiang Xin
In-Reply-To: <cover.1705411391.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
When commit b236752a (Support remote archive from all smart transports,
2009-12-09) added "remote archive" support for "smart transports", it
was for transport that supports the ".connect" method. The
"connect_helper()" function protected itself from getting called for a
transport without the method before calling process_connect_service(),
which only worked with the ".connect" method.
Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
2018-03-15) added a way for a transport without the ".connect" method
to establish a "stateless" connection in protocol-v2, where
process_connect_service() was taught to handle the ".stateless_connect"
method, making the old protection too strict. But commit edc9caf7 forgot
to adjust this protection accordingly.
Remove the restriction in the "connect_helper()" function and give the
function "process_connect_service()" the opportunity to establish a
connection using ".connect" or ".stateless_connect" for protocol v2. So
we can connect with a stateless-rpc and do something useful. E.g., in a
later commit, implements remote archive for a repository over HTTP
protocol.
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Linus Arver <linusa@google.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
transport-helper.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/transport-helper.c b/transport-helper.c
index 49811ef176..2e127d24a5 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
/* Get_helper so connect is inited. */
get_helper(transport);
- if (!data->connect)
- die(_("operation not supported by protocol"));
if (!process_connect_service(transport, name, exec))
die(_("can't connect to subservice %s"), name);
--
2.43.0
^ permalink raw reply related
* [PATCH v5 2/6] remote-curl: supports git-upload-archive service
From: Jiang Xin @ 2024-01-16 13:39 UTC (permalink / raw)
To: Git List, Junio C Hamano, Linus Arver; +Cc: Jiang Xin
In-Reply-To: <cover.1705411391.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Add new service (git-upload-archive) support in remote-curl, so we can
support remote archive over HTTP/HTTPS protocols. Differences between
git-upload-archive and other serices:
1. The git-archive command does not expect to see protocol version and
capabilities when connecting to remote-helper, so do not send them
in remote-curl for the git-upload-archive service.
2. We need to detect protocol version by calling discover_refs(),
Fallback to use the git-upload-pack service (which, like
git-upload-archive, is a read-only operation) to discover protocol
version.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
remote-curl.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index ef05752ca5..ce6cb8ac05 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)
* establish a stateless connection, otherwise we need to tell the
* client to fallback to using other transport helper functions to
* complete their request.
+ *
+ * The "git-upload-archive" service is a read-only operation. Fallback
+ * to use "git-upload-pack" service to discover protocol version.
*/
- discover = discover_refs(service_name, 0);
+ if (!strcmp(service_name, "git-upload-archive"))
+ discover = discover_refs("git-upload-pack", 0);
+ else
+ discover = discover_refs(service_name, 0);
if (discover->version != protocol_v2) {
printf("fallback\n");
fflush(stdout);
@@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)
/*
* Dump the capability listing that we got from the server earlier
- * during the info/refs request.
+ * during the info/refs request. This does not work with the
+ * "git-upload-archive" service.
*/
- write_or_die(rpc.in, discover->buf, discover->len);
+ if (strcmp(service_name, "git-upload-archive"))
+ write_or_die(rpc.in, discover->buf, discover->len);
/* Until we see EOF keep sending POSTs */
while (1) {
--
2.43.0
^ permalink raw reply related
* [PATCH v5 3/6] transport-helper: protocol-v2 supports upload-archive
From: Jiang Xin @ 2024-01-16 13:39 UTC (permalink / raw)
To: Git List, Junio C Hamano, Linus Arver; +Cc: Jiang Xin
In-Reply-To: <cover.1705411391.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
We used to support only git-upload-pack service for protocol-v2. In
order to support remote archive over HTTP/HTTPS protocols, add new
service support for git-upload-archive in protocol-v2.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
transport-helper.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/transport-helper.c b/transport-helper.c
index 2e127d24a5..6fe9f4f208 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -628,7 +628,8 @@ static int process_connect_service(struct transport *transport,
ret = run_connect(transport, &cmdbuf);
} else if (data->stateless_connect &&
(get_protocol_version_config() == protocol_v2) &&
- !strcmp("git-upload-pack", name)) {
+ (!strcmp("git-upload-pack", name) ||
+ !strcmp("git-upload-archive", name))) {
strbuf_addf(&cmdbuf, "stateless-connect %s\n", name);
ret = run_connect(transport, &cmdbuf);
if (ret)
--
2.43.0
^ permalink raw reply related
* [PATCH v5 4/6] http-backend: new rpc-service for git-upload-archive
From: Jiang Xin @ 2024-01-16 13:39 UTC (permalink / raw)
To: Git List, Junio C Hamano, Linus Arver; +Cc: Jiang Xin, Eric Sunshine
In-Reply-To: <cover.1705411391.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Add new rpc-service "upload-archive" in http-backend to add server side
support for remote archive over HTTP/HTTPS protocols.
Also add new test cases in t5003. In the test case "archive remote http
repository", git-archive exits with a non-0 exit code even though we
create the archive correctly. It will be fixed in a later commit.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
http-backend.c | 13 ++++++++++---
t/t5003-archive-zip.sh | 34 ++++++++++++++++++++++++++++++++++
2 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/http-backend.c b/http-backend.c
index ff07b87e64..1ed1e29d07 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -38,6 +38,7 @@ struct rpc_service {
static struct rpc_service rpc_service[] = {
{ "upload-pack", "uploadpack", 1, 1 },
{ "receive-pack", "receivepack", 0, -1 },
+ { "upload-archive", "uploadarchive", 0, -1 },
};
static struct string_list *get_parameters(void)
@@ -639,10 +640,15 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)
static void service_rpc(struct strbuf *hdr, char *service_name)
{
- const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
+ struct strvec argv = STRVEC_INIT;
struct rpc_service *svc = select_service(hdr, service_name);
struct strbuf buf = STRBUF_INIT;
+ strvec_push(&argv, svc->name);
+ if (strcmp(service_name, "git-upload-archive"))
+ strvec_push(&argv, "--stateless-rpc");
+ strvec_push(&argv, ".");
+
strbuf_reset(&buf);
strbuf_addf(&buf, "application/x-git-%s-request", svc->name);
check_content_type(hdr, buf.buf);
@@ -655,9 +661,9 @@ static void service_rpc(struct strbuf *hdr, char *service_name)
end_headers(hdr);
- argv[0] = svc->name;
- run_service(argv, svc->buffer_input);
+ run_service(argv.v, svc->buffer_input);
strbuf_release(&buf);
+ strvec_clear(&argv);
}
static int dead;
@@ -723,6 +729,7 @@ static struct service_cmd {
{"GET", "/objects/pack/pack-[0-9a-f]{64}\\.idx$", get_idx_file},
{"POST", "/git-upload-pack$", service_rpc},
+ {"POST", "/git-upload-archive$", service_rpc},
{"POST", "/git-receive-pack$", service_rpc}
};
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index fc499cdff0..6f85bd3463 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -239,4 +239,38 @@ check_zip with_untracked2
check_added with_untracked2 untracked one/untracked
check_added with_untracked2 untracked two/untracked
+# Test remote archive over HTTP protocol.
+#
+# Note: this should be the last part of this test suite, because
+# by including lib-httpd.sh, the test may end early if httpd tests
+# should not be run.
+#
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success "setup for HTTP protocol" '
+ cp -R bare.git "$HTTPD_DOCUMENT_ROOT_PATH/bare.git" &&
+ git -C "$HTTPD_DOCUMENT_ROOT_PATH/bare.git" \
+ config http.uploadpack true &&
+ set_askpass user@host pass@host
+'
+
+setup_askpass_helper
+
+test_expect_success 'remote archive does not work with protocol v1' '
+ test_must_fail git -c protocol.version=1 archive \
+ --remote="$HTTPD_URL/auth/smart/bare.git" \
+ --output=remote-http.zip HEAD >actual 2>&1 &&
+ cat >expect <<-EOF &&
+ fatal: can${SQ}t connect to subservice git-upload-archive
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'archive remote http repository' '
+ test_must_fail git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
+ --output=remote-http.zip HEAD &&
+ test_cmp_bin d.zip remote-http.zip
+'
+
test_done
--
2.43.0
^ permalink raw reply related
* [PATCH v5 5/6] transport-helper: call do_take_over() in connect_helper
From: Jiang Xin @ 2024-01-16 13:39 UTC (permalink / raw)
To: Git List, Junio C Hamano, Linus Arver; +Cc: Jiang Xin
In-Reply-To: <cover.1705411391.git.zhiyou.jx@alibaba-inc.com>
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. This will fix the exit code of git-archive
in test case "archive remote http repository" of t5003.
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 and a service like "git-upload-archive", the
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.
Helped-by: Linus Arver <linusa@google.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
t/t5003-archive-zip.sh | 2 +-
transport-helper.c | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 6f85bd3463..961c6aac25 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -268,7 +268,7 @@ test_expect_success 'remote archive does not work with protocol v1' '
'
test_expect_success 'archive remote http repository' '
- test_must_fail git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
+ git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
--output=remote-http.zip HEAD &&
test_cmp_bin d.zip remote-http.zip
'
diff --git a/transport-helper.c b/transport-helper.c
index 6fe9f4f208..91381be622 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -669,6 +669,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.43.0
^ permalink raw reply related
* [PATCH v5 6/6] transport-helper: call do_take_over() in process_connect
From: Jiang Xin @ 2024-01-16 13:39 UTC (permalink / raw)
To: Git List, Junio C Hamano, Linus Arver; +Cc: Jiang Xin
In-Reply-To: <cover.1705411391.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
The existing pattern among all callers of process_connect() seems to be
if (process_connect(...)) {
do_take_over();
... dispatch to the underlying method ...
}
... otherwise implement the fallback ...
where the return value from process_connect() is the return value of the
call it makes to process_connect_service().
Move the call of do_take_over() inside process_connect(), so that
calling the process_connect() function is more concise and will not
miss do_take_over().
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
transport-helper.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/transport-helper.c b/transport-helper.c
index 91381be622..566f7473df 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -646,6 +646,7 @@ static int process_connect(struct transport *transport,
struct helper_data *data = transport->data;
const char *name;
const char *exec;
+ int ret;
name = for_push ? "git-receive-pack" : "git-upload-pack";
if (for_push)
@@ -653,7 +654,10 @@ static int process_connect(struct transport *transport,
else
exec = data->transport_options.uploadpack;
- return process_connect_service(transport, name, exec);
+ ret = process_connect_service(transport, name, exec);
+ if (ret)
+ do_take_over(transport);
+ return ret;
}
static int connect_helper(struct transport *transport, const char *name,
@@ -685,10 +689,8 @@ static int fetch_refs(struct transport *transport,
get_helper(transport);
- if (process_connect(transport, 0)) {
- do_take_over(transport);
+ if (process_connect(transport, 0))
return transport->vtable->fetch_refs(transport, nr_heads, to_fetch);
- }
/*
* If we reach here, then the server, the client, and/or the transport
@@ -1145,10 +1147,8 @@ static int push_refs(struct transport *transport,
{
struct helper_data *data = transport->data;
- if (process_connect(transport, 1)) {
- do_take_over(transport);
+ if (process_connect(transport, 1))
return transport->vtable->push_refs(transport, remote_refs, flags);
- }
if (!remote_refs) {
fprintf(stderr,
@@ -1189,11 +1189,9 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,
{
get_helper(transport);
- if (process_connect(transport, for_push)) {
- do_take_over(transport);
+ if (process_connect(transport, for_push))
return transport->vtable->get_refs_list(transport, for_push,
transport_options);
- }
return get_refs_list_using_list(transport, for_push);
}
@@ -1277,10 +1275,8 @@ static int get_bundle_uri(struct transport *transport)
{
get_helper(transport);
- if (process_connect(transport, 0)) {
- do_take_over(transport);
+ if (process_connect(transport, 0))
return transport->vtable->get_bundle_uri(transport);
- }
return -1;
}
--
2.43.0
^ permalink raw reply related
* [PATCH] rebase: Fix documentation about used shell in -x
From: Nikolay Borisov @ 2024-01-16 14:18 UTC (permalink / raw)
To: git; +Cc: Nikolay Borisov
The shell used when using the -x option is the one pointed to by the
SHELL_PATH constant at build time. This erroneous statement in the
documentation sent me on a 10 minute wild goose chase wondering why my
$SHELL was pointing to /bin/bash and my /bin/sh to dash and git was
using dash and not bash.
Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
---
Documentation/git-rebase.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 25516c45d8b8..08cf52daf39e 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -964,7 +964,7 @@ non-0 status) to give you an opportunity to fix the problem. You can
continue with `git rebase --continue`.
The "exec" command launches the command in a shell (the one specified
-in `$SHELL`, or the default shell if `$SHELL` is not set), so you can
+by the build-time SHELL_PATH variable, usually /bin/sh), so you can
use shell features (like "cd", ">", ";" ...). The command is run from
the root of the working tree.
--
2.34.1
^ permalink raw reply related
* Re: [PATCH] rebase: Fix documentation about used shell in -x
From: Jeff King @ 2024-01-16 14:37 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: git
In-Reply-To: <20240116141842.193151-1-nik.borisov@suse.com>
On Tue, Jan 16, 2024 at 04:18:42PM +0200, Nikolay Borisov wrote:
> The shell used when using the -x option is the one pointed to by the
> SHELL_PATH constant at build time. This erroneous statement in the
> documentation sent me on a 10 minute wild goose chase wondering why my
> $SHELL was pointing to /bin/bash and my /bin/sh to dash and git was
> using dash and not bash.
Good catch. It originally used $SHELL when the documentation was added
in cd035b1cef (rebase -i: add exec command to launch a shell command,
2010-08-10). But that was lost when it was converted to C (which is
perhaps a regression, but nobody seems to have noticed or cared until
now, and at this point we should stick with the new behavior).
(I don't have an exact date since the conversion was somewhat piecemeal,
but it was done by 2018).
Since then, we use the code in run-command.c's prepare_shell_cmd().
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 25516c45d8b8..08cf52daf39e 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -964,7 +964,7 @@ non-0 status) to give you an opportunity to fix the problem. You can
> continue with `git rebase --continue`.
>
> The "exec" command launches the command in a shell (the one specified
> -in `$SHELL`, or the default shell if `$SHELL` is not set), so you can
> +by the build-time SHELL_PATH variable, usually /bin/sh), so you can
> use shell features (like "cd", ">", ";" ...). The command is run from
> the root of the working tree.
Avoiding $SHELL is obviously correct, but I think mentioning SHELL_PATH
is a little hairy. It is not used on Windows; see 776297548e (Do not use
SHELL_PATH from build system in prepare_shell_cmd on Windows, 2012-04-17).
Maybe it makes sense to just say:
...in a shell (the default one, usually /bin/sh), ...
It might even make sense to just drop the parenthetical phrase entirely.
Git executes lots of things using a shell, and it is always "the default
one", but we don't bother saying so in most places.
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] ci: add macOS jobs to GitLab CI
From: Phillip Wood @ 2024-01-16 14:58 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <d196cfd9d01fe3b52c75a1e4e0aca9f67567ab43.1705318985.git.ps@pks.im>
Hi Patrick
On 15/01/2024 11:45, Patrick Steinhardt wrote:
> Add two macOS-based jobs to GitLab CI, one for Clang and one for GCC.
This doesn't match whats in the rest of the commit message where you
explain why there is no gcc job. The patch itself looks good to me and
it is nice that we'll now be testing on arm64 with the GitLab runners.
> This matches equivalent jobs we have for GitHub Workflows, except that
> we use macOS 14 instead of macOS 13.
>
> Note that one test marked as `test_must_fail` is surprisingly passing:
>
> t7815-grep-binary.sh (Wstat: 0 Tests: 22 Failed: 0)
> TODO passed: 12
>
> This seems to boil down to an unexpected difference in how regcomp(1)
nit: regcomp(3)?
Best Wishes
Phillip
> works when matching NUL bytes. Cross-checking with the respective GitHub
> job shows though that this is not an issue unique to the GitLab CI job
> as it passes in the same way there.
>
> Further note that we do not include the equivalent for the "osx-gcc" job
> that we use with GitHub Workflows. This is because the runner for macOS
> on GitLab is running on Apple M1 machines and thus uses the "arm64"
> architecture. GCC does not support this platform yet.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> .gitlab-ci.yml | 26 +++++++++++++++++++++++++-
> ci/lib.sh | 9 ++++++++-
> 2 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 793243421c..9748970798 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -7,7 +7,7 @@ workflow:
> - if: $CI_COMMIT_TAG
> - if: $CI_COMMIT_REF_PROTECTED == "true"
>
> -test:
> +test:linux:
> image: $image
> before_script:
> - ./ci/install-docker-dependencies.sh
> @@ -52,6 +52,30 @@ test:
> - t/failed-test-artifacts
> when: on_failure
>
> +test:osx:
> + image: $image
> + tags:
> + - saas-macos-medium-m1
> + before_script:
> + - ./ci/install-dependencies.sh
> + script:
> + - ./ci/run-build-and-tests.sh
> + after_script:
> + - |
> + if test "$CI_JOB_STATUS" != 'success'
> + then
> + ./ci/print-test-failures.sh
> + fi
> + parallel:
> + matrix:
> + - jobname: osx-clang
> + image: macos-13-xcode-14
> + CC: clang
> + artifacts:
> + paths:
> + - t/failed-test-artifacts
> + when: on_failure
> +
> static-analysis:
> image: ubuntu:22.04
> variables:
> diff --git a/ci/lib.sh b/ci/lib.sh
> index f631206a44..d5dd2f2697 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -252,7 +252,14 @@ then
> CI_COMMIT="$CI_COMMIT_SHA"
> case "$CI_JOB_IMAGE" in
> macos-*)
> - CI_OS_NAME=osx;;
> + # GitLab CI has Python installed via multiple package managers,
> + # most notably via asdf and Homebrew. Ensure that our builds
> + # pick up the Homebrew one by prepending it to our PATH as the
> + # asdf one breaks tests.
> + export PATH="$(brew --prefix)/bin:$PATH"
> +
> + CI_OS_NAME=osx
> + ;;
> alpine:*|fedora:*|ubuntu:*)
> CI_OS_NAME=linux;;
> *)
^ permalink raw reply
* Re: Strange behaviour when pushing a commit object to remote's refs/HEAD
From: Jeff King @ 2024-01-16 15:00 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Pratyush Yadav, git
In-Reply-To: <CAOLa=ZRfr+oEKCo8AfFSAFtS8pbDgmG_EeBSwm7GwukzVcqSrg@mail.gmail.com>
On Tue, Jan 16, 2024 at 08:24:04AM -0500, Karthik Nayak wrote:
> This is one of those ambiguities, we store HEAD for remotes as
> $GIT_DIR/refs/remotes/<remote>/HEAD
> and remote branches as
> $GIT_DIR/refs/remotes/<remote>/<branch>
>
> So what happens if there is a branch named HEAD? This is the problem
> you're facing...
Yeah, this is a long-standing issue. The reason we have not fixed it is
that it would require a new refs/remotes layout, which implies new
lookup rules (e.g., dwim_ref() will convert the name "foo" to
"refs/remotes/foo/HEAD", but would need to be taught about the new
layout). Likewise, a new layout should probably store per-remote tags
(rather than splatting them into the main refs/tags/) along with new
dwim_ref() rules to make lookup work more or less as it does now.
So it's not impossible, but some care has to be given the design and
to handling compatibility. If anybody is interested, there are probably
some nuggets of wisdom to mine from this old thread:
https://lore.kernel.org/git/AANLkTi=yFwOAQMHhvLsB1_xmYOE9HHP2YB4H4TQzwwc8@mail.gmail.com/
In the meantime, I think the current wisdom is "don't name a branch
HEAD". ;) We even added logic to "git branch" to forbid this, but tools
like "git push" are a bit more flexible.
-Peff
^ permalink raw reply
* Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper
From: Jeff King @ 2024-01-16 15:10 UTC (permalink / raw)
To: Linus Arver; +Cc: Taylor Blau, git, Junio C Hamano
In-Reply-To: <owly8r4qi5zj.fsf@fine.c.googlers.com>
On Mon, Jan 15, 2024 at 02:31:12PM -0800, Linus Arver wrote:
> > +static int pair_chunk_expect_fn(const unsigned char *chunk_start,
> > + size_t chunk_size,
> > + void *data)
> > +{
> > + struct pair_chunk_data *pcd = data;
> > + if (chunk_size / pcd->record_size != pcd->record_nr)
> > + return -1;
> > + *pcd->p = chunk_start;
> > + return 0;
> > +}
> > +
>
> I don't think this function should assume anything about the inputs
> (chunk_size, record_size, nor record_nr). If we are asking the helper to
> be the common tool for multiple locations, it should assume even less
> about what these inputs could look like.
>
> For example, if record_size is 0 this will lead to a
> divide-by-zero. Likewise, if record_nr is zero it doesn't make
> sense to check if chunk_size fits into record_size * record_nr.
I'm not sure that divide-by-zero is a big deal, because 0-length
fixed-size records do not make any sense to ask about. And even if
somebody accidentally passed 0, even though it won't be caught by the
compiler, it would barf on any input, so even rudimentary testing would
catch it.
A zero record_nr is a perfectly reasonable thing to ask about. If you
have a chunk file with zero entries for some entity, then we are
checking that the chunk is the expected zero length as well.
> And even if there are no (unexpected) zero-values involved, shouldn't we
> also check for nonsensical comparisons, such as if chunk_size is smaller
> than record_size?
Aren't we checking that already? If chunk_size is less than record_size,
then the division will result in "0". If the expected number of records
is not also 0, then that's a bogus file.
What we really care about here is that we won't walk off the end of the
buffer while looking at N fixed-size records (in that sense, the "too
big" test is overly careful, but it's easy to include as a sanity
check).
> So in summary there appear to be the following possibilities:
>
> CHUNK_MISSING
> CHUNK_TOO_SMALL
> CHUNK_OK
> CHUNK_TOO_BIG_ALIGNED
> CHUNK_TOO_BIG_MISALIGNED
So yes, I agree all these cases exist. We detect them all except the
"misaligned" case (which I think was discussed earlier in the thread as
not worth caring too much about).
But...
> (on top of the cases where record_* inputs are zero).
>
> And it seems prudent to treat each of the not-OK cases differently
> (including how we report error messages) once we know which category we
> fall into.
I'm not sure it is worth caring too much about the different cases. From
the caller's perspective the end result is always to avoid using the
chunk/file. From the user's perspective, any error of the form "we
expected N bytes and got X" is plenty. Especially since record_nr
typically comes from another part of the file, we cannot tell if our
chunk is too big/small, or if another chunk gave us a bogus record_nr.
-Peff
^ permalink raw reply
* Re: [PATCH v2 2/5] reftable/stack: refactor reloading to use file descriptor
From: Jeff King @ 2024-01-16 15:14 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Han-Wen Nienhuys, Junio C Hamano
In-Reply-To: <ZaUC-WevQqOj31u9@tanuki>
On Mon, Jan 15, 2024 at 11:03:37AM +0100, Patrick Steinhardt wrote:
> > which means we'll feed a negative value to stat_validity_update(). I
> > think this may be OK, because I'd imagine the only sensible thing to do
> > is call stat_validity_clear() instead. And using a negative fd means
> > fstat() will fail, which will cause stat_validity_update() to clear the
> > validity struct anyway. But I thought it was worth double-checking.
>
> Good catch, and thanks a lot for double-checking. I was briefly
> wondering whether this behaviour is actually specified by POSIX. In any
> case, fstat(3P) explicitly documents `EBADF` as:
>
> The fildes argument is not a valid file descriptor.
>
> That makes me think that this code is indeed POSIX-compliant, as
> implementations are expected to handle invalid file descriptors via this
> error code.
>
> So overall this works as intended, even though I would not consider it
> to be the cleanest way to handle this. Unless you or others think that
> this should be refactored I'll leave it as-is for now though.
Thanks for confirming. I think we can leave your patch as-is. If
anything, I would say that stat_validity_update() should check for "fd <
0" itself. Not because I think fstat() is unlikely to behave differently
on some platform, but simply because it more clearly documents the
expectation.
-Peff
^ permalink raw reply
* git-bugreport-2024-01-16-2051
From: Vaibhav Naik @ 2024-01-16 15:30 UTC (permalink / raw)
To: git
[-- Attachment #1.1: Type: text/plain, Size: 1 bytes --]
[-- Attachment #1.2: Type: text/html, Size: 47 bytes --]
[-- Attachment #2: git-bugreport-2024-01-16-2051.txt --]
[-- Type: text/plain, Size: 2179 bytes --]
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
i typed this command in order:
git checkout ca35d53
git tag 1.1.2
git commit --allow-empty -m "want to commit at any cost"
(i just wanted to commit to catch the bug)
What did you expect to happen? (Expected behavior)
when I checkout, it was showing the commit id at the place of HEAD Point:
vaibh@VaibhavNaik MINGW64 /d/E Drive/MyCodes/Git-Testing ((ca35d53...))
and then after creating a new tag it was showing the tag name at the place of HEAD Point:
vaibh@VaibhavNaik MINGW64 /d/E Drive/MyCodes/Git-Testing ((1.1.2))
which is wrong, that place is to show where the HEAD is pointing at and not the newly created tag name
What happened instead? (Actual behavior)
It showed the newly created tag name at the place of the HEAD Point:
vaibh@VaibhavNaik MINGW64 /d/E Drive/MyCodes/Git-Testing ((1.1.2))
What's different between what you expected and what actually happened?
It should show the the detached HEAD commit id at the place of HEAD Point and not the newly created tag name
I proved this wrong by doing a commit which didn't got committed to any branch or tag but it moved the HEAD:
vaibh@VaibhavNaik MINGW64 /d/E Drive/MyCodes/Git-Testing ((newt))
$ git commit --allow-empty -m "want to commit at any cost"
[detached HEAD caad8ef] want to commit at any cost
vaibh@VaibhavNaik MINGW64 /d/E Drive/MyCodes/Git-Testing ((caad8ef...))
$ git status
HEAD detached from ca35d53
nothing to commit, working tree clean
it showed the tag name in the brackets
Anything else you want to add:
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.43.0.windows.1
cpu: x86_64
built from commit: 4b968f3ea3b32a7bc50846bab49f3f381841d297
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Windows 10.0 22631
compiler info: gnuc: 13.2
libc info: no libc information available
$SHELL (typically, interactive shell): C:\Program Files\Git\usr\bin\bash.exe
[Enabled Hooks]
^ permalink raw reply
* Re: [Outreachy][PATCH v5] Port helper/test-ctype.c to unit-tests/t-ctype.c
From: Junio C Hamano @ 2024-01-16 15:38 UTC (permalink / raw)
To: Phillip Wood
Cc: Achu Luma, git, chriscool, christian.couder, l.s.r, me,
phillip.wood, steadmon
In-Reply-To: <0d18a95a-543a-41de-8441-c8894d46d380@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> Thanks for adding back the test for EOF, this version looks good to me.
Thanks. Let's merge it to 'next'.
^ permalink raw reply
* Re: [PATCH v5 1/2] t7501: add tests for --include and --only
From: Junio C Hamano @ 2024-01-16 15:41 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git, phillip.wood123, christian.couder
In-Reply-To: <20240113042254.38602-2-shyamthakkar001@gmail.com>
Thanks.
"git am" auto-fixed these whitespace breakage.
.git/rebase-apply/patch:25: trailing whitespace.
.git/rebase-apply/patch:38: trailing whitespace.
#
.git/rebase-apply/patch:40: trailing whitespace.
# and is not an endorsement to the current behavior, and we may
.git/rebase-apply/patch:56: trailing whitespace.
do
.git/rebase-apply/patch:82: trailing whitespace.
warning: 5 lines applied after fixing whitespace errors.
Applying: t7501: add tests for --include and --only
^ permalink raw reply
* Re: [PATCH v5 1/2] t7501: add tests for --include and --only
From: Junio C Hamano @ 2024-01-16 15:56 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: git, phillip.wood123, christian.couder
In-Reply-To: <20240113042254.38602-2-shyamthakkar001@gmail.com>
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
> +test_expect_success 'fail to commit untracked pathspec (even with --include/--only)' '
"untracked pathspec" is a nonsense word. We do not track pathspec;
you may use pathspec to specify which path(s) to work on. I think
this is more about untracked files being ignored for the purpose of
"-i" and "-o".
You used to call this "untracked file", which probably makes more
sense.
> + # TODO: as for --include, the below command will fail because nothing is
> + # staged. If something was staged, it would not fail even if the
> + # pathspec was untracked (however, pathspec will remain untracked and
> + # staged changes would be committed.) In either cases, no error is
Both uses of "pathspec" here are nonsense. Saying "even though the
pathspec does not match any tracked path" is OK. "(however, the
untracked path that match the pathspec are not added and only the
changes in the index gets commit)" is also OK.
> + # returned to stderr like in (-o and without -o/-i) cases. In a
> + # similar manner, "git add -u baz" also does not error out.
> + #
> + # Therefore, the below test is just to document the current behavior
> + # and is not an endorsement to the current behavior, and we may
> + # want to fix this. And when that happens, this test should be
> + # updated accordingly.
OK.
> @@ -117,6 +143,55 @@ test_expect_success '--long with stuff to commit returns ok' '
> git commit -m next -a --long
> '
>
> +for opt in "" "-o" "--only"
> +do
> + test_expect_success 'exclude additional staged changes when given pathspec' '
> + echo content >>file &&
> + echo content >>baz &&
> + git add baz &&
> + git commit $opt -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
It is probably more common to say
test_write_lines baz >expect &&
git diff --name-only --cached >actual &&
test_cmp expect actual
especially when the expected pattern is a file with short lines,
instead of here-text. It makes it easier to debug tests, too,
because after "sh t7501-*.sh -i" fails, you can go to the trash
directory and the expectation as well as the actual output are
there in files.
> + git diff --name-only HEAD^ HEAD >actual &&
> + test_cmp - actual <<-EOF
> + file
> + EOF
Ditto.
This tests the most important aspect of "-o"; even when the index
has other changes relative to HEAD already, they are skipped over
and only the changes to paths that match the given pathspec are
committed, and this test demonstrates it. Well done.
> +test_expect_success '-i/--include includes staged changes' '
> + echo content >>file &&
> + echo content >>baz &&
> + git add file &&
> +
> + # baz is in the index, therefore, it will be committed
> + git commit --include -m "file and 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
Again with "test_write_lines baz file >expect", this test becomes a
bit shorter.
This tests the most important aspect of "-i"; changes to the paths
that match the given pathspec are added to the index, and recorded
together with changes added to the index already to the commit.
Well done.
> +'
> +
> +test_expect_success '--include and --only do not mix' '
> + test_when_finished "git reset --hard" &&
> + echo content >>file &&
> + echo content >>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
Very nicely done.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] tests: move t0009-prio-queue.sh to the new unit testing framework
From: Junio C Hamano @ 2024-01-16 16:36 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget; +Cc: git, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1642.v2.git.1705220304781.gitgitgadget@gmail.com>
"Chandra Pratap via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/t/unit-tests/t-prio-queue.c b/t/unit-tests/t-prio-queue.c
> new file mode 100644
> index 00000000000..07b112f5894
> --- /dev/null
> +++ b/t/unit-tests/t-prio-queue.c
> @@ -0,0 +1,99 @@
> +#include "test-lib.h"
> +#include "prio-queue.h"
> +
> +static int intcmp(const void *va, const void *vb, void *data UNUSED)
> +{
> + const int *a = va, *b = vb;
> + return *a - *b;
> +}
This came from the original and looks obviously OK.
> +static int show(int *v)
> +{
> + int ret = -1;
> + if (v)
> + ret = *v;
> + free(v);
> + return ret;
> +}
The original allowed us to differentiate "prio queue did not yield
anything" case and "the integer obtained from the queue happened to
be negative" cases, but with this, -1 is taken.
It is not a huge loss, as the tests can limit themselves to a subset
of "int" (say, positive integers only). But this realization can
lead to further simplfication.
> +static int test_prio_queue(const char **input, int *result)
This takes an array of strings, but it does not have to. Make it
take an array of "int" and then with something like
#define MISSING -1
#define GET -2
#define DUMP -3
#define STACK -4
you can get rid of strcmp(), atoi(), and xmalloc(), like so:
while (*input) {
int val = *input++;
switch (val) {
case GET:
void *peek = prio_queue_peek(&pq);
...
result[i++] = peek ? (*(int*)peek) : MISSING;
break;
... other "commands" here ...
default:
prio_queue_put(&pq, val);
break;
}
}
I inlined the show() in the above illustration, but you can keep it
as a separate small helper function. The point is that it makes it
far easier to follow by using the MISSING symbolic constant either
way.
> +{
> + struct prio_queue pq = { intcmp };
> + int i = 0;
> +
> + while (*input) {
> + if (!strcmp(*input, "get")) {
> + void *peek = prio_queue_peek(&pq);
> + void *get = prio_queue_get(&pq);
> + if (peek != get)
> + BUG("peek and get results do not match");
> + result[i++] = show(get);
> + } else if (!strcmp(*input, "dump")) {
> + void *peek;
> + void *get;
> + while ((peek = prio_queue_peek(&pq))) {
> + get = prio_queue_get(&pq);
> + if (peek != get)
> + BUG("peek and get results do not match");
> + result[i++] = show(get);
> + }
> + } else if (!strcmp(*input, "stack")) {
> + pq.compare = NULL;
> + } else if (!strcmp(*input, "reverse")) {
> + prio_queue_reverse(&pq);
> + } else {
> + int *v = xmalloc(sizeof(*v));
> + *v = atoi(*input);
> + prio_queue_put(&pq, v);
> + }
> + input++;
> + }
> +
> + clear_prio_queue(&pq);
> +
> + return 0;
> +}
> +
> +#define INPUT_SIZE 6
You do not need this (see below)
> +#define BASIC_INPUT "1", "2", "3", "4", "5", "5", "dump"
> +#define BASIC_EXPECTED 1, 2, 3, 4, 5, 5
> +
> +#define MIXED_PUT_GET_INPUT "6", "2", "4", "get", "5", "3", "get", "get", "1", "dump"
> +#define MIXED_PUT_GET_EXPECTED 2, 3, 4, 1, 5, 6
> +
> +#define EMPTY_QUEUE_INPUT "1", "2", "get", "get", "get", "1", "2", "get", "get", "get"
> +#define EMPTY_QUEUE_EXPECTED 1, 2, -1, 1, 2, -1
And these input and expectation can become all integers, i.e.
#define EMPTY_QUEUE_INPUT 1, 2, GET, GET, GET, 1, 2, GET, GET, GET
#define EMPTY_QUEUE_EXPECTED 1, 2, MISSING, 1, 2, MISSING
> +
> +#define STACK_INPUT "stack", "1", "5", "4", "6", "2", "3", "dump"
> +#define STACK_EXPECTED 3, 2, 6, 4, 5, 1
> +
> +#define REVERSE_STACK_INPUT "stack", "1", "2", "3", "4", "5", "6", "reverse", "dump"
> +#define REVERSE_STACK_EXPECTED 1, 2, 3, 4, 5, 6
> +
> +#define TEST_INPUT(INPUT, EXPECTED, name) \
> + static void test_##name(void) \
> +{ \
> + const char *input[] = {INPUT}; \
> + int expected[] = {EXPECTED}; \
> + int result[INPUT_SIZE]; \
This can be written more like
int result[ARRAY_SIZE(expected)]
so that you can freely extend each test without having to worry
about increasing the hardcoded limit.
> + test_prio_queue(input, result); \
> + check(!memcmp(expected, result, sizeof(expected))); \
> +}
> +
> +TEST_INPUT(BASIC_INPUT, BASIC_EXPECTED, basic)
> +TEST_INPUT(MIXED_PUT_GET_INPUT, MIXED_PUT_GET_EXPECTED, mixed)
> +TEST_INPUT(EMPTY_QUEUE_INPUT, EMPTY_QUEUE_EXPECTED, empty)
> +TEST_INPUT(STACK_INPUT, STACK_EXPECTED, stack)
> +TEST_INPUT(REVERSE_STACK_INPUT, REVERSE_STACK_EXPECTED, reverse)
> +
> +int cmd_main(int argc, const char **argv)
> +{
> + TEST(test_basic(), "prio-queue works for basic input");
> + TEST(test_mixed(), "prio-queue works for mixed put & get commands");
> + TEST(test_empty(), "prio-queue works when queue is empty");
> + TEST(test_stack(), "prio-queue works when used as a LIFO stack");
> + TEST(test_reverse(), "prio-queue works when LIFO stack is reversed");
> +
> + return test_done();
> +}
>
> base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
^ permalink raw reply
* Re: [PATCH v2 2/5] reftable/stack: refactor reloading to use file descriptor
From: Junio C Hamano @ 2024-01-16 16:44 UTC (permalink / raw)
To: Jeff King; +Cc: Patrick Steinhardt, git, Han-Wen Nienhuys
In-Reply-To: <20240116151447.GD2119690@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> So overall this works as intended, even though I would not consider it
>> to be the cleanest way to handle this. Unless you or others think that
>> this should be refactored I'll leave it as-is for now though.
>
> Thanks for confirming. I think we can leave your patch as-is. If
> anything, I would say that stat_validity_update() should check for "fd <
> 0" itself. Not because I think fstat() is unlikely to behave differently
> on some platform, but simply because it more clearly documents the
> expectation.
Thanks, I agree with your point that we should avoid calling system
functions that take a file descriptor with a known-invalid (like
negative) one.
^ permalink raw reply
* Re: [PATCH] rebase: Fix documentation about used shell in -x
From: Kristoffer Haugsbakk @ 2024-01-16 16:50 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: git
In-Reply-To: <20240116141842.193151-1-nik.borisov@suse.com>
Hi
Some nitpicks since it seems like there will be another round (v2):
> rebase: Fix documentation about used shell in -x
Lower-case “fix” is more conventional.[1]
> SHELL_PATH constant at build time. This erroneous statement in the
> documentation sent me on a 10 minute wild goose chase wondering why my
> $SHELL was pointing to /bin/bash and my /bin/sh to dash and git was
> using dash and not bash.
I think anecdotes are not kept in the commit message, usually? Often they
are put after the three-hyphen/three-dash line. But I didn’t manage to
find any email that says that.
The shell used when using the -x option is the one pointed to by the
SHELL_PATH constant at build time.
Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
---
This erroneous statement in the documentation sent me on a 10 minute
wild goose chase wondering why my $SHELL was pointing to /bin/bash and
my /bin/sh to dash and git was using dash and not bash.
Documentation/git-rebase.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
† 1: SubmittingPatches:
“ [[summary-section]]
The title sentence after the "area:" prefix omits the full stop at the
end, and its first word is not capitalized (the omission
of capitalization applies only to the word after the "area:"
prefix of the title)
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH] rev-list-options: fix off-by-one in '--filter=blob:limit=<n>' explainer
From: Junio C Hamano @ 2024-01-16 16:54 UTC (permalink / raw)
To: Nikolay Edigaryev via GitGitGadget; +Cc: git, Nikolay Edigaryev
In-Reply-To: <pull.1645.git.git.1705261850650.gitgitgadget@gmail.com>
"Nikolay Edigaryev via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Nikolay Edigaryev <edigaryev@gmail.com>
>
> '--filter=blob:limit=<n>' was introduced in 25ec7bcac0 (list-objects:
> filter objects in traverse_commit_list, 2017-11-21) and later expanded
> to bitmaps in 84243da129 (pack-bitmap: implement BLOB_LIMIT filtering,
> 2020-02-14)
>
> The logic that was introduced in these commits (and that still persists
> to this day) omits blobs larger than _or equal_ to n bytes or units.
Good eyes. The former does this
if (object_length < filter_data->max_bytes)
goto include_it;
and the latter does this
if (!bitmap_get(tips, pos) &&
get_size_by_pos(bitmap_git, pos) >= limit)
bitmap_unset(to_filter, pos);
> However, the documentation (Documentation/rev-list-options.txt) states:
>
>>The form '--filter=blob:limit=<n>[kmg]' omits blobs larger than n
> bytes or units. n may be zero.
>
> Moreover, the t6113-rev-list-bitmap-filters.sh tests for exactly this
> logic, so it seems it is the documentation that needs fixing, not the
> code.
Yup. The mechanism is used for things like "we do not want a large
blob, like 100MB", and a byte on the boundary does not matter all
that much in such a countext, but it does not hurt to be more
correct ;-)
> The form '--filter=blob:none' omits all blobs.
> +
> -The form '--filter=blob:limit=<n>[kmg]' omits blobs larger than n bytes
> -or units. n may be zero. The suffixes k, m, and g can be used to name
> -units in KiB, MiB, or GiB. For example, 'blob:limit=1k' is the same
> -as 'blob:limit=1024'.
> +The form '--filter=blob:limit=<n>[kmg]' omits blobs of size at least n
> +bytes or units. n may be zero. The suffixes k, m, and g can be used
> +to name units in KiB, MiB, or GiB. For example, 'blob:limit=1k'
> +is the same as 'blob:limit=1024'.
With unnecessary paragraph wrapping, it is a bit hard to compare the
preimage and the postimage, but I manually checked that this only
does
"larger than" -> "of size at least"
and nothing else, which is expected and in line with what the
proposed commit message claimed to do. Good job.
Will queue. Thanks.
> +
> The form '--filter=object:type=(tag|commit|tree|blob)' omits all objects
> which are not of the requested type.
>
> base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d
^ permalink raw reply
* Re: Suggested clarification for .gitattributes reference documentation
From: Torsten Bögershausen @ 2024-01-16 17:44 UTC (permalink / raw)
To: brian m. carlson, Michael Litwak, Matthias Aßhauer,
git@vger.kernel.org
In-Reply-To: <ZaXkt715TjNpuprG@tapette.crustytoothpaste.net>
On Tue, Jan 16, 2024 at 02:06:47AM +0000, brian m. carlson wrote:
> On 2024-01-16 at 00:19:20, Michael Litwak wrote:
> > As for documentation clarifications for the .gitattributes manpage at
> > https://git-scm.com/docs/gitattributes, I still suggest adding an
> > explicit example for UTF-16LE with BOM, and/or adding a table listing
> > which working-tree-encoding value to use for each of the following
> > UTF-16 text encodings:
> >
> > ENCODING 'working-tree-encoding' VALUE
> > ------------------- -----------------------------
> > UTF-16LE with BOM UTF-16LE-BOM
>
> I should point out that this encoding, while very common on Windows, is
> also nonstandard.
In general, I agree with everything that is snipped, thanks for the ong wordings.
[]
> (Apparently Emacs, which is not on my system, may
> permit that, which does not surprise me in the least.)
emacs seems to handle UTF-16LE-BOM just fine.
>
> > UTF-16BE with BOM UTF-16
>
[]
> I think the addition of this table is too much. UTF-16LE-BOM is common
> on Windows, and the rest are substantially less common. It's also very
> difficult to explain in a table what "UTF-16" means in an understandable
> way. And I also think it's also pretty clear that users should be using
> UTF-8 without BOM where possible.
>
> We do already mention both UTF-16, UTF-16LE, and UTF-16LE-BOM as options
> in the gitattributes manual page, and it's up to the user to know what
> their program wants and supports if that's not UTF-8.
What exactly is missing in the documentation ?
Could you please try to send us a diff (or even better a patch), so
that we can get an idea, of what can be improved ?
From my reading UTF-16LE-BOM is already mentioned.
It would be nice to see (from a user), what is probably missing.
> > Finally, I am not sure how to use git add --renormalize to correct a
> > UTF-16 file that was previously added incorrectly (i.e. with a missing
> > or incorrect working-tree-encoding entry in .gitattributes). The git
> > add documentation at https://git-scm.com/docs/git-add implies
> > 'renormalize' resets only the end-of-line values; however, I suspect
> > it also re-converts text encoding when a working-tree-encoding
> > property is set. It would be helpful to know one way or the other.
>
> It does indeed affect the working-tree-encoding. If you wanted to send
> an inline patch created with git format-patch, it would probably be
> welcome to mention that. However, because in this project we typically
> scratch our own itch, if you don't send one, it's likely nobody else
> will, either.
For the record: It will even run the "clean" filter, if it has changed,
or being freshly enabled.
So yes, a patch would be appreciated.
Thanks for bringing this up.
^ permalink raw reply
* Re: Custom merge drivers: accessing the pathnames and revisions of the files being merged
From: Junio C Hamano @ 2024-01-16 17:51 UTC (permalink / raw)
To: Antonin Delpeuch; +Cc: git
In-Reply-To: <8bb5e41e-4db9-4527-8492-3aca6a0f40bf@delpeuch.eu>
Antonin Delpeuch <antonin@delpeuch.eu> writes:
> So, I wonder: would people be open to exposing additional parameters
> to merge drivers? For instance we could add parameters "%X", "%Y" "%Z"
> to expose those "revision:pathname" strings for each part. (I think
> colons cannot be part of revision names, so this can be parsed
> unambiguously by the custom merge driver to recover the revision and
> pathname independently, if needed.)
The last time this changed was in ef45bb1f (ll-merge: pass the
original path to external drivers, 2015-06-04).
I may not necessarily endorse the choice of XYZ [*], but I do not
fundamentally oppose to such a new feature existing. The mechanism
to define a custom merge driver is designed to be future-proof in
that only the parameters it uses is given as the value of
merge.*.driver variable, so it is not a problem that existing merge
drivers will not know what to do with "pathname in the common
ancestor", "pathname on our side", and "pathname on their side".
[Footnote]
* Whatever letters we choose, they must have mnemonic value that
signals two of them are the both sides of the merge that are
equal participants, and the other one is the old-file, their
common ancestor that plays quite a different from these two in
the merge. I cannot tell which one of the XYZ would be the more
special than other two, which is the primary reason why I do not
know if XYZ is a good idea.
^ 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