threads / patch / 37143

patch, 12 partsrefs.c: add an err argument to delete_ref_loose

Subject: [PATCH 03/12] refs.c: add an err argument to delete_ref_loose

## tl;dr

26 messages between Jul 16, 2014 and Jul 22, 2014. Diffs are folded; open one to read it.

replies: 25people: 2as markdown or json

Ronnie Sahlberg· Jul 16, 2014, 22:23 UTC · lore

[PATCH 00/12] Use ref transactions part 3

This is the third and final part of the original 48 patch series for basic transaction support.

It is used ontop of the previous two series :
* rs/ref-transaction-0 (2014-07-14) 19 commits
* rs/ref-transaction-1 (2014-07-16) 20 commits

This version implements some changes suggested by mhagger for the warn_if_removable changes. It also adds a new patch "fix handling of badly named refs" that repairs the handling of badly named refs.

Ronnie Sahlberg (12):
  wrapper.c: simplify warn_if_unremovable
  wrapper.c: add a new function unlink_or_msg
  refs.c: add an err argument to delete_ref_loose
  refs.c: pass the ref log message to _create/delete/update instead of
    _commit
  refs.c: pass NULL as *flags to read_ref_full
  refs.c: move the check for valid refname to lock_ref_sha1_basic
  refs.c: call lock_ref_sha1_basic directly from commit
  refs.c: pass a skip list to name_conflict_fn
  refs.c: propagate any errno==ENOTDIR from _commit back to the callers
  fetch.c: change s_update_ref to use a ref transaction
  refs.c: make write_ref_sha1 static
  refs.c: fix handling of badly named refs
 branch.c                |   4 +-
 builtin/blame.c         |   2 +-
 builtin/branch.c        |   6 +-
 builtin/clone.c         |   2 +-
 builtin/commit.c        |   4 +-
 builtin/fetch.c         |  36 ++++---
 builtin/fmt-merge-msg.c |   2 +-
 builtin/for-each-ref.c  |   6 +-
 builtin/log.c           |   3 +-
 builtin/receive-pack.c  |   5 +-
 builtin/remote.c        |   5 +-
 builtin/replace.c       |   4 +-
 builtin/show-branch.c   |   6 +-
 builtin/tag.c           |   4 +-
 builtin/update-ref.c    |  13 +--
 bundle.c                |   2 +-
 cache.h                 |  18 ++--
 fast-import.c           |   8 +-
 git-compat-util.h       |   6 ++
 http-backend.c          |   3 +-
 reflog-walk.c           |   3 +-
 refs.c                  | 247 +++++++++++++++++++++++++++++++-----------------
 refs.h                  |  17 ++--
 remote.c                |   6 +-
 sequencer.c             |   6 +-
 transport-helper.c      |   2 +-
 transport.c             |   5 +-
 walker.c                |   5 +-
 wrapper.c               |  30 ++++--
 29 files changed, 291 insertions(+), 169 deletions(-)
-- 
2.0.1.527.gc6b782e
Ronnie Sahlberg· Jul 16, 2014, 22:23 UTC · re: Ronnie Sahlberg · lore

[PATCH 01/12] wrapper.c: simplify warn_if_unremovable

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 wrapper.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)
Show changes to wrapper.c +6 −8
diff --git a/wrapper.c b/wrapper.c
index bc1bfb8..740e193 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -429,14 +429,12 @@ int xmkstemp_mode(char *template, int mode)
 
 static int warn_if_unremovable(const char *op, const char *file, int rc)
 {
-	if (rc < 0) {
-		int err = errno;
-		if (ENOENT != err) {
-			warning("unable to %s %s: %s",
-				op, file, strerror(errno));
-			errno = err;
-		}
-	}
+	int err;
+	if (rc >= 0 || errno == ENOENT)
+		return rc;
+	err = errno;
+	warning("unable to %s %s: %s", op, file, strerror(errno));
+	errno = err;
 	return rc;
 }
 

-- 
2.0.1.527.gc6b782e
Junio C Hamano· Jul 18, 2014, 22:21 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH 01/12] wrapper.c: simplify warn_if_unremovable

Ronnie Sahlberg <sahlberg@google.com> writes:
Show 29 quoted lines
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  wrapper.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/wrapper.c b/wrapper.c
> index bc1bfb8..740e193 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -429,14 +429,12 @@ int xmkstemp_mode(char *template, int mode)
>  
>  static int warn_if_unremovable(const char *op, const char *file, int rc)
>  {
> -	if (rc < 0) {
> -		int err = errno;
> -		if (ENOENT != err) {
> -			warning("unable to %s %s: %s",
> -				op, file, strerror(errno));
> -			errno = err;
> -		}
> -	}
> +	int err;
> +	if (rc >= 0 || errno == ENOENT)
> +		return rc;
> +	err = errno;
> +	warning("unable to %s %s: %s", op, file, strerror(errno));
> +	errno = err;
>  	return rc;
>  }
Looks sensible.
Ronnie Sahlberg· Jul 16, 2014, 22:23 UTC · re: Ronnie Sahlberg · lore

[PATCH 02/12] wrapper.c: add a new function unlink_or_msg

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 git-compat-util.h |  6 ++++++
 wrapper.c         | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+)
Show changes to 2 files +24 −0

git-compat-util.h, wrapper.c

diff --git a/git-compat-util.h b/git-compat-util.h
index b6f03b3..426bc98 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -704,12 +704,18 @@ void git_qsort(void *base, size_t nmemb, size_t size,
 #endif
 #endif
 
+#include "strbuf.h"
+
 /*
  * Preserves errno, prints a message, but gives no warning for ENOENT.
  * Always returns the return value of unlink(2).
  */
 int unlink_or_warn(const char *path);
 /*
+ * Like unlink_or_warn but populates a strbuf
+ */
+int unlink_or_msg(const char *file, struct strbuf *err);
+/*
  * Likewise for rmdir(2).
  */
 int rmdir_or_warn(const char *path);
diff --git a/wrapper.c b/wrapper.c
index 740e193..74a0cc0 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -438,6 +438,24 @@ static int warn_if_unremovable(const char *op, const char *file, int rc)
 	return rc;
 }
 
+int unlink_or_msg(const char *file, struct strbuf *err)
+{
+	if (err) {
+		int rc = unlink(file);
+		int save_errno = errno;
+
+		if (rc < 0 && errno != ENOENT) {
+			strbuf_addf(err, "unable to unlink %s: %s",
+				    file, strerror(errno));
+			errno = save_errno;
+			return -1;
+		}
+		return 0;
+	}
+
+	return unlink_or_warn(file);
+}
+
 int unlink_or_warn(const char *file)
 {
 	return warn_if_unremovable("unlink", file, unlink(file));

-- 
2.0.1.527.gc6b782e
Junio C Hamano· Jul 18, 2014, 22:25 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH 02/12] wrapper.c: add a new function unlink_or_msg

Ronnie Sahlberg <sahlberg@google.com> writes:
Show 54 quoted lines
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  git-compat-util.h |  6 ++++++
>  wrapper.c         | 18 ++++++++++++++++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index b6f03b3..426bc98 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -704,12 +704,18 @@ void git_qsort(void *base, size_t nmemb, size_t size,
>  #endif
>  #endif
>  
> +#include "strbuf.h"
> +
>  /*
>   * Preserves errno, prints a message, but gives no warning for ENOENT.
>   * Always returns the return value of unlink(2).
>   */
>  int unlink_or_warn(const char *path);
>  /*
> + * Like unlink_or_warn but populates a strbuf
> + */
> +int unlink_or_msg(const char *file, struct strbuf *err);
> +/*
>   * Likewise for rmdir(2).
>   */
>  int rmdir_or_warn(const char *path);
> diff --git a/wrapper.c b/wrapper.c
> index 740e193..74a0cc0 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -438,6 +438,24 @@ static int warn_if_unremovable(const char *op, const char *file, int rc)
>  	return rc;
>  }
>  
> +int unlink_or_msg(const char *file, struct strbuf *err)
> +{
> +	if (err) {
> +		int rc = unlink(file);
> +		int save_errno = errno;
> +
> +		if (rc < 0 && errno != ENOENT) {
> +			strbuf_addf(err, "unable to unlink %s: %s",
> +				    file, strerror(errno));
> +			errno = save_errno;
> +			return -1;
> +		}
> +		return 0;
> +	}
> +
> +	return unlink_or_warn(file);
> +}

In general, I do not generally like to see messages propagated upwards from deeper levels of the callchain to the callers to be used later, primarily because that will easily make it harder to localize the message-lego.

For this partcular one, shouldn't the caller be doing
	if (unlink(file) && errno != ENOENT) {
        	... do its own error message ...
	}
