[PATCH 0/8] Fix some problems with reflog expiration

[PATCH 0/8] Fix some problems with reflog expiration

[Michael Haggerty]

In addition to a few cleanups, this patch series fixes some problems
that I noticed when working on mh/reflog-expire:

* Ignore '--updateref' when expiring the reflog of a symbolic
  reference, because the alternatives are all pretty silly. See the
  log message for commit 6/8 for more information.

* Ignore '--updateref' when *all* reflog entries are expired by "git
  reflog expire" or "git reflog delete". Currently, this sets the
  reference to 0{40}, which breaks the repository. (Another
  alternative would be to delete the reference in this situation, but
  that seemed too radical to me somehow.)

* When expiring the reflog for a symbolic reference, lock the symbolic
  reference rather than its referent.

This patch series applies on top of master merged together with
sb/atomic-push, like the "refs-have-new" patch series that I just
submitted. It is also available from my GitHub account [1] as branch
"expire-updateref-fixes".

There is a minor conflict between this patch series and
"mh/refs-have-new":

<<<<<<< HEAD
		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)) {
||||||| merged common ancestors
		if (!is_null_sha1(update->new_sha1)) {
			if (write_ref_sha1(update->lock, update->new_sha1,
					   update->msg)) {
=======
		if ((flags & REF_HAVE_NEW) && !is_null_sha1(update->new_sha1)) {
			if (write_ref_sha1(update->lock, update->new_sha1,
					   update->msg)) {
>>>>>>> refs-have-new

It can be resolved in the obvious way:

<<<<<<<
		if ((flags & REF_HAVE_NEW) && !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)) {
>>>>>>>

By the way, both of these patch series conflict with
sb/atomic-push-fix, which is in pu. My understanding is that Stefan
wants to rework that patch series anyway, but if not I would be happy
to show how to resolve the conflicts.

Michael

[1] https://github.com/mhagger/git

Michael Haggerty (8):
  write_ref_sha1(): remove check for lock == NULL
  write_ref_sha1(): Move write elision test to callers
  lock_ref_sha1_basic(): do not set force_write for missing references
  reflog: fix documentation
  reflog: rearrange the manpage
  reflog_expire(): ignore --updateref for symbolic references
  reflog_expire(): never update a reference to null_sha1
  reflog_expire(): lock symbolic refs themselves, not their referent

 Documentation/git-reflog.txt | 65 +++++++++++++++++++++++---------------------
 builtin/reflog.c             |  2 +-
 refs.c                       | 55 ++++++++++++++++++++-----------------
 3 files changed, 65 insertions(+), 57 deletions(-)

-- 
2.1.4

[PATCH 1/8] write_ref_sha1(): remove check for lock == NULL

[Michael Haggerty]

None of the callers pass NULL to this function, and there doesn't seem
to be any usefulness to allowing them to do so.

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

Re: [PATCH 1/8] write_ref_sha1(): remove check for lock == NULL

[Stefan Beller]

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.

so
Reviewed-by: Stefan Beller <sbeller@google.com>

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

Re: [PATCH 1/8] write_ref_sha1(): remove check for lock == NULL

[Jeff King]

On Tue, Feb 10, 2015 at 02:52:23PM -0800, Stefan Beller wrote:

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

Thanks, I was iffy on this change for the same reason, but after your
explanation, I too think it is reasonable.

-Peff

[PATCH 2/8] write_ref_sha1(): Move write elision test to callers

[Michael Haggerty]

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 */
 		}
 	}
 
-- 
2.1.4

Re: [PATCH 2/8] write_ref_sha1(): Move write elision test to callers

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

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

Good.  I recall that this convoluted "the callee checks if the
values are the same so we need to tell it not to do that" logic
looked very disturbing.



> 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 */
>  		}
>  	}

[PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references

[Michael Haggerty]

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.

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

Re: [PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references

[Stefan Beller]

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 the other hand, there is more than 6 years difference, so I guess
the meaning and implications of some variables and functions may have
slightly changed.

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

Re: [PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references

[Jeff King]

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

Re: [PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references

[Stefan Beller]

On Tue, Feb 10, 2015 at 4:05 PM, Jeff King <peff@peff.net> wrote:
> 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

Makes sense.

Re: [PATCH 3/8] lock_ref_sha1_basic(): do not set force_write for missing references

[Michael Haggerty]

On 02/11/2015 01:05 AM, Jeff King wrote:
> 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.

I don't understand that old commit, either. Maybe there was an idea of
storing 0{40} in a loose ref file to mark a packed reference as deleted?
CC to Shawn Pearce in case he can shed some light on the situation.

I still think that my change is OK, because we definitely don't want to
write 0{40} to any loose reference file. The reference-reading code
can't deal with it, so this would break the repository.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH 4/8] reflog: fix documentation

[Michael Haggerty]

Update the "git reflog" usage documentation in the manpage and the
command help to match the current reality.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/git-reflog.txt | 6 ++++--
 builtin/reflog.c             | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
index 70791b9..b410ee6 100644
--- a/Documentation/git-reflog.txt
+++ b/Documentation/git-reflog.txt
@@ -17,9 +17,11 @@ The command takes various subcommands, and different options
 depending on the subcommand:
 
 [verse]
-'git reflog expire' [--dry-run] [--stale-fix] [--verbose]
+'git reflog expire' [--dry-run] [--verbose]
+	[--rewrite] [--updateref] [--stale-fix]
 	[--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...
-'git reflog delete' ref@\{specifier\}...
+'git reflog delete' [--dry-run] [--verbose]
+	[--rewrite] [--updateref] ref@\{specifier\}...
 'git reflog' ['show'] [log-options] [<ref>]
 
 Reflog is a mechanism to record when the tip of branches are
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 49c64f9..d89a6dd 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -13,7 +13,7 @@
  */
 
 static const char reflog_expire_usage[] =
-"git reflog expire [--verbose] [--dry-run] [--stale-fix] [--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...";
+"git reflog expire [--verbose] [--dry-run] [--rewrite] [--updateref] [--stale-fix] [--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...";
 static const char reflog_delete_usage[] =
 "git reflog delete [--verbose] [--dry-run] [--rewrite] [--updateref] <refs>...";
 
-- 
2.1.4

Re: [PATCH 4/8] reflog: fix documentation

[Stefan Beller]

On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Update the "git reflog" usage documentation in the manpage and the
> command help to match the current reality.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

Reviewed-by: Stefan Beller <sbeller@google.com>
> ---

[PATCH 5/8] reflog: rearrange the manpage

[Michael Haggerty]

Rearrange the "git reflog" manpage to list more common operations
earlier.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/git-reflog.txt | 56 ++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
index b410ee6..f15a48e 100644
--- a/Documentation/git-reflog.txt
+++ b/Documentation/git-reflog.txt
@@ -17,21 +17,21 @@ The command takes various subcommands, and different options
 depending on the subcommand:
 
 [verse]
+'git reflog' ['show'] [log-options] [<ref>]
 'git reflog expire' [--dry-run] [--verbose]
 	[--rewrite] [--updateref] [--stale-fix]
 	[--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...
 'git reflog delete' [--dry-run] [--verbose]
 	[--rewrite] [--updateref] ref@\{specifier\}...
-'git reflog' ['show'] [log-options] [<ref>]
 
-Reflog is a mechanism to record when the tip of branches are
-updated.  This command is to manage the information recorded in it.
+Reflog is a mechanism to record when the tips of branches are updated.
+The reflog is useful in various Git commands, to specify the old value
+of a reference. For example, `HEAD@{2}` means "where HEAD used to be
+two moves ago", `master@{one.week.ago}` means "where master used to
+point to one week ago", and so on. See linkgit:gitrevisions[7] for
+more details.
 
-The subcommand "expire" is used to prune older reflog entries.
-Entries older than `expire` time, or entries older than
-`expire-unreachable` time and not reachable from the current
-tip, are removed from the reflog.  This is typically not used
-directly by the end users -- instead, see linkgit:git-gc[1].
+This command manages the information recorded in the reflog.
 
 The subcommand "show" (which is also the default, in the absence of any
 subcommands) will take all the normal log options, and show the log of
@@ -40,11 +40,11 @@ The reflog will cover all recent actions (HEAD reflog records branch switching
 as well).  It is an alias for `git log -g --abbrev-commit --pretty=oneline`;
 see linkgit:git-log[1].
 
-The reflog is useful in various Git commands, to specify the old value
-of a reference. For example, `HEAD@{2}` means "where HEAD used to be
-two moves ago", `master@{one.week.ago}` means "where master used to
-point to one week ago", and so on. See linkgit:gitrevisions[7] for
-more details.
+The subcommand "expire" is used to prune older reflog entries.
+Entries older than `expire` time, or entries older than
+`expire-unreachable` time and not reachable from the current
+tip, are removed from the reflog.  This is typically not used
+directly by the end users -- instead, see linkgit:git-gc[1].
 
 To delete single entries from the reflog, use the subcommand "delete"
 and specify the _exact_ entry (e.g. "`git reflog delete master@{2}`").
@@ -53,19 +53,6 @@ and specify the _exact_ entry (e.g. "`git reflog delete master@{2}`").
 OPTIONS
 -------
 
---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.
-+
-This computation involves traversing all the reachable objects, i.e. it
-has the same cost as 'git prune'.  Fortunately, once this is run, we
-should not have to ever worry about missing objects, because the current
-prune and pack-objects know about reflogs and protect objects referred by
-them.
-
 --expire=<time>::
 	Entries older than this time are pruned.  Without the
 	option it is taken from configuration `gc.reflogExpire`,
@@ -83,8 +70,18 @@ them.
 	turns off early pruning of unreachable entries (but see
 	--expire).
 
---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.
++
+This computation involves traversing all the reachable objects, i.e. it
+has the same cost as 'git prune'.  Fortunately, once this is run, we
+should not have to ever worry about missing objects, because the current
+prune and pack-objects know about reflogs and protect objects referred by
+them.
 
 --updateref::
 	Update the ref with the sha1 of the top reflog entry (i.e.
@@ -98,6 +95,9 @@ them.
 --verbose::
 	Print extra information on screen.
 
+--all::
+	Instead of listing <refs> explicitly, prune all refs.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
-- 
2.1.4

Re: [PATCH 5/8] reflog: rearrange the manpage

[Stefan Beller]

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= ?

While talking about this man page, we should also add --dry-run?

Re: [PATCH 5/8] reflog: rearrange the manpage

[Michael Haggerty]

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.

> While talking about this man page, we should also add --dry-run?

Good point. I'll fix that, too.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 5/8] reflog: rearrange the manpage

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

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

The subcommands of "git reflog" live in their own world.  The
options that are applicable to "git reflog expire" do not have
much to do with what you can give to "git reflog show".

So perhaps a good first step would be to split OPTIONS section
into three (or four---if we have options common to all of them),
and list them under separate headings (i.e. "options applicable to
all subcommands" (optional), "options for 'expire'", etc.)

Thanks.

[PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references

[Michael Haggerty]

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.

We choose to implement option 4.

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

Re: [PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references

[Stefan Beller]

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)

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

Re: [PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references

[Michael Haggerty]

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

Re: [PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references

[Stefan Beller]

On Thu, Feb 12, 2015 at 8:08 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> 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.

Yes I can understand that, maybe I was thinking one step
further in the wrong direction. The question was rather:
Do we have symbolic reflogs, i.e.
$ cat .git/logs/<some_ref>:
symbolic log: find log in other reflog

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

Yes because we cannot do inverse symref resolution, we have this kludge
(with the long comment, "This should do 99% of the time in theory
and 100% in practise") of checking if it is also HEAD whenever we
touch another ref.

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

I am not going to bring a patch for option 3. I just learned to be extra
suspicious if we want to drop something silently, so I wanted to
understand in what circumstances we'd run into trouble with that.
That said:

Reviewed-by: Stefan Beller <sbeller@google.com>

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

Re: [PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

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

I have been wondering what the valid situation where --updateref is
a right thing to do on _any_ ref, be it a real one or a symbolic,
unless you are trying to manually fix a corrupted repository that
lack random objects left and right.  We may expire old reflog
entries and make stale objects disappear, but even when we purge all
reflog entries by expiring anything that is older than a nanosecond
ago, I do not think of a situation where we see some reflog entry
surface as the "latest" entry that points at an object that is _not_
at the tip of the actual ref.

Except for the stash, of course, as you pointed out.  We could drop
the tip (i.e. the latest) while keeping the other ones.

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

I agree with that assessment.  We might even want to start thinking
about a good strategy to remove the --updateref option command,
which in turn would need to restructure how a stash is represented.

Re: [PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references

[Jeff King]

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)

?

-Peff

Re: [PATCH 6/8] reflog_expire(): ignore --updateref for symbolic references

[Michael Haggerty]

On 02/12/2015 10:54 PM, Jeff King wrote:
> 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)
> 
> ?

You're right, of course. Thanks.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH 7/8] reflog_expire(): never update a reference to null_sha1

[Michael Haggerty]

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>
---
Another alternative would be to delete the reference in this
situation. This behavior would be kindof logically consistent and
happens to be just the thing that "git stash drop" wants. It wouldn't
even really be backwards-incompatible, given that the current code
leaves a broken repository in this situation. However, this change
would require some special casing if the reference whose reflog is
being expired is the current reference, and it somehow seems too
"lossy" to me for some reason. I'm open to feedback on this point.

 refs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index c0001da..1b2a497 100644
--- a/refs.c
+++ b/refs.c
@@ -4077,10 +4077,13 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
 		/*
 		 * 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.
+		 * the symbolic reference's reflog. Nor can we update
+		 * a reference if there are no remaining reflog
+		 * entries.
 		 */
 		int update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&
-			~(type & REF_ISSYMREF);
+			~(type & REF_ISSYMREF) &&
+			!is_null_sha1(cb.last_kept_sha1);
 
 		if (close_lock_file(&reflog_lock)) {
 			status |= error("couldn't write %s: %s", log_file,
-- 
2.1.4

Re: [PATCH 7/8] reflog_expire(): never update a reference to null_sha1

[Eric Sunshine]

On Mon, Feb 9, 2015 at 4:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> 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

s/want/want to/

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

Re: [PATCH 7/8] reflog_expire(): never update a reference to null_sha1

[Michael Haggerty]

On 02/09/2015 09:55 PM, Eric Sunshine wrote:
> 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/
> [...]

Thanks; will fix.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

[Michael Haggerty]

When processing the reflog of a symbolic ref, hold the lock on the
symbolic reference itself, not on the reference that it points 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

Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

[Stefan Beller]

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?

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

Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

[Junio C Hamano]

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.

Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

[Stefan Beller]

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?

Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

[Michael Haggerty]

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

Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

[Stefan Beller]

On Thu, Feb 12, 2015 at 8:52 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> 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?

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.

Reviewed-by: Stefan Beller <sbeller@google.com>

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

Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

[Michael Haggerty]

On 02/12/2015 07:04 PM, Stefan Beller wrote:
> 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.

Stefan: This locking policy doesn't only affect "reflog expire"; we also
need to lock the reflog when we add a new entry to it, for example when
updating refs/heads/master through HEAD. Do you know what JGit locks in
that scenario?


I had a discussion in IRC [1] with Carlos Martín Nieto about how libgit2
handles this:

* When libgit2 updates a reference through a symref, it locks the
pointed-to reference while updating the reflog for the symbolic ref. So,
for example, "git commit" would hold refs/heads/master.lock while
updating both logs/refs/heads/master and logs/HEAD. (This matches the
current Git behavior.)
* libgit2 doesn't support "reflog expire", though it does have an API
that allows users to overwrite the reflog with a specified list of
entries. That API locks the reference itself; i.e., HEAD.lock. This
disagrees with Git's current behavior.

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.

In fact, it's even worse. "git checkout foo" and "git symbolic-ref HEAD
refs/heads/foo" release "HEAD.lock" *before* updating logs/HEAD--in
other words, they append to logs/HEAD without holding *any* lock.

What is the best way forward?

I think the current locking policy is untenable, even aside from the bug
in "symbolic-ref".

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

Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

[Stefan Beller]

On Fri, Feb 13, 2015 at 8:26 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>
> 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.

Yeah, I agree on that. It's ok to lock both for now (it doesn't
really harm the user). But we really need to document it in the
source code why we lock both and that the 2nd lock can be removed
after time has passed, something like:

/*
 * We need to lock both the symbolic ref as well as
 * the dereferenced ref for now because of inconsistencies with
 * older and other implementations of git. Originally only the
 * dereferenced ref lock was held, but discussion($gmane/263555)
 * turned out, we actually want to hold the lock on the symbolic ref.
 * For now hold both locks until all implementation hold the lock on
 * the symbolic ref. (Feb/2015)
 */

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

Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

[Junio C Hamano]

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

Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

[Stefan Beller]

On Fri, Feb 13, 2015 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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 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?


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

That would be the safest option indeed.

Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

[Junio C Hamano]

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?

Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

[Stefan Beller]

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.

So the other party updating the underlying ref would also need to lock
HEAD then,
which ... they don't. But they try writing to it nevertheless, in
refs.c line 3121-3150
see the /* special hack */ comment.

Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

HEAD (or refs/remotes/origin/HEAD) reflog and reflog for
refs/heads/master (or refs/remotes/origin/next) would have to be
different as long as we allow symbolic refs to be repointed to
different refs.  If HEAD refers to 'next' today, and at the tip of
next sits commit X, the reflog for both of them would record the
fact that they were pointing at X.  If you repoint HEAD to point at
'master' (e.g. "git checkout master") whose tip is at commit Y, then
reflog for HEAD would record the fact that now HEAD points at Y, and
reflogs for 'master' or 'next' would not change merely because you
switched where HEAD points at.

And there is anything to like or not to like about 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.

Replace S and R with either (HEAD, refs/heads/master) or
(refs/remotes/origin/HEAD, refs/remotes/origin/next) in the above
and we want both to be true.

How best to achieve that, and what is the set of right ref(s) to
take lock(s) on?  I am not very much interested in how incorrect
today's code might behave.  That is not the central point when
discussing what is the best way forward.

> So the other party updating the underlying ref would also need to lock
> HEAD then,

Yes, that is what I meant.  Your "also" can be read in two different
ways ("other party, too" or "HEAD, too"), though, and I think we
want both ;-).  That is why I hinted that it was iffy to state that
we only have to take the lock only on S and not on R, but only as a
workaround to keep older implementation out we take both---once they
get extinct we can get away with by taking a lock only on S.

When pushing to update 'master' and 'next' into a repository whose
'HEAD' points at 'master', we would want to take locks on 'next' (no
question), but is it sensible to take the lock on 'HEAD' and
deliberately leave 'master' unlocked?  Or is it more sensible to
take all locks on the underlying refs involved (i.e. 'next' and
'master') and in addition any symbolic refs that are pointing at
these refs involved (i.e. 'HEAD')?

Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

[Michael Haggerty]

On 02/13/2015 08:12 PM, Junio C Hamano wrote:
> [...]
> 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.

Let's first talk about an ideal world if we had complete support for
symbolic references.

Yes, I agree with your principle. Moreover, suppose that S and S2 *both*
point at R, and there is a third symref S3 that points at symref S
(symbolic refs can point at other symbolic refs!):

X <- R <- S <- S3
        \
         S2

Now, if R updated from X to Y (regardless of whether the update is via R
directly or via one of the symrefs), then each of the four reflogs (R,
S, S2, and S3) should gain a new entry reflecting the update.

If S is reseated to point at R2 instead of R, then the reflogs for S and
for S3 should each gain new entries

What locks should we hold? In my opinion, we should hold the locks on
exactly those references (symbolic or regular) whose reflogs we want to
change. So in the first example, we should hold

    $GIT_DIR/$R.lock
    $GIT_DIR/$S.lock
    $GIT_DIR/$S2.lock, and
    $GIT_DIR/$S3.lock

Ideally, we should acquire all of the locks before making any modifications.


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.

If a symref is reseated, then its reflog is changed but not that of any
symrefs that might be pointed at it.

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

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

[Junio C Hamano]

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.

Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

[Michael Haggerty]

On 02/13/2015 10:53 PM, Junio C Hamano wrote:
> 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.

Yes. The first is approximately the status quo, except that you would
like explicitly to *suppress* creating reflogs for symbolic refs other
than HEAD even if a reference is altered via the symbolic ref.

The second makes sense, though I think reflogs for remote HEADs are far
less useful than those for HEAD. So I think this is a low-priority project.

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

I forgot to mention that if we want to retain lock-compatibility with
older clients, then we *also* need to lock the reference pointed at by a
symbolic ref when modifying the symbolic ref's reflog. This is often
implied by the previous rule, but not when we reseat a symbolic
reference or when we expire a symbolic reference's reflog.

I will look at how hard this is to implement. If it is at all involved,
then I might drop this patch from the current patch series and defer it
to another one.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 0/8] Fix some problems with reflog expiration

[Stefan Beller]

On Mon, Feb 9, 2015 at 1:12 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

> By the way, both of these patch series conflict with
> sb/atomic-push-fix, which is in pu. My understanding is that Stefan
> wants to rework that patch series anyway, but if not I would be happy
> to show how to resolve the conflicts.

Heh, I had it already reworked on Friday evening, but forgot to send it out
for review. As mentioned before, sb/atomic-push-fix is a misleading branch name
as it actually enables large transactions [ "large" means > $(ulimit -n) ].

I don't mind reworking that series again on top of either this or the other
patch series you sent out, as I wasn't entirely happy with it anyway.
(Naming is hard)

[PATCH] refs.c: get rid of force_write flag

[Stefan Beller]

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?
    
    Thanks,
    Stefan

 refs.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index 3fcf342..ae24502 100644
--- a/refs.c
+++ b/refs.c
@@ -12,7 +12,6 @@ struct ref_lock {
 	struct lock_file *lk;
 	unsigned char old_sha1[20];
 	int lock_fd;
-	int force_write;
 };
 
 /*
@@ -2319,8 +2318,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 ((flags & REF_NODEREF) && (type & REF_ISSYMREF))
-		lock->force_write = 1;
 
  retry:
 	switch (safe_create_leading_directories(ref_file)) {
@@ -3788,8 +3785,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		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)) {
+			/* Ignore symbolic links when told not to dereference */
+			if (!((update->type & REF_ISSYMREF)
+			      && (update->flags & REF_NODEREF))
+			    && !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,
-- 
2.2.1.62.g3f15098

Re: [PATCH] refs.c: get rid of force_write flag

[Michael Haggerty]

On 02/11/2015 12:12 AM, Stefan Beller wrote:
> 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?
> [...]

This is a good idea; I'll add it in the re-roll.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu