Re: [PATCH v6 1/7] refs.c: add err arguments to reflog functions
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Jul 9, 2015, 06:47 UTC
- Message-ID
- <559E1917.6000205@alum.mit.edu>
- In-Reply-To
- <xmqqr3oiy3pm.fsf@gitster.dls.corp.google.com>
On 07/08/2015 07:11 PM, Junio C Hamano wrote:
Show 38 quoted lines
> Michael Haggerty <mhagger@alum.mit.edu> writes: > >> I think your v7 of this patch goes too far, by turning a failure to >> write to the reflog into a failure of the whole transaction. The problem >> is that this failure comes too late, in the commit phase of the >> transaction. Aborting at this late stage can leave some references >> changed and others rolled back, violating the promise of atomicity. > > Yeah, that sounds problematic. > >> The old code reported a failure to write the reflog to stderr but didn't >> fail the transaction. I think that behavior is more appropriate. The >> reflog is of lower importance than the references themselves. Junio, do >> you agree? > > That is actually a loaded question. > > Do I agree that the current (i.e. before this change) behaviour is > more appropriate given the current choice of representation of refs > and reflogs on the filesystem, treating a failure to update reflog > as lower importance event and accept it as a limitation that it > cannot abort the whole transaction atomically? Compared to leaving > the repository in a half-updated state where some refs and their > logs are updated already, other remaining proposed updates are > ignored, and the transaction claims to have failed even though some > things have already changed and we cannot rollback, I would say that > is a better compromise to treat reflog update as a lower importance. > > Do I agree that reflog writing should stay to be best-effort in the > longer term? Not really. If we are moving the ref API in the > direction where we can plug a backend that is different from the > traditional filesystem based one for storing refs, I think the > backend should also be responsible for storing logs for the refs it > stores, and if the backend wants to promise atomicity, then we > should be able to fail the whole transaction when updates to refs > could proceed but updates to the log of one of these updated refs > cannot. So I do not agree to cast in stone "the reflog is of lower > importance" as a general rule.
Junio,
You make a good distinction. I was describing a compromise that we have to make now due to the limitations of the current ref/reflog backend. But I agree 100% that a future storage backend that can do correct rollback of refs *and* reflogs can fail a transaction if the reflog updates cannot be committed.
Thanks, Michael
-- Michael Haggerty mhagger@alum.mit.edu