* Clarify the meaning of "character" in the documentation
From: Manlio Perillo @ 2024-03-05 8:43 UTC (permalink / raw)
To: git
The term "character" is confusing: does it mean 7bit/ASCII character
or Unicode Code Point?
As an example, with
git config --add core.commentChar • // Bullet (U+2022)
git does not complain, but it is rejected later.
A counter example is using UTF-8 with "user.name", where it is handled
correctly.
I sent this email after reading the documentation of "git diff
--color-moved=blocks, where the text says:
> Blocks of moved text of at least 20 alphanumeric characters are detected greedily.
In this case it is not clear if the number of characters are counted
as UTF-8 or normal 8bit bytes.
Thanks
Manlio Perillo
^ permalink raw reply
* Re: Clarify the meaning of "character" in the documentation
From: Kristoffer Haugsbakk @ 2024-03-05 9:00 UTC (permalink / raw)
To: Manlio Perillo; +Cc: git
In-Reply-To: <CAAToxAEE-2B_8Wqrquk1peOnqTbOpV_8KZmsJ2dgk-mfCZXHiA@mail.gmail.com>
On Tue, Mar 5, 2024, at 09:43, Manlio Perillo wrote:
> The term "character" is confusing: does it mean 7bit/ASCII character
> or Unicode Code Point?
IMO it should say “ASCII” in contexts where it is restricted to
that. Otherwise UTF-8 can be assumed since git(1) handles that well.
> As an example, with
> git config --add core.commentChar • // Bullet (U+2022)
> git does not complain, but it is rejected later.
I think this is more about `git config --add` not doing any
validation. It just sets things. You can do `git config --add
core.commentChar 'ffd'` and get the same effect.
> A counter example is using UTF-8 with "user.name", where it is handled
> correctly.
Yep.
It will also handle UTF-8 in cross-systems setting, in my experience: if
you generate patches with git-format-patch(1) it will handle UTF-8 that
ends up in email headers correctly (it needs its own encoding).
It’s quite UTF-8 friendly.
> I sent this email after reading the documentation of "git diff
> --color-moved=blocks, where the text says:
>> Blocks of moved text of at least 20 alphanumeric characters are detected greedily.
>
> In this case it is not clear if the number of characters are counted
> as UTF-8 or normal 8bit bytes.
Alphanumeric characters (a-z and A-Z and 0-9) are ASCII. And one ASCII
char is represented using one byte in UTF-8. This already looks precise
to me.
I’ve never run into a case where git-diff(1) does not handle UTF-8. I
don’t even know if it really needs to “handle” it per se as opposed to
just treating it as opaque bytes. Maybe it matters for things like
whitespace and word-boundaries, I don’t know.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: What's cooking in git.git (Mar 2024, #01; Mon, 4)
From: Patrick Steinhardt @ 2024-03-05 10:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq4jdloc9a.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]
On Mon, Mar 04, 2024 at 02:36:33PM -0800, Junio C Hamano wrote:
> * ps/reftable-iteration-perf-part2 (2024-03-04) 13 commits
> - refs/reftable: precompute prefix length
> - reftable: allow inlining of a few functions
> - reftable/record: decode keys in place
> - reftable/record: reuse refname when copying
> - reftable/record: reuse refname when decoding
> - reftable/merged: avoid duplicate pqueue emptiness check
> - reftable/merged: circumvent pqueue with single subiter
> - reftable/merged: handle subiter cleanup on close only
> - reftable/merged: remove unnecessary null check for subiters
> - reftable/merged: make subiters own their records
> - reftable/merged: advance subiter on subsequent iteration
> - reftable/merged: make `merged_iter` structure private
> - reftable/pq: use `size_t` to track iterator index
>
> The code to iterate over refs with the reftable backend has seen
> some optimization.
>
> Will merge to 'next'?
> source: <cover.1709548907.git.ps@pks.im>
Yeah, I think this topic is ready to go.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/5] t3200: improve test style
From: Kristoffer Haugsbakk @ 2024-03-05 10:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqplw9lbav.fsf@gitster.g>
On Tue, Mar 5, 2024, at 02:25, Junio C Hamano wrote:
> Especially the change to use "-\EOF" to make them align better
> caused too many tests to be touched, but overall the result may have
> become much easier to follow. Good job.
I reckon that this can be worth doing now as long as no other topics in
`next` or `seen` happen to touch the same code. What do you think? I can
evict hunks if they happen to overlap with other in-flight topics.
>> -mv .git/config .git/config-saved
>> -
>> test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' '
>> + test_when_finished mv .git/config-saved .git/config &&
>> + mv .git/config .git/config-saved &&
>> git branch -m q q2 &&
>> git branch -m q2 q
>> '
>>
>> -mv .git/config-saved .git/config
>
> The above is a truly valuable clean-up.
>
> But I am not really sure if the paritcular condition is worth
> testing in the first place these days. No configuration file means
> we cannot even read the repository format version, and working under
> such a condition is quite a bad promise that we would rather not to
> having to keep. But that is an entirely different topic from what
> this patch is doing.
Okay. I could undo this change and remove the test in its own commit?
>
>> -git config branch.s/s.dummy Hello
>> -
>> -test_expect_success 'git branch -m s/s s should work when s/t is deleted' '
>> +test_expect_success '(setup) git branch -m s/s s should work when s/t is deleted' '
>> + git config branch.s/s.dummy Hello &&
>> git branch --create-reflog s/s &&
>> git reflog exists refs/heads/s/s &&
>> git branch --create-reflog s/t &&
>
> I do not know if the change of the title is warranted. It is doing
> its own test, not just setup. It may be merely donw for the side
> effect of making the step unskippable, but still ....
Sure, I’ll remove `(setup)`. The test name suggests that the test
depends on the previous one in any case.
^ permalink raw reply
* Re: [PATCH v3 3/5] advice: use backticks for code
From: Kristoffer Haugsbakk @ 2024-03-05 10:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqzfvdlfhp.fsf@gitster.g>
On Tue, Mar 5, 2024, at 00:54, Junio C Hamano wrote:
>> Use backticks for quoting code rather than single quotes.
>
> Good. Technically it does not have to be "code", but rather what
> the user would literally type from their keyboard verbatim, but
> "quoting code" is so concise way to describe, it probably is good
> enough hint for future developers who will find this commit via "git
> blame" and read "git show" to read this explanation.
I agree. Either works fine but “verbatim” is a more general term. I’ll
use that.
^ permalink raw reply
* Re: [PATCH v3 2/5] advice: make all entries stylistically consistent
From: Kristoffer Haugsbakk @ 2024-03-05 10:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq4jdlmu6q.fsf@gitster.g>
On Tue, Mar 5, 2024, at 00:52, Junio C Hamano wrote:
>> detachedHead::
>> - Advice shown when you used
>> + Shown when the user uses
>> linkgit:git-switch[1] or linkgit:git-checkout[1]
>> - to move to the detached HEAD state, to instruct how to
>> + to move to the detached HEAD state; instruct how to
>> create a local branch after the fact.
>
> I agree "Advice shown when" -> "Shown when" is a good change for
> brevity, but I do not think the other change is an improvement.
>
> This advice message is shown when the user does X, in order to
> instruct the user how to do Y after that. And "to instruct" is a
> common way to say the same thing as "in order to instruct".
Well argued. I’ll go back to the comma.
>> implicitIdentity::
>> - Advice on how to set your identity configuration when
>> - your information is guessed from the system username and
>> - domain name.
>> + Shown when the user's information is guessed from the
>> + system username and domain name: tell the user how to
>> + set their identity configuration.
>
> Should that be a colon? Stopping a half-sentence and connecting to
> another half-sentence is usually done with a semicolon (like you did
> in the new version of detachedHEAD above).
>
> Shown when ... and domain name, to tell the user how to set
> their identity configuration.
>
> perhaps? There may be other similar entries whose updated text uses
> colon followed by an imperative sentence, but I didn't look very
> carefully.
I’ll spoil it for you: there are a lot of colons. ;)
Good point. I’ll go over it again and probably use more semicolons
instead.
>> statusUoption::
>> - Advise to consider using the `-u` option to linkgit:git-status[1]
>> - when the command takes more than 2 seconds to enumerate untracked
>> - files.
>> + Shown when linkgit:git-status[1] takes more than 2
>> + seconds to enumerate untracked files: consider using the
>> + `-u` option.
>
> Earlier ones after a colon (or semicolon in detachedHEAD case), you
> gave an order to the advice message (e.g. "hey detachedHead advice,
> tell the user how to create a local branch"), but this one is giving
> an order to the end user, which feels inconsistent.
>
> I do not have a strong objection against giving an order to the
> advice message, as long as it is done consistently. If we did so,
> the part after the colon would start with "instruct the user ..." or
> "tell the user ..." and the like, and the gist of what this one
> would say would be "shown when it is taking too long: suggest the
> user to consider `-u`".
Yeah, I paused for a minute when writing that. I’ll change to “tell” or
something similar.
> FWIW, my earlier "in order to" took an approach that is different
> from either of the two "giving an order" approaches. I was trying
> to make the description explain what the message tries to do and/or
> why the message is given (e.g., "shown if it takes too long in order
> to suggest users to consider the -u option").
>
> Thanks.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions
From: shejialuo @ 2024-03-05 11:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Sunshine, Patrick Steinhardt
In-Reply-To: <xmqq7cihrjxj.fsf@gitster.g>
> Just for the next single-patch topic you'd work on, here below the
> three-dash line is where you may mention what's different between
> the previous iteration and this one, if you wanted to, instead of
> having a separate cover-letter message.
Thanks for your suggestions.
At last, Thank every reviewer for your dedicated comments which make me
learn a lot.
^ permalink raw reply
* Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD
From: Patrick Steinhardt @ 2024-03-05 11:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, git, Mike Hommey
In-Reply-To: <xmqq5xy2rmfy.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 3296 bytes --]
On Mon, Mar 04, 2024 at 08:28:17AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> > Now there is a very particular edge case in this situation: when
> >> > preparing an empty ref transacton, we end up returning whatever value
> >> > `read_ref_without_reload()` returned to the caller. Under normal
> >> > conditions this would be fine: "HEAD" should usually exist, and thus the
> >> > function would return `0`. But if "HEAD" doesn't exist, the function
> >> > returns a positive value which we end up returning to the caller.
> >> >
> >> > Fix this bug by resetting the return code to `0` and add a test.
>
> So this _will_ surface as a bug when the other change in the series
> is applied, but it nevertheless is worth fixing independently of the
> other one, because ...
>
> >> > @@ -821,6 +821,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
> >> > &head_referent, &head_type);
> >> > if (ret < 0)
> >> > goto done;
> >> > + ret = 0;
>
> ... after "if the refs->err records an error already, skip
> everything we do and return to the caller", we should take the
> ownership of what we return (which will be in "ret") from now on.
>
> So the current code uses "ret" as an uninitialized variable, even
> not technically so because it is "initialized" to "refs->err"
> upfront, and this is like a fix of uninitialized variable use.
The problem is a bit different. We call `read_ref_without_reload()` to
look up the "HEAD" ref, which will return a positive value in case the
ref wasn't found. This is customary in the reftable library: positive
error values indicate that an iter is over, and thus by extension that a
value was not found. It's fine though if the ref doesn't exist, and we
handle that case gracefully.
The only exception is when the transaction is also empty. In that case,
we skip the loop and thus end up not assigning to `ret` anymore. Thus,
the positive error code we still have in `ret` from the failed "HEAD"
lookup gets returned to the caller, which is wrong.
So it's not uninitialized, it rather is stale.
But yes, the bug _can_ be hit independently of the second patch in this
series. It's just really unlikely as a repo without "HEAD" is considered
to be broken anyway.
> >> So this is not really a problem in this function, it's more of that
> >> `refs.c:ref_transaction_prepare` checks if `ret` is non-zero.
> >
> > Well, yes. I'd claim that it is a problem in this function because it
> > returns positive even though the transaction was prepared successfully.
> >
> >> Nit: would be nice to have a comment about why overriding this value is
> >> ok.
> >
> > True.
>
> Yup. It seems we will see a v2 for updating the test code as well,
> so I'll assume that you'd explain this as an independent fix (as
> well as a required preliminary fix for the other one).
I see that the patch series has been merged to "next" a few days ago
though, and is slated to be merged to "master". That's why I refrained
from sending a v2.
I can send a follow-up patch to remove the useless variable assignment
in the test, but other than that I don't think anything needs to change
here. Or did I miss anything else?
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 0/7] reftable: memory optimizations for reflog iteration
From: Patrick Steinhardt @ 2024-03-05 12:10 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 1826 bytes --]
Hi,
this patch series does the same as all the preceding patch series that
optimized how the reftable library iterates through refs, but for
reflogs instead.
The goal of this patch series is to arrive at a constant number of
allocations when iterating refs. This is achieved in mostly the same way
we did it for ref iteration, namely by reusing already-allocated memory.
Overall, this brings us down from 8 allocations per reflog record to
essentially 0 allocations per reflog. Iterating through 1 million
reflogs with `git reflog list` thus goes down from 8.068m allocations to
only around 68.5k.
This series is built on top of "master" at b387623c12 (The third batch,
2024-03-01) with Junio's "ps/reftable-iteration-perf-part2" at
43f70eaea0 (refs/reftable: precompute prefix length, 2024-03-04) merged
into it.
Patrick
Patrick Steinhardt (7):
refs/reftable: reload correct stack when creating reflog iter
reftable/record: convert old and new object IDs to arrays
reftable/record: avoid copying author info
reftable/record: reuse refnames when decoding log records
reftable/record: reuse message when decoding log records
reftable/record: use scratch buffer when decoding records
refs/reftable: track last log record name via strbuf
refs/reftable-backend.c | 52 +++++----------
reftable/block.c | 4 +-
reftable/block.h | 2 +
reftable/merged_test.c | 11 ++--
reftable/readwrite_test.c | 62 +++++++-----------
reftable/record.c | 129 ++++++++++++++-----------------------
reftable/record.h | 5 +-
reftable/record_test.c | 68 ++++++++++---------
reftable/reftable-record.h | 6 +-
reftable/stack_test.c | 26 ++++----
10 files changed, 154 insertions(+), 211 deletions(-)
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 1/7] refs/reftable: reload correct stack when creating reflog iter
From: Patrick Steinhardt @ 2024-03-05 12:10 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1709640322.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 877 bytes --]
When creating a new reflog iterator, we first have to reload the stack
that the iterator is being created. This is done so that any concurrent
writes to the stack are reflected. But `reflog_iterator_for_stack()`
always reloads the main stack, which is wrong.
Fix this and reload the correct stack.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs/reftable-backend.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 249a618b5a..f04be942ac 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1682,7 +1682,7 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl
if (ret)
goto done;
- ret = reftable_stack_reload(refs->main_stack);
+ ret = reftable_stack_reload(stack);
if (ret < 0)
goto done;
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/7] reftable/record: convert old and new object IDs to arrays
From: Patrick Steinhardt @ 2024-03-05 12:10 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1709640322.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 19223 bytes --]
In 7af607c58d (reftable/record: store "val1" hashes as static arrays,
2024-01-03) and b31e3cc620 (reftable/record: store "val2" hashes as
static arrays, 2024-01-03) we have converted ref records to store their
object IDs in a static array. Convert log records to do the same so that
their old and new object IDs are arrays, too.
This change results in two allocations less per log record that we're
iterating over. Before:
HEAP SUMMARY:
in use at exit: 13,473 bytes in 122 blocks
total heap usage: 8,068,495 allocs, 8,068,373 frees, 401,011,862 bytes allocated
After:
HEAP SUMMARY:
in use at exit: 13,473 bytes in 122 blocks
total heap usage: 6,068,489 allocs, 6,068,367 frees, 361,011,822 bytes allocated
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs/reftable-backend.c | 39 ++++++------------------
reftable/merged_test.c | 11 +++----
reftable/readwrite_test.c | 62 +++++++++++++++-----------------------
reftable/record.c | 59 ++++++------------------------------
reftable/record_test.c | 11 -------
reftable/reftable-record.h | 4 +--
reftable/stack_test.c | 26 +++++++---------
7 files changed, 61 insertions(+), 151 deletions(-)
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index f04be942ac..2de57c047a 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -171,23 +171,6 @@ static int should_write_log(struct ref_store *refs, const char *refname)
}
}
-static void clear_reftable_log_record(struct reftable_log_record *log)
-{
- switch (log->value_type) {
- case REFTABLE_LOG_UPDATE:
- /*
- * When we write log records, the hashes are owned by the
- * caller and thus shouldn't be free'd.
- */
- log->value.update.old_hash = NULL;
- log->value.update.new_hash = NULL;
- break;
- case REFTABLE_LOG_DELETION:
- break;
- }
- reftable_log_record_release(log);
-}
-
static void fill_reftable_log_record(struct reftable_log_record *log)
{
const char *info = git_committer_info(0);
@@ -1102,8 +1085,8 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
fill_reftable_log_record(log);
log->update_index = ts;
log->refname = xstrdup(u->refname);
- log->value.update.new_hash = u->new_oid.hash;
- log->value.update.old_hash = tx_update->current_oid.hash;
+ memcpy(log->value.update.new_hash, u->new_oid.hash, GIT_MAX_RAWSZ);
+ memcpy(log->value.update.old_hash, tx_update->current_oid.hash, GIT_MAX_RAWSZ);
log->value.update.message =
xstrndup(u->msg, arg->refs->write_options.block_size / 2);
}
@@ -1158,7 +1141,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
done:
assert(ret != REFTABLE_API_ERROR);
for (i = 0; i < logs_nr; i++)
- clear_reftable_log_record(&logs[i]);
+ reftable_log_record_release(&logs[i]);
free(logs);
return ret;
}
@@ -1275,13 +1258,13 @@ static int write_create_symref_table(struct reftable_writer *writer, void *cb_da
log.update_index = ts;
log.value.update.message = xstrndup(create->logmsg,
create->refs->write_options.block_size / 2);
- log.value.update.new_hash = new_oid.hash;
+ memcpy(log.value.update.new_hash, new_oid.hash, GIT_MAX_RAWSZ);
if (refs_resolve_ref_unsafe(&create->refs->base, create->refname,
RESOLVE_REF_READING, &old_oid, NULL))
- log.value.update.old_hash = old_oid.hash;
+ memcpy(log.value.update.old_hash, old_oid.hash, GIT_MAX_RAWSZ);
ret = reftable_writer_add_log(writer, &log);
- clear_reftable_log_record(&log);
+ reftable_log_record_release(&log);
return ret;
}
@@ -1420,7 +1403,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)
logs[logs_nr].update_index = deletion_ts;
logs[logs_nr].value.update.message =
xstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);
- logs[logs_nr].value.update.old_hash = old_ref.value.val1;
+ memcpy(logs[logs_nr].value.update.old_hash, old_ref.value.val1, GIT_MAX_RAWSZ);
logs_nr++;
ret = read_ref_without_reload(arg->stack, "HEAD", &head_oid, &head_referent, &head_type);
@@ -1452,7 +1435,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)
logs[logs_nr].update_index = creation_ts;
logs[logs_nr].value.update.message =
xstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);
- logs[logs_nr].value.update.new_hash = old_ref.value.val1;
+ memcpy(logs[logs_nr].value.update.new_hash, old_ref.value.val1, GIT_MAX_RAWSZ);
logs_nr++;
/*
@@ -1515,10 +1498,6 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)
for (i = 0; i < logs_nr; i++) {
if (!strcmp(logs[i].refname, "HEAD"))
continue;
- if (logs[i].value.update.old_hash == old_ref.value.val1)
- logs[i].value.update.old_hash = NULL;
- if (logs[i].value.update.new_hash == old_ref.value.val1)
- logs[i].value.update.new_hash = NULL;
logs[i].refname = NULL;
reftable_log_record_release(&logs[i]);
}
@@ -2180,7 +2159,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,
dest->value_type = REFTABLE_LOG_DELETION;
} else {
if ((flags & EXPIRE_REFLOGS_REWRITE) && last_hash)
- dest->value.update.old_hash = last_hash;
+ memcpy(dest->value.update.old_hash, last_hash, GIT_MAX_RAWSZ);
last_hash = logs[i].value.update.new_hash;
}
}
diff --git a/reftable/merged_test.c b/reftable/merged_test.c
index d0f77a3b8f..530fc82d1c 100644
--- a/reftable/merged_test.c
+++ b/reftable/merged_test.c
@@ -289,16 +289,13 @@ merged_table_from_log_records(struct reftable_log_record **logs,
static void test_merged_logs(void)
{
- uint8_t hash1[GIT_SHA1_RAWSZ] = { 1 };
- uint8_t hash2[GIT_SHA1_RAWSZ] = { 2 };
- uint8_t hash3[GIT_SHA1_RAWSZ] = { 3 };
struct reftable_log_record r1[] = {
{
.refname = "a",
.update_index = 2,
.value_type = REFTABLE_LOG_UPDATE,
.value.update = {
- .old_hash = hash2,
+ .old_hash = { 2 },
/* deletion */
.name = "jane doe",
.email = "jane@invalid",
@@ -310,8 +307,8 @@ static void test_merged_logs(void)
.update_index = 1,
.value_type = REFTABLE_LOG_UPDATE,
.value.update = {
- .old_hash = hash1,
- .new_hash = hash2,
+ .old_hash = { 1 },
+ .new_hash = { 2 },
.name = "jane doe",
.email = "jane@invalid",
.message = "message1",
@@ -324,7 +321,7 @@ static void test_merged_logs(void)
.update_index = 3,
.value_type = REFTABLE_LOG_UPDATE,
.value.update = {
- .new_hash = hash3,
+ .new_hash = { 3 },
.name = "jane doe",
.email = "jane@invalid",
.message = "message3",
diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c
index 363fe0f998..a6dbd214c5 100644
--- a/reftable/readwrite_test.c
+++ b/reftable/readwrite_test.c
@@ -77,18 +77,15 @@ static void write_table(char ***names, struct strbuf *buf, int N,
}
for (i = 0; i < N; i++) {
- uint8_t hash[GIT_SHA256_RAWSZ] = { 0 };
char name[100];
int n;
- set_test_hash(hash, i);
-
snprintf(name, sizeof(name), "refs/heads/branch%02d", i);
log.refname = name;
log.update_index = update_index;
log.value_type = REFTABLE_LOG_UPDATE;
- log.value.update.new_hash = hash;
+ set_test_hash(log.value.update.new_hash, i);
log.value.update.message = "message";
n = reftable_writer_add_log(w, &log);
@@ -137,13 +134,10 @@ static void test_log_buffer_size(void)
/* This tests buffer extension for log compression. Must use a random
hash, to ensure that the compressed part is larger than the original.
*/
- uint8_t hash1[GIT_SHA1_RAWSZ], hash2[GIT_SHA1_RAWSZ];
for (i = 0; i < GIT_SHA1_RAWSZ; i++) {
- hash1[i] = (uint8_t)(git_rand() % 256);
- hash2[i] = (uint8_t)(git_rand() % 256);
+ log.value.update.old_hash[i] = (uint8_t)(git_rand() % 256);
+ log.value.update.new_hash[i] = (uint8_t)(git_rand() % 256);
}
- log.value.update.old_hash = hash1;
- log.value.update.new_hash = hash2;
reftable_writer_set_limits(w, update_index, update_index);
err = reftable_writer_add_log(w, &log);
EXPECT_ERR(err);
@@ -161,25 +155,26 @@ static void test_log_overflow(void)
.block_size = ARRAY_SIZE(msg),
};
int err;
- struct reftable_log_record
- log = { .refname = "refs/heads/master",
- .update_index = 0xa,
- .value_type = REFTABLE_LOG_UPDATE,
- .value = { .update = {
- .name = "Han-Wen Nienhuys",
- .email = "hanwen@google.com",
- .tz_offset = 100,
- .time = 0x5e430672,
- .message = msg,
- } } };
+ struct reftable_log_record log = {
+ .refname = "refs/heads/master",
+ .update_index = 0xa,
+ .value_type = REFTABLE_LOG_UPDATE,
+ .value = {
+ .update = {
+ .old_hash = { 1 },
+ .new_hash = { 2 },
+ .name = "Han-Wen Nienhuys",
+ .email = "hanwen@google.com",
+ .tz_offset = 100,
+ .time = 0x5e430672,
+ .message = msg,
+ },
+ },
+ };
struct reftable_writer *w =
reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
- uint8_t hash1[GIT_SHA1_RAWSZ] = {1}, hash2[GIT_SHA1_RAWSZ] = { 2 };
-
memset(msg, 'x', sizeof(msg) - 1);
- log.value.update.old_hash = hash1;
- log.value.update.new_hash = hash2;
reftable_writer_set_limits(w, update_index, update_index);
err = reftable_writer_add_log(w, &log);
EXPECT(err == REFTABLE_ENTRY_TOO_BIG_ERROR);
@@ -219,16 +214,13 @@ static void test_log_write_read(void)
EXPECT_ERR(err);
}
for (i = 0; i < N; i++) {
- uint8_t hash1[GIT_SHA1_RAWSZ], hash2[GIT_SHA1_RAWSZ];
struct reftable_log_record log = { NULL };
- set_test_hash(hash1, i);
- set_test_hash(hash2, i + 1);
log.refname = names[i];
log.update_index = i;
log.value_type = REFTABLE_LOG_UPDATE;
- log.value.update.old_hash = hash1;
- log.value.update.new_hash = hash2;
+ set_test_hash(log.value.update.old_hash, i);
+ set_test_hash(log.value.update.new_hash, i + 1);
err = reftable_writer_add_log(w, &log);
EXPECT_ERR(err);
@@ -298,18 +290,15 @@ static void test_log_zlib_corruption(void)
struct reftable_writer *w =
reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
const struct reftable_stats *stats = NULL;
- uint8_t hash1[GIT_SHA1_RAWSZ] = { 1 };
- uint8_t hash2[GIT_SHA1_RAWSZ] = { 2 };
char message[100] = { 0 };
int err, i, n;
-
struct reftable_log_record log = {
.refname = "refname",
.value_type = REFTABLE_LOG_UPDATE,
.value = {
.update = {
- .new_hash = hash1,
- .old_hash = hash2,
+ .new_hash = { 1 },
+ .old_hash = { 2 },
.name = "My Name",
.email = "myname@invalid",
.message = message,
@@ -821,13 +810,12 @@ static void test_write_multiple_indices(void)
}
for (i = 0; i < 100; i++) {
- unsigned char hash[GIT_SHA1_RAWSZ] = {i};
struct reftable_log_record log = {
.update_index = 1,
.value_type = REFTABLE_LOG_UPDATE,
.value.update = {
- .old_hash = hash,
- .new_hash = hash,
+ .old_hash = { i },
+ .new_hash = { i },
},
};
diff --git a/reftable/record.c b/reftable/record.c
index 367de04600..8d2cd6b081 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -763,16 +763,10 @@ static void reftable_log_record_copy_from(void *rec, const void *src_rec,
xstrdup(dst->value.update.message);
}
- if (dst->value.update.new_hash) {
- REFTABLE_ALLOC_ARRAY(dst->value.update.new_hash, hash_size);
- memcpy(dst->value.update.new_hash,
- src->value.update.new_hash, hash_size);
- }
- if (dst->value.update.old_hash) {
- REFTABLE_ALLOC_ARRAY(dst->value.update.old_hash, hash_size);
- memcpy(dst->value.update.old_hash,
- src->value.update.old_hash, hash_size);
- }
+ memcpy(dst->value.update.new_hash,
+ src->value.update.new_hash, hash_size);
+ memcpy(dst->value.update.old_hash,
+ src->value.update.old_hash, hash_size);
break;
}
}
@@ -790,8 +784,6 @@ void reftable_log_record_release(struct reftable_log_record *r)
case REFTABLE_LOG_DELETION:
break;
case REFTABLE_LOG_UPDATE:
- reftable_free(r->value.update.new_hash);
- reftable_free(r->value.update.old_hash);
reftable_free(r->value.update.name);
reftable_free(r->value.update.email);
reftable_free(r->value.update.message);
@@ -808,33 +800,20 @@ static uint8_t reftable_log_record_val_type(const void *rec)
return reftable_log_record_is_deletion(log) ? 0 : 1;
}
-static uint8_t zero[GIT_SHA256_RAWSZ] = { 0 };
-
static int reftable_log_record_encode(const void *rec, struct string_view s,
int hash_size)
{
const struct reftable_log_record *r = rec;
struct string_view start = s;
int n = 0;
- uint8_t *oldh = NULL;
- uint8_t *newh = NULL;
if (reftable_log_record_is_deletion(r))
return 0;
- oldh = r->value.update.old_hash;
- newh = r->value.update.new_hash;
- if (!oldh) {
- oldh = zero;
- }
- if (!newh) {
- newh = zero;
- }
-
if (s.len < 2 * hash_size)
return -1;
- memcpy(s.buf, oldh, hash_size);
- memcpy(s.buf + hash_size, newh, hash_size);
+ memcpy(s.buf, r->value.update.old_hash, hash_size);
+ memcpy(s.buf + hash_size, r->value.update.new_hash, hash_size);
string_view_consume(&s, 2 * hash_size);
n = encode_string(r->value.update.name ? r->value.update.name : "", s);
@@ -891,8 +870,6 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,
if (val_type != r->value_type) {
switch (r->value_type) {
case REFTABLE_LOG_UPDATE:
- FREE_AND_NULL(r->value.update.old_hash);
- FREE_AND_NULL(r->value.update.new_hash);
FREE_AND_NULL(r->value.update.message);
FREE_AND_NULL(r->value.update.email);
FREE_AND_NULL(r->value.update.name);
@@ -909,11 +886,6 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,
if (in.len < 2 * hash_size)
return REFTABLE_FORMAT_ERROR;
- r->value.update.old_hash =
- reftable_realloc(r->value.update.old_hash, hash_size);
- r->value.update.new_hash =
- reftable_realloc(r->value.update.new_hash, hash_size);
-
memcpy(r->value.update.old_hash, in.buf, hash_size);
memcpy(r->value.update.new_hash, in.buf + hash_size, hash_size);
@@ -983,17 +955,6 @@ static int null_streq(char *a, char *b)
return 0 == strcmp(a, b);
}
-static int zero_hash_eq(uint8_t *a, uint8_t *b, int sz)
-{
- if (!a)
- a = zero;
-
- if (!b)
- b = zero;
-
- return !memcmp(a, b, sz);
-}
-
static int reftable_log_record_equal_void(const void *a,
const void *b, int hash_size)
{
@@ -1037,10 +998,10 @@ int reftable_log_record_equal(const struct reftable_log_record *a,
b->value.update.email) &&
null_streq(a->value.update.message,
b->value.update.message) &&
- zero_hash_eq(a->value.update.old_hash,
- b->value.update.old_hash, hash_size) &&
- zero_hash_eq(a->value.update.new_hash,
- b->value.update.new_hash, hash_size);
+ !memcmp(a->value.update.old_hash,
+ b->value.update.old_hash, hash_size) &&
+ !memcmp(a->value.update.new_hash,
+ b->value.update.new_hash, hash_size);
}
abort();
diff --git a/reftable/record_test.c b/reftable/record_test.c
index 89209894d8..57e56138c0 100644
--- a/reftable/record_test.c
+++ b/reftable/record_test.c
@@ -183,8 +183,6 @@ static void test_reftable_log_record_roundtrip(void)
.value_type = REFTABLE_LOG_UPDATE,
.value = {
.update = {
- .old_hash = reftable_malloc(GIT_SHA1_RAWSZ),
- .new_hash = reftable_malloc(GIT_SHA1_RAWSZ),
.name = xstrdup("han-wen"),
.email = xstrdup("hanwen@google.com"),
.message = xstrdup("test"),
@@ -202,13 +200,6 @@ static void test_reftable_log_record_roundtrip(void)
.refname = xstrdup("branch"),
.update_index = 33,
.value_type = REFTABLE_LOG_UPDATE,
- .value = {
- .update = {
- .old_hash = reftable_malloc(GIT_SHA1_RAWSZ),
- .new_hash = reftable_malloc(GIT_SHA1_RAWSZ),
- /* rest of fields left empty. */
- },
- },
}
};
set_test_hash(in[0].value.update.new_hash, 1);
@@ -231,8 +222,6 @@ static void test_reftable_log_record_roundtrip(void)
.value_type = REFTABLE_LOG_UPDATE,
.value = {
.update = {
- .new_hash = reftable_calloc(GIT_SHA1_RAWSZ, 1),
- .old_hash = reftable_calloc(GIT_SHA1_RAWSZ, 1),
.name = xstrdup("old name"),
.email = xstrdup("old@email"),
.message = xstrdup("old message"),
diff --git a/reftable/reftable-record.h b/reftable/reftable-record.h
index e657001d42..2659ea008c 100644
--- a/reftable/reftable-record.h
+++ b/reftable/reftable-record.h
@@ -88,8 +88,8 @@ struct reftable_log_record {
union {
struct {
- uint8_t *new_hash;
- uint8_t *old_hash;
+ unsigned char new_hash[GIT_MAX_RAWSZ];
+ unsigned char old_hash[GIT_MAX_RAWSZ];
char *name;
char *email;
uint64_t time;
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 509f486623..7336757cf5 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -468,8 +468,6 @@ static void test_reftable_stack_add(void)
logs[i].refname = xstrdup(buf);
logs[i].update_index = N + i + 1;
logs[i].value_type = REFTABLE_LOG_UPDATE;
-
- logs[i].value.update.new_hash = reftable_malloc(GIT_SHA1_RAWSZ);
logs[i].value.update.email = xstrdup("identity@invalid");
set_test_hash(logs[i].value.update.new_hash, i);
}
@@ -547,16 +545,17 @@ static void test_reftable_stack_log_normalize(void)
};
struct reftable_stack *st = NULL;
char *dir = get_tmp_dir(__LINE__);
-
- uint8_t h1[GIT_SHA1_RAWSZ] = { 0x01 }, h2[GIT_SHA1_RAWSZ] = { 0x02 };
-
- struct reftable_log_record input = { .refname = "branch",
- .update_index = 1,
- .value_type = REFTABLE_LOG_UPDATE,
- .value = { .update = {
- .new_hash = h1,
- .old_hash = h2,
- } } };
+ struct reftable_log_record input = {
+ .refname = "branch",
+ .update_index = 1,
+ .value_type = REFTABLE_LOG_UPDATE,
+ .value = {
+ .update = {
+ .new_hash = { 1 },
+ .old_hash = { 2 },
+ },
+ },
+ };
struct reftable_log_record dest = {
.update_index = 0,
};
@@ -627,8 +626,6 @@ static void test_reftable_stack_tombstone(void)
logs[i].update_index = 42;
if (i % 2 == 0) {
logs[i].value_type = REFTABLE_LOG_UPDATE;
- logs[i].value.update.new_hash =
- reftable_malloc(GIT_SHA1_RAWSZ);
set_test_hash(logs[i].value.update.new_hash, i);
logs[i].value.update.email =
xstrdup("identity@invalid");
@@ -810,7 +807,6 @@ static void test_reflog_expire(void)
logs[i].update_index = i;
logs[i].value_type = REFTABLE_LOG_UPDATE;
logs[i].value.update.time = i;
- logs[i].value.update.new_hash = reftable_malloc(GIT_SHA1_RAWSZ);
logs[i].value.update.email = xstrdup("identity@invalid");
set_test_hash(logs[i].value.update.new_hash, i);
}
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 3/7] reftable/record: avoid copying author info
From: Patrick Steinhardt @ 2024-03-05 12:11 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1709640322.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3516 bytes --]
Each reflog entry contains information regarding the authorship of who
has made the change. This authorship information is not the same as that
of any of the commits that the reflog entry references, but instead
corresponds to the local user that has executed the command. Thus, it is
almost always the case that all reflog entries have the same author.
We can make use of this fact when decoding reftable records: instead of
freeing and then reallocating the authorship information of log records,
we can special-case when the next record during an iteration has the
exact same authorship as the preceding record. If so, then there is no
need to reallocate the respective fields.
This change results in two allocations less per log record that we're
iterating over in the most common case. Before:
HEAP SUMMARY:
in use at exit: 13,473 bytes in 122 blocks
total heap usage: 6,068,489 allocs, 6,068,367 frees, 361,011,822 bytes allocated
After:
HEAP SUMMARY:
in use at exit: 13,473 bytes in 122 blocks
total heap usage: 4,068,487 allocs, 4,068,365 frees, 332,011,793 bytes allocated
An alternative would be to store the capacity of both name and email and
then use `REFTABLE_ALLOC_GROW()` to conditionally reallocate the array.
But reftable records are copied around quite a lot, and thus we need to
be a bit mindful of the overall record size. Furthermore, a memory
comparison should also be more efficient than having to copy over memory
even if we wouldn't have to allocate a new array every time.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/record.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/reftable/record.c b/reftable/record.c
index 8d2cd6b081..d816de6d93 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -896,10 +896,19 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,
goto done;
string_view_consume(&in, n);
- r->value.update.name =
- reftable_realloc(r->value.update.name, dest.len + 1);
- memcpy(r->value.update.name, dest.buf, dest.len);
- r->value.update.name[dest.len] = 0;
+ /*
+ * In almost all cases we can expect the reflog name to not change for
+ * reflog entries as they are tied to the local identity, not to the
+ * target commits. As an optimization for this common case we can thus
+ * skip copying over the name in case it's accurate already.
+ */
+ if (!r->value.update.name ||
+ strcmp(r->value.update.name, dest.buf)) {
+ r->value.update.name =
+ reftable_realloc(r->value.update.name, dest.len + 1);
+ memcpy(r->value.update.name, dest.buf, dest.len);
+ r->value.update.name[dest.len] = 0;
+ }
strbuf_reset(&dest);
n = decode_string(&dest, in);
@@ -907,10 +916,14 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,
goto done;
string_view_consume(&in, n);
- r->value.update.email =
- reftable_realloc(r->value.update.email, dest.len + 1);
- memcpy(r->value.update.email, dest.buf, dest.len);
- r->value.update.email[dest.len] = 0;
+ /* Same as above, but for the reflog email. */
+ if (!r->value.update.email ||
+ strcmp(r->value.update.email, dest.buf)) {
+ r->value.update.email =
+ reftable_realloc(r->value.update.email, dest.len + 1);
+ memcpy(r->value.update.email, dest.buf, dest.len);
+ r->value.update.email[dest.len] = 0;
+ }
ts = 0;
n = get_var_int(&ts, &in);
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 4/7] reftable/record: reuse refnames when decoding log records
From: Patrick Steinhardt @ 2024-03-05 12:11 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1709640322.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]
When decoding a log record we always reallocate their refname arrays.
This results in quite a lot of needless allocation churn.
Refactor the code to grow the array as required only. Like this, we
should usually only end up reallocating the array a small handful of
times when iterating over many refs. Before:
HEAP SUMMARY:
in use at exit: 13,473 bytes in 122 blocks
total heap usage: 4,068,487 allocs, 4,068,365 frees, 332,011,793 bytes allocated
After:
HEAP SUMMARY:
in use at exit: 13,473 bytes in 122 blocks
total heap usage: 3,068,488 allocs, 3,068,366 frees, 307,122,961 bytes allocated
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/record.c | 2 +-
reftable/reftable-record.h | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/reftable/record.c b/reftable/record.c
index d816de6d93..82780b2d06 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -861,7 +861,7 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,
if (key.len <= 9 || key.buf[key.len - 9] != 0)
return REFTABLE_FORMAT_ERROR;
- r->refname = reftable_realloc(r->refname, key.len - 8);
+ REFTABLE_ALLOC_GROW(r->refname, key.len - 8, r->refname_cap);
memcpy(r->refname, key.buf, key.len - 8);
ts = get_be64(key.buf + key.len - 8);
diff --git a/reftable/reftable-record.h b/reftable/reftable-record.h
index 2659ea008c..d28f38175c 100644
--- a/reftable/reftable-record.h
+++ b/reftable/reftable-record.h
@@ -74,6 +74,7 @@ int reftable_ref_record_equal(const struct reftable_ref_record *a,
/* reftable_log_record holds a reflog entry */
struct reftable_log_record {
char *refname;
+ size_t refname_cap;
uint64_t update_index; /* logical timestamp of a transactional update.
*/
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 5/7] reftable/record: reuse message when decoding log records
From: Patrick Steinhardt @ 2024-03-05 12:11 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1709640322.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]
Same as the preceding commit we can allocate log messages as needed when
decoding log records, thus further reducing the number of allocations.
Before:
HEAP SUMMARY:
in use at exit: 13,473 bytes in 122 blocks
total heap usage: 3,068,488 allocs, 3,068,366 frees, 307,122,961 bytes allocated
After:
HEAP SUMMARY:
in use at exit: 13,473 bytes in 122 blocks
total heap usage: 2,068,487 allocs, 2,068,365 frees, 305,122,946 bytes allocated
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/record.c | 5 +++--
reftable/reftable-record.h | 1 +
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/reftable/record.c b/reftable/record.c
index 82780b2d06..7c86877586 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -871,6 +871,7 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,
switch (r->value_type) {
case REFTABLE_LOG_UPDATE:
FREE_AND_NULL(r->value.update.message);
+ r->value.update.message_cap = 0;
FREE_AND_NULL(r->value.update.email);
FREE_AND_NULL(r->value.update.name);
break;
@@ -943,8 +944,8 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,
goto done;
string_view_consume(&in, n);
- r->value.update.message =
- reftable_realloc(r->value.update.message, dest.len + 1);
+ REFTABLE_ALLOC_GROW(r->value.update.message, dest.len + 1,
+ r->value.update.message_cap);
memcpy(r->value.update.message, dest.buf, dest.len);
r->value.update.message[dest.len] = 0;
diff --git a/reftable/reftable-record.h b/reftable/reftable-record.h
index d28f38175c..2a2943cd13 100644
--- a/reftable/reftable-record.h
+++ b/reftable/reftable-record.h
@@ -96,6 +96,7 @@ struct reftable_log_record {
uint64_t time;
int16_t tz_offset;
char *message;
+ size_t message_cap;
} update;
} value;
};
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 6/7] reftable/record: use scratch buffer when decoding records
From: Patrick Steinhardt @ 2024-03-05 12:11 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1709640322.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 13543 bytes --]
When decoding log records we need a temporary buffer to decode the
reflog entry's name, mail address and message. As this buffer is local
to the function we thus have to reallocate it for every single log
record which we're about to decode, which is inefficient.
Refactor the code such that callers need to pass in a scratch buffer,
which allows us to reuse it for multiple decodes. This reduces the
number of allocations when iterating through reflogs. Before:
HEAP SUMMARY:
in use at exit: 13,473 bytes in 122 blocks
total heap usage: 2,068,487 allocs, 2,068,365 frees, 305,122,946 bytes allocated
After:
HEAP SUMMARY:
in use at exit: 13,473 bytes in 122 blocks
total heap usage: 1,068,485 allocs, 1,068,363 frees, 281,122,886 bytes allocated
Note that this commit also drop some redundant calls to `strbuf_reset()`
right before calling `decode_string()`. The latter already knows to
reset the buffer, so there is no need for these.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 4 ++-
reftable/block.h | 2 ++
reftable/record.c | 52 ++++++++++++++++++--------------------
reftable/record.h | 5 ++--
reftable/record_test.c | 57 ++++++++++++++++++++++++++----------------
5 files changed, 68 insertions(+), 52 deletions(-)
diff --git a/reftable/block.c b/reftable/block.c
index ad9074dba6..b67300a734 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -332,7 +332,8 @@ int block_iter_next(struct block_iter *it, struct reftable_record *rec)
return REFTABLE_FORMAT_ERROR;
string_view_consume(&in, n);
- n = reftable_record_decode(rec, it->last_key, extra, in, it->br->hash_size);
+ n = reftable_record_decode(rec, it->last_key, extra, in, it->br->hash_size,
+ &it->scratch);
if (n < 0)
return -1;
string_view_consume(&in, n);
@@ -369,6 +370,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->scratch);
}
int block_reader_seek(struct block_reader *br, struct block_iter *it,
diff --git a/reftable/block.h b/reftable/block.h
index 51699af233..47acc62c0a 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 scratch;
};
#define BLOCK_ITER_INIT { \
.last_key = STRBUF_INIT, \
+ .scratch = STRBUF_INIT, \
}
/* initializes a block reader. */
diff --git a/reftable/record.c b/reftable/record.c
index 7c86877586..060244337f 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -374,7 +374,7 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,
static int reftable_ref_record_decode(void *rec, struct strbuf key,
uint8_t val_type, struct string_view in,
- int hash_size)
+ int hash_size, struct strbuf *scratch)
{
struct reftable_ref_record *r = rec;
struct string_view start = in;
@@ -425,13 +425,12 @@ static int reftable_ref_record_decode(void *rec, struct strbuf key,
break;
case REFTABLE_REF_SYMREF: {
- struct strbuf dest = STRBUF_INIT;
- int n = decode_string(&dest, in);
+ int n = decode_string(scratch, in);
if (n < 0) {
return -1;
}
string_view_consume(&in, n);
- r->value.symref = dest.buf;
+ r->value.symref = strbuf_detach(scratch, NULL);
} break;
case REFTABLE_REF_DELETION:
@@ -579,7 +578,7 @@ static int reftable_obj_record_encode(const void *rec, struct string_view s,
static int reftable_obj_record_decode(void *rec, struct strbuf key,
uint8_t val_type, struct string_view in,
- int hash_size)
+ int hash_size, struct strbuf *scratch UNUSED)
{
struct string_view start = in;
struct reftable_obj_record *r = rec;
@@ -849,13 +848,12 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,
static int reftable_log_record_decode(void *rec, struct strbuf key,
uint8_t val_type, struct string_view in,
- int hash_size)
+ int hash_size, struct strbuf *scratch)
{
struct string_view start = in;
struct reftable_log_record *r = rec;
uint64_t max = 0;
uint64_t ts = 0;
- struct strbuf dest = STRBUF_INIT;
int n;
if (key.len <= 9 || key.buf[key.len - 9] != 0)
@@ -892,7 +890,7 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,
string_view_consume(&in, 2 * hash_size);
- n = decode_string(&dest, in);
+ n = decode_string(scratch, in);
if (n < 0)
goto done;
string_view_consume(&in, n);
@@ -904,26 +902,25 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,
* skip copying over the name in case it's accurate already.
*/
if (!r->value.update.name ||
- strcmp(r->value.update.name, dest.buf)) {
+ strcmp(r->value.update.name, scratch->buf)) {
r->value.update.name =
- reftable_realloc(r->value.update.name, dest.len + 1);
- memcpy(r->value.update.name, dest.buf, dest.len);
- r->value.update.name[dest.len] = 0;
+ reftable_realloc(r->value.update.name, scratch->len + 1);
+ memcpy(r->value.update.name, scratch->buf, scratch->len);
+ r->value.update.name[scratch->len] = 0;
}
- strbuf_reset(&dest);
- n = decode_string(&dest, in);
+ n = decode_string(scratch, in);
if (n < 0)
goto done;
string_view_consume(&in, n);
/* Same as above, but for the reflog email. */
if (!r->value.update.email ||
- strcmp(r->value.update.email, dest.buf)) {
+ strcmp(r->value.update.email, scratch->buf)) {
r->value.update.email =
- reftable_realloc(r->value.update.email, dest.len + 1);
- memcpy(r->value.update.email, dest.buf, dest.len);
- r->value.update.email[dest.len] = 0;
+ reftable_realloc(r->value.update.email, scratch->len + 1);
+ memcpy(r->value.update.email, scratch->buf, scratch->len);
+ r->value.update.email[scratch->len] = 0;
}
ts = 0;
@@ -938,22 +935,19 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,
r->value.update.tz_offset = get_be16(in.buf);
string_view_consume(&in, 2);
- strbuf_reset(&dest);
- n = decode_string(&dest, in);
+ n = decode_string(scratch, in);
if (n < 0)
goto done;
string_view_consume(&in, n);
- REFTABLE_ALLOC_GROW(r->value.update.message, dest.len + 1,
+ REFTABLE_ALLOC_GROW(r->value.update.message, scratch->len + 1,
r->value.update.message_cap);
- memcpy(r->value.update.message, dest.buf, dest.len);
- r->value.update.message[dest.len] = 0;
+ memcpy(r->value.update.message, scratch->buf, scratch->len);
+ r->value.update.message[scratch->len] = 0;
- strbuf_release(&dest);
return start.len - in.len;
done:
- strbuf_release(&dest);
return REFTABLE_FORMAT_ERROR;
}
@@ -1093,7 +1087,7 @@ static int reftable_index_record_encode(const void *rec, struct string_view out,
static int reftable_index_record_decode(void *rec, struct strbuf key,
uint8_t val_type, struct string_view in,
- int hash_size)
+ int hash_size, struct strbuf *scratch UNUSED)
{
struct string_view start = in;
struct reftable_index_record *r = rec;
@@ -1174,10 +1168,12 @@ uint8_t reftable_record_val_type(struct reftable_record *rec)
}
int reftable_record_decode(struct reftable_record *rec, struct strbuf key,
- uint8_t extra, struct string_view src, int hash_size)
+ uint8_t extra, struct string_view src, int hash_size,
+ struct strbuf *scratch)
{
return reftable_record_vtable(rec)->decode(reftable_record_data(rec),
- key, extra, src, hash_size);
+ key, extra, src, hash_size,
+ scratch);
}
void reftable_record_release(struct reftable_record *rec)
diff --git a/reftable/record.h b/reftable/record.h
index 5e8304e052..826ee1c55c 100644
--- a/reftable/record.h
+++ b/reftable/record.h
@@ -55,7 +55,8 @@ struct reftable_record_vtable {
/* decode data from `src` into the record. */
int (*decode)(void *rec, struct strbuf key, uint8_t extra,
- struct string_view src, int hash_size);
+ struct string_view src, int hash_size,
+ struct strbuf *scratch);
/* deallocate and null the record. */
void (*release)(void *rec);
@@ -138,7 +139,7 @@ int reftable_record_encode(struct reftable_record *rec, struct string_view dest,
int hash_size);
int reftable_record_decode(struct reftable_record *rec, struct strbuf key,
uint8_t extra, struct string_view src,
- int hash_size);
+ int hash_size, struct strbuf *scratch);
int reftable_record_is_deletion(struct reftable_record *rec);
static inline uint8_t reftable_record_type(struct reftable_record *rec)
diff --git a/reftable/record_test.c b/reftable/record_test.c
index 57e56138c0..c158ee79ff 100644
--- a/reftable/record_test.c
+++ b/reftable/record_test.c
@@ -99,6 +99,7 @@ static void set_hash(uint8_t *h, int j)
static void test_reftable_ref_record_roundtrip(void)
{
+ struct strbuf scratch = STRBUF_INIT;
int i = 0;
for (i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) {
@@ -140,7 +141,7 @@ static void test_reftable_ref_record_roundtrip(void)
EXPECT(n > 0);
/* decode into a non-zero reftable_record to test for leaks. */
- m = reftable_record_decode(&out, key, i, dest, GIT_SHA1_RAWSZ);
+ m = reftable_record_decode(&out, key, i, dest, GIT_SHA1_RAWSZ, &scratch);
EXPECT(n == m);
EXPECT(reftable_ref_record_equal(&in.u.ref, &out.u.ref,
@@ -150,6 +151,8 @@ static void test_reftable_ref_record_roundtrip(void)
strbuf_release(&key);
reftable_record_release(&out);
}
+
+ strbuf_release(&scratch);
}
static void test_reftable_log_record_equal(void)
@@ -175,7 +178,6 @@ static void test_reftable_log_record_equal(void)
static void test_reftable_log_record_roundtrip(void)
{
int i;
-
struct reftable_log_record in[] = {
{
.refname = xstrdup("refs/heads/master"),
@@ -202,6 +204,8 @@ static void test_reftable_log_record_roundtrip(void)
.value_type = REFTABLE_LOG_UPDATE,
}
};
+ struct strbuf scratch = STRBUF_INIT;
+
set_test_hash(in[0].value.update.new_hash, 1);
set_test_hash(in[0].value.update.old_hash, 2);
set_test_hash(in[2].value.update.new_hash, 3);
@@ -241,7 +245,7 @@ static void test_reftable_log_record_roundtrip(void)
EXPECT(n >= 0);
valtype = reftable_record_val_type(&rec);
m = reftable_record_decode(&out, key, valtype, dest,
- GIT_SHA1_RAWSZ);
+ GIT_SHA1_RAWSZ, &scratch);
EXPECT(n == m);
EXPECT(reftable_log_record_equal(&in[i], &out.u.log,
@@ -250,6 +254,8 @@ static void test_reftable_log_record_roundtrip(void)
strbuf_release(&key);
reftable_record_release(&out);
}
+
+ strbuf_release(&scratch);
}
static void test_u24_roundtrip(void)
@@ -299,23 +305,27 @@ static void test_reftable_obj_record_roundtrip(void)
{
uint8_t testHash1[GIT_SHA1_RAWSZ] = { 1, 2, 3, 4, 0 };
uint64_t till9[] = { 1, 2, 3, 4, 500, 600, 700, 800, 9000 };
- struct reftable_obj_record recs[3] = { {
- .hash_prefix = testHash1,
- .hash_prefix_len = 5,
- .offsets = till9,
- .offset_len = 3,
- },
- {
- .hash_prefix = testHash1,
- .hash_prefix_len = 5,
- .offsets = till9,
- .offset_len = 9,
- },
- {
- .hash_prefix = testHash1,
- .hash_prefix_len = 5,
- } };
+ struct reftable_obj_record recs[3] = {
+ {
+ .hash_prefix = testHash1,
+ .hash_prefix_len = 5,
+ .offsets = till9,
+ .offset_len = 3,
+ },
+ {
+ .hash_prefix = testHash1,
+ .hash_prefix_len = 5,
+ .offsets = till9,
+ .offset_len = 9,
+ },
+ {
+ .hash_prefix = testHash1,
+ .hash_prefix_len = 5,
+ },
+ };
+ struct strbuf scratch = STRBUF_INIT;
int i = 0;
+
for (i = 0; i < ARRAY_SIZE(recs); i++) {
uint8_t buffer[1024] = { 0 };
struct string_view dest = {
@@ -339,13 +349,15 @@ static void test_reftable_obj_record_roundtrip(void)
EXPECT(n > 0);
extra = reftable_record_val_type(&in);
m = reftable_record_decode(&out, key, extra, dest,
- GIT_SHA1_RAWSZ);
+ GIT_SHA1_RAWSZ, &scratch);
EXPECT(n == m);
EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ));
strbuf_release(&key);
reftable_record_release(&out);
}
+
+ strbuf_release(&scratch);
}
static void test_reftable_index_record_roundtrip(void)
@@ -362,6 +374,7 @@ static void test_reftable_index_record_roundtrip(void)
.buf = buffer,
.len = sizeof(buffer),
};
+ struct strbuf scratch = STRBUF_INIT;
struct strbuf key = STRBUF_INIT;
struct reftable_record out = {
.type = BLOCK_TYPE_INDEX,
@@ -379,13 +392,15 @@ static void test_reftable_index_record_roundtrip(void)
EXPECT(n > 0);
extra = reftable_record_val_type(&in);
- m = reftable_record_decode(&out, key, extra, dest, GIT_SHA1_RAWSZ);
+ m = reftable_record_decode(&out, key, extra, dest, GIT_SHA1_RAWSZ,
+ &scratch);
EXPECT(m == n);
EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ));
reftable_record_release(&out);
strbuf_release(&key);
+ strbuf_release(&scratch);
strbuf_release(&in.u.idx.last_key);
}
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 7/7] refs/reftable: track last log record name via strbuf
From: Patrick Steinhardt @ 2024-03-05 12:11 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1709640322.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3335 bytes --]
The reflog iterator enumerates all reflogs known to a ref backend. In
the "reftable" backend there is no way to list all existing reflogs
directly. Instead, we have to iterate through all reflog entries and
discard all those redundant entries for which we have already returned a
reflog entry.
This logic is implemented by tracking the last reflog name that we have
emitted to the iterator's user. If the next log record has the same name
we simply skip it until we find another record with a different refname.
This last reflog name is stored in a simple C string, which requires us
to free and reallocate it whenever we need to update the reflog name.
Convert it to use a `struct strbuf` instead, which reduces the number of
allocations. Before:
HEAP SUMMARY:
in use at exit: 13,473 bytes in 122 blocks
total heap usage: 1,068,485 allocs, 1,068,363 frees, 281,122,886 bytes allocated
After:
HEAP SUMMARY:
in use at exit: 13,473 bytes in 122 blocks
total heap usage: 68,485 allocs, 68,363 frees, 256,234,072 bytes allocated
Note that even after this change we still allocate quite a lot of data,
even though the number of allocations does not scale with the number of
log records anymore. This remainder comes mostly from decompressing the
log blocks, where we decompress each block into newly allocated memory.
This will be addressed at a later point in time.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs/reftable-backend.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 2de57c047a..12960d93ff 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1575,7 +1575,7 @@ struct reftable_reflog_iterator {
struct reftable_ref_store *refs;
struct reftable_iterator iter;
struct reftable_log_record log;
- char *last_name;
+ struct strbuf last_name;
int err;
};
@@ -1594,15 +1594,15 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)
* we've already produced this name. This could be faster by
* seeking directly to reflog@update_index==0.
*/
- if (iter->last_name && !strcmp(iter->log.refname, iter->last_name))
+ if (!strcmp(iter->log.refname, iter->last_name.buf))
continue;
if (check_refname_format(iter->log.refname,
REFNAME_ALLOW_ONELEVEL))
continue;
- free(iter->last_name);
- iter->last_name = xstrdup(iter->log.refname);
+ strbuf_reset(&iter->last_name);
+ strbuf_addstr(&iter->last_name, iter->log.refname);
iter->base.refname = iter->log.refname;
break;
@@ -1635,7 +1635,7 @@ static int reftable_reflog_iterator_abort(struct ref_iterator *ref_iterator)
(struct reftable_reflog_iterator *)ref_iterator;
reftable_log_record_release(&iter->log);
reftable_iterator_destroy(&iter->iter);
- free(iter->last_name);
+ strbuf_release(&iter->last_name);
free(iter);
return ITER_DONE;
}
@@ -1655,6 +1655,7 @@ 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);
+ strbuf_init(&iter->last_name, 0);
iter->refs = refs;
ret = refs->err;
--
2.44.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH] show-ref: add --unresolved option
From: Phillip Wood @ 2024-03-05 15:30 UTC (permalink / raw)
To: John Cai via GitGitGadget, git; +Cc: John Cai
In-Reply-To: <pull.1684.git.git.1709592718743.gitgitgadget@gmail.com>
Hi John
On 04/03/2024 22:51, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
>
> For reftable development, it would be handy to have a tool to provide
> the direct value of any ref whether it be a symbolic ref or not.
> Currently there is git-symbolic-ref, which only works for symbolic refs,
> and git-rev-parse, which will resolve the ref. Let's add a --unresolved
> option that will only take one ref and return whatever it points to
> without dereferencing it.
"--unresolved" makes me think of merge conflicts. I wonder if
"--no-dereference" would be clearer.
Best Wishes
Phillip
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
> show-ref: add --unresolved option
>
> For reftable development, it would be handy to have a tool to provide
> the direct value of any ref whether it be a symbolic ref or not.
> Currently there is git-symbolic-ref, which only works for symbolic refs,
> and git-rev-parse, which will resolve the ref. Let's add a --unresolved
> option that will only take one ref and return whatever it points to
> without dereferencing it.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1684%2Fjohn-cai%2Fjc%2Fshow-ref-direct-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1684/john-cai/jc/show-ref-direct-v1
> Pull-Request: https://github.com/git/git/pull/1684
>
> Documentation/git-show-ref.txt | 8 ++++++
> builtin/show-ref.c | 33 ++++++++++++++++--------
> t/t1403-show-ref.sh | 47 ++++++++++++++++++++++++++++++++++
> 3 files changed, 77 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
> index ba757470059..2f9b4de1346 100644
> --- a/Documentation/git-show-ref.txt
> +++ b/Documentation/git-show-ref.txt
> @@ -16,6 +16,7 @@ SYNOPSIS
> [--] [<ref>...]
> 'git show-ref' --exclude-existing[=<pattern>]
> 'git show-ref' --exists <ref>
> +'git show-ref' --unresolved <ref>
>
> DESCRIPTION
> -----------
> @@ -76,6 +77,13 @@ OPTIONS
> it does, 2 if it is missing, and 1 in case looking up the reference
> failed with an error other than the reference being missing.
>
> +--unresolved::
> +
> + Prints out what the reference points to without resolving it. Returns
> + an exit code of 0 if it does, 2 if it is missing, and 1 in case looking
> + up the reference failed with an error other than the reference being
> + missing.
> +
> --abbrev[=<n>]::
>
> Abbreviate the object name. When using `--hash`, you do
> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> index 1c15421e600..58efa078399 100644
> --- a/builtin/show-ref.c
> +++ b/builtin/show-ref.c
> @@ -18,6 +18,7 @@ static const char * const show_ref_usage[] = {
> " [--] [<ref>...]"),
> N_("git show-ref --exclude-existing[=<pattern>]"),
> N_("git show-ref --exists <ref>"),
> + N_("git show-ref --unresolved <ref>"),
> NULL
> };
>
> @@ -220,11 +221,11 @@ static int cmd_show_ref__patterns(const struct patterns_options *opts,
> return 0;
> }
>
> -static int cmd_show_ref__exists(const char **refs)
> +static int cmd_show_ref__raw(const char **refs, int show)
> {
> - struct strbuf unused_referent = STRBUF_INIT;
> - struct object_id unused_oid;
> - unsigned int unused_type;
> + struct strbuf referent = STRBUF_INIT;
> + struct object_id oid;
> + unsigned int type;
> int failure_errno = 0;
> const char *ref;
> int ret = 0;
> @@ -236,7 +237,7 @@ static int cmd_show_ref__exists(const char **refs)
> die("--exists requires exactly one reference");
>
> if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
> - &unused_oid, &unused_referent, &unused_type,
> + &oid, &referent, &type,
> &failure_errno)) {
> if (failure_errno == ENOENT || failure_errno == EISDIR) {
> error(_("reference does not exist"));
> @@ -250,8 +251,16 @@ static int cmd_show_ref__exists(const char **refs)
> goto out;
> }
>
> + if (!show)
> + goto out;
> +
> + if (type & REF_ISSYMREF)
> + printf("ref: %s\n", referent.buf);
> + else
> + printf("ref: %s\n", oid_to_hex(&oid));
> +
> out:
> - strbuf_release(&unused_referent);
> + strbuf_release(&referent);
> return ret;
> }
>
> @@ -284,11 +293,12 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
> struct exclude_existing_options exclude_existing_opts = {0};
> struct patterns_options patterns_opts = {0};
> struct show_one_options show_one_opts = {0};
> - int verify = 0, exists = 0;
> + int verify = 0, exists = 0, unresolved = 0;
> const struct option show_ref_options[] = {
> OPT_BOOL(0, "tags", &patterns_opts.tags_only, N_("only show tags (can be combined with heads)")),
> OPT_BOOL(0, "heads", &patterns_opts.heads_only, N_("only show heads (can be combined with tags)")),
> OPT_BOOL(0, "exists", &exists, N_("check for reference existence without resolving")),
> + OPT_BOOL(0, "unresolved", &unresolved, N_("print out unresolved value of reference")),
> OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
> "requires exact ref path")),
> OPT_HIDDEN_BOOL('h', NULL, &patterns_opts.show_head,
> @@ -314,16 +324,17 @@ 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);
>
> - die_for_incompatible_opt3(exclude_existing_opts.enabled, "--exclude-existing",
> + die_for_incompatible_opt4(exclude_existing_opts.enabled, "--exclude-existing",
> verify, "--verify",
> - exists, "--exists");
> + exists, "--exists",
> + unresolved, "--unresolved");
>
> if (exclude_existing_opts.enabled)
> return cmd_show_ref__exclude_existing(&exclude_existing_opts);
> else if (verify)
> return cmd_show_ref__verify(&show_one_opts, argv);
> - else if (exists)
> - return cmd_show_ref__exists(argv);
> + else if (exists || unresolved)
> + return cmd_show_ref__raw(argv, unresolved);
> else
> return cmd_show_ref__patterns(&patterns_opts, &show_one_opts, argv);
> }
> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
> index 33fb7a38fff..11811201738 100755
> --- a/t/t1403-show-ref.sh
> +++ b/t/t1403-show-ref.sh
> @@ -218,6 +218,16 @@ test_expect_success 'show-ref sub-modes are mutually exclusive' '
> test_must_fail git show-ref --exclude-existing --exists 2>err &&
> grep "exclude-existing" err &&
> grep "exists" err &&
> + grep "cannot be used together" err &&
> +
> + test_must_fail git show-ref --exclude-existing --unresolved 2>err &&
> + grep "exclude-existing" err &&
> + grep "unresolved" err &&
> + grep "cannot be used together" err &&
> +
> + test_must_fail git show-ref --verify --unresolved 2>err &&
> + grep "verify" err &&
> + grep "unresolved" err &&
> grep "cannot be used together" err
> '
>
> @@ -286,4 +296,41 @@ test_expect_success '--exists with existing special ref' '
> git show-ref --exists FETCH_HEAD
> '
>
> +test_expect_success '--unresolved with existing reference' '
> + commit_oid=$(git rev-parse refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME) &&
> + cat >expect <<-EOF &&
> + ref: $commit_oid
> + EOF
> + git show-ref --unresolved refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success '--unresolved with symbolic ref' '
> + test_when_finished "git symbolic-ref -d SYMBOLIC_REF_A" &&
> + cat >expect <<-EOF &&
> + ref: refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> + EOF
> + git symbolic-ref SYMBOLIC_REF_A refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME &&
> + git show-ref --unresolved SYMBOLIC_REF_A >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success '--unresolved with nonexistent object ID' '
> + oid=$(test_oid 002) &&
> + test-tool ref-store main update-ref msg refs/heads/missing-oid-2 $oid $ZERO_OID REF_SKIP_OID_VERIFICATION &&
> + cat >expect <<-EOF &&
> + ref: $oid
> + EOF
> + git show-ref --unresolved refs/heads/missing-oid-2 >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success '--unresolved with nonexistent reference' '
> + cat >expect <<-EOF &&
> + error: reference does not exist
> + EOF
> + test_expect_code 2 git show-ref --unresolved refs/heads/not-exist 2>err &&
> + test_cmp expect err
> +'
> +
> test_done
>
> base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
^ permalink raw reply
* Re: Clarify the meaning of "character" in the documentation
From: Junio C Hamano @ 2024-03-05 15:32 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Manlio Perillo, git
In-Reply-To: <5e10f1e5-b87f-43cd-ac1e-d7c01b7dad21@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
>> As an example, with
>> git config --add core.commentChar • // Bullet (U+2022)
>> git does not complain, but it is rejected later.
>
> I think this is more about `git config --add` not doing any
> validation. It just sets things. You can do `git config --add
> core.commentChar 'ffd'` and get the same effect.
This is not wrong per-se, but it merely explains why "config" takes
it without complaining (the command just does not know anything
about what each variable means and what the valid range of values
are). core.commentChar is limited to "a byte" so in the context of
everything else (like commit log message in the editor) being UTF-8,
it means ASCII would only work there.
As you said, we should document core.commentChar as limited to an
ASCII character, at least as a short term solution.
I personally do not see a reason, however, why we need to be limited
to a single byte, though. If a patch cleanly implements to allow us
to use any one-or-more-byte sequence as core.commentChar, I do not
offhand see a good reason to reject it---it would be fully backward
compatible and allows you to use a UTF-8 charcter outside ASCII, as
well as "//" and the like.
> Alphanumeric characters (a-z and A-Z and 0-9) are ASCII. And one ASCII
> char is represented using one byte in UTF-8. This already looks precise
> to me.
Correct.
> I’ve never run into a case where git-diff(1) does not handle UTF-8. I
> don’t even know if it really needs to “handle” it per se as opposed to
> just treating it as opaque bytes. Maybe it matters for things like
> whitespace and word-boundaries, I don’t know.
The core part of "diff" is very much line oriented, and after
chopping your random sequence of bytes at each LF that appears in
it, the code is pretty oblivious to the character boundary, except
for a few cases. "-w" needs to know what the whitespace characters
are (it knows only the limited basic set like SP HT and probably
VT), "-i" needs to know that "A" and "a" are equivalent (I think it
only knows the ASCII, but I may be misremembering). Outside the
core part of "diff", there are frills that need to know about
character boundaries, like chopping the function header comment
placed on a hunk header "@@ -1682,7 +1682,7 @@" to a reasonable
length, --color-words/--word-diff that first separates lines into
multi-character tokens and align matching sequences in them, etc.
^ permalink raw reply
* Re: Clarify the meaning of "character" in the documentation
From: Dragan Simic @ 2024-03-05 15:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kristoffer Haugsbakk, Manlio Perillo, git
In-Reply-To: <xmqqsf1466eh.fsf@gitster.g>
On 2024-03-05 16:32, Junio C Hamano wrote:
> "Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
>> I think this is more about `git config --add` not doing any
>> validation. It just sets things. You can do `git config --add
>> core.commentChar 'ffd'` and get the same effect.
>
> As you said, we should document core.commentChar as limited to an
> ASCII character, at least as a short term solution.
>
> I personally do not see a reason, however, why we need to be limited
> to a single byte, though. If a patch cleanly implements to allow us
> to use any one-or-more-byte sequence as core.commentChar, I do not
> offhand see a good reason to reject it---it would be fully backward
> compatible and allows you to use a UTF-8 charcter outside ASCII, as
> well as "//" and the like.
May I ask why would we want the comment character to possibly be
a multibyte character? I mean, I support localization, to make it all
easier for the users who opt not to use English, but wouldn't allowing
multibyte characters for the comment character simply be a bit unneeded?
Maybe I'm missing something?
^ permalink raw reply
* Re: [PATCH 1/2] refs/reftable: don't fail empty transactions in repo without HEAD
From: Junio C Hamano @ 2024-03-05 15:59 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, git, Mike Hommey
In-Reply-To: <ZecFXXqUdGEQ3YhC@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> So it's not uninitialized, it rather is stale.
What I wanted to say with "technically it is not" was that these two
are moral equivalents in the code around there after the early return
happened ;-).
> I can send a follow-up patch to remove the useless variable assignment
> in the test, but other than that I don't think anything needs to change
> here.
OK, thanks.
^ permalink raw reply
* Re: [PATCH v3 1/5] t3200: improve test style
From: Junio C Hamano @ 2024-03-05 16:02 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <166d2baa-933c-44f8-b6fb-94c8bce63a86@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
>>> -mv .git/config .git/config-saved
>>> -
>>> test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' '
>>> + test_when_finished mv .git/config-saved .git/config &&
>>> + mv .git/config .git/config-saved &&
>>> git branch -m q q2 &&
>>> git branch -m q2 q
>>> '
>>>
>>> -mv .git/config-saved .git/config
>>
>> The above is a truly valuable clean-up.
>>
>> But I am not really sure if the paritcular condition is worth
>> testing in the first place these days. No configuration file means
>> we cannot even read the repository format version, and working under
>> such a condition is quite a bad promise that we would rather not to
>> having to keep. But that is an entirely different topic from what
>> this patch is doing.
>
> Okay. I could undo this change and remove the test in its own commit?
No, please keep it.
I think removing it is totally outside the scope of this series. We
do preliminary clean-up in various areas, so that the last step can
do the advise thing for "git branch". In the context of the series,
removing this test does not fit anywhere. It is not a clean-up like
any other preliminary steps.
Thanks.
^ permalink raw reply
* Re: Should --update-refs exclude refs pointing to the current HEAD?
From: Junio C Hamano @ 2024-03-05 16:22 UTC (permalink / raw)
To: Stefan Haller
Cc: git, Derrick Stolee, Elijah Newren, Phillip Wood,
Christian Couder
In-Reply-To: <354f9fed-567f-42c8-9da9-148a5e223022@haller-berlin.de>
Stefan Haller <lists@haller-berlin.de> writes:
>> Both of these cases could be fixed by --update-refs not touching any
>> refs that point to the current HEAD. I'm having a hard time coming up
>> with cases where you would ever want those to be updated, in fact.
The point of "update-refs", as I understand it, is that in addition
to the end point of the history (E in "git rebase --onto N O E"),
any branch tips that are between O..E can be migrated to point at
their rewritten counterparts. So I am not sure how it fundamentally
solves much by protecting only refs that point at a single commit
("the current HEAD" in your statement).
When I want to see how the rebased history would look like without
touching the original, I often rebase a detached HEAD (i.e. instead
of the earlier one, use "git rebase --onto N O E^0", or when
rebasing the current branch, "git rebase [--onto N] O HEAD^0") and
that would protect the current branch well, but --update-refs of
course would not work well. There is no handy place like detached
HEAD that can be used to save rewritten version of these extra
branch tips.
If branch tips A, B, and C are involved in the range of commits
being rewritten, one way to help us in such a situation may be to
teach "git rebase" to (1) somehow create a new set of proposed-A,
proposed-B, and proposed-C refs (they do not have to be branches),
while keeping the original A, B, and C intact, (2) allow us to
inspect the resulting refs, compare the corresponding ones from
these two sets, and (3) allow us to promote (possibly a subset of)
proposed- ones to their counterpart real branches after we inspect
them. The latter two do not have to be subcommands of "git rebase"
but can be separate and new commands.
^ permalink raw reply
* Re: Clarify the meaning of "character" in the documentation
From: Kristoffer Haugsbakk @ 2024-03-05 16:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kristoffer Haugsbakk, Manlio Perillo, git
In-Reply-To: <xmqqsf1466eh.fsf@gitster.g>
On Tue, Mar 5, 2024, at 16:32, Junio C Hamano wrote:
> "Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
>
>>> As an example, with
>>> git config --add core.commentChar • // Bullet (U+2022)
>>> git does not complain, but it is rejected later.
>>
>> I think this is more about `git config --add` not doing any
>> validation. It just sets things. You can do `git config --add
>> core.commentChar 'ffd'` and get the same effect.
>
> This is not wrong per-se, but it merely explains why "config" takes
> it without complaining (the command just does not know anything
> about what each variable means and what the valid range of values
> are). core.commentChar is limited to "a byte" so in the context of
> everything else (like commit log message in the editor) being UTF-8,
> it means ASCII would only work there.
Yep, I neglected to mention that part.
> I personally do not see a reason, however, why we need to be limited
> to a single byte, though. If a patch cleanly implements to allow us
> to use any one-or-more-byte sequence as core.commentChar, I do not
> offhand see a good reason to reject it---it would be fully backward
> compatible and allows you to use a UTF-8 charcter outside ASCII, as
> well as "//" and the like.
Allow one codepoint or a string? Since a Unicode “character” can be
composed of multiple codepoints. And at that point it might be more work
to validate that it is a “character” compared to allowing any kind of
string.
Maybe introduce `core.commentString` and make it a synonym for
`core.commentChar`?
> The core part of "diff" is very much line oriented, and after
> chopping your random sequence of bytes at each LF that appears in
> it, the code is pretty oblivious to the character boundary, except
> for a few cases. "-w" needs to know what the whitespace characters
> are (it knows only the limited basic set like SP HT and probably
> VT), "-i" needs to know that "A" and "a" are equivalent (I think it
> only knows the ASCII, but I may be misremembering). Outside the
> core part of "diff", there are frills that need to know about
> character boundaries, like chopping the function header comment
> placed on a hunk header "@@ -1682,7 +1682,7 @@" to a reasonable
> length, --color-words/--word-diff that first separates lines into
> multi-character tokens and align matching sequences in them, etc.
Ah, interesting. Thanks :)
> As you said, we should document core.commentChar as limited to an
> ASCII character, at least as a short term solution.
Aha, I see now that the config documentation doesn’t make that clear.
-- >8 --
Subject: [PATCH] config: document `core.commentChar` as ASCII-only
d3b3419f8f2 (config: tell the user that we expect an ASCII character,
2023-03-27) updated an error message to make clear that this option
specifically wants an ASCII character but neglected to consider the
config documentation.
Reported-by: Manlio Perillo <manlio.perillo@gmail.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
I didn’t find any other relevant occurences with
git grep 'commentChar' -- ':(exclude)po'
`Documentation/git-commit.txt` mentions it but it doesn’t seem like a
clarification is needed in that context.
Documentation/config/core.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index 0e8c2832bf9..2d4bbdb25fa 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -521,7 +521,7 @@ core.editor::
core.commentChar::
Commands such as `commit` and `tag` that let you edit
- messages consider a line that begins with this character
+ messages consider a line that begins with this ASCII character
commented, and removes them after the editor returns
(default '#').
+
--
2.44.0.64.g52b67adbeb2
^ permalink raw reply related
* Re: Clarify the meaning of "character" in the documentation
From: Kristoffer Haugsbakk @ 2024-03-05 16:58 UTC (permalink / raw)
To: Dragan Simic; +Cc: Manlio Perillo, git, Junio C Hamano
In-Reply-To: <52d6850914982ffaf15dda937d611ffb@manjaro.org>
On Tue, Mar 5, 2024, at 16:42, Dragan Simic wrote:
>
> May I ask why would we want the comment character to possibly be
> a multibyte character? I mean, I support localization, to make it all
> easier for the users who opt not to use English, but wouldn't allowing
> multibyte characters for the comment character simply be a bit unneeded?
>
> Maybe I'm missing something?
Personally I think it’s okay. `%` for example is a good candidate since
you seldom use that as a leading character in prose (after a
whitespace), and it seems that `%` is often recommended as an
alternative.
But if it doesn’t make the code more complex: why not? (I just
personally don’t have a use-case.)
^ permalink raw reply
* Re: [PATCH] show-ref: add --unresolved option
From: Kristoffer Haugsbakk @ 2024-03-05 17:01 UTC (permalink / raw)
To: Phillip Wood; +Cc: John Cai, Josh Soref, git
In-Reply-To: <a3de2b7b-4603-4604-a4d2-938a598e312e@gmail.com>
On Tue, Mar 5, 2024, at 16:30, Phillip Wood wrote:
> Hi John
>
> On 04/03/2024 22:51, John Cai via GitGitGadget wrote:
>> From: John Cai <johncai86@gmail.com>
>>
>> For reftable development, it would be handy to have a tool to provide
>> the direct value of any ref whether it be a symbolic ref or not.
>> Currently there is git-symbolic-ref, which only works for symbolic refs,
>> and git-rev-parse, which will resolve the ref. Let's add a --unresolved
>> option that will only take one ref and return whatever it points to
>> without dereferencing it.
>
> "--unresolved" makes me think of merge conflicts. I wonder if
> "--no-dereference" would be clearer.
Yeah, a `--no`-style option looks more consistent.
^ 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