Re: [PATCH v19 00/20] Reftable support git-core

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 1, 2020, 20:56 UTC
Message-ID: <xmqqmu4jc5th.fsf@gitster.c.googlers.com>
In-Reply-To: <CAFQ2z_MZoc3Q15O0LDTgVYV7nKEK1Oc8GLWEZYrrFjU9VSsUCQ@mail.gmail.com>

Han-Wen Nienhuys <hanwen@google.com> writes:

Show 18 quoted lines

> 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.