instead of calling any of the unlink_or_whatever() helper?
>  int unlink_or_warn(const char *file)
>  {
>  	return warn_if_unremovable("unlink", file, unlink(file));
Junio C Hamano· Jul 18, 2014, 22:59 UTC · re: Junio C Hamano · lore

Re: [PATCH 02/12] wrapper.c: add a new function unlink_or_msg

Hmm, the primary reason for this seems to be because you are going to handle multiple refs at a time, some of them might fail to lock due to this lowest-level helper to unlink failing, some others may fail to lock due to some other reason, and the user may want to be able to differentiate various modes of failure.

But even if that were the case, would it be necessary to buffer the messages like this and give them all at the end? In the transaction code path, it is likely that you would be aborting the whole thing at the first failure, no?

I dunno...
On Fri, Jul 18, 2014 at 3:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 74 quoted lines
> Ronnie Sahlberg <sahlberg@google.com> writes:
>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  git-compat-util.h |  6 ++++++
>>  wrapper.c         | 18 ++++++++++++++++++
>>  2 files changed, 24 insertions(+)
>>
>> diff --git a/git-compat-util.h b/git-compat-util.h
>> index b6f03b3..426bc98 100644
>> --- a/git-compat-util.h
>> +++ b/git-compat-util.h
>> @@ -704,12 +704,18 @@ void git_qsort(void *base, size_t nmemb, size_t size,
>>  #endif
>>  #endif
>>
>> +#include "strbuf.h"
>> +
>>  /*
>>   * Preserves errno, prints a message, but gives no warning for ENOENT.
>>   * Always returns the return value of unlink(2).
>>   */
>>  int unlink_or_warn(const char *path);
>>  /*
>> + * Like unlink_or_warn but populates a strbuf
>> + */
>> +int unlink_or_msg(const char *file, struct strbuf *err);
>> +/*
>>   * Likewise for rmdir(2).
>>   */
>>  int rmdir_or_warn(const char *path);
>> diff --git a/wrapper.c b/wrapper.c
>> index 740e193..74a0cc0 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -438,6 +438,24 @@ static int warn_if_unremovable(const char *op, const char *file, int rc)
>>       return rc;
>>  }
>>
>> +int unlink_or_msg(const char *file, struct strbuf *err)
>> +{
>> +     if (err) {
>> +             int rc = unlink(file);
>> +             int save_errno = errno;
>> +
>> +             if (rc < 0 && errno != ENOENT) {
>> +                     strbuf_addf(err, "unable to unlink %s: %s",
>> +                                 file, strerror(errno));
>> +                     errno = save_errno;
>> +                     return -1;
>> +             }
>> +             return 0;
>> +     }
>> +
>> +     return unlink_or_warn(file);
>> +}
>
> In general, I do not generally like to see messages propagated
> upwards from deeper levels of the callchain to the callers to be
> used later, primarily because that will easily make it harder to
> localize the message-lego.
>
> For this partcular one, shouldn't the caller be doing
>
>         if (unlink(file) && errno != ENOENT) {
>                 ... do its own error message ...
>         }
>
> instead of calling any of the unlink_or_whatever() helper?
>
>
>>  int unlink_or_warn(const char *file)
>>  {
>>       return warn_if_unremovable("unlink", file, unlink(file));
Ronnie Sahlberg· Jul 22, 2014, 17:42 UTC · re: Junio C Hamano · lore

Re: [PATCH 02/12] wrapper.c: add a new function unlink_or_msg

On Fri, Jul 18, 2014 at 3:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> Hmm, the primary reason for this seems to be because you are going to handle
> multiple refs at a time, some of them might fail to lock due to this
> lowest-level
> helper to unlink failing, some others may fail to lock due to some other reason,
> and the user may want to be able to differentiate various modes of failure.
>
> But even if that were the case, would it be necessary to buffer the messages
> like this and give them all at the end? In the transaction code path,
> it is likely
> that you would be aborting the whole thing at the first failure, no?

Not necessarily. I can think of reasons both for and against "abort on first failure".

One reason for the former could be if there are problems with multiple
refs in a single transaction.
It would be very annoying to have to do
$ git <some command>
   error: ref foo has a problem
$ <run command to fix the problem>
$ git <some sommand>     (try again)
   error: ref bar has a problem
...
And it might be more userfriendly if that instead would be
$ git <some command>
   error: ref foo has a problem
   error: ref bar has a problem
   ...
And get all the failures in one go instead of having to iterate.

The reason for the latter I think is it would be cleaner/simpler/... to just have an "abort on first failure".

On the past discussions on the list I think I have heard voices for both approaches. I don't think we have all that many multiple-refs-in-a-single-transaction yet in what is checked in so far so I think we are practically still "abort on first error".

I personally do not know yet which approach is the best but would like to keep the door open for the "try all and fail at the end". That said, I do not feel all that strongly about this. If you have strong feelings about this I can remove the unlink_or_msg patch and rework the rest of the series to cope with it.

regards ronnie sahlberg

Show 79 quoted lines
>
> I dunno...
>
>
> On Fri, Jul 18, 2014 at 3:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Ronnie Sahlberg <sahlberg@google.com> writes:
>>
>>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>>> ---
>>>  git-compat-util.h |  6 ++++++
>>>  wrapper.c         | 18 ++++++++++++++++++
>>>  2 files changed, 24 insertions(+)
>>>
>>> diff --git a/git-compat-util.h b/git-compat-util.h
>>> index b6f03b3..426bc98 100644
>>> --- a/git-compat-util.h
>>> +++ b/git-compat-util.h
>>> @@ -704,12 +704,18 @@ void git_qsort(void *base, size_t nmemb, size_t size,
>>>  #endif
>>>  #endif
>>>
>>> +#include "strbuf.h"
>>> +
>>>  /*
>>>   * Preserves errno, prints a message, but gives no warning for ENOENT.
>>>   * Always returns the return value of unlink(2).
>>>   */
>>>  int unlink_or_warn(const char *path);
>>>  /*
>>> + * Like unlink_or_warn but populates a strbuf
>>> + */
>>> +int unlink_or_msg(const char *file, struct strbuf *err);
>>> +/*
>>>   * Likewise for rmdir(2).
>>>   */
>>>  int rmdir_or_warn(const char *path);
>>> diff --git a/wrapper.c b/wrapper.c
>>> index 740e193..74a0cc0 100644
>>> --- a/wrapper.c
>>> +++ b/wrapper.c
>>> @@ -438,6 +438,24 @@ static int warn_if_unremovable(const char *op, const char *file, int rc)
>>>       return rc;
>>>  }
>>>
>>> +int unlink_or_msg(const char *file, struct strbuf *err)
>>> +{
>>> +     if (err) {
>>> +             int rc = unlink(file);
>>> +             int save_errno = errno;
>>> +
>>> +             if (rc < 0 && errno != ENOENT) {
>>> +                     strbuf_addf(err, "unable to unlink %s: %s",
>>> +                                 file, strerror(errno));
>>> +                     errno = save_errno;
>>> +                     return -1;
>>> +             }
>>> +             return 0;
>>> +     }
>>> +
>>> +     return unlink_or_warn(file);
>>> +}
>>
>> In general, I do not generally like to see messages propagated
>> upwards from deeper levels of the callchain to the callers to be
>> used later, primarily because that will easily make it harder to
>> localize the message-lego.
>>
>> For this partcular one, shouldn't the caller be doing
>>
>>         if (unlink(file) && errno != ENOENT) {
>>                 ... do its own error message ...
>>         }
>>
>> instead of calling any of the unlink_or_whatever() helper?
>>
>>
>>>  int unlink_or_warn(const char *file)
>>>  {
>>>       return warn_if_unremovable("unlink", file, unlink(file));
Junio C Hamano· Jul 22, 2014, 17:56 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH 02/12] wrapper.c: add a new function unlink_or_msg

Ronnie Sahlberg <sahlberg@google.com> writes:
Show 21 quoted lines
> One reason for the former could be if there are problems with multiple
> refs in a single transaction.
> It would be very annoying to have to do
> $ git <some command>
>    error: ref foo has a problem
>
> $ <run command to fix the problem>
> $ git <some sommand>     (try again)
>    error: ref bar has a problem
> ...
>
> And it might be more userfriendly if that instead would be
> $ git <some command>
>    error: ref foo has a problem
>    error: ref bar has a problem
>    ...
>
> And get all the failures in one go instead of having to iterate.
> ...
> I personally do not know yet which approach is the best but would like
> to keep the door open for the "try all and fail at the end".

Yes, and often it is useful (e.g. we allow to push multiple and then show the result for individual refs). But that still does not show a need for accumulating the error messages to strbuf, does it?

Ronnie Sahlberg· Jul 16, 2014, 22:23 UTC · re: Ronnie Sahlberg · lore

Add an err argument to delete_loose_ref so that we can pass a descriptive error string back to the caller. Pass the err argument from transaction commit to this function so that transaction users will have a nice error string if the transaction failed due to delete_loose_ref.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
Show changes to refs.c +6 −5
diff --git a/refs.c b/refs.c
index 0017d9c..24f9546 100644
--- a/refs.c
+++ b/refs.c
@@ -2544,16 +2544,16 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 	return ret;
 }
 
-static int delete_ref_loose(struct ref_lock *lock, int flag)
+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
 {
 	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
 		/* loose */
-		int err, i = strlen(lock->lk->filename) - 5; /* .lock */
+		int res, i = strlen(lock->lk->filename) - 5; /* .lock */
 
 		lock->lk->filename[i] = 0;
-		err = unlink_or_warn(lock->lk->filename);
+		res = unlink_or_msg(lock->lk->filename, err);
 		lock->lk->filename[i] = '.';
-		if (err && errno != ENOENT)
+		if (res)
 			return 1;
 	}
 	return 0;
@@ -3602,7 +3602,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		struct ref_update *update = updates[i];
 
 		if (update->lock) {
-			ret |= delete_ref_loose(update->lock, update->type);
+			ret |= delete_ref_loose(update->lock, update->type,
+						err);
 			if (!(update->flags & REF_ISPRUNING))
 				delnames[delnum++] = update->lock->ref_name;
 		}

-- 
2.0.1.527.gc6b782e
Ronnie Sahlberg· Jul 16, 2014, 22:23 UTC · re: Ronnie Sahlberg · lore

[PATCH 04/12] refs.c: pass the ref log message to _create/delete/update instead of _commit

Change the reference transactions so that we pass the reflog message through to the create/delete/update function instead of the commit message. This allows for individual messages for each change in a multi ref transaction.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 branch.c               |  4 ++--
 builtin/commit.c       |  4 ++--
 builtin/fetch.c        |  3 +--
 builtin/receive-pack.c |  5 +++--
 builtin/replace.c      |  4 ++--
 builtin/tag.c          |  4 ++--
 builtin/update-ref.c   | 13 +++++++------
 fast-import.c          |  8 ++++----
 refs.c                 | 34 +++++++++++++++++++++-------------
 refs.h                 |  8 ++++----
 sequencer.c            |  4 ++--
 walker.c               |  5 ++---
 12 files changed, 52 insertions(+), 44 deletions(-)
Show changes to 12 files +52 −44

branch.c, builtin/commit.c, builtin/fetch.c, builtin/receive-pack.c, builtin/replace.c, builtin/tag.c, builtin/update-ref.c, fast-import.c, refs.c, refs.h, sequencer.c, walker.c

diff --git a/branch.c b/branch.c
index c1eae00..e0439af 100644
--- a/branch.c
+++ b/branch.c
@@ -301,8 +301,8 @@ void create_branch(const char *head,
 		transaction = ref_transaction_begin(&err);
 		if (!transaction ||
 		    ref_transaction_update(transaction, ref.buf, sha1,
-					   null_sha1, 0, !forcing, &err) ||
-		    ref_transaction_commit(transaction, msg, &err))
+					   null_sha1, 0, !forcing, msg, &err) ||
+		    ref_transaction_commit(transaction, &err))
 			die("%s", err.buf);
 		ref_transaction_free(transaction);
 	}
diff --git a/builtin/commit.c b/builtin/commit.c
index 668fa6a..c499826 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1767,8 +1767,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	    ref_transaction_update(transaction, "HEAD", sha1,
 				   current_head ?
 				   current_head->object.sha1 : NULL,
-				   0, !!current_head, &err) ||
-	    ref_transaction_commit(transaction, sb.buf, &err)) {
+				   0, !!current_head, sb.buf, &err) ||
+	    ref_transaction_commit(transaction, &err)) {
 		rollback_index_files();
 		die("%s", err.buf);
 	}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index dd46b61..92fad2d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -702,10 +702,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 			}
 		}
 	}
-
 	if (rc & STORE_REF_ERROR_DF_CONFLICT)
 		error(_("some local refs could not be updated; try running\n"
-		      " 'git remote prune %s' to remove any old, conflicting "
+		      "'git remote prune %s' to remove any old, conflicting "
 		      "branches"), remote_name);
 
  abort:
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 91099ad..4752225 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -585,8 +585,9 @@ static char *update(struct command *cmd, struct shallow_info *si)
 		transaction = ref_transaction_begin(&err);
 		if (!transaction ||
 		    ref_transaction_update(transaction, namespaced_name,
-					   new_sha1, old_sha1, 0, 1, &err) ||
-		    ref_transaction_commit(transaction, "push", &err)) {
+					   new_sha1, old_sha1, 0, 1, "push",
+					   &err) ||
+		    ref_transaction_commit(transaction, &err)) {
 			char *str = strbuf_detach(&err, NULL);
 			ref_transaction_free(transaction);
 
diff --git a/builtin/replace.c b/builtin/replace.c
index 7528f3d..df060f8 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -170,8 +170,8 @@ static int replace_object_sha1(const char *object_ref,
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref, repl, prev,
-				   0, !is_null_sha1(prev), &err) ||
-	    ref_transaction_commit(transaction, NULL, &err))
+				   0, !is_null_sha1(prev), NULL, &err) ||
+	    ref_transaction_commit(transaction, &err))
 		die("%s", err.buf);
 
 	ref_transaction_free(transaction);
diff --git a/builtin/tag.c b/builtin/tag.c
index 1aa88a2..3834b06 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -705,8 +705,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, object, prev,
-				   0, 1, &err) ||
-	    ref_transaction_commit(transaction, NULL, &err))
+				   0, 1, NULL, &err) ||
+	    ref_transaction_commit(transaction, &err))
 		die("%s", err.buf);
 	ref_transaction_free(transaction);
 	if (force && !is_null_sha1(prev) && hashcmp(prev, object))
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index c6ad0be..28b478a 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -16,6 +16,7 @@ static struct ref_transaction *transaction;
 
 static char line_termination = '\n';
 static int update_flags;
