From: Jeff King <email@example.com>
To: Derrick Stolee via GitGitGadget <firstname.lastname@example.org>
Cc: email@example.com, Derrick Stolee <firstname.lastname@example.org>,
Junio C Hamano <email@example.com>
Subject: Re: [PATCH 1/1] commit-graph: fix writing first commit-graph during fetch
Date: Tue, 22 Oct 2019 16:33:16 -0400 [thread overview]
Message-ID: <20191022203316.GC12270@sigill.intra.peff.net> (raw)
On Tue, Oct 22, 2019 at 05:28:55PM +0000, Derrick Stolee via GitGitGadget wrote:
> However, the UNINTERESTING flag is used in lots of places in the
> codebase. This flag usually means some barrier to stop a commit walk,
> such as in revision-walking to compare histories. It is not often
> cleared after the walk completes because the starting points of those
> walks do not have the UNINTERESTING flag, and clear_commit_marks() would
> stop immediately.
Oof. Nicely explained, and your fix makes sense.
The global-ness of revision flags always makes me nervous about doing
more things in-process (this isn't the first such bug we've had).
I have a dream of converting most uses of flags into using a
commit-slab. That provides cheap access to an auxiliary structure, so
each traversal, etc, could keep its own flag structure. I'm not sure if
it would have a performance impact, though. Even though it's O(1), it is
an indirect lookup, which could have some memory-access impact (though
my guess is it would be lost in the noise).
One of the sticking points is that all object types, not just commits,
use flags. But we only assign slab ids to commits. I noticed recently
that "struct object" has quite a few spare bits in it these days,
because the switch to a 32-byte oid means 64-bit machines now have an
extra 4 bytes of padding. I wonder if we could use that to store an
Anyway, that's getting far off the topic; clearly we need a fix in the
meantime, and what you have here looks good to me.
> I tested running clear_commit_marks_many() to clear the UNINTERESTING
> flag inside close_reachable(), but the tips did not have the flag, so
> that did nothing.
Another option would be clear_object_flags(), which just walks all of
the in-memory structs. Your REACHABLE solution is cheaper, though.
> Instead, I finally arrived on the conclusion that I should use a flag
> that is not used in any other part of the code. In commit-reach.c, a
> number of flags were defined for commit walk algorithms. The REACHABLE
> flag seemed like it made the most sense, and it seems it was not
> actually used in the file.
Yeah, being able to remove it from commit-reach.c surprised me for a
moment. To further add to the confusion, builtin/fsck.c has its own
REACHABLE flag (with a different bit and a totally different purpose). I
don't think there's any practical problem there, though.
> I have failed to produce a test using the file:// protocol that
> demonstrates this bug.
Hmm, from the description, it sounds like it should be easy. I might
poke at it a bit.
next prev parent reply other threads:[~2019-10-22 20:33 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-22 17:28 [PATCH 0/1] [v2.24.0-rc0 BUG] fetch.writeCommitGraph fails on first fetch Derrick Stolee via GitGitGadget
2019-10-22 17:28 ` [PATCH 1/1] commit-graph: fix writing first commit-graph during fetch Derrick Stolee via GitGitGadget
2019-10-22 20:33 ` Jeff King [this message]
2019-10-22 21:45 ` Jeff King
2019-10-22 23:35 ` SZEDER Gábor
2019-10-23 0:35 ` Derrick Stolee
2019-10-23 0:48 ` Jeff King
2019-10-23 1:22 ` Jeff King
2019-10-23 13:01 ` [PATCH v2 0/2] [v2.24.0-rc0 BUG] fetch.writeCommitGraph fails on first fetch Derrick Stolee via GitGitGadget
2019-10-23 13:01 ` [PATCH v2 1/2] t5510-fetch.sh: demonstrate fetch.writeCommitGraph bug Derrick Stolee via GitGitGadget
2019-10-23 14:18 ` SZEDER Gábor
2019-10-23 20:46 ` Derrick Stolee
2019-10-24 12:18 ` SZEDER Gábor
2019-10-23 13:01 ` [PATCH v2 2/2] commit-graph: fix writing first commit-graph during fetch Derrick Stolee via GitGitGadget
2019-10-23 15:04 ` SZEDER Gábor
2019-10-24 10:39 ` Derrick Stolee
2019-10-30 14:31 ` SZEDER Gábor
2019-10-24 12:18 ` [PATCH v3 0/2] [v2.24.0-rc0 BUG] fetch.writeCommitGraph fails on first fetch Derrick Stolee via GitGitGadget
2019-10-24 12:18 ` [PATCH v3 1/2] t5510-fetch.sh: demonstrate fetch.writeCommitGraph bug Derrick Stolee via GitGitGadget
2019-10-24 12:18 ` [PATCH v3 2/2] commit-graph: fix writing first commit-graph during fetch Derrick Stolee via GitGitGadget
2019-10-24 13:40 ` [PATCH v4 0/2] [v2.24.0-rc0 BUG] fetch.writeCommitGraph fails on first fetch Derrick Stolee via GitGitGadget
2019-10-24 13:40 ` [PATCH v4 1/2] t5510-fetch.sh: demonstrate fetch.writeCommitGraph bug Derrick Stolee via GitGitGadget
2019-10-24 13:40 ` [PATCH v4 2/2] commit-graph: fix writing first commit-graph during fetch Derrick Stolee via GitGitGadget
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).