| Message ID | cover.1580424766.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | commit-graph: use 'struct object_directory *' everywhere | expand |
On Thu, Jan 30, 2020 at 03:00:40PM -0800, Taylor Blau wrote: > This series became a little bit longer than I was expecting it to be, so > here is the high-level structure: > > - 1/6 fixes a bug in a test that would cause a subsequent failure if > unaddressed. > > - 2/6 does the first half of the removal by using 'struct > object_directory *'s within the 'commit_graph' structure. > > - 4/6 does the second half by removing 'char *object_dir' usage in the > 'write_commit_graph_context' structure. > > - 5/6 ties 2/6 and 4/6 together by removing all path normalization > completely, fixing the uninitialized read bug. > > - And 6/6 cleans up. With the exception of the patch-ordering discussion in the sub-thread with Martin, this looks good to me. Patch 3 is a change in user-visible behavior, as it restricts how --object-dir can be used (it must be the main object-dir or an alternate within the repository). I don't _think_ anybody would care, as the semantics of those options seemed kind of ill-defined to me in the first place. But it's worth calling out as a potential risk. I suppose the alternative is to make a one-off fake "struct object_directory" within the process that isn't connected to the repository. But if nobody cares, I'd just as soon avoid that. One other funny thing with this series: the Date headers of your patches seem out of order. They ordering in your cover letter here is fine and presumably reflects the commit topology: > Taylor Blau (6): > t5318: don't pass non-object directory to '--object-dir' > commit-graph.h: store object directory in 'struct commit_graph' > builtin/commit-graph.c: die() with unknown '--object-dir' > commit-graph.h: store an odb in 'struct write_commit_graph_context' > commit-graph.c: remove path normalization, comparison > commit-graph.h: use odb in 'load_commit_graph_one_fd_st' but the Date headers in order of 1-6 are: Date: Thu, 30 Jan 2020 15:00:43 -0800 Date: Thu, 30 Jan 2020 15:00:50 -0800 Date: Thu, 30 Jan 2020 15:00:54 -0800 Date: Thu, 30 Jan 2020 15:00:52 -0800 Date: Thu, 30 Jan 2020 15:00:45 -0800 Date: Thu, 30 Jan 2020 15:00:47 -0800 It's like your sending script rewrites the Date header and puts a 2-second bump between each one (which is good, and what git-send-email does), but got fed the patches in the wrong order (perhaps their _original_ date order, if there was clean-up rebasing). -Peff
On 1/31/2020 5:30 AM, Jeff King wrote: > On Thu, Jan 30, 2020 at 03:00:40PM -0800, Taylor Blau wrote: > >> This series became a little bit longer than I was expecting it to be, so >> here is the high-level structure: >> >> - 1/6 fixes a bug in a test that would cause a subsequent failure if >> unaddressed. >> >> - 2/6 does the first half of the removal by using 'struct >> object_directory *'s within the 'commit_graph' structure. >> >> - 4/6 does the second half by removing 'char *object_dir' usage in the >> 'write_commit_graph_context' structure. >> >> - 5/6 ties 2/6 and 4/6 together by removing all path normalization >> completely, fixing the uninitialized read bug. >> >> - And 6/6 cleans up. > > With the exception of the patch-ordering discussion in the sub-thread > with Martin, this looks good to me. I agree. Martin's comment is a good one. I can't find anything else to improve the series. > Patch 3 is a change in user-visible behavior, as it restricts how > --object-dir can be used (it must be the main object-dir or an alternate > within the repository). I don't _think_ anybody would care, as the > semantics of those options seemed kind of ill-defined to me in the first > place. But it's worth calling out as a potential risk. I suppose the > alternative is to make a one-off fake "struct object_directory" within > the process that isn't connected to the repository. But if nobody cares, > I'd just as soon avoid that. I think that this change of behavior is fine, especially because if someone writes a commit-graph to an --object-dir that is not an alternate, then that repo will not discover the new commit-graph anyway. I do like that you state a possible work-around in case someone shows up with a legitimate use case for a non-alternate object-dir. Thanks, -Stolee
On Fri, Jan 31, 2020 at 08:22:42AM -0500, Derrick Stolee wrote: > On 1/31/2020 5:30 AM, Jeff King wrote: > > On Thu, Jan 30, 2020 at 03:00:40PM -0800, Taylor Blau wrote: > > > >> This series became a little bit longer than I was expecting it to be, so > >> here is the high-level structure: > >> > >> - 1/6 fixes a bug in a test that would cause a subsequent failure if > >> unaddressed. > >> > >> - 2/6 does the first half of the removal by using 'struct > >> object_directory *'s within the 'commit_graph' structure. > >> > >> - 4/6 does the second half by removing 'char *object_dir' usage in the > >> 'write_commit_graph_context' structure. > >> > >> - 5/6 ties 2/6 and 4/6 together by removing all path normalization > >> completely, fixing the uninitialized read bug. > >> > >> - And 6/6 cleans up. > > > > With the exception of the patch-ordering discussion in the sub-thread > > with Martin, this looks good to me. > > I agree. Martin's comment is a good one. I can't find anything else > to improve the series. Thanks for your review! > > Patch 3 is a change in user-visible behavior, as it restricts how > > --object-dir can be used (it must be the main object-dir or an alternate > > within the repository). I don't _think_ anybody would care, as the > > semantics of those options seemed kind of ill-defined to me in the first > > place. But it's worth calling out as a potential risk. I suppose the > > alternative is to make a one-off fake "struct object_directory" within > > the process that isn't connected to the repository. But if nobody cares, > > I'd just as soon avoid that. > > I think that this change of behavior is fine, especially because if > someone writes a commit-graph to an --object-dir that is not an > alternate, then that repo will not discover the new commit-graph > anyway. And thanks for the ack. I would be somewhat surprised if someone were really relying on this behavior in practice. > I do like that you state a possible work-around in case someone shows > up with a legitimate use case for a non-alternate object-dir. > > Thanks, > -Stolee Thanks, Taylor
Hi, While working on deploying layered commit-graphs at GitHub, I noticed that I was able to trigger the mechanism to prevent merging cross-alternate commit-graph layers in funny ways, like claiming that "./objects" and "objects" were in different directories. This resulted in removing all of the places in the commit-graph.c code that use 'char *object_dir's with 'struct object_directory *'s instead. This allows us to replace brittle string-based path comparisons with raw pointer comparisons to check whether two commit-graphs belong to the same object store or not. This has a pleasant side effect in PATCH 5/6 of fixing an uninitialized read as described in [1]. This series became a little bit longer than I was expecting it to be, so here is the high-level structure: - 1/6 fixes a bug in a test that would cause a subsequent failure if unaddressed. - 2/6 does the first half of the removal by using 'struct object_directory *'s within the 'commit_graph' structure. - 4/6 does the second half by removing 'char *object_dir' usage in the 'write_commit_graph_context' structure. - 5/6 ties 2/6 and 4/6 together by removing all path normalization completely, fixing the uninitialized read bug. - And 6/6 cleans up. We've been running a version of this series based on our fork (which is in turn based on 2.24) for a few hours without issue. Thanks in advance for your review. [1]: https://lore.kernel.org/git/20191027042116.GA5801@sigill.intra.peff.net/ Taylor Blau (6): t5318: don't pass non-object directory to '--object-dir' commit-graph.h: store object directory in 'struct commit_graph' builtin/commit-graph.c: die() with unknown '--object-dir' commit-graph.h: store an odb in 'struct write_commit_graph_context' commit-graph.c: remove path normalization, comparison commit-graph.h: use odb in 'load_commit_graph_one_fd_st' Documentation/git-commit-graph.txt | 5 +- builtin/commit-graph.c | 25 ++++-- builtin/commit.c | 3 +- builtin/fetch.c | 2 +- builtin/gc.c | 2 +- commit-graph.c | 135 +++++++++++++++-------------- commit-graph.h | 17 ++-- t/helper/test-read-graph.c | 8 +- t/t5318-commit-graph.sh | 4 +- 9 files changed, 114 insertions(+), 87 deletions(-) -- 2.25.0.dirty