* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Liam Beguin @ 2023-12-10 21:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Kousik Sanagavarapu, git
In-Reply-To: <xmqqwmucjhuw.fsf@gitster.g>
Hi Junio,
Apologies for the late reply.
On Tue, Nov 21, 2023 at 05:21:43AM +0900, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Another line of thought is perhaps it is potentially useful to teach
> > the --format= machinery to be a bit more programmable, e.g. allowing
> > to compute a substring of an existing field %{%aE#*@} without having
> > to waste a letter each for the local part and domain part. But as I
> > already said, we are now talking about "postprocessing", and adding
> > complexity to our codebase only to have incomplete flexibility may
> > not be worth it. A more specific %(authoremail:localpart) and its
> > domain counterpart may be easier to explain and understand.
> >
> > In any case, it is a bit too late to say "let's not waste the
> > precious single letter namespace to add useless features", as we
> > have come way too far, so I do not mind too much using a currently
> > unused letter $X for yet another author and committer trait.
>
> When I wrote the above, I somehow forgot the existing work in the
> ref-filter (aka "for-each-ref") placeholders, where we have support
> to a lot more flexible way to customize these things.
I looked into this a little, after your first email. I'll try to make
time to have another look.
> For example, "%(authoremail:mailmap,localpart)" can be used to say,
> instead of wasting two letters 'l' and 'L' out of precious 52, that
> we want e-mail address honoring the mailmap, and take only the local
> part. And the support for the host part of the address that this
> topic discussed should be implementable fairly easily (just adding
> EO_HOSTPART bit to the email_option structure would be sufficient)
> on the ref-filter side.
>
> We saw efforts from time to time to give "log --pretty=format:" more
> of the good things from the "for-each-ref --format=" placeholders
> (and vice versa), and it may give us a good way forward.
This definitely sounds like a better approach than wasting two more
letters.
Liam
^ permalink raw reply
* [BUG] git bisect start gives incorrect error message when good/bad swapped
From: Britton Kerin @ 2023-12-11 6:20 UTC (permalink / raw)
To: git
When good/bad are entirely swapped git bisect says:
$ git bisect start initial_commit latest_master_commit
Some good revs are not ancestors of the bad rev.
git bisect cannot work properly in this case.
Maybe you mistook good and bad revs?
But that's not true because when good is a non-ancestor from a branch
a common ancestor is automatically selected:
$ git bisect start latest_master_commit latest_unmerged_branch_commit
Bisecting: a merge base must be tested
[b93212577c2e8603ed7285b55a0931dcf552c628] I'm yet another test commit
In this case latest_unmerged_branch_commit is not an ancestor of
latest_master_commit but a common ancestor is selected automatically
with a message, and when the branch point is marked as bad git
correctly indicates that the problem has been fixed on the branch (and
bisection stops):
$ git bisect bad
The merge base b93212577c2e8603ed7285b55a0931dcf552c628 is bad.
This means the bug has been fixed between
b93212577c2e8603ed7285b55a0931dcf552c628 and
[1b4470e66cb26244be9aa5f68cca042a0ef4270e].
I suspect what the message at the top should be saying is that the bad
rev *is* an ancestor of some good rev but I haven't thought/tested
carefully enough to feel sure that's the case.
I think this is worth fixing somehow because the wrong message gives a
wrong idea of git capabilities and might make the case when a common
ancestor is automatically selected more unexpected and so more
confusing.
On a more minor note the use of 'merge base' to refer to the common
ancestor is unfortunate since these messages are produced when working
on branches that aren't actually merged. It would be better to use
'common ancestor' (as the merge-base man page does).
Britton
^ permalink raw reply
* Re: [PATCH 3/9] imap-send: don't use git_die_config() inside callback
From: Patrick Steinhardt @ 2023-12-11 7:43 UTC (permalink / raw)
To: Taylor Blau; +Cc: Jeff King, git
In-Reply-To: <ZXOfrKYsmOjOHGmj@nand.local>
[-- Attachment #1: Type: text/plain, Size: 991 bytes --]
On Fri, Dec 08, 2023 at 05:58:52PM -0500, Taylor Blau wrote:
> On Thu, Dec 07, 2023 at 09:57:55AM +0100, Patrick Steinhardt wrote:
> > On Thu, Dec 07, 2023 at 02:24:58AM -0500, Jeff King wrote:
> > [snip]
> > > diff --git a/imap-send.c b/imap-send.c
> > > index 996651e4f8..5b0fe4f95a 100644
> > > --- a/imap-send.c
> > > +++ b/imap-send.c
> > > @@ -1346,7 +1346,7 @@ static int git_imap_config(const char *var, const char *val,
> > > server.port = git_config_int(var, val, ctx->kvi);
> > > else if (!strcmp("imap.host", var)) {
> > > if (!val) {
> > > - git_die_config("imap.host", "Missing value for 'imap.host'");
> > > + return error("Missing value for 'imap.host'");
> >
> > Nit: while at it we might also mark this error for translation. Not
> > worth a reroll on its own though.
>
> This string goes away entirely in the next patch, so I don't think we
> need to mark it here.
>
> Thanks,
> Taylor
Ah, true. Never mind in that case.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH][RESEND] show-ref: use die_for_incompatible_opt3()
From: René Scharfe @ 2023-12-11 8:09 UTC (permalink / raw)
To: Git List
Cc: Jean-Noël Avila, Patrick Steinhardt, Junio C Hamano,
Eric Sunshine
In-Reply-To: <CAPig+cR5PKkyC24LkOU4+yzng1xeBOBbADTBHXH61xkAR7kymw@mail.gmail.com>
Use the standard message for reporting the use of multiple mutually
exclusive options by calling die_for_incompatible_opt3() instead of
rolling our own. This has the benefits of showing only the actually
given options, reducing the number of strings to translate and making
the UI slightly more consistent.
Adjust the test to no longer insist on a specific order of the
reported options, as this implementation detail does not affect the
usefulness of the error message.
Reported-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
---
Original submission:
https://lore.kernel.org/git/d1f28272-635d-4638-b0f4-76d64013b0d5@web.de/
builtin/show-ref.c | 6 +++---
t/t1403-show-ref.sh | 16 +++++++++-------
2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 7aac525a87..59d2291cbf 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -315,9 +315,9 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, show_ref_options,
show_ref_usage, 0);
- if ((!!exclude_existing_opts.enabled + !!verify + !!exists) > 1)
- die(_("only one of '%s', '%s' or '%s' can be given"),
- "--exclude-existing", "--verify", "--exists");
+ die_for_incompatible_opt3(exclude_existing_opts.enabled, "--exclude-existing",
+ verify, "--verify",
+ exists, "--exists");
if (exclude_existing_opts.enabled)
return cmd_show_ref__exclude_existing(&exclude_existing_opts);
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index b50ae6fcf1..d477689e33 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -197,18 +197,20 @@ test_expect_success 'show-ref --verify with dangling ref' '
'
test_expect_success 'show-ref sub-modes are mutually exclusive' '
- cat >expect <<-EOF &&
- fatal: only one of ${SQ}--exclude-existing${SQ}, ${SQ}--verify${SQ} or ${SQ}--exists${SQ} can be given
- EOF
-
test_must_fail git show-ref --verify --exclude-existing 2>err &&
- test_cmp expect err &&
+ grep "verify" err &&
+ grep "exclude-existing" err &&
+ grep "cannot be used together" err &&
test_must_fail git show-ref --verify --exists 2>err &&
- test_cmp expect err &&
+ grep "verify" err &&
+ grep "exists" err &&
+ grep "cannot be used together" err &&
test_must_fail git show-ref --exclude-existing --exists 2>err &&
- test_cmp expect err
+ grep "exclude-existing" err &&
+ grep "exists" err &&
+ grep "cannot be used together" err
'
test_expect_success '--exists with existing reference' '
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 09/24] repack: implement `--extend-disjoint` mode
From: Patrick Steinhardt @ 2023-12-11 8:18 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King, Junio C Hamano
In-Reply-To: <ZXOdPLotcS5daNke@nand.local>
[-- Attachment #1: Type: text/plain, Size: 4189 bytes --]
On Fri, Dec 08, 2023 at 05:48:28PM -0500, Taylor Blau wrote:
> On Fri, Dec 08, 2023 at 09:19:25AM +0100, Patrick Steinhardt wrote:
> > > > One thing I wondered: do we need to consider the `-l` flag? When using
> > > > an alternate object directory it is totally feasible that the alternate
> > > > may be creating new disjoint packages without us knowing, and thus we
> > > > may not be able to guarantee the disjoint property anymore.
> > >
> > > I don't think so. We'd only care about one direction of this (that
> > > alternates do not create disjoint packs which overlap with ours, instead
> > > of the other way around), but since we don't put non-local packs in the
> > > MIDX, I think we're OK.
> > >
> > > I suppose that you might run into trouble if you use the chained MIDX
> > > thing (via its `->next` pointer). I haven't used that feature myself, so
> > > I'd have to play around with it.
> >
> > We do use this feature at GitLab for forks, where forks connect to a
> > common alternate object directory to deduplicate objects. As both the
> > fork repository and the alternate object directory use an MIDX I think
> > they would be set up exactly like that.
>
> Yep, that's right. I wasn't sure whether or not this feature had been
> used extensively in production or not (we don't use it at GitHub, since
> objects only live in their fork repositories for a short while before
> moving to the fork network repository).
>
> > I guess the only really viable solution here is to ignore disjoint packs
> > in the main repo that connects to the alternate in the case where the
> > alternate has any disjoint packs itself.
>
> I think the behavior you'd get here is that we'd only look for disjoint
> packs in the first MIDX in the chain (which is always the local one),
> and we'd only recognizes packs from that MIDX as being potentially
> disjoint.
>
> If you have the bulk of your repositories in the alternate, then I think
> you might want to consider how we combine the two.
Yes, in the general case the bulk of objects is indeed contained in the
alternate.
> My sense is that you'd want to be disjoint with respect to anything
> downstream of you.
Ideally yes, but this is unfortunately not easily achievable in the
general case. It's one of the many painpoints that alternates bring with
them.
Suppose two forks A and B are connected to the same alternate. Both A
and B now gain the same set of objects via whatever means. At this point
these objects can be stored in disjoint packs in each of the repos as
they are not yet deduplicated via the alternate. But if you were to pull
objects from either A or B into the alternate then you cannot ensure
disjointedness at all anymore because you would first have to repack
objects in both A and B.
For two forks this might still seem manageable. But as soon as your fork
network grows larger it's clear that this becomes almost impossible to
do. So ultimately, I don't see an alternative to ignoring disjointedness
bits in either of the two linked-together repos.
> Whether or not this is a feature that you/others need, I definitely
> think we should leave it out of this series, since I am (a) fairly
> certain that this is possible to do, and (b) already feel like this
> series on its own is complicated enough.
I'm perfectly fine if we say that the benefits of your patch series
cannot yet be applied to repositories with alternates. But from my point
of view it's a requirement that this patch series does not silently
break this usecase due to Git starting to generate disjointed packs
where it cannot ensure that the disjointedness property holds.
As I haven't yet read through this whole patch series, the question is
boils down to whether the end result is opt-in or opt-out. If it was
opt-out then I could see the above usecase breaking silently. If it was
opt-in then things should be fine and we can address this ommission in a
follow up patch series. We at GitLab would definitely be interested in
helping out with this given that it directly affects us and that the
demonstrated savings seem very promising.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v3 01/11] reftable: wrap EXPECT macros in do/while
From: Patrick Steinhardt @ 2023-12-11 9:07 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Jonathan Nieder, Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1702285387.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4427 bytes --]
The `EXPECT` macros used by the reftable test framework are all using a
single `if` statement with the actual condition. This results in weird
syntax when using them in if/else statements like the following:
```
if (foo)
EXPECT(foo == 2)
else
EXPECT(bar == 2)
```
Note that there need not be a trailing semicolon. Furthermore, it is not
immediately obvious whether the else now belongs to the `if (foo)` or
whether it belongs to the expanded `if (foo == 2)` from the macro.
Fix this by wrapping the macros in a do/while loop.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/test_framework.h | 58 +++++++++++++++++++++------------------
1 file changed, 32 insertions(+), 26 deletions(-)
diff --git a/reftable/test_framework.h b/reftable/test_framework.h
index 774cb275bf..ee44f735ae 100644
--- a/reftable/test_framework.h
+++ b/reftable/test_framework.h
@@ -12,32 +12,38 @@ license that can be found in the LICENSE file or at
#include "system.h"
#include "reftable-error.h"
-#define EXPECT_ERR(c) \
- if (c != 0) { \
- fflush(stderr); \
- fflush(stdout); \
- fprintf(stderr, "%s: %d: error == %d (%s), want 0\n", \
- __FILE__, __LINE__, c, reftable_error_str(c)); \
- abort(); \
- }
-
-#define EXPECT_STREQ(a, b) \
- if (strcmp(a, b)) { \
- fflush(stderr); \
- fflush(stdout); \
- fprintf(stderr, "%s:%d: %s (%s) != %s (%s)\n", __FILE__, \
- __LINE__, #a, a, #b, b); \
- abort(); \
- }
-
-#define EXPECT(c) \
- if (!(c)) { \
- fflush(stderr); \
- fflush(stdout); \
- fprintf(stderr, "%s: %d: failed assertion %s\n", __FILE__, \
- __LINE__, #c); \
- abort(); \
- }
+#define EXPECT_ERR(c) \
+ do { \
+ if (c != 0) { \
+ fflush(stderr); \
+ fflush(stdout); \
+ fprintf(stderr, "%s: %d: error == %d (%s), want 0\n", \
+ __FILE__, __LINE__, c, reftable_error_str(c)); \
+ abort(); \
+ } \
+ } while (0)
+
+#define EXPECT_STREQ(a, b) \
+ do { \
+ if (strcmp(a, b)) { \
+ fflush(stderr); \
+ fflush(stdout); \
+ fprintf(stderr, "%s:%d: %s (%s) != %s (%s)\n", __FILE__, \
+ __LINE__, #a, a, #b, b); \
+ abort(); \
+ } \
+ } while (0)
+
+#define EXPECT(c) \
+ do { \
+ if (!(c)) { \
+ fflush(stderr); \
+ fflush(stdout); \
+ fprintf(stderr, "%s: %d: failed assertion %s\n", __FILE__, \
+ __LINE__, #c); \
+ abort(); \
+ } \
+ } while (0)
#define RUN_TEST(f) \
fprintf(stderr, "running %s\n", #f); \
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 00/11] reftable: small set of fixes
From: Patrick Steinhardt @ 2023-12-11 9:07 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Jonathan Nieder, Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1700549493.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5933 bytes --]
Hi,
this is the third version of my patch series that addresses several
smallish issues in the reftable backend.
There's only a small set of changes compared to v2:
- Patch 4: convert to use a `struct strbuf` instead of `snprintf()`.
- Patch 5: improve commit message.
- Patch 6: note that `stack_filename()` resets the `struct strbuf` in
the commit message.
- Patch 7: use the `struct filelock`'s lock path instead of the
temporary buffer.
Thanks for your suggestions, Taylor and Eric!
Patrick
Patrick Steinhardt (11):
reftable: wrap EXPECT macros in do/while
reftable: handle interrupted reads
reftable: handle interrupted writes
reftable/stack: verify that `reftable_stack_add()` uses
auto-compaction
reftable/stack: perform auto-compaction with transactional interface
reftable/stack: reuse buffers when reloading stack
reftable/stack: fix stale lock when dying
reftable/stack: fix use of unseeded randomness
reftable/merged: reuse buffer to compute record keys
reftable/block: introduce macro to initialize `struct block_iter`
reftable/block: reuse buffer to compute record keys
reftable/block.c | 23 ++++----
reftable/block.h | 6 +++
reftable/block_test.c | 4 +-
reftable/blocksource.c | 2 +-
reftable/iter.h | 8 +--
reftable/merged.c | 31 +++++------
reftable/merged.h | 2 +
reftable/reader.c | 7 ++-
reftable/readwrite_test.c | 6 +--
reftable/stack.c | 73 +++++++++++---------------
reftable/stack_test.c | 107 +++++++++++++++++++++++++++++++++++++-
reftable/test_framework.h | 58 ++++++++++++---------
12 files changed, 213 insertions(+), 114 deletions(-)
Range-diff against v2:
1: 0ebbb02d32 = 1: 5b2a64ca9f reftable: wrap EXPECT macros in do/while
2: b404fdf066 = 2: 3e8e63ece5 reftable: handle interrupted reads
3: 8c1d78b12b = 3: 1700d00d1c reftable: handle interrupted writes
4: 8061b9d2fc ! 4: 5e27d0a556 reftable/stack: verify that `reftable_stack_add()` uses auto-compaction
@@ reftable/stack_test.c: static void test_reftable_stack_auto_compaction(void)
+{
+ struct reftable_write_options cfg = { 0 };
+ struct reftable_stack *st = NULL;
++ struct strbuf refname = STRBUF_INIT;
+ char *dir = get_tmp_dir(__LINE__);
+ int err, i, n = 20;
+
@@ reftable/stack_test.c: static void test_reftable_stack_auto_compaction(void)
+ .value_type = REFTABLE_REF_SYMREF,
+ .value.symref = "master",
+ };
-+ char name[100];
+
+ /*
+ * Disable auto-compaction for all but the last runs. Like this
@@ reftable/stack_test.c: static void test_reftable_stack_auto_compaction(void)
+ */
+ st->disable_auto_compact = i != n;
+
-+ snprintf(name, sizeof(name), "branch%04d", i);
-+ ref.refname = name;
++ strbuf_reset(&refname);
++ strbuf_addf(&refname, "branch-%04d", i);
++ ref.refname = refname.buf;
+
+ err = reftable_stack_add(st, &write_test_ref, &ref);
+ EXPECT_ERR(err);
@@ reftable/stack_test.c: static void test_reftable_stack_auto_compaction(void)
+ }
+
+ reftable_stack_destroy(st);
++ strbuf_release(&refname);
+ clear_dir(dir);
+}
+
5: 77b9ae8aa6 ! 5: dd180eba40 reftable/stack: perform auto-compaction with transactional interface
@@ Commit message
Whenever updating references or reflog entries in the reftable stack, we
need to add a new table to the stack, thus growing the stack's length by
- one. It can thus happen quite fast that the stack grows very long, which
- results in performance issues when trying to read records. But besides
- performance issues, this can also lead to exhaustion of file descriptors
- very rapidly as every single table requires a separate descriptor when
+ one. The stack can grow to become quite long rather quickly, leading to
+ performance issues when trying to read records. But besides performance
+ issues, this can also lead to exhaustion of file descriptors very
+ rapidly as every single table requires a separate descriptor when
opening the stack.
While git-pack-refs(1) fixes this issue for us by merging the tables, it
6: f797feff8d ! 6: 6ed9ba60db reftable/stack: reuse buffers when reloading stack
@@ Commit message
of the allocated buffer outside of the loop.
Refactor the code to instead reuse the buffers to reduce the number of
- allocations we need to do.
+ allocations we need to do. Note that we do not have to manually reset
+ the buffer because `stack_filename()` does this for us already.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
7: e82a68aecd ! 7: fbd9efa56d reftable/stack: fix stale lock when dying
@@ reftable/stack.c: static int reftable_stack_init_addition(struct reftable_additi
}
if (st->config.default_permissions) {
- if (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) {
-+ if (chmod(lock_file_name.buf, st->config.default_permissions) < 0) {
++ if (chmod(add->lock_file->filename.buf, st->config.default_permissions) < 0) {
err = REFTABLE_IO_ERROR;
goto done;
}
8: bab4fb93df = 8: 5598460b81 reftable/stack: fix use of unseeded randomness
9: cbf77ec45a = 9: 79e0382603 reftable/merged: reuse buffer to compute record keys
10: c9a1405a9a = 10: 8574ad7635 reftable/block: introduce macro to initialize `struct block_iter`
11: 02b11f3a80 = 11: eeb6c35823 reftable/block: reuse buffer to compute record keys
base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v3 02/11] reftable: handle interrupted reads
From: Patrick Steinhardt @ 2023-12-11 9:07 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Jonathan Nieder, Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1702285387.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]
There are calls to pread(3P) and read(3P) where we don't properly handle
interrupts. Convert them to use `pread_in_full()` and `read_in_full()`,
respectively.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/blocksource.c | 2 +-
reftable/stack.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/reftable/blocksource.c b/reftable/blocksource.c
index 8331b34e82..a1ea304429 100644
--- a/reftable/blocksource.c
+++ b/reftable/blocksource.c
@@ -109,7 +109,7 @@ static int file_read_block(void *v, struct reftable_block *dest, uint64_t off,
struct file_block_source *b = v;
assert(off + size <= b->size);
dest->data = reftable_malloc(size);
- if (pread(b->fd, dest->data, size, off) != size)
+ if (pread_in_full(b->fd, dest->data, size, off) != size)
return -1;
dest->len = size;
return size;
diff --git a/reftable/stack.c b/reftable/stack.c
index ddbdf1b9c8..ed108a929b 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -92,7 +92,7 @@ static int fd_read_lines(int fd, char ***namesp)
}
buf = reftable_malloc(size + 1);
- if (read(fd, buf, size) != size) {
+ if (read_in_full(fd, buf, size) != size) {
err = REFTABLE_IO_ERROR;
goto done;
}
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 03/11] reftable: handle interrupted writes
From: Patrick Steinhardt @ 2023-12-11 9:07 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Jonathan Nieder, Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1702285387.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1923 bytes --]
There are calls to write(3P) where we don't properly handle interrupts.
Convert them to use `write_in_full()`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/stack.c | 6 +++---
reftable/stack_test.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index ed108a929b..f0cadad490 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -42,7 +42,7 @@ static void stack_filename(struct strbuf *dest, struct reftable_stack *st,
static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz)
{
int *fdp = (int *)arg;
- return write(*fdp, data, sz);
+ return write_in_full(*fdp, data, sz);
}
int reftable_new_stack(struct reftable_stack **dest, const char *dir,
@@ -554,7 +554,7 @@ int reftable_addition_commit(struct reftable_addition *add)
strbuf_addstr(&table_list, "\n");
}
- err = write(add->lock_file_fd, table_list.buf, table_list.len);
+ err = write_in_full(add->lock_file_fd, table_list.buf, table_list.len);
strbuf_release(&table_list);
if (err < 0) {
err = REFTABLE_IO_ERROR;
@@ -1024,7 +1024,7 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,
strbuf_addstr(&ref_list_contents, "\n");
}
- err = write(lock_file_fd, ref_list_contents.buf, ref_list_contents.len);
+ err = write_in_full(lock_file_fd, ref_list_contents.buf, ref_list_contents.len);
if (err < 0) {
err = REFTABLE_IO_ERROR;
unlink(new_table_path.buf);
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index d0b717510f..0644c8ad2e 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -78,7 +78,7 @@ static void test_read_file(void)
int i = 0;
EXPECT(fd > 0);
- n = write(fd, out, strlen(out));
+ n = write_in_full(fd, out, strlen(out));
EXPECT(n == strlen(out));
err = close(fd);
EXPECT(err >= 0);
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 04/11] reftable/stack: verify that `reftable_stack_add()` uses auto-compaction
From: Patrick Steinhardt @ 2023-12-11 9:07 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Jonathan Nieder, Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1702285387.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2581 bytes --]
While we have several tests that check whether we correctly perform
auto-compaction when manually calling `reftable_stack_auto_compact()`,
we don't have any tests that verify whether `reftable_stack_add()` does
call it automatically. Add one.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/stack_test.c | 49 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 0644c8ad2e..52b4dc3b14 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -850,6 +850,54 @@ static void test_reftable_stack_auto_compaction(void)
clear_dir(dir);
}
+static void test_reftable_stack_add_performs_auto_compaction(void)
+{
+ struct reftable_write_options cfg = { 0 };
+ struct reftable_stack *st = NULL;
+ struct strbuf refname = STRBUF_INIT;
+ char *dir = get_tmp_dir(__LINE__);
+ int err, i, n = 20;
+
+ err = reftable_new_stack(&st, dir, cfg);
+ EXPECT_ERR(err);
+
+ for (i = 0; i <= n; i++) {
+ struct reftable_ref_record ref = {
+ .update_index = reftable_stack_next_update_index(st),
+ .value_type = REFTABLE_REF_SYMREF,
+ .value.symref = "master",
+ };
+
+ /*
+ * Disable auto-compaction for all but the last runs. Like this
+ * we can ensure that we indeed honor this setting and have
+ * better control over when exactly auto compaction runs.
+ */
+ st->disable_auto_compact = i != n;
+
+ strbuf_reset(&refname);
+ strbuf_addf(&refname, "branch-%04d", i);
+ ref.refname = refname.buf;
+
+ err = reftable_stack_add(st, &write_test_ref, &ref);
+ EXPECT_ERR(err);
+
+ /*
+ * The stack length should grow continuously for all runs where
+ * auto compaction is disabled. When enabled, we should merge
+ * all tables in the stack.
+ */
+ if (i != n)
+ EXPECT(st->merged->stack_len == i + 1);
+ else
+ EXPECT(st->merged->stack_len == 1);
+ }
+
+ reftable_stack_destroy(st);
+ strbuf_release(&refname);
+ clear_dir(dir);
+}
+
static void test_reftable_stack_compaction_concurrent(void)
{
struct reftable_write_options cfg = { 0 };
@@ -960,6 +1008,7 @@ int stack_test_main(int argc, const char *argv[])
RUN_TEST(test_reftable_stack_add);
RUN_TEST(test_reftable_stack_add_one);
RUN_TEST(test_reftable_stack_auto_compaction);
+ RUN_TEST(test_reftable_stack_add_performs_auto_compaction);
RUN_TEST(test_reftable_stack_compaction_concurrent);
RUN_TEST(test_reftable_stack_compaction_concurrent_clean);
RUN_TEST(test_reftable_stack_hash_id);
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 05/11] reftable/stack: perform auto-compaction with transactional interface
From: Patrick Steinhardt @ 2023-12-11 9:07 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Jonathan Nieder, Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1702285387.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4283 bytes --]
Whenever updating references or reflog entries in the reftable stack, we
need to add a new table to the stack, thus growing the stack's length by
one. The stack can grow to become quite long rather quickly, leading to
performance issues when trying to read records. But besides performance
issues, this can also lead to exhaustion of file descriptors very
rapidly as every single table requires a separate descriptor when
opening the stack.
While git-pack-refs(1) fixes this issue for us by merging the tables, it
runs too irregularly to keep the length of the stack within reasonable
limits. This is why the reftable stack has an auto-compaction mechanism:
`reftable_stack_add()` will call `reftable_stack_auto_compact()` after
its added the new table, which will auto-compact the stack as required.
But while this logic works alright for `reftable_stack_add()`, we do not
do the same in `reftable_addition_commit()`, which is the transactional
equivalent to the former function that allows us to write multiple
updates to the stack atomically. Consequentially, we will easily run
into file descriptor exhaustion in code paths that use many separate
transactions like e.g. non-atomic fetches.
Fix this issue by calling `reftable_stack_auto_compact()`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/stack.c | 6 +++++
reftable/stack_test.c | 56 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 62 insertions(+)
diff --git a/reftable/stack.c b/reftable/stack.c
index f0cadad490..f5d18a842a 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -584,6 +584,12 @@ int reftable_addition_commit(struct reftable_addition *add)
add->new_tables_len = 0;
err = reftable_stack_reload(add->stack);
+ if (err)
+ goto done;
+
+ if (!add->stack->disable_auto_compact)
+ err = reftable_stack_auto_compact(add->stack);
+
done:
reftable_addition_close(add);
return err;
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 52b4dc3b14..14a3fc11ee 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -289,6 +289,61 @@ static void test_reftable_stack_transaction_api(void)
clear_dir(dir);
}
+static void test_reftable_stack_transaction_api_performs_auto_compaction(void)
+{
+ char *dir = get_tmp_dir(__LINE__);
+ struct reftable_write_options cfg = {0};
+ struct reftable_addition *add = NULL;
+ struct reftable_stack *st = NULL;
+ int i, n = 20, err;
+
+ err = reftable_new_stack(&st, dir, cfg);
+ EXPECT_ERR(err);
+
+ for (i = 0; i <= n; i++) {
+ struct reftable_ref_record ref = {
+ .update_index = reftable_stack_next_update_index(st),
+ .value_type = REFTABLE_REF_SYMREF,
+ .value.symref = "master",
+ };
+ char name[100];
+
+ snprintf(name, sizeof(name), "branch%04d", i);
+ ref.refname = name;
+
+ /*
+ * Disable auto-compaction for all but the last runs. Like this
+ * we can ensure that we indeed honor this setting and have
+ * better control over when exactly auto compaction runs.
+ */
+ st->disable_auto_compact = i != n;
+
+ err = reftable_stack_new_addition(&add, st);
+ EXPECT_ERR(err);
+
+ err = reftable_addition_add(add, &write_test_ref, &ref);
+ EXPECT_ERR(err);
+
+ err = reftable_addition_commit(add);
+ EXPECT_ERR(err);
+
+ reftable_addition_destroy(add);
+
+ /*
+ * The stack length should grow continuously for all runs where
+ * auto compaction is disabled. When enabled, we should merge
+ * all tables in the stack.
+ */
+ if (i != n)
+ EXPECT(st->merged->stack_len == i + 1);
+ else
+ EXPECT(st->merged->stack_len == 1);
+ }
+
+ reftable_stack_destroy(st);
+ clear_dir(dir);
+}
+
static void test_reftable_stack_validate_refname(void)
{
struct reftable_write_options cfg = { 0 };
@@ -1016,6 +1071,7 @@ int stack_test_main(int argc, const char *argv[])
RUN_TEST(test_reftable_stack_log_normalize);
RUN_TEST(test_reftable_stack_tombstone);
RUN_TEST(test_reftable_stack_transaction_api);
+ RUN_TEST(test_reftable_stack_transaction_api_performs_auto_compaction);
RUN_TEST(test_reftable_stack_update_index_check);
RUN_TEST(test_reftable_stack_uptodate);
RUN_TEST(test_reftable_stack_validate_refname);
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 06/11] reftable/stack: reuse buffers when reloading stack
From: Patrick Steinhardt @ 2023-12-11 9:07 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Jonathan Nieder, Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1702285387.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2436 bytes --]
In `reftable_stack_reload_once()` we iterate over all the tables added
to the stack in order to figure out whether any of the tables needs to
be reloaded. We use a set of buffers in this context to compute the
paths of these tables, but discard those buffers on every iteration.
This is quite wasteful given that we do not need to transfer ownership
of the allocated buffer outside of the loop.
Refactor the code to instead reuse the buffers to reduce the number of
allocations we need to do. Note that we do not have to manually reset
the buffer because `stack_filename()` does this for us already.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/stack.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index f5d18a842a..2dd2373360 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -204,6 +204,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,
reftable_calloc(sizeof(struct reftable_table) * names_len);
int new_readers_len = 0;
struct reftable_merged_table *new_merged = NULL;
+ struct strbuf table_path = STRBUF_INIT;
int i;
while (*names) {
@@ -223,13 +224,10 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,
if (!rd) {
struct reftable_block_source src = { NULL };
- struct strbuf table_path = STRBUF_INIT;
stack_filename(&table_path, st, name);
err = reftable_block_source_from_file(&src,
table_path.buf);
- strbuf_release(&table_path);
-
if (err < 0)
goto done;
@@ -267,16 +265,13 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,
for (i = 0; i < cur_len; i++) {
if (cur[i]) {
const char *name = reader_name(cur[i]);
- struct strbuf filename = STRBUF_INIT;
- stack_filename(&filename, st, name);
+ stack_filename(&table_path, st, name);
reader_close(cur[i]);
reftable_reader_free(cur[i]);
/* On Windows, can only unlink after closing. */
- unlink(filename.buf);
-
- strbuf_release(&filename);
+ unlink(table_path.buf);
}
}
@@ -288,6 +283,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,
reftable_free(new_readers);
reftable_free(new_tables);
reftable_free(cur);
+ strbuf_release(&table_path);
return err;
}
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 07/11] reftable/stack: fix stale lock when dying
From: Patrick Steinhardt @ 2023-12-11 9:07 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Jonathan Nieder, Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1702285387.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4525 bytes --]
When starting a transaction via `reftable_stack_init_addition()`, we
create a lockfile for the reftable stack itself which we'll write the
new list of tables to. But if we terminate abnormally e.g. via a call to
`die()`, then we do not remove the lockfile. Subsequent executions of
Git which try to modify references will thus fail with an out-of-date
error.
Fix this bug by registering the lock as a `struct tempfile`, which
ensures automatic cleanup for us.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/stack.c | 47 +++++++++++++++--------------------------------
1 file changed, 15 insertions(+), 32 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index 2dd2373360..0c235724e2 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -17,6 +17,8 @@ license that can be found in the LICENSE file or at
#include "reftable-merged.h"
#include "writer.h"
+#include "tempfile.h"
+
static int stack_try_add(struct reftable_stack *st,
int (*write_table)(struct reftable_writer *wr,
void *arg),
@@ -440,8 +442,7 @@ static void format_name(struct strbuf *dest, uint64_t min, uint64_t max)
}
struct reftable_addition {
- int lock_file_fd;
- struct strbuf lock_file_name;
+ struct tempfile *lock_file;
struct reftable_stack *stack;
char **new_tables;
@@ -449,24 +450,19 @@ struct reftable_addition {
uint64_t next_update_index;
};
-#define REFTABLE_ADDITION_INIT \
- { \
- .lock_file_name = STRBUF_INIT \
- }
+#define REFTABLE_ADDITION_INIT {0}
static int reftable_stack_init_addition(struct reftable_addition *add,
struct reftable_stack *st)
{
+ struct strbuf lock_file_name = STRBUF_INIT;
int err = 0;
add->stack = st;
- strbuf_reset(&add->lock_file_name);
- strbuf_addstr(&add->lock_file_name, st->list_file);
- strbuf_addstr(&add->lock_file_name, ".lock");
+ strbuf_addf(&lock_file_name, "%s.lock", st->list_file);
- add->lock_file_fd = open(add->lock_file_name.buf,
- O_EXCL | O_CREAT | O_WRONLY, 0666);
- if (add->lock_file_fd < 0) {
+ add->lock_file = create_tempfile(lock_file_name.buf);
+ if (!add->lock_file) {
if (errno == EEXIST) {
err = REFTABLE_LOCK_ERROR;
} else {
@@ -475,7 +471,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
goto done;
}
if (st->config.default_permissions) {
- if (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) {
+ if (chmod(add->lock_file->filename.buf, st->config.default_permissions) < 0) {
err = REFTABLE_IO_ERROR;
goto done;
}
@@ -495,6 +491,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
if (err) {
reftable_addition_close(add);
}
+ strbuf_release(&lock_file_name);
return err;
}
@@ -512,15 +509,7 @@ static void reftable_addition_close(struct reftable_addition *add)
add->new_tables = NULL;
add->new_tables_len = 0;
- if (add->lock_file_fd > 0) {
- close(add->lock_file_fd);
- add->lock_file_fd = 0;
- }
- if (add->lock_file_name.len > 0) {
- unlink(add->lock_file_name.buf);
- strbuf_release(&add->lock_file_name);
- }
-
+ delete_tempfile(&add->lock_file);
strbuf_release(&nm);
}
@@ -536,8 +525,10 @@ void reftable_addition_destroy(struct reftable_addition *add)
int reftable_addition_commit(struct reftable_addition *add)
{
struct strbuf table_list = STRBUF_INIT;
+ int lock_file_fd = get_tempfile_fd(add->lock_file);
int i = 0;
int err = 0;
+
if (add->new_tables_len == 0)
goto done;
@@ -550,28 +541,20 @@ int reftable_addition_commit(struct reftable_addition *add)
strbuf_addstr(&table_list, "\n");
}
- err = write_in_full(add->lock_file_fd, table_list.buf, table_list.len);
+ err = write_in_full(lock_file_fd, table_list.buf, table_list.len);
strbuf_release(&table_list);
if (err < 0) {
err = REFTABLE_IO_ERROR;
goto done;
}
- err = close(add->lock_file_fd);
- add->lock_file_fd = 0;
- if (err < 0) {
- err = REFTABLE_IO_ERROR;
- goto done;
- }
-
- err = rename(add->lock_file_name.buf, add->stack->list_file);
+ err = rename_tempfile(&add->lock_file, add->stack->list_file);
if (err < 0) {
err = REFTABLE_IO_ERROR;
goto done;
}
/* success, no more state to clean up. */
- strbuf_release(&add->lock_file_name);
for (i = 0; i < add->new_tables_len; i++) {
reftable_free(add->new_tables[i]);
}
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 08/11] reftable/stack: fix use of unseeded randomness
From: Patrick Steinhardt @ 2023-12-11 9:07 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Jonathan Nieder, Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1702285387.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2117 bytes --]
When writing a new reftable stack, Git will first create the stack with
a random suffix so that concurrent updates will not try to write to the
same file. This random suffix is computed via a call to rand(3P). But we
never seed the function via srand(3P), which means that the suffix is in
fact always the same.
Fix this bug by using `git_rand()` instead, which does not need to be
initialized. While this function is likely going to be slower depending
on the platform, this slowness should not matter in practice as we only
use it when writing a new reftable stack.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/readwrite_test.c | 6 +++---
reftable/stack.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c
index 469ab79a5a..278663f22d 100644
--- a/reftable/readwrite_test.c
+++ b/reftable/readwrite_test.c
@@ -141,8 +141,8 @@ static void test_log_buffer_size(void)
*/
uint8_t hash1[GIT_SHA1_RAWSZ], hash2[GIT_SHA1_RAWSZ];
for (i = 0; i < GIT_SHA1_RAWSZ; i++) {
- hash1[i] = (uint8_t)(rand() % 256);
- hash2[i] = (uint8_t)(rand() % 256);
+ hash1[i] = (uint8_t)(git_rand() % 256);
+ hash2[i] = (uint8_t)(git_rand() % 256);
}
log.value.update.old_hash = hash1;
log.value.update.new_hash = hash2;
@@ -320,7 +320,7 @@ static void test_log_zlib_corruption(void)
};
for (i = 0; i < sizeof(message) - 1; i++)
- message[i] = (uint8_t)(rand() % 64 + ' ');
+ message[i] = (uint8_t)(git_rand() % 64 + ' ');
reftable_writer_set_limits(w, 1, 1);
diff --git a/reftable/stack.c b/reftable/stack.c
index 0c235724e2..16bab82063 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -434,7 +434,7 @@ int reftable_stack_add(struct reftable_stack *st,
static void format_name(struct strbuf *dest, uint64_t min, uint64_t max)
{
char buf[100];
- uint32_t rnd = (uint32_t)rand();
+ uint32_t rnd = (uint32_t)git_rand();
snprintf(buf, sizeof(buf), "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x",
min, max, rnd);
strbuf_reset(dest);
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 09/11] reftable/merged: reuse buffer to compute record keys
From: Patrick Steinhardt @ 2023-12-11 9:08 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Jonathan Nieder, Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1702285387.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3361 bytes --]
When iterating over entries in the merged iterator's queue, we compute
the key of each of the entries and write it into a buffer. We do not
reuse the buffer though and thus re-allocate it on every iteration,
which is wasteful given that we never transfer ownership of the
allocated bytes outside of the loop.
Refactor the code to reuse the buffer. This also fixes a potential
memory leak when `merged_iter_advance_subiter()` returns an error.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/merged.c | 31 ++++++++++++++++---------------
reftable/merged.h | 2 ++
2 files changed, 18 insertions(+), 15 deletions(-)
diff --git a/reftable/merged.c b/reftable/merged.c
index 5ded470c08..556bb5c556 100644
--- a/reftable/merged.c
+++ b/reftable/merged.c
@@ -52,6 +52,8 @@ static void merged_iter_close(void *p)
reftable_iterator_destroy(&mi->stack[i]);
}
reftable_free(mi->stack);
+ strbuf_release(&mi->key);
+ strbuf_release(&mi->entry_key);
}
static int merged_iter_advance_nonnull_subiter(struct merged_iter *mi,
@@ -85,7 +87,6 @@ static int merged_iter_advance_subiter(struct merged_iter *mi, size_t idx)
static int merged_iter_next_entry(struct merged_iter *mi,
struct reftable_record *rec)
{
- struct strbuf entry_key = STRBUF_INIT;
struct pq_entry entry = { 0 };
int err = 0;
@@ -105,33 +106,31 @@ static int merged_iter_next_entry(struct merged_iter *mi,
such a deployment, the loop below must be changed to collect all
entries for the same key, and return new the newest one.
*/
- reftable_record_key(&entry.rec, &entry_key);
+ reftable_record_key(&entry.rec, &mi->entry_key);
while (!merged_iter_pqueue_is_empty(mi->pq)) {
struct pq_entry top = merged_iter_pqueue_top(mi->pq);
- struct strbuf k = STRBUF_INIT;
- int err = 0, cmp = 0;
+ int cmp = 0;
- reftable_record_key(&top.rec, &k);
+ reftable_record_key(&top.rec, &mi->key);
- cmp = strbuf_cmp(&k, &entry_key);
- strbuf_release(&k);
-
- if (cmp > 0) {
+ cmp = strbuf_cmp(&mi->key, &mi->entry_key);
+ if (cmp > 0)
break;
- }
merged_iter_pqueue_remove(&mi->pq);
err = merged_iter_advance_subiter(mi, top.index);
- if (err < 0) {
- return err;
- }
+ if (err < 0)
+ goto done;
reftable_record_release(&top.rec);
}
reftable_record_copy_from(rec, &entry.rec, hash_size(mi->hash_id));
+
+done:
reftable_record_release(&entry.rec);
- strbuf_release(&entry_key);
- return 0;
+ strbuf_release(&mi->entry_key);
+ strbuf_release(&mi->key);
+ return err;
}
static int merged_iter_next(struct merged_iter *mi, struct reftable_record *rec)
@@ -248,6 +247,8 @@ static int merged_table_seek_record(struct reftable_merged_table *mt,
.typ = reftable_record_type(rec),
.hash_id = mt->hash_id,
.suppress_deletions = mt->suppress_deletions,
+ .key = STRBUF_INIT,
+ .entry_key = STRBUF_INIT,
};
int n = 0;
int err = 0;
diff --git a/reftable/merged.h b/reftable/merged.h
index 7d9f95d27e..d5b39dfe7f 100644
--- a/reftable/merged.h
+++ b/reftable/merged.h
@@ -31,6 +31,8 @@ struct merged_iter {
uint8_t typ;
int suppress_deletions;
struct merged_iter_pqueue pq;
+ struct strbuf key;
+ struct strbuf entry_key;
};
void merged_table_release(struct reftable_merged_table *mt);
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 10/11] reftable/block: introduce macro to initialize `struct block_iter`
From: Patrick Steinhardt @ 2023-12-11 9:08 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Jonathan Nieder, Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1702285387.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3434 bytes --]
There are a bunch of locations where we initialize members of `struct
block_iter`, which makes it harder than necessary to expand this struct
to have additional members. Unify the logic via a new `BLOCK_ITER_INIT`
macro that initializes all members.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 4 +---
reftable/block.h | 4 ++++
reftable/block_test.c | 4 ++--
reftable/iter.h | 8 ++++----
reftable/reader.c | 7 +++----
5 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/reftable/block.c b/reftable/block.c
index 34d4d07369..8c6a8c77fc 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -389,9 +389,7 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,
struct reftable_record rec = reftable_new_record(block_reader_type(br));
struct strbuf key = STRBUF_INIT;
int err = 0;
- struct block_iter next = {
- .last_key = STRBUF_INIT,
- };
+ struct block_iter next = BLOCK_ITER_INIT;
int i = binsearch(br->restart_count, &restart_key_less, &args);
if (args.error) {
diff --git a/reftable/block.h b/reftable/block.h
index 87c77539b5..51699af233 100644
--- a/reftable/block.h
+++ b/reftable/block.h
@@ -86,6 +86,10 @@ struct block_iter {
struct strbuf last_key;
};
+#define BLOCK_ITER_INIT { \
+ .last_key = STRBUF_INIT, \
+}
+
/* initializes a block reader. */
int block_reader_init(struct block_reader *br, struct reftable_block *bl,
uint32_t header_off, uint32_t table_block_size,
diff --git a/reftable/block_test.c b/reftable/block_test.c
index cb88af4a56..c00bbc8aed 100644
--- a/reftable/block_test.c
+++ b/reftable/block_test.c
@@ -32,7 +32,7 @@ static void test_block_read_write(void)
int i = 0;
int n;
struct block_reader br = { 0 };
- struct block_iter it = { .last_key = STRBUF_INIT };
+ struct block_iter it = BLOCK_ITER_INIT;
int j = 0;
struct strbuf want = STRBUF_INIT;
@@ -87,7 +87,7 @@ static void test_block_read_write(void)
block_iter_close(&it);
for (i = 0; i < N; i++) {
- struct block_iter it = { .last_key = STRBUF_INIT };
+ struct block_iter it = BLOCK_ITER_INIT;
strbuf_reset(&want);
strbuf_addstr(&want, names[i]);
diff --git a/reftable/iter.h b/reftable/iter.h
index 09eb0cbfa5..47d67d84df 100644
--- a/reftable/iter.h
+++ b/reftable/iter.h
@@ -53,10 +53,10 @@ struct indexed_table_ref_iter {
int is_finished;
};
-#define INDEXED_TABLE_REF_ITER_INIT \
- { \
- .cur = { .last_key = STRBUF_INIT }, .oid = STRBUF_INIT, \
- }
+#define INDEXED_TABLE_REF_ITER_INIT { \
+ .cur = BLOCK_ITER_INIT, \
+ .oid = STRBUF_INIT, \
+}
void iterator_from_indexed_table_ref_iter(struct reftable_iterator *it,
struct indexed_table_ref_iter *itr);
diff --git a/reftable/reader.c b/reftable/reader.c
index b4db23ce18..9de64f50b4 100644
--- a/reftable/reader.c
+++ b/reftable/reader.c
@@ -224,10 +224,9 @@ struct table_iter {
struct block_iter bi;
int is_finished;
};
-#define TABLE_ITER_INIT \
- { \
- .bi = {.last_key = STRBUF_INIT } \
- }
+#define TABLE_ITER_INIT { \
+ .bi = BLOCK_ITER_INIT \
+}
static void table_iter_copy_from(struct table_iter *dest,
struct table_iter *src)
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v3 11/11] reftable/block: reuse buffer to compute record keys
From: Patrick Steinhardt @ 2023-12-11 9:08 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys, Jonathan Nieder, Taylor Blau, Eric Sunshine
In-Reply-To: <cover.1702285387.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3135 bytes --]
When iterating over entries in the block iterator we compute the key of
each of the entries and write it into a buffer. We do not reuse the
buffer though and thus re-allocate it on every iteration, which is
wasteful.
Refactor the code to reuse the buffer.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 19 ++++++++-----------
reftable/block.h | 2 ++
2 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/reftable/block.c b/reftable/block.c
index 8c6a8c77fc..1df3d8a0f0 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -323,30 +323,28 @@ int block_iter_next(struct block_iter *it, struct reftable_record *rec)
.len = it->br->block_len - it->next_off,
};
struct string_view start = in;
- struct strbuf key = STRBUF_INIT;
uint8_t extra = 0;
int n = 0;
if (it->next_off >= it->br->block_len)
return 1;
- n = reftable_decode_key(&key, &extra, it->last_key, in);
+ n = reftable_decode_key(&it->key, &extra, it->last_key, in);
if (n < 0)
return -1;
- if (!key.len)
+ if (!it->key.len)
return REFTABLE_FORMAT_ERROR;
string_view_consume(&in, n);
- n = reftable_record_decode(rec, key, extra, in, it->br->hash_size);
+ n = reftable_record_decode(rec, it->key, extra, in, it->br->hash_size);
if (n < 0)
return -1;
string_view_consume(&in, n);
strbuf_reset(&it->last_key);
- strbuf_addbuf(&it->last_key, &key);
+ strbuf_addbuf(&it->last_key, &it->key);
it->next_off += start.len - in.len;
- strbuf_release(&key);
return 0;
}
@@ -377,6 +375,7 @@ int block_iter_seek(struct block_iter *it, struct strbuf *want)
void block_iter_close(struct block_iter *it)
{
strbuf_release(&it->last_key);
+ strbuf_release(&it->key);
}
int block_reader_seek(struct block_reader *br, struct block_iter *it,
@@ -387,7 +386,6 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,
.r = br,
};
struct reftable_record rec = reftable_new_record(block_reader_type(br));
- struct strbuf key = STRBUF_INIT;
int err = 0;
struct block_iter next = BLOCK_ITER_INIT;
@@ -414,8 +412,8 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,
if (err < 0)
goto done;
- reftable_record_key(&rec, &key);
- if (err > 0 || strbuf_cmp(&key, want) >= 0) {
+ reftable_record_key(&rec, &it->key);
+ if (err > 0 || strbuf_cmp(&it->key, want) >= 0) {
err = 0;
goto done;
}
@@ -424,8 +422,7 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,
}
done:
- strbuf_release(&key);
- strbuf_release(&next.last_key);
+ block_iter_close(&next);
reftable_record_release(&rec);
return err;
diff --git a/reftable/block.h b/reftable/block.h
index 51699af233..17481e6331 100644
--- a/reftable/block.h
+++ b/reftable/block.h
@@ -84,10 +84,12 @@ struct block_iter {
/* key for last entry we read. */
struct strbuf last_key;
+ struct strbuf key;
};
#define BLOCK_ITER_INIT { \
.last_key = STRBUF_INIT, \
+ .key = STRBUF_INIT, \
}
/* initializes a block reader. */
--
2.43.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH v2 02/11] reftable: handle interrupted reads
From: Patrick Steinhardt @ 2023-12-11 9:08 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Han-Wen Nienhuys, Jonathan Nieder
In-Reply-To: <ZXOK2o0jTgLmxPWZ@nand.local>
[-- Attachment #1: Type: text/plain, Size: 658 bytes --]
On Fri, Dec 08, 2023 at 04:30:02PM -0500, Taylor Blau wrote:
> On Fri, Dec 08, 2023 at 03:53:02PM +0100, Patrick Steinhardt wrote:
> > There are calls to pread(3P) and read(3P) where we don't properly handle
> > interrupts. Convert them to use `pread_in_full()` and `read_in_full()`,
>
> Just checking... do you mean "interrupt" in the kernel sense? Or are you
> referring to the possibility of short reads/writes (in later patches)?
Both. The callsites I'm converting are explicitly checking that they get
the exact number of requested bytes. That means that we'll have to loop
both around EINTR/EAGAIN, but also around short reads.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 04/11] reftable/stack: verify that `reftable_stack_add()` uses auto-compaction
From: Patrick Steinhardt @ 2023-12-11 9:08 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Taylor Blau, git, Han-Wen Nienhuys, Jonathan Nieder
In-Reply-To: <CAPig+cRGZvyhSs9=3-tkBKRZDjDUsb-VDs+dzOaZof__qyBjbA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1910 bytes --]
On Fri, Dec 08, 2023 at 06:46:33PM -0500, Eric Sunshine wrote:
> On Fri, Dec 8, 2023 at 4:35 PM Taylor Blau <me@ttaylorr.com> wrote:
> > On Fri, Dec 08, 2023 at 03:53:10PM +0100, Patrick Steinhardt wrote:
> > > +static void test_reftable_stack_add_performs_auto_compaction(void)
> > > +{
> > > + char name[100];
> > > + snprintf(name, sizeof(name), "branch%04d", i);
> > > + ref.refname = name;
> >
> > Is there a reason that we have to use snprintf() here and not a strbuf?
> >
> > I would have expected to see something like:
> >
> > struct strbuf buf = STRBUF_INIT;
> > /* ... */
> > strbuf_addf(&buf, "branch%04d", i);
> > ref.refname = strbuf_detach(&buf, NULL);
>
> If I'm reading the code correctly, this use of strbuf would leak each
> time through the loop.
>
> > I guess it doesn't matter too much, but I think if we can avoid using
> > snprintf(), it's worth doing. If we must use snprintf() here, we should
> > probably use Git's xsnprintf() instead.
>
> xstrfmt() from strbuf.h would be even simpler if the intention is to
> allocate a new string which will be freed later.
>
> In this case, though, assuming I understand the intent, I think the
> more common and safe idiom in this codebase is something like this:
>
> struct strbuf name = STRBUF_INIT;
> strbuf_addstr(&name, "branch");
> size_t len = name.len;
> for (...) {
> strbuf_setlen(&name, len);
> strbuf_addf(&name, "%04d", i);
> ref.refname = name.buf;
> ...
> }
> strbuf_release(&name);
Yeah, I'll convert this to use a `struct strbuf` instead. But instead of
tracking the length I'll just use a `strbuf_reset()` followed by
`strbuf_addf("branch-%04d")`. It's simpler to read and we don't need to
squeeze every last drop of performance out of this loop anyway.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 05/11] reftable/stack: perform auto-compaction with transactional interface
From: Patrick Steinhardt @ 2023-12-11 9:08 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Han-Wen Nienhuys, Jonathan Nieder
In-Reply-To: <ZXOVVWGyUtrGWYMB@nand.local>
[-- Attachment #1: Type: text/plain, Size: 546 bytes --]
On Fri, Dec 08, 2023 at 05:14:45PM -0500, Taylor Blau wrote:
> On Fri, Dec 08, 2023 at 03:53:14PM +0100, Patrick Steinhardt wrote:
> > [...] It can thus happen quite fast that the stack grows very long, which
> > results in performance issues when trying to read records.
>
> This sentence was a little confusing to read at first glance. Perhaps
> instead:
>
> The stack can grow to become quite long rather quickly, leading to
> performance issues when trying to read records.
Thanks, this reads better indeed.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 06/11] reftable/stack: reuse buffers when reloading stack
From: Patrick Steinhardt @ 2023-12-11 9:08 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Han-Wen Nienhuys, Jonathan Nieder
In-Reply-To: <ZXOV8TCqaH0xXRnS@nand.local>
[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]
On Fri, Dec 08, 2023 at 05:17:21PM -0500, Taylor Blau wrote:
> On Fri, Dec 08, 2023 at 03:53:18PM +0100, Patrick Steinhardt wrote:
> > In `reftable_stack_reload_once()` we iterate over all the tables added
> > to the stack in order to figure out whether any of the tables needs to
> > be reloaded. We use a set of buffers in this context to compute the
> > paths of these tables, but discard those buffers on every iteration.
> > This is quite wasteful given that we do not need to transfer ownership
> > of the allocated buffer outside of the loop.
> >
> > Refactor the code to instead reuse the buffers to reduce the number of
> > allocations we need to do.
>
> > @@ -267,16 +265,13 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,
> > for (i = 0; i < cur_len; i++) {
> > if (cur[i]) {
> > const char *name = reader_name(cur[i]);
> > - struct strbuf filename = STRBUF_INIT;
> > - stack_filename(&filename, st, name);
> > + stack_filename(&table_path, st, name);
>
> This initially caught me by surprise, but on closer inspection I agree
> that this is OK, since stack_filename() calls strbuf_reset() before
> adjusting the buffer contents.
>
> (As a side-note, I do find the side-effect of stack_filename() to be a
> little surprising, but that's not the fault of this series and not worth
> changing here.)
Agreed, I also found this to be a bit confusing at first. I'll amend the
commit message with "Note that we do not have to manually reset the
buffer because `stack_filename()` does this for us already." to help
future readers.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 07/11] reftable/stack: fix stale lock when dying
From: Patrick Steinhardt @ 2023-12-11 9:08 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Han-Wen Nienhuys, Jonathan Nieder
In-Reply-To: <ZXOXpQstse8CdI7J@nand.local>
[-- Attachment #1: Type: text/plain, Size: 1471 bytes --]
On Fri, Dec 08, 2023 at 05:24:37PM -0500, Taylor Blau wrote:
> On Fri, Dec 08, 2023 at 03:53:23PM +0100, Patrick Steinhardt wrote:
> > When starting a transaction via `reftable_stack_init_addition()`, we
> > create a lockfile for the reftable stack itself which we'll write the
> > new list of tables to. But if we terminate abnormally e.g. via a call to
> > `die()`, then we do not remove the lockfile. Subsequent executions of
> > Git which try to modify references will thus fail with an out-of-date
> > error.
> >
> > Fix this bug by registering the lock as a `struct tempfile`, which
> > ensures automatic cleanup for us.
>
> Makes sense.
>
> > @@ -475,7 +471,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
> > goto done;
> > }
> > if (st->config.default_permissions) {
> > - if (chmod(add->lock_file_name.buf, st->config.default_permissions) < 0) {
> > + if (chmod(lock_file_name.buf, st->config.default_permissions) < 0) {
>
> Hmm. Would we want to use add->lock_file->filename.buf here instead? I
> don't think that it matters (other than that the lockfile's pathname is
> absolute). But it arguably makes it clearer to readers that we're
> touching calling chmod() on the lockfile here, and hardens us against
> the contents of the temporary strbuf changing.
It doesn't make much of a difference, but I can see how this might make
things a bit clearer to the reader. Will change.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 04/11] reftable/stack: verify that `reftable_stack_add()` uses auto-compaction
From: Eric Sunshine @ 2023-12-11 9:36 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Taylor Blau, git, Han-Wen Nienhuys, Jonathan Nieder
In-Reply-To: <ZXbRkOiD80zT7tC5@tanuki>
On Mon, Dec 11, 2023 at 4:08 AM Patrick Steinhardt <ps@pks.im> wrote:
> On Fri, Dec 08, 2023 at 06:46:33PM -0500, Eric Sunshine wrote:
> > In this case, though, assuming I understand the intent, I think the
> > more common and safe idiom in this codebase is something like this:
> >
> > struct strbuf name = STRBUF_INIT;
> > strbuf_addstr(&name, "branch");
> > size_t len = name.len;
> > for (...) {
> > strbuf_setlen(&name, len);
> > strbuf_addf(&name, "%04d", i);
> > ref.refname = name.buf;
> > ...
> > }
> > strbuf_release(&name);
>
> Yeah, I'll convert this to use a `struct strbuf` instead. But instead of
> tracking the length I'll just use a `strbuf_reset()` followed by
> `strbuf_addf("branch-%04d")`. It's simpler to read and we don't need to
> squeeze every last drop of performance out of this loop anyway.
Sounds perfectly reasonable to me, and I agree with the reasoning,
especially the lower cognitive load with strbuf_reset(). Thanks.
^ permalink raw reply
* Re: [PATCH 0/7] clone: fix init of refdb with wrong object format
From: Patrick Steinhardt @ 2023-12-11 11:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, brian m. carlson
In-Reply-To: <xmqq7clmn3w1.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 3953 bytes --]
On Sat, Dec 09, 2023 at 07:16:30PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > While at it I noticed that this actually fixes a bug with bundle URIs
> > when the object formats diverge in this way.
> > ...
> > This patch series is actually the last incompatibility for the reftable
> > backend that I have found. All tests except for the files-backend
> > specific ones pass now with the current state I have at [1], which is
> > currently at e6f2f592b7 (t: skip tests which are incompatible with
> > reftable, 2023-11-24)
>
> An existing test
>
> $ make && cd t && GIT_TEST_DEFAULT_HASH=sha256 sh t5550-http-fetch-dumb.sh
>
> passes with vanilla Git 2.43, but with these patches applied, it
> fails the "7 - empty dumb HTTP" step.
Indeed -- now that the GitLab CI definitions have landed in your master
branch I can also see this failure as part of our CI job [1]. And with
the NixOS httpd refactorings I'm actually able to run these tests in the
first place. Really happy to see that things come together like this, as
it means that we'll detect such issues before we send the series to the
mailing list from now on.
Anyway, regarding the test itself. I think the refactorings in this
patch series uncover a preexisting and already-known issue with empty
repositories when using the dumb HTTP protocol: there is no proper way
to know about the hash function that the remote repository uses [2].
Before my refactorings we used to fall back to the local default hash
format with which we've already initialized the repository, which is
wrong. Now we use the hash format we detected via the remote, which we
cannot detect because the remote is empty and does not advertise the
hash function, so we fall back to SHA1 and thus also do the wrong thing.
The only correct thing here would be to use the actual hash function
that the remote repository uses, but we have no to do so. So we're kind
of stuck here and can only choose between two different wrong ways to
pick the hash function.
We can restore the previous wrong behaviour by honoring GIT_DEFAULT_HASH
in git-remote-curl(1) in the case where we do not have a repository set
up yet. So something similar in spirit to the following:
```
diff --git a/remote-curl.c b/remote-curl.c
index fc29757b65..7e97c9c2e9 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -27,6 +27,7 @@
static struct remote *remote;
/* always ends with a trailing slash */
static struct strbuf url = STRBUF_INIT;
+static int nongit;
struct options {
int verbosity;
@@ -275,8 +276,30 @@ static const struct git_hash_algo *detect_hash_algo(struct discovery *heads)
{
const char *p = memchr(heads->buf, '\t', heads->len);
int algo;
- if (!p)
- return the_hash_algo;
+
+ if (!p) {
+ const char *env;
+
+ if (!nongit)
+ return the_hash_algo;
+
+ /*
+ * In case the remote neither advertises the hash format nor
+ * any references we have no way to detect the correct hash
+ * format. We can thus only guess what the remote is using,
+ * where the best guess is to fall back to the default hash.
+ */
+ env = getenv("GIT_DEFAULT_HASH");
+ if (env) {
+ algo = hash_algo_by_name(env);
+ if (algo == GIT_HASH_UNKNOWN)
+ die(_("unknown hash algorithm '%s'"), env);
+ } else {
+ algo = GIT_HASH_SHA1;
+ }
+
+ return &hash_algos[algo];
+ }
algo = hash_algo_by_length((p - heads->buf) / 2);
if (algo == GIT_HASH_UNKNOWN)
@@ -1521,7 +1544,6 @@ static int stateless_connect(const char *service_name)
int cmd_main(int argc, const char **argv)
{
struct strbuf buf = STRBUF_INIT;
- int nongit;
int ret = 1;
setup_git_directory_gently(&nongit);
```
+Cc brian, as he's the author of [2].
Patrick
[1]: https://gitlab.com/gitlab-org/git/-/jobs/5723052108
[2]: ac093d0790 (remote-curl: detect algorithm for dumb HTTP by size, 2020-06-19)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH 1/7] setup: extract function to create the refdb
From: Patrick Steinhardt @ 2023-12-11 11:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, git
In-Reply-To: <xmqq1qbw1f0j.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1787 bytes --]
On Sat, Dec 09, 2023 at 07:54:52AM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > On Wed, Dec 06, 2023 at 10:10:37PM +0100, Karthik Nayak wrote:
> >> On Wed, Dec 6, 2023 at 1:40 PM Patrick Steinhardt <ps@pks.im> wrote:
> >> > +static void create_reference_database(const char *initial_branch, int quiet)
> >> > +{
> >> > + struct strbuf err = STRBUF_INIT;
> >> > + int reinit = is_reinit();
> >> > +
> >> > + /*
> >> > + * We need to create a "refs" dir in any case so that older
> >> > + * versions of git can tell that this is a repository.
> >> > + */
> >>
> >> How does this work though, even if an earlier version of git can tell
> >> that this is a repository, it still won't be able to read the reftable
> >> backend. In that sense, what do we achieve here?
> >
> > This is a good question, and there is related ongoing discussion about
> > this topic in the thread starting at [1]. There are a few benefits to
> > letting clients discover such repos even if they don't understand the
> > new reference backend format:
> >
> > - They know to stop walking up the parent-directory chain. Otherwise a
> > client might end up detecting a Git repository in the parent dir.
> >
> > - The user gets a proper error message why the repository cannot be
> > accessed. Instead of failing to detect the repository altogether we
> > instead say that we don't understand the "extensions.refFormat"
> > extension.
>
> Yup, both are very good reasons. Would it help to sneak a condensed
> version of it in the in-code comment, perhaps?
Sure, let's do so. I failed to condense this meaningfully, but hope that
the result will be okay regardless of that.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ 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