Re: [PATCH v20 00/48] Use ref transactions

On Tue, Jul 8, 2014 at 9:29 AM, Michael Haggerty [off-list ref] wrote:

On 06/20/2014 04:42 PM, Ronnie Sahlberg wrote:

quoted

This patch series can also be found at
https://github.com/rsahlberg/git/tree/ref-transactions

This patch series is based on current master and expands on the transaction
API. It converts all ref updates, inside refs.c as well as external, to use the
transaction API for updates. This makes most of the ref updates to become
atomic when there are failures locking or writing to a ref.

This version completes the work to convert all ref updates to use transactions.
Now that all updates are through transactions I will start working on
cleaning up the reading of refs and to create an api for managing reflogs but
all that will go in a different patch series.

Version 20:
 - Whitespace and style changes suggested by Jun.

I spent most of the day on reviewing this patch series,

Thanks!

but now I'm out
of time again.  Here is a summary from my point of view:

Patches 01-19 -- ACK mhagger
Patches 20-42 -- I sent various comments, small to large, concerning
these patches
Patch 43 -- Needs more justification if it is to be acceptable
Patch 44 -- Depends on 43
Patches 45-48 -- I didn't quite get to these, but...

Perhaps it would be more appropriate for the rules about reference name
conflicts to be enforced by the backend, since it is the limitations of
the current backend that impose the restrictions.  Would that make sense?

On the other hand, removing the restrictions isn't simply a matter of
picking a different backend, because all Git repositories have to be
able to interact with each other.

So, I don't yet have a considered opinion on the matter.

I think for compatibility I would prefer to keep the same rules for
name conflicts as for the current files implementation.
But we could have a configuration option to disable these checks, with
the caveat that this might mean that some users will
no longer be able to access pull all the branches anymore.

I think it would be good to try to merge the first part of this patch
series to lock in some progress while we continue iterating on the
remainder.  I'm satisfied that it is all going in the right direction
and I am thankful to Ronnie for pushing it forward.  But handling
48-patch series is very daunting and I would welcome a split.

Will do.

I'm not sure whether patches 01-19 are necessarily the right split
between merge-now/iterate-more; it is more or less an accident that I
stopped after patch 19 on an earlier review.  Maybe Ronnie could propose
a logical subset of the commits as being ready to be merged to next in
the nearish term?

Michael

--
Michael Haggerty
mhagger@alum.mit.edu