Re: [PATCH v3 0/6] Object store refactoring: commit graph
On 7/11/2018 6:42 PM, Jonathan Tan wrote: This is on _both_ ds/commit-graph-fsck and sb/object-store-lookup, following Stolee's suggestion. (It also seems better to build it this way to me, since both these branches are going into "next" according to the latest What's Cooking.) Junio wrote in [1]: I've added SQUASH??? patch at the tip of each of the above, rebuilt 'pu' with them and pushed the result out. It seems that Travis is happier with the result. Please do not forget to squash them in when/if rerolling. If there is no need to change anything else other than squashing them, you can tell me to which commit in your series the fix needs to be squashed in (that would save me time to figure it out, obviously). I'm rerolling because I also need to update the last patch with the new lookup_commit() function signature that Stefan's sb/object-store-lookup introduces. I have squashed the SQUASH??? patch into the corresponding patch in this patch set. Changes from v2: - now also based on sb/object-store-lookup in addition to ds/commit-graph-fsck (I rebased ds/commit-graph-fsck onto sb-object-store-lookup, then rebased this patch set onto the result) - patches 1-5 are unchanged - patch 6: - used "PRItime" instead of "ul" when printing a timestamp (the SQUASH??? patch) - updated invocations of lookup_commit() to take a repository object [1] https://public-inbox.org/git/xmqqpnzt1myi@gitster-ct.c.googlers.com/ I re-read the patch series and think you addressed all feedback. I have no more to add. Reviewed-by: Derrick Stolee
Re: [PATCH v3 0/6] Object store refactoring: commit graph
Jonathan Tan writes: > This is on _both_ ds/commit-graph-fsck and sb/object-store-lookup, > following Stolee's suggestion. > > (It also seems better to build it this way to me, since both these > branches are going into "next" according to the latest What's Cooking.) OK. I am perfectly OK if this left lookup_commit() with older function signature and instead asked the integrator to fix up with evil merge with sb/object-store-lookup, but given that this topic is no more urgent than the other one, making them intertwined is fine. Merging ds/commit-graph-fsck into sb/object-store-lookup already requires an evil merge to build correctly due to this new parameter. I really wish we did the more canonical "do not change signature of a widely used function. If the widely used one is a mere specialization that calls the new one with the default parameter, make that widely used one a thin wraper (or a macro) of the new one" approach to avoid having to repeatedly create such pointless evil merges X-<. > Makefile | 1 + > builtin/commit-graph.c | 2 + > builtin/fsck.c | 2 +- > cache.h| 1 - > commit-graph.c | 108 + > commit-graph.h | 11 ++-- > commit.c | 6 +-- > config.c | 5 -- > environment.c | 1 - > object-store.h | 6 +++ > object.c | 5 ++ > ref-filter.c | 2 +- > t/helper/test-repository.c | 82 > t/helper/test-tool.c | 1 + > t/helper/test-tool.h | 1 + > t/t5318-commit-graph.sh| 35 > 16 files changed, 207 insertions(+), 62 deletions(-) > create mode 100644 t/helper/test-repository.c
[PATCH v3 0/6] Object store refactoring: commit graph
This is on _both_ ds/commit-graph-fsck and sb/object-store-lookup, following Stolee's suggestion. (It also seems better to build it this way to me, since both these branches are going into "next" according to the latest What's Cooking.) Junio wrote in [1]: > I've added SQUASH??? patch at the tip of each of the above, > rebuilt 'pu' with them and pushed the result out. It seems that > Travis is happier with the result. > > Please do not forget to squash them in when/if rerolling. If there > is no need to change anything else other than squashing them, you > can tell me to which commit in your series the fix needs to be > squashed in (that would save me time to figure it out, obviously). I'm rerolling because I also need to update the last patch with the new lookup_commit() function signature that Stefan's sb/object-store-lookup introduces. I have squashed the SQUASH??? patch into the corresponding patch in this patch set. Changes from v2: - now also based on sb/object-store-lookup in addition to ds/commit-graph-fsck (I rebased ds/commit-graph-fsck onto sb-object-store-lookup, then rebased this patch set onto the result) - patches 1-5 are unchanged - patch 6: - used "PRItime" instead of "ul" when printing a timestamp (the SQUASH??? patch) - updated invocations of lookup_commit() to take a repository object [1] https://public-inbox.org/git/xmqqpnzt1myi@gitster-ct.c.googlers.com/ Jonathan Tan (6): commit-graph: refactor preparing commit graph object-store: add missing include commit-graph: add missing forward declaration commit-graph: add free_commit_graph commit-graph: store graph in struct object_store commit-graph: add repo arg to graph readers Makefile | 1 + builtin/commit-graph.c | 2 + builtin/fsck.c | 2 +- cache.h| 1 - commit-graph.c | 108 + commit-graph.h | 11 ++-- commit.c | 6 +-- config.c | 5 -- environment.c | 1 - object-store.h | 6 +++ object.c | 5 ++ ref-filter.c | 2 +- t/helper/test-repository.c | 82 t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t5318-commit-graph.sh| 35 16 files changed, 207 insertions(+), 62 deletions(-) create mode 100644 t/helper/test-repository.c -- 2.18.0.203.gfac676dfb9-goog
