* Re: Git Push Always uses Protocol Version 0
From: Zsolt Imre @ 2024-01-22 19:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqa5oxurnj.fsf@gitster.g>
Hi. Thanks for your response. Yes, I saw those switch statements, but probably misunderstood the intent behind them.
Checking `determine_protocol_version_server` the in-code documentation says:
* Used by a server to determine which protocol version should be used based on
* a client's request, communicated via the 'GIT_PROTOCOL' environment variable
I explicitly set the `GIT_PROTOCOL` environment variable to `version=2`. (Will share more at the end of this email why, as you asked anyways)
The client-side calls `discover_version` to check the version supported by the server. Which ultimately ends up being done as:
enum protocol_version determine_protocol_version_client(const char *server_response)
{
enum protocol_version version = protocol_v0;
if (skip_prefix(server_response, "version ", &server_response)) {
version = parse_protocol_version(server_response);
if (version == protocol_unknown_version)
die("server is speaking an unknown protocol");
if (version == protocol_v0)
die("protocol error: server explicitly said version 0");
}
return version;
}
The server, according to my understanding of the documentations so far, will not return a version identifier if the client is not talking the version 2 (or maybe even version 1) of the protocol.
The git clone I mentioned in my previous email was clearly using protocol version 2 and it set the appropriate header to indicate this when discovering capabilities. See full request headers below.
POST /testing.git/git-upload-pack HTTP/1.1
Host: 127.0.0.1:9000
Accept: application/x-git-upload-pack-result
Accept-Encoding: deflate, gzip
Accept-Language: en-GB, *;q=0.9
Content-Length: 122
Content-Type: application/x-git-upload-pack-request
Git-Protocol: version=2
User-Agent: git/2.43.0
The server, in this case did not return a version identifier as the first PKT-LINE either, but responded otherwise according to the V2 protocol. Everything was working well.
Then, the above request was followed by by a proper fetch command in which the client again specified the use of V2 protocol:
POST /testing.git/git-upload-pack HTTP/1.1
Host: 127.0.0.1:9000
Accept: application/x-git-upload-pack-result
Accept-Encoding: deflate, gzip
Accept-Language: en-GB, *;q=0.9
Content-Length: 160
Content-Type: application/x-git-upload-pack-request
Git-Protocol: version=2
User-Agent: git/2.43.0
Accordingly, the cloning worked perfectly fine.
But then, when I want to push to the repo, the client does not do any capabilities or version discovery, just goes with V0 of the protocol to get the refs:
GET /testing.git/info/refs?service=git-receive-pack HTTP/1.1
Host: 127.0.0.1:9000
Accept: */*
Accept-Encoding: deflate, gzip
Accept-Language: en-GB, *;q=0.9
Cache-Control: no-cache
Pragma: no-cache
User-Agent: git/2.43.0
What I was expecting, given things are going stateless, that on push the client first discovers the protocol supported by the server and picks the most recent - in this case, that would be version 2.
And to answer your question of what I cannot do with the "current versions" of the protocol: I could do everything, of course. But, if there's protocol 0, 1 and 2 and I wanted to implement only version 2, I thought I should be able to. If protocol V2 was complete, I would not have to worry about implementing V0 and V1 (saving some time and headache), especially because I do not care about supporting old clients. I may have misunderstood the word "version" and version 2 is more of an "extension" to V1?
> On 22 Jan 2024, at 18:52, Junio C Hamano <gitster@pobox.com> wrote:
>
> Zsolt Imre <imrexzsolt@gmail.com> writes:
>
>> I'm not entirely sure if this is a bug or I am missing something,
>> but I thought I would share in the hope someone can help out. I'm
>> playing around with Git and trying to implement a git server that
>> communicates over HTTP and supports Git protocol version 2 *only*.
>>
>> When I `clone` a repository, the Git client (version 2.43.0),
>> after fetching the capabilities using protocol version 2, it
>> proceeds to fetch the refs, again, via protocol version 2 using
>> the `ls-refs` command. However, when I try to `push` my changes
>> to the repo, the Git client refuses to use protocol version 2 and
>> tries to obtain the ref list using protocol version 0, even if I
>> pass in the `-c protocol.version=2` command line argument.
>
> Given that v0 and v1 in the push direction behave exactly the same,
> and there has been no need to add features that were not supportable
> in v1 in the push direction, it is not surprising to see this code
>
> int cmd_send_pack(int argc, const char **argv, const char *prefix)
> {
> ...
> switch (discover_version(&reader)) {
> case protocol_v2:
> die("support for protocol v2 not implemented yet");
> break;
>
> in https://github.com/git/git/blob/master/builtin/send-pack.c#L282
> and also
>
> int cmd_receive_pack(int argc, const char **argv, const char *prefix)
> {
> ...
> switch (determine_protocol_version_server()) {
> case protocol_v2:
> /*
> * push support for protocol v2 has not been implemented yet,
> * so ignore the request to use v2 and fallback to using v0.
> */
> break;
>
> in https://github.com/git/git/blob/master/builtin/receive-pack.c#L2538
>
> that tells the receiving end to demote "v2" request down to "v0",
> and have the pushing end honor that choice.
>
> What specifically did you want to gain by using protocol version 2
> in the "push" direction that you cannot do with the current
> versions?
>
>
^ permalink raw reply
* Re: [PATCH v4] tests: move t0009-prio-queue.sh to the new unit testing framework
From: Junio C Hamano @ 2024-01-22 19:09 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget
Cc: git, Chandra Pratap, Jeff King, Phillip Wood, Chandra Pratap
In-Reply-To: <pull.1642.v4.git.1705865326185.gitgitgadget@gmail.com>
> diff --git a/t/unit-tests/t-prio-queue.c b/t/unit-tests/t-prio-queue.c
> new file mode 100644
> index 00000000000..d78b002f9ea
> --- /dev/null
> +++ b/t/unit-tests/t-prio-queue.c
> @@ -0,0 +1,98 @@
> +#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;
> +}
> +
> +
> +#define MISSING -1
> +#define DUMP -2
> +#define STACK -3
> +#define GET -4
> +#define REVERSE -5
> +
> +static int show(int *v)
> +{
> + return v ? *v : MISSING;
> +}
> +
> +static void test_prio_queue(int *input, int *result, size_t input_size)
> +{
> + struct prio_queue pq = { intcmp };
> +
> + for (int i = 0, j = 0; i < input_size; i++) {
> + void *peek, *get;
> + switch(input[i]) {
Style: in this codebase, a flow control keyword followed by a
parenthesized stuff always get a single SP before the parenthesis.
switch (input[i]) {
There are similar style violations in this patch with if().
> + case GET:
> + peek = prio_queue_peek(&pq);
> + get = prio_queue_get(&pq);
> + if (!check(peek == get))
> + return;
> + if(!check_int(result[j++], ==, show(get)))
> + test_msg("failed at result[] index %d", j-1);
> + break;
> + case DUMP:
> + while ((peek = prio_queue_peek(&pq))) {
> + get = prio_queue_get(&pq);
> + if (!check(peek == get))
> + return;
> + if(!check_int(result[j++], ==, show(get)))
> + test_msg("failed at result[] index %d", j-1);
> + }
> + break;
OK. So this one checks as we go. I am not sure how easy to grok a
breakage diagnosis with these giving the same message, without
giving any context of the failure (e.g. when we are fed
INPUT = 6 2 4 GET 5 3 GET GET 1 DUMP
EXPECT = 2 3 4 1 5 6
and for some reason if the first GET did not yield expected 2 but
gave us, say, 6, we only see "left: 2, right: 6" followed by "failed
at result[] index 0", and nothing else.
If it said something like "We pushed 6, 2, 4 and then did GET" to
give the reader a bit more context, it would make it easier to see
why we were complaining, i.e. expecting to see 2, instead getting 6.
But perhaps that is too much to ask to this code?
I dunno. Those who wanted to see an easier-to-diagnose output may
have better ideas.
Thanks.
> + case STACK:
> + pq.compare = NULL;
> + break;
> + case REVERSE:
> + prio_queue_reverse(&pq);
> + break;
> + default:
> + prio_queue_put(&pq, &input[i]);
> + break;
> + }
> + }
> + clear_prio_queue(&pq);
> +}
> +
> +#define BASIC_INPUT 2, 6, 3, 10, 9, 5, 7, 4, 5, 8, 1, DUMP
> +#define BASIC_RESULT 1, 2, 3, 4, 5, 5, 6, 7, 8, 9, 10
> +
> +#define MIXED_PUT_GET_INPUT 6, 2, 4, GET, 5, 3, GET, GET, 1, DUMP
> +#define MIXED_PUT_GET_RESULT 2, 3, 4, 1, 5, 6
> +
> +#define EMPTY_QUEUE_INPUT 1, 2, GET, GET, GET, 1, 2, GET, GET, GET
> +#define EMPTY_QUEUE_RESULT 1, 2, MISSING, 1, 2, MISSING
> +
> +#define STACK_INPUT STACK, 8, 1, 5, 4, 6, 2, 3, DUMP
> +#define STACK_RESULT 3, 2, 6, 4, 5, 1, 8
> +
> +#define REVERSE_STACK_INPUT STACK, 1, 2, 3, 4, 5, 6, REVERSE, DUMP
> +#define REVERSE_STACK_RESULT 1, 2, 3, 4, 5, 6
> +
> +#define TEST_INPUT(INPUT, RESULT, name) \
> + static void test_##name(void) \
> +{ \
> + int input[] = {INPUT}; \
> + int result[] = {RESULT}; \
> + test_prio_queue(input, result, ARRAY_SIZE(input)); \
> +}
> +
> +TEST_INPUT(BASIC_INPUT, BASIC_RESULT, basic)
> +TEST_INPUT(MIXED_PUT_GET_INPUT, MIXED_PUT_GET_RESULT, mixed)
> +TEST_INPUT(EMPTY_QUEUE_INPUT, EMPTY_QUEUE_RESULT, empty)
> +TEST_INPUT(STACK_INPUT, STACK_RESULT, stack)
> +TEST_INPUT(REVERSE_STACK_INPUT, REVERSE_STACK_RESULT, 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: Git Push Always uses Protocol Version 0
From: Junio C Hamano @ 2024-01-22 18:52 UTC (permalink / raw)
To: Zsolt Imre; +Cc: git
In-Reply-To: <FD545E92-18EF-44B5-A7D5-61ECADD880E6@gmail.com>
Zsolt Imre <imrexzsolt@gmail.com> writes:
> I'm not entirely sure if this is a bug or I am missing something,
> but I thought I would share in the hope someone can help out. I'm
> playing around with Git and trying to implement a git server that
> communicates over HTTP and supports Git protocol version 2 *only*.
>
> When I `clone` a repository, the Git client (version 2.43.0),
> after fetching the capabilities using protocol version 2, it
> proceeds to fetch the refs, again, via protocol version 2 using
> the `ls-refs` command. However, when I try to `push` my changes
> to the repo, the Git client refuses to use protocol version 2 and
> tries to obtain the ref list using protocol version 0, even if I
> pass in the `-c protocol.version=2` command line argument.
Given that v0 and v1 in the push direction behave exactly the same,
and there has been no need to add features that were not supportable
in v1 in the push direction, it is not surprising to see this code
int cmd_send_pack(int argc, const char **argv, const char *prefix)
{
...
switch (discover_version(&reader)) {
case protocol_v2:
die("support for protocol v2 not implemented yet");
break;
in https://github.com/git/git/blob/master/builtin/send-pack.c#L282
and also
int cmd_receive_pack(int argc, const char **argv, const char *prefix)
{
...
switch (determine_protocol_version_server()) {
case protocol_v2:
/*
* push support for protocol v2 has not been implemented yet,
* so ignore the request to use v2 and fallback to using v0.
*/
break;
in https://github.com/git/git/blob/master/builtin/receive-pack.c#L2538
that tells the receiving end to demote "v2" request down to "v0",
and have the pushing end honor that choice.
What specifically did you want to gain by using protocol version 2
in the "push" direction that you cannot do with the current
versions?
^ permalink raw reply
* Re: [PATCH 4/5] refs: introduce `refs_for_each_all_refs()`
From: Junio C Hamano @ 2024-01-22 17:45 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
In-Reply-To: <CAOLa=ZTbtqvejpvNVY5MHU=Adx3tWQ=FqVJdRLG1gaxYu4BG7A@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> On Fri, Jan 19, 2024 at 9:57 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> This looks more like add_pseudoref_entries() given that the general
>> direction is to have an "allow" list of pseudo refs (at this point
>> after the previous step of the series, is_pseudoref_syntax() is the
>> is_pseudoref() function, and uses ends_with("_HEAD") as a mere
>> optimization to avoid listing all the possible pseudo refs that
>> exists or will be added in the future whose name ends with "_HEAD").
>>
>> Other than the naming, I think these two steps make sense.
>
> I think overall the naming is correct, I would change the comments in
> `is_pseudoref_syntax()`.
>
> Because, apart from pseudorefs, we also want to print HEAD. This is also
> why the pattern matches "HEAD" instead of "_HEAD". I'll add some more
> comments to clarify this.
With the hardcoded "these are definitely pseudorefs" list in the
function, it no longer is is_pseudoref_SYNTAX() at all. I would
rather prefer to see is_pseudoref() that says no to HEAD and have
the callers check
- if (is_pseudoref_syntax(foo))
+ if (is_pseudoref(foo) || is_headref(foo))
than keeping the messy semantics we have. My second preference is
to call it is_pseudoref_or_head() that says yes to "HEAD" and
pseudorefs, even though I like it much less.
Similarly, between giving the function under discussion a more
descriptive name add_pseudoref_and_head_entries(), or adding a new
function add_head_entry() to make the callers call add_head_entry()
and add_pseudoref_entries() separately, I have a slight preference
for the latter.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/4] completion: complete --patch-with-raw
From: Junio C Hamano @ 2024-01-22 16:09 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget; +Cc: git, Philippe Blain
In-Reply-To: <1debe2fe18eec7f431f355241117afb0a5e2bf5f.1705810072.git.gitgitgadget@gmail.com>
"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> contrib/completion/git-completion.bash | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 6108d523a11..ccb17f4ad7b 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1807,7 +1807,7 @@ __git_diff_common_options="--stat --numstat --shortstat --summary
> --output= --output-indicator-context=
> --output-indicator-new= --output-indicator-old=
> --ws-error-highlight=
> - --pickaxe-all --pickaxe-regex
> + --pickaxe-all --pickaxe-regex --patch-with-raw
> "
Its ancient company, --patch-with-stat, is listed there, so it is
not all that wrong to include it as a suggestion, I guess.
But in the longer term, I think we would want to slim "git diff -h"
output by hiding them (no reason to touch code to remove the
support) from the "common diff options" part. They were added as a
kludge before I realized we need more than these two combinations
and made the options --patch, --stat, and --raw cumulative.
Thanks.
^ permalink raw reply
* Re: [PATCH v6 0/6] support remote archive via stateless transport
From: Junio C Hamano @ 2024-01-22 15:54 UTC (permalink / raw)
To: Linus Arver; +Cc: Jiang Xin, Git List, Jiang Xin
In-Reply-To: <owlyy1cifwur.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> This v6 version LGTM. Thanks!
Thanks, both of you. Will queue.
^ permalink raw reply
* Re: [PATCH 4/5] refs: introduce `refs_for_each_all_refs()`
From: Karthik Nayak @ 2024-01-22 15:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqsf2thwhj.fsf@gitster.g>
On Fri, Jan 19, 2024 at 9:57 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> This looks more like add_pseudoref_entries() given that the general
> direction is to have an "allow" list of pseudo refs (at this point
> after the previous step of the series, is_pseudoref_syntax() is the
> is_pseudoref() function, and uses ends_with("_HEAD") as a mere
> optimization to avoid listing all the possible pseudo refs that
> exists or will be added in the future whose name ends with "_HEAD").
>
> Other than the naming, I think these two steps make sense.
I think overall the naming is correct, I would change the comments in
`is_pseudoref_syntax()`.
Because, apart from pseudorefs, we also want to print HEAD. This is also
why the pattern matches "HEAD" instead of "_HEAD". I'll add some more
comments to clarify this.
So to summarize:
1. `is_pseudoref_syntax()` checks for pseudoref like syntax, this also matches
HEAD. Will add comments here to clarify that we do not actually check
ref contents.
2. `add_pseudoref_like_entries()` adds pseudorefs and HEAD to the loose refs
cache.
^ permalink raw reply
* Re: [PATCH v2 0/5] ci: add support for macOS to GitLab CI
From: Junio C Hamano @ 2024-01-22 15:44 UTC (permalink / raw)
To: Phillip Wood; +Cc: Patrick Steinhardt, git, Matthias Aßhauer
In-Reply-To: <6e190a32-ee45-451b-b841-25cc6eb2c5ab@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> Hi Patrick
> ...
> I've read though all the patches and they seem sensible to me though
> I'm hardly a macOS expert. I did wonder about the use of pushd/popd in
> the fourth patch as they are bashisms but that matches what we're
> doing on Ubuntu already. It's nice to see the GitLab CI running on
> macOS as well as Linux now.
Thanks, both.
^ permalink raw reply
* Re: [PATCH 1/5] refs: expose `is_pseudoref_syntax()`
From: Karthik Nayak @ 2024-01-22 15:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq34utjbz9.fsf@gitster.g>
On Fri, Jan 19, 2024 at 9:37 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > +/*
> > + * Check whether a refname matches the pseudoref syntax. This is a surface
> > + * level check and can present false positives.
> > + */
>
> What does "false positive" mean in this context?
>
> is_pseudoref_syntax("FOO_HEAD") says "true", and then if it is
> "false positive", that would mean "FOO_HEAD" is not a pseudo ref,
> right? What can a caller of this function do to deal with a false
> positive?
>
> Or do you mean "FOO_HEAD" is still a pseudo ref, but it may not
> currently exist? That is different from "false positive".
>
Yes, I think this is what I wanted to say and what you say below is what I'll
change it to.
> As the check is about "does it match the pseudoref syntax?", I would
> understand if what you wanted to say was something like: This only
> checks the syntax, and such a pseudoref may not currently exist in
> the repository---for that you'd need to call read_ref_full() or
> other ref API functions.
>
> Puzzled...
>
> Thanks.
Thanks!
^ permalink raw reply
* Re: [PATCH v2 08/12] t1415: move reffiles specific tests to t0601
From: Karthik Nayak @ 2024-01-22 14:12 UTC (permalink / raw)
To: John Cai via GitGitGadget, git; +Cc: Patrick Steinhardt, John Cai
In-Reply-To: <8327b12a313b00d1ca392f446e13f9c1018f1d84.1705695540.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 276 bytes --]
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: John Cai <johncai86@gmail.com>
>
> Move this test into t0601 with other reffiles pack-refs specific tests
> since it checks for individua loose refs and thus is specific to the
Nit: s/individua/individual
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v2 07/12] t1410: move reffiles specific tests to t0600
From: Karthik Nayak @ 2024-01-22 14:12 UTC (permalink / raw)
To: John Cai via GitGitGadget, git; +Cc: Patrick Steinhardt, John Cai
In-Reply-To: <d93c9c410b995b0c72958b1e9edc27c785857c55.1705695540.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 294 bytes --]
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: John Cai <johncai86@gmail.com>
>
> Move these tests to t0600 with other reffiles specific tests since they
> do things like take a lock on an individual ref, and write directly into
> the reflog refs
>
Nit: missing period.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH 1/7] sequencer: clean up pseudo refs with REF_NO_DEREF
From: Patrick Steinhardt @ 2024-01-22 12:28 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
In-Reply-To: <CAOLa=ZQakWpRgFZdLnALq9f+acAR3hBwgO_kxJ6gb_yv+vrX=g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1060 bytes --]
On Mon, Jan 22, 2024 at 03:49:37AM -0800, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > When cleaning up the state-tracking pseudorefs CHERRY_PICK_HEAD or
> > REVERT_HEAD we do not set REF_NO_DEREF. In the unlikely case where those
> > refs are a symref we would thus end up deleting the symref targets, and
> > not the symrefs themselves.
> >
> > Harden the code to use REF_NO_DEREF to fix this.
> >
>
> Just a question for my understanding. Is that that currently
> CHERRY_PICK_HEAD or REVERT_HEAD are never symrefs, so we don't really
> worry about this, but adding this change makes it safer?
They shouldn't ever be, at least Git would never end up writing symrefs
there. So for those refs to be symrefs the user would need to manually
create them as such, and in that case it would certainly be unexpected
if we deleted the symref targets when cleaning up state after a revert
or cherry-pick.
So yes, it's about hardening the code for edge cases that shouldn't
really happen in the first place.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 3/7] refs: convert AUTO_MERGE to become a normal pseudo-ref
From: Karthik Nayak @ 2024-01-22 12:02 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <ae2df6316f79e372c51d59666d685e59981d2f98.1705659748.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 387 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> In 70c70de616 (refs: complete list of special refs, 2023-12-14) we have
> inrtoduced a new `is_special_ref()` function that classifies some refs
> as being special. The rule is that special refs are exclusively read and
> written via the filesystem directly, whereas normal refs exclucsively go
> via the refs API.
>
s/exclucsively/exclusively
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH 1/7] sequencer: clean up pseudo refs with REF_NO_DEREF
From: Karthik Nayak @ 2024-01-22 11:49 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <48b95fe954c1dbdd080ce7a0cc871f4850bddeae.1705659748.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 532 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> When cleaning up the state-tracking pseudorefs CHERRY_PICK_HEAD or
> REVERT_HEAD we do not set REF_NO_DEREF. In the unlikely case where those
> refs are a symref we would thus end up deleting the symref targets, and
> not the symrefs themselves.
>
> Harden the code to use REF_NO_DEREF to fix this.
>
Just a question for my understanding. Is that that currently
CHERRY_PICK_HEAD or REVERT_HEAD are never symrefs, so we don't really
worry about this, but adding this change makes it safer?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v2 00/12] Group reffiles tests
From: Patrick Steinhardt @ 2024-01-22 11:36 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, John Cai
In-Reply-To: <pull.1647.v2.git.git.1705695540.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1583 bytes --]
On Fri, Jan 19, 2024 at 08:18:48PM +0000, John Cai via GitGitGadget wrote:
> This series groups REFFILES specific tests together. These tests are
> currently grouped together across the test suite based on functionality.
> However, since they exercise low-level behavior specific to the refs backend
> being used (in these cases, the ref-files backend), group them together
> based on which refs backend they test. This way, in the near future when the
> reftables backend gets upstreamed we can add tests that exercise the
> reftables backend close by in the t06xx area.
>
> These patches also remove the REFFILES prerequisite, since all the tests in
> t06xx are reffiles specific. In the near future, once the reftable backend
> is upstreamed, all the tests in t06xx will be forced to run with the
> reffiles backend.
>
> Changes since V1:
>
> * Moved some pack-refs tests to t0601 instead of t0600
> * Clarified some commit messages
> * Converted a test to be refs-backend agnostic
> * Other minor rearranging of tests
I've got two minor nits, but other than that this looks good to me. I've
also verified that all tests continue to pass with the current version
of the reftable backend.
There's a minor merge conflict with db4192c364 (t: mark tests regarding
git-pack-refs(1) to be backend specific, 2024-01-10). This conflict
comes from the fact that both patch series add the REFFILES prereq to
t3210, semantically the changes are the same. So it doesn't quite matter
which of both versions we retain as they both do the same.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 04/12] t1404: move reffiles specific tests to t0600
From: Patrick Steinhardt @ 2024-01-22 11:31 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, John Cai
In-Reply-To: <c3f0b81200cb9199de96737745345ad93061a8d0.1705695540.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1207 bytes --]
On Fri, Jan 19, 2024 at 08:18:52PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
>
> These tests modify loose refs manually and are specific to the reffiles
> backend. Move these to t0600 to be part of a test suite of reffiles
> specific tests.
>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
> t/t0600-reffiles-backend.sh | 263 +++++++++++++++++++++++++++++++++++
> t/t1404-update-ref-errors.sh | 237 -------------------------------
> 2 files changed, 263 insertions(+), 237 deletions(-)
> create mode 100755 t/t0600-reffiles-backend.sh
>
> diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
> new file mode 100755
> index 00000000000..2f910bd76ad
> --- /dev/null
> +++ b/t/t0600-reffiles-backend.sh
> @@ -0,0 +1,263 @@
> +#!/bin/sh
> +
> +test_description='Test reffiles backend'
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +TEST_PASSES_SANITIZE_LEAK=true
> +. ./test-lib.sh
> +
> +if ! test_have_prereq REFFILES
> + then
> + skip_all='skipping reffiles specific tests'
> + test_done
> +fi
Same issue here, indentation is off.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 01/12] t3210: move to t0601
From: Patrick Steinhardt @ 2024-01-22 11:31 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, John Cai
In-Reply-To: <ca65b9e6122d10a7b43d06a6069dae00e645a392.1705695540.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1490 bytes --]
On Fri, Jan 19, 2024 at 08:18:49PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
>
> Move t3210 to t0601, since these tests are reffiles specific in that
> they modify loose refs manually. This is part of the effort to
> categorize these tests together based on the ref backend they test. When
> we upstream the reftable backend, we can add more tests to t06xx. This
> way, all tests that test specific ref backend behavior will be grouped
> together.
>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
> t/{t3210-pack-refs.sh => t0601-reffiles-pack-refs.sh} | 6 ++++++
> 1 file changed, 6 insertions(+)
> rename t/{t3210-pack-refs.sh => t0601-reffiles-pack-refs.sh} (98%)
>
> diff --git a/t/t3210-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh
> similarity index 98%
> rename from t/t3210-pack-refs.sh
> rename to t/t0601-reffiles-pack-refs.sh
> index 7f4e98db7db..f7a3f693901 100755
> --- a/t/t3210-pack-refs.sh
> +++ b/t/t0601-reffiles-pack-refs.sh
> @@ -15,6 +15,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> TEST_PASSES_SANITIZE_LEAK=true
> . ./test-lib.sh
>
> +if ! test_have_prereq REFFILES
> + then
> + skip_all='skipping reffiles specific tests'
> + test_done
> +fi
Indentation here is off as we do not typically ident the `then`. So this
should rather look like the following:
if ! test_have_prereq REFFILES
then
skip_all='skipping reffiles specific tests'
test_done
fi
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 4/7] sequencer: introduce functions to handle autostashes via refs
From: Patrick Steinhardt @ 2024-01-22 10:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqbk9hjdai.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2168 bytes --]
On Fri, Jan 19, 2024 at 12:09:25PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > We're about to convert the MERGE_AUTOSTASH ref to become non-special,
> > using the refs API instead of direct filesystem access to both read and
> > write the ref. The current interfaces to write autostashes is entirely
> > path-based though, so we need to extend them to also support writes via
> > the refs API instead.
> >
> > Ideally, we would be able to fully replace the old set of path-based
> > interfaces. But the sequencer will continue to write state into
> > "rebase-merge/autostash". This path is not considered to be a ref at all
> > and will thus stay is-is for now, which requires us to keep both path-
>
> "is-is"???
Oops, "as-is" :)
> > and refs-based interfaces to handle autostashes.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > sequencer.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++----
> > sequencer.h | 3 +++
> > 2 files changed, 64 insertions(+), 5 deletions(-)
>
> The conversion (rather, the introduction to allow refs API to be
> used to access them) look correct, but offhand I do not know what
> the implication of leaving the file based interface would be.
We have said in past discussions that the sequencer state should remain
self contained, and that requires us to keep the files-based interface.
Refactoring it would likely be a larger undertaking, as we have also
said that refs must either have pseudo-ref syntax or start with "refs/".
The sequencer with its "rebase-merge/autostash" files doesn't conform to
either of those requirements, so we would also have to rename those
reflike files. I doubt that this is really worth it, so I would keep
those around as internal implementation details of how the sequencer
works. These refs are not supposed to be accessed by the user in the
first place, and we do not document their existence to the best of my
knowledge. Also, `git rev-parse rebase-merge/autostash` would fail.
So overall I think it's fine to leave this internal sequencer state as
self-contained as it is.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 3/7] refs: convert AUTO_MERGE to become a normal pseudo-ref
From: Patrick Steinhardt @ 2024-01-22 10:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Elijah Newren
In-Reply-To: <xmqqjzo5jf79.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2240 bytes --]
On Fri, Jan 19, 2024 at 11:28:10AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > In 70c70de616 (refs: complete list of special refs, 2023-12-14) we have
> > inrtoduced a new `is_special_ref()` function that classifies some refs
>
> "introduced"
>
> > @@ -4687,10 +4685,17 @@ void merge_switch_to_result(struct merge_options *opt,
> > trace2_region_leave("merge", "record_conflicted", opt->repo);
> >
> > trace2_region_enter("merge", "write_auto_merge", opt->repo);
> > - filename = git_path_auto_merge(opt->repo);
> > - fp = xfopen(filename, "w");
> > - fprintf(fp, "%s\n", oid_to_hex(&result->tree->object.oid));
> > - fclose(fp);
> > + if (refs_update_ref(get_main_ref_store(opt->repo), "", "AUTO_MERGE",
> > + &result->tree->object.oid, NULL, REF_NO_DEREF,
> > + UPDATE_REFS_MSG_ON_ERR)) {
> > + /* failure to function */
> > + opt->priv = NULL;
> > + result->clean = -1;
> > + merge_finalize(opt, result);
> > + trace2_region_leave("merge", "write_auto_merge",
> > + opt->repo);
> > + return;
> > + }
> > trace2_region_leave("merge", "write_auto_merge", opt->repo);
> > }
> > if (display_update_msgs)
>
> We used to ignore errors while updating AUTO_MERGE, implying that it
> is an optional nicety that does not have to block the merge. Now we
> explicitly mark the resulting index unclean. While my gut feeling
> says that it should not matter all that much (as such a failure
> would be rare enough that the user may want to inspect and double
> check the situation before going forward), I am not 100% sure if the
> change is behaviour is acceptable by everybody (cc'ed Elijah for
> second opinion).
We only ignored _some_ errors when updating AUTO_MERGE. Most notably we
die when we fail to create the file, but we succeed in case its contents
aren't written. This does not make much sense to me -- my expectation
would be that we should verify either the complete operation or nothing
of it and ignore all failures. Gracefully leaving an empty file behind
is a weird in-between state, so I'd claim it's more or less an oversight
that we did not perform proper error checking here.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 4/4] cherry-pick: Add `--empty` for more robust redundant commit handling
From: Phillip Wood @ 2024-01-22 10:40 UTC (permalink / raw)
To: Brian Lyles, Kristoffer Haugsbakk; +Cc: Taylor Blau, Elijah Newren, git
In-Reply-To: <CAHPHrSddqTEjtfhgVJ=vmRhbtuwXcGEiE1KFZqR1QhKw-HDtSg@mail.gmail.com>
Hi Brian
On 21/01/2024 18:28, Brian Lyles wrote:
> Is the sample pre-commit hook the ideal way to prevent this in the
> future? Or is there some config I could set globally to enforce this
> across repositories? I was having a little trouble finding a good way to
> accomplish this globally.
If you want to run the same hooks in all your repositories then you can
run 'git config --global core.hooksPath <my-hooks-path>' and git will
look for hooks in 'my-hooks-path' instead of '.git/hooks'. It makes it
tricky to run different linters in different projects though.
I'll try and take a proper look at these patches in the next couple of days.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH 1/7] sequencer: clean up pseudo refs with REF_NO_DEREF
From: Patrick Steinhardt @ 2024-01-22 10:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqttn9jfta.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]
On Fri, Jan 19, 2024 at 11:14:57AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > When cleaning up the state-tracking pseudorefs CHERRY_PICK_HEAD or
> > REVERT_HEAD we do not set REF_NO_DEREF. In the unlikely case where those
> > refs are a symref we would thus end up deleting the symref targets, and
> > not the symrefs themselves.
> >
> > Harden the code to use REF_NO_DEREF to fix this.
>
> This level of attention to detail is very much appreciated. I
> suspect that this was inspired by the other topic we discussed the
> other day to tighten accesses to {MERGE,CHERRY_PICK,REVERT}_HEAD
> that is done via repo_get_oid() to the one based on read_ref_full()?
At least not directly, no. I was adding new calls to `delete_ref()` and
was reflexively adding REF_NO_DEREF, which made me wonder why the other
cases in the vicinity didn't use it yet. So it's more inspired by the
first patch series that introduced the "special refs" classification,
where I already added ref deletions with `REF_NO_DEREF`.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] reftable/stack: fix race in up-to-date check
From: Patrick Steinhardt @ 2024-01-22 10:32 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20240120010559.GE117170@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2687 bytes --]
On Fri, Jan 19, 2024 at 08:05:59PM -0500, Jeff King wrote:
> On Thu, Jan 18, 2024 at 02:41:56PM +0100, Patrick Steinhardt wrote:
>
> > Refactor the code to stop using `stat_validity_check()`. Instead, we
> > manually stat(3P) the file descriptors to make relevant information
> > available. On Windows and MSYS2 the result will have both `st_dev` and
> > `st_ino` set to 0, which allows us to address the first issue by not
> > using the stat-based cache in that case. It also allows us to make sure
> > that we always compare `st_dev` and `st_ino`, addressing the second
> > issue.
>
> I didn't think too hard about the details, but does this mean that
> every user of stat_validity_check() has the same issue? The other big
> one is packed-refs (for which the code was originally written). Should
> this fix go into that API?
In theory, the issue is the same for the `packed-refs` file. But in
practice it's much less likely to be an issue:
- The file gets rewritten a lot less frequently than the "tables.list"
file, making the race less likely in the first place. It can only
happen when git-pack-refs(1) races with concurrent readers, whereas
it can happen for any two concurrent process with reftables.
- Due to entries in the `packed-refs` being of variable length it's
less likely that the size will be exactly the same after the file
has been rewritten. For reftables we have constant-length entries in
the "tables.list", so it's likely to happen there.
- It is very unlikely that we have the same issue with inode reuse
with the packed-refs file. The only reason why we have it with the
reftable backend is that it is very likely that we end up writing to
"tables.list" twice, once for the normal update and once for auto
compaction.
So overall, I doubt that it's all that critical in practice for the
packed-refs backend. It _is_ possible to happen, but chances are
significantly lower. I cannot recall a single report of this issue,
which underpins how unlikely it seems to be for the files backend.
Also, applying the same fix for the packed-refs would essentially mean
that the caching mechanism is now ineffective on Windows systems where
we do not have proper `st_dev` and `st_ino` values available. I think
this is a no-go in the context of packed-refs because we don't have a
second-level caching mechanism like we do in the reftable backend. It's
not great that we have to reread the "tables.list" file on Windows
systems for now, but at least it's better than having to reread the
complete "packed-refs" file like we'd have to do with the files backend.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Git Push Always uses Protocol Version 0
From: Zsolt Imre @ 2024-01-22 8:59 UTC (permalink / raw)
To: git
Hi there,
I'm not entirely sure if this is a bug or I am missing something, but I thought I would share in the hope someone can help out. I'm playing around with Git and trying to implement a git server that communicates over HTTP and supports Git protocol version 2 *only*.
When I `clone` a repository, the Git client (version 2.43.0), after fetching the capabilities using protocol version 2, it proceeds to fetch the refs, again, via protocol version 2 using the `ls-refs` command. However, when I try to `push` my changes to the repo, the Git client refuses to use protocol version 2 and tries to obtain the ref list using protocol version 0, even if I pass in the `-c protocol.version=2` command line argument.
Is there a way to make the client use only protocol version 2 instead of mixing the different protocols?
Thanks in advance for any help/guidance.
^ permalink raw reply
* [PATCH 2/2] patch-id: replace `atoi()` with `strtol_i2()`
From: Mohit Marathe via GitGitGadget @ 2024-01-22 8:51 UTC (permalink / raw)
To: git; +Cc: Mohit Marathe, Mohit Marathe
In-Reply-To: <pull.1646.git.1705913519.gitgitgadget@gmail.com>
From: Mohit Marathe <mohitmarathe23@gmail.com>
The change is made to improve the error-handling capabilities
during the conversion of string representations to integers.
The `strtol_i2(` function offers a more robust mechanism for
converting strings to integers by providing enhanced error
detection. Unlike `atoi(`, `strtol_i2(` allows the code to
differentiate between a valid conversion and an invalid one,
offering better resilience against potential issues such as
reading hunk header of a corrupted patch.
Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>
---
builtin/patch-id.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 3894d2b9706..6bfb263de5e 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -1,3 +1,4 @@
+#include "git-compat-util.h"
#include "builtin.h"
#include "config.h"
#include "diff.h"
@@ -29,33 +30,32 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
{
static const char digits[] = "0123456789";
const char *q, *r;
+ char *endp;
int n;
q = p + 4;
n = strspn(q, digits);
if (q[n] == ',') {
q += n + 1;
- *p_before = atoi(q);
+ if (strtol_i2(q, 10, p_before, &endp) != 0)
+ return 0;
n = strspn(q, digits);
} else {
*p_before = 1;
}
- if (n == 0 || q[n] != ' ' || q[n+1] != '+')
+ if (q[n] != ' ' || q[n+1] != '+')
return 0;
r = q + n + 2;
n = strspn(r, digits);
if (r[n] == ',') {
r += n + 1;
- *p_after = atoi(r);
- n = strspn(r, digits);
+ if (strtol_i2(r, 10, p_after, &endp) != 0)
+ return 0;
} else {
*p_after = 1;
}
- if (n == 0)
- return 0;
-
return 1;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/2] git-compat-util: add strtol_i2
From: Mohit Marathe via GitGitGadget @ 2024-01-22 8:51 UTC (permalink / raw)
To: git; +Cc: Mohit Marathe, Mohit Marathe
In-Reply-To: <pull.1646.git.1705913519.gitgitgadget@gmail.com>
From: Mohit Marathe <mohitmarathe23@gmail.com>
This function is an updated version of strtol_i function. It will
give more control to handle parsing of the characters after the
integer and better error handling while parsing numbers.
Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>
---
git-compat-util.h | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 7c2a6538e5a..0a014c837d8 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1309,6 +1309,29 @@ static inline int strtol_i(char const *s, int base, int *result)
return 0;
}
+#define strtol_i(s,b,r) strtol_i2((s), (b), (r), NULL)
+static inline int strtol_i2(char const *s, int base, int *result, char **endp)
+{
+ long ul;
+ char *dummy = NULL;
+
+ if (!endp)
+ endp = &dummy;
+ errno = 0;
+ ul = strtol(s, endp, base);
+ if (errno ||
+ /*
+ * if we are told to parse to the end of the string by
+ * passing NULL to endp, it is an error to have any
+ * remaining character after the digits.
+ */
+ (dummy && *dummy) ||
+ *endp == s || (int) ul != ul)
+ return -1;
+ *result = ul;
+ return 0;
+}
+
void git_stable_qsort(void *base, size_t nmemb, size_t size,
int(*compar)(const void *, const void *));
#ifdef INTERNAL_QSORT
--
gitgitgadget
^ permalink raw reply related
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