While working on my commit-graph patch [1] and using a local build in my
usual workflows, I found a bug in my branch.
Essentially, when calling `git rev-list --header`, the header
information is actually missing for many commits except the first. This
was not caught in my testing since t6000-rev-list-misc.sh creates a test
repo with only one commit.
The root cause is that the serialized commit graph gets its speedup by
not loading buffers for every commit. For many use-cases (merge bases,
--topo-order, etc.) we do not need the buffer for most of the commits.
In the rev-list example, the buffer is loaded due to a side-effect of
being referenced by HEAD or a branch. A similar effect happened in `git
log`, hence the following change in my patch:
In rev-list, the "--header" option outputs a value and expects the
buffer to be cached. It outputs the header info only if
get_cached_commit_buffer() returns a non-null buffer, giving incorrect
output. If it called get_commit_buffer() instead, it would immediately
call get_cached_commit_buffer() and on failure actually load the buffer.
This has not been a problem before, since the buffer was always loaded
at some point for each commit (and saved because of the
save_commit_buffer global).
I propose to make get_cached_commit_buffer() static to commit.c and
convert all callers to get_commit_buffer(). Is there any reason to _not_
do this? It seems that there is no functional or performance change.
After the serialized commit graph exists and is used, the only change is
that we delay loading the buffer until we need to read the commit
metadata that is not stored in the graph (message, author/committer).
Thanks,
-Stolee
[1]
https://public-inbox.org/git/4d1ee202-7d79-d73c-6e05-d0fc85db943c@gmail.com/T/#m381bfd3f2eafbd254e35a5147cd198bc35055e92
[Patch v4 00/14] Serialized Git Commit Graph
[2]
https://public-inbox.org/git/20140610214039.GJ19147@sigill.intra.peff.net/
[Patch 10/17] provide helpers to access the commit buffer
From: Jeff King <hidden> Date: 2018-02-20 22:57:33
On Tue, Feb 20, 2018 at 05:12:50PM -0500, Derrick Stolee wrote:
In rev-list, the "--header" option outputs a value and expects the buffer to
be cached. It outputs the header info only if get_cached_commit_buffer()
returns a non-null buffer, giving incorrect output. If it called
get_commit_buffer() instead, it would immediately call
get_cached_commit_buffer() and on failure actually load the buffer.
This has not been a problem before, since the buffer was always loaded at
some point for each commit (and saved because of the save_commit_buffer
global).
I propose to make get_cached_commit_buffer() static to commit.c and convert
all callers to get_commit_buffer(). Is there any reason to _not_ do this? It
seems that there is no functional or performance change.
That helper was added in 152ff1cceb (provide helpers to access the
commit buffer, 2014-06-10). I think interesting part is the final
paragraph:
Note that we also need to add a "get_cached" variant which
returns NULL when we do not have a cached buffer. At first
glance this seems to defeat the purpose of "get", which is
to always provide a return value. However, some log code
paths actually use the NULL-ness of commit->buffer as a
boolean flag to decide whether to try printing the
commit. At least for now, we want to continue supporting
that use.
So I think a conversion to get_commit_buffer() would break the callers
that use the boolean nature for something useful. Unfortunately the
author did not enumerate exactly what those uses are, so we'll have to
dig. :)
My guess is that it has to do with showing some commits twice, since we
would normally have the buffer available the first time we hit the
commit, and then free it in finish_commit().
If we blame that rev-list line (and then walk back over a bunch of
uninteresting commits via parent re-blaming), it comes from 3131b71301
(Add "--show-all" revision walker flag for debugging, 2008-02-09).
So there it is. It does show commits multiple times, but suppresses the
verbose header after the first showing. If we do something like this:
git rev-list --show-all --pretty --boundary c93150cfb0^-
you'll see some boundary commits that _don't_ have their pretty headers
shown. And with your proposed patch, we'd show them again. To keep the
same behavior we need to store that "we've already seen this" boolean
somewhere else (e.g., in an object flag; possibly SEEN, but that might
run afoul of other logic).
It looks like the call in log-tree comes from the same commit, and
serves the same purpose.
Aside from storing the boolean "did we show it" in another way, the
other option is to simply change the behavior and accept that we might
pretty-print the commit twice. This is a backwards-incompatible change,
but I'm not sure if anybody would care. According to that commit,
--show-all was added explicitly for debugging, and it has never been
documented. I couldn't find any reference to people actually using it
on the list (a grep of the whole archive turns up 32 messages, most of
which are just it appearing in context; the only person mentioning its
actual use was Linus in 2008.
-Peff
On Tue, Feb 20, 2018 at 05:12:50PM -0500, Derrick Stolee wrote:
quoted
In rev-list, the "--header" option outputs a value and expects the buffer to
be cached. It outputs the header info only if get_cached_commit_buffer()
returns a non-null buffer, giving incorrect output. If it called
get_commit_buffer() instead, it would immediately call
get_cached_commit_buffer() and on failure actually load the buffer.
This has not been a problem before, since the buffer was always loaded at
some point for each commit (and saved because of the save_commit_buffer
global).
I propose to make get_cached_commit_buffer() static to commit.c and convert
all callers to get_commit_buffer(). Is there any reason to _not_ do this? It
seems that there is no functional or performance change.
That helper was added in 152ff1cceb (provide helpers to access the
commit buffer, 2014-06-10). I think interesting part is the final
paragraph:
Note that we also need to add a "get_cached" variant which
returns NULL when we do not have a cached buffer. At first
glance this seems to defeat the purpose of "get", which is
to always provide a return value. However, some log code
paths actually use the NULL-ness of commit->buffer as a
boolean flag to decide whether to try printing the
commit. At least for now, we want to continue supporting
that use.
So I think a conversion to get_commit_buffer() would break the callers
that use the boolean nature for something useful. Unfortunately the
author did not enumerate exactly what those uses are, so we'll have to
dig. :)
My guess is that it has to do with showing some commits twice, since we
would normally have the buffer available the first time we hit the
commit, and then free it in finish_commit().
If we blame that rev-list line (and then walk back over a bunch of
uninteresting commits via parent re-blaming), it comes from 3131b71301
(Add "--show-all" revision walker flag for debugging, 2008-02-09).
Thanks for doing this digging. I appreciate the breadcrumbs, too, so I
can do a better job of digging next time.
So there it is. It does show commits multiple times, but suppresses the
verbose header after the first showing. If we do something like this:
git rev-list --show-all --pretty --boundary c93150cfb0^-
you'll see some boundary commits that _don't_ have their pretty headers
shown. And with your proposed patch, we'd show them again. To keep the
same behavior we need to store that "we've already seen this" boolean
somewhere else (e.g., in an object flag; possibly SEEN, but that might
run afoul of other logic).
What confuses me about this behavior is that the OID is still shown on
the repeat (and in the case of `git log --oneline` will not actually
have a line break between two short-OIDs). I don't believe this behavior
is something to preserve.
You are right that we definitely don't want to show the full content twice.
It looks like the call in log-tree comes from the same commit, and
serves the same purpose.
Aside from storing the boolean "did we show it" in another way, the
other option is to simply change the behavior and accept that we might
pretty-print the commit twice. This is a backwards-incompatible change,
but I'm not sure if anybody would care. According to that commit,
--show-all was added explicitly for debugging, and it has never been
documented. I couldn't find any reference to people actually using it
on the list (a grep of the whole archive turns up 32 messages, most of
which are just it appearing in context; the only person mentioning its
actual use was Linus in 2008.
Unless I am misunderstanding, the current behavior on a repeated commit
is already incorrect: some amount of output occurs before checking the
buffer, so the output includes repeated records but with formatting that
violates the expectation. By doing the simple change of swapping
get_cached_commit_buffer() with get_commit_buffer(), we correct that
format violation but have duplicate copies.
The most-correct thing to do (in my opinion) is to put the requirement
of "no repeats" into the revision walk logic and stop having the
formatting methods expect them. Then, however we change this boolean
setting of "we have seen this before" it will not require the formatting
methods to change.
I can start working on a patch to move the duplicate-removal logic into
revision.c instead of these three callers:
builtin/rev-list.c: if (revs->verbose_header &&
get_cached_commit_buffer(commit, NULL)) {
log-tree.c: if (!get_cached_commit_buffer(commit, NULL))
object.c: if (!get_cached_commit_buffer(commit,
NULL)) {
But this caller seems pretty important in pretty.c:
/*
* Otherwise, we still want to munge the encoding header in the
* result, which will be done by modifying the buffer. If we
* are using a fresh copy, we can reuse it. But if we are using
* the cached copy from get_commit_buffer, we need to duplicate it
* to avoid munging the cached copy.
*/
if (msg == get_cached_commit_buffer(commit, NULL))
out = xstrdup(msg);
else
out = (char *)msg
Thanks,
-Stolee
From: Jeff King <hidden> Date: 2018-02-21 18:48:19
On Wed, Feb 21, 2018 at 09:13:22AM -0500, Derrick Stolee wrote:
quoted
So there it is. It does show commits multiple times, but suppresses the
verbose header after the first showing. If we do something like this:
git rev-list --show-all --pretty --boundary c93150cfb0^-
you'll see some boundary commits that _don't_ have their pretty headers
shown. And with your proposed patch, we'd show them again. To keep the
same behavior we need to store that "we've already seen this" boolean
somewhere else (e.g., in an object flag; possibly SEEN, but that might
run afoul of other logic).
What confuses me about this behavior is that the OID is still shown on the
repeat (and in the case of `git log --oneline` will not actually have a line
break between two short-OIDs). I don't believe this behavior is something to
preserve.
I think that repeating the oid is intentional; the point is to dump how
the traversal code is hitting the endpoints, even if we do so multiple
times.
The --oneline behavior just looks like a bug. I think --format is broken
with --show-all, too (it does not show anything!).
Unless I am misunderstanding, the current behavior on a repeated commit is
already incorrect: some amount of output occurs before checking the buffer,
so the output includes repeated records but with formatting that violates
the expectation. By doing the simple change of swapping
get_cached_commit_buffer() with get_commit_buffer(), we correct that format
violation but have duplicate copies.
Yeah, I'd agree with that assessment.
The most-correct thing to do (in my opinion) is to put the requirement of
"no repeats" into the revision walk logic and stop having the formatting
methods expect them. Then, however we change this boolean setting of "we
have seen this before" it will not require the formatting methods to change.
But then you wouldn't show repeats at all. If I'm understanding you
correctly.
TBH, I do not think it is worth spending a lot of effort on this
--show-all feature. It seems mostly like forgotten debugging cruft to
me. That's why I'd be OK with showing the whole header as the simplest
fix (i.e., just removing those calls entirely, not even converting them
to get_commit_buffer).
I can start working on a patch to move the duplicate-removal logic into
revision.c instead of these three callers:
builtin/rev-list.c: if (revs->verbose_header &&
get_cached_commit_buffer(commit, NULL)) {
log-tree.c: if (!get_cached_commit_buffer(commit, NULL))
object.c: if (!get_cached_commit_buffer(commit, NULL))
Those first two are duplicate detection. The third one in object.c
should stay, though. We've been fed a commit buffer to parse, and we
want to know whether we should attach it as the cached buffer for that
commit. But if we already have a cached buffer, there's no point in
doing so. And that's what we're checking there.
Though I think it would be equally correct to have set_commit_buffer()
just throw away the existing cache entry and replace it with this one. I
don't think there's a real reason to prefer the old to the new. And that
might be worth doing if it would let us drop get_cached_commit_buffer()
as a public function. But...
But this caller seems pretty important in pretty.c:
/*
* Otherwise, we still want to munge the encoding header in the
* result, which will be done by modifying the buffer. If we
* are using a fresh copy, we can reuse it. But if we are using
* the cached copy from get_commit_buffer, we need to duplicate it
* to avoid munging the cached copy.
*/
if (msg == get_cached_commit_buffer(commit, NULL))
out = xstrdup(msg);
else
out = (char *)msg
Like the one in object.c, this really does want to know about the cached
entry. And it should be unaffected by your patch, since we will have
called get_commit_buffer() at the top of that function.
If we wanted to write this one without get_cached_commit_buffer(), we'd
really need a function to ask "did this pointer come from the cache, or
was it freshly allocated?". That's the same thing we do for
unuse_commit_buffer(). So in theory we could have a boolean function
that would check that, and that would let us make
get_cached_commit_buffer() private.
In my opinion it's not really worth trying to make it private. The
confusion you're fixing in the first two calls is not due to a bad API,
but due to some subtly confusing logic in that code's use of the API. ;)
So I'd probably do this:
with the rationale that:
1. Nobody really cares about this verbose-output suppression anyway.
2. The code is confusing and fragile, since it uses the cached commit
buffer as an implicit boolean for "did we show the commit already".
3. It's broken for --oneline and user-formats, and this fixes it.
-Peff
From: Jeff King <hidden> Date: 2018-02-21 18:52:13
On Wed, Feb 21, 2018 at 01:48:11PM -0500, Jeff King wrote:
quoted
What confuses me about this behavior is that the OID is still shown on the
repeat (and in the case of `git log --oneline` will not actually have a line
break between two short-OIDs). I don't believe this behavior is something to
preserve.
I think that repeating the oid is intentional; the point is to dump how
the traversal code is hitting the endpoints, even if we do so multiple
times.
The --oneline behavior just looks like a bug. I think --format is broken
with --show-all, too (it does not show anything!).
I poked at one of the examples a little more closely. I actually think
these are not repeats, but simply UNINTERESTING parents that we never
needed to look at in our traversal (because we hit a point where
everything was UNINTERESTING).
So we are relying not on finish_commit() to have freed the buffer, but
on the traversal code to have never parsed those commits in the first
place. Which is doubly subtle.
I think the rest of my email stands, though: we should just show the
full headers for those commits.
-Peff
The get_cached_commit_buffer() method provides access to the buffer
loaded for a struct commit, if it was ever loadead and was not freed.
Two places use this to inform how to output information about commits.
log-tree.c uses this method to short-circuit the output of commit
information when the buffer is not cached. However, this leads to
incorrect output in 'git log --oneline' where the short-OID is written
but then the rest of the commit information is dropped and the next
commit is written on the same line.
rev-list uses this method for two reasons:
- First, if the revision walk visits a commit twice, the buffer was
freed by rev-list in the first write. The output then does not
match the format expectations, since the OID is written without the
rest of the content.
- Second, if the revision walk visits a commit that was marked
UNINTERESTING, the walk may never load a buffer and hence rev-list
will not output the verbose information.
These behaviors are undocumented, untested, and unlikely to be
expected by users or other software attempting to parse this output.
Helped-by: Jeff King [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
builtin/rev-list.c | 2 +-
log-tree.c | 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)
The get_cached_commit_buffer() method provides access to the buffer
loaded for a struct commit, if it was ever loadead and was not freed.
Two places use this to inform how to output information about commits.
log-tree.c uses this method to short-circuit the output of commit
information when the buffer is not cached. However, this leads to
incorrect output in 'git log --oneline' where the short-OID is written
but then the rest of the commit information is dropped and the next
commit is written on the same line.
rev-list uses this method for two reasons:
- First, if the revision walk visits a commit twice, the buffer was
freed by rev-list in the first write. The output then does not
match the format expectations, since the OID is written without the
rest of the content.
- Second, if the revision walk visits a commit that was marked
UNINTERESTING, the walk may never load a buffer and hence rev-list
will not output the verbose information.
These behaviors are undocumented, untested, and unlikely to be
expected by users or other software attempting to parse this output.
Helped-by: Jeff King [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
This would be a good time to allow multiple authors, or to just change
the author, since this is exactly the diff you (Peff) provided in an
earlier email. The commit message hopefully summarizes our discussion,
but I welcome edits.
From: Jeff King <hidden> Date: 2018-02-21 23:13:45
On Wed, Feb 21, 2018 at 02:17:11PM -0500, Derrick Stolee wrote:
The get_cached_commit_buffer() method provides access to the buffer
loaded for a struct commit, if it was ever loadead and was not freed.
Two places use this to inform how to output information about commits.
log-tree.c uses this method to short-circuit the output of commit
information when the buffer is not cached. However, this leads to
incorrect output in 'git log --oneline' where the short-OID is written
but then the rest of the commit information is dropped and the next
commit is written on the same line.
rev-list uses this method for two reasons:
- First, if the revision walk visits a commit twice, the buffer was
freed by rev-list in the first write. The output then does not
match the format expectations, since the OID is written without the
rest of the content.
I'm not sure after my earlier digging if there is even a way to trigger
this (and if so, it is probably accidental, since those lines were added
explicitly for --show-all).
And actually after re-reading the commit message for 3131b7130 again, I
think the current behavior is definitely not something that was
carefully planned. So I'd propose a commit message like below.
-- >8 --
Subject: [PATCH] commit: drop uses of get_cached_commit_buffer()
The "--show-all" revision option shows UNINTERESTING
commits. Some of these commits may be unparsed when we try
to show them (since we may or may not need to walk their
parents to fulfill the request).
Commit 3131b71301 (Add "--show-all" revision walker flag for
debugging, 2008-02-09) resolved this by just skipping
pretty-printing for commits without their object contents
cached, saying:
Because we now end up listing commits we may not even have been parsed
at all "show_log" and "show_commit" need to protect against commits
that don't have a commit buffer entry.
That was the easy fix to avoid the pretty-printer segfaulting,
but:
1. It doesn't work for all formats. E.g., --oneline
prints the oid for each such commit but not a trailing
newline, leading to jumbled output.
2. It only affects some commits, depending on whether we
happened to parse them or not (so if they were at the
tip of an UNINTERESTING starting point, or if we
happened to traverse over them, you'd see more data).
3. It unncessarily ties the decision to show the verbose
header to whether the commit buffer was cached. That
makes it harder to change the logic around caching
(e.g., if we could traverse without actually loading
the full commit objects).
These days it's safe to feed such a commit to the
pretty-print code. Since be5c9fb904 (logmsg_reencode: lazily
load missing commit buffers, 2013-01-26), we'll load it on
demand in such a case. So let's just always show the verbose
headers.
This does change the behavior of plumbing, but:
a. The --show-all option was explicitly introduced as a
debugging aid, and was never documented (and has rarely
even been mentioned on the list by git devs).
b. Avoiding the commits was already not deterministic due
to (2) above. So the caller might have seen full
headers for these commits anyway, and would need to be
prepared for it.
Signed-off-by: Jeff King <redacted>
---
builtin/rev-list.c | 2 +-
log-tree.c | 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)
Now if we'd get around to rewrite pretty.c as well, we could make it static,
giving a stronger reason of not using that function. But it looks a bit
complicated to me, who is not familiar in that area of the code.
Thanks for making less use of this suboptimal API,
Stefan
Now if we'd get around to rewrite pretty.c as well, we could make it static,
giving a stronger reason of not using that function. But it looks a bit
complicated to me, who is not familiar in that area of the code.
Thanks for making less use of this suboptimal API,
From: Jeff King <hidden> Date: 2018-02-21 23:34:40
On Wed, Feb 21, 2018 at 02:19:17PM -0500, Derrick Stolee wrote:
quoted
These behaviors are undocumented, untested, and unlikely to be
expected by users or other software attempting to parse this output.
Helped-by: Jeff King [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
This would be a good time to allow multiple authors, or to just change the
author, since this is exactly the diff you (Peff) provided in an earlier
email. The commit message hopefully summarizes our discussion, but I welcome
edits.
The point is moot if we take the revision I just sent (though in
retrospect I really ought to have put in a Reported-by: for you there).
But some communities are settling on Co-authored-by as a trailer for
this case. And GitHub has started parsing and showing that along with
author information:
https://github.com/blog/2496-commit-together-with-co-authors
-Peff
On Wed, Feb 21, 2018 at 02:17:11PM -0500, Derrick Stolee wrote:
quoted
The get_cached_commit_buffer() method provides access to the buffer
loaded for a struct commit, if it was ever loadead and was not freed.
Two places use this to inform how to output information about commits.
log-tree.c uses this method to short-circuit the output of commit
information when the buffer is not cached. However, this leads to
incorrect output in 'git log --oneline' where the short-OID is written
but then the rest of the commit information is dropped and the next
commit is written on the same line.
rev-list uses this method for two reasons:
- First, if the revision walk visits a commit twice, the buffer was
freed by rev-list in the first write. The output then does not
match the format expectations, since the OID is written without the
rest of the content.
I'm not sure after my earlier digging if there is even a way to trigger
this (and if so, it is probably accidental, since those lines were added
explicitly for --show-all).
And actually after re-reading the commit message for 3131b7130 again, I
think the current behavior is definitely not something that was
carefully planned. So I'd propose a commit message like below.
I only submitted my patch to avoid making you do the work of writing the
commit message. My messages still don't have quite the right amount of
detail (or the correct details, in this case).
Junio: please add
Reported-by: Derrick Stolee <redacted>
Thanks,
-Stolee
quoted hunk
-- >8 --
Subject: [PATCH] commit: drop uses of get_cached_commit_buffer()
The "--show-all" revision option shows UNINTERESTING
commits. Some of these commits may be unparsed when we try
to show them (since we may or may not need to walk their
parents to fulfill the request).
Commit 3131b71301 (Add "--show-all" revision walker flag for
debugging, 2008-02-09) resolved this by just skipping
pretty-printing for commits without their object contents
cached, saying:
Because we now end up listing commits we may not even have been parsed
at all "show_log" and "show_commit" need to protect against commits
that don't have a commit buffer entry.
That was the easy fix to avoid the pretty-printer segfaulting,
but:
1. It doesn't work for all formats. E.g., --oneline
prints the oid for each such commit but not a trailing
newline, leading to jumbled output.
2. It only affects some commits, depending on whether we
happened to parse them or not (so if they were at the
tip of an UNINTERESTING starting point, or if we
happened to traverse over them, you'd see more data).
3. It unncessarily ties the decision to show the verbose
header to whether the commit buffer was cached. That
makes it harder to change the logic around caching
(e.g., if we could traverse without actually loading
the full commit objects).
These days it's safe to feed such a commit to the
pretty-print code. Since be5c9fb904 (logmsg_reencode: lazily
load missing commit buffers, 2013-01-26), we'll load it on
demand in such a case. So let's just always show the verbose
headers.
This does change the behavior of plumbing, but:
a. The --show-all option was explicitly introduced as a
debugging aid, and was never documented (and has rarely
even been mentioned on the list by git devs).
b. Avoiding the commits was already not deterministic due
to (2) above. So the caller might have seen full
headers for these commits anyway, and would need to be
prepared for it.
Signed-off-by: Jeff King <redacted>
---
builtin/rev-list.c | 2 +-
log-tree.c | 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)