Message ID | pull.539.v19.git.1593457018.gitgitgadget@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | Reftable support git-core | expand |
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
> base-commit: b9a2d1a0207fb9ded3fa524f54db3bc322a12cc4
This is based on 'next', which usually is a sure way for a topic to
stay forever out of 'next', but we have impactful dependence only on
two topics, I think, and a good news is that both of them are in
pretty good shape. I think Brian's part 2 of SHA-256 work should be
on the 'master' branch soon, and Dscho's "customizable default
branch" is also ready---it just would, like all other topics, want
to spend at least one week on 'next' to be safe. And after that,
this topic can be directly on 'master' (there is another trivial
conflict around bisect--helper, but I am not worried about it),
which looks quite good.
Tentatively I've prepared a merge of bc/sha-256-part-2 and
js/default-branch-name on top of today's 'master' and these patches
applied cleanly on the result. As you mentioned, we may want to
flip the orders of patches a bit to split uncontroversial ones out
to a separate preparatory topic and get them merged to 'next' and
then to 'master' earlier than the remainder.
Thanks.
-- >8 --
fixup! Reftable support for git-core
Noticed by the "make && make distclean && git clean -n -x" test.
diff --git a/Makefile b/Makefile
index d949b79720..c22fd41662 100644
--- a/Makefile
+++ b/Makefile
@@ -3153,7 +3153,7 @@ cocciclean:
clean: profile-clean coverage-clean cocciclean
$(RM) *.res
$(RM) $(OBJECTS)
- $(RM) $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB) $(REFTABLE_LIB)
+ $(RM) $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
$(RM) $(TEST_PROGRAMS)
$(RM) $(FUZZ_PROGRAMS)
On Tue, Jun 30, 2020 at 12:54 AM Junio C Hamano <gitster@pobox.com> wrote: > > "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > base-commit: b9a2d1a0207fb9ded3fa524f54db3bc322a12cc4 > > This is based on 'next', which usually is a sure way for a topic to > stay forever out of 'next', but we have impactful dependence only on > two topics, I think, and a good news is that both of them are in > pretty good shape. I think Brian's part 2 of SHA-256 work should be > on the 'master' branch soon, and Dscho's "customizable default > branch" is also ready---it just would, like all other topics, want > to spend at least one week on 'next' to be safe. And after that, > this topic can be directly on 'master' (there is another trivial > conflict around bisect--helper, but I am not worried about it), > which looks quite good. ok. For the next time, I should keep basing myself on master, even if I know there are conflicts? Do you have an opinion on https://public-inbox.org/git/pull.665.git.1592580071.gitgitgadget@gmail.com/ ? There is some overlap with in sequencer.c, and Phillip's approach is likely more principled, so I'd like to base reftable on that. > fixup! Reftable support for git-core thanks, folded in!
Han-Wen Nienhuys <hanwen@google.com> writes: > On Tue, Jun 30, 2020 at 12:54 AM Junio C Hamano <gitster@pobox.com> wrote: >> >> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >> > base-commit: b9a2d1a0207fb9ded3fa524f54db3bc322a12cc4 >> >> This is based on 'next', which usually is a sure way for a topic to >> stay forever out of 'next', but we have impactful dependence only on >> two topics, I think, and a good news is that both of them are in >> pretty good shape. I think Brian's part 2 of SHA-256 work should be >> on the 'master' branch soon, and Dscho's "customizable default >> branch" is also ready---it just would, like all other topics, want >> to spend at least one week on 'next' to be safe. And after that, >> this topic can be directly on 'master' (there is another trivial >> conflict around bisect--helper, but I am not worried about it), >> which looks quite good. > > ok. For the next time, I should keep basing myself on master, even if > I know there are conflicts? Right now we know there won't be and that is why I said the above. If your next round would change the code drastically, or somebody sends changes that deliberately conflicts with what you are doing to sabotage you, the situation would become different. > Do you have an opinion on > https://public-inbox.org/git/pull.665.git.1592580071.gitgitgadget@gmail.com/ > ? > > There is some overlap with in sequencer.c, and Phillip's approach is > likely more principled, so I'd like to base reftable on that. I assumed that these were offered to you as possible improvements to be folded into your series, so I didn't read them very carefully and I didn't queue them myself. I expected that I would see them, possibly modified to fit the context better, as part of your series sent from you, perhaps to become a part of early clean-up portion of your topic. Thanks.
On Wed, Jul 1, 2020 at 2:03 AM Junio C Hamano <gitster@pobox.com> wrote: > > Do you have an opinion on > > https://public-inbox.org/git/pull.665.git.1592580071.gitgitgadget@gmail.com/ > > ? > > > > There is some overlap with in sequencer.c, and Phillip's approach is > > likely more principled, so I'd like to base reftable on that. > > I assumed that these were offered to you as possible improvements to > be folded into your series, so I didn't read them very carefully and > I didn't queue them myself. I expected that I would see them, > possibly modified to fit the context better, as part of your series > sent from you, perhaps to become a part of early clean-up portion of > your topic. They are changing the signature of widely used functions, which is useful for my series but not completely necessary. I would rather that someone else decides on how to go forward with the series.
Han-Wen Nienhuys <hanwen@google.com> writes: > On Wed, Jul 1, 2020 at 2:03 AM Junio C Hamano <gitster@pobox.com> wrote: >> > Do you have an opinion on >> > https://public-inbox.org/git/pull.665.git.1592580071.gitgitgadget@gmail.com/ >> > ? >> > >> > There is some overlap with in sequencer.c, and Phillip's approach is >> > likely more principled, so I'd like to base reftable on that. >> >> I assumed that these were offered to you as possible improvements to >> be folded into your series, so I didn't read them very carefully and >> I didn't queue them myself. I expected that I would see them, >> possibly modified to fit the context better, as part of your series >> sent from you, perhaps to become a part of early clean-up portion of >> your topic. > > They are changing the signature of widely used functions, which is > useful for my series but not completely necessary. I would rather that > someone else decides on how to go forward with the series. I was about to say that having written one ref backend, you are likely to have better context to judge how much better Phillip's patches would make the world be than I do ;-), but having worked on the "cleanse caller-supplied reflog message at generic layer" illustration patch, I think I am familiar enough with these three functions Phillip's patch touched to comment on them. I am not sure why delete_ref() needs to be modified to take a caller-supplied repository when refs_delete_ref() is already even a better way to do so (it allows the caller to give a ref_store, which is a part of a repository). The same between update_ref() and refs_update_ref(). Introducing a new repo_delete_ref() to extend the API into a three-tuple, delete_ref(...) { return repo_delete_ref(the_repository, ...); } repo_delete_ref(repo, ...) { return refs_delete_ref(get_main_ref_store(repo), ...); } refs_delete_ref(...) { /* as we already have it */ } _might_ make sense, but then we should do so for not just delete and update, but for all the operations consistently. I do not know if that is worth it offhand. The issue Phillip's patch addresses looks like a mere "lack of convenience", not a fundamental "without this we cannot write correct code easily", at least to me. What disturbed me more while I was looking at refs.c was that some operations are not done (perhaps cannot be done) as a part of a transaction. refs_delete_refs() and refs_rename_ref() directly call into the backend layer, for example, and that does not smell right.