patch, 8 partsreflog: fix documentation

>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index c5fa709..d1130e2 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3080,10 +3080,6 @@ static int write_ref_sha1(struct ref_lock *lock,
>         static char term = '\n';
>         struct object *o;
>
> -       if (!lock) {
> -               errno = EINVAL;
> -               return -1;
> -       }
>         if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
>                 unlock_ref(lock);
>                 return 0;
> --
> 2.1.4
>

> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> > None of the callers pass NULL to this function, and there doesn't seem
> > to be any usefulness to allowing them to do so.
> 
> Usually I'd oppose this change, as it seems to be a good defensive
> measure. (I cannot assume future me or anybody knows what they're
> doing), but as this function (write_ref_sha1) is not widely exposed
> any more since aae383db8 (Apr 28, refs.c: make write_ref_sha1 static),
> I think it's safe to assume changes affecting this call are well
> understood in the future.

> write_ref_sha1() previously skipped the write if the reference already
> had the desired value, unless lock->force_write was set. Instead,
> perform that test at the callers.
>
> Two of the callers (in rename_ref()) unconditionally set force_write
> just before calling write_ref_sha1(), so they don't need the extra
> check at all. Nor do they need to set force_write anymore.

> The last caller, in ref_transaction_commit(), still needs the test.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index d1130e2..651e37e 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2878,7 +2878,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>  		error("unable to lock %s for update", newrefname);
>  		goto rollback;
>  	}
> -	lock->force_write = 1;
>  	hashcpy(lock->old_sha1, orig_sha1);
>  	if (write_ref_sha1(lock, orig_sha1, logmsg)) {
>  		error("unable to write current sha1 into %s", newrefname);
> @@ -2894,7 +2893,6 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>  		goto rollbacklog;
>  	}
>  
> -	lock->force_write = 1;
>  	flag = log_all_ref_updates;
>  	log_all_ref_updates = 0;
>  	if (write_ref_sha1(lock, orig_sha1, NULL))
> @@ -3080,10 +3078,6 @@ static int write_ref_sha1(struct ref_lock *lock,
>  	static char term = '\n';
>  	struct object *o;
>  
> -	if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
> -		unlock_ref(lock);
> -		return 0;
> -	}
>  	o = parse_object(sha1);
>  	if (!o) {
>  		error("Trying to write ref %s with nonexistent object %s",
> @@ -3797,15 +3791,21 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  		struct ref_update *update = updates[i];
>  
>  		if (!is_null_sha1(update->new_sha1)) {
> -			if (write_ref_sha1(update->lock, update->new_sha1,
> -					   update->msg)) {
> +			if (!update->lock->force_write &&
> +			    !hashcmp(update->lock->old_sha1, update->new_sha1)) {
> +				unlock_ref(update->lock);
> +				update->lock = NULL;
> +			} else if (write_ref_sha1(update->lock, update->new_sha1,
> +						  update->msg)) {
>  				update->lock = NULL; /* freed by write_ref_sha1 */
>  				strbuf_addf(err, "Cannot update the ref '%s'.",
>  					    update->refname);
>  				ret = TRANSACTION_GENERIC_ERROR;
>  				goto cleanup;
> +			} else {
> +				/* freed by write_ref_sha1(): */
> +				update->lock = NULL;
>  			}
> -			update->lock = NULL; /* freed by write_ref_sha1 */
>  		}
>  	}

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 651e37e..b083858 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2259,7 +2259,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>         int type, lflags;
>         int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
>         int resolve_flags = 0;
> -       int missing = 0;
>         int attempts_remaining = 3;
>
>         lock = xcalloc(1, sizeof(struct ref_lock));
> @@ -2298,13 +2297,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>                         orig_refname, strerror(errno));
>                 goto error_return;
>         }
> -       missing = is_null_sha1(lock->old_sha1);
> -       /* When the ref did not exist and we are creating it,
> -        * make sure there is no existing ref that is packed
> -        * whose name begins with our refname, nor a ref whose
> -        * name is a proper prefix of our refname.
> +       /*
> +        * When the ref did not exist and we are creating it, make
> +        * sure there is no existing packed ref whose name begins with
> +        * our refname, nor a packed ref whose name is a proper prefix
> +        * of our refname.
>          */
> -       if (missing &&
> +       if (is_null_sha1(lock->old_sha1) &&
>              !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) {
>                 last_errno = ENOTDIR;
>                 goto error_return;
> @@ -2320,8 +2319,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>         lock->ref_name = xstrdup(refname);
>         lock->orig_ref_name = xstrdup(orig_refname);
>         ref_file = git_path("%s", refname);
> -       if (missing)
> -               lock->force_write = 1;
>         if ((flags & REF_NODEREF) && (type & REF_ISSYMREF))
>                 lock->force_write = 1;
>
> --
> 2.1.4
>

> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> > If a reference is missing, its SHA-1 will be null_sha1, which can't
> > possibly match a new value that ref_transaction_commit() is trying to
> > update it to. So there is no need to set force_write in this scenario.
> >
> 
> This commit reverts half the lines of 5bdd8d4a3062a (2008-11, do not
> force write of packed refs). And reading both commit messages, they
> seem to contradict each other. (Both agree on  "If a reference is
> missing, its SHA-1 will be null_sha1 as provided by resolve_ref", but
> the conclusion seems to be different.)

> On Tue, Feb 10, 2015 at 03:24:47PM -0800, Stefan Beller wrote:
>
>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> > If a reference is missing, its SHA-1 will be null_sha1, which can't
>> > possibly match a new value that ref_transaction_commit() is trying to
>> > update it to. So there is no need to set force_write in this scenario.
>> >
>>
>> This commit reverts half the lines of 5bdd8d4a3062a (2008-11, do not
>> force write of packed refs). And reading both commit messages, they
>> seem to contradict each other. (Both agree on  "If a reference is
>> missing, its SHA-1 will be null_sha1 as provided by resolve_ref", but
>> the conclusion seems to be different.)
>
> Most of the lines of 5bdd8d4a3062a that are being reverted here are
> caching the is_null_sha1() check in the "missing" variable. And that's
> a cleanup in this patch that is not strictly necessary ("missing" would
> only be used once, so it becomes noise).
>
> The interesting thing in the earlier commit was to use the null sha1 to
> cause a force-write, rather than lstat()ing the filesystem. And here we
> are saying the force-write is not necessary at all, no matter what
> storage scheme is used. So I don't think there is any contradiction
> between the two.
>
> Is this patch correct that the force-write is not necessary? I think so.
> The force-write flag comes from:
>
> commit 732232a123e1e61e38babb1c572722bb8a189ba3
> Author: Shawn Pearce <spearce@spearce.org>
> Date:   Fri May 19 03:29:05 2006 -0400
>
>     Force writing ref if it doesn't exist.
>
>     Normally we try to skip writing a ref if its value hasn't changed
>     but in the special case that the ref doesn't exist but the new
>     value is going to be 0{40} then force writing the ref anyway.
>
> but I am not sure that logic still holds (if it ever did). We do not ever write
> 0{40} into a ref value.
>
> -Peff

> On Tue, Feb 10, 2015 at 03:24:47PM -0800, Stefan Beller wrote:
> 
>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> If a reference is missing, its SHA-1 will be null_sha1, which can't
>>> possibly match a new value that ref_transaction_commit() is trying to
>>> update it to. So there is no need to set force_write in this scenario.
>>>
>>
>> This commit reverts half the lines of 5bdd8d4a3062a (2008-11, do not
>> force write of packed refs). And reading both commit messages, they
>> seem to contradict each other. (Both agree on  "If a reference is
>> missing, its SHA-1 will be null_sha1 as provided by resolve_ref", but
>> the conclusion seems to be different.)
> 
> Most of the lines of 5bdd8d4a3062a that are being reverted here are
> caching the is_null_sha1() check in the "missing" variable. And that's
> a cleanup in this patch that is not strictly necessary ("missing" would
> only be used once, so it becomes noise).
> 
> The interesting thing in the earlier commit was to use the null sha1 to
> cause a force-write, rather than lstat()ing the filesystem. And here we
> are saying the force-write is not necessary at all, no matter what
> storage scheme is used. So I don't think there is any contradiction
> between the two.
> 
> Is this patch correct that the force-write is not necessary? I think so.
> The force-write flag comes from:
> 
> commit 732232a123e1e61e38babb1c572722bb8a189ba3
> Author: Shawn Pearce <spearce@spearce.org>
> Date:   Fri May 19 03:29:05 2006 -0400
> 
>     Force writing ref if it doesn't exist.
>     
>     Normally we try to skip writing a ref if its value hasn't changed
>     but in the special case that the ref doesn't exist but the new
>     value is going to be 0{40} then force writing the ref anyway.
> 
> but I am not sure that logic still holds (if it ever did). We do not ever write
> 0{40} into a ref value.

> ---all::
> -       Instead of listing <refs> explicitly, prune all refs.
> +--stale-fix::
> +       This revamps the logic -- the definition of "broken commit"
> +       becomes: a commit that is not reachable from any of the refs and
> +       there is a missing object among the commit, tree, or blob
> +       objects reachable from it that is not reachable from any of the
> +       refs.

> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> ---all::
>> -       Instead of listing <refs> explicitly, prune all refs.
>> +--stale-fix::
>> +       This revamps the logic -- the definition of "broken commit"
>> +       becomes: a commit that is not reachable from any of the refs and
>> +       there is a missing object among the commit, tree, or blob
>> +       objects reachable from it that is not reachable from any of the
>> +       refs.
> 
> --stale-fix becomes more and more irrelevant over time,
> so why not put in at the very end even after --all ?
> 
> Thinking out loud:
> (--expire=,--expire-unreachable= and --stale-fix) look like a group
> and (--updateref --rewrite --verbose and --all) also feel like a group,
> so you wanted to keep --stale-fix after --expire-unreachable= ?

> On 02/11/2015 12:42 AM, Stefan Beller wrote:
>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> ---all::
>>> -       Instead of listing <refs> explicitly, prune all refs.
>>> +--stale-fix::
>>> +       This revamps the logic -- the definition of "broken commit"
>>> +       becomes: a commit that is not reachable from any of the refs and
>>> +       there is a missing object among the commit, tree, or blob
>>> +       objects reachable from it that is not reachable from any of the
>>> +       refs.
>> 
>> --stale-fix becomes more and more irrelevant over time,
>> so why not put in at the very end even after --all ?
>> 
>> Thinking out loud:
>> (--expire=,--expire-unreachable= and --stale-fix) look like a group
>> and (--updateref --rewrite --verbose and --all) also feel like a group,
>> so you wanted to keep --stale-fix after --expire-unreachable= ?
>
> Yes, that's what I was thinking.
>
> But you are right that the docs could be improved even more, so in the
> re-roll I will make some further changes.

> If we are expiring reflog entries for a symbolic reference, then how
> should --updateref be handled if the newest reflog entry is expired?
>
> Option 1: Update the referred-to reference. (This is what the current
> code does.) This doesn't make sense, because the referred-to reference
> has its own reflog, which hasn't been rewritten.
>
> Option 2: Update the symbolic reference itself (as in, REF_NODEREF).
> This would convert the symbolic reference into a non-symbolic
> reference (e.g., detaching HEAD), which is surely not what a user
> would expect.
>
> Option 3: Error out. This is plausible, but it would make the
> following usage impossible:
>
>     git reflog expire ... --updateref --all
>
> Option 4: Ignore --updateref for symbolic references.
>

>
> Note: there are still other problems in this code that will be fixed
> in a moment.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  Documentation/git-reflog.txt |  3 ++-
>  refs.c                       | 15 ++++++++++++---
>  2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
> index f15a48e..9b87b46 100644
> --- a/Documentation/git-reflog.txt
> +++ b/Documentation/git-reflog.txt
> @@ -85,7 +85,8 @@ them.
>
>  --updateref::
>         Update the ref with the sha1 of the top reflog entry (i.e.
> -       <ref>@\{0\}) after expiring or deleting.
> +       <ref>@\{0\}) after expiring or deleting.  (This option is
> +       ignored for symbolic references.)
>
>  --rewrite::
>         While expiring or deleting, adjust each reflog entry to ensure
> diff --git a/refs.c b/refs.c
> index b083858..c0001da 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -4025,6 +4025,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
>         struct ref_lock *lock;
>         char *log_file;
>         int status = 0;
> +       int type;
>
>         memset(&cb, 0, sizeof(cb));
>         cb.flags = flags;
> @@ -4036,7 +4037,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
>          * reference itself, plus we might need to update the
>          * reference if --updateref was specified:
>          */
> -       lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, NULL);
> +       lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, &type);
>         if (!lock)
>                 return error("cannot lock ref '%s'", refname);
>         if (!reflog_exists(refname)) {
> @@ -4073,10 +4074,18 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
>         (*cleanup_fn)(cb.policy_cb);
>
>         if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
> +               /*
> +                * It doesn't make sense to adjust a reference pointed
> +                * to by a symbolic ref based on expiring entries in
> +                * the symbolic reference's reflog.
> +                */
> +               int update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&
> +                       ~(type & REF_ISSYMREF);
> +
>                 if (close_lock_file(&reflog_lock)) {
>                         status |= error("couldn't write %s: %s", log_file,
>                                         strerror(errno));
> -               } else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
> +               } else if (update &&
>                         (write_in_full(lock->lock_fd,
>                                 sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
>                          write_str_in_full(lock->lock_fd, "\n") != 1 ||
> @@ -4087,7 +4096,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
>                 } else if (commit_lock_file(&reflog_lock)) {
>                         status |= error("unable to commit reflog '%s' (%s)",
>                                         log_file, strerror(errno));
> -               } else if ((flags & EXPIRE_REFLOGS_UPDATE_REF) && commit_ref(lock)) {
> +               } else if (update && commit_ref(lock)) {
>                         status |= error("couldn't set %s", lock->ref_name);
>                 }
>         }
> --
> 2.1.4
>

> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> 
>> If we are expiring reflog entries for a symbolic reference, then how
>> should --updateref be handled if the newest reflog entry is expired?
>>
>> Option 1: Update the referred-to reference. (This is what the current
>> code does.) This doesn't make sense, because the referred-to reference
>> has its own reflog, which hasn't been rewritten.
>>
>> Option 2: Update the symbolic reference itself (as in, REF_NODEREF).
>> This would convert the symbolic reference into a non-symbolic
>> reference (e.g., detaching HEAD), which is surely not what a user
>> would expect.
>>
>> Option 3: Error out. This is plausible, but it would make the
>> following usage impossible:
>>
>>     git reflog expire ... --updateref --all
>>
>> Option 4: Ignore --updateref for symbolic references.
>>
> 
> Ok let me ask a question first about the symbolic refs.
> 
> We used to use symbolic links for that, but because of
> portability issues we decided to not make it a link, but rather
> a standard file containing the pointing link (The content of
> .git/HEAD is "ref: refs/heads/master\n" except when detached)
> 
> So this is the only distinction? Or is there also a concept of
> symbolic links/pointers for the reflog handling?

>> We choose to implement option 4.
> 
> You're only saying why the other options are insane, not why this
> is sane.
> 
> Also I'd rather tend for option 3 than 4, as it is a safety measure
> (assuming we give a hint to the user what the problem is, and
> how it is circumventable)

> On 02/11/2015 01:44 AM, Stefan Beller wrote:
>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>
>>> If we are expiring reflog entries for a symbolic reference, then how
>>> should --updateref be handled if the newest reflog entry is expired?
>>>
>>> Option 1: Update the referred-to reference. (This is what the current
>>> code does.) This doesn't make sense, because the referred-to reference
>>> has its own reflog, which hasn't been rewritten.
>>>
>>> Option 2: Update the symbolic reference itself (as in, REF_NODEREF).
>>> This would convert the symbolic reference into a non-symbolic
>>> reference (e.g., detaching HEAD), which is surely not what a user
>>> would expect.
>>>
>>> Option 3: Error out. This is plausible, but it would make the
>>> following usage impossible:
>>>
>>>     git reflog expire ... --updateref --all
>>>
>>> Option 4: Ignore --updateref for symbolic references.
>>>
>>
>> Ok let me ask a question first about the symbolic refs.
>>
>> We used to use symbolic links for that, but because of
>> portability issues we decided to not make it a link, but rather
>> a standard file containing the pointing link (The content of
>> .git/HEAD is "ref: refs/heads/master\n" except when detached)
>>
>> So this is the only distinction? Or is there also a concept of
>> symbolic links/pointers for the reflog handling?
>
> A symbolic reference can have a reflog just like a normal reference can.

>
> When a reference is updated through a symbolic reference, then
> write_ref_sha1() writes a reflog entry for both the reference and the
> symbolic reference. Also (as an extra kludge), if *any* reference is
> updated directly and it happens to be the current HEAD reference, then
> an entry is added to HEAD's reflog.

>
> "HEAD" is the only symbolic reference that is ever transferred across
> repositories.
>
> Symbolic references are always stored loose (i.e., not in packed-refs).
>
> Does that answer your questions?
>
>>> We choose to implement option 4.
>>
>> You're only saying why the other options are insane, not why this
>> is sane.
>>
>> Also I'd rather tend for option 3 than 4, as it is a safety measure
>> (assuming we give a hint to the user what the problem is, and
>> how it is circumventable)
>
> This is a pretty exotic usage. I can't think of any real-life use case
> for using "--updateref" together with a symbolic reference. In our
> entire code base, "--updateref" is only used a single time, in
> "git-stash.sh", and that is always for "refs/stash", which is never a
> symbolic reference. "git-stash" itself is implemented in a very stylized
> way ("stylized" being a polite way of saying "bizarre"), and I doubt
> that there are many more users of this option in the wild, let alone
> "--updateref" together with a symbolic reference.
>
> So, honestly, I don't think it is worth the effort of deciding between 3
> vs. 4. Since 4 is easier to implement (and already implemented), I'd
> rather leave it as is. If you want to submit a patch implementing 3, I
> won't argue against it.

>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
>

> This is a pretty exotic usage. I can't think of any real-life use case
> for using "--updateref" together with a symbolic reference. In our
> entire code base, "--updateref" is only used a single time, in
> "git-stash.sh", and that is always for "refs/stash", which is never a
> symbolic reference.

> "git-stash" itself is implemented in a very stylized
> way ("stylized" being a polite way of saying "bizarre"), and I doubt
> that there are many more users of this option in the wild, let alone
> "--updateref" together with a symbolic reference.
>
> So, honestly, I don't think it is worth the effort of deciding between 3
> vs. 4.

>  	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
> +		/*
> +		 * It doesn't make sense to adjust a reference pointed
> +		 * to by a symbolic ref based on expiring entries in
> +		 * the symbolic reference's reflog.
> +		 */
> +		int update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&
> +			~(type & REF_ISSYMREF);
> +

> On Mon, Feb 09, 2015 at 10:12:42AM +0100, Michael Haggerty wrote:
> 
>>  	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
>> +		/*
>> +		 * It doesn't make sense to adjust a reference pointed
>> +		 * to by a symbolic ref based on expiring entries in
>> +		 * the symbolic reference's reflog.
>> +		 */
>> +		int update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&
>> +			~(type & REF_ISSYMREF);
>> +
> 
> Isn't this second clause tautological? Unless REF_ISSYMREF covers all
> bits in "type", then "type & REF_ISSYMREF" must always have at least one
> bit 0, and negating it becomes non-zero. Did you mean:
> 
>    !(type & REF_ISSYMREF)
> 
> ?

> Currently, if --updateref is specified and the very last reflog entry
> is expired or deleted, the reference's value is set to 0{40}. This is
> an invalid state of the repository, and breaks, for example, "git
> fsck" and "git for-each-ref".
>
> The only place we use --updateref in our own code is when dropping
> stash entries. In that code, the very next step is to check if the
> reflog has been made empty, and if so, delete the "refs/stash"
> reference entirely. Thus that code path ultimately leaves the
> repository in a valid state.
>
> But we don't want the repository in an invalid state even temporarily,
> and we don't want leave an invalid state if other callers of "git

> reflog expire|delete --updateref" don't think to do the extra cleanup
> step.
>
> So, if "git reflog expire|delete" leaves no more entries in the
> reflog, just leave the reference un-updated.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

> On Mon, Feb 9, 2015 at 4:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> [...]
>> But we don't want the repository in an invalid state even temporarily,
>> and we don't want leave an invalid state if other callers of "git
> 
> s/want/want to/
> [...]

>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 1b2a497..3fcf342 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -4037,7 +4037,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
>          * reference itself, plus we might need to update the
>          * reference if --updateref was specified:
>          */
> -       lock = lock_ref_sha1_basic(refname, sha1, NULL, 0, &type);
> +       lock = lock_ref_sha1_basic(refname, sha1, NULL, REF_NODEREF, &type);
>         if (!lock)
>                 return error("cannot lock ref '%s'", refname);
>         if (!reflog_exists(refname)) {
> --
> 2.1.4
>

> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> When processing the reflog of a symbolic ref, hold the lock on the
>> symbolic reference itself, not on the reference that it points to.
>
> I am not sure if that makes sense.
> So when expiring HEAD, you want to have a .git/HEAD.lock file
> instead of a .git/refs/heads/master.lock file?
>
> What would happen if there is a concurrent modification
> to the master branch?

> Stefan Beller <sbeller@google.com> writes:
>
>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> When processing the reflog of a symbolic ref, hold the lock on the
>>> symbolic reference itself, not on the reference that it points to.
>>
>> I am not sure if that makes sense.
>> So when expiring HEAD, you want to have a .git/HEAD.lock file
>> instead of a .git/refs/heads/master.lock file?
>>
>> What would happen if there is a concurrent modification
>> to the master branch?
>
> The HEAD may be pointing at 'master' and the other party that is
> trying to modify it would fail when it tries to update the reflog
> for HEAD thanks to HEAD.lock being held by us.  The HEAD may be
> pointing at 'next' and the other part that updates 'master' would
> not try to modify HEAD reflog and we do not conflict.
>
> At least, I think that is the rationale behind this change.

> On Wed, Feb 11, 2015 at 2:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>>> When processing the reflog of a symbolic ref, hold the lock on the
>>>> symbolic reference itself, not on the reference that it points to.
>>>
>>> I am not sure if that makes sense.
>>> So when expiring HEAD, you want to have a .git/HEAD.lock file
>>> instead of a .git/refs/heads/master.lock file?
>>>
>>> What would happen if there is a concurrent modification
>>> to the master branch?
>>
>> The HEAD may be pointing at 'master' and the other party that is
>> trying to modify it would fail when it tries to update the reflog
>> for HEAD thanks to HEAD.lock being held by us.  The HEAD may be
>> pointing at 'next' and the other part that updates 'master' would
>> not try to modify HEAD reflog and we do not conflict.
>>
>> At least, I think that is the rationale behind this change.
> 
> That makes sense! Do we have documentation on symrefs?
> 
> Originally I was wondering if this would make things
> complicated for  symbolic branches which are not HEAD.
> Then you could update the branch pointed to, because it
> has no lock as the lock is on the symref. On the other hand
> this seems to be an improvement, as you cannot move the
> symref itself, as it has the lock and we don't really have other
> symrefs?

> On 02/12/2015 12:25 AM, Stefan Beller wrote:
>> On Wed, Feb 11, 2015 at 2:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Stefan Beller <sbeller@google.com> writes:
>>>
>>>> On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>>>> When processing the reflog of a symbolic ref, hold the lock on the
>>>>> symbolic reference itself, not on the reference that it points to.
>>>>
>>>> I am not sure if that makes sense.
>>>> So when expiring HEAD, you want to have a .git/HEAD.lock file
>>>> instead of a .git/refs/heads/master.lock file?
>>>>
>>>> What would happen if there is a concurrent modification
>>>> to the master branch?
>>>
>>> The HEAD may be pointing at 'master' and the other party that is
>>> trying to modify it would fail when it tries to update the reflog
>>> for HEAD thanks to HEAD.lock being held by us.  The HEAD may be
>>> pointing at 'next' and the other part that updates 'master' would
>>> not try to modify HEAD reflog and we do not conflict.
>>>
>>> At least, I think that is the rationale behind this change.
>>
>> That makes sense! Do we have documentation on symrefs?
>>
>> Originally I was wondering if this would make things
>> complicated for  symbolic branches which are not HEAD.
>> Then you could update the branch pointed to, because it
>> has no lock as the lock is on the symref. On the other hand
>> this seems to be an improvement, as you cannot move the
>> symref itself, as it has the lock and we don't really have other
>> symrefs?
>
> The convention is that holding lock $GIT_DIR/$refname.lock (where
> $refname might be, for example, "HEAD" or "refs/heads/master") protects
> two things:
>
> * The loose-reference file $GIT_DIR/$refname
> * The reflog file $GIT_DIR/logs/$refname
>
> And this makes sense:
>
> Suppose that HEAD is refs/heads/master. These two thing have independent
> reflogs, so there is no reason that one process can't be expiring the
> reflog of HEAD while another expires the reflog of refs/heads/master.
>
> The behavior before this patch was that the reflog for "HEAD" was
> modified while holding the reflog for "refs/heads/master". This is too
> strict and would make those two processes contend unnecessarily.
>
> I can't think of a reason that the current behavior is unsafe. But it's
> more restrictive than necessary, and more confusing than necessary. My
> guess is that it was unintended (i.e., a bug). It dates from
>
>     68db31cc28 (2007-05-09) git-update-ref: add --no-deref option for
> overwriting/detaching ref
>
> which initially added the REF_NODEREF constant and probably forgot that
> the new flag should be used in this invocation.
>
> However, another important question is whether other Git implementations
> have copied this unusual locking policy. If so, that would be a reason
> not to change it. I just pinged the libgit2 maintainer to find out their
> policy. Maybe you could find out about JGit?

>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
>

> On Thu, Feb 12, 2015 at 8:52 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> However, another important question is whether other Git implementations
>> have copied this unusual locking policy. If so, that would be a reason
>> not to change it. I just pinged the libgit2 maintainer to find out their
>> policy. Maybe you could find out about JGit?
> 
> I could not really find reflog expiration for jgit for a while, but
> then I stumbled upon this:
> 
>     // TODO: implement reflog_expire(pm, repo);
> 
> so I think it's safe to change it in git.git for now.

>
> What is the best way forward?
>
> Switching to holding only "HEAD.lock" while updating "logs/HEAD" is the
> right solution, but it would introduce an incompatibility with old
> versions of Git and libgit2 (and maybe JGit?) Probably nobody would
> notice, but who knows?
>
> Therefore, I propose that we hold *both* "HEAD.lock" *and*
> "refs/heads/master.lock" when modifying "logs/HEAD" (even when running
> "reflog expire" or "reflog delete"). I think this will be compatible
> with old versions of Git and libgit2, and it will also fix some design
> problems mentioned above. Plus, it will prevent updates to the two
> reflogs from appearing out of order relative to each other.

>
> Someday, when old clients have been retired, we can optionally switch to
> locking *only* "HEAD.lock" when modifying "logs/HEAD".
>
> What do people think?
> Michael
>
> [1] https://github.com/libgit2/libgit2/issues/2902
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
>

> I also realized that Git's current policy is probably not tenable if one
> process is re-seating a symref at the same time as another process is
> expiring its reflog. The "git reflog expire HEAD" might grab
> "refs/heads/master.lock" then start rewriting "logs/HEAD". Meanwhile,
> "git checkout foo" would grab "HEAD.lock" (not being blocked by the
> "expire" process), then rewrite it to "ref: refs/heads/foo", then grab
> "refs/heads/foo.lock" before updating "logs/HEAD". So both processes
> could be writing "logs/HEAD" at the same time.
> ...
> Switching to holding only "HEAD.lock" while updating "logs/HEAD" is the
> right solution,...

> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> I also realized that Git's current policy is probably not tenable if one
>> process is re-seating a symref at the same time as another process is
>> expiring its reflog. The "git reflog expire HEAD" might grab
>> "refs/heads/master.lock" then start rewriting "logs/HEAD". Meanwhile,
>> "git checkout foo" would grab "HEAD.lock" (not being blocked by the
>> "expire" process), then rewrite it to "ref: refs/heads/foo", then grab
>> "refs/heads/foo.lock" before updating "logs/HEAD". So both processes
>> could be writing "logs/HEAD" at the same time.
>> ...
>> Switching to holding only "HEAD.lock" while updating "logs/HEAD" is the
>> right solution,...
>
> We convinced ourselves that not locking the symref is wrong, but
> have we actually convinced us that not locking the underlying ref,
> as long as we have a lock on the symref, is safe?
>
> To protect you, the holder of a lock on refs/remotes/origin/HEAD
> that happens to point at refs/remotes/origin/next, from somebody who
> is updating the underlying refs/remotes/origin/next directly without
> going through the symbolic ref (e.g. receive-pack), wouldn't the
> other party need to find any and all symbolic refs that point at the
> underlying ref and take locks on them?

>
> As dereferencing a symbolic ref in the forward direction is much
> cheaper than in the reverse, and because you need to dereference it
> anyway, I wonder if we want the endgame to be "hold locks on both",
> not "just hold a lock on the symlink".

> On Fri, Feb 13, 2015 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> We convinced ourselves that not locking the symref is wrong, but
>> have we actually convinced us that not locking the underlying ref,
>> as long as we have a lock on the symref, is safe?
>>
>> To protect you, the holder of a lock on refs/remotes/origin/HEAD
>> that happens to point at refs/remotes/origin/next, from somebody who
>> is updating the underlying refs/remotes/origin/next directly without
>> going through the symbolic ref (e.g. receive-pack), wouldn't the
>> other party need to find any and all symbolic refs that point at the
>> underlying ref and take locks on them?
>
> As we're just modifying the ref log of HEAD in this case, we don't bother
> with where the HEAD points to. The other party may change
> refs/remotes/origin/next without us noticing, but we don't care here as
> all we do is rewriting the ref log for HEAD.
>
> If the other party were to modify HEAD (point it to some other branch, or
> forward the branch pointed to), they'd be expected to lock HEAD and
> would fail as we have the lock?

> Stefan Beller <sbeller@google.com> writes:
>
>> On Fri, Feb 13, 2015 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> We convinced ourselves that not locking the symref is wrong, but
>>> have we actually convinced us that not locking the underlying ref,
>>> as long as we have a lock on the symref, is safe?
>>>
>>> To protect you, the holder of a lock on refs/remotes/origin/HEAD
>>> that happens to point at refs/remotes/origin/next, from somebody who
>>> is updating the underlying refs/remotes/origin/next directly without
>>> going through the symbolic ref (e.g. receive-pack), wouldn't the
>>> other party need to find any and all symbolic refs that point at the
>>> underlying ref and take locks on them?
>>
>> As we're just modifying the ref log of HEAD in this case, we don't bother
>> with where the HEAD points to. The other party may change
>> refs/remotes/origin/next without us noticing, but we don't care here as
>> all we do is rewriting the ref log for HEAD.
>>
>> If the other party were to modify HEAD (point it to some other branch, or
>> forward the branch pointed to), they'd be expected to lock HEAD and
>> would fail as we have the lock?
>
> I was not talking about HEAD in what you are responding to, though.
> Don't we maintain a reflog on refs/remotes/origin/HEAD?  Is it OK to
> allow its entries to become inconsistent with the underlying ref?
>

> On Fri, Feb 13, 2015 at 10:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> On Fri, Feb 13, 2015 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>>> We convinced ourselves that not locking the symref is wrong, but
>>>> have we actually convinced us that not locking the underlying ref,
>>>> as long as we have a lock on the symref, is safe?
>>>>
>>>> To protect you, the holder of a lock on refs/remotes/origin/HEAD
>>>> that happens to point at refs/remotes/origin/next, from somebody who
>>>> is updating the underlying refs/remotes/origin/next directly without
>>>> going through the symbolic ref (e.g. receive-pack), wouldn't the
>>>> other party need to find any and all symbolic refs that point at the
>>>> underlying ref and take locks on them?
>>>
>>> As we're just modifying the ref log of HEAD in this case, we don't bother
>>> with where the HEAD points to. The other party may change
>>> refs/remotes/origin/next without us noticing, but we don't care here as
>>> all we do is rewriting the ref log for HEAD.
>>>
>>> If the other party were to modify HEAD (point it to some other branch, or
>>> forward the branch pointed to), they'd be expected to lock HEAD and
>>> would fail as we have the lock?
>>
>> I was not talking about HEAD in what you are responding to, though.
>> Don't we maintain a reflog on refs/remotes/origin/HEAD?  Is it OK to
>> allow its entries to become inconsistent with the underlying ref?
>
> Yes we do have a HEAD ref log, and that ref log would differ from
> the underlying ref log. I don't know if that is desired, but I
> would tend to not like it.

> [...]
> As we are trying to see a way to move forward to do the right thing
> around reflog, I was wondering if locking only the symbolic ref is a
> sensible endgame.  "The right thing" being:
> 
>    When a symbolic ref S points at underlying ref R, and if R's tip
>    changes from X to Y, we would want to know from the reflog of S
>    that S used to point at X and then changed to point at Y.

> Now back to the real world. Currently, if R is changed *through* a
> symbolic reference S, then the reflogs for both R and S are updated, but
> not the reflogs for any other symbolic references that might point at R.
> If R is changed directly, then no symref's reflogs are affected, except
> for the special case that HEAD's reflog is changed if it points directly
> at R. This limitation is a hack to avoid having to walk symrefs
> backwards to find any symrefs that might be pointing at R.

> It might actually not be extremely expensive to follow symrefs
> backwards. Symbolic references cannot be packed, so we would only have
> to scan the loose references; we could ignore packed refs. But it would
> still be a lot more expensive than just updating one file. I don't know
> that it's worth it, given that symbolic references are used so sparingly.

> I think that the rule about locks as expressed above can be carried over
> the the real world:
>
>     We should hold the locks on exactly those references (symbolic
>     or regular) whose reflogs we plan to change. We should acquire all
>     of the locks before making any changes.

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Now back to the real world. Currently, if R is changed *through* a
>> symbolic reference S, then the reflogs for both R and S are updated, but
>> not the reflogs for any other symbolic references that might point at R.
>> If R is changed directly, then no symref's reflogs are affected, except
>> for the special case that HEAD's reflog is changed if it points directly
>> at R. This limitation is a hack to avoid having to walk symrefs
>> backwards to find any symrefs that might be pointing at R.
> 
> Yup.
> 
>> It might actually not be extremely expensive to follow symrefs
>> backwards. Symbolic references cannot be packed, so we would only have
>> to scan the loose references; we could ignore packed refs. But it would
>> still be a lot more expensive than just updating one file. I don't know
>> that it's worth it, given that symbolic references are used so sparingly.
> 
> I personally do not think it is worth it.  I further think that it
> would be perfectly OK to do one of the following:
> 
>     - We only maintain reflogs for $GIT_DIR/HEAD; no other symrefs
>       get their own reflog, and we only check $GIT_DIR/HEAD when
>       updating refs/heads/* and no other refs for direct reference
>       (i.e. HEAD -> refs/something/else -> refs/heads/master symref
>       chain is ignored).
> 
>     - In addition to the above, we also maintain reflogs for
>       $GIT_DIR/refs/remotes/*/HEAD but support only when they
>       directly point into a remote tracking branch in the same
>       hierarchy.  $GIT_DIR/refs/remotes/foo/HEAD that points at
>       $GIT_DIR/refs/remotes/bar/master is ignored and will get an
>       undefined behaviour.

>> I think that the rule about locks as expressed above can be carried over
>> the the real world:
>>
>>     We should hold the locks on exactly those references (symbolic
>>     or regular) whose reflogs we plan to change. We should acquire all
>>     of the locks before making any changes.
> 
> Sure.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> Notes:
>     When this patch series is applied, you only have 3 occurences of force_write.
>     
>     1. In the struct as an undocumented int.
>     2. In  lock_ref_sha1_basic:
>     	if ((flags & REF_NODEREF) && (type & REF_ISSYMREF))
>     		lock->force_write = 1;
>     3: In ref_transaction_commit:
>     	/* Perform updates first so live commits remain referenced */
>     	for (i = 0; i < n; i++) {
>     		struct ref_update *update = updates[i];
>     
>     		if (!is_null_sha1(update->new_sha1)) {
>     			if (!update->lock->force_write &&
>     			    !hashcmp(update->lock->old_sha1, update->new_sha1)) {
>     				unlock_ref(update->lock);
>     				update->lock = NULL;
>     			} else if (write_ref_sha1(update->lock, update->new_sha1,
>     						  update->msg)) {
>     				update->lock = NULL; /* freed by write_ref_sha1 */
>     				strbuf_addf(err, "Cannot update the ref '%s'.",
>     					    update->refname);
>     				ret = TRANSACTION_GENERIC_ERROR;
>     				goto cleanup;
>     			} else {
>     				/* freed by write_ref_sha1(): */
>     				update->lock = NULL;
>     			}
>     		}
>     	}
>     
>     So maybe we can solve it even more elegant by omiting the first 2 occurences and
>     directly check the type and flags in ref_transaction_commit.
>     
>     Maybe this makes sense to go on top of that series?
> [...]