From: Jeff King <peff@peff.net>
To: "Martin Ågren" <martin.agren@gmail.com>
Cc: Taylor Blau <me@ttaylorr.com>,
Git Mailing List <git@vger.kernel.org>,
Derrick Stolee <dstolee@microsoft.com>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 2/6] commit-graph.h: store object directory in 'struct commit_graph'
Date: Fri, 31 Jan 2020 05:20:49 -0500 [thread overview]
Message-ID: <20200131102049.GC2916051@coredump.intra.peff.net> (raw)
In-Reply-To: <CAN0heSrrrWjBKnzjMfzEkTMVTge2AfVdwsp6D5Mx==5S8-ZLJQ@mail.gmail.com>
On Fri, Jan 31, 2020 at 07:52:02AM +0100, Martin Ågren wrote:
> > --- a/commit-graph.c
> > +++ b/commit-graph.c
>
> > +struct object_directory *find_odb(struct repository *r, const char *obj_dir)
>
> This doesn't look commit-graph related -- could/should it go somewhere
> else?
I think the right place is actually as a static inside
builtin/commit-graph.c, as this is really about handling its weird
--object-dir options.
But it can't go there in this patch, because there's a caller in
commit-graph.c. In patch 4, we convert write_commit_graph() to take an
odb, too, and that call goes away. At that point, we could move it into
the builtin as a static.
Ideally we could flip the order of this patch and patch 4, but that
doesn't work either: by switching to an odb we lose our path
normalization, but if the other side hasn't switched either, then we
can't just compare odb pointers. It would be a temporary regression.
So there's a circular dependency between the two patches. I think we
ought to do done of:
- move find_odb() to a static as a cleanup on top
- squash those two patches together into a single
- swap the patch order, but have write_commit_graph_ctx store both the
"odb" _and_ the normalized copy of the path we do now. That leaves
it correct, and then it can be cleaned up in favor of an odb pointer
comparison in patch 5, along with the rest of the normalized bits.
I'm OK with any of those. The second two have the added bonus that we
could introduce the die() behavior into find_odb() immediately, and
explain it (there's another temporary weirdness in this patch where
specifying an --object-dir outside of the repository becomes a silent
noop, and then the next patch turns it into an error, but that could all
be done in a single step when we introduce find_odb()).
> I think you could drop `cmp` and that final check, and write the loop
> body as "if (!strcmp(...)) break". You could also have an empty loop
> body, but I wouldn't go there -- I'd find that less readable. (Maybe
> that's just me.)
Yeah, I believe you are correct (and this is a nice simplification worth
doing).
-Peff
next prev parent reply other threads:[~2020-01-31 10:20 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-30 23:00 [PATCH 0/6] commit-graph: use 'struct object_directory *' everywhere Taylor Blau
2020-01-30 23:00 ` [PATCH 1/6] t5318: don't pass non-object directory to '--object-dir' Taylor Blau
2020-01-30 23:00 ` [PATCH 5/6] commit-graph.c: remove path normalization, comparison Taylor Blau
2020-01-30 23:00 ` [PATCH 6/6] commit-graph.h: use odb in 'load_commit_graph_one_fd_st' Taylor Blau
2020-01-30 23:00 ` [PATCH 2/6] commit-graph.h: store object directory in 'struct commit_graph' Taylor Blau
2020-01-31 6:52 ` Martin Ågren
2020-01-31 10:20 ` Jeff King [this message]
2020-01-31 19:19 ` Martin Ågren
2020-02-03 4:36 ` Taylor Blau
2020-02-03 8:36 ` Jeff King
2020-01-31 20:49 ` Junio C Hamano
2020-02-03 3:58 ` Taylor Blau
2020-01-30 23:00 ` [PATCH 4/6] commit-graph.h: store an odb in 'struct write_commit_graph_context' Taylor Blau
2020-01-30 23:00 ` [PATCH 3/6] builtin/commit-graph.c: die() with unknown '--object-dir' Taylor Blau
2020-01-31 10:30 ` [PATCH 0/6] commit-graph: use 'struct object_directory *' everywhere Jeff King
2020-01-31 13:22 ` Derrick Stolee
2020-02-03 4:38 ` Taylor Blau
2020-02-03 21:17 ` [PATCH v2 0/5] " Taylor Blau
2020-02-03 21:17 ` [PATCH v2 1/5] t5318: don't pass non-object directory to '--object-dir' Taylor Blau
2020-02-03 21:17 ` [PATCH v2 2/5] commit-graph.h: store an odb in 'struct write_commit_graph_context' Taylor Blau
2020-02-04 5:51 ` Taylor Blau
2020-02-04 19:54 ` Junio C Hamano
2020-02-04 21:28 ` Taylor Blau
2020-02-04 21:44 ` Junio C Hamano
2020-02-03 21:18 ` [PATCH v2 3/5] commit-graph.h: store object directory in 'struct commit_graph' Taylor Blau
2020-02-03 21:18 ` [PATCH v2 4/5] commit-graph.c: remove path normalization, comparison Taylor Blau
2020-02-03 21:18 ` [PATCH v2 5/5] commit-graph.h: use odb in 'load_commit_graph_one_fd_st' Taylor Blau
2020-02-05 12:30 ` [PATCH v2 0/5] commit-graph: use 'struct object_directory *' everywhere Jeff King
Reply instructions:
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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200131102049.GC2916051@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=martin.agren@gmail.com \
--cc=me@ttaylorr.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* 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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).