Re: [PATCH v8 41/44] refs.c: add a new flag for transaction delete for refs we know are packed only

I hate rename_ref :-)


I have reworked the transaction code to special case the deletion of
the old ref for n/n -> n  and n -> n/n renames
so that we can carefully avoid n/n.lock files to exist or prevent the
directory <-> file transition for n during these renames.

This should allow us to have rename_ref becoming reasonably
implementation agnostic, aside for that it wants to lstat the ref to
see if it is a soft link, and re-use the code for other refs backends.


Please see the ref-transaction branch.


On Thu, May 22, 2014 at 12:12 PM, Ronnie Sahlberg [off-list ref] wrote:

On Thu, May 22, 2014 at 11:17 AM, Jonathan Nieder [off-list ref] wrote:

quoted

Hi,

Ronnie Sahlberg wrote:

quoted

Add a new flag REF_ISPACKONLY that we can use in ref_transaction_delete.
This flag indicates that the ref does not exist as a loose ref andf only as
a packed ref. If this is the case we then change the commit code so that
we skip taking out a lock file and we skip calling delete_ref_loose.

This seems wrong.  Can't someone else create a loose ref which will
shadow the packed ref and break the serializability of updates to this
ref?

The above doesn't explain why we want to avoid locking the loose ref,
but I assume it's for the sake of the "git branch -m foo/bar foo"
case.  For that case, wouldn't the following sequence of filesystem
operations work?

        - create $GIT_DIR/refs/heads/foo/bar.lock
        - create $GIT_DIR/refs/heads/foo.lock
        - if $GIT_DIR/refs/heads/foo/bar exists, add the ref to
          packed-refs (using the usual lockfile-update mechanism)
          and unlink $GIT_DIR/refs/heads/foo/bar
        - verify that current foo and foo/bar state are okay.  If
          not, roll back.
        - unlink $GIT_DIR/refs/heads/foo/bar.lock
        - rmdir $GIT_DIR/refs/heads/foo
        - rename $GIT_DIR/refs/heads/foo.lock into place

Or:

        - create $GIT_DIR/refs/heads/foo/bar.lock
        - create $GIT_DIR/refs/heads/foo.lock
        - verify state of all relevant refs

        - update packed-refs to remove refs/heads/foo/bar and
          add refs/heads/foo

        - unlink $GIT_DIR/refs/heads/foo/bar
        - unlink $GIT_DIR/refs/heads/foo
        - unlink $GIT_DIR/refs/heads/foo/bar.lock
        - unlink $GIT_DIR/refs/heads/foo.lock

I removed all the rename_ref related patches for now. rename_ref is
probably best implemented specifically for each backend anyway.

I will still produce a separate patch that will do something like this
you suggested
(since rename_ref is still racy and fragile)

quoted

        - create $GIT_DIR/refs/heads/foo/bar.lock
        - create $GIT_DIR/refs/heads/foo.lock
        - verify state of all relevant refs

        - update packed-refs to remove refs/heads/foo/bar and
          add refs/heads/foo

        - unlink $GIT_DIR/refs/heads/foo/bar
        - unlink $GIT_DIR/refs/heads/foo
        - unlink $GIT_DIR/refs/heads/foo/bar.lock
        - unlink $GIT_DIR/refs/heads/foo.lock

Thanks
ronnie sahlberg