+static const char *msg;
 static struct strbuf err = STRBUF_INIT;
 
 /*
@@ -199,7 +200,7 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)
 		die("update %s: extra input: %s", refname, next);
 
 	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-				   update_flags, have_old, &err))
+				   update_flags, have_old, msg, &err))
 		die("%s", err.buf);
 
 	update_flags = 0;
@@ -227,7 +228,7 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)
 		die("create %s: extra input: %s", refname, next);
 
 	if (ref_transaction_create(transaction, refname, new_sha1,
-				   update_flags, &err))
+				   update_flags, msg, &err))
 		die("%s", err.buf);
 
 	update_flags = 0;
@@ -259,7 +260,7 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 		die("delete %s: extra input: %s", refname, next);
 
 	if (ref_transaction_delete(transaction, refname, old_sha1,
-				   update_flags, have_old, &err))
+				   update_flags, have_old, msg, &err))
 		die("%s", err.buf);
 
 	update_flags = 0;
@@ -292,7 +293,7 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
 		die("verify %s: extra input: %s", refname, next);
 
 	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-				   update_flags, have_old, &err))
+				   update_flags, have_old, msg, &err))
 		die("%s", err.buf);
 
 	update_flags = 0;
@@ -345,7 +346,7 @@ static void update_refs_stdin(void)
 
 int cmd_update_ref(int argc, const char **argv, const char *prefix)
 {
-	const char *refname, *oldval, *msg = NULL;
+	const char *refname, *oldval;
 	unsigned char sha1[20], oldsha1[20];
 	int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
 	struct option options[] = {
@@ -371,7 +372,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		if (end_null)
 			line_termination = '\0';
 		update_refs_stdin();
-		if (ref_transaction_commit(transaction, msg, &err))
+		if (ref_transaction_commit(transaction, &err))
 			die("%s", err.buf);
 		ref_transaction_free(transaction);
 		return 0;
diff --git a/fast-import.c b/fast-import.c
index a95e1be..0876db2 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1708,8 +1708,8 @@ static int update_branch(struct branch *b)
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
-				   0, 1, &err) ||
-	    ref_transaction_commit(transaction, msg, &err)) {
+				   0, 1, msg, &err) ||
+	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_free(transaction);
 		error("%s", err.buf);
 		strbuf_release(&err);
@@ -1748,12 +1748,12 @@ static void dump_tags(void)
 		strbuf_addf(&ref_name, "refs/tags/%s", t->name);
 
 		if (ref_transaction_update(transaction, ref_name.buf, t->sha1,
-					   NULL, 0, 0, &err)) {
+					   NULL, 0, 0, msg, &err)) {
 			failure |= error("%s", err.buf);
 			goto cleanup;
 		}
 	}
-	if (ref_transaction_commit(transaction, msg, &err))
+	if (ref_transaction_commit(transaction, &err))
 		failure |= error("%s", err.buf);
 
  cleanup:
diff --git a/refs.c b/refs.c
index 24f9546..7d65253 100644
--- a/refs.c
+++ b/refs.c
@@ -2393,8 +2393,8 @@ static void prune_ref(struct ref_to_prune *r)
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_delete(transaction, r->name, r->sha1,
-				   REF_ISPRUNING, 1, &err) ||
-	    ref_transaction_commit(transaction, NULL, &err)) {
+				   REF_ISPRUNING, 1, NULL, &err) ||
+	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_free(transaction);
 		error("%s", err.buf);
 		strbuf_release(&err);
@@ -2567,8 +2567,8 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_delete(transaction, refname, sha1, delopt,
-				   sha1 && !is_null_sha1(sha1), &err) ||
-	    ref_transaction_commit(transaction, NULL, &err)) {
+				   sha1 && !is_null_sha1(sha1), NULL, &err) ||
+	    ref_transaction_commit(transaction, &err)) {
 		error("%s", err.buf);
 		ref_transaction_free(transaction);
 		strbuf_release(&err);
@@ -3345,6 +3345,7 @@ struct ref_update {
 	int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
 	struct ref_lock *lock;
 	int type;
+	char *msg;
 	const char refname[FLEX_ARRAY];
 };
 
@@ -3391,9 +3392,10 @@ void ref_transaction_free(struct ref_transaction *transaction)
 	if (!transaction)
 		return;
 
-	for (i = 0; i < transaction->nr; i++)
+	for (i = 0; i < transaction->nr; i++) {
+		free(transaction->updates[i]->msg);
 		free(transaction->updates[i]);
-
+	}
 	free(transaction->updates);
 	free(transaction);
 }
@@ -3414,7 +3416,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
 			   const char *refname,
 			   const unsigned char *new_sha1,
 			   const unsigned char *old_sha1,
-			   int flags, int have_old,
+			   int flags, int have_old, const char *msg,
 			   struct strbuf *err)
 {
 	struct ref_update *update;
@@ -3431,13 +3433,15 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	update->have_old = have_old;
 	if (have_old)
 		hashcpy(update->old_sha1, old_sha1);
+	if (msg)
+		update->msg = xstrdup(msg);
 	return 0;
 }
 
 int ref_transaction_create(struct ref_transaction *transaction,
 			   const char *refname,
 			   const unsigned char *new_sha1,
-			   int flags,
+			   int flags, const char *msg,
 			   struct strbuf *err)
 {
 	struct ref_update *update;
@@ -3454,13 +3458,15 @@ int ref_transaction_create(struct ref_transaction *transaction,
 	hashclr(update->old_sha1);
 	update->flags = flags;
 	update->have_old = 1;
+	if (msg)
+		update->msg = xstrdup(msg);
 	return 0;
 }
 
 int ref_transaction_delete(struct ref_transaction *transaction,
 			   const char *refname,
 			   const unsigned char *old_sha1,
-			   int flags, int have_old,
+			   int flags, int have_old, const char *msg,
 			   struct strbuf *err)
 {
 	struct ref_update *update;
@@ -3478,6 +3484,8 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 		assert(!is_null_sha1(old_sha1));
 		hashcpy(update->old_sha1, old_sha1);
 	}
+	if (msg)
+		update->msg = xstrdup(msg);
 	return 0;
 }
 
@@ -3491,8 +3499,8 @@ int update_ref(const char *action, const char *refname,
 	t = ref_transaction_begin(&err);
 	if (!t ||
 	    ref_transaction_update(t, refname, sha1, oldval, flags,
-				   !!oldval, &err) ||
-	    ref_transaction_commit(t, action, &err)) {
+				   !!oldval, action, &err) ||
+	    ref_transaction_commit(t, &err)) {
 		const char *str = "update_ref failed for ref '%s': %s";
 
 		ref_transaction_free(t);
@@ -3536,7 +3544,7 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 }
 
 int ref_transaction_commit(struct ref_transaction *transaction,
-			   const char *msg, struct strbuf *err)
+			   struct strbuf *err)
 {
 	int ret = 0, delnum = 0, i;
 	const char **delnames;
@@ -3585,7 +3593,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 		if (!is_null_sha1(update->new_sha1)) {
 			ret = write_ref_sha1(update->lock, update->new_sha1,
-					     msg);
+					     update->msg);
 			update->lock = NULL; /* freed by write_ref_sha1 */
 			if (ret) {
 				const char *str = "Cannot update the ref '%s'.";
diff --git a/refs.h b/refs.h
index aad846c..6a4d1a7 100644
--- a/refs.h
+++ b/refs.h
@@ -290,7 +290,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
 			   const char *refname,
 			   const unsigned char *new_sha1,
 			   const unsigned char *old_sha1,
-			   int flags, int have_old,
+			   int flags, int have_old, const char *msg,
 			   struct strbuf *err);
 
 /*
@@ -305,7 +305,7 @@ int ref_transaction_update(struct ref_transaction *transaction,
 int ref_transaction_create(struct ref_transaction *transaction,
 			   const char *refname,
 			   const unsigned char *new_sha1,
-			   int flags,
+			   int flags, const char *msg,
 			   struct strbuf *err);
 
 /*
@@ -319,7 +319,7 @@ int ref_transaction_create(struct ref_transaction *transaction,
 int ref_transaction_delete(struct ref_transaction *transaction,
 			   const char *refname,
 			   const unsigned char *old_sha1,
-			   int flags, int have_old,
+			   int flags, int have_old, const char *msg,
 			   struct strbuf *err);
 
 /*
@@ -328,7 +328,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,
  * problem.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
-			   const char *msg, struct strbuf *err);
+			   struct strbuf *err);
 
 /*
  * Free an existing transaction and all associated data.
diff --git a/sequencer.c b/sequencer.c
index cf17c69..284059b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -286,8 +286,8 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 	if (!transaction ||
 	    ref_transaction_update(transaction, "HEAD",
 				   to, unborn ? null_sha1 : from,
-				   0, 1, &err) ||
-	    ref_transaction_commit(transaction, sb.buf, &err)) {
+				   0, 1, sb.buf, &err) ||
+	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_free(transaction);
 		error("%s", err.buf);
 		strbuf_release(&sb);
diff --git a/walker.c b/walker.c
index 60d9f9e..fd9ef87 100644
--- a/walker.c
+++ b/walker.c
@@ -295,15 +295,14 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 		strbuf_addf(&ref_name, "refs/%s", write_ref[i]);
 		if (ref_transaction_update(transaction, ref_name.buf,
 					   &sha1[20 * i], NULL, 0, 0,
+					   msg ? msg : "fetch (unknown)",
 					   &err)) {
 			error("%s", err.buf);
 			goto rollback_and_fail;
 		}
 	}
 	if (write_ref) {
-		if (ref_transaction_commit(transaction,
-					   msg ? msg : "fetch (unknown)",
-					   &err)) {
+		if (ref_transaction_commit(transaction, &err)) {
 			error("%s", err.buf);
 			goto rollback_and_fail;
 		}

-- 
2.0.1.527.gc6b782e
Ronnie Sahlberg· Jul 16, 2014, 22:23 UTC · re: Ronnie Sahlberg · lore

[PATCH 05/12] refs.c: pass NULL as *flags to read_ref_full

We call read_ref_full with a pointer to flags from rename_ref but since we never actually use the returned flags we can just pass NULL here instead.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to refs.c +1 −1
diff --git a/refs.c b/refs.c
index 7d65253..0df6894 100644
--- a/refs.c
+++ b/refs.c
@@ -2666,7 +2666,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 		goto rollback;
 	}
 
-	if (!read_ref_full(newrefname, sha1, 1, &flag) &&
+	if (!read_ref_full(newrefname, sha1, 1, NULL) &&
 	    delete_ref(newrefname, sha1, REF_NODEREF)) {
 		if (errno==EISDIR) {
 			if (remove_empty_directories(git_path("%s", newrefname))) {

-- 
2.0.1.527.gc6b782e
Junio C Hamano· Jul 18, 2014, 22:31 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH 05/12] refs.c: pass NULL as *flags to read_ref_full

Ronnie Sahlberg <sahlberg@google.com> writes:
> We call read_ref_full with a pointer to flags from rename_ref but since
> we never actually use the returned flags we can just pass NULL here instead.

Sensible, at least for the current callers. I had to wonder if rename_ref() would never want to take advantage of the flags return parameter in the future, though. For example, would it want to act differently when the given ref turns out to be a symref? Would it want to report something when the ref to be overwritten was a broken one?

Show 19 quoted lines
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  refs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 7d65253..0df6894 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2666,7 +2666,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>  		goto rollback;
>  	}
>  
> -	if (!read_ref_full(newrefname, sha1, 1, &flag) &&
> +	if (!read_ref_full(newrefname, sha1, 1, NULL) &&
>  	    delete_ref(newrefname, sha1, REF_NODEREF)) {
>  		if (errno==EISDIR) {
>  			if (remove_empty_directories(git_path("%s", newrefname))) {
Ronnie Sahlberg· Jul 22, 2014, 18:19 UTC · re: Junio C Hamano · lore

Re: [PATCH 05/12] refs.c: pass NULL as *flags to read_ref_full

On Fri, Jul 18, 2014 at 3:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> Ronnie Sahlberg <sahlberg@google.com> writes:
>
>> We call read_ref_full with a pointer to flags from rename_ref but since
>> we never actually use the returned flags we can just pass NULL here instead.
>
> Sensible, at least for the current callers.  I had to wonder if
> rename_ref() would never want to take advantage of the flags return
> parameter in the future, though.  For example, would it want to act
> differently when the given ref turns out to be a symref?
I don't know.

We have a check if the old refname was a symref or not since the old version did not have code for how to handle renaming the reflog. (That check is removed in a later series when we have enough transaction code and reflog api changes so that we no longer need to call rename() for the reflog handling.)

I can not think of any reason right now why, but if we need it we can add the argument back when the need arises.

> Would it
> want to report something when the ref to be overwritten was a broken
> one?
Good point.
There are two cases where the new ref could be broken.
1) It could either contain a broken SHA1, like if we do this :
$ echo "Broken ref" > .git/refs/heads/foo-broken-1
2) or it could be broken due to having a bad/invalid name :
$ cp .git.refs.heads.master .git/refs/heads/foo-broken-1-\*...

For 2) I think this should not be allowed so the rename should just fail with something like : $ ./git branch -M foo foo-broken-1-\*... fatal: 'foo-broken-1-*...' is not a valid branch name.

For 1) if the new branch already exists but it has a broken SHA1, for that case I think we should allow rename_ref to overwrite the existing bad SHA1 with the new, good, SHA1 value. Currently this does not work in master : $ echo "Broken ref" > .git/refs/heads/foo-broken-1 $ ./git branch -m foo foo-broken-1 error: unable to resolve reference refs/heads/foo-broken-1: Invalid argument error: unable to lock refs/heads/foo-broken-1 for update fatal: Branch rename failed

And the only way to recover is to first delete the branch as my other patch in this series now allows and then trying the rename again.

For 1), since we are planning to overwrite the current branch with a new SHA1 value, I think that what makes most sense would be to treat the "branch exist but is broken" as if the branch did not exist at all and just allow overwriting it with the new good value.

Currently this does not work in master :

$ echo "Broken ref" > .git/refs/heads/foo-broken-1 $ ./git branch -m foo foo-broken-1 error: unable to resolve reference refs/heads/foo-broken-1: Invalid argument error: unable to lock refs/heads/foo-broken-1 for update fatal: Branch rename failed so since this is not a regression I will not update this particular patch series but instead I can add a new patch to the next patch series to allow this so that we can do : $ echo "Broken ref" > .git/refs/heads/foo-broken-1 $ ./git branch -m foo foo-broken-1 <success>

Comments/opinions?

regards ronnie sahlberg

Show 20 quoted lines
>
>> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  refs.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 7d65253..0df6894 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2666,7 +2666,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>>               goto rollback;
>>       }
>>
>> -     if (!read_ref_full(newrefname, sha1, 1, &flag) &&
>> +     if (!read_ref_full(newrefname, sha1, 1, NULL) &&
>>           delete_ref(newrefname, sha1, REF_NODEREF)) {
>>               if (errno==EISDIR) {
>>                       if (remove_empty_directories(git_path("%s", newrefname))) {
Ronnie Sahlberg· Jul 22, 2014, 21:44 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH 05/12] refs.c: pass NULL as *flags to read_ref_full

On Tue, Jul 22, 2014 at 11:19 AM, Ronnie Sahlberg <sahlberg@google.com> wrote:
Show 74 quoted lines
> On Fri, Jul 18, 2014 at 3:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Ronnie Sahlberg <sahlberg@google.com> writes:
>>
>>> We call read_ref_full with a pointer to flags from rename_ref but since
>>> we never actually use the returned flags we can just pass NULL here instead.
>>
>> Sensible, at least for the current callers.  I had to wonder if
>> rename_ref() would never want to take advantage of the flags return
>> parameter in the future, though.  For example, would it want to act
>> differently when the given ref turns out to be a symref?
>
> I don't know.
>
> We have a check if the old refname was a symref or not since the old
> version did not have code for how to handle renaming the reflog.
> (That check is removed in a later series when we have enough
> transaction code and reflog api changes so that we no longer need to
> call rename() for the reflog handling.)
>
> I can not think of any reason right now why, but if we need it we can
> add the argument back when the need arises.
>
>> Would it
>> want to report something when the ref to be overwritten was a broken
>> one?
>
> Good point.
>
> There are two cases where the new ref could be broken.
> 1) It could either contain a broken SHA1, like if we do this :
> $ echo "Broken ref" > .git/refs/heads/foo-broken-1
> 2) or it could be broken due to having a bad/invalid name :
> $ cp .git.refs.heads.master .git/refs/heads/foo-broken-1-\*...
>
> For 2) I think this should not be allowed so the rename should just
> fail with something like :
> $ ./git branch -M foo foo-broken-1-\*...
> fatal: 'foo-broken-1-*...' is not a valid branch name.
>
> For 1)  if the new branch already exists but it has a broken SHA1, for
> that case I think we should allow rename_ref to overwrite the existing
> bad SHA1 with the new, good, SHA1 value.
> Currently this does not work in master :
> $ echo "Broken ref" > .git/refs/heads/foo-broken-1
> $ ./git branch -m foo foo-broken-1
> error: unable to resolve reference refs/heads/foo-broken-1: Invalid argument
> error: unable to lock refs/heads/foo-broken-1 for update
> fatal: Branch rename failed
>
>
> And the only way to recover is to first delete the branch as my other
> patch in this series now allows and then trying the rename again.
>
> For 1), since we are planning to overwrite the current branch with a
> new SHA1 value, I think that what makes most sense would be to treat
> the "branch exist but is broken" as if the branch did not exist at all
> and just allow overwriting it with the new good value.
>
>
>
> Currently this does not work in master :
>
> $ echo "Broken ref" > .git/refs/heads/foo-broken-1
> $ ./git branch -m foo foo-broken-1
> error: unable to resolve reference refs/heads/foo-broken-1: Invalid argument
> error: unable to lock refs/heads/foo-broken-1 for update
> fatal: Branch rename failed
> so since this is not a regression I will not update this particular
> patch series but instead I
> can add a new patch to the next patch series to allow this so that we can do :
> $ echo "Broken ref" > .git/refs/heads/foo-broken-1
> $ ./git branch -m foo foo-broken-1
> <success>
>

I have a patch to make it possible to delete a broken ref that can not be resolved in : https://github.com/rsahlberg/git/commit/763ab16e1874d58a4fc5c37920abc1ea40ccd814

This patch is scheduled at the end of the next patch series (use transactions for all reflog updates) I plan to send out tomorrow.

Ronnie Sahlberg· Jul 16, 2014, 22:23 UTC · re: Ronnie Sahlberg · lore

[PATCH 06/12] refs.c: move the check for valid refname to lock_ref_sha1_basic

Move the check for check_refname_format from lock_any_ref_for_update to lock_ref_sha1_basic. At some later stage we will get rid of lock_any_ref_for_update completely.

If lock_ref_sha1_basic fails the check_refname_format test, set errno to EINVAL before returning NULL. This to guarantee that we will not return an error without updating errno.

This leaves lock_any_ref_for_updates as a no-op wrapper which could be removed. But this wrapper is also called from an external caller and we will soon make changes to the signature to lock_ref_sha1_basic that we do not want to expose to that caller.

This changes semantics for lock_ref_sha1_basic slightly. With this change it is no longer possible to open a ref that has a badly name which breaks any codepaths that tries to open and repair badly named refs. The normal refs API should not allow neither creating nor accessing refs with invalid names. If we need such recovery code we could add it as an option to git fsck and have git fsck be the only sanctioned way of bypassing the normal API and checks.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Show changes to refs.c +5 −2
diff --git a/refs.c b/refs.c
index 0df6894..f29f18a 100644
--- a/refs.c
+++ b/refs.c
@@ -2088,6 +2088,11 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	int missing = 0;
 	int attempts_remaining = 3;
 
+	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+		errno = EINVAL;
+		return NULL;
+	}
+
 	lock = xcalloc(1, sizeof(struct ref_lock));
 	lock->lock_fd = -1;
 
@@ -2179,8 +2184,6 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,
 					 const unsigned char *old_sha1,
 					 int flags, int *type_p)
 {
-	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
-		return NULL;
 	return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
 }
 

-- 
2.0.1.527.gc6b782e
Junio C Hamano· Jul 18, 2014, 22:37 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH 06/12] refs.c: move the check for valid refname to lock_ref_sha1_basic

Ronnie Sahlberg <sahlberg@google.com> writes:
Show 12 quoted lines
> Move the check for check_refname_format from lock_any_ref_for_update
> to lock_ref_sha1_basic. At some later stage we will get rid of
> lock_any_ref_for_update completely.
>
> If lock_ref_sha1_basic fails the check_refname_format test, set errno to
> EINVAL before returning NULL. This to guarantee that we will not return an
> error without updating errno.
>
> This leaves lock_any_ref_for_updates as a no-op wrapper which could be removed.
> But this wrapper is also called from an external caller and we will soon
> make changes to the signature to lock_ref_sha1_basic that we do not want to
> expose to that caller.

That might be sensible if our only goal were to remove lock-any-ref-for-updates, but I wonder what the impact of this change to other existing callers of lock-ref-sha1-basic. I may be recalling things incorrectly, but I suspect that it was deliberate to keep the lowest-level internal helper function (i.e. _basic()) to be lenient so that those who do not want the format checks can choose to pass refnames that are not exactly kosher.

> If we need such recovery code we could add it as an option to git fsck and have
> git fsck be the only sanctioned way of bypassing the normal API and checks.

But fsck is about checking and never about recovering, isn't it? Does it offer to remove misnamed refs? Should it?

Show 29 quoted lines
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  refs.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 0df6894..f29f18a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2088,6 +2088,11 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>  	int missing = 0;
>  	int attempts_remaining = 3;
>  
> +	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
> +		errno = EINVAL;
> +		return NULL;
> +	}
> +
>  	lock = xcalloc(1, sizeof(struct ref_lock));
>  	lock->lock_fd = -1;
>  
> @@ -2179,8 +2184,6 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,
>  					 const unsigned char *old_sha1,
>  					 int flags, int *type_p)
>  {
> -	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
> -		return NULL;
>  	return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
>  }
Ronnie Sahlberg· Jul 16, 2014, 22:23 UTC · re: Ronnie Sahlberg · lore

[PATCH 07/12] refs.c: call lock_ref_sha1_basic directly from commit

Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic directly from the commit function.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Show changes to refs.c +6 −6
diff --git a/refs.c b/refs.c
index f29f18a..d3fedbb 100644
--- a/refs.c
+++ b/refs.c
@@ -3575,12 +3575,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
-		update->lock = lock_any_ref_for_update(update->refname,
-						       (update->have_old ?
-							update->old_sha1 :
-							NULL),
-						       update->flags,
-						       &update->type);
+		update->lock = lock_ref_sha1_basic(update->refname,
+						   (update->have_old ?
+						    update->old_sha1 :
+						    NULL),
+						   update->flags,
+						   &update->type);
 		if (!update->lock) {
 			if (err)
 				strbuf_addf(err, "Cannot lock the ref '%s'.",

-- 
2.0.1.527.gc6b782e
Ronnie Sahlberg· Jul 16, 2014, 22:23 UTC · re: Ronnie Sahlberg · lore

[PATCH 08/12] refs.c: pass a skip list to name_conflict_fn

Allow passing a list of refs to skip checking to name_conflict_fn. There are some conditions where we want to allow a temporary conflict and skip checking those refs. For example if we have a transaction that 1, guarantees that m is a packed refs and there is no loose ref for m 2, the transaction will delete m from the packed ref 3, the transaction will create conflicting m/m

For this case we want to be able to lock and create m/m since we know that the conflict is only transient. I.e. the conflict will be automatically resolved by the transaction when it deletes m.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)
Show changes to refs.c +26 −15
diff --git a/refs.c b/refs.c
index d3fedbb..a115478 100644
--- a/refs.c
+++ b/refs.c
@@ -801,15 +801,18 @@ static int names_conflict(const char *refname1, const char *refname2)
 
 struct name_conflict_cb {
 	const char *refname;
-	const char *oldrefname;
 	const char *conflicting_refname;
+	const char **skip;
+	int skipnum;
 };
 
 static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
 {
 	struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
-	if (data->oldrefname && !strcmp(data->oldrefname, entry->name))
-		return 0;
+	int i;
+	for (i = 0; i < data->skipnum; i++)
+		if (!strcmp(entry->name, data->skip[i]))
+			return 0;
 	if (names_conflict(data->refname, entry->name)) {
 		data->conflicting_refname = entry->name;
 		return 1;
@@ -822,15 +825,18 @@ static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
  * conflicting with the name of an existing reference in dir.  If
  * oldrefname is non-NULL, ignore potential conflicts with oldrefname
  * (e.g., because oldrefname is scheduled for deletion in the same
- * operation).
+ * operation). skip contains a list of refs we want to skip checking for
+ * conflicts with.
  */
