From: Han-Wen Nienhuys via GitGitGadget <hidden> Date: 2021-11-22 14:21:00
From: Han-Wen Nienhuys <redacted>
Before, --reflog option would look for '\t' in the reflog message. As refs.c
already parses the reflog line, the '\t' was never found, and show-branch
--reflog would always say "(none)" as reflog message
Add test.
Signed-off-by: Han-Wen Nienhuys <redacted>
---
builtin/show-branch.c | 12 +++++++-----
t/t3202-show-branch.sh | 15 +++++++++++++++
2 files changed, 22 insertions(+), 5 deletions(-)
From: Han-Wen Nienhuys via GitGitGadget <hidden> Date: 2021-11-22 14:21:01
From: Han-Wen Nienhuys <redacted>
Commit 523fa69c ("reflog: cleanse messages in the refs.c layer") standardizes
how write entries into the reflog. This commit standardizes how we get messages
out of the reflog. Before, the files backend implicitly added '\n' to the end of
reflog message on reading, which creates a subtle incompatibility with alternate
ref storage backends, such as reftable.
We address this by stripping LF from the message before we pass it to the
user-provided callback.
Signed-off-by: Han-Wen Nienhuys <redacted>
---
builtin/show-branch.c | 5 -----
reflog-walk.c | 6 ++----
refs/files-backend.c | 30 +++++++++++++++---------------
t/t1405-main-ref-store.sh | 5 ++---
t/t1406-submodule-ref-store.sh | 4 ++--
5 files changed, 21 insertions(+), 29 deletions(-)
@@ -1936,17 +1936,15 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *cinttz;constchar*p=sb->buf;-/* old SP new SP name <email> SP time TAB msg LF */-if(!sb->len||sb->buf[sb->len-1]!='\n'||-parse_oid_hex(p,&ooid,&p)||*p++!=' '||+/* old SP new SP name <email> SP time TAB msg */+if(!sb->len||parse_oid_hex(p,&ooid,&p)||*p++!=' '||parse_oid_hex(p,&noid,&p)||*p++!=' '||-!(email_end=strchr(p,'>'))||-email_end[1]!=' '||+!(email_end=strchr(p,'>'))||email_end[1]!=' '||!(timestamp=parse_timestamp(email_end+2,&message,10))||!message||message[0]!=' '||-(message[1]!='+'&&message[1]!='-')||-!isdigit(message[2])||!isdigit(message[3])||-!isdigit(message[4])||!isdigit(message[5]))+(message[1]!='+'&&message[1]!='-')||!isdigit(message[2])||+!isdigit(message[3])||!isdigit(message[4])||+!isdigit(message[5]))return0;/* corrupt? */email_end[1]='\0';tz=strtol(message+1,NULL,10);
@@ -2038,6 +2036,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,strbuf_splice(&sb,0,0,bp+1,endp-(bp+1));scanp=bp;endp=bp+1;+strbuf_trim_trailing_newline(&sb);ret=show_one_reflog_ent(&sb,fn,cb_data);strbuf_reset(&sb);if(ret)
@@ -2050,6 +2049,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,*Processit,andwecanendtheloop.*/strbuf_splice(&sb,0,0,buf,endp-buf);+strbuf_trim_trailing_newline(&sb);ret=show_one_reflog_ent(&sb,fn,cb_data);strbuf_reset(&sb);break;
@@ -2099,7 +2099,7 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,if(!logfp)return-1;-while(!ret&&!strbuf_getwholeline(&sb,logfp,'\n'))+while(!ret&&!strbuf_getline(&sb,logfp))ret=show_one_reflog_ent(&sb,fn,cb_data);fclose(logfp);strbuf_release(&sb);
From: Han-Wen Nienhuys via GitGitGadget <hidden> Date: 2021-11-22 14:21:02
From: Han-Wen Nienhuys <redacted>
Follow the reflog format more closely, so it can be used for comparing
reflogs in tests without using inspecting files under .git/logs/
Signed-off-by: Han-Wen Nienhuys <redacted>
---
t/helper/test-ref-store.c | 5 ++---
t/t1405-main-ref-store.sh | 1 +
2 files changed, 3 insertions(+), 3 deletions(-)
From: Han-Wen Nienhuys via GitGitGadget <hidden> Date: 2021-11-22 14:21:06
From: Han-Wen Nienhuys <redacted>
This avoids inspecting the file system, which only works with the files ref
backend.
Signed-off-by: Han-Wen Nienhuys <redacted>
---
t/t1400-update-ref.sh | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
So preceding commits added a trailing tab, or was that always the case
with the alternate test utility?
log_ref_write_fd() only writes \t into the log message if there is a
non-empty message, and previously this was checking the log file
directly. The test helper tweaked in one of the previous commits
unconditionally prints a \t, which we now have to deal with.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
From: Junio C Hamano <hidden> Date: 2021-11-22 22:23:12
"Han-Wen Nienhuys via GitGitGadget" [off-list ref] writes:
From: Han-Wen Nienhuys <redacted>
Before, --reflog option would look for '\t' in the reflog message. As refs.c
already parses the reflog line, the '\t' was never found, and show-branch
--reflog would always say "(none)" as reflog message
Well spotted. It may show that nobody pays attention to output from
"show-branch -g" (or nobody runs it in the first place), but it is
good to fix it anyway.
Thanks.
From: Junio C Hamano <hidden> Date: 2021-11-22 22:28:00
"Han-Wen Nienhuys via GitGitGadget" [off-list ref] writes:
From: Han-Wen Nienhuys <redacted>
Commit 523fa69c ("reflog: cleanse messages in the refs.c layer") standardizes
how write entries into the reflog. This commit standardizes how we get messages
out of the reflog. Before, the files backend implicitly added '\n' to the end of
reflog message on reading, which creates a subtle incompatibility with alternate
ref storage backends, such as reftable.
We address this by stripping LF from the message before we pass it to the
user-provided callback.
If this were truly "user-provided", then I'd argue that all backends
should follow whatever the files backend has been doing forever---if
the files added LF implicitly, others should, too, because that is
pretty much what these "user-provided" callbacks have been expecting
to see.
In other words, it would be a bug for newer backends to behave
differently.
But I _think_ these callbacks are all under our control, and if that
is the case, I am fine either way, even though I would have a strong
preference not to have to change the API without a good reason, even
if it is a purely internal one.
So, let's go and see if we can find a good reason in the changes we
can make to the callback functions ;-)
- end = strchr(logmsg, '\n');
- if (end)
- *end = '\0';
-
We could argue that the lack of LF at the end from the API output
made this caller simpler, which may be a plus.
@@ -1936,17 +1936,15 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *cinttz;constchar*p=sb->buf;-/* old SP new SP name <email> SP time TAB msg LF */-if(!sb->len||sb->buf[sb->len-1]!='\n'||-parse_oid_hex(p,&ooid,&p)||*p++!=' '||+/* old SP new SP name <email> SP time TAB msg */+if(!sb->len||parse_oid_hex(p,&ooid,&p)||*p++!=' '||parse_oid_hex(p,&noid,&p)||*p++!=' '||-!(email_end=strchr(p,'>'))||-email_end[1]!=' '||+!(email_end=strchr(p,'>'))||email_end[1]!=' '||!(timestamp=parse_timestamp(email_end+2,&message,10))||!message||message[0]!=' '||-(message[1]!='+'&&message[1]!='-')||-!isdigit(message[2])||!isdigit(message[3])||-!isdigit(message[4])||!isdigit(message[5]))+(message[1]!='+'&&message[1]!='-')||!isdigit(message[2])||+!isdigit(message[3])||!isdigit(message[4])||+!isdigit(message[5]))return0;/* corrupt? */email_end[1]='\0';tz=strtol(message+1,NULL,10);
@@ -2038,6 +2036,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,strbuf_splice(&sb,0,0,bp+1,endp-(bp+1));scanp=bp;endp=bp+1;+strbuf_trim_trailing_newline(&sb);ret=show_one_reflog_ent(&sb,fn,cb_data);strbuf_reset(&sb);if(ret)
@@ -2050,6 +2049,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,*Processit,andwecanendtheloop.*/strbuf_splice(&sb,0,0,buf,endp-buf);+strbuf_trim_trailing_newline(&sb);ret=show_one_reflog_ent(&sb,fn,cb_data);strbuf_reset(&sb);break;
@@ -2099,7 +2099,7 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,if(!logfp)return-1;-while(!ret&&!strbuf_getwholeline(&sb,logfp,'\n'))+while(!ret&&!strbuf_getline(&sb,logfp))ret=show_one_reflog_ent(&sb,fn,cb_data);fclose(logfp);strbuf_release(&sb);
I got an impression from the proposed log message and the changes to
the code (except for the refs/files-backend.c, which I only skimmed)
that the idea is that the refs API stops adding LF at the end, and
the callers got a matching change to compensate for the (now)
missing LF. If that is the idea behind the change, why do we need
to change any existing test? The only way any tests need to be
modified due to such a change I can think of is when we forget to or
failed to make compensating change to the callers of the API.
Puzzled...
Ah, the $RUN is hiding what is really going on; it is running the
"test-tool ref-store" helper, and we did not adjust that helper. So
if we make a compensating change to the test-tool then we do not
have to have these changes at all? But that point may be moot.
In any case, in order to lose 5 lines from show-branch.c, and 2
lines from reflog-walk.c, I see that we had to touch 30+ lines in
refs/files-backend.c. I find it a bit hard to sell this as an
improvement to the API, to be honest.
Luckily, it looks to me that this step is mostly unreleated to the
main thrust of these patches in the series, which is "reading
.git/logs/ in the test would work only when testing files backend;
use for-each-ref test helper to recreate what would have been read
by such tests from the files backend's files and inspect that
instead, and that would allow us test other backends for free".
So I suspect that this step can be safely dropped?
Thanks.
From: Junio C Hamano <hidden> Date: 2021-11-22 22:31:33
"Han-Wen Nienhuys via GitGitGadget" [off-list ref] writes:
From: Han-Wen Nienhuys <redacted>
Follow the reflog format more closely, so it can be used for comparing
There is no v$n designator on the title line, but I have this
feeling that I've seen this patch before. More importantly, I
remember that I found it unclear what you exactly mean "the" reflog
format. Is that what the files backend stores on one line in its
file? The reason I suspect that may be the answer is because I do
not recall documenting "the" reflog format in Documentation/ and
whatever we have historically been writing would be the most
canonical and/or authoritative format.
reflogs in tests without using inspecting files under .git/logs/
I agree 100% with the goal.
It seems that one line of .git/logs/HEAD looks like
<new> SP <old> SP <user> SP '<' <email> '>' SP <time> SP <zone> HT <oneline> LF
and being able to extract a line like that for given reflog entry
out of any backend in a consistent way is valuable when testing
different backends.
It seems that is what the new code is writing, so perhaps the first
paragraph can be clarified to indicate as such.
We have some tests that read from files in .git/logs/ hierarchy
when checking if correct reflog entries are created, but that is
too specific to the files backend. Other backends like reftable
may not store its reflog entries in such a "one line per entry"
format.
Update for-each-reflog-ent test helper to produce output that
would be identical to lines in a reflog file files backend uses.
That way, (1) the current tests can be updated to use the test
helper to read the reflog entries instead of (parts of) reflog
files, and perform the same inspection for correctness, and (2)
when the ref backend is swapped to another backend, the updated
test can be used as-is to check the correctness.
or something along the line?
Looks good to me. We might want to make the printf format
conditional to add \t%s only when msg is not empty, though.
Hopefully such a change would follow the reflog format even more
closely to make 4/4 unnecessary?
I am not sure how this new test helps validate the change to the
code.
What was different in the old output was that the timezone was not
in %+05d format, and the field separator before the log message was
not HT. So if this grepped for HT or +0000 to make sure we are
using the format that is close to what actually is stored in the
files, I would understand this change, but it is unclear what it
proves to make sure that the oldest entry has recreate-main.
In fact, with the code part of the patch reverted, this new test
seems to pass.
On 22/11/21 21.20, Han-Wen Nienhuys via GitGitGadget wrote:
From: Han-Wen Nienhuys <redacted>
Before, --reflog option would look for '\t' in the reflog message. As refs.c
already parses the reflog line, the '\t' was never found, and show-branch
--reflog would always say "(none)" as reflog message
Add test.
On Mon, Nov 22, 2021 at 11:44 PM Bagas Sanjaya [off-list ref] wrote:
On 22/11/21 21.20, Han-Wen Nienhuys via GitGitGadget wrote:
quoted
From: Han-Wen Nienhuys <redacted>
Before, --reflog option would look for '\t' in the reflog message. As refs.c
already parses the reflog line, the '\t' was never found, and show-branch
--reflog would always say "(none)" as reflog message
Add test.
I haven't looked deeply into this topic as a whole, but FWIW this
conflicts with a topic I've got locally and was going to submit sometime
after the current batch of my own ref fixes in "next".
That we've got verbose output at all in file-backend.c is a wart, and it
should just be moved out if it entirely, this commit (stacked on top of
various other fixes I've got in the area) does that:
https://github.com/avar/git/commit/eff40d2d81b
With that change the reftable code will need to handle far less of this,
as it'll be handled in builtin/reflog.c, it just needs to behave
properly in the appropriate "expire reflog?" callback. I.e. the backend
tells us "does this expire?", the builtin/reflog.c code consumes that
and does any verbose or non-verbose (or progress etc.) output.
Now, the merge conflict between that and this looks rather trivial, but
I can't help but wonder if we're going in the wrong direction here
API-wise, or maybe "wrong direction" is too strong, but "sticking with
the wrong patterN?".
I think your cleanup works, but wouldn't a better thing be to move this
to callbacks rather than tweaking the fprintf formats?
I.e. in my version the whole body of this function has become:
static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
const char *email, timestamp_t timestamp, int tz,
const char *message, void *cb_data)
{
struct expire_reflog_cb *cb = cb_data;
reflog_expiry_should_prune_fn *fn = cb->should_prune_fn;
if (cb->rewrite)
ooid = &cb->last_kept_oid;
if (fn(ooid, noid, email, timestamp, tz, message, cb->policy_cb))
return 0;
if (cb->dry_run)
return 0; /* --dry-run */
fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s", oid_to_hex(ooid),
oid_to_hex(noid), email, timestamp, tz, message);
oidcpy(&cb->last_kept_oid, noid);
return 0;
}
The only file-backend specific part of it is that fprintf(). If we moved
towards making that part of it be:
write_reflog_entry_cb(oid_to_hex(ooid), oid_to_hex(noid), email, timestamp, tz, message);
Then we could lift the whole of this API to a level that makes more
sense for a backed to implement.
The refs/files-backend.c shouldn't need to have one function calll the
"should_prune_fn" *and* write out the data. Instead some code common to
all backends should call the "should prune?", and then call the
backend's "here's an entry for you to write" callback.
But maybe I'm overthinking this whole thing. I'm just wondering if we
have say a sqlite reflog backend as opposed to the file/reftable backend
that wants to store this data in a schema. Such a a backend would need
to unpack the data, as we're sprintf() formatting function parameters
before it gets passed to the backends.
On Mon, Nov 22, 2021 at 11:27 PM Junio C Hamano [off-list ref] wrote:
"Han-Wen Nienhuys via GitGitGadget" [off-list ref] writes:
quoted
From: Han-Wen Nienhuys <redacted>
Commit 523fa69c ("reflog: cleanse messages in the refs.c layer") standardizes
how write entries into the reflog. This commit standardizes how we get messages
out of the reflog. Before, the files backend implicitly added '\n' to the end of
reflog message on reading, which creates a subtle incompatibility with alternate
ref storage backends, such as reftable.
We address this by stripping LF from the message before we pass it to the
user-provided callback.
If this were truly "user-provided", then I'd argue that all backends
should follow whatever the files backend has been doing forever---if
the files added LF implicitly, others should, too, because that is
pretty much what these "user-provided" callbacks have been expecting
to see.
I think it's just wrong. If you pass `msg` to a storage API, you
should get `msg` when you read it back, not (msg + "\n").
Ah, the $RUN is hiding what is really going on; it is running the
"test-tool ref-store" helper, and we did not adjust that helper. So
if we make a compensating change to the test-tool then we do not
have to have these changes at all? But that point may be moot.
In any case, in order to lose 5 lines from show-branch.c, and 2
lines from reflog-walk.c, I see that we had to touch 30+ lines in
refs/files-backend.c. I find it a bit hard to sell this as an
improvement to the API, to be honest.
The test-tool ref-store adds its own '\n', so you always get a blank
line in the output. That serves no purpose, and leads to the
tail-n2 | head -n1
in order to read the last log line. I think it's silly, and should be dropped.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
On Tue, Nov 23, 2021 at 11:40 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
I think your cleanup works, but wouldn't a better thing be to move this
to callbacks rather than tweaking the fprintf formats?
sure. In the reftable glue, I have
if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
/* XXX - skip writing records that were not changed. */
err = reftable_addition_commit(add);
} else {
/* XXX - print something */
}
letting the callbacks do the printing means less work for reftable.
The refs/files-backend.c shouldn't need to have one function calll the
"should_prune_fn" *and* write out the data. Instead some code common to
all backends should call the "should prune?", and then call the
backend's "here's an entry for you to write" callback.
not sure if that will work. For reftable, you have to write something
(a tombstone) if you _do_ want to prune the entry.
But maybe I'm overthinking this whole thing. I'm just wondering if we
have say a sqlite reflog backend as opposed to the file/reftable backend
that wants to store this data in a schema. Such a a backend would need
reftable also stores this in a schema: there are separate fields for
e-mail, timezone, timestamp etc.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Looks good to me. We might want to make the printf format
conditional to add \t%s only when msg is not empty, though.
Hopefully such a change would follow the reflog format even more
closely to make 4/4 unnecessary?
I think the conditional formatting of \t is impractical. It makes things like
(metadata, msg) = line.split('\t')
in Python require special casing in case msg is empty.
I am not sure how this new test helps validate the change to the
code.
It's for consistency with the preceding test. I can make a separate commit.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
From: Han-Wen Nienhuys via GitGitGadget <hidden> Date: 2021-11-25 16:10:17
From: Han-Wen Nienhuys <redacted>
If we are checking for a certain ordering, we should check that there are two
entries. Do this by mirroring the preceding test.
Signed-off-by: Han-Wen Nienhuys <redacted>
---
t/t1405-main-ref-store.sh | 1 +
1 file changed, 1 insertion(+)
From: Han-Wen Nienhuys via GitGitGadget <hidden> Date: 2021-11-25 16:10:21
From: Han-Wen Nienhuys <redacted>
On iteration, the reflog message is always terminated by a newline. Trim it to
avoid clobbering the console with is this extra newline.
Signed-off-by: Han-Wen Nienhuys <redacted>
---
refs/debug.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
From: Han-Wen Nienhuys via GitGitGadget <hidden> Date: 2021-11-25 16:10:38
From: Han-Wen Nienhuys <redacted>
Before, --reflog option would look for '\t' in the reflog message. As refs.c
already parses the reflog line, the '\t' was never found, and show-branch
--reflog would always say "(none)" as reflog message
Add test.
Signed-off-by: Han-Wen Nienhuys <redacted>
---
builtin/show-branch.c | 12 +++++++-----
t/t3202-show-branch.sh | 15 +++++++++++++++
2 files changed, 22 insertions(+), 5 deletions(-)
From: Han-Wen Nienhuys via GitGitGadget <hidden> Date: 2021-11-25 16:10:43
From: Han-Wen Nienhuys <redacted>
We have some tests that read from files in .git/logs/ hierarchy
when checking if correct reflog entries are created, but that is
too specific to the files backend. Other backends like reftable
may not store its reflog entries in such a "one line per entry"
format.
Update for-each-reflog-ent test helper to produce output that
is identical to lines in a reflog file files backend uses.
That way, (1) the current tests can be updated to use the test
helper to read the reflog entries instead of (parts of) reflog
files, and perform the same inspection for correctness, and (2)
when the ref backend is swapped to another backend, the updated
test can be used as-is to check the correctness.
Adapt t1400 to use the for-each-reflog-ent test helper.
Signed-off-by: Han-Wen Nienhuys <redacted>
---
t/helper/test-ref-store.c | 5 +++--
t/t1400-update-ref.sh | 13 ++++++++-----
2 files changed, 11 insertions(+), 7 deletions(-)
On Thu, Nov 25 2021, Han-Wen Nienhuys via GitGitGadget wrote:
This helps for reftable support, and will help if we want to reconsider
under which conditions reflogs get created/updated.
Having looked at this in a bit more detail than last time
(https://lore.kernel.org/git/211123.864k83w3y4.gmgdl@evledraar.gmail.com/)
I applaud the goals, but to be blunt the specific approach just seems a
bit backwards to me.
As noted in that message I have patches to tweak the "verbose" mode to
be backend-independent, which as we see from your series is one thing in
the files backend that consumes the "message" and assumes things about
newlines.
Similarly from peeking at the reflog code I see it stores name/email
separately in a struct, but the refs.[ch] API wants to pass "name
<email>" as one string.
The below working patch shows that we have three consumers of that
specific format, so we can just xstrfmt() that instead of xstrdup()-ing
it in one case, and tweak the printf in the other two, and then we pass
it as two parameters.
So it's truly backend-independent now, i.e. a "real DB" like reftable,
SQL or whatever would store those in two separate fields.
So on the "seems backwards" above: If you apply this (and it passes all
tests) and look at what "message" is used for you're guaranteed to find
all the callers, and only a handful more of them are using that for
anything.
So I'd really expect a series like this to just change the
strbuf_getwholeline() in files_for_each_reflog_ent() to be a
strbuf_getline(). Then flip everything downstream that expects a '\n' to
either not, or to add if needed.
E.g. the test helper that's now adding a newline would be correct, and
we'd just need to tweak the file backend specific stuff that's now
skipping the addition of newlines to add them instead.
Anyway, changing the callbacks is quite verbose, and I just did this
ident->name/email change expecting that it would be more difficult.
But likewise the big benefit is that we *are* forced to tweak all
callers, so we're more likely to catch any subtle regressions, and it's
thus easier to review once we get past the verbosity.
I also wonder if for name/email/message a bit more tweaking of this to
make them all const char */size_t pairs wouldn't result in a much better
end-state. I.e. now we read into a strbuf in a loop in files-backend.c
and xstrdup() it.
Perhaps reftable is capable of just handing the underlying code pointers
into the mmap()'d file, so we could even skip all (re)allocations? Or if
not, that certainly seems like a sensible thing to anticipate in a
backend-independent interface. We could do that in the file backend if
we were a bit smarter and used mmap() instead of the
fopen()/read-in-a-loop pattern.
Just my 0.02, if you're interested in running with the below assume my
SOB.
@@ -463,8 +463,9 @@ int delete_reflog(const char *refname);*/typedefinteach_reflog_ent_fn(structobject_id*old_oid,structobject_id*new_oid,-constchar*committer,timestamp_ttimestamp,-inttz,constchar*msg,void*cb_data);+constchar*committer_name,constchar*committer_email,+timestamp_ttimestamp,inttz,+constchar*msg,void*cb_data);/* Iterate over reflog entries in the log for `refname`. */
@@ -1948,13 +1953,20 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c!isdigit(message[2])||!isdigit(message[3])||!isdigit(message[4])||!isdigit(message[5]))return0;/* corrupt? */-email_end[1]='\0';+/* \0 the SP before "<", marks end of "name"... */+*(email-1)='\0';+/* ... and advance past the opening... "<" */+email++;+/* ...and stop at the ">" */+email_end[0]='\0';+tz=strtol(message+1,NULL,10);if(message[6]!='\t')message+=6;elsemessage+=7;-returnfn(&ooid,&noid,p,timestamp,tz,message,cb_data);+returnfn(&ooid,&noid,name,email,timestamp,tz,message,+cb_data);}staticchar*find_beginning_of_line(char*bob,char*scan)
@@ -2428,8 +2428,9 @@ struct check_and_collect_until_cb_data {/* Get the timestamp of the latest entry. */staticintpeek_reflog(structobject_id*o_oid,structobject_id*n_oid,-constchar*ident,timestamp_ttimestamp,-inttz,constchar*message,void*cb_data)+constchar*committer_name,constchar*committer_email,+timestamp_ttimestamp,inttz,+constchar*message,void*cb_data){timestamp_t*ts=cb_data;*ts=timestamp;
On Mon, Nov 29, 2021 at 11:14 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
This helps for reftable support, and will help if we want to reconsider
under which conditions reflogs get created/updated.
Having looked at this in a bit more detail than last time
(https://lore.kernel.org/git/211123.864k83w3y4.gmgdl@evledraar.gmail.com/)
I applaud the goals, but to be blunt the specific approach just seems a
bit backwards to me.
As noted in that message I have patches to tweak the "verbose" mode to
be backend-independent, which as we see from your series is one thing in
the files backend that consumes the "message" and assumes things about
newlines.
In v2, I went with Jun's suggestion, and left the newlines alone, just
trimming them in refs/debug.c . I think that makes most of your mail
irrelevant?
Perhaps reftable is capable of just handing the underlying code pointers
into the mmap()'d file, so we could even skip all (re)allocations? Or if
not, that certainly seems like a sensible thing to anticipate in a
backend-independent interface. We could do that in the file backend if
we were a bit smarter and used mmap() instead of the
fopen()/read-in-a-loop pattern.
It sounds like premature optimization. Reading reflogs is not usually
a performance sensitive operation. Also, if you hand out mmap'd
pointers, how would the reftable storage code know when it is safe to
close and munmap the file?
Just my 0.02, if you're interested in running with the below assume my
SOB.
What is SOB in this context?
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
On Mon, Nov 29, 2021 at 11:14 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
quoted
This helps for reftable support, and will help if we want to reconsider
under which conditions reflogs get created/updated.
Having looked at this in a bit more detail than last time
(https://lore.kernel.org/git/211123.864k83w3y4.gmgdl@evledraar.gmail.com/)
I applaud the goals, but to be blunt the specific approach just seems a
bit backwards to me.
As noted in that message I have patches to tweak the "verbose" mode to
be backend-independent, which as we see from your series is one thing in
the files backend that consumes the "message" and assumes things about
newlines.
In v2, I went with Jun's suggestion, and left the newlines alone, just
trimming them in refs/debug.c . I think that makes most of your mail
irrelevant?
To whatever immediate problem you're trying to solve? Probably. I'm
mainly pointing out that we can make some of these APIs nicer & a bit
more abstract from the details of the file backend.
Or, as is the case with "ident" v.s. "committer_name/committer_email" in
these reflog callbacks no existing caller actually cares about that
format, so if/when we get to that reftable integration (which seems to
have the two as seperate fields) splitting those up in refs.[ch]
probably makes sense.
quoted
Perhaps reftable is capable of just handing the underlying code pointers
into the mmap()'d file, so we could even skip all (re)allocations? Or if
not, that certainly seems like a sensible thing to anticipate in a
backend-independent interface. We could do that in the file backend if
we were a bit smarter and used mmap() instead of the
fopen()/read-in-a-loop pattern.
It sounds like premature optimization. Reading reflogs is not usually
a performance sensitive operation. Also, if you hand out mmap'd
pointers, how would the reftable storage code know when it is safe to
close and munmap the file?
The same way we do the fopen/fclose lifetime now, i.e. you could rely on
them for the iteration, which seems to be a common pattern in code that
needs this.
I was aiming more for the lack of premature optimization there,
i.e. instead of byte-twiddling things by injecting \0s we could just
have char */size_t offsets (as noted elsewhere), which would also nicely
allow binary storage formats, if those formats happen to have an
embedded string. Is that true of reftable?
On Tue, Nov 30, 2021 at 12:41 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
I was aiming more for the lack of premature optimization there,
i.e. instead of byte-twiddling things by injecting \0s we could just
have char */size_t offsets (as noted elsewhere), which would also nicely
allow binary storage formats, if those formats happen to have an
embedded string. Is that true of reftable?
reftable zlib compresses the reflog, so you can't mmap strings from the storage.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
From: Han-Wen Nienhuys via GitGitGadget <hidden> Date: 2021-12-02 17:36:47
From: Han-Wen Nienhuys <redacted>
Before, --reflog option would look for '\t' in the reflog message. As refs.c
already parses the reflog line, the '\t' was never found, and show-branch
--reflog would always say "(none)" as reflog message
Add test.
Signed-off-by: Han-Wen Nienhuys <redacted>
---
builtin/show-branch.c | 12 +++++++-----
t/t3202-show-branch.sh | 15 +++++++++++++++
2 files changed, 22 insertions(+), 5 deletions(-)
From: Han-Wen Nienhuys via GitGitGadget <hidden> Date: 2021-12-02 17:36:48
From: Han-Wen Nienhuys <redacted>
On iteration, the reflog message is always terminated by a newline. Trim it to
avoid clobbering the console with is this extra newline.
Signed-off-by: Han-Wen Nienhuys <redacted>
---
refs/debug.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
From: Han-Wen Nienhuys via GitGitGadget <hidden> Date: 2021-12-02 17:36:49
From: Han-Wen Nienhuys <redacted>
We have some tests that read from files in .git/logs/ hierarchy
when checking if correct reflog entries are created, but that is
too specific to the files backend. Other backends like reftable
may not store its reflog entries in such a "one line per entry"
format.
Update for-each-reflog-ent test helper to produce output that
is identical to lines in a reflog file files backend uses.
That way, (1) the current tests can be updated to use the test
helper to read the reflog entries instead of (parts of) reflog
files, and perform the same inspection for correctness, and (2)
when the ref backend is swapped to another backend, the updated
test can be used as-is to check the correctness.
Adapt t1400 to use the for-each-reflog-ent test helper.
Signed-off-by: Han-Wen Nienhuys <redacted>
---
t/helper/test-ref-store.c | 5 +++--
t/t1400-update-ref.sh | 13 ++++++++-----
2 files changed, 11 insertions(+), 7 deletions(-)
From: Han-Wen Nienhuys via GitGitGadget <hidden> Date: 2021-12-02 17:36:51
From: Han-Wen Nienhuys <redacted>
If we are checking for a certain ordering, we should check that there are two
entries. Do this by mirroring the preceding test.
Signed-off-by: Han-Wen Nienhuys <redacted>
---
t/t1405-main-ref-store.sh | 1 +
1 file changed, 1 insertion(+)