* Re: Git in GSoC 2024
From: Karthik Nayak @ 2024-02-21 8:59 UTC (permalink / raw)
To: Kaartic Sivaraam, Christian Couder, Patrick Steinhardt, git
Cc: Taylor Blau, Junio C Hamano, Victoria Dye
In-Reply-To: <CA+ARAtqicQkhKFcTxoT+GWMhCxnV-BNqd0oOcn2YwznfFnnRPw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 372 bytes --]
Hello,
Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> Hello Christian, Patrick, Karthik and all,
> Karthik, I was able to get your Gmail address and have sent an invite to
> you. Could you accept the same after reading through the program rules
> and member agreement?
>
I confirm the same, and have signed up.
Thanks for handling everything here.
- Karthik
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2024, #07; Tue, 20)
From: Karthik Nayak @ 2024-02-21 9:04 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <xmqqplwrqbd2.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 836 bytes --]
Hello,
Junio C Hamano <gitster@pobox.com> writes:
> * kn/for-all-refs (2024-02-12) 6 commits
> - for-each-ref: add new option to include root refs
> - ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR'
> - refs: introduce `refs_for_each_include_root_refs()`
> - refs: extract out `loose_fill_ref_dir_regular_file()`
> - refs: introduce `is_pseudoref()` and `is_headref()`
> - Merge branch 'ps/reftable-backend' into kn/for-all-refs
> (this branch uses ps/reftable-backend.)
>
> "git for-each-ref" filters its output with prefixes given from the
> command line, but it did not honor an empty string to mean "pass
> everything", which has been corrected.
>
> Will merge to 'next'?
> source: <20240211183923.131278-1-karthik.188@gmail.com>
Let me know if there's something more I could do here.
Thanks!
- Karthik
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* [PATCH] Add unix domain socket support to HTTP transport.
From: Leslie Cheng via GitGitGadget @ 2024-02-21 9:14 UTC (permalink / raw)
To: git; +Cc: Leslie Cheng, Leslie Cheng
From: Leslie Cheng <leslie.cheng5@gmail.com>
This changeset introduces an `http.unixSocket` option so that users can
proxy their git over HTTP remotes to a unix domain socket. In terms of
why, since UDS are local and git already has a local protocol: some
corporate environments use a UDS to proxy requests to internal resources
(ie. source control), so this change would support those use-cases. This
proxy can occasionally be necessary to attach MFA tokens or client
certificates for CLI tools.
The implementation leverages `--unix-socket` option [0] via the
`CURLOPT_UNIX_SOCKET_PATH` flag available with libcurl [1].
`GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH` and `NO_UNIX_SOCKETS` were kept
separate so that we can spit out better error messages for users if git
was compiled with `NO_UNIX_SOCKETS`.
[0] https://curl.se/docs/manpage.html#--unix-socket
[1] https://curl.se/libcurl/c/CURLOPT_UNIX_SOCKET_PATH.html
Signed-off-by: Leslie Cheng <leslie@lc.fyi>
---
Add unix domain socket support to HTTP transport.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1681%2Flcfyi%2Flcfyi%2Fadd-unix-socket-support-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1681/lcfyi/lcfyi/add-unix-socket-support-v1
Pull-Request: https://github.com/git/git/pull/1681
Documentation/config/http.txt | 5 ++
git-curl-compat.h | 7 +++
http.c | 23 +++++++++
t/t5565-http-unix-domain-socket.sh | 80 ++++++++++++++++++++++++++++++
4 files changed, 115 insertions(+)
create mode 100755 t/t5565-http-unix-domain-socket.sh
diff --git a/Documentation/config/http.txt b/Documentation/config/http.txt
index 2d4e0c9b869..bf48cbd599a 100644
--- a/Documentation/config/http.txt
+++ b/Documentation/config/http.txt
@@ -277,6 +277,11 @@ http.followRedirects::
the base for the follow-up requests, this is generally
sufficient. The default is `initial`.
+http.unixSocket::
+ Connect through this Unix domain socket via HTTP, instead of using the
+ network. If set, this config takes precendence over `http.proxy` and
+ is incompatible with the proxy options (see `curl(1)`).
+
http.<url>.*::
Any of the http.* options above can be applied selectively to some URLs.
For a config key to match a URL, each element of the config key is
diff --git a/git-curl-compat.h b/git-curl-compat.h
index fd96b3cdffd..f0f3bec0e17 100644
--- a/git-curl-compat.h
+++ b/git-curl-compat.h
@@ -74,6 +74,13 @@
#define GIT_CURL_HAVE_CURLE_SSL_PINNEDPUBKEYNOTMATCH 1
#endif
+/**
+ * CURLOPT_UNIX_SOCKET_PATH was added in 7.40.0, released in January 2015.
+ */
+#if LIBCURL_VERSION_NUM >= 0x074000
+#define GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH 1
+#endif
+
/**
* CURL_HTTP_VERSION_2 was added in 7.43.0, released in June 2015.
*
diff --git a/http.c b/http.c
index e73b136e589..8cfdcaeac82 100644
--- a/http.c
+++ b/http.c
@@ -79,6 +79,9 @@ static const char *http_proxy_ssl_ca_info;
static struct credential proxy_cert_auth = CREDENTIAL_INIT;
static int proxy_ssl_cert_password_required;
+#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS)
+static const char *curl_unix_socket_path;
+#endif
static struct {
const char *name;
long curlauth_param;
@@ -455,6 +458,20 @@ static int http_options(const char *var, const char *value,
return 0;
}
+ if (!strcmp("http.unixsocket", var)) {
+#ifdef GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH
+#ifndef NO_UNIX_SOCKETS
+ return git_config_string(&curl_unix_socket_path, var, value);
+#else
+ warning(_("Unix socket support unavailable in this build of Git"));
+ return 0;
+#endif
+#else
+ warning(_("Unix socket support is not supported with cURL < 7.40.0"));
+ return 0;
+#endif
+ }
+
if (!strcmp("http.cookiefile", var))
return git_config_pathname(&curl_cookie_file, var, value);
if (!strcmp("http.savecookies", var)) {
@@ -1203,6 +1220,12 @@ static CURL *get_curl_handle(void)
}
init_curl_proxy_auth(result);
+#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS)
+ if (curl_unix_socket_path) {
+ curl_easy_setopt(result, CURLOPT_UNIX_SOCKET_PATH, curl_unix_socket_path);
+ }
+#endif
+
set_curl_keepalive(result);
return result;
diff --git a/t/t5565-http-unix-domain-socket.sh b/t/t5565-http-unix-domain-socket.sh
new file mode 100755
index 00000000000..4ebcdfaa515
--- /dev/null
+++ b/t/t5565-http-unix-domain-socket.sh
@@ -0,0 +1,80 @@
+#!/bin/sh
+
+test_description="test fetching through http via unix domain socket"
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+
+test -z "$NO_UNIX_SOCKETS" || {
+ skip_all='skipping http-unix-socket tests, unix sockets not available'
+ test_done
+}
+
+UDS_TO_TCP_FIFO=uds_to_tcp
+TCP_TO_UDS_FIFO=tcp_to_uds
+UDS_PID=
+TCP_PID=
+UDS_SOCKET="$(pwd)/uds.sock"
+UNRESOLVABLE_ENDPOINT=http://localhost:4242
+
+start_proxy_unix_to_tcp() {
+ local socket_path="$UDS_SOCKET"
+ local host=127.0.0.1
+ local port=$LIB_HTTPD_PORT
+
+ rm -f "$UDS_TO_TCP_FIFO"
+ rm -f "$TCP_TO_UDS_FIFO"
+ rm -f "$socket_path"
+ mkfifo "$UDS_TO_TCP_FIFO"
+ mkfifo "$TCP_TO_UDS_FIFO"
+ nc -klU "$socket_path" <tcp_to_uds >uds_to_tcp &
+ UDS_PID=$!
+
+ nc "$host" "$port" >tcp_to_uds <uds_to_tcp &
+ TCP_PID=$!
+
+ test_atexit 'stop_proxy_unix_to_tcp'
+}
+
+stop_proxy_unix_to_tcp() {
+ kill "$UDS_PID"
+ kill "$TCP_PID"
+ rm -f "$UDS_TO_TCP_FIFO"
+ rm -f "$TCP_TO_UDS_FIFO"
+}
+
+start_httpd
+start_proxy_unix_to_tcp
+
+test_expect_success 'setup repository' '
+ test_commit foo &&
+ git init --bare "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ git push --mirror "$HTTPD_DOCUMENT_ROOT_PATH/repo.git"
+'
+
+# sanity check that we can't clone normally
+test_expect_success 'cloning without UDS fails' '
+ test_must_fail git clone "$UNRESOLVABLE_ENDPOINT/smart/repo.git" clone
+'
+
+test_expect_success 'cloning with UDS succeeds' '
+ test_when_finished "rm -rf clone" &&
+ test_config_global http.unixsocket "$UDS_SOCKET" &&
+ git clone "$UNRESOLVABLE_ENDPOINT/smart/repo.git" clone
+'
+
+test_expect_success 'cloning with a non-existent http proxy fails' '
+ git clone $HTTPD_URL/smart/repo.git clone &&
+ rm -rf clone &&
+ test_config_global http.proxy 127.0.0.1:0 &&
+ test_must_fail git clone $HTTPD_URL/smart/repo.git clone
+'
+
+test_expect_success 'UDS socket takes precedence over http proxy' '
+ test_when_finished "rm -rf clone" &&
+ test_config_global http.proxy 127.0.0.1:0 &&
+ test_config_global http.unixsocket "$UDS_SOCKET" &&
+ git clone $HTTPD_URL/smart/repo.git clone
+'
+
+test_done
base-commit: 3e0d3cd5c7def4808247caf168e17f2bbf47892b
--
gitgitgadget
^ permalink raw reply related
* Re: Segfault: git show-branch --reflog refs/pullreqs/1
From: Patrick Steinhardt @ 2024-02-21 9:52 UTC (permalink / raw)
To: Yasushi SHOJI; +Cc: Git Mailing List
In-Reply-To: <CAELBRWK-bZTV0qx6_34HAgpmYwy+5Zo2E0M+4B6yZJJ3CqweTw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 555 bytes --]
On Wed, Feb 21, 2024 at 10:48:25AM +0900, Yasushi SHOJI wrote:
> Hi all,
>
> Does anyone see a segfault on `git show-branch --reflog refs/pullreqs/1`?
TIL. I didn't even know that git-show-branch(1) is a thing.
By default, no refs but "HEAD" and refs starting with "refs/heads/*"
have a reflog. Thus, your ref "refs/pullreqs/1" won't have a reflog, and
git-show-branch(1) then happily segfaults when it didn't find a reflog
at all. This is of course a bug that needs fixing.
I'll send a patch in a bit, thanks for your report!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 0/2] Detect empty or missing reflogs with `ref@{0}`
From: Patrick Steinhardt @ 2024-02-21 9:56 UTC (permalink / raw)
To: git; +Cc: Yasushi SHOJI
In-Reply-To: <CAELBRWK-bZTV0qx6_34HAgpmYwy+5Zo2E0M+4B6yZJJ3CqweTw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]
Hi,
this patch series addresses some shortcomings when parsing `ref@{n}`
syntax via `read_ref_at()` when the reflog is missing or empty:
- First, as reported by Yasushi, git-show-branch(1) would segfault
because the function does not report when the 0th entry wasn't
found.
- Second, `ref@{0}` would fall back to return the object ID of ref
itself in case the reflog is empty or missing. This behaviour is
quite confusing and only works by chance.
The series addresses both of these issues by detecting and reporting the
case where the reflog is empty or missing.
Patrick
Patrick Steinhardt (2):
object-name: detect and report empty reflogs
builtin/show-branch: detect empty reflogs
builtin/show-branch.c | 2 ++
object-name.c | 10 ++++++----
refs.c | 3 ++-
t/t1506-rev-parse-diagnosis.sh | 8 ++++++++
t/t1508-at-combinations.sh | 29 +++++++++++++++++++++++++----
t/t3202-show-branch.sh | 25 +++++++++++++++++++++++++
6 files changed, 68 insertions(+), 9 deletions(-)
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 1/2] object-name: detect and report empty reflogs
From: Patrick Steinhardt @ 2024-02-21 9:56 UTC (permalink / raw)
To: git; +Cc: Yasushi SHOJI
In-Reply-To: <cover.1708509190.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7090 bytes --]
The `ref@{n}` syntax allows the user to request the n'th reflog entry
for a ref. This syntax is zero-indexed, meaning that entry zero refers
to the first entry in the reflog. The behaviour here is quite confusing
though and depends on the state of the corresponding reflog:
- If the reflog is not empty, we return the first reflog entry.
- If the reflog is empty, we return the object ID of the ref.
- If the reflog is missing, we return an error.
This is inconsistent and quite misleading.
This behaviour goes back to 6436a20284 (refs: allow @{n} to work with
n-sized reflog, 2021-01-07), which fixed a bug that wouldn't allow a
user to return the n'th reflog entry with an n-sized reflog. With this
commit, `read_ref_at()` started to special case reading the first entry
of the reflog via a separate `read_ref_at_ent_newest()` function. The
problem here is that we forgot to check whether the callback was invoked
at all, and thus we don't notice empty reflogs.
The commit in question added a test for `ref@{0}` when the reflog is
empty. But that test only works by chance: while `read_ref_at()` won't
initialize the object ID passed in by the pointer, all callers of this
function happen to call `repo_ref_dwim()` and thus pre-populate the
object ID. Thus, the consequence is that we indeed return the object ID
of the refname when the reflog is empty.
This behaviour is documented nowhere, and the fact that we return a
somewhat sensible result to the caller by sheer luck further stresses
the point that this behaviour is only accidental, even if there is a
test covering it.
Furthermore, this behaviour causes problems for the git-show-branch(1)
command. When executing `git show-branch --reflog` for a ref that either
has no or an empty reflog we run into a segfault. This is because the
`read_ref_at()` function doesn't report the error to us, and thus parts
of its out-parameters are not initialized.
Start to detect and report empty or missing reflogs in `read_ref_at()`
and report them to the caller. This results in a change in behaviour
when asking for `ref@{0}` with an empty or missing reflog because we now
die instead of returning the object ID of the ref itself. This adapted
behaviour should lead to less surprises as we now really only report
object IDs to the caller that actually come from the reflog, thus making
the user experience a whole lot more consistent.
This change also fixes the segfault in git-show-branch(1). Note that
this commit does not add a test yet -- this will be handled in the next
commit.
Reported-by: Yasushi SHOJI <yasushi.shoji@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
object-name.c | 10 ++++++----
refs.c | 3 ++-
t/t1506-rev-parse-diagnosis.sh | 8 ++++++++
t/t1508-at-combinations.sh | 29 +++++++++++++++++++++++++----
4 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/object-name.c b/object-name.c
index 3a2ef5d680..e2a6c9d2ec 100644
--- a/object-name.c
+++ b/object-name.c
@@ -994,8 +994,8 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
if (reflog_len) {
int nth, i;
timestamp_t at_time;
- timestamp_t co_time;
- int co_tz, co_cnt;
+ timestamp_t co_time = 0;
+ int co_tz = 0, co_cnt = 0;
/* Is it asking for N-th entry, or approxidate? */
for (i = nth = 0; 0 <= nth && i < reflog_len; i++) {
@@ -1020,6 +1020,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
return -1;
}
}
+
if (read_ref_at(get_main_ref_store(r),
real_ref, flags, at_time, nth, oid, NULL,
&co_time, &co_tz, &co_cnt)) {
@@ -1035,9 +1036,10 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
show_date(co_time, co_tz, DATE_MODE(RFC2822)));
}
} else {
- if (flags & GET_OID_QUIETLY) {
+ if (flags & GET_OID_QUIETLY)
exit(128);
- }
+ if (!co_cnt)
+ die(_("log for '%.*s' is empty"), len, str);
die(_("log for '%.*s' only has %d entries"),
len, str, co_cnt);
}
diff --git a/refs.c b/refs.c
index c633abf284..a2369e7835 100644
--- a/refs.c
+++ b/refs.c
@@ -1084,6 +1084,7 @@ static int read_ref_at_ent_newest(struct object_id *ooid UNUSED,
struct read_ref_at_cb *cb = cb_data;
set_read_ref_cutoffs(cb, timestamp, tz, message);
+ cb->found_it = 1;
oidcpy(cb->oid, noid);
/* We just want the first entry */
return 1;
@@ -1123,7 +1124,7 @@ int read_ref_at(struct ref_store *refs, const char *refname,
if (cb.cnt == 0) {
refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent_newest, &cb);
- return 0;
+ return !cb.found_it;
}
refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb);
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index ef40511d89..9d147c4ade 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -140,6 +140,14 @@ test_expect_success 'incorrect file in :path and :N:path' '
test_grep "path .disk-only.txt. exists on disk, but not in the index" error
'
+test_expect_success '@{0} reference with empty reflog' '
+ git checkout -B empty-reflog main &&
+ git reflog expire --expire=now refs/heads/empty-reflog &&
+ test_must_fail git rev-parse empty-reflog@{0} >output 2>error &&
+ test_must_be_empty output &&
+ test_grep "log for ${SQ}empty-reflog${SQ} is empty" error
+'
+
test_expect_success 'invalid @{n} reference' '
test_must_fail git rev-parse main@{99999} >output 2>error &&
test_must_be_empty output &&
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index e841309d0e..020106a1cc 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -110,10 +110,31 @@ test_expect_success '@{1} works with only one reflog entry' '
test_cmp_rev newbranch~ newbranch@{1}
'
-test_expect_success '@{0} works with empty reflog' '
- git checkout -B newbranch main &&
- git reflog expire --expire=now refs/heads/newbranch &&
- test_cmp_rev newbranch newbranch@{0}
+test_expect_success '@{0} fails with empty reflog' '
+ git checkout -B empty-reflog main &&
+ git reflog expire --expire=now refs/heads/empty-reflog &&
+ cat >expect <<-EOF &&
+ fatal: Needed a single revision
+ EOF
+ test_must_fail git rev-parse --verify missing-reflog@{0} 2>err &&
+ test_cmp expect err
+'
+
+test_expect_success '@{0} fails with missing reflog' '
+ git -c core.logAllRefUpdates=false checkout -B missing-reflog main &&
+ cat >expect <<-EOF &&
+ fatal: Needed a single revision
+ EOF
+ test_must_fail git rev-parse --verify missing-reflog@{0} 2>err &&
+ test_cmp expect err
+'
+
+test_expect_success '@{0} favors first reflog entry with diverged reflog' '
+ git checkout -B diverged-reflog main &&
+ test_commit A &&
+ test_commit B &&
+ git reflog delete diverged-reflog@{0} &&
+ test_cmp_rev diverged-reflog~ diverged-reflog@{0}
'
test_done
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/2] builtin/show-branch: detect empty reflogs
From: Patrick Steinhardt @ 2024-02-21 9:56 UTC (permalink / raw)
To: git; +Cc: Yasushi SHOJI
In-Reply-To: <cover.1708509190.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2525 bytes --]
The `--reflog=n` option for git-show-branch(1) allows the user to list
the reflog of a specific branch, where `n` specifies how many reflog
entries to show. When there are less than `n` entries we handle this
gracefully and simply report less entries.
There is a special case though when the ref either has no reflog or when
its reflog is empty. In this case, we end up printing nothing at all
while returning successfully. This is rather confusing as there is no
indicator why we didn't print anything.
Adapt the behaviour so that we die instead of leaving no user visible
traces. This change in behaviour should be fine given that this case
used to segfault before the preceding commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-branch.c | 2 ++
t/t3202-show-branch.sh | 25 +++++++++++++++++++++++++
2 files changed, 27 insertions(+)
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index b01ec761d2..8837415031 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -785,6 +785,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
if (read_ref_at(get_main_ref_store(the_repository),
ref, flags, 0, base + i, &oid, &logmsg,
×tamp, &tz, NULL)) {
+ if (!i)
+ die(_("log for %s is empty"), ref);
reflog = i;
break;
}
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index 6a98b2df76..e5b74cc55f 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -264,4 +264,29 @@ test_expect_success 'error descriptions on orphan branch' '
test_branch_op_in_wt -c new-branch
'
+test_expect_success 'show-branch --reflog with empty reflog' '
+ git checkout -B empty-reflog &&
+ git reflog expire --expire=now refs/heads/empty-reflog &&
+
+ cat >expect <<-EOF &&
+ fatal: log for refs/heads/empty-reflog is empty
+ EOF
+ test_must_fail git show-branch --reflog=1 empty-reflog 2>err &&
+ test_cmp expect err &&
+ test_must_fail git show-branch --reflog empty-reflog 2>err &&
+ test_cmp expect err
+'
+
+test_expect_success 'show-branch --reflog with missing reflog' '
+ git -c core.logAllRefUpdates=false checkout -B missing-reflog &&
+
+ cat >expect <<-EOF &&
+ fatal: log for refs/heads/missing-reflog is empty
+ EOF
+ test_must_fail git show-branch --reflog=1 missing-reflog 2>err &&
+ test_cmp expect err &&
+ test_must_fail git show-branch --reflog missing-reflog 2>err &&
+ test_cmp expect err
+'
+
test_done
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: Segfault: git show-branch --reflog refs/pullreqs/1
From: Patrick Steinhardt @ 2024-02-21 10:05 UTC (permalink / raw)
To: Jeff King; +Cc: Yasushi SHOJI, Denton Liu, Junio C Hamano, Git Mailing List
In-Reply-To: <20240221084250.GA25385@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]
On Wed, Feb 21, 2024 at 03:42:50AM -0500, Jeff King wrote:
> On Wed, Feb 21, 2024 at 10:48:25AM +0900, Yasushi SHOJI wrote:
>
> > Does anyone see a segfault on `git show-branch --reflog refs/pullreqs/1`?
> >
> > It seems files_reflog_path() creates a wrong path with the above command
> > using REF_WORKTREE_SHARED.
> I am still trying to wrap my head around how it can get such wrong
> results for show-branch when asking for "git rev-parse branch@{0}", etc,
> are correct. I think it is that "rev-parse branch@{0}" is only looking
> at the output oid and does not consider the reflog message at all. So I
> think it is subtly broken, but in a way that happens to work for that
> caller. But I'm not sure of the correct fix. At least not at this time
> of night.
>
> Cc-ing folks involved in 6436a20284.
Ah, our mails crossed, but we came to the same conclusion. Things indeed
are subtly broken here and work just by chance because all callers pre
initialize the object ID. So in the case where the reflog is missing or
empty we'd use that pre-initialized object ID because `read_ref_at()`
does not indicate the failure to the callers.
I think that this behaviour is not sensible in the first place. When
asking for the reflog, we should only ever return object IDs parsed from
the reflog. Falling back to parsing the ref itself does not make much
sense. I've thus sent a patch series that changes the behaviour here.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] object-name: detect and report empty reflogs
From: Kristoffer Haugsbakk @ 2024-02-21 10:37 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Yasushi SHOJI, git
In-Reply-To: <0fac6ebb098c7e8cdc87cb75f2dcffdc4b1ccfaa.1708509190.git.ps@pks.im>
On Wed, Feb 21, 2024, at 10:56, Patrick Steinhardt wrote:
> The `ref@{n}` syntax allows the user to request the n'th reflog entry
> for a ref. This syntax is zero-indexed, meaning that entry zero refers
> to the first entry in the reflog. The behaviour here is quite confusing
> though and depends on the state of the corresponding reflog:
Maybe this is obvious to other readers, but I sometimes get tripped up
when reading about such logs: what’s the “first entry”? The oldest one
or newest one? How about:
“ The `ref@{n}` syntax allows the user to request the n'th reflog entry
for a ref, starting from `ref@{0}` which points to the commit that
`ref` points to (zero-indexed). The behaviour here is quite confusing
though and depends on the state of the corresponding reflog:
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v2 0/7] reflog: introduce subcommand to list reflogs
From: Patrick Steinhardt @ 2024-02-21 11:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq34tnrqxv.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1840 bytes --]
On Tue, Feb 20, 2024 at 09:22:36AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
[snip]
> > 3: e4e4fac05c ! 3: 32b24a3d4b refs/files: sort reflogs returned by the reflog iterator
> > @@ refs/files-backend.c: static struct ref_iterator *reflog_iterator_begin(struct r
> > iter->dir_iterator = diter;
> > iter->ref_store = ref_store;
> > strbuf_release(&sb);
> > +@@ refs/files-backend.c: static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st
> > + return reflog_iterator_begin(ref_store, refs->gitcommondir);
> > + } else {
> > + return merge_ref_iterator_begin(
> > +- 0, reflog_iterator_begin(ref_store, refs->base.gitdir),
> > ++ 1, reflog_iterator_begin(ref_store, refs->base.gitdir),
> > + reflog_iterator_begin(ref_store, refs->gitcommondir),
> > + reflog_iterator_select, refs);
> > + }
>
> This hunk is new. Is there a downside to force merged iterators to
> always be sorted? The ones that are combined are all sorted so it
> is natural to force sorting like this code does? It might deserve
> explaining, and would certainly help future readers who runs "blame"
> on this code to figure out what made us think always sorting is a
> good direction forward.
Not really -- it merely gets passed down to the base ref iterator to
indicate that the entries are returned in lexicographic order. But I've
been jumping the gun here: the `reflog_iterator_select()` function does
not ensure lexicographic ordering between the two merged iterators right
now. I was assuming so because I implemented it in the reftable backend
like that. Should've double checked.
It's an easy fix though, which I'll add as another patch on top. Thanks
for making me think twice.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v3 0/8] reflog: introduce subcommand to list reflogs
From: Patrick Steinhardt @ 2024-02-21 12:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708353264.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7246 bytes --]
Hi,
this is the second version of my patch series that introduces a new `git
reflog list` subcommand to list available reflogs in a repository.
There is only a single change compared to v2. Junio made me double check
my assumption that the select function for reflogs yielded by the merged
iterator in the "files" backend would return things lexicographically.
Turns out it didn't -- instead, it returns all worktree refs first, then
all common refs.
This isn't only an issue because I prematurely marked the merged iter as
sorted. It's also an issue because the user-visible ordering would be
quite weird when executing `git reflog list` in a worktree.
I've thus added another commit on top that extracts the preexisting
logic to merge worktree and common refs by name from the "reftable"
backend and reuses it for the "files" reflogs.
Patrick
Patrick Steinhardt (8):
dir-iterator: pass name to `prepare_next_entry_data()` directly
dir-iterator: support iteration in sorted order
refs/files: sort reflogs returned by the reflog iterator
refs/files: sort merged worktree and common reflogs
refs: always treat iterators as ordered
refs: drop unused params from the reflog iterator callback
refs: stop resolving ref corresponding to reflogs
builtin/reflog: introduce subcommand to list reflogs
Documentation/git-reflog.txt | 3 +
builtin/fsck.c | 4 +-
builtin/reflog.c | 37 ++++++++++-
dir-iterator.c | 105 +++++++++++++++++++++++++++-----
dir-iterator.h | 3 +
refs.c | 27 ++++++---
refs.h | 11 +++-
refs/debug.c | 3 +-
refs/files-backend.c | 55 +++--------------
refs/iterator.c | 69 +++++++++++++++------
refs/packed-backend.c | 2 +-
refs/ref-cache.c | 2 +-
refs/refs-internal.h | 27 ++++-----
refs/reftable-backend.c | 67 +++-----------------
revision.c | 4 +-
t/helper/test-ref-store.c | 18 ++++--
t/t0600-reffiles-backend.sh | 24 ++++----
t/t1405-main-ref-store.sh | 8 +--
t/t1406-submodule-ref-store.sh | 8 +--
t/t1410-reflog.sh | 108 +++++++++++++++++++++++++++++++++
20 files changed, 380 insertions(+), 205 deletions(-)
Range-diff against v2:
1: 12de25dfe2 = 1: d474f9cf77 dir-iterator: pass name to `prepare_next_entry_data()` directly
2: 788afce189 = 2: 89cf960d47 dir-iterator: support iteration in sorted order
3: 32b24a3d4b ! 3: 8ad63eb3f6 refs/files: sort reflogs returned by the reflog iterator
@@ refs/files-backend.c: static struct ref_iterator *reflog_iterator_begin(struct r
iter->dir_iterator = diter;
iter->ref_store = ref_store;
strbuf_release(&sb);
-@@ refs/files-backend.c: static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st
- return reflog_iterator_begin(ref_store, refs->gitcommondir);
- } else {
- return merge_ref_iterator_begin(
-- 0, reflog_iterator_begin(ref_store, refs->base.gitdir),
-+ 1, reflog_iterator_begin(ref_store, refs->base.gitdir),
- reflog_iterator_begin(ref_store, refs->gitcommondir),
- reflog_iterator_select, refs);
- }
## t/t0600-reffiles-backend.sh ##
@@ t/t0600-reffiles-backend.sh: test_expect_success 'for_each_reflog()' '
-: ---------- > 4: 0b52f6c4af refs/files: sort merged worktree and common reflogs
4: 4254f23fd4 ! 5: d44564c8b3 refs: always treat iterators as ordered
@@ refs/files-backend.c: static struct ref_iterator *files_reflog_iterator_begin(st
- 1, reflog_iterator_begin(ref_store, refs->base.gitdir),
+ reflog_iterator_begin(ref_store, refs->base.gitdir),
reflog_iterator_begin(ref_store, refs->gitcommondir),
- reflog_iterator_select, refs);
+ ref_iterator_select, refs);
}
## refs/iterator.c ##
@@ refs/refs-internal.h: enum do_for_each_ref_flags {
const char *refname;
const struct object_id *oid;
unsigned int flags;
-@@ refs/refs-internal.h: typedef enum iterator_selection ref_iterator_select_fn(
+@@ refs/refs-internal.h: enum iterator_selection ref_iterator_select(struct ref_iterator *iter_worktree,
* Iterate over the entries from iter0 and iter1, with the values
* interleaved as directed by the select function. The iterator takes
* ownership of iter0 and iter1 and frees them when the iteration is
@@ refs/reftable-backend.c: static struct ref_iterator *reftable_be_iterator_begin(
worktree_iter = ref_iterator_for_stack(refs, refs->worktree_stack, prefix, flags);
- return merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base,
+ return merge_ref_iterator_begin(&worktree_iter->base, &main_iter->base,
- iterator_select, NULL);
+ ref_iterator_select, NULL);
}
@@ refs/reftable-backend.c: static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl
@@ refs/reftable-backend.c: static struct ref_iterator *reftable_be_reflog_iterator
- return merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base,
+ return merge_ref_iterator_begin(&worktree_iter->base, &main_iter->base,
- iterator_select, NULL);
+ ref_iterator_select, NULL);
}
5: 240334df6c = 6: c06fef8a64 refs: drop unused params from the reflog iterator callback
6: 7928661318 = 7: fc96d5bbab refs: stop resolving ref corresponding to reflogs
7: d7b9cff4c3 ! 8: f3f50f3742 builtin/reflog: introduce subcommand to list reflogs
@@ t/t1410-reflog.sh: test_expect_success 'empty reflog' '
+ )
+'
+
++test_expect_success 'list reflogs with worktree' '
++ test_when_finished "rm -rf repo" &&
++ git init repo &&
++ (
++ cd repo &&
++
++ test_commit A &&
++ git worktree add wt &&
++ git -c core.logAllRefUpdates=always \
++ update-ref refs/worktree/main HEAD &&
++ git -c core.logAllRefUpdates=always \
++ update-ref refs/worktree/per-worktree HEAD &&
++ git -c core.logAllRefUpdates=always -C wt \
++ update-ref refs/worktree/per-worktree HEAD &&
++ git -c core.logAllRefUpdates=always -C wt \
++ update-ref refs/worktree/worktree HEAD &&
++
++ cat >expect <<-EOF &&
++ HEAD
++ refs/heads/main
++ refs/heads/wt
++ refs/worktree/main
++ refs/worktree/per-worktree
++ EOF
++ git reflog list >actual &&
++ test_cmp expect actual &&
++
++ cat >expect <<-EOF &&
++ HEAD
++ refs/heads/main
++ refs/heads/wt
++ refs/worktree/per-worktree
++ refs/worktree/worktree
++ EOF
++ git -C wt reflog list >actual &&
++ test_cmp expect actual
++ )
++'
++
+test_expect_success 'reflog list returns error with additional args' '
+ cat >expect <<-EOF &&
+ error: list does not accept arguments: ${SQ}bogus${SQ}
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v3 1/8] dir-iterator: pass name to `prepare_next_entry_data()` directly
From: Patrick Steinhardt @ 2024-02-21 12:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708518982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1694 bytes --]
When adding the next directory entry for `struct dir_iterator` we pass
the complete `struct dirent *` to `prepare_next_entry_data()` even
though we only need the entry's name.
Refactor the code to pass in the name, only. This prepares for a
subsequent commit where we introduce the ability to iterate through
dir entries in an ordered manner.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
dir-iterator.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/dir-iterator.c b/dir-iterator.c
index 278b04243a..f58a97e089 100644
--- a/dir-iterator.c
+++ b/dir-iterator.c
@@ -94,15 +94,15 @@ static int pop_level(struct dir_iterator_int *iter)
/*
* Populate iter->base with the necessary information on the next iteration
- * entry, represented by the given dirent de. Return 0 on success and -1
+ * entry, represented by the given name. Return 0 on success and -1
* otherwise, setting errno accordingly.
*/
static int prepare_next_entry_data(struct dir_iterator_int *iter,
- struct dirent *de)
+ const char *name)
{
int err, saved_errno;
- strbuf_addstr(&iter->base.path, de->d_name);
+ strbuf_addstr(&iter->base.path, name);
/*
* We have to reset these because the path strbuf might have
* been realloc()ed at the previous strbuf_addstr().
@@ -159,7 +159,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
if (is_dot_or_dotdot(de->d_name))
continue;
- if (prepare_next_entry_data(iter, de)) {
+ if (prepare_next_entry_data(iter, de->d_name)) {
if (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)
goto error_out;
continue;
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 2/8] dir-iterator: support iteration in sorted order
From: Patrick Steinhardt @ 2024-02-21 12:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708518982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6195 bytes --]
The `struct dir_iterator` is a helper that allows us to iterate through
directory entries. This iterator returns entries in the exact same order
as readdir(3P) does -- or in other words, it guarantees no specific
order at all.
This is about to become problematic as we are introducing a new reflog
subcommand to list reflogs. As the "files" backend uses the directory
iterator to enumerate reflogs, returning reflog names and exposing them
to the user would inherit the indeterministic ordering. Naturally, it
would make for a terrible user interface to show a list with no
discernible order.
While this could be handled at a higher level by the new subcommand
itself by collecting and ordering the reflogs, this would be inefficient
because we would first have to collect all reflogs before we can sort
them, which would introduce additional latency when there are many
reflogs.
Instead, introduce a new option into the directory iterator that asks
for its entries to be yielded in lexicographical order. If set, the
iterator will read all directory entries greedily and sort them before
we start to iterate over them.
While this will of course also incur overhead as we cannot yield the
directory entries immediately, it should at least be more efficient than
having to sort the complete list of reflogs as we only need to sort one
directory at a time.
This functionality will be used in a follow-up commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
dir-iterator.c | 99 +++++++++++++++++++++++++++++++++++++++++++-------
dir-iterator.h | 3 ++
2 files changed, 89 insertions(+), 13 deletions(-)
diff --git a/dir-iterator.c b/dir-iterator.c
index f58a97e089..de619846f2 100644
--- a/dir-iterator.c
+++ b/dir-iterator.c
@@ -2,10 +2,19 @@
#include "dir.h"
#include "iterator.h"
#include "dir-iterator.h"
+#include "string-list.h"
struct dir_iterator_level {
DIR *dir;
+ /*
+ * The directory entries of the current level. This list will only be
+ * populated when the iterator is ordered. In that case, `dir` will be
+ * set to `NULL`.
+ */
+ struct string_list entries;
+ size_t entries_idx;
+
/*
* The length of the directory part of path at this level
* (including a trailing '/'):
@@ -43,6 +52,31 @@ struct dir_iterator_int {
unsigned int flags;
};
+static int next_directory_entry(DIR *dir, const char *path,
+ struct dirent **out)
+{
+ struct dirent *de;
+
+repeat:
+ errno = 0;
+ de = readdir(dir);
+ if (!de) {
+ if (errno) {
+ warning_errno("error reading directory '%s'",
+ path);
+ return -1;
+ }
+
+ return 1;
+ }
+
+ if (is_dot_or_dotdot(de->d_name))
+ goto repeat;
+
+ *out = de;
+ return 0;
+}
+
/*
* Push a level in the iter stack and initialize it with information from
* the directory pointed by iter->base->path. It is assumed that this
@@ -72,6 +106,35 @@ static int push_level(struct dir_iterator_int *iter)
return -1;
}
+ string_list_init_dup(&level->entries);
+ level->entries_idx = 0;
+
+ /*
+ * When the iterator is sorted we read and sort all directory entries
+ * directly.
+ */
+ if (iter->flags & DIR_ITERATOR_SORTED) {
+ struct dirent *de;
+
+ while (1) {
+ int ret = next_directory_entry(level->dir, iter->base.path.buf, &de);
+ if (ret < 0) {
+ if (errno != ENOENT &&
+ iter->flags & DIR_ITERATOR_PEDANTIC)
+ return -1;
+ continue;
+ } else if (ret > 0) {
+ break;
+ }
+
+ string_list_append(&level->entries, de->d_name);
+ }
+ string_list_sort(&level->entries);
+
+ closedir(level->dir);
+ level->dir = NULL;
+ }
+
return 0;
}
@@ -88,6 +151,7 @@ static int pop_level(struct dir_iterator_int *iter)
warning_errno("error closing directory '%s'",
iter->base.path.buf);
level->dir = NULL;
+ string_list_clear(&level->entries, 0);
return --iter->levels_nr;
}
@@ -139,27 +203,34 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
struct dirent *de;
struct dir_iterator_level *level =
&iter->levels[iter->levels_nr - 1];
+ const char *name;
strbuf_setlen(&iter->base.path, level->prefix_len);
- errno = 0;
- de = readdir(level->dir);
- if (!de) {
- if (errno) {
- warning_errno("error reading directory '%s'",
- iter->base.path.buf);
+ if (level->dir) {
+ int ret = next_directory_entry(level->dir, iter->base.path.buf, &de);
+ if (ret < 0) {
if (iter->flags & DIR_ITERATOR_PEDANTIC)
goto error_out;
- } else if (pop_level(iter) == 0) {
- return dir_iterator_abort(dir_iterator);
+ continue;
+ } else if (ret > 0) {
+ if (pop_level(iter) == 0)
+ return dir_iterator_abort(dir_iterator);
+ continue;
}
- continue;
- }
- if (is_dot_or_dotdot(de->d_name))
- continue;
+ name = de->d_name;
+ } else {
+ if (level->entries_idx >= level->entries.nr) {
+ if (pop_level(iter) == 0)
+ return dir_iterator_abort(dir_iterator);
+ continue;
+ }
- if (prepare_next_entry_data(iter, de->d_name)) {
+ name = level->entries.items[level->entries_idx++].string;
+ }
+
+ if (prepare_next_entry_data(iter, name)) {
if (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)
goto error_out;
continue;
@@ -188,6 +259,8 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)
warning_errno("error closing directory '%s'",
iter->base.path.buf);
}
+
+ string_list_clear(&level->entries, 0);
}
free(iter->levels);
diff --git a/dir-iterator.h b/dir-iterator.h
index 479e1ec784..6d438809b6 100644
--- a/dir-iterator.h
+++ b/dir-iterator.h
@@ -54,8 +54,11 @@
* and ITER_ERROR is returned immediately. In both cases, a meaningful
* warning is emitted. Note: ENOENT errors are always ignored so that
* the API users may remove files during iteration.
+ *
+ * - DIR_ITERATOR_SORTED: sort directory entries alphabetically.
*/
#define DIR_ITERATOR_PEDANTIC (1 << 0)
+#define DIR_ITERATOR_SORTED (1 << 1)
struct dir_iterator {
/* The current path: */
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 3/8] refs/files: sort reflogs returned by the reflog iterator
From: Patrick Steinhardt @ 2024-02-21 12:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708518982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3433 bytes --]
We use a directory iterator to return reflogs via the reflog iterator.
This iterator returns entries in the same order as readdir(3P) would and
will thus yield reflogs with no discernible order.
Set the new `DIR_ITERATOR_SORTED` flag that was introduced in the
preceding commit so that the order is deterministic. While the effect of
this can only been observed in a test tool, a subsequent commit will
start to expose this functionality to users via a new `git reflog list`
subcommand.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs/files-backend.c | 4 ++--
t/t0600-reffiles-backend.sh | 4 ++--
t/t1405-main-ref-store.sh | 2 +-
t/t1406-submodule-ref-store.sh | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 75dcc21ecb..2ffc63185f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2193,7 +2193,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
strbuf_addf(&sb, "%s/logs", gitdir);
- diter = dir_iterator_begin(sb.buf, 0);
+ diter = dir_iterator_begin(sb.buf, DIR_ITERATOR_SORTED);
if (!diter) {
strbuf_release(&sb);
return empty_ref_iterator_begin();
@@ -2202,7 +2202,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
CALLOC_ARRAY(iter, 1);
ref_iterator = &iter->base;
- base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);
+ base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 1);
iter->dir_iterator = diter;
iter->ref_store = ref_store;
strbuf_release(&sb);
diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index e6a5f1868f..4f860285cc 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -287,7 +287,7 @@ test_expect_success 'for_each_reflog()' '
mkdir -p .git/worktrees/wt/logs/refs/bisect &&
echo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random &&
- $RWT for-each-reflog | cut -d" " -f 2- | sort >actual &&
+ $RWT for-each-reflog | cut -d" " -f 2- >actual &&
cat >expected <<-\EOF &&
HEAD 0x1
PSEUDO-WT 0x0
@@ -297,7 +297,7 @@ test_expect_success 'for_each_reflog()' '
EOF
test_cmp expected actual &&
- $RMAIN for-each-reflog | cut -d" " -f 2- | sort >actual &&
+ $RMAIN for-each-reflog | cut -d" " -f 2- >actual &&
cat >expected <<-\EOF &&
HEAD 0x1
PSEUDO-MAIN 0x0
diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
index 976bd71efb..cfb583f544 100755
--- a/t/t1405-main-ref-store.sh
+++ b/t/t1405-main-ref-store.sh
@@ -74,7 +74,7 @@ test_expect_success 'verify_ref(new-main)' '
'
test_expect_success 'for_each_reflog()' '
- $RUN for-each-reflog | sort -k2 | cut -d" " -f 2- >actual &&
+ $RUN for-each-reflog | cut -d" " -f 2- >actual &&
cat >expected <<-\EOF &&
HEAD 0x1
refs/heads/main 0x0
diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
index e6a7f7334b..40332e23cc 100755
--- a/t/t1406-submodule-ref-store.sh
+++ b/t/t1406-submodule-ref-store.sh
@@ -63,7 +63,7 @@ test_expect_success 'verify_ref(new-main)' '
'
test_expect_success 'for_each_reflog()' '
- $RUN for-each-reflog | sort | cut -d" " -f 2- >actual &&
+ $RUN for-each-reflog | cut -d" " -f 2- >actual &&
cat >expected <<-\EOF &&
HEAD 0x1
refs/heads/main 0x0
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 4/8] refs/files: sort merged worktree and common reflogs
From: Patrick Steinhardt @ 2024-02-21 12:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708518982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 8134 bytes --]
When iterating through reflogs in a worktree we create a merged iterator
that merges reflogs from both refdbs. The resulting refs are ordered so
that instead we first return all worktree reflogs before we return all
common refs.
This is the only remaining case where a ref iterator returns entries in
a non-lexicographic order. The result would look something like the
following (listed with a command we introduce in a subsequent commit):
```
$ git reflog list
HEAD
refs/worktree/per-worktree
refs/heads/main
refs/heads/wt
```
So we first print the per-worktree reflogs in lexicographic order, then
the common reflogs in lexicographic order. This is confusing and not
consistent with how we print per-worktree refs, which are exclusively
sorted lexicographically.
Sort reflogs lexicographically in the same way as we sort normal refs.
As this is already implemented properly by the "reftable" backend via a
separate selection function, we simply pull out that logic and reuse it
for the "files" backend. As logs are properly sorted now, mark the
merged reflog iterator as sorted.
Tests will be added in a subsequent commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs/files-backend.c | 30 ++------------------------
refs/iterator.c | 43 +++++++++++++++++++++++++++++++++++++
refs/refs-internal.h | 9 ++++++++
refs/reftable-backend.c | 47 ++---------------------------------------
4 files changed, 56 insertions(+), 73 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2ffc63185f..551cafdf76 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2210,32 +2210,6 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
return ref_iterator;
}
-static enum iterator_selection reflog_iterator_select(
- struct ref_iterator *iter_worktree,
- struct ref_iterator *iter_common,
- void *cb_data UNUSED)
-{
- if (iter_worktree) {
- /*
- * We're a bit loose here. We probably should ignore
- * common refs if they are accidentally added as
- * per-worktree refs.
- */
- return ITER_SELECT_0;
- } else if (iter_common) {
- if (parse_worktree_ref(iter_common->refname, NULL, NULL,
- NULL) == REF_WORKTREE_SHARED)
- return ITER_SELECT_1;
-
- /*
- * The main ref store may contain main worktree's
- * per-worktree refs, which should be ignored
- */
- return ITER_SKIP_1;
- } else
- return ITER_DONE;
-}
-
static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)
{
struct files_ref_store *refs =
@@ -2246,9 +2220,9 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st
return reflog_iterator_begin(ref_store, refs->gitcommondir);
} else {
return merge_ref_iterator_begin(
- 0, reflog_iterator_begin(ref_store, refs->base.gitdir),
+ 1, reflog_iterator_begin(ref_store, refs->base.gitdir),
reflog_iterator_begin(ref_store, refs->gitcommondir),
- reflog_iterator_select, refs);
+ ref_iterator_select, refs);
}
}
diff --git a/refs/iterator.c b/refs/iterator.c
index 6b680f610e..b7ab5dc92f 100644
--- a/refs/iterator.c
+++ b/refs/iterator.c
@@ -98,6 +98,49 @@ struct merge_ref_iterator {
struct ref_iterator **current;
};
+enum iterator_selection ref_iterator_select(struct ref_iterator *iter_worktree,
+ struct ref_iterator *iter_common,
+ void *cb_data UNUSED)
+{
+ if (iter_worktree && !iter_common) {
+ /*
+ * Return the worktree ref if there are no more common refs.
+ */
+ return ITER_SELECT_0;
+ } else if (iter_common) {
+ /*
+ * In case we have pending worktree and common refs we need to
+ * yield them based on their lexicographical order. Worktree
+ * refs that have the same name as common refs shadow the
+ * latter.
+ */
+ if (iter_worktree) {
+ int cmp = strcmp(iter_worktree->refname,
+ iter_common->refname);
+ if (cmp < 0)
+ return ITER_SELECT_0;
+ else if (!cmp)
+ return ITER_SELECT_0_SKIP_1;
+ }
+
+ /*
+ * We now know that the lexicographically-next ref is a common
+ * ref. When the common ref is a shared one we return it.
+ */
+ if (parse_worktree_ref(iter_common->refname, NULL, NULL,
+ NULL) == REF_WORKTREE_SHARED)
+ return ITER_SELECT_1;
+
+ /*
+ * Otherwise, if the common ref is a per-worktree ref we skip
+ * it because it would belong to the main worktree, not ours.
+ */
+ return ITER_SKIP_1;
+ } else {
+ return ITER_DONE;
+ }
+}
+
static int merge_ref_iterator_advance(struct ref_iterator *ref_iterator)
{
struct merge_ref_iterator *iter =
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 83e0f0bba3..51f612e122 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -386,6 +386,15 @@ typedef enum iterator_selection ref_iterator_select_fn(
struct ref_iterator *iter0, struct ref_iterator *iter1,
void *cb_data);
+/*
+ * An implementation of ref_iterator_select_fn that merges worktree and common
+ * refs. Per-worktree refs from the common iterator are ignored, worktree refs
+ * override common refs. Refs are selected lexicographically.
+ */
+enum iterator_selection ref_iterator_select(struct ref_iterator *iter_worktree,
+ struct ref_iterator *iter_common,
+ void *cb_data);
+
/*
* Iterate over the entries from iter0 and iter1, with the values
* interleaved as directed by the select function. The iterator takes
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index a14f2ad7f4..68d32a9101 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -504,49 +504,6 @@ static struct reftable_ref_iterator *ref_iterator_for_stack(struct reftable_ref_
return iter;
}
-static enum iterator_selection iterator_select(struct ref_iterator *iter_worktree,
- struct ref_iterator *iter_common,
- void *cb_data UNUSED)
-{
- if (iter_worktree && !iter_common) {
- /*
- * Return the worktree ref if there are no more common refs.
- */
- return ITER_SELECT_0;
- } else if (iter_common) {
- /*
- * In case we have pending worktree and common refs we need to
- * yield them based on their lexicographical order. Worktree
- * refs that have the same name as common refs shadow the
- * latter.
- */
- if (iter_worktree) {
- int cmp = strcmp(iter_worktree->refname,
- iter_common->refname);
- if (cmp < 0)
- return ITER_SELECT_0;
- else if (!cmp)
- return ITER_SELECT_0_SKIP_1;
- }
-
- /*
- * We now know that the lexicographically-next ref is a common
- * ref. When the common ref is a shared one we return it.
- */
- if (parse_worktree_ref(iter_common->refname, NULL, NULL,
- NULL) == REF_WORKTREE_SHARED)
- return ITER_SELECT_1;
-
- /*
- * Otherwise, if the common ref is a per-worktree ref we skip
- * it because it would belong to the main worktree, not ours.
- */
- return ITER_SKIP_1;
- } else {
- return ITER_DONE;
- }
-}
-
static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_store,
const char *prefix,
const char **exclude_patterns,
@@ -576,7 +533,7 @@ static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_sto
*/
worktree_iter = ref_iterator_for_stack(refs, refs->worktree_stack, prefix, flags);
return merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base,
- iterator_select, NULL);
+ ref_iterator_select, NULL);
}
static int reftable_be_read_raw_ref(struct ref_store *ref_store,
@@ -1759,7 +1716,7 @@ static struct ref_iterator *reftable_be_reflog_iterator_begin(struct ref_store *
worktree_iter = reflog_iterator_for_stack(refs, refs->worktree_stack);
return merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base,
- iterator_select, NULL);
+ ref_iterator_select, NULL);
}
static int yield_log_record(struct reftable_log_record *log,
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 5/8] refs: always treat iterators as ordered
From: Patrick Steinhardt @ 2024-02-21 12:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708518982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 11366 bytes --]
In the preceding commit we have converted the reflog iterator of the
"files" backend to be ordered, which was the only remaining ref iterator
that wasn't ordered. Refactor the ref iterator infrastructure so that we
always assume iterators to be ordered, thus simplifying the code.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 4 ----
refs/debug.c | 3 +--
refs/files-backend.c | 7 +++----
refs/iterator.c | 26 ++++++++------------------
refs/packed-backend.c | 2 +-
refs/ref-cache.c | 2 +-
refs/refs-internal.h | 18 ++----------------
refs/reftable-backend.c | 8 ++++----
8 files changed, 20 insertions(+), 50 deletions(-)
diff --git a/refs.c b/refs.c
index fff343c256..dc25606a82 100644
--- a/refs.c
+++ b/refs.c
@@ -1594,10 +1594,6 @@ struct ref_iterator *refs_ref_iterator_begin(
if (trim)
iter = prefix_ref_iterator_begin(iter, "", trim);
- /* Sanity check for subclasses: */
- if (!iter->ordered)
- BUG("reference iterator is not ordered");
-
return iter;
}
diff --git a/refs/debug.c b/refs/debug.c
index 634681ca44..c7531b17f0 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -181,7 +181,6 @@ static int debug_ref_iterator_advance(struct ref_iterator *ref_iterator)
trace_printf_key(&trace_refs, "iterator_advance: %s (0)\n",
diter->iter->refname);
- diter->base.ordered = diter->iter->ordered;
diter->base.refname = diter->iter->refname;
diter->base.oid = diter->iter->oid;
diter->base.flags = diter->iter->flags;
@@ -222,7 +221,7 @@ debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,
drefs->refs->be->iterator_begin(drefs->refs, prefix,
exclude_patterns, flags);
struct debug_ref_iterator *diter = xcalloc(1, sizeof(*diter));
- base_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable, 1);
+ base_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable);
diter->iter = res;
trace_printf_key(&trace_refs, "ref_iterator_begin: \"%s\" (0x%x)\n",
prefix, flags);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 551cafdf76..05bb0c875c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -879,8 +879,7 @@ static struct ref_iterator *files_ref_iterator_begin(
CALLOC_ARRAY(iter, 1);
ref_iterator = &iter->base;
- base_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable,
- overlay_iter->ordered);
+ base_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable);
iter->iter0 = overlay_iter;
iter->repo = ref_store->repo;
iter->flags = flags;
@@ -2202,7 +2201,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
CALLOC_ARRAY(iter, 1);
ref_iterator = &iter->base;
- base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 1);
+ base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);
iter->dir_iterator = diter;
iter->ref_store = ref_store;
strbuf_release(&sb);
@@ -2220,7 +2219,7 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st
return reflog_iterator_begin(ref_store, refs->gitcommondir);
} else {
return merge_ref_iterator_begin(
- 1, reflog_iterator_begin(ref_store, refs->base.gitdir),
+ reflog_iterator_begin(ref_store, refs->base.gitdir),
reflog_iterator_begin(ref_store, refs->gitcommondir),
ref_iterator_select, refs);
}
diff --git a/refs/iterator.c b/refs/iterator.c
index b7ab5dc92f..9db8b056d5 100644
--- a/refs/iterator.c
+++ b/refs/iterator.c
@@ -25,11 +25,9 @@ int ref_iterator_abort(struct ref_iterator *ref_iterator)
}
void base_ref_iterator_init(struct ref_iterator *iter,
- struct ref_iterator_vtable *vtable,
- int ordered)
+ struct ref_iterator_vtable *vtable)
{
iter->vtable = vtable;
- iter->ordered = !!ordered;
iter->refname = NULL;
iter->oid = NULL;
iter->flags = 0;
@@ -74,7 +72,7 @@ struct ref_iterator *empty_ref_iterator_begin(void)
struct empty_ref_iterator *iter = xcalloc(1, sizeof(*iter));
struct ref_iterator *ref_iterator = &iter->base;
- base_ref_iterator_init(ref_iterator, &empty_ref_iterator_vtable, 1);
+ base_ref_iterator_init(ref_iterator, &empty_ref_iterator_vtable);
return ref_iterator;
}
@@ -250,7 +248,6 @@ static struct ref_iterator_vtable merge_ref_iterator_vtable = {
};
struct ref_iterator *merge_ref_iterator_begin(
- int ordered,
struct ref_iterator *iter0, struct ref_iterator *iter1,
ref_iterator_select_fn *select, void *cb_data)
{
@@ -265,7 +262,7 @@ struct ref_iterator *merge_ref_iterator_begin(
* references through only if they exist in both iterators.
*/
- base_ref_iterator_init(ref_iterator, &merge_ref_iterator_vtable, ordered);
+ base_ref_iterator_init(ref_iterator, &merge_ref_iterator_vtable);
iter->iter0 = iter0;
iter->iter1 = iter1;
iter->select = select;
@@ -314,12 +311,9 @@ struct ref_iterator *overlay_ref_iterator_begin(
} else if (is_empty_ref_iterator(back)) {
ref_iterator_abort(back);
return front;
- } else if (!front->ordered || !back->ordered) {
- BUG("overlay_ref_iterator requires ordered inputs");
}
- return merge_ref_iterator_begin(1, front, back,
- overlay_iterator_select, NULL);
+ return merge_ref_iterator_begin(front, back, overlay_iterator_select, NULL);
}
struct prefix_ref_iterator {
@@ -358,16 +352,12 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator)
if (cmp > 0) {
/*
- * If the source iterator is ordered, then we
+ * As the source iterator is ordered, we
* can stop the iteration as soon as we see a
* refname that comes after the prefix:
*/
- if (iter->iter0->ordered) {
- ok = ref_iterator_abort(iter->iter0);
- break;
- } else {
- continue;
- }
+ ok = ref_iterator_abort(iter->iter0);
+ break;
}
if (iter->trim) {
@@ -439,7 +429,7 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,
CALLOC_ARRAY(iter, 1);
ref_iterator = &iter->base;
- base_ref_iterator_init(ref_iterator, &prefix_ref_iterator_vtable, iter0->ordered);
+ base_ref_iterator_init(ref_iterator, &prefix_ref_iterator_vtable);
iter->iter0 = iter0;
iter->prefix = xstrdup(prefix);
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index a499a91c7e..4e826c05ff 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1111,7 +1111,7 @@ static struct ref_iterator *packed_ref_iterator_begin(
CALLOC_ARRAY(iter, 1);
ref_iterator = &iter->base;
- base_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable, 1);
+ base_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable);
if (exclude_patterns)
populate_excluded_jump_list(iter, snapshot, exclude_patterns);
diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index a372a00941..9f9797209a 100644
--- a/refs/ref-cache.c
+++ b/refs/ref-cache.c
@@ -486,7 +486,7 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,
CALLOC_ARRAY(iter, 1);
ref_iterator = &iter->base;
- base_ref_iterator_init(ref_iterator, &cache_ref_iterator_vtable, 1);
+ base_ref_iterator_init(ref_iterator, &cache_ref_iterator_vtable);
ALLOC_GROW(iter->levels, 10, iter->levels_alloc);
iter->levels_nr = 1;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 51f612e122..a9b6e887f8 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -312,13 +312,6 @@ enum do_for_each_ref_flags {
*/
struct ref_iterator {
struct ref_iterator_vtable *vtable;
-
- /*
- * Does this `ref_iterator` iterate over references in order
- * by refname?
- */
- unsigned int ordered : 1;
-
const char *refname;
const struct object_id *oid;
unsigned int flags;
@@ -399,11 +392,9 @@ enum iterator_selection ref_iterator_select(struct ref_iterator *iter_worktree,
* Iterate over the entries from iter0 and iter1, with the values
* interleaved as directed by the select function. The iterator takes
* ownership of iter0 and iter1 and frees them when the iteration is
- * over. A derived class should set `ordered` to 1 or 0 based on
- * whether it generates its output in order by reference name.
+ * over.
*/
struct ref_iterator *merge_ref_iterator_begin(
- int ordered,
struct ref_iterator *iter0, struct ref_iterator *iter1,
ref_iterator_select_fn *select, void *cb_data);
@@ -432,8 +423,6 @@ struct ref_iterator *overlay_ref_iterator_begin(
* As an convenience to callers, if prefix is the empty string and
* trim is zero, this function returns iter0 directly, without
* wrapping it.
- *
- * The resulting ref_iterator is ordered if iter0 is.
*/
struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,
const char *prefix,
@@ -444,14 +433,11 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,
/*
* Base class constructor for ref_iterators. Initialize the
* ref_iterator part of iter, setting its vtable pointer as specified.
- * `ordered` should be set to 1 if the iterator will iterate over
- * references in order by refname; otherwise it should be set to 0.
* This is meant to be called only by the initializers of derived
* classes.
*/
void base_ref_iterator_init(struct ref_iterator *iter,
- struct ref_iterator_vtable *vtable,
- int ordered);
+ struct ref_iterator_vtable *vtable);
/*
* Base class destructor for ref_iterators. Destroy the ref_iterator
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 68d32a9101..39e9a9d4e2 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -479,7 +479,7 @@ static struct reftable_ref_iterator *ref_iterator_for_stack(struct reftable_ref_
int ret;
iter = xcalloc(1, sizeof(*iter));
- base_ref_iterator_init(&iter->base, &reftable_ref_iterator_vtable, 1);
+ base_ref_iterator_init(&iter->base, &reftable_ref_iterator_vtable);
iter->prefix = prefix;
iter->base.oid = &iter->oid;
iter->flags = flags;
@@ -532,7 +532,7 @@ static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_sto
* single iterator.
*/
worktree_iter = ref_iterator_for_stack(refs, refs->worktree_stack, prefix, flags);
- return merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base,
+ return merge_ref_iterator_begin(&worktree_iter->base, &main_iter->base,
ref_iterator_select, NULL);
}
@@ -1680,7 +1680,7 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl
int ret;
iter = xcalloc(1, sizeof(*iter));
- base_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable, 1);
+ base_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable);
iter->refs = refs;
iter->base.oid = &iter->oid;
@@ -1715,7 +1715,7 @@ static struct ref_iterator *reftable_be_reflog_iterator_begin(struct ref_store *
worktree_iter = reflog_iterator_for_stack(refs, refs->worktree_stack);
- return merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base,
+ return merge_ref_iterator_begin(&worktree_iter->base, &main_iter->base,
ref_iterator_select, NULL);
}
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 6/8] refs: drop unused params from the reflog iterator callback
From: Patrick Steinhardt @ 2024-02-21 12:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708518982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 12615 bytes --]
The ref and reflog iterators share much of the same underlying code to
iterate over the corresponding entries. This results in some weird code
because the reflog iterator also exposes an object ID as well as a flag
to the callback function. Neither of these fields do refer to the reflog
though -- they refer to the corresponding ref with the same name. This
is quite misleading. In practice at least the object ID cannot really be
implemented in any other way as a reflog does not have a specific object
ID in the first place. This is further stressed by the fact that none of
the callbacks except for our test helper make use of these fields.
Split up the infrastucture so that ref and reflog iterators use separate
callback signatures. This allows us to drop the nonsensical fields from
the reflog iterator.
Note that internally, the backends still use the same shared infra to
iterate over both types. As the backends should never end up being
called directly anyway, this is not much of a problem and thus kept
as-is for simplicity's sake.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fsck.c | 4 +---
builtin/reflog.c | 3 +--
refs.c | 23 +++++++++++++++++++----
refs.h | 11 +++++++++--
refs/files-backend.c | 8 +-------
refs/reftable-backend.c | 8 +-------
revision.c | 4 +---
t/helper/test-ref-store.c | 18 ++++++++++++------
t/t0600-reffiles-backend.sh | 24 ++++++++++++------------
t/t1405-main-ref-store.sh | 8 ++++----
t/t1406-submodule-ref-store.sh | 8 ++++----
11 files changed, 65 insertions(+), 54 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index a7cf94f67e..f892487c9b 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -509,9 +509,7 @@ static int fsck_handle_reflog_ent(struct object_id *ooid, struct object_id *noid
return 0;
}
-static int fsck_handle_reflog(const char *logname,
- const struct object_id *oid UNUSED,
- int flag UNUSED, void *cb_data)
+static int fsck_handle_reflog(const char *logname, void *cb_data)
{
struct strbuf refname = STRBUF_INIT;
diff --git a/builtin/reflog.c b/builtin/reflog.c
index a5a4099f61..3a0c4d4322 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -60,8 +60,7 @@ struct worktree_reflogs {
struct string_list reflogs;
};
-static int collect_reflog(const char *ref, const struct object_id *oid UNUSED,
- int flags UNUSED, void *cb_data)
+static int collect_reflog(const char *ref, void *cb_data)
{
struct worktree_reflogs *cb = cb_data;
struct worktree *worktree = cb->worktree;
diff --git a/refs.c b/refs.c
index dc25606a82..f9261267f0 100644
--- a/refs.c
+++ b/refs.c
@@ -2512,18 +2512,33 @@ int refs_verify_refname_available(struct ref_store *refs,
return ret;
}
-int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data)
+struct do_for_each_reflog_help {
+ each_reflog_fn *fn;
+ void *cb_data;
+};
+
+static int do_for_each_reflog_helper(struct repository *r UNUSED,
+ const char *refname,
+ const struct object_id *oid UNUSED,
+ int flags,
+ void *cb_data)
+{
+ struct do_for_each_reflog_help *hp = cb_data;
+ return hp->fn(refname, hp->cb_data);
+}
+
+int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_data)
{
struct ref_iterator *iter;
- struct do_for_each_ref_help hp = { fn, cb_data };
+ struct do_for_each_reflog_help hp = { fn, cb_data };
iter = refs->be->reflog_iterator_begin(refs);
return do_for_each_repo_ref_iterator(the_repository, iter,
- do_for_each_ref_helper, &hp);
+ do_for_each_reflog_helper, &hp);
}
-int for_each_reflog(each_ref_fn fn, void *cb_data)
+int for_each_reflog(each_reflog_fn fn, void *cb_data)
{
return refs_for_each_reflog(get_main_ref_store(the_repository), fn, cb_data);
}
diff --git a/refs.h b/refs.h
index 303c5fac4d..895579aeb7 100644
--- a/refs.h
+++ b/refs.h
@@ -534,12 +534,19 @@ int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn, void *cb_dat
/* youngest entry first */
int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn, void *cb_data);
+/*
+ * The signature for the callback function for the {refs_,}for_each_reflog()
+ * functions below. The memory pointed to by the refname argument is only
+ * guaranteed to be valid for the duration of a single callback invocation.
+ */
+typedef int each_reflog_fn(const char *refname, void *cb_data);
+
/*
* Calls the specified function for each reflog file until it returns nonzero,
* and returns the value. Reflog file order is unspecified.
*/
-int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data);
-int for_each_reflog(each_ref_fn fn, void *cb_data);
+int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_data);
+int for_each_reflog(each_reflog_fn fn, void *cb_data);
#define REFNAME_ALLOW_ONELEVEL 1
#define REFNAME_REFSPEC_PATTERN 2
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 05bb0c875c..c7aff6b331 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2115,10 +2115,8 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,
struct files_reflog_iterator {
struct ref_iterator base;
-
struct ref_store *ref_store;
struct dir_iterator *dir_iterator;
- struct object_id oid;
};
static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
@@ -2129,8 +2127,6 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
int ok;
while ((ok = dir_iterator_advance(diter)) == ITER_OK) {
- int flags;
-
if (!S_ISREG(diter->st.st_mode))
continue;
if (diter->basename[0] == '.')
@@ -2140,14 +2136,12 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
if (!refs_resolve_ref_unsafe(iter->ref_store,
diter->relative_path, 0,
- &iter->oid, &flags)) {
+ NULL, NULL)) {
error("bad ref for %s", diter->path.buf);
continue;
}
iter->base.refname = diter->relative_path;
- iter->base.oid = &iter->oid;
- iter->base.flags = flags;
return ITER_OK;
}
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 39e9a9d4e2..4998b676c2 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1594,7 +1594,6 @@ struct reftable_reflog_iterator {
struct reftable_ref_store *refs;
struct reftable_iterator iter;
struct reftable_log_record log;
- struct object_id oid;
char *last_name;
int err;
};
@@ -1605,8 +1604,6 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)
(struct reftable_reflog_iterator *)ref_iterator;
while (!iter->err) {
- int flags;
-
iter->err = reftable_iterator_next_log(&iter->iter, &iter->log);
if (iter->err)
break;
@@ -1620,7 +1617,7 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)
continue;
if (!refs_resolve_ref_unsafe(&iter->refs->base, iter->log.refname,
- 0, &iter->oid, &flags)) {
+ 0, NULL, NULL)) {
error(_("bad ref for %s"), iter->log.refname);
continue;
}
@@ -1628,8 +1625,6 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)
free(iter->last_name);
iter->last_name = xstrdup(iter->log.refname);
iter->base.refname = iter->log.refname;
- iter->base.oid = &iter->oid;
- iter->base.flags = flags;
break;
}
@@ -1682,7 +1677,6 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl
iter = xcalloc(1, sizeof(*iter));
base_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable);
iter->refs = refs;
- iter->base.oid = &iter->oid;
ret = refs->err;
if (ret)
diff --git a/revision.c b/revision.c
index 2424c9bd67..ac45c6d8f2 100644
--- a/revision.c
+++ b/revision.c
@@ -1686,9 +1686,7 @@ static int handle_one_reflog_ent(struct object_id *ooid, struct object_id *noid,
return 0;
}
-static int handle_one_reflog(const char *refname_in_wt,
- const struct object_id *oid UNUSED,
- int flag UNUSED, void *cb_data)
+static int handle_one_reflog(const char *refname_in_wt, void *cb_data)
{
struct all_refs_cb *cb = cb_data;
struct strbuf refname = STRBUF_INIT;
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 702ec1f128..7a0f6cac53 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -221,15 +221,21 @@ static int cmd_verify_ref(struct ref_store *refs, const char **argv)
return ret;
}
+static int each_reflog(const char *refname, void *cb_data UNUSED)
+{
+ printf("%s\n", refname);
+ return 0;
+}
+
static int cmd_for_each_reflog(struct ref_store *refs,
const char **argv UNUSED)
{
- return refs_for_each_reflog(refs, each_ref, NULL);
+ return refs_for_each_reflog(refs, each_reflog, NULL);
}
-static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,
- const char *committer, timestamp_t timestamp,
- int tz, const char *msg, void *cb_data UNUSED)
+static int each_reflog_ent(struct object_id *old_oid, struct object_id *new_oid,
+ const char *committer, timestamp_t timestamp,
+ int tz, const char *msg, void *cb_data UNUSED)
{
printf("%s %s %s %" PRItime " %+05d%s%s", oid_to_hex(old_oid),
oid_to_hex(new_oid), committer, timestamp, tz,
@@ -241,14 +247,14 @@ static int cmd_for_each_reflog_ent(struct ref_store *refs, const char **argv)
{
const char *refname = notnull(*argv++, "refname");
- return refs_for_each_reflog_ent(refs, refname, each_reflog, refs);
+ return refs_for_each_reflog_ent(refs, refname, each_reflog_ent, refs);
}
static int cmd_for_each_reflog_ent_reverse(struct ref_store *refs, const char **argv)
{
const char *refname = notnull(*argv++, "refname");
- return refs_for_each_reflog_ent_reverse(refs, refname, each_reflog, refs);
+ return refs_for_each_reflog_ent_reverse(refs, refname, each_reflog_ent, refs);
}
static int cmd_reflog_exists(struct ref_store *refs, const char **argv)
diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
index 4f860285cc..56a3196b83 100755
--- a/t/t0600-reffiles-backend.sh
+++ b/t/t0600-reffiles-backend.sh
@@ -287,23 +287,23 @@ test_expect_success 'for_each_reflog()' '
mkdir -p .git/worktrees/wt/logs/refs/bisect &&
echo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random &&
- $RWT for-each-reflog | cut -d" " -f 2- >actual &&
+ $RWT for-each-reflog >actual &&
cat >expected <<-\EOF &&
- HEAD 0x1
- PSEUDO-WT 0x0
- refs/bisect/wt-random 0x0
- refs/heads/main 0x0
- refs/heads/wt-main 0x0
+ HEAD
+ PSEUDO-WT
+ refs/bisect/wt-random
+ refs/heads/main
+ refs/heads/wt-main
EOF
test_cmp expected actual &&
- $RMAIN for-each-reflog | cut -d" " -f 2- >actual &&
+ $RMAIN for-each-reflog >actual &&
cat >expected <<-\EOF &&
- HEAD 0x1
- PSEUDO-MAIN 0x0
- refs/bisect/random 0x0
- refs/heads/main 0x0
- refs/heads/wt-main 0x0
+ HEAD
+ PSEUDO-MAIN
+ refs/bisect/random
+ refs/heads/main
+ refs/heads/wt-main
EOF
test_cmp expected actual
'
diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
index cfb583f544..3eee758bce 100755
--- a/t/t1405-main-ref-store.sh
+++ b/t/t1405-main-ref-store.sh
@@ -74,11 +74,11 @@ test_expect_success 'verify_ref(new-main)' '
'
test_expect_success 'for_each_reflog()' '
- $RUN for-each-reflog | cut -d" " -f 2- >actual &&
+ $RUN for-each-reflog >actual &&
cat >expected <<-\EOF &&
- HEAD 0x1
- refs/heads/main 0x0
- refs/heads/new-main 0x0
+ HEAD
+ refs/heads/main
+ refs/heads/new-main
EOF
test_cmp expected actual
'
diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
index 40332e23cc..c01f0f14a1 100755
--- a/t/t1406-submodule-ref-store.sh
+++ b/t/t1406-submodule-ref-store.sh
@@ -63,11 +63,11 @@ test_expect_success 'verify_ref(new-main)' '
'
test_expect_success 'for_each_reflog()' '
- $RUN for-each-reflog | cut -d" " -f 2- >actual &&
+ $RUN for-each-reflog >actual &&
cat >expected <<-\EOF &&
- HEAD 0x1
- refs/heads/main 0x0
- refs/heads/new-main 0x0
+ HEAD
+ refs/heads/main
+ refs/heads/new-main
EOF
test_cmp expected actual
'
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 7/8] refs: stop resolving ref corresponding to reflogs
From: Patrick Steinhardt @ 2024-02-21 12:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708518982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3407 bytes --]
The reflog iterator tries to resolve the corresponding ref for every
reflog that it is about to yield. Historically, this was done due to
multiple reasons:
- It ensures that the refname is safe because we end up calling
`check_refname_format()`. Also, non-conformant refnames are skipped
altogether.
- The iterator used to yield the resolved object ID as well as its
flags to the callback. This info was never used though, and the
corresponding parameters were dropped in the preceding commit.
- When a ref is corrupt then the reflog is not emitted at all.
We're about to introduce a new `git reflog list` subcommand that will
print all reflogs that the refdb knows about. Skipping over reflogs
whose refs are corrupted would be quite counterproductive in this case
as the user would have no way to learn about reflogs which may still
exist in their repository to help and rescue such a corrupted ref. Thus,
the only remaining reason for why we'd want to resolve the ref is to
verify its refname.
Refactor the code to call `check_refname_format()` directly instead of
trying to resolve the ref. This is significantly more efficient given
that we don't have to hit the object database anymore to list reflogs.
And second, it ensures that we end up showing reflogs of broken refs,
which will help to make the reflog more useful.
Note that this really only impacts the case where the corresponding ref
is corrupt. Reflogs for nonexistent refs would have been returned to the
caller beforehand already as we did not pass `RESOLVE_REF_READING` to
the function, and thus `refs_resolve_ref_unsafe()` would have returned
successfully in that case.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs/files-backend.c | 12 ++----------
refs/reftable-backend.c | 6 ++----
2 files changed, 4 insertions(+), 14 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index c7aff6b331..6f98168a81 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2129,17 +2129,9 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
while ((ok = dir_iterator_advance(diter)) == ITER_OK) {
if (!S_ISREG(diter->st.st_mode))
continue;
- if (diter->basename[0] == '.')
+ if (check_refname_format(diter->basename,
+ REFNAME_ALLOW_ONELEVEL))
continue;
- if (ends_with(diter->basename, ".lock"))
- continue;
-
- if (!refs_resolve_ref_unsafe(iter->ref_store,
- diter->relative_path, 0,
- NULL, NULL)) {
- error("bad ref for %s", diter->path.buf);
- continue;
- }
iter->base.refname = diter->relative_path;
return ITER_OK;
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 4998b676c2..6c11c4a5e3 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1616,11 +1616,9 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)
if (iter->last_name && !strcmp(iter->log.refname, iter->last_name))
continue;
- if (!refs_resolve_ref_unsafe(&iter->refs->base, iter->log.refname,
- 0, NULL, NULL)) {
- error(_("bad ref for %s"), iter->log.refname);
+ if (check_refname_format(iter->log.refname,
+ REFNAME_ALLOW_ONELEVEL))
continue;
- }
free(iter->last_name);
iter->last_name = xstrdup(iter->log.refname);
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 8/8] builtin/reflog: introduce subcommand to list reflogs
From: Patrick Steinhardt @ 2024-02-21 12:37 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1708518982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7053 bytes --]
While the git-reflog(1) command has subcommands to show reflog entries
or check for reflog existence, it does not have any subcommands that
would allow the user to enumerate all existing reflogs. This makes it
quite hard to discover which reflogs a repository has. While this can
be worked around with the "files" backend by enumerating files in the
".git/logs" directory, users of the "reftable" backend don't enjoy such
a luxury.
Introduce a new subcommand `git reflog list` that lists all reflogs the
repository knows of to fill this gap.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-reflog.txt | 3 +
builtin/reflog.c | 34 +++++++++++
t/t1410-reflog.sh | 108 +++++++++++++++++++++++++++++++++++
3 files changed, 145 insertions(+)
diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
index ec64cbff4c..a929c52982 100644
--- a/Documentation/git-reflog.txt
+++ b/Documentation/git-reflog.txt
@@ -10,6 +10,7 @@ SYNOPSIS
--------
[verse]
'git reflog' [show] [<log-options>] [<ref>]
+'git reflog list'
'git reflog expire' [--expire=<time>] [--expire-unreachable=<time>]
[--rewrite] [--updateref] [--stale-fix]
[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]
@@ -39,6 +40,8 @@ actions, and in addition the `HEAD` reflog records branch switching.
`git reflog show` is an alias for `git log -g --abbrev-commit
--pretty=oneline`; see linkgit:git-log[1] for more information.
+The "list" subcommand lists all refs which have a corresponding reflog.
+
The "expire" subcommand prunes older reflog entries. Entries older
than `expire` time, or entries older than `expire-unreachable` time
and not reachable from the current tip, are removed from the reflog.
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 3a0c4d4322..63cd4d8b29 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -7,11 +7,15 @@
#include "wildmatch.h"
#include "worktree.h"
#include "reflog.h"
+#include "refs.h"
#include "parse-options.h"
#define BUILTIN_REFLOG_SHOW_USAGE \
N_("git reflog [show] [<log-options>] [<ref>]")
+#define BUILTIN_REFLOG_LIST_USAGE \
+ N_("git reflog list")
+
#define BUILTIN_REFLOG_EXPIRE_USAGE \
N_("git reflog expire [--expire=<time>] [--expire-unreachable=<time>]\n" \
" [--rewrite] [--updateref] [--stale-fix]\n" \
@@ -29,6 +33,11 @@ static const char *const reflog_show_usage[] = {
NULL,
};
+static const char *const reflog_list_usage[] = {
+ BUILTIN_REFLOG_LIST_USAGE,
+ NULL,
+};
+
static const char *const reflog_expire_usage[] = {
BUILTIN_REFLOG_EXPIRE_USAGE,
NULL
@@ -46,6 +55,7 @@ static const char *const reflog_exists_usage[] = {
static const char *const reflog_usage[] = {
BUILTIN_REFLOG_SHOW_USAGE,
+ BUILTIN_REFLOG_LIST_USAGE,
BUILTIN_REFLOG_EXPIRE_USAGE,
BUILTIN_REFLOG_DELETE_USAGE,
BUILTIN_REFLOG_EXISTS_USAGE,
@@ -238,6 +248,29 @@ static int cmd_reflog_show(int argc, const char **argv, const char *prefix)
return cmd_log_reflog(argc, argv, prefix);
}
+static int show_reflog(const char *refname, void *cb_data UNUSED)
+{
+ printf("%s\n", refname);
+ return 0;
+}
+
+static int cmd_reflog_list(int argc, const char **argv, const char *prefix)
+{
+ struct option options[] = {
+ OPT_END()
+ };
+ struct ref_store *ref_store;
+
+ argc = parse_options(argc, argv, prefix, options, reflog_list_usage, 0);
+ if (argc)
+ return error(_("%s does not accept arguments: '%s'"),
+ "list", argv[0]);
+
+ ref_store = get_main_ref_store(the_repository);
+
+ return refs_for_each_reflog(ref_store, show_reflog, NULL);
+}
+
static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
{
struct cmd_reflog_expire_cb cmd = { 0 };
@@ -417,6 +450,7 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)
parse_opt_subcommand_fn *fn = NULL;
struct option options[] = {
OPT_SUBCOMMAND("show", &fn, cmd_reflog_show),
+ OPT_SUBCOMMAND("list", &fn, cmd_reflog_list),
OPT_SUBCOMMAND("expire", &fn, cmd_reflog_expire),
OPT_SUBCOMMAND("delete", &fn, cmd_reflog_delete),
OPT_SUBCOMMAND("exists", &fn, cmd_reflog_exists),
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index d2f5f42e67..5bf883f1e3 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -436,4 +436,112 @@ test_expect_success 'empty reflog' '
test_must_be_empty err
'
+test_expect_success 'list reflogs' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ git reflog list >actual &&
+ test_must_be_empty actual &&
+
+ test_commit A &&
+ cat >expect <<-EOF &&
+ HEAD
+ refs/heads/main
+ EOF
+ git reflog list >actual &&
+ test_cmp expect actual &&
+
+ git branch b &&
+ cat >expect <<-EOF &&
+ HEAD
+ refs/heads/b
+ refs/heads/main
+ EOF
+ git reflog list >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'list reflogs with worktree' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+
+ test_commit A &&
+ git worktree add wt &&
+ git -c core.logAllRefUpdates=always \
+ update-ref refs/worktree/main HEAD &&
+ git -c core.logAllRefUpdates=always \
+ update-ref refs/worktree/per-worktree HEAD &&
+ git -c core.logAllRefUpdates=always -C wt \
+ update-ref refs/worktree/per-worktree HEAD &&
+ git -c core.logAllRefUpdates=always -C wt \
+ update-ref refs/worktree/worktree HEAD &&
+
+ cat >expect <<-EOF &&
+ HEAD
+ refs/heads/main
+ refs/heads/wt
+ refs/worktree/main
+ refs/worktree/per-worktree
+ EOF
+ git reflog list >actual &&
+ test_cmp expect actual &&
+
+ cat >expect <<-EOF &&
+ HEAD
+ refs/heads/main
+ refs/heads/wt
+ refs/worktree/per-worktree
+ refs/worktree/worktree
+ EOF
+ git -C wt reflog list >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'reflog list returns error with additional args' '
+ cat >expect <<-EOF &&
+ error: list does not accept arguments: ${SQ}bogus${SQ}
+ EOF
+ test_must_fail git reflog list bogus 2>err &&
+ test_cmp expect err
+'
+
+test_expect_success 'reflog for symref with unborn target can be listed' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit A &&
+ git symbolic-ref HEAD refs/heads/unborn &&
+ cat >expect <<-EOF &&
+ HEAD
+ refs/heads/main
+ EOF
+ git reflog list >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'reflog with invalid object ID can be listed' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ test_commit A &&
+ test-tool ref-store main update-ref msg refs/heads/missing \
+ $(test_oid deadbeef) "$ZERO_OID" REF_SKIP_OID_VERIFICATION &&
+ cat >expect <<-EOF &&
+ HEAD
+ refs/heads/main
+ refs/heads/missing
+ EOF
+ git reflog list >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
2.44.0-rc1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH 04/12] fsmonitor: refactor refresh callback on directory events
From: Patrick Steinhardt @ 2024-02-21 12:54 UTC (permalink / raw)
To: Jeff Hostetler; +Cc: Jeff Hostetler via GitGitGadget, git, Jeff Hostetler
In-Reply-To: <3fcb41e4-b17e-48ec-b54c-50452654b28c@jeffhostetler.com>
[-- Attachment #1: Type: text/plain, Size: 2672 bytes --]
On Tue, Feb 20, 2024 at 01:54:44PM -0500, Jeff Hostetler wrote:
>
>
> On 2/15/24 4:32 AM, Patrick Steinhardt wrote:
> > On Tue, Feb 13, 2024 at 08:52:13PM +0000, Jeff Hostetler via GitGitGadget wrote:
> > > From: Jeff Hostetler <jeffhostetler@github.com>
> > >
> > > Signed-off-by: Jeff Hostetler <jeffhostetler@github.com>
> > > ---
> > > fsmonitor.c | 52 ++++++++++++++++++++++++++++++----------------------
> > > 1 file changed, 30 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/fsmonitor.c b/fsmonitor.c
> > > index f670c509378..b1ef01bf3cd 100644
> > > --- a/fsmonitor.c
> > > +++ b/fsmonitor.c
> > > @@ -183,6 +183,35 @@ static int query_fsmonitor_hook(struct repository *r,
> > > return result;
> > > }
> > > +static void fsmonitor_refresh_callback_slash(
> > > + struct index_state *istate, const char *name, int len, int pos)
> >
> > `len` should be `size_t` as it tracks the length of the name. This is
> > a preexisting issue already because `fsmonitor_refresh_callback()`
> > assigns `int len = strlen(name)`, which is wrong.
> >
> > > +{
> > > + int i;
> >
> > `i` is used to iterate through `istate->cache_nr`, which is an `unsigned
> > int` and not an `int`. I really wish we would compile the Git code base
> > with `-Wconversion`, but that's a rather big undertaking.
> >
> > Anyway, none of these issues are new as you merely move the code into a
> > new function.
> >
> > Patrick
> >
>
> Yeah, the int types are bit of a mess, but I'd like to defer that to
> another series.
>
> There are several things going on here as you point out.
>
> (1) 'int len' for the length of a pathname buffer. This could be fixed,
> but the odds of Git correctly working with a >2Gb pathname doesn't make
> this urgent.
>
> (2) 'int i' is signed, but should be unsigned -- but elsewhere in this
> function we use 'int pos' which an index on the same array and has
> double duty as the suggested insertion point when negative. So we can
> fix the type of 'i', but that just sets us up to hide errors with 'pos',
> since they'd have different types. Also, since it is really unlikely
> for Git to work with >2G cache-entries, I think this one can wait too.
>
> I don't mean to be confrontational, but I think these changes should
> wait until another series that is focused on just those types of issues.
No worries, this doesn't come across as confrontational at all. As I
said myself, the problems aren't really new to your patch series but are
are preexisting and run deeper than what you can see in the diffs here.
So I'm perfectly fine with deferring this.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Breaking change with "git log -n" since 2.43
From: Maarten Ackermans @ 2024-02-21 13:32 UTC (permalink / raw)
To: git
Hi all,
I would like to report a breaking change with "git log -n" introduced
in 2.43 that's causing some trouble:
https://github.com/git/git/commit/71a1e94821666909b7b2bd62a36244c601f8430e#diff-380c4eac267b5af349ace88c78a2b004a16ed20c2b605c76827981063924bbf9R2222
To reproduce, the command `git log -n 9007199254740991` fails on
2.43.2, whereas it didn't on 2.42.0. This specific number corresponds
to the Number.MAX_SAFE_INTEGER (2^53 - 1) in JavaScript (docs:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER).
The max value that is supported now is a signed 32-bit integer (2^31 -
1).
I suppose git simply ignored the extra digits of the number, as the
commit message describes.
See https://github.com/intuit/auto/issues/2425#issuecomment-1956557071
for the impact.
Best regards,
Maarten Ackermans
^ permalink raw reply
* Re: Breaking change with "git log -n" since 2.43
From: Kristoffer Haugsbakk @ 2024-02-21 13:55 UTC (permalink / raw)
To: Maarten Ackermans; +Cc: git
In-Reply-To: <CAB=tB2vB0LbP=DznSqTFYHCRxDxd6U=Q+P33yeBzGssq2eK1vA@mail.gmail.com>
On Wed, Feb 21, 2024, at 14:32, Maarten Ackermans wrote:
> Hi all,
>
> I would like to report a breaking change with "git log -n" introduced
> in 2.43 that's causing some trouble:
> https://github.com/git/git/commit/71a1e94821666909b7b2bd62a36244c601f8430e#diff-380c4eac267b5af349ace88c78a2b004a16ed20c2b605c76827981063924bbf9R2222
>
> To reproduce, the command `git log -n 9007199254740991` fails on
> 2.43.2, whereas it didn't on 2.42.0. This specific number corresponds
> to the Number.MAX_SAFE_INTEGER (2^53 - 1) in JavaScript (docs:
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER).
> The max value that is supported now is a signed 32-bit integer (2^31 -
> 1).
>
> I suppose git simply ignored the extra digits of the number, as the
> commit message describes.
>
> See https://github.com/intuit/auto/issues/2425#issuecomment-1956557071
> for the impact.
>
> Best regards,
>
> Maarten Ackermans
I don’t see how this is a breaking change considering the range is not
documented.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: Breaking change with "git log -n" since 2.43
From: Maarten Ackermans @ 2024-02-21 14:21 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <9c52ea4e-f84e-4c64-977d-14a468236c80@app.fastmail.com>
Whether or not that is Git's definition of a breaking change, the
message of the commit in question acknowledges that the commands in
the "log" family are the oldest in the system:
> The "rev-list" and other commands in the "log" family, being the oldest part of the system, use their own custom argument parsers, and integer values of some options are parsed with atoi(), which allows a non-digit after the number (e.g., "1q") to be silently ignored. As a natural consequence, an argument that does not begin with a digit (e.g., "q") silently becomes zero, too.
Applications that have been relying on undocumented features and
limits since they were introduced, now face a hard crash: "fatal:
'9007199254740991': not an integer". Regardless of whether this is an
improvement for future implementations, a crash in existing ones is a
suboptimal experience at the least.
On Wed, Feb 21, 2024 at 8:55 PM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
>
> On Wed, Feb 21, 2024, at 14:32, Maarten Ackermans wrote:
> > Hi all,
> >
> > I would like to report a breaking change with "git log -n" introduced
> > in 2.43 that's causing some trouble:
> > https://github.com/git/git/commit/71a1e94821666909b7b2bd62a36244c601f8430e#diff-380c4eac267b5af349ace88c78a2b004a16ed20c2b605c76827981063924bbf9R2222
> >
> > To reproduce, the command `git log -n 9007199254740991` fails on
> > 2.43.2, whereas it didn't on 2.42.0. This specific number corresponds
> > to the Number.MAX_SAFE_INTEGER (2^53 - 1) in JavaScript (docs:
> > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER).
> > The max value that is supported now is a signed 32-bit integer (2^31 -
> > 1).
> >
> > I suppose git simply ignored the extra digits of the number, as the
> > commit message describes.
> >
> > See https://github.com/intuit/auto/issues/2425#issuecomment-1956557071
> > for the impact.
> >
> > Best regards,
> >
> > Maarten Ackermans
>
> I don’t see how this is a breaking change considering the range is not
> documented.
>
> --
> Kristoffer Haugsbakk
>
^ permalink raw reply
* Re: Breaking change with "git log -n" since 2.43
From: Sean Allred @ 2024-02-21 14:24 UTC (permalink / raw)
To: Maarten Ackermans; +Cc: Kristoffer Haugsbakk, git
In-Reply-To: <CAB=tB2tgbLjBPvgBQDoNJi7e8+LMzxHSbg6D2jKUSJXPmQFrxA@mail.gmail.com>
Maarten Ackermans <maarten.ackermans@gmail.com> writes:
> Applications that have been relying on undocumented features and
> limits since they were introduced, now face a hard crash: "fatal:
> '9007199254740991': not an integer". Regardless of whether this is an
> improvement for future implementations, a crash in existing ones is a
> suboptimal experience at the least.
What behavior would you propose instead?
--
Sean Allred
^ permalink raw reply
* Re: Breaking change with "git log -n" since 2.43
From: Maarten Ackermans @ 2024-02-21 15:07 UTC (permalink / raw)
To: Sean Allred; +Cc: Kristoffer Haugsbakk, git
In-Reply-To: <m04je1dhdx.fsf@epic96565.epic.com>
I would suggest displaying a warning in case of invalid input (such as
this out of range error), and to fall back to output all as if the
"-n" flag was unspecified. If more strict handling is still desired,
it could instead be a deprecation warning with a grace period, giving
applications some time to update their git usage.
On Wed, Feb 21, 2024 at 9:25 PM Sean Allred <allred.sean@gmail.com> wrote:
>
>
> Maarten Ackermans <maarten.ackermans@gmail.com> writes:
> > Applications that have been relying on undocumented features and
> > limits since they were introduced, now face a hard crash: "fatal:
> > '9007199254740991': not an integer". Regardless of whether this is an
> > improvement for future implementations, a crash in existing ones is a
> > suboptimal experience at the least.
>
> What behavior would you propose instead?
>
> --
> Sean Allred
^ 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