-static int is_refname_available(const char *refname, const char *oldrefname,
-				struct ref_dir *dir)
+static int is_refname_available(const char *refname,
+				struct ref_dir *dir,
+				const char **skip, int skipnum)
 {
 	struct name_conflict_cb data;
 	data.refname = refname;
-	data.oldrefname = oldrefname;
 	data.conflicting_refname = NULL;
+	data.skip = skip;
+	data.skipnum = skipnum;
 
 	sort_ref_dir(dir);
 	if (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data)) {
@@ -2077,7 +2083,8 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 /* This function should make sure errno is meaningful on error */
 static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 					    const unsigned char *old_sha1,
-					    int flags, int *type_p)
+					    int flags, int *type_p,
+					    const char **skip, int skipnum)
 {
 	char *ref_file;
 	const char *orig_refname = refname;
@@ -2126,7 +2133,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	 * name is a proper prefix of our refname.
 	 */
 	if (missing &&
-	     !is_refname_available(refname, NULL, get_packed_refs(&ref_cache))) {
+	     !is_refname_available(refname, get_packed_refs(&ref_cache),
+				   skip, skipnum)) {
 		last_errno = ENOTDIR;
 		goto error_return;
 	}
@@ -2184,7 +2192,7 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,
 					 const unsigned char *old_sha1,
 					 int flags, int *type_p)
 {
-	return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
+	return lock_ref_sha1_basic(refname, old_sha1, flags, type_p, NULL, 0);
 }
 
 /*
@@ -2654,10 +2662,12 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	if (!symref)
 		return error("refname %s not found", oldrefname);
 
-	if (!is_refname_available(newrefname, oldrefname, get_packed_refs(&ref_cache)))
+	if (!is_refname_available(newrefname, get_packed_refs(&ref_cache),
+				  &oldrefname, 1))
 		return 1;
 
-	if (!is_refname_available(newrefname, oldrefname, get_loose_refs(&ref_cache)))
+	if (!is_refname_available(newrefname, get_loose_refs(&ref_cache),
+				  &oldrefname, 1))
 		return 1;
 
 	if (log && rename(git_path("logs/%s", oldrefname), git_path(TMP_RENAMED_LOG)))
@@ -2687,7 +2697,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 
 	logmoved = log;
 
-	lock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL);
+	lock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL, NULL, 0);
 	if (!lock) {
 		error("unable to lock %s for update", newrefname);
 		goto rollback;
@@ -2702,7 +2712,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	return 0;
 
  rollback:
-	lock = lock_ref_sha1_basic(oldrefname, NULL, 0, NULL);
+	lock = lock_ref_sha1_basic(oldrefname, NULL, 0, NULL, NULL, 0);
 	if (!lock) {
 		error("unable to lock %s for rollback", oldrefname);
 		goto rollbacklog;
@@ -3580,7 +3590,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 						    update->old_sha1 :
 						    NULL),
 						   update->flags,
-						   &update->type);
+						   &update->type,
+						   delnames, delnum);
 		if (!update->lock) {
 			if (err)
 				strbuf_addf(err, "Cannot lock the ref '%s'.",

-- 
2.0.1.527.gc6b782e
Ronnie Sahlberg· Jul 16, 2014, 22:23 UTC · re: Ronnie Sahlberg · lore

[PATCH 09/12] refs.c: propagate any errno==ENOTDIR from _commit back to the callers

In _commit, ENOTDIR can happen in the call to lock_ref_sha1_basic, either when we lstat the new refname and it returns ENOTDIR or if the name checking function reports that the same type of conflict happened. In both cases it means that we can not create the new ref due to a name conflict.

For these cases, save the errno value and abort and make sure that the caller can see errno==ENOTDIR.

Also start defining specific return codes for _commit, assign -1 as a generic error and -2 as the error that refers to a name conflict. Callers can (and should) use that return value inspecting errno directly.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 22 +++++++++++++++-------
 refs.h |  6 ++++++
 2 files changed, 21 insertions(+), 7 deletions(-)
Show changes to 2 files +21 −7

refs.c, refs.h

diff --git a/refs.c b/refs.c
index a115478..69cbca5 100644
--- a/refs.c
+++ b/refs.c
@@ -3559,7 +3559,7 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err)
 {
-	int ret = 0, delnum = 0, i;
+	int ret = 0, delnum = 0, i, df_conflict = 0;
 	const char **delnames;
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
@@ -3577,9 +3577,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 	/* Copy, sort, and reject duplicate refs */
 	qsort(updates, n, sizeof(*updates), ref_update_compare);
-	ret = ref_update_reject_duplicates(updates, n, err);
-	if (ret)
+	if (ref_update_reject_duplicates(updates, n, err)) {
+		ret = -1;
 		goto cleanup;
+	}
 
 	/* Acquire all locks while verifying old values */
 	for (i = 0; i < n; i++) {
@@ -3593,10 +3594,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 						   &update->type,
 						   delnames, delnum);
 		if (!update->lock) {
+			if (errno == ENOTDIR)
+				df_conflict = 1;
 			if (err)
 				strbuf_addf(err, "Cannot lock the ref '%s'.",
 					    update->refname);
-			ret = 1;
+			ret = -1;
 			goto cleanup;
 		}
 	}
@@ -3614,6 +3617,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 
 				if (err)
 					strbuf_addf(err, str, update->refname);
+				ret = -1;
 				goto cleanup;
 			}
 		}
@@ -3624,14 +3628,16 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		struct ref_update *update = updates[i];
 
 		if (update->lock) {
-			ret |= delete_ref_loose(update->lock, update->type,
-						err);
+			if (delete_ref_loose(update->lock, update->type, err))
+				ret = -1;
+
 			if (!(update->flags & REF_ISPRUNING))
 				delnames[delnum++] = update->lock->ref_name;
 		}
 	}
 
-	ret |= repack_without_refs(delnames, delnum, err);
+	if (repack_without_refs(delnames, delnum, err))
+		ret = -1;
 	for (i = 0; i < delnum; i++)
 		unlink_or_warn(git_path("logs/%s", delnames[i]));
 	clear_loose_ref_cache(&ref_cache);
@@ -3644,6 +3650,8 @@ cleanup:
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
 	free(delnames);
+	if (df_conflict)
+		ret = -2;
 	return ret;
 }
 
diff --git a/refs.h b/refs.h
index 6a4d1a7..fc7942c 100644
--- a/refs.h
+++ b/refs.h
@@ -326,7 +326,13 @@ int ref_transaction_delete(struct ref_transaction *transaction,
  * Commit all of the changes that have been queued in transaction, as
  * atomically as possible.  Return a nonzero value if there is a
  * problem.
+ * If the transaction is already in failed state this function will return
+ * an error.
+ * Function returns 0 on success, -1 for generic failures and
+ * UPDATE_REFS_NAME_CONFLICT (-2) if the failure was due to a name
+ * collision (ENOTDIR).
  */
+#define UPDATE_REFS_NAME_CONFLICT -2
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err);
 

-- 
2.0.1.527.gc6b782e
Ronnie Sahlberg· Jul 16, 2014, 22:23 UTC · re: Ronnie Sahlberg · lore

[PATCH 10/12] fetch.c: change s_update_ref to use a ref transaction

Change s_update_ref to use a ref transaction for the ref update.
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/fetch.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)
Show changes to builtin/fetch.c +23 −10
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 92fad2d..383c385 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -404,23 +404,36 @@ static int s_update_ref(const char *action,
 {
 	char msg[1024];
 	char *rla = getenv("GIT_REFLOG_ACTION");
-	static struct ref_lock *lock;
+	struct ref_transaction *transaction;
+	struct strbuf err = STRBUF_INIT;
+	int ret, df_conflict = 0;
 
 	if (dry_run)
 		return 0;
 	if (!rla)
 		rla = default_rla.buf;
 	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
-	lock = lock_any_ref_for_update(ref->name,
-				       check_old ? ref->old_sha1 : NULL,
-				       0, NULL);
-	if (!lock)
-		return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
-					  STORE_REF_ERROR_OTHER;
-	if (write_ref_sha1(lock, ref->new_sha1, msg) < 0)
-		return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
-					  STORE_REF_ERROR_OTHER;
+
+	transaction = ref_transaction_begin(&err);
+	if (!transaction ||
+	    ref_transaction_update(transaction, ref->name, ref->new_sha1,
+				   ref->old_sha1, 0, check_old, msg, &err))
+		goto fail;
+
+	ret = ref_transaction_commit(transaction, &err);
+	if (ret == UPDATE_REFS_NAME_CONFLICT)
+		df_conflict = 1;
+	if (ret)
+		goto fail;
+
+	ref_transaction_free(transaction);
 	return 0;
+fail:
+	ref_transaction_free(transaction);
+	error("%s", err.buf);
+	strbuf_release(&err);
+	return df_conflict ? STORE_REF_ERROR_DF_CONFLICT
+			   : STORE_REF_ERROR_OTHER;
 }
 
 #define REFCOL_WIDTH  10

-- 
2.0.1.527.gc6b782e
Ronnie Sahlberg· Jul 16, 2014, 22:23 UTC · re: Ronnie Sahlberg · lore

[PATCH 11/12] refs.c: make write_ref_sha1 static

No external users call write_ref_sha1 any more so lets declare it static.
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 10 ++++++++--
 refs.h |  3 ---
 2 files changed, 8 insertions(+), 5 deletions(-)
Show changes to 2 files +8 −5

refs.c, refs.h

diff --git a/refs.c b/refs.c
index 69cbca5..6c7a9d2 100644
--- a/refs.c
+++ b/refs.c
@@ -2643,6 +2643,9 @@ static int rename_tmp_log(const char *newrefname)
 	return 0;
 }
 
+static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
+			  const char *logmsg);
+
 int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)
 {
 	unsigned char sha1[20], orig_sha1[20];
@@ -2892,8 +2895,11 @@ static int is_branch(const char *refname)
 	return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/");
 }
 
-/* This function must return a meaningful errno */
-int write_ref_sha1(struct ref_lock *lock,
+/*
+ * Writes sha1 into the ref specified by the lock. Makes sure that errno
+ * is sane on error.
+ */
+static int write_ref_sha1(struct ref_lock *lock,
 	const unsigned char *sha1, const char *logmsg)
 {
 	static char term = '\n';
diff --git a/refs.h b/refs.h
index fc7942c..b0476c1 100644
--- a/refs.h
+++ b/refs.h
@@ -196,9 +196,6 @@ extern int commit_ref(struct ref_lock *lock);
 /** Release any lock taken but not written. **/
 extern void unlock_ref(struct ref_lock *lock);
 
-/** Writes sha1 into the ref specified by the lock. **/
-extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);
-
 /*
  * Setup reflog before using. Set errno to something meaningful on failure.
  */

-- 
2.0.1.527.gc6b782e
Ronnie Sahlberg· Jul 16, 2014, 22:23 UTC · re: Ronnie Sahlberg · lore

[PATCH 12/12] refs.c: fix handling of badly named refs

We currently do not handle badly named refs well :
$ cp .git/refs/heads/master .git/refs/heads/master.....@\*@\\.
$ git branch
   fatal: Reference has invalid format: 'refs/heads/master.....@*@\.'
$ git branch -D master.....@\*@\\.
  error: branch 'master.....@*@\.' not found.

But we can not really recover from a badly named ref with less than manually deleting the .git/refs/heads/<refname> file.

Change resolve_ref_unsafe to take a flags field instead of a 'reading' boolean and update all callers that used a non-zero value for reading to pass the flag RESOLVE_REF_READING instead. Add another flag RESOLVE_REF_ALLOW_BAD_NAME that will make resolve_ref_unsafe skip checking the refname for sanity and use this from branch.c so that we will be able to call resolve_ref_unsafe on such refs when trying to delete it. Add checks for refname sanity when updating (not deleting) a ref in ref_transaction_update and in ref_transaction_create to make the transaction fail if an attempt is made to create/update a badly named ref. Since all ref changes will later go through the transaction layer this means we no longer need to check for and fail for bad refnames in lock_ref_sha1_basic.

Change lock_ref_sha1_basic to not fail for bad refnames. Just check the refname, and print an error, and remember that the refname is bad so that we can skip calling verify_lock().

Allow reading refs with bad names from loose refs and packed refs but flag them as broken. This means that the refs will at least show up in git branch even if their name is invalid/broken.

Since we now do the refname checks, when we need to, before the call to create_ref_entry we can remove the check_name argument to the function.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/blame.c         |   2 +-
 builtin/branch.c        |   6 ++-
 builtin/clone.c         |   2 +-
 builtin/fmt-merge-msg.c |   2 +-
 builtin/for-each-ref.c  |   6 ++-
 builtin/log.c           |   3 +-
 builtin/remote.c        |   5 +-
 builtin/show-branch.c   |   6 ++-
 bundle.c                |   2 +-
 cache.h                 |  18 ++++---
 http-backend.c          |   3 +-
 reflog-walk.c           |   3 +-
 refs.c                  | 126 ++++++++++++++++++++++++++++++------------------
 remote.c                |   6 ++-
 sequencer.c             |   2 +-
 transport-helper.c      |   2 +-
 transport.c             |   5 +-
 17 files changed, 123 insertions(+), 76 deletions(-)
Show changes to 17 files +123 −76

builtin/blame.c, builtin/branch.c, builtin/clone.c, builtin/fmt-merge-msg.c, builtin/for-each-ref.c, builtin/log.c, builtin/remote.c, builtin/show-branch.c, bundle.c, cache.h, http-backend.c, reflog-walk.c, refs.c, remote.c, sequencer.c, transport-helper.c, transport.c

diff --git a/builtin/blame.c b/builtin/blame.c
index 662e3fe..76340e2 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2278,7 +2278,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 	commit->object.type = OBJ_COMMIT;
 	parent_tail = &commit->parents;
 
-	if (!resolve_ref_unsafe("HEAD", head_sha1, 1, NULL))
+	if (!resolve_ref_unsafe("HEAD", head_sha1, RESOLVE_REF_READING, NULL))
 		die("no such ref: HEAD");
 
 	parent_tail = append_parent(parent_tail, head_sha1);
diff --git a/builtin/branch.c b/builtin/branch.c
index 652b1d2..5c95656 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -129,7 +129,8 @@ static int branch_merged(int kind, const char *name,
 		    branch->merge[0] &&
 		    branch->merge[0]->dst &&
 		    (reference_name = reference_name_to_free =
-		     resolve_refdup(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
+		     resolve_refdup(branch->merge[0]->dst, sha1,
+				    RESOLVE_REF_READING, NULL)) != NULL)
 			reference_rev = lookup_commit_reference(sha1);
 	}
 	if (!reference_rev)
@@ -233,7 +234,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		free(name);
 
 		name = mkpathdup(fmt, bname.buf);
-		target = resolve_ref_unsafe(name, sha1, 0, &flags);
+		target = resolve_ref_unsafe(name, sha1,
+					    RESOLVE_REF_ALLOW_BAD_NAME, &flags);
 		if (!target ||
 		    (!(flags & REF_ISSYMREF) && is_null_sha1(sha1))) {
 			error(remote_branch
diff --git a/builtin/clone.c b/builtin/clone.c
index b12989d..f7307e6 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -622,7 +622,7 @@ static int checkout(void)
 	if (option_no_checkout)
 		return 0;
 
-	head = resolve_refdup("HEAD", sha1, 1, NULL);
+	head = resolve_refdup("HEAD", sha1, RESOLVE_REF_READING, NULL);
 	if (!head) {
 		warning(_("remote HEAD refers to nonexistent ref, "
 			  "unable to checkout.\n"));
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 3906eda..d8ab177 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -602,7 +602,7 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
 
 	/* get current branch */
 	current_branch = current_branch_to_free =
-		resolve_refdup("HEAD", head_sha1, 1, NULL);
+		resolve_refdup("HEAD", head_sha1, RESOLVE_REF_READING, NULL);
 	if (!current_branch)
 		die("No current branch");
 	if (starts_with(current_branch, "refs/heads/"))
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 4135980..a5833fd 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -649,7 +649,8 @@ static void populate_value(struct refinfo *ref)
 
 	if (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {
 		unsigned char unused1[20];
-		ref->symref = resolve_refdup(ref->refname, unused1, 1, NULL);
+		ref->symref = resolve_refdup(ref->refname, unused1,
+					     RESOLVE_REF_READING, NULL);
 		if (!ref->symref)
 			ref->symref = "";
 	}
@@ -707,7 +708,8 @@ static void populate_value(struct refinfo *ref)
 			const char *head;
 			unsigned char sha1[20];
 
-			head = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
+			head = resolve_ref_unsafe("HEAD", sha1,
+						  RESOLVE_REF_READING, NULL);
 			if (!strcmp(ref->refname, head))
 				v->s = "*";
 			else
diff --git a/builtin/log.c b/builtin/log.c
index a7ba211..92db809 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1395,7 +1395,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		if (check_head) {
 			unsigned char sha1[20];
 			const char *ref;
-			ref = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
+			ref = resolve_ref_unsafe("HEAD", sha1,
+						 RESOLVE_REF_READING, NULL);
 			if (ref && starts_with(ref, "refs/heads/"))
 				branch_name = xstrdup(ref + strlen("refs/heads/"));
 			else
diff --git a/builtin/remote.c b/builtin/remote.c
index 401feb3..be8ebac 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -568,7 +568,8 @@ static int read_remote_branches(const char *refname,
 	strbuf_addf(&buf, "refs/remotes/%s/", rename->old);
 	if (starts_with(refname, buf.buf)) {
 		item = string_list_append(rename->remote_branches, xstrdup(refname));
-		symref = resolve_ref_unsafe(refname, orig_sha1, 1, &flag);
+		symref = resolve_ref_unsafe(refname, orig_sha1,
+					    RESOLVE_REF_READING, &flag);
 		if (flag & REF_ISSYMREF)
 			item->util = xstrdup(symref);
 		else
@@ -704,7 +705,7 @@ static int mv(int argc, const char **argv)
 		int flag = 0;
 		unsigned char sha1[20];
 
-		read_ref_full(item->string, sha1, 1, &flag);
+		read_ref_full(item->string, sha1, RESOLVE_REF_READING, &flag);
 		if (!(flag & REF_ISSYMREF))
 			continue;
 		if (delete_ref(item->string, NULL, REF_NODEREF))
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index d873172..a9a5eb3 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -727,7 +727,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		if (ac == 0) {
 			static const char *fake_av[2];
 
-			fake_av[0] = resolve_refdup("HEAD", sha1, 1, NULL);
+			fake_av[0] = resolve_refdup("HEAD", sha1,
+						    RESOLVE_REF_READING, NULL);
 			fake_av[1] = NULL;
 			av = fake_av;
 			ac = 1;
@@ -789,7 +790,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		}
 	}
 
-	head_p = resolve_ref_unsafe("HEAD", head_sha1, 1, NULL);
+	head_p = resolve_ref_unsafe("HEAD", head_sha1,
+				    RESOLVE_REF_READING, NULL);
 	if (head_p) {
 		head_len = strlen(head_p);
 		memcpy(head, head_p, head_len + 1);
diff --git a/bundle.c b/bundle.c
index 1222952..8aaf5f8 100644
--- a/bundle.c
+++ b/bundle.c
@@ -311,7 +311,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 			continue;
 		if (dwim_ref(e->name, strlen(e->name), sha1, &ref) != 1)
 			continue;
-		if (read_ref_full(e->name, sha1, 1, &flag))
+		if (read_ref_full(e->name, sha1, RESOLVE_REF_READING, &flag))
 			flag = 0;
 		display_ref = (flag & REF_ISSYMREF) ? e->name : ref;
 
diff --git a/cache.h b/cache.h
index 4ca4583..aab246d 100644
--- a/cache.h
+++ b/cache.h
@@ -948,7 +948,7 @@ extern int get_sha1_hex(const char *hex, unsigned char *sha1);
 
 extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
 extern int read_ref_full(const char *refname, unsigned char *sha1,
-			 int reading, int *flags);
+			 int flags, int *ref_flag);
 extern int read_ref(const char *refname, unsigned char *sha1);
 
 /*
@@ -960,17 +960,17 @@ extern int read_ref(const char *refname, unsigned char *sha1);
  * or the input ref.
  *
  * If the reference cannot be resolved to an object, the behavior
- * depends on the "reading" argument:
+ * depends on the RESOLVE_REF_READING flag:
  *
- * - If reading is set, return NULL.
+ * - If RESOLVE_REF_READING is set, return NULL.
  *
- * - If reading is not set, clear sha1 and return the name of the last
- *   reference name in the chain, which will either be a non-symbolic
+ * - If RESOLVE_REF_READING is not set, clear sha1 and return the name of
+ *   the last reference name in the chain, which will either be a non-symbolic
  *   reference or an undefined reference.  If this is a prelude to
  *   "writing" to the ref, the return value is the name of the ref
  *   that will actually be created or changed.
  *
- * If flag is non-NULL, set the value that it points to the
+ * If ref_flag is non-NULL, set the value that it points to the
  * combination of REF_ISPACKED (if the reference was found among the
  * packed references) and REF_ISSYMREF (if the initial reference was a
  * symbolic reference).
@@ -981,8 +981,10 @@ extern int read_ref(const char *refname, unsigned char *sha1);
  *
  * errno is set to something meaningful on error.
  */
-extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag);
-extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);
+#define RESOLVE_REF_READING        0x01
+#define RESOLVE_REF_ALLOW_BAD_NAME 0x02
+extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int flags, int *ref_flag);
+extern char *resolve_refdup(const char *ref, unsigned char *sha1, int flags, int *ref_flag);
 
 extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);
 extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
diff --git a/http-backend.c b/http-backend.c
index d2c0a62..059f790 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -417,7 +417,8 @@ static int show_head_ref(const char *refname, const unsigned char *sha1,
 
 	if (flag & REF_ISSYMREF) {
 		unsigned char unused[20];
-		const char *target = resolve_ref_unsafe(refname, unused, 1, NULL);
+		const char *target = resolve_ref_unsafe(refname, unused,
+						RESOLVE_REF_READING, NULL);
 		const char *target_nons = strip_namespace(target);
 
 		strbuf_addf(buf, "ref: %s\n", target_nons);
diff --git a/reflog-walk.c b/reflog-walk.c
index 9ce8b53..d80a42a 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -48,7 +48,8 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)
 		unsigned char sha1[20];
 		const char *name;
 		void *name_to_free;
-		name = name_to_free = resolve_refdup(ref, sha1, 1, NULL);
+		name = name_to_free = resolve_refdup(ref, sha1,
+						     RESOLVE_REF_READING, NULL);
 		if (name) {
 			for_each_reflog_ent(name, read_one_reflog, reflogs);
 			free(name_to_free);
diff --git a/refs.c b/refs.c
index 6c7a9d2..6dcb920 100644
--- a/refs.c
+++ b/refs.c
@@ -281,15 +281,11 @@ static struct ref_dir *get_ref_dir(struct ref_entry *entry)
 }
 
 static struct ref_entry *create_ref_entry(const char *refname,
-					  const unsigned char *sha1, int flag,
-					  int check_name)
+					  const unsigned char *sha1, int flag)
 {
 	int len;
 	struct ref_entry *ref;
 
-	if (check_name &&
-	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT))
-		die("Reference has invalid format: '%s'", refname);
 	len = strlen(refname) + 1;
 	ref = xmalloc(sizeof(struct ref_entry) + len);
 	hashcpy(ref->u.value.sha1, sha1);
@@ -1062,7 +1058,12 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
 
 		refname = parse_ref_line(refline, sha1);
 		if (refname) {
-			last = create_ref_entry(refname, sha1, REF_ISPACKED, 1);
+			int flag = REF_ISPACKED;
+
+			if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT)) {
+				flag |= REF_ISBROKEN;
+			}
+			last = create_ref_entry(refname, sha1, flag);
 			if (peeled == PEELED_FULLY ||
 			    (peeled == PEELED_TAGS && starts_with(refname, "refs/tags/")))
 				last->flag |= REF_KNOWS_PEELED;
@@ -1135,8 +1136,10 @@ void add_packed_ref(const char *refname, const unsigned char *sha1)
 
 	if (!packed_ref_cache->lock)
 		die("internal error: packed refs not locked");
+	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT))
+		die("Reference has invalid format: '%s'", refname);
 	add_ref(get_packed_ref_dir(packed_ref_cache),
-		create_ref_entry(refname, sha1, REF_ISPACKED, 1));
+		create_ref_entry(refname, sha1, REF_ISPACKED));
 }
 
 /*
@@ -1194,12 +1197,17 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
 					hashclr(sha1);
 					flag |= REF_ISBROKEN;
 				}
-			} else if (read_ref_full(refname.buf, sha1, 1, &flag)) {
+			} else if (read_ref_full(refname.buf, sha1,
+						 RESOLVE_REF_READING, &flag)) {
+				hashclr(sha1);
+				flag |= REF_ISBROKEN;
+			}
+			if (check_refname_format(refname.buf, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT)) {
 				hashclr(sha1);
 				flag |= REF_ISBROKEN;
 			}
 			add_entry_to_dir(dir,
-					 create_ref_entry(refname.buf, sha1, flag, 1));
+					 create_ref_entry(refname.buf, sha1, flag));
 		}
 		strbuf_setlen(&refname, dirnamelen);
 	}
@@ -1346,21 +1354,21 @@ static const char *handle_missing_loose_ref(const char *refname,
 }
 
 /* This function needs to return a meaningful errno on failure */
-const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)
+const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int flags, int *ref_flag)
 {
 	int depth = MAXDEPTH;
 	ssize_t len;
 	char buffer[256];
 	static char refname_buffer[256];
 
-	if (flag)
-		*flag = 0;
+	if (ref_flag)
+		*ref_flag = 0;
 
-	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+	if (!(flags & RESOLVE_REF_ALLOW_BAD_NAME) &&
+	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
 		errno = EINVAL;
 		return NULL;
 	}
-
 	for (;;) {
 		char path[PATH_MAX];
 		struct stat st;
@@ -1387,7 +1395,8 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 		if (lstat(path, &st) < 0) {
 			if (errno == ENOENT)
 				return handle_missing_loose_ref(refname, sha1,
-								reading, flag);
+						flags & RESOLVE_REF_READING,
+						ref_flag);
 			else
 				return NULL;
 		}
@@ -1407,8 +1416,8 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 					!check_refname_format(buffer, 0)) {
 				strcpy(refname_buffer, buffer);
 				refname = refname_buffer;
-				if (flag)
-					*flag |= REF_ISSYMREF;
+				if (ref_flag)
+					*ref_flag |= REF_ISSYMREF;
 				continue;
 			}
 		}
@@ -1453,21 +1462,21 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 			 */
 			if (get_sha1_hex(buffer, sha1) ||
 			    (buffer[40] != '\0' && !isspace(buffer[40]))) {
-				if (flag)
-					*flag |= REF_ISBROKEN;
+				if (ref_flag)
+					*ref_flag |= REF_ISBROKEN;
 				errno = EINVAL;
 				return NULL;
 			}
 			return refname;
 		}
-		if (flag)
-			*flag |= REF_ISSYMREF;
+		if (ref_flag)
+			*ref_flag |= REF_ISSYMREF;
 		buf = buffer + 4;
 		while (isspace(*buf))
 			buf++;
 		if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
-			if (flag)
-				*flag |= REF_ISBROKEN;
+			if (ref_flag)
+				*ref_flag |= REF_ISBROKEN;
 			errno = EINVAL;
 			return NULL;
 		}
@@ -1475,9 +1484,9 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 	}
 }
 
-char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)
+char *resolve_refdup(const char *ref, unsigned char *sha1, int flags, int *ref_flag)
 {
-	const char *ret = resolve_ref_unsafe(ref, sha1, reading, flag);
+	const char *ret = resolve_ref_unsafe(ref, sha1, flags, ref_flag);
 	return ret ? xstrdup(ret) : NULL;
 }
 
@@ -1488,22 +1497,22 @@ struct ref_filter {
 	void *cb_data;
 };
 
-int read_ref_full(const char *refname, unsigned char *sha1, int reading, int *flags)
+int read_ref_full(const char *refname, unsigned char *sha1, int flags, int *ref_flag)
 {
-	if (resolve_ref_unsafe(refname, sha1, reading, flags))
+	if (resolve_ref_unsafe(refname, sha1, flags, ref_flag))
 		return 0;
 	return -1;
 }
 
 int read_ref(const char *refname, unsigned char *sha1)
 {
-	return read_ref_full(refname, sha1, 1, NULL);
+	return read_ref_full(refname, sha1, RESOLVE_REF_READING, NULL);
 }
 
 int ref_exists(const char *refname)
 {
 	unsigned char sha1[20];
-	return !!resolve_ref_unsafe(refname, sha1, 1, NULL);
+	return !!resolve_ref_unsafe(refname, sha1, RESOLVE_REF_READING, NULL);
 }
 
 static int filter_refs(const char *refname, const unsigned char *sha1, int flags,
@@ -1617,7 +1626,7 @@ int peel_ref(const char *refname, unsigned char *sha1)
 		return 0;
 	}
 
-	if (read_ref_full(refname, base, 1, &flag))
+	if (read_ref_full(refname, base, RESOLVE_REF_READING, &flag))
 		return -1;
 
 	/*
@@ -1783,7 +1792,7 @@ static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)
 		return 0;
 	}
 
-	if (!read_ref_full("HEAD", sha1, 1, &flag))
+	if (!read_ref_full("HEAD", sha1, RESOLVE_REF_READING, &flag))
 		return fn("HEAD", sha1, flag, cb_data);
 
 	return 0;
@@ -1863,7 +1872,7 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)
 	int flag;
 
 	strbuf_addf(&buf, "%sHEAD", get_git_namespace());
-	if (!read_ref_full(buf.buf, sha1, 1, &flag))
+	if (!read_ref_full(buf.buf, sha1, RESOLVE_REF_READING, &flag))
 		ret = fn(buf.buf, sha1, flag, cb_data);
 	strbuf_release(&buf);
 
@@ -1958,7 +1967,8 @@ int refname_match(const char *abbrev_name, const char *full_name)
 static struct ref_lock *verify_lock(struct ref_lock *lock,
 	const unsigned char *old_sha1, int mustexist)
 {
-	if (read_ref_full(lock->ref_name, lock->old_sha1, mustexist, NULL)) {
+	if (read_ref_full(lock->ref_name, lock->old_sha1,
+			  mustexist ? RESOLVE_REF_READING : 0, NULL)) {
 		int save_errno = errno;
 		error("Can't verify ref %s", lock->ref_name);
 		unlock_ref(lock);
@@ -2031,7 +2041,8 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 
 		this_result = refs_found ? sha1_from_ref : sha1;
 		mksnpath(fullref, sizeof(fullref), *p, len, str);
-		r = resolve_ref_unsafe(fullref, this_result, 1, &flag);
+		r = resolve_ref_unsafe(fullref, this_result,
+				       RESOLVE_REF_READING, &flag);
 		if (r) {
 			if (!refs_found++)
 				*ref = xstrdup(r);
@@ -2060,7 +2071,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 		const char *ref, *it;
 
 		mksnpath(path, sizeof(path), *p, len, str);
-		ref = resolve_ref_unsafe(path, hash, 1, NULL);
+		ref = resolve_ref_unsafe(path, hash, RESOLVE_REF_READING, NULL);
 		if (!ref)
 			continue;
 		if (reflog_exists(path))
@@ -2092,18 +2103,22 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	int last_errno = 0;
 	int type, lflags;
 	int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
+	int resolve_flags;
 	int missing = 0;
 	int attempts_remaining = 3;
-
-	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
-		errno = EINVAL;
-		return NULL;
-	}
+	int bad_refname;
 
 	lock = xcalloc(1, sizeof(struct ref_lock));
 	lock->lock_fd = -1;
 
-	refname = resolve_ref_unsafe(refname, lock->old_sha1, mustexist, &type);
+	bad_refname = check_refname_format(refname, REFNAME_ALLOW_ONELEVEL);
+
+	resolve_flags = RESOLVE_REF_ALLOW_BAD_NAME;
+	if (mustexist)
+		resolve_flags |= RESOLVE_REF_READING;
+
+	refname = resolve_ref_unsafe(refname, lock->old_sha1, resolve_flags,
+				     &type);
 	if (!refname && errno == EISDIR) {
 		/* we are trying to lock foo but we used to
 		 * have foo/bar which now does not exist;
@@ -2116,7 +2131,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			error("there are still refs under '%s'", orig_refname);
 			goto error_return;
 		}
-		refname = resolve_ref_unsafe(orig_refname, lock->old_sha1, mustexist, &type);
+		refname = resolve_ref_unsafe(orig_refname, lock->old_sha1,
+					     resolve_flags, &type);
 	}
 	if (type_p)
 	    *type_p = type;
@@ -2180,6 +2196,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		else
 			unable_to_lock_index_die(ref_file, errno);
 	}
+	if (bad_refname)
+		return lock;
 	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
 
  error_return:
@@ -2343,8 +2361,9 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)
 		packed_entry->flag = REF_ISPACKED | REF_KNOWS_PEELED;
 		hashcpy(packed_entry->u.value.sha1, entry->u.value.sha1);
 	} else {
-		packed_entry = create_ref_entry(entry->name, entry->u.value.sha1,
-						REF_ISPACKED | REF_KNOWS_PEELED, 0);
+		packed_entry = create_ref_entry(entry->name,
+					entry->u.value.sha1,
+					REF_ISPACKED | REF_KNOWS_PEELED);
 		add_ref(cb->packed_refs, packed_entry);
 	}
 	hashcpy(packed_entry->u.value.peeled, entry->u.value.peeled);
@@ -2658,7 +2677,8 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldrefname);
 
-	symref = resolve_ref_unsafe(oldrefname, orig_sha1, 1, &flag);
+	symref = resolve_ref_unsafe(oldrefname, orig_sha1,
+				    RESOLVE_REF_READING, &flag);
 	if (flag & REF_ISSYMREF)
 		return error("refname %s is a symbolic ref, renaming it is not supported",
 			oldrefname);
@@ -2682,7 +2702,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 		goto rollback;
 	}
 
-	if (!read_ref_full(newrefname, sha1, 1, NULL) &&
+	if (!read_ref_full(newrefname, sha1, RESOLVE_REF_READING, NULL) &&
 	    delete_ref(newrefname, sha1, REF_NODEREF)) {
 		if (errno==EISDIR) {
 			if (remove_empty_directories(git_path("%s", newrefname))) {
@@ -2960,7 +2980,8 @@ static int write_ref_sha1(struct ref_lock *lock,
 		unsigned char head_sha1[20];
 		int head_flag;
 		const char *head_ref;
-		head_ref = resolve_ref_unsafe("HEAD", head_sha1, 1, &head_flag);
+		head_ref = resolve_ref_unsafe("HEAD", head_sha1,
+					      RESOLVE_REF_READING, &head_flag);
 		if (head_ref && (head_flag & REF_ISSYMREF) &&
 		    !strcmp(head_ref, lock->ref_name))
 			log_ref_write("HEAD", lock->old_sha1, sha1, logmsg);
@@ -3446,6 +3467,12 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	if (have_old && !old_sha1)
 		die("BUG: have_old is true but old_sha1 is NULL");
 
+	if (!is_null_sha1(new_sha1) &&
+	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+		strbuf_addf(err, "Bad refname: %s", refname);
+		return -1;
+	}
+
 	update = add_update(transaction, refname);
 	hashcpy(update->new_sha1, new_sha1);
 	update->flags = flags;
@@ -3471,6 +3498,11 @@ int ref_transaction_create(struct ref_transaction *transaction,
 	if (!new_sha1 || is_null_sha1(new_sha1))
 		die("BUG: create ref with null new_sha1");
 
+	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+		strbuf_addf(err, "Bad refname: %s", refname);
+		return -1;
+	}
+
 	update = add_update(transaction, refname);
 
 	hashcpy(update->new_sha1, new_sha1);
diff --git a/remote.c b/remote.c
index ae04043..de84ac3 100644
--- a/remote.c
+++ b/remote.c
@@ -1121,7 +1121,8 @@ static char *guess_ref(const char *name, struct ref *peer)
 	struct strbuf buf = STRBUF_INIT;
 	unsigned char sha1[20];
 
-	const char *r = resolve_ref_unsafe(peer->name, sha1, 1, NULL);
+	const char *r = resolve_ref_unsafe(peer->name, sha1,
+					   RESOLVE_REF_READING, NULL);
 	if (!r)
 		return NULL;
 
@@ -1182,7 +1183,8 @@ static int match_explicit(struct ref *src, struct ref *dst,
 		unsigned char sha1[20];
 		int flag;
 
-		dst_value = resolve_ref_unsafe(matched_src->name, sha1, 1, &flag);
+		dst_value = resolve_ref_unsafe(matched_src->name, sha1,
+					       RESOLVE_REF_READING, &flag);
 		if (!dst_value ||
 		    ((flag & REF_ISSYMREF) &&
 		     !starts_with(dst_value, "refs/heads/")))
diff --git a/sequencer.c b/sequencer.c
index 284059b..e1419bf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -365,7 +365,7 @@ static int is_index_unchanged(void)
 	unsigned char head_sha1[20];
 	struct commit *head_commit;
 
-	if (!resolve_ref_unsafe("HEAD", head_sha1, 1, NULL))
+	if (!resolve_ref_unsafe("HEAD", head_sha1, RESOLVE_REF_READING, NULL))
 		return error(_("Could not resolve HEAD commit\n"));
 
 	head_commit = lookup_commit(head_sha1);
diff --git a/transport-helper.c b/transport-helper.c
index 84c616f..270ae28 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -889,7 +889,7 @@ static int push_refs_with_export(struct transport *transport,
 					int flag;
 
 					/* Follow symbolic refs (mainly for HEAD). */
-					name = resolve_ref_unsafe(ref->peer_ref->name, sha1, 1, &flag);
+					name = resolve_ref_unsafe(ref->peer_ref->name, sha1, RESOLVE_REF_READING, &flag);
 					if (!name || !(flag & REF_ISSYMREF))
 						name = ref->peer_ref->name;
 
diff --git a/transport.c b/transport.c
index 325f03e..f40e950 100644
--- a/transport.c
+++ b/transport.c
@@ -168,7 +168,8 @@ static void set_upstreams(struct transport *transport, struct ref *refs,
 		/* Follow symbolic refs (mainly for HEAD). */
 		localname = ref->peer_ref->name;
 		remotename = ref->name;
-		tmp = resolve_ref_unsafe(localname, sha, 1, &flag);
+		tmp = resolve_ref_unsafe(localname, sha,
+					 RESOLVE_REF_READING, &flag);
 		if (tmp && flag & REF_ISSYMREF &&
 			starts_with(tmp, "refs/heads/"))
 			localname = tmp;
@@ -753,7 +754,7 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 	unsigned char head_sha1[20];
 	char *head;
 
-	head = resolve_refdup("HEAD", head_sha1, 1, NULL);
+	head = resolve_refdup("HEAD", head_sha1, RESOLVE_REF_READING, NULL);
 
 	if (verbose) {
 		for (ref = refs; ref; ref = ref->next)

-- 
2.0.1.527.gc6b782e
Junio C Hamano· Jul 22, 2014, 20:41 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH 12/12] refs.c: fix handling of badly named refs

Ronnie Sahlberg <sahlberg@google.com> writes:
Show 17 quoted lines
> We currently do not handle badly named refs well :
> $ cp .git/refs/heads/master .git/refs/heads/master.....@\*@\\.
> $ git branch
>    fatal: Reference has invalid format: 'refs/heads/master.....@*@\.'
> $ git branch -D master.....@\*@\\.
>   error: branch 'master.....@*@\.' not found.
>
> But we can not really recover from a badly named ref with less than
> manually deleting the .git/refs/heads/<refname> file.
>
> Change resolve_ref_unsafe to take a flags field instead of a 'reading'
> boolean and update all callers that used a non-zero value for reading
> to pass the flag RESOLVE_REF_READING instead.
> Add another flag RESOLVE_REF_ALLOW_BAD_NAME that will make
> resolve_ref_unsafe skip checking the refname for sanity and use this
> from branch.c so that we will be able to call resolve_ref_unsafe on such
> refs when trying to delete it.
Makes sense.
Show 10 quoted lines
> Add checks for refname sanity when updating (not deleting) a ref in
> ref_transaction_update and in ref_transaction_create to make the transaction
> fail if an attempt is made to create/update a badly named ref.
> Since all ref changes will later go through the transaction layer this means
> we no longer need to check for and fail for bad refnames in
> lock_ref_sha1_basic.
>
> Change lock_ref_sha1_basic to not fail for bad refnames. Just check the
> refname, and print an error, and remember that the refname is bad so that
> we can skip calling verify_lock().
This is somewhat puzzling, though.
Show 6 quoted lines
> @@ -2180,6 +2196,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>  		else
>  			unable_to_lock_index_die(ref_file, errno);
>  	}
> +	if (bad_refname)
> +		return lock;

Hmph. If the only offence was that the ref was named badly due to historically loose code, does the caller not still benefit from the verify-lock check to make sure that other people did not muck with the ref while we were planning to update it?

>  	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
>  
>   error_return:
Ronnie Sahlberg· Jul 22, 2014, 21:30 UTC · re: Junio C Hamano · lore

Re: [PATCH 12/12] refs.c: fix handling of badly named refs

On Tue, Jul 22, 2014 at 1:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 47 quoted lines
> Ronnie Sahlberg <sahlberg@google.com> writes:
>
>> We currently do not handle badly named refs well :
>> $ cp .git/refs/heads/master .git/refs/heads/master.....@\*@\\.
>> $ git branch
>>    fatal: Reference has invalid format: 'refs/heads/master.....@*@\.'
>> $ git branch -D master.....@\*@\\.
>>   error: branch 'master.....@*@\.' not found.
>>
>> But we can not really recover from a badly named ref with less than
>> manually deleting the .git/refs/heads/<refname> file.
>>
>> Change resolve_ref_unsafe to take a flags field instead of a 'reading'
>> boolean and update all callers that used a non-zero value for reading
>> to pass the flag RESOLVE_REF_READING instead.
>> Add another flag RESOLVE_REF_ALLOW_BAD_NAME that will make
>> resolve_ref_unsafe skip checking the refname for sanity and use this
>> from branch.c so that we will be able to call resolve_ref_unsafe on such
>> refs when trying to delete it.
>
> Makes sense.
>
>> Add checks for refname sanity when updating (not deleting) a ref in
>> ref_transaction_update and in ref_transaction_create to make the transaction
>> fail if an attempt is made to create/update a badly named ref.
>> Since all ref changes will later go through the transaction layer this means
>> we no longer need to check for and fail for bad refnames in
>> lock_ref_sha1_basic.
>>
>> Change lock_ref_sha1_basic to not fail for bad refnames. Just check the
>> refname, and print an error, and remember that the refname is bad so that
>> we can skip calling verify_lock().
>
> This is somewhat puzzling, though.
>
>> @@ -2180,6 +2196,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>>               else
>>                       unable_to_lock_index_die(ref_file, errno);
>>       }
>> +     if (bad_refname)
>> +             return lock;
>
> Hmph.  If the only offence was that the ref was named badly due to
> historically loose code, does the caller not still benefit from the
> verify-lock check to make sure that other people did not muck with
> the ref while we were planning to update it?
>
I don't think we need to do that.
That would imply that we would need to be able to also allow reading
the content of a badly named ref.
Currently a badly named ref can not be accessed by any function except
 git branch -D <badlynamedref> which contains the special flag that
allows locking it eventhough the ref has an illegal name.
So no one else should be able to read or modify the ref at all.

I think it is sufficient for this case to just have the semantics "just delete it, I don't care what it used to point to." for this special case git branch -D <badrefname> so therefore since it is not the content of the ref but the name of the ref itself we have a problem with I don't think we need to read the old value or verify it.

Ronnie Sahlberg· Jul 22, 2014, 21:36 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH 12/12] refs.c: fix handling of badly named refs

On Tue, Jul 22, 2014 at 2:30 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
Show 64 quoted lines
> On Tue, Jul 22, 2014 at 1:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Ronnie Sahlberg <sahlberg@google.com> writes:
>>
>>> We currently do not handle badly named refs well :
>>> $ cp .git/refs/heads/master .git/refs/heads/master.....@\*@\\.
>>> $ git branch
>>>    fatal: Reference has invalid format: 'refs/heads/master.....@*@\.'
>>> $ git branch -D master.....@\*@\\.
>>>   error: branch 'master.....@*@\.' not found.
>>>
>>> But we can not really recover from a badly named ref with less than
>>> manually deleting the .git/refs/heads/<refname> file.
>>>
>>> Change resolve_ref_unsafe to take a flags field instead of a 'reading'
>>> boolean and update all callers that used a non-zero value for reading
>>> to pass the flag RESOLVE_REF_READING instead.
>>> Add another flag RESOLVE_REF_ALLOW_BAD_NAME that will make
>>> resolve_ref_unsafe skip checking the refname for sanity and use this
>>> from branch.c so that we will be able to call resolve_ref_unsafe on such
>>> refs when trying to delete it.
>>
>> Makes sense.
>>
>>> Add checks for refname sanity when updating (not deleting) a ref in
>>> ref_transaction_update and in ref_transaction_create to make the transaction
>>> fail if an attempt is made to create/update a badly named ref.
>>> Since all ref changes will later go through the transaction layer this means
>>> we no longer need to check for and fail for bad refnames in
>>> lock_ref_sha1_basic.
>>>
>>> Change lock_ref_sha1_basic to not fail for bad refnames. Just check the
>>> refname, and print an error, and remember that the refname is bad so that
>>> we can skip calling verify_lock().
>>
>> This is somewhat puzzling, though.
>>
>>> @@ -2180,6 +2196,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>>>               else
>>>                       unable_to_lock_index_die(ref_file, errno);
>>>       }
>>> +     if (bad_refname)
>>> +             return lock;
>>
>> Hmph.  If the only offence was that the ref was named badly due to
>> historically loose code, does the caller not still benefit from the
>> verify-lock check to make sure that other people did not muck with
>> the ref while we were planning to update it?
>>
>
> I don't think we need to do that.
> That would imply that we would need to be able to also allow reading
> the content of a badly named ref.
> Currently a badly named ref can not be accessed by any function except
>  git branch -D <badlynamedref> which contains the special flag that
> allows locking it eventhough the ref has an illegal name.
>
> So no one else should be able to read or modify the ref at all.
>
> I think it is sufficient for this case to just have the semantics
> "just delete it, I don't care what it used to point to." for this
> special case  git branch -D <badrefname>
> so therefore since it is not the content of the ref but the name of
> the ref itself we have a problem with I don't think we need to read
> the old value or verify it.

It is also that prior to this change we could not access these badly named refs at all. This change tries to be careful to not open up too much as it tries to only allow git branch -D and nothing else to start working for such refs. (To avoid accidentally opening things up so that it becomes possible to start using/depending on such refs)

Junio C Hamano· Jul 22, 2014, 21:46 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH 12/12] refs.c: fix handling of badly named refs

Ronnie Sahlberg <sahlberg@google.com> writes:
Show 8 quoted lines
> I don't think we need to do that.
> That would imply that we would need to be able to also allow reading
> the content of a badly named ref.
> Currently a badly named ref can not be accessed by any function except
>  git branch -D <badlynamedref> which contains the special flag that
> allows locking it eventhough the ref has an illegal name.
>
> So no one else should be able to read or modify the ref at all.
OK.
Show 6 quoted lines
> I think it is sufficient for this case to just have the semantics
> "just delete it, I don't care what it used to point to." for this
> special case  git branch -D <badrefname>
> so therefore since it is not the content of the ref but the name of
> the ref itself we have a problem with I don't think we need to read
> the old value or verify it.

← back to recent threads