threads / patch / 36563

v6, 42 partsrefs.c: constify the sha arguments for ref_transaction_create|delete|update

Subject: [PATCH v6 01/42] refs.c: constify the sha arguments for ref_transaction_create|delete|update

## tl;dr

78 messages between May 1, 2014 and May 15, 2014. Diffs are folded; open one to read it.

replies: 77people: 4as markdown or json

Ronnie Sahlberg· May 1, 2014, 20:37 UTC · lore

[PATCH v6 00/42] Use ref transactions for all ref updates

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

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

Version 6:
 - Convert all updates in refs.c to use transactions too.
Version 5:
 - Reword commit messages for having _create/_delete/_update returning
   success/failure. There are no conditions yet that return an error from
   these failures but there will be in the future. So we still check the
   return from these functions in the callers in preparation for this.
 - Don't leak memory by just passing a strbuf_detach() pointer to functions.
   Use <obj>.buf and explicitely strbuf_release the data afterwards.
 - Remove the function update_ref_lock.
 - Remove the function update_ref_write.
 - Track transaction status and die(BUG:) if we call _create/_delete/_update/
   _commit for a transaction that is not OPEN.
Version 4:
 - Rename patch series from "Use ref transactions from most callers" to
   "Use ref transactions for all ref updates".
 - Convert all external ref writes to use transactions and make write_ref_sha1
   and lock_ref_sha1 static functions.
 - Change the ref commit and free handling so we no longer pass pointer to
   pointer to _commit. _commit no longer frees the transaction. The caller
   MUST call _free itself.
 - Change _commit to take a strbuf pointer instead of a char* for error
   reporting back to the caller.
 - Re-add the walker patch after fixing it.
Version 3:
 - Remove the walker patch for now. Walker needs more complex solution
   so defer it until the basics are done.
 - Remove the onerr argument to ref_transaction_commit(). All callers
   that need to die() on error now have to do this explicitely.
 - Pass an error string from ref_transaction_commit() back to the callers
   so that they can craft a nice error message upon failures.
 - Make ref_transaction_rollback() accept NULL as argument.
 - Change ref_transaction_commit() to take a pointer to pointer argument for
   the transaction and have it clear the callers pointer to NULL when
   invoked. This allows for much nicer handling of transaction rollback on
   failure.
Version 2:
 - Add a patch to ref_transaction_commit to make it honor onerr even if the
   error triggered in ref_Transaction_commit itself rather than in a call
   to other functions (that already honor onerr).
 - Add a patch to make the update_ref() helper function use transactions
   internally.
 - Change ref_transaction_update to die() instead of error() if we pass
   if a NULL old_sha1 but have have_old == true.
 - Change ref_transaction_create to die() instead of error() if new_sha1
   is false but we pass it a null_sha1.
 - Change ref_transaction_delete die() instead of error() if we pass
   if a NULL old_sha1 but have have_old == true.
 - Change several places to do  if(!transaction || ref_transaction_update()
   || ref_Transaction_commit()) die(generic-message) instead of checking each
   step separately and having a different message for each failure.
   Most users are likely not interested in what step of the transaction
   failed and only whether it failed or not.
 - Change commit.c to only pass a pointer to ref_transaction_update
   iff current_head is non-NULL.
   The previous patch used to compute a garbage pointer for
   current_head->object.sha1 and relied on the fact that ref_transaction_update
   would not try to dereference this pointer if !!current_head was 0.
 - Updated commit message for the walker_fetch change to try to justify why
   the change in locking semantics should not be harmful.
Ronnie Sahlberg (42):
  refs.c: constify the sha arguments for
    ref_transaction_create|delete|update
  refs.c: allow passing NULL to ref_transaction_free
  refs.c: add a strbuf argument to ref_transaction_commit for error
    logging
  refs.c: make ref_update_reject_duplicates take a strbuf argument for
    errors
  update-ref.c: log transaction error from the update_ref
  refs.c: make update_ref_write update a strbuf on failure
  refs.c: remove the onerr argument to ref_transaction_commit
  refs.c: change ref_transaction_update() to do error checking and
    return status
  refs.c: change ref_transaction_create to do error checking and return
    status
  refs.c: ref_transaction_delete to check for error and return status
  tag.c: use ref transactions when doing updates
  replace.c: use the ref transaction functions for updates
  commit.c: use ref transactions for updates
  sequencer.c: use ref transactions for all ref updates
  fast-import.c: change update_branch to use ref transactions
  branch.c: use ref transaction for all ref updates
  refs.c: change update_ref to use a transaction
  refs.c: free the transaction before returning when number of updates
    is 0
  refs.c: ref_transaction_commit should not free the transaction
  fetch.c: clear errno before calling functions that might set it
  fetch.c: change s_update_ref to use a ref transaction
  fetch.c: use a single ref transaction for all ref updates
  receive-pack.c: use a reference transaction for updating the refs
  fast-import.c: use a ref transaction when dumping tags
  walker.c: use ref transaction for ref updates
  refs.c: make write_ref_sha1 static
  refs.c: make lock_ref_sha1 static
  refs.c: add transaction.status and track OPEN/CLOSED/ERROR
  refs.c: remove the update_ref_lock function
  refs.c: remove the update_ref_write function
  refs.c: remove lock_ref_sha1
  refs.c: make prune_ref use a transaction to delete the ref
  refs.c: make delete_ref use a transaction
  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: pack all refs before we start to rename a ref
  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: add a new flag for transaction delete for refs we know are
    packed only
  refs.c: pass a skip list to name_conflict_fn
  refs.c: make rename_ref use a transaction
  refs.c: remove forward declaraion of write_ref_sha1
 branch.c               |  31 ++--
 builtin/commit.c       |  24 ++-
 builtin/fetch.c        |  29 ++--
 builtin/receive-pack.c |  20 +--
 builtin/replace.c      |  15 +-
 builtin/tag.c          |  15 +-
 builtin/update-ref.c   |  32 ++--
 fast-import.c          |  39 +++--
 refs.c                 | 404 +++++++++++++++++++++++++++----------------------
 refs.h                 |  49 +++---
 sequencer.c            |  24 ++-
 t/t3200-branch.sh      |   2 +-
 walker.c               |  51 ++++---
 13 files changed, 414 insertions(+), 321 deletions(-)
-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

ref_transaction_create|delete|update has no need to modify the sha1 arguments passed to it so it should use const unsigned char* instead of unsigned char*.

Some functions, such as fast_forward_to(), already have its old/new sha1 arguments as consts. This function will at some point need to use ref_transaction_update() in which case this change is required.

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

refs.c, refs.h

diff --git a/refs.c b/refs.c
index 728a761..138ab70 100644
--- a/refs.c
+++ b/refs.c
@@ -3329,7 +3329,8 @@ static struct ref_update *add_update(struct ref_transaction *transaction,
 
 void ref_transaction_update(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *new_sha1, unsigned char *old_sha1,
+			    const unsigned char *new_sha1,
+			    const unsigned char *old_sha1,
 			    int flags, int have_old)
 {
 	struct ref_update *update = add_update(transaction, refname);
@@ -3343,7 +3344,7 @@ void ref_transaction_update(struct ref_transaction *transaction,
 
 void ref_transaction_create(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *new_sha1,
+			    const unsigned char *new_sha1,
 			    int flags)
 {
 	struct ref_update *update = add_update(transaction, refname);
@@ -3357,7 +3358,7 @@ void ref_transaction_create(struct ref_transaction *transaction,
 
 void ref_transaction_delete(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *old_sha1,
+			    const unsigned char *old_sha1,
 			    int flags, int have_old)
 {
 	struct ref_update *update = add_update(transaction, refname);
diff --git a/refs.h b/refs.h
index 0f08def..892c5b6 100644
--- a/refs.h
+++ b/refs.h
@@ -239,7 +239,8 @@ void ref_transaction_rollback(struct ref_transaction *transaction);
  */
 void ref_transaction_update(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *new_sha1, unsigned char *old_sha1,
+			    const unsigned char *new_sha1,
+			    const unsigned char *old_sha1,
 			    int flags, int have_old);
 
 /*
@@ -250,7 +251,7 @@ void ref_transaction_update(struct ref_transaction *transaction,
  */
 void ref_transaction_create(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *new_sha1,
+			    const unsigned char *new_sha1,
 			    int flags);
 
 /*
@@ -260,7 +261,7 @@ void ref_transaction_create(struct ref_transaction *transaction,
  */
 void ref_transaction_delete(struct ref_transaction *transaction,
 			    const char *refname,
-			    unsigned char *old_sha1,
+			    const unsigned char *old_sha1,
 			    int flags, int have_old);
 
 /*

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 02/42] refs.c: allow passing NULL to ref_transaction_free

Allow ref_transaction_free to be called with NULL and in extension allow ref_transaction_rollback to be called for a NULL transaction.

This allows us to write code that will
  if ( (!transaction ||
        ref_transaction_update(...))  ||
      (ref_transaction_commit(...) && !(transaction = NULL)) {
          ref_transaction_rollback(transaction);
          ...
  }

In this case transaction is reset to NULL IFF ref_transaction_commit() was invoked and thus the rollback becomes ref_transaction_rollback(NULL) which is safe. IF the conditional triggered prior to ref_transaction_commit() then transaction is untouched and then ref_transaction_rollback(transaction) will rollback the failed transaction.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to refs.c +3 −0
diff --git a/refs.c b/refs.c
index 138ab70..2d83ef6 100644
--- a/refs.c
+++ b/refs.c
@@ -3303,6 +3303,9 @@ static void ref_transaction_free(struct ref_transaction *transaction)
 {
 	int i;
 
+	if (!transaction)
+		return;
+
 	for (i = 0; i < transaction->nr; i++)
 		free(transaction->updates[i]);
 

-- 
2.0.0.rc1.351.g4d2c8e4
Jonathan Nieder· May 13, 2014, 22:44 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 02/42] refs.c: allow passing NULL to ref_transaction_free

Ronnie Sahlberg wrote:
> Allow ref_transaction_free to be called with NULL and in extension allow
> ref_transaction_rollback to be called for a NULL transaction.
In extension = as a result?
Makes sense.  It lets someone do the usual
	struct ref_transaction *transaction;
	int ret = 0;
	if (something_fails()) {
		ret = -1;
		goto cleanup;
	}
	...
 cleanup:
	ref_transaction_free(transaction);
	return ret;
just like you can already do with free().
Show 8 quoted lines
> This allows us to write code that will
>
>   if ( (!transaction ||
>         ref_transaction_update(...))  ||
>       (ref_transaction_commit(...) && !(transaction = NULL)) {
>           ref_transaction_rollback(transaction);
>           ...
>   }
Somewhere in the whitespace and parentheses I'm lost.

Is the idea that when ref_transaction_commit fails it will have freed the transaction so we need not to roll back to prevent a double free? I think it would be simpler for the caller to unconditionally set transaction to NULL after calling ref_transaction_commit in such a case to avoid use-after-free.

Even better if it is the caller's responsibility to free the transaction. At any rate, it doesn't seem related to this patch.

Show 8 quoted lines
> --- a/refs.c
> +++ b/refs.c
> @@ -3303,6 +3303,9 @@ static void ref_transaction_free(struct ref_transaction *transaction)
>  {
>  	int i;
>  
> +	if (!transaction)
> +		return;
Except for the unclear commit message,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Ronnie Sahlberg· May 13, 2014, 22:52 UTC · re: Jonathan Nieder · lore

Re: [PATCH v6 02/42] refs.c: allow passing NULL to ref_transaction_free

On Tue, May 13, 2014 at 3:44 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 38 quoted lines
> Ronnie Sahlberg wrote:
>
>> Allow ref_transaction_free to be called with NULL and in extension allow
>> ref_transaction_rollback to be called for a NULL transaction.
>
> In extension = as a result?
>
> Makes sense.  It lets someone do the usual
>
>         struct ref_transaction *transaction;
>         int ret = 0;
>
>         if (something_fails()) {
>                 ret = -1;
>                 goto cleanup;
>         }
>         ...
>
>  cleanup:
>         ref_transaction_free(transaction);
>         return ret;
>
> just like you can already do with free().
>
>> This allows us to write code that will
>>
>>   if ( (!transaction ||
>>         ref_transaction_update(...))  ||
>>       (ref_transaction_commit(...) && !(transaction = NULL)) {
>>           ref_transaction_rollback(transaction);
>>           ...
>>   }
>
> Somewhere in the whitespace and parentheses I'm lost.
>
> Is the idea that when ref_transaction_commit fails it will have
> freed the transaction so we need not to roll back to prevent a
> double free?

Yes. But also, this horribleness is also to illustrate a weak point in the API in that ref_transaction_commit actually frees the transaction if successful, so the && !(transaction = NULL) kludge is to avoid a double free in the ref_transaction_rollback.

This is horrible, but all this is going away later in the patch series when _commit is fixed so that it does not free the transaction anymore. When that patch comes in later in this series, this horribleness will go away.

 I think it would be simpler for the caller to
> unconditionally set transaction to NULL after calling
> ref_transaction_commit in such a case to avoid use-after-free.

Yes. Later patches does that by having ref_transaction_commit not free the transaction and instead requiring the caller to explicitely free it by calling ref_transaction_free.

Maybe see this as this is how ugly rollback is by the current _commit semantics. Then see how beautiful it all gets once _commit is repaired and the && !(transaction = NULL) kludge is removed. :-)

Show 16 quoted lines
>
> Even better if it is the caller's responsibility to free
> the transaction.  At any rate, it doesn't seem related to this
> patch.
>
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3303,6 +3303,9 @@ static void ref_transaction_free(struct ref_transaction *transaction)
>>  {
>>       int i;
>>
>> +     if (!transaction)
>> +             return;
>
> Except for the unclear commit message,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Ronnie Sahlberg· May 14, 2014, 15:14 UTC · re: Jonathan Nieder · lore

Re: [PATCH v6 02/42] refs.c: allow passing NULL to ref_transaction_free

Thanks. I changed the commit message.

On Tue, May 13, 2014 at 3:44 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 56 quoted lines
> Ronnie Sahlberg wrote:
>
>> Allow ref_transaction_free to be called with NULL and in extension allow
>> ref_transaction_rollback to be called for a NULL transaction.
>
> In extension = as a result?
>
> Makes sense.  It lets someone do the usual
>
>         struct ref_transaction *transaction;
>         int ret = 0;
>
>         if (something_fails()) {
>                 ret = -1;
>                 goto cleanup;
>         }
>         ...
>
>  cleanup:
>         ref_transaction_free(transaction);
>         return ret;
>
> just like you can already do with free().
>
>> This allows us to write code that will
>>
>>   if ( (!transaction ||
>>         ref_transaction_update(...))  ||
>>       (ref_transaction_commit(...) && !(transaction = NULL)) {
>>           ref_transaction_rollback(transaction);
>>           ...
>>   }
>
> Somewhere in the whitespace and parentheses I'm lost.
>
> Is the idea that when ref_transaction_commit fails it will have
> freed the transaction so we need not to roll back to prevent a
> double free?  I think it would be simpler for the caller to
> unconditionally set transaction to NULL after calling
> ref_transaction_commit in such a case to avoid use-after-free.
>
> Even better if it is the caller's responsibility to free
> the transaction.  At any rate, it doesn't seem related to this
> patch.
>
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3303,6 +3303,9 @@ static void ref_transaction_free(struct ref_transaction *transaction)
>>  {
>>       int i;
>>
>> +     if (!transaction)
>> +             return;
>
> Except for the unclear commit message,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 03/42] refs.c: add a strbuf argument to ref_transaction_commit for error logging

Add a strbuf argument to _commit so that we can pass an error string back to the caller. So that we can do error logging from the caller instead of from _commit.

Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR and craft any log messages from the callers themselves and finally remove the onerr argument completely.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/update-ref.c | 2 +-
 refs.c               | 6 +++++-
 refs.h               | 5 ++++-
 3 files changed, 10 insertions(+), 3 deletions(-)
Show changes to 3 files +10 −3

builtin/update-ref.c, refs.c, refs.h

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 405267f..aaa06aa 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -367,7 +367,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		if (end_null)
 			line_termination = '\0';
 		update_refs_stdin();
-		ret = ref_transaction_commit(transaction, msg,
+		ret = ref_transaction_commit(transaction, msg, NULL,
 					     UPDATE_REFS_DIE_ON_ERR);
 		return ret;
 	}
diff --git a/refs.c b/refs.c
index 2d83ef6..64e8feb 100644
--- a/refs.c
+++ b/refs.c
@@ -3414,7 +3414,8 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 }
 
 int ref_transaction_commit(struct ref_transaction *transaction,
-			   const char *msg, enum action_on_err onerr)
+			   const char *msg, struct strbuf *err,
+			   enum action_on_err onerr)
 {
 	int ret = 0, delnum = 0, i;
 	const char **delnames;
@@ -3443,6 +3444,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 					       update->flags,
 					       &update->type, onerr);
 		if (!update->lock) {
+			if (err)
+				strbuf_addf(err ,"Cannot lock the ref '%s'.",
+					    update->refname);
 			ret = 1;
 			goto cleanup;
 		}
diff --git a/refs.h b/refs.h
index 892c5b6..ff87e14 100644
--- a/refs.h
+++ b/refs.h
@@ -268,9 +268,12 @@ void 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.  The ref_transaction is freed by this function.
+ * If err is non-NULL we will add an error string to it to explain why
+ * the transaction failed.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
-			   const char *msg, enum action_on_err onerr);
+			   const char *msg, struct strbuf *err,
+			   enum action_on_err onerr);
 
 /** Lock a ref and then write its file */
 int update_ref(const char *action, const char *refname,

-- 
2.0.0.rc1.351.g4d2c8e4
Jonathan Nieder· May 13, 2014, 23:10 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 03/42] refs.c: add a strbuf argument to ref_transaction_commit for error logging

Ronnie Sahlberg wrote:
Show 7 quoted lines
> Add a strbuf argument to _commit so that we can pass an error string back to
> the caller. So that we can do error logging from the caller instead of from
> _commit.
>
> Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR
> and craft any log messages from the callers themselves and finally remove the
> onerr argument completely.
Very nice.
[...]
> +++ b/refs.c
[...]
Show 7 quoted lines
> @@ -3443,6 +3444,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  					       update->flags,
>  					       &update->type, onerr);
>  		if (!update->lock) {
> +			if (err)
> +				strbuf_addf(err ,"Cannot lock the ref '%s'.",
> +					    update->refname);
Tiny nit: whitespace.
[...]
Show 8 quoted lines
> --- a/refs.h
> +++ b/refs.h
> @@ -268,9 +268,12 @@ void 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.  The ref_transaction is freed by this function.
> + * If err is non-NULL we will add an error string to it to explain why
> + * the transaction failed.

Probably worth mentioning the error string doesn't end with a newline so the caller knows how to use it.

With the whitespace fix and with or without the comment tweak,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Show changes to diff +3 −3
diff --git i/refs.c w/refs.c
index 64e8feb..2ca3169 100644
--- i/refs.c
+++ w/refs.c
@@ -3445,7 +3445,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 					       &update->type, onerr);
 		if (!update->lock) {
 			if (err)
-				strbuf_addf(err ,"Cannot lock the ref '%s'.",
+				strbuf_addf(err, "Cannot lock the ref '%s'.",
 					    update->refname);
 			ret = 1;
 			goto cleanup;
diff --git i/refs.h w/refs.h
index ff87e14..d45212f 100644
--- i/refs.h
+++ w/refs.h
@@ -268,8 +268,8 @@ void 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.  The ref_transaction is freed by this function.
- * If err is non-NULL we will add an error string to it to explain why
- * the transaction failed.
+ * If err is non-NULL we will append an error string (with no trailing
+ * newline) to it to explain why the transaction failed.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   const char *msg, struct strbuf *err,
Ronnie Sahlberg· May 14, 2014, 15:20 UTC · re: Jonathan Nieder · lore

Re: [PATCH v6 03/42] refs.c: add a strbuf argument to ref_transaction_commit for error logging

Thanks. Comment added and whitespace fixed.

On Tue, May 13, 2014 at 4:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 69 quoted lines
> Ronnie Sahlberg wrote:
>
>> Add a strbuf argument to _commit so that we can pass an error string back to
>> the caller. So that we can do error logging from the caller instead of from
>> _commit.
>>
>> Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR
>> and craft any log messages from the callers themselves and finally remove the
>> onerr argument completely.
>
> Very nice.
>
> [...]
>> +++ b/refs.c
> [...]
>> @@ -3443,6 +3444,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>>                                              update->flags,
>>                                              &update->type, onerr);
>>               if (!update->lock) {
>> +                     if (err)
>> +                             strbuf_addf(err ,"Cannot lock the ref '%s'.",
>> +                                         update->refname);
>
> Tiny nit: whitespace.
>
> [...]
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -268,9 +268,12 @@ void 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.  The ref_transaction is freed by this function.
>> + * If err is non-NULL we will add an error string to it to explain why
>> + * the transaction failed.
>
> Probably worth mentioning the error string doesn't end with a newline
> so the caller knows how to use it.
>
> With the whitespace fix and with or without the comment tweak,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> diff --git i/refs.c w/refs.c
> index 64e8feb..2ca3169 100644
> --- i/refs.c
> +++ w/refs.c
> @@ -3445,7 +3445,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>                                                &update->type, onerr);
>                 if (!update->lock) {
>                         if (err)
> -                               strbuf_addf(err ,"Cannot lock the ref '%s'.",
> +                               strbuf_addf(err, "Cannot lock the ref '%s'.",
>                                             update->refname);
>                         ret = 1;
>                         goto cleanup;
> diff --git i/refs.h w/refs.h
> index ff87e14..d45212f 100644
> --- i/refs.h
> +++ w/refs.h
> @@ -268,8 +268,8 @@ void 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.  The ref_transaction is freed by this function.
> - * If err is non-NULL we will add an error string to it to explain why
> - * the transaction failed.
> + * If err is non-NULL we will append an error string (with no trailing
> + * newline) to it to explain why the transaction failed.
>   */
>  int ref_transaction_commit(struct ref_transaction *transaction,
>                            const char *msg, struct strbuf *err,
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 04/42] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors

Make ref_update_reject_duplicates return any error that occurs through a new strbuf argument.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
Show changes to refs.c +5 −1
diff --git a/refs.c b/refs.c
index 64e8feb..21a147b 100644
--- a/refs.c
+++ b/refs.c
@@ -3393,6 +3393,7 @@ static int ref_update_compare(const void *r1, const void *r2)
 }
 
 static int ref_update_reject_duplicates(struct ref_update **updates, int n,
+					struct strbuf *err,
 					enum action_on_err onerr)
 {
 	int i;
@@ -3400,6 +3401,9 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 		if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {
 			const char *str =
 				"Multiple updates for ref '%s' not allowed.";
+			if (err)
+				strbuf_addf(err, str, updates[i]->refname);
+
 			switch (onerr) {
 			case UPDATE_REFS_MSG_ON_ERR:
 				error(str, updates[i]->refname); break;
@@ -3430,7 +3434,7 @@ 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, onerr);
+	ret = ref_update_reject_duplicates(updates, n, err, onerr);
 	if (ret)
 		goto cleanup;
 

-- 
2.0.0.rc1.351.g4d2c8e4
Jonathan Nieder· May 14, 2014, 00:04 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 04/42] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors

Ronnie Sahlberg wrote:
> Make ref_update_reject_duplicates return any error that occurs through a
> new strbuf argument.

Sensible. The caller-visible effect would be that now ref_transaction_commit() can pass back a helpful error message through its "err" argument when asked to make multiple updates for the same ref.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Ronnie Sahlberg· May 14, 2014, 15:24 UTC · re: Jonathan Nieder · lore

Re: [PATCH v6 04/42] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors

Thanks.

I updated the commit message to highlight that this means the error string can now be passed all the way back to the caller.

On Tue, May 13, 2014 at 5:04 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 11 quoted lines
> Ronnie Sahlberg wrote:
>
>> Make ref_update_reject_duplicates return any error that occurs through a
>> new strbuf argument.
>
> Sensible.  The caller-visible effect would be that now
> ref_transaction_commit() can pass back a helpful error message through
> its "err" argument when asked to make multiple updates for the same
> ref.
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 05/42] update-ref.c: log transaction error from the update_ref

Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is returned to print a log message if/after the transaction fails.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/update-ref.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to builtin/update-ref.c +5 −5
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index aaa06aa..207e24d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	const char *refname, *oldval, *msg = NULL;
 	unsigned char sha1[20], oldsha1[20];
 	int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
+	struct strbuf err = STRBUF_INIT;
 	struct option options[] = {
 		OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the update")),
 		OPT_BOOL('d', NULL, &delete, N_("delete the reference")),
@@ -359,17 +360,16 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		die("Refusing to perform update with empty message.");
 
 	if (read_stdin) {
-		int ret;
 		transaction = ref_transaction_begin();
-
 		if (delete || no_deref || argc > 0)
 			usage_with_options(git_update_ref_usage, options);
 		if (end_null)
 			line_termination = '\0';
 		update_refs_stdin();
-		ret = ref_transaction_commit(transaction, msg, NULL,
-					     UPDATE_REFS_DIE_ON_ERR);
-		return ret;
+		if (ref_transaction_commit(transaction, msg, &err,
+					   UPDATE_REFS_QUIET_ON_ERR))
+			die("%s", err.buf);
+		return 0;
 	}
 
 	if (end_null)

-- 
2.0.0.rc1.351.g4d2c8e4
Jonathan Nieder· May 14, 2014, 22:08 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 05/42] update-ref.c: log transaction error from the update_ref

Hi,
Ronnie Sahlberg wrote:
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
[...]
> @@ -359,17 +360,16 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
[...]
Show 11 quoted lines
>  		if (delete || no_deref || argc > 0)
>  			usage_with_options(git_update_ref_usage, options);
>  		if (end_null)
>  			line_termination = '\0';
>  		update_refs_stdin();
> -		ret = ref_transaction_commit(transaction, msg, NULL,
> -					     UPDATE_REFS_DIE_ON_ERR);
> -		return ret;
> +		if (ref_transaction_commit(transaction, msg, &err,
> +					   UPDATE_REFS_QUIET_ON_ERR))
> +			die("%s", err.buf);

Nice. I like this much more than passing a flag to each function to tell it how to handle errors. :)

ref_transaction_commit didn't have any stray codepaths that return some other exit code instead of die()-ing with UPDATE_REFS_DIE_ON_ERR, so this should be safe as far as the exit code is concerned.

The only danger would be that some codepath leaves 'err' alone and forgets to write a messages, so we die with

	"fatal: "
Alas, it looks like this patch can do that.
 i. The call to update_ref_write can error out without updating the
    error string.
 ii. delete_ref_loose can print a message and then fail without updating
     the error string so the output looks like
	warning: unable to unlink .git/refs/heads/master.lock: Permission denied
	fatal:
	$
 iii. repack_without_refs can similarly return an error
	error: Unable to create '/home/jrn/test/.git/packed-refs.lock: Permission denied
	error: cannot delete 'refs/heads/master' from packed refs
	fatal:
	$
 iv. commit_lock_file in commit_packed_refs is silent on error.
     repack_without_refs probably intends to write a message in that
     case but doesn't :(

I wish there were some way to automatically detect missed spots or make them stand out (like with the current "return error()" idiom a bare "return -1" stands out).

(i) is fixed by a later patch. It would be better to put that before this one for bisectability.

I don't see fixes to (ii), (iii), and (iv) in the series yet from a quick glance.

Thanks, Jonathan

Ronnie Sahlberg· May 15, 2014, 15:47 UTC · re: Jonathan Nieder · lore

Re: [PATCH v6 05/42] update-ref.c: log transaction error from the update_ref

On Wed, May 14, 2014 at 3:08 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 38 quoted lines
> Hi,
>
> Ronnie Sahlberg wrote:
>
>> --- a/builtin/update-ref.c
>> +++ b/builtin/update-ref.c
>> @@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
> [...]
>> @@ -359,17 +360,16 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
> [...]
>>               if (delete || no_deref || argc > 0)
>>                       usage_with_options(git_update_ref_usage, options);
>>               if (end_null)
>>                       line_termination = '\0';
>>               update_refs_stdin();
>> -             ret = ref_transaction_commit(transaction, msg, NULL,
>> -                                          UPDATE_REFS_DIE_ON_ERR);
>> -             return ret;
>> +             if (ref_transaction_commit(transaction, msg, &err,
>> +                                        UPDATE_REFS_QUIET_ON_ERR))
>> +                     die("%s", err.buf);
>
> Nice.  I like this much more than passing a flag to each function to
> tell it how to handle errors. :)
>
> ref_transaction_commit didn't have any stray codepaths that return
> some other exit code instead of die()-ing with UPDATE_REFS_DIE_ON_ERR,
> so this should be safe as far as the exit code is concerned.
>
> The only danger would be that some codepath leaves 'err' alone and
> forgets to write a messages, so we die with
>
>         "fatal: "
>
> Alas, it looks like this patch can do that.
>
>  i. The call to update_ref_write can error out without updating the
>     error string.

Fixed. I reordered the patches so the change to update_ref_write to take an err argument will come before the change to update-ref.c as you suggested.

Show 8 quoted lines
>
>  ii. delete_ref_loose can print a message and then fail without updating
>      the error string so the output looks like
>
>         warning: unable to unlink .git/refs/heads/master.lock: Permission denied
>         fatal:
>         $
>

Fixed. I have added a new patch before the change to update-ref.c to add err to delete_ref_loose.

Show 10 quoted lines
>  iii. repack_without_refs can similarly return an error
>
>         error: Unable to create '/home/jrn/test/.git/packed-refs.lock: Permission denied
>         error: cannot delete 'refs/heads/master' from packed refs
>         fatal:
>         $
>
>  iv. commit_lock_file in commit_packed_refs is silent on error.
>      repack_without_refs probably intends to write a message in that
>      case but doesn't :(

Fixed. I added a patch to take an err argument to repack_without_refs and update it for both conditions iii and iv.

Show 10 quoted lines
>
> I wish there were some way to automatically detect missed spots or
> make them stand out (like with the current "return error()" idiom a
> bare "return -1" stands out).
>
> (i) is fixed by a later patch.  It would be better to put that before
> this one for bisectability.
>
> I don't see fixes to (ii), (iii), and (iv) in the series yet from a
> quick glance.

Fixed in the next version of the patch series I will send out. Thanks.

Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 06/42] refs.c: make update_ref_write update a strbuf on failure

Change update_ref_write to also update an error strbuf on failure. This makes the error available to ref_transaction_commit callers if the transaction failed due to update_ref_sha1/write_ref_sha1 failures.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Show changes to refs.c +6 −3
diff --git a/refs.c b/refs.c
index 21a147b..5e06fd9 100644
--- a/refs.c
+++ b/refs.c
@@ -3253,10 +3253,13 @@ static struct ref_lock *update_ref_lock(const char *refname,
 
 static int update_ref_write(const char *action, const char *refname,
 			    const unsigned char *sha1, struct ref_lock *lock,
-			    enum action_on_err onerr)
+			    struct strbuf *err, enum action_on_err onerr)
 {
 	if (write_ref_sha1(lock, sha1, action) < 0) {
 		const char *str = "Cannot update the ref '%s'.";
+		if (err)
+			strbuf_addf(err, str, refname);
+
 		switch (onerr) {
 		case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
 		case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
@@ -3382,7 +3385,7 @@ int update_ref(const char *action, const char *refname,
 	lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
 	if (!lock)
 		return 1;
-	return update_ref_write(action, refname, sha1, lock, onerr);
+	return update_ref_write(action, refname, sha1, lock, NULL, onerr);
 }
 
 static int ref_update_compare(const void *r1, const void *r2)
@@ -3464,7 +3467,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			ret = update_ref_write(msg,
 					       update->refname,
 					       update->new_sha1,
-					       update->lock, onerr);
+					       update->lock, err, onerr);
 			update->lock = NULL; /* freed by update_ref_write */
 			if (ret)
 				goto cleanup;

-- 
2.0.0.rc1.351.g4d2c8e4
Jonathan Nieder· May 14, 2014, 23:04 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 06/42] refs.c: make update_ref_write update a strbuf on failure

Ronnie Sahlberg wrote:
> Change update_ref_write to also update an error strbuf on failure.
> This makes the error available to ref_transaction_commit callers if the
> transaction failed due to update_ref_sha1/write_ref_sha1 failures.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 07/42] refs.c: remove the onerr argument to ref_transaction_commit

Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr argument any more. Remove the onerr argument from the ref_transaction_commit signature.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/update-ref.c |  3 +--
 refs.c               | 22 +++++++---------------
 refs.h               |  3 +--
 3 files changed, 9 insertions(+), 19 deletions(-)
Show changes to 3 files +9 −19

builtin/update-ref.c, refs.c, refs.h

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 207e24d..2bef2a0 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -366,8 +366,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,
-					   UPDATE_REFS_QUIET_ON_ERR))
+		if (ref_transaction_commit(transaction, msg, &err))
 			die("%s", err.buf);
 		return 0;
 	}
diff --git a/refs.c b/refs.c
index 5e06fd9..308e13e 100644
--- a/refs.c
+++ b/refs.c
@@ -3396,8 +3396,7 @@ static int ref_update_compare(const void *r1, const void *r2)
 }
 
 static int ref_update_reject_duplicates(struct ref_update **updates, int n,
-					struct strbuf *err,
-					enum action_on_err onerr)
+					struct strbuf *err)
 {
 	int i;
 	for (i = 1; i < n; i++)
@@ -3407,22 +3406,13 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 			if (err)
 				strbuf_addf(err, str, updates[i]->refname);
 
-			switch (onerr) {
-			case UPDATE_REFS_MSG_ON_ERR:
-				error(str, updates[i]->refname); break;
-			case UPDATE_REFS_DIE_ON_ERR:
-				die(str, updates[i]->refname); break;
-			case UPDATE_REFS_QUIET_ON_ERR:
-				break;
-			}
 			return 1;
 		}
 	return 0;
 }
 
 int ref_transaction_commit(struct ref_transaction *transaction,
-			   const char *msg, struct strbuf *err,
-			   enum action_on_err onerr)
+			   const char *msg, struct strbuf *err)
 {
 	int ret = 0, delnum = 0, i;
 	const char **delnames;
@@ -3437,7 +3427,7 @@ 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, onerr);
+	ret = ref_update_reject_duplicates(updates, n, err);
 	if (ret)
 		goto cleanup;
 
@@ -3449,7 +3439,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 					       (update->have_old ?
 						update->old_sha1 : NULL),
 					       update->flags,
-					       &update->type, onerr);
+					       &update->type,
+					       UPDATE_REFS_QUIET_ON_ERR);
 		if (!update->lock) {
 			if (err)
 				strbuf_addf(err ,"Cannot lock the ref '%s'.",
@@ -3467,7 +3458,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			ret = update_ref_write(msg,
 					       update->refname,
 					       update->new_sha1,
-					       update->lock, err, onerr);
+					       update->lock, err,
+					       UPDATE_REFS_QUIET_ON_ERR);
 			update->lock = NULL; /* freed by update_ref_write */
 			if (ret)
 				goto cleanup;
diff --git a/refs.h b/refs.h
index ff87e14..bc7715e 100644
--- a/refs.h
+++ b/refs.h
@@ -272,8 +272,7 @@ void ref_transaction_delete(struct ref_transaction *transaction,
  * the transaction failed.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
-			   const char *msg, struct strbuf *err,
-			   enum action_on_err onerr);
+			   const char *msg, struct strbuf *err);
 
 /** Lock a ref and then write its file */
 int update_ref(const char *action, const char *refname,

-- 
2.0.0.rc1.351.g4d2c8e4
Jonathan Nieder· May 14, 2014, 23:06 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 07/42] refs.c: remove the onerr argument to ref_transaction_commit

Ronnie Sahlberg wrote:
> Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr
> argument any more. Remove the onerr argument from the ref_transaction_commit
> signature.
Nice, and obviously correct.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 08/42] refs.c: change ref_transaction_update() to do error checking and return status

Update ref_transaction_update() do some basic error checking and return true on error. Update all callers to check ref_transaction_update() for error. There are currently no conditions in _update that will return error but there will be in the future.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/update-ref.c | 10 ++++++----
 refs.c               |  9 +++++++--
 refs.h               | 10 +++++-----
 3 files changed, 18 insertions(+), 11 deletions(-)
Show changes to 3 files +18 −11

builtin/update-ref.c, refs.c, refs.h

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 2bef2a0..59c4d6b 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -197,8 +197,9 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)
 	if (*next != line_termination)
 		die("update %s: extra input: %s", refname, next);
 
-	ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-			       update_flags, have_old);
+	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+				   update_flags, have_old))
+		die("update %s: failed", refname);
 
 	update_flags = 0;
 	free(refname);
@@ -286,8 +287,9 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
 	if (*next != line_termination)
 		die("verify %s: extra input: %s", refname, next);
 
-	ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-			       update_flags, have_old);
+	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+				   update_flags, have_old))
+		die("failed transaction update for %s", refname);
 
 	update_flags = 0;
 	free(refname);
diff --git a/refs.c b/refs.c
index 308e13e..1a903fb 100644
--- a/refs.c
+++ b/refs.c
@@ -3333,19 +3333,24 @@ static struct ref_update *add_update(struct ref_transaction *transaction,
 	return update;
 }
 
-void ref_transaction_update(struct ref_transaction *transaction,
+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)
 {
-	struct ref_update *update = add_update(transaction, refname);
+	struct ref_update *update;
+
+	if (have_old && !old_sha1)
+		die("have_old is true but old_sha1 is NULL");
 
+	update = add_update(transaction, refname);
 	hashcpy(update->new_sha1, new_sha1);
 	update->flags = flags;
 	update->have_old = have_old;
 	if (have_old)
 		hashcpy(update->old_sha1, old_sha1);
+	return 0;
 }
 
 void ref_transaction_create(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index bc7715e..0364a3e 100644
--- a/refs.h
+++ b/refs.h
@@ -237,11 +237,11 @@ void ref_transaction_rollback(struct ref_transaction *transaction);
  * that the reference should have had before the update, or zeros if
  * it must not have existed beforehand.
  */
-void 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 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);
 
 /*
  * Add a reference creation to transaction.  new_sha1 is the value

-- 
2.0.0.rc1.351.g4d2c8e4
Michael Haggerty· May 5, 2014, 13:08 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 08/42] refs.c: change ref_transaction_update() to do error checking and return status

On 05/01/2014 10:37 PM, Ronnie Sahlberg wrote:
> Update ref_transaction_update() do some basic error checking and return
> true on error. Update all callers to check ref_transaction_update() for error.
> There are currently no conditions in _update that will return error but there
> will be in the future.

I would change s/true/non-zero/, because error return values are not just boolean values; the error values sometimes encode the type of error that occurred.

Show 56 quoted lines
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/update-ref.c | 10 ++++++----
>  refs.c               |  9 +++++++--
>  refs.h               | 10 +++++-----
>  3 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 2bef2a0..59c4d6b 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -197,8 +197,9 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)
>  	if (*next != line_termination)
>  		die("update %s: extra input: %s", refname, next);
>  
> -	ref_transaction_update(transaction, refname, new_sha1, old_sha1,
> -			       update_flags, have_old);
> +	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
> +				   update_flags, have_old))
> +		die("update %s: failed", refname);
>  
>  	update_flags = 0;
>  	free(refname);
> @@ -286,8 +287,9 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
>  	if (*next != line_termination)
>  		die("verify %s: extra input: %s", refname, next);
>  
> -	ref_transaction_update(transaction, refname, new_sha1, old_sha1,
> -			       update_flags, have_old);
> +	if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
> +				   update_flags, have_old))
> +		die("failed transaction update for %s", refname);
>  
>  	update_flags = 0;
>  	free(refname);
> diff --git a/refs.c b/refs.c
> index 308e13e..1a903fb 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3333,19 +3333,24 @@ static struct ref_update *add_update(struct ref_transaction *transaction,
>  	return update;
>  }
>  
> -void ref_transaction_update(struct ref_transaction *transaction,
> +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)
>  {
> -	struct ref_update *update = add_update(transaction, refname);
> +	struct ref_update *update;
> +
> +	if (have_old && !old_sha1)
> +		die("have_old is true but old_sha1 is NULL");
This new check is orthogonal to the rest of the patch, isn't it?
Show 19 quoted lines
>  
> +	update = add_update(transaction, refname);
>  	hashcpy(update->new_sha1, new_sha1);
>  	update->flags = flags;
>  	update->have_old = have_old;
>  	if (have_old)
>  		hashcpy(update->old_sha1, old_sha1);
> +	return 0;
>  }
>  
>  void ref_transaction_create(struct ref_transaction *transaction,
> diff --git a/refs.h b/refs.h
> index bc7715e..0364a3e 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -237,11 +237,11 @@ void ref_transaction_rollback(struct ref_transaction *transaction);
>   * that the reference should have had before the update, or zeros if
>   * it must not have existed beforehand.
>   */
Please update the docstring above to explain the return value.
Show 14 quoted lines
> -void 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 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);
>  
>  /*
>   * Add a reference creation to transaction.  new_sha1 is the value
> 
Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Ronnie Sahlberg· May 5, 2014, 23:09 UTC · re: Michael Haggerty · lore

Re: [PATCH v6 08/42] refs.c: change ref_transaction_update() to do error checking and return status

Thanks!
On Mon, May 5, 2014 at 6:08 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 9 quoted lines
> On 05/01/2014 10:37 PM, Ronnie Sahlberg wrote:
>> Update ref_transaction_update() do some basic error checking and return
>> true on error. Update all callers to check ref_transaction_update() for error.
>> There are currently no conditions in _update that will return error but there
>> will be in the future.
>
> I would change s/true/non-zero/, because error return values are not
> just boolean values; the error values sometimes encode the type of error
> that occurred.
Done.
Show 59 quoted lines
>
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  builtin/update-ref.c | 10 ++++++----
>>  refs.c               |  9 +++++++--
>>  refs.h               | 10 +++++-----
>>  3 files changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
>> index 2bef2a0..59c4d6b 100644
>> --- a/builtin/update-ref.c
>> +++ b/builtin/update-ref.c
>> @@ -197,8 +197,9 @@ static const char *parse_cmd_update(struct strbuf *input, const char *next)
>>       if (*next != line_termination)
>>               die("update %s: extra input: %s", refname, next);
>>
>> -     ref_transaction_update(transaction, refname, new_sha1, old_sha1,
>> -                            update_flags, have_old);
>> +     if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
>> +                                update_flags, have_old))
>> +             die("update %s: failed", refname);
>>
>>       update_flags = 0;
>>       free(refname);
>> @@ -286,8 +287,9 @@ static const char *parse_cmd_verify(struct strbuf *input, const char *next)
>>       if (*next != line_termination)
>>               die("verify %s: extra input: %s", refname, next);
>>
>> -     ref_transaction_update(transaction, refname, new_sha1, old_sha1,
>> -                            update_flags, have_old);
>> +     if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
>> +                                update_flags, have_old))
>> +             die("failed transaction update for %s", refname);
>>
>>       update_flags = 0;
>>       free(refname);
>> diff --git a/refs.c b/refs.c
>> index 308e13e..1a903fb 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3333,19 +3333,24 @@ static struct ref_update *add_update(struct ref_transaction *transaction,
>>       return update;
>>  }
>>
>> -void ref_transaction_update(struct ref_transaction *transaction,
>> +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)
>>  {
>> -     struct ref_update *update = add_update(transaction, refname);
>> +     struct ref_update *update;
>> +
>> +     if (have_old && !old_sha1)
>> +             die("have_old is true but old_sha1 is NULL");
>
> This new check is orthogonal to the rest of the patch, isn't it?
Yes.

I have updated the commit message to mention that we also check and die(BUG:...) for this condition.

Show 22 quoted lines
>
>>
>> +     update = add_update(transaction, refname);
>>       hashcpy(update->new_sha1, new_sha1);
>>       update->flags = flags;
>>       update->have_old = have_old;
>>       if (have_old)
>>               hashcpy(update->old_sha1, old_sha1);
>> +     return 0;
>>  }
>>
>>  void ref_transaction_create(struct ref_transaction *transaction,
>> diff --git a/refs.h b/refs.h
>> index bc7715e..0364a3e 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -237,11 +237,11 @@ void ref_transaction_rollback(struct ref_transaction *transaction);
>>   * that the reference should have had before the update, or zeros if
>>   * it must not have existed beforehand.
>>   */
>
> Please update the docstring above to explain the return value.
Done.
Show 22 quoted lines
>
>> -void 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 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);
>>
>>  /*
>>   * Add a reference creation to transaction.  new_sha1 is the value
>>
>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/
Jonathan Nieder· May 14, 2014, 23:40 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 08/42] refs.c: change ref_transaction_update() to do error checking and return status

Ronnie Sahlberg wrote:
Show 11 quoted lines
> Update ref_transaction_update() do some basic error checking and return
> true on error. Update all callers to check ref_transaction_update() for error.
> There are currently no conditions in _update that will return error but there
> will be in the future.
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/update-ref.c | 10 ++++++----
>  refs.c               |  9 +++++++--
>  refs.h               | 10 +++++-----
>  3 files changed, 18 insertions(+), 11 deletions(-)
Revisiting comments from [1]:
 * When I call ref_transaction_update, what does it mean that I get
   a nonzero return value?  Does it mean the _update failed and had
   no effect?  What will I want to do next: should I try again or
   print an error and exit?
   Ideally I should be able to answer these questions by reading
   the signature of ref_transaction_update and the comment documenting
   it.  The comment doesn't say anything about what errors
   mean here.
 * the error message change for the have_old && !old_sha1 case (to add
   "BUG:" so users know the impossible has happened and translators
   know not to bother with it) seems to have snuck ahead into patch 28
   (refs.c: add transaction.status and track OPEN/CLOSED/ERROR).
 * It would be easier to make sense of the error path (does the error
   message have enough information?  Will the user be bewildered?)
   if there were an example of how ref_transaction_update can fail.
   There still doesn't seem to be one by the end of the series.
The general idea still seems sensible.

Thanks, Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/246437/focus=247115
Ronnie Sahlberg· May 15, 2014, 16:06 UTC · re: Jonathan Nieder · lore

Re: [PATCH v6 08/42] refs.c: change ref_transaction_update() to do error checking and return status

On Wed, May 14, 2014 at 4:40 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 20 quoted lines
> Ronnie Sahlberg wrote:
>
>> Update ref_transaction_update() do some basic error checking and return
>> true on error. Update all callers to check ref_transaction_update() for error.
>> There are currently no conditions in _update that will return error but there
>> will be in the future.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  builtin/update-ref.c | 10 ++++++----
>>  refs.c               |  9 +++++++--
>>  refs.h               | 10 +++++-----
>>  3 files changed, 18 insertions(+), 11 deletions(-)
>
> Revisiting comments from [1]:
>
>  * When I call ref_transaction_update, what does it mean that I get
>    a nonzero return value?  Does it mean the _update failed and had
>    no effect?  What will I want to do next: should I try again or
>    print an error and exit?

It means the transaction will no longer work and must be rolled back. See below for the updated text I added to refs.h

Show 5 quoted lines
>
>    Ideally I should be able to answer these questions by reading
>    the signature of ref_transaction_update and the comment documenting
>    it.  The comment doesn't say anything about what errors
>    mean here.
I have updated the description to include :
 * Function returns 0 on success and non-zero on failure. A failure to update
 * means that the transaction as a whole has failed and will need to be
 * rolled back.
Show 5 quoted lines
>
>  * the error message change for the have_old && !old_sha1 case (to add
>    "BUG:" so users know the impossible has happened and translators
>    know not to bother with it) seems to have snuck ahead into patch 28
>    (refs.c: add transaction.status and track OPEN/CLOSED/ERROR).
Done.
Show 6 quoted lines
>
>  * It would be easier to make sense of the error path (does the error
>    message have enough information?  Will the user be bewildered?)
>    if there were an example of how ref_transaction_update can fail.
>
>    There still doesn't seem to be one by the end of the series.

This patch series got a lot longer than I initially thought so I did not get to the point where we it would make sense to start returning !0. :-(

The next patchseries I sent out for review does add things to _update that will cause it to return failures. For example, locking the ref there happens in _update instead of _commit and then it starts make sense to return failures back to the caller for things such as "Multiple updates for ref '%s' not allowed."

Unfortunate but since this patch series reached >40 patches I did not want to continue expanding on it. This means that actually starting to use "let _update return error" did not actually start becomming used until the second patch series, which now is well over 30 patches in size :-(

I just felt I had to stop growing this series or it would never be finished.
Show 7 quoted lines
>
> The general idea still seems sensible.
>
> Thanks,
> Jonathan
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/246437/focus=247115
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 09/42] refs.c: change ref_transaction_create to do error checking and return status

Do basic error checking in ref_transaction_create() and make it return status. Update all callers to check the result of ref_transaction_create() There are currently no conditions in _create that will return error but there will be in the future.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/update-ref.c |  4 +++-
 refs.c               | 17 +++++++++++------
 refs.h               |  8 ++++----
 3 files changed, 18 insertions(+), 11 deletions(-)
Show changes to 3 files +18 −11

builtin/update-ref.c, refs.c, refs.h

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 59c4d6b..3fab810 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -225,7 +225,9 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)
 	if (*next != line_termination)
 		die("create %s: extra input: %s", refname, next);
 
-	ref_transaction_create(transaction, refname, new_sha1, update_flags);
+	if (ref_transaction_create(transaction, refname, new_sha1,
+				   update_flags))
+		die("failed transaction create for %s", refname);
 
 	update_flags = 0;
 	free(refname);
diff --git a/refs.c b/refs.c
index 1a903fb..27db737 100644
--- a/refs.c
+++ b/refs.c
@@ -3353,18 +3353,23 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	return 0;
 }
 
-void ref_transaction_create(struct ref_transaction *transaction,
-			    const char *refname,
-			    const unsigned char *new_sha1,
-			    int flags)
+int ref_transaction_create(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *new_sha1,
+			   int flags)
 {
-	struct ref_update *update = add_update(transaction, refname);
+	struct ref_update *update;
+
+	if (!new_sha1 || is_null_sha1(new_sha1))
+		die("create ref with null new_sha1");
+
+	update = add_update(transaction, refname);
 
-	assert(!is_null_sha1(new_sha1));
 	hashcpy(update->new_sha1, new_sha1);
 	hashclr(update->old_sha1);
 	update->flags = flags;
 	update->have_old = 1;
+	return 0;
 }
 
 void ref_transaction_delete(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 0364a3e..01d773c 100644
--- a/refs.h
+++ b/refs.h
@@ -249,10 +249,10 @@ int ref_transaction_update(struct ref_transaction *transaction,
  * null SHA-1.  It is verified that the reference does not exist
  * already.
  */
-void ref_transaction_create(struct ref_transaction *transaction,
-			    const char *refname,
-			    const unsigned char *new_sha1,
-			    int flags);
+int ref_transaction_create(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *new_sha1,
+			   int flags);
 
 /*
  * Add a reference deletion to transaction.  If have_old is true, then

-- 
2.0.0.rc1.351.g4d2c8e4
Jonathan Nieder· May 15, 2014, 00:04 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 09/42] refs.c: change ref_transaction_create to do error checking and return status

Ronnie Sahlberg wrote:
> Do basic error checking in ref_transaction_create() and make it return
> status. Update all callers to check the result of ref_transaction_create()
> There are currently no conditions in _create that will return error but there
> will be in the future.
Same concerns as with _update:
[...]
Show 9 quoted lines
> +++ b/builtin/update-ref.c
> @@ -225,7 +225,9 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)
>  	if (*next != line_termination)
>  		die("create %s: extra input: %s", refname, next);
>  
> -	ref_transaction_create(transaction, refname, new_sha1, update_flags);
> +	if (ref_transaction_create(transaction, refname, new_sha1,
> +				   update_flags))
> +		die("failed transaction create for %s", refname);
If it were ever triggered, the message
	error: some bad thing
	fatal: failed transaction create for refs/heads/master
looks overly verbose and unclear.  Something like
	fatal: cannot create ref refs/heads/master: some bad thing
might work better.  It's hard to tell without an example in mind.
[...]
> @@ -3353,18 +3353,23 @@ void ref_transaction_create(struct ref_transaction *transaction,
[...]
> -	assert(!is_null_sha1(new_sha1));
> +	if (!new_sha1 || is_null_sha1(new_sha1))
> +		die("create ref with null new_sha1");
One less 'assert' is nice. :)

As with _update, the message should start with "BUG:" to make it clear to users and translators that this should never happen, even with malformed user input. That gets corrected in patch 28 but it's clearer to include it from the start.

Ronnie Sahlberg· May 15, 2014, 16:23 UTC · re: Jonathan Nieder · lore

Re: [PATCH v6 09/42] refs.c: change ref_transaction_create to do error checking and return status

On Wed, May 14, 2014 at 5:04 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 8 quoted lines
> Ronnie Sahlberg wrote:
>
>> Do basic error checking in ref_transaction_create() and make it return
>> status. Update all callers to check the result of ref_transaction_create()
>> There are currently no conditions in _create that will return error but there
>> will be in the future.
>
> Same concerns as with _update:
Done.
Show 20 quoted lines
>
> [...]
>> +++ b/builtin/update-ref.c
>> @@ -225,7 +225,9 @@ static const char *parse_cmd_create(struct strbuf *input, const char *next)
>>       if (*next != line_termination)
>>               die("create %s: extra input: %s", refname, next);
>>
>> -     ref_transaction_create(transaction, refname, new_sha1, update_flags);
>> +     if (ref_transaction_create(transaction, refname, new_sha1,
>> +                                update_flags))
>> +             die("failed transaction create for %s", refname);
>
> If it were ever triggered, the message
>
>         error: some bad thing
>         fatal: failed transaction create for refs/heads/master
>
> looks overly verbose and unclear.  Something like
>
>         fatal: cannot create ref refs/heads/master: some bad thing
I changed it to :
   die("cannot create ref '%s'", refname);
But it would still mean you would have
         error: some bad thing
         fatal: cannot create 'refs/heads/master'

To make it better we have to wait until the end of the second patch series, ref-transactions-next where we will have an err argument to _update/_create/_delete too and thus we can do this from update-ref.c :

   if (transaction_create_sha1(transaction, refname, new_sha1,
       update_flags, msg, &err))
   die("%s", err.buf);
Show 16 quoted lines
>
> might work better.  It's hard to tell without an example in mind.
>
> [...]
>> @@ -3353,18 +3353,23 @@ void ref_transaction_create(struct ref_transaction *transaction,
> [...]
>> -     assert(!is_null_sha1(new_sha1));
>> +     if (!new_sha1 || is_null_sha1(new_sha1))
>> +             die("create ref with null new_sha1");
>
> One less 'assert' is nice. :)
>
> As with _update, the message should start with "BUG:" to make it clear
> to users and translators that this should never happen, even with
> malformed user input.  That gets corrected in patch 28 but it's
> clearer to include it from the start.
Jonathan Nieder· May 15, 2014, 16:56 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 09/42] refs.c: change ref_transaction_create to do error checking and return status

Ronnie Sahlberg wrote:
> On Wed, May 14, 2014 at 5:04 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 24 quoted lines
>> If it were ever triggered, the message
>>
>>         error: some bad thing
>>         fatal: failed transaction create for refs/heads/master
>>
>> looks overly verbose and unclear.  Something like
>>
>>         fatal: cannot create ref refs/heads/master: some bad thing
>
> I changed it to :
>    die("cannot create ref '%s'", refname);
>
> But it would still mean you would have
>          error: some bad thing
>          fatal: cannot create 'refs/heads/master'
>
> To make it better we have to wait until the end of the second patch
> series, ref-transactions-next
> where we will have an err argument to _update/_create/_delete too and
> thus we can do this from update-ref.c :
>
>    if (transaction_create_sha1(transaction, refname, new_sha1,
>        update_flags, msg, &err))
>    die("%s", err.buf);
Thanks.  Sounds good.
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 10/42] refs.c: ref_transaction_delete to check for error and return status

Change ref_transaction_delete() to do basic error checking and return status. Update all callers to check the return for ref_transaction_delete() There are currently no conditions in _delete that will return error but there will be in the future.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/update-ref.c |  5 +++--
 refs.c               | 15 ++++++++++-----
 refs.h               |  8 ++++----
 3 files changed, 17 insertions(+), 11 deletions(-)
Show changes to 3 files +17 −11

builtin/update-ref.c, refs.c, refs.h

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 3fab810..fc3512f 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -257,8 +257,9 @@ static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 	if (*next != line_termination)
 		die("delete %s: extra input: %s", refname, next);
 
-	ref_transaction_delete(transaction, refname, old_sha1,
-			       update_flags, have_old);
+	if (ref_transaction_delete(transaction, refname, old_sha1,
+				   update_flags, have_old))
+		die("failed transaction delete for %s", refname);
 
 	update_flags = 0;
 	free(refname);
diff --git a/refs.c b/refs.c
index 27db737..0a4e28e 100644
--- a/refs.c
+++ b/refs.c
@@ -3372,19 +3372,24 @@ int ref_transaction_create(struct ref_transaction *transaction,
 	return 0;
 }
 
-void ref_transaction_delete(struct ref_transaction *transaction,
-			    const char *refname,
-			    const unsigned char *old_sha1,
-			    int flags, int have_old)
+int ref_transaction_delete(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *old_sha1,
+			   int flags, int have_old)
 {
-	struct ref_update *update = add_update(transaction, refname);
+	struct ref_update *update;
 
+	if (have_old && !old_sha1)
+		die("have_old is true but old_sha1 is NULL");
+
+	update = add_update(transaction, refname);
 	update->flags = flags;
 	update->have_old = have_old;
 	if (have_old) {
 		assert(!is_null_sha1(old_sha1));
 		hashcpy(update->old_sha1, old_sha1);
 	}
+	return 0;
 }
 
 int update_ref(const char *action, const char *refname,
diff --git a/refs.h b/refs.h
index 01d773c..5eb500e 100644
--- a/refs.h
+++ b/refs.h
@@ -259,10 +259,10 @@ int ref_transaction_create(struct ref_transaction *transaction,
  * old_sha1 holds the value that the reference should have had before
  * the update (which must not be the null SHA-1).
  */
-void ref_transaction_delete(struct ref_transaction *transaction,
-			    const char *refname,
-			    const unsigned char *old_sha1,
-			    int flags, int have_old);
+int ref_transaction_delete(struct ref_transaction *transaction,
+			   const char *refname,
+			   const unsigned char *old_sha1,
+			   int flags, int have_old);
 
 /*
  * Commit all of the changes that have been queued in transaction, as

-- 
2.0.0.rc1.351.g4d2c8e4
Jonathan Nieder· May 15, 2014, 00:19 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 10/42] refs.c: ref_transaction_delete to check for error and return status

Ronnie Sahlberg wrote:
Show 6 quoted lines
> Change ref_transaction_delete() to do basic error checking and return
> status. Update all callers to check the return for ref_transaction_delete()
> There are currently no conditions in _delete that will return error but there
> will be in the future.
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Same comments as _delete and _update. :)
Ronnie Sahlberg· May 15, 2014, 16:26 UTC · re: Jonathan Nieder · lore

Re: [PATCH v6 10/42] refs.c: ref_transaction_delete to check for error and return status

On Wed, May 14, 2014 at 5:19 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 10 quoted lines
> Ronnie Sahlberg wrote:
>
>> Change ref_transaction_delete() to do basic error checking and return
>> status. Update all callers to check the return for ref_transaction_delete()
>> There are currently no conditions in _delete that will return error but there
>> will be in the future.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>
> Same comments as _delete and _update. :)

Done. Thanks.

Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 11/42] tag.c: use ref transactions when doing updates

Change tag.c to use ref transactions for all ref updates.
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/tag.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)
Show changes to builtin/tag.c +8 −6
diff --git a/builtin/tag.c b/builtin/tag.c
index f344002..1cf5650 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -548,7 +548,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct strbuf ref = STRBUF_INIT;
 	unsigned char object[20], prev[20];
 	const char *object_ref, *tag;
-	struct ref_lock *lock;
 	struct create_tag_options opt;
 	char *cleanup_arg = NULL;
 	int annotate = 0, force = 0, lines = -1;
@@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
 	struct commit_list *with_commit = NULL;
+	struct ref_transaction *transaction;
+	struct strbuf err = STRBUF_INIT;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
@@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (annotate)
 		create_tag(object, tag, &buf, &opt, prev, object);
 
-	lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
-	if (!lock)
-		die(_("%s: cannot lock the ref"), ref.buf);
-	if (write_ref_sha1(lock, object, NULL) < 0)
-		die(_("%s: cannot update the ref"), ref.buf);
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_update(transaction, ref.buf, object, prev,
+				   0, !is_null_sha1(prev)) ||
+	    ref_transaction_commit(transaction, NULL, &err))
+		die(_("%s: cannot update the ref: %s"), ref.buf, err.buf);
 	if (force && !is_null_sha1(prev) && hashcmp(prev, object))
 		printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));
 

-- 
2.0.0.rc1.351.g4d2c8e4
Jonathan Nieder· May 15, 2014, 00:27 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 11/42] tag.c: use ref transactions when doing updates

Ronnie Sahlberg wrote:
Show 17 quoted lines
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	if (annotate)
>  		create_tag(object, tag, &buf, &opt, prev, object);
>  
> -	lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
> -	if (!lock)
> -		die(_("%s: cannot lock the ref"), ref.buf);
> -	if (write_ref_sha1(lock, object, NULL) < 0)
> -		die(_("%s: cannot update the ref"), ref.buf);
> +	transaction = ref_transaction_begin();
> +	if (!transaction ||
> +	    ref_transaction_update(transaction, ref.buf, object, prev,
> +				   0, !is_null_sha1(prev)) ||
> +	    ref_transaction_commit(transaction, NULL, &err))
> +		die(_("%s: cannot update the ref: %s"), ref.buf, err.buf);

Makes sense for the _update and _commit case. (BTW, why is have_old a separate boolean instead of a bit in flags?)

For the _begin() case, can ref_transaction_begin() ever fail? xcalloc die()s on allocation failure. So I think it's fine to assume transaction is non-null (i.e., drop the !transaction condition), or if you want to be defensive, then label it as a bug --- e.g.:

	if (!transaction)
		die("BUG: ref_transaction_begin() returned NULL?");

Otherwise if ref_transaction_begin regresses in the future and this case is tripped then the message would be

	fatal: refs/tags/v1.0: cannot update the ref:

which is not as obvious an indicator that the user should contact the mailing list.

Thanks, Jonathan

Ronnie Sahlberg· May 15, 2014, 16:45 UTC · re: Jonathan Nieder · lore

Re: [PATCH v6 11/42] tag.c: use ref transactions when doing updates

On Wed, May 14, 2014 at 5:27 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 38 quoted lines
> Ronnie Sahlberg wrote:
>
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>       if (annotate)
>>               create_tag(object, tag, &buf, &opt, prev, object);
>>
>> -     lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
>> -     if (!lock)
>> -             die(_("%s: cannot lock the ref"), ref.buf);
>> -     if (write_ref_sha1(lock, object, NULL) < 0)
>> -             die(_("%s: cannot update the ref"), ref.buf);
>> +     transaction = ref_transaction_begin();
>> +     if (!transaction ||
>> +         ref_transaction_update(transaction, ref.buf, object, prev,
>> +                                0, !is_null_sha1(prev)) ||
>> +         ref_transaction_commit(transaction, NULL, &err))
>> +             die(_("%s: cannot update the ref: %s"), ref.buf, err.buf);
>
> Makes sense for the _update and _commit case.  (BTW, why is have_old
> a separate boolean instead of a bit in flags?)
>
> For the _begin() case, can ref_transaction_begin() ever fail?  xcalloc
> die()s on allocation failure.  So I think it's fine to assume
> transaction is non-null (i.e., drop the !transaction condition), or if
> you want to be defensive, then label it as a bug --- e.g.:
>
>         if (!transaction)
>                 die("BUG: ref_transaction_begin() returned NULL?");
>
> Otherwise if ref_transaction_begin regresses in the future and this
> case is tripped then the message would be
>
>         fatal: refs/tags/v1.0: cannot update the ref:
>
> which is not as obvious an indicator that the user should contact
> the mailing list.

For the current refs implementation, _begin can never return NULL since the only failure mode would be OOM in which case we die(). And then for that case we could remove the !transaction check since transaction can never be NULL here.

(I am not a big fan of die() in general)

However, if we implement a different datastore for refs in the future it is likely that the ref_transaction_begin equivalent for that backend could well start returning failures for a lot other reasons than just OOM.

I could imagine that tdb_transaction_start() could fail for a corrupted database. An SQL based backend could fail due to the client library failing to open a socket to the db, etc.

But you bring a good point about the error message.

Instead of the suggestions above, would you accept an alternative approach where I would add an err argument to ref_transaction_begin() instead?

For a hypothetical mysql backend, this could then do something like :
Show 6 quoted lines
>> +     transaction = ref_transaction_begin();
>> +     if (!transaction ||
>> +         ref_transaction_update(transaction, ref.buf, object, prev,
>> +                                0, !is_null_sha1(prev)) ||
>> +         ref_transaction_commit(transaction, NULL, &err))
>> +             die(_("%s: cannot update the ref: %s"), ref.buf, err.buf);
Which could then result in output like

fatal: refs/heads/master: cannot update the ref: failed to connect to mysql database ...

So I suggest that instead of doing these changes I will add an err argument to ref_transaction_begin. Does that sound ok with you?

>
> Thanks,
> Jonathan
Jonathan Nieder· May 15, 2014, 16:53 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 11/42] tag.c: use ref transactions when doing updates

Ronnie Sahlberg wrote:
Show 5 quoted lines
> Instead of the suggestions above, would you accept an alternative
> approach where I would
> add an err argument to ref_transaction_begin() instead?
>
> For a hypothetical mysql backend, this could then do something like :
[...]
> fatal: refs/heads/master: cannot update the ref: failed to connect to mysql database ...
Yes, sounds like a good thing to do.

Thanks, Jonathan

Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 12/42] replace.c: use the ref transaction functions for updates

Update replace.c to use ref transactions for updates.
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/replace.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)
Show changes to builtin/replace.c +8 −6
diff --git a/builtin/replace.c b/builtin/replace.c
index b62420a..b037b29 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -129,7 +129,8 @@ static int replace_object(const char *object_ref, const char *replace_ref,
 	unsigned char object[20], prev[20], repl[20];
 	enum object_type obj_type, repl_type;
 	char ref[PATH_MAX];
-	struct ref_lock *lock;
+	struct ref_transaction *transaction;
+	struct strbuf err = STRBUF_INIT;
 
 	if (get_sha1(object_ref, object))
 		die("Failed to resolve '%s' as a valid ref.", object_ref);
@@ -157,11 +158,12 @@ static int replace_object(const char *object_ref, const char *replace_ref,
 	else if (!force)
 		die("replace ref '%s' already exists", ref);
 
-	lock = lock_any_ref_for_update(ref, prev, 0, NULL);
-	if (!lock)
-		die("%s: cannot lock the ref", ref);
-	if (write_ref_sha1(lock, repl, NULL) < 0)
-		die("%s: cannot update the ref", ref);
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_update(transaction, ref, repl, prev,
+				   0, !is_null_sha1(prev)) ||
+	    ref_transaction_commit(transaction, NULL, &err))
+		die(_("%s: failed to replace ref: %s"), ref, err.buf);
 
 	return 0;
 }

-- 
2.0.0.rc1.351.g4d2c8e4
Jonathan Nieder· May 15, 2014, 00:30 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 12/42] replace.c: use the ref transaction functions for updates

Ronnie Sahlberg wrote:
[...]
> +++ b/builtin/replace.c
[...]
Show 15 quoted lines
> @@ -157,11 +158,12 @@ static int replace_object(const char *object_ref, const char *replace_ref,
>  	else if (!force)
>  		die("replace ref '%s' already exists", ref);
>  
> -	lock = lock_any_ref_for_update(ref, prev, 0, NULL);
> -	if (!lock)
> -		die("%s: cannot lock the ref", ref);
> -	if (write_ref_sha1(lock, repl, NULL) < 0)
> -		die("%s: cannot update the ref", ref);
> +	transaction = ref_transaction_begin();
> +	if (!transaction ||
> +	    ref_transaction_update(transaction, ref, repl, prev,
> +				   0, !is_null_sha1(prev)) ||
> +	    ref_transaction_commit(transaction, NULL, &err))
> +		die(_("%s: failed to replace ref: %s"), ref, err.buf);
Same question about the !transaction case.

This makes the message translated, which is a nice change but not mentioned in the commit message. (Generally speaking, I don't mind either way about adding or not adding _() to new messages in files that have not already undergone a pass of marking everything for translation.)

Ronnie Sahlberg· May 15, 2014, 16:50 UTC · re: Jonathan Nieder · lore

Re: [PATCH v6 12/42] replace.c: use the ref transaction functions for updates

On Wed, May 14, 2014 at 5:30 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 28 quoted lines
> Ronnie Sahlberg wrote:
>
> [...]
>> +++ b/builtin/replace.c
> [...]
>> @@ -157,11 +158,12 @@ static int replace_object(const char *object_ref, const char *replace_ref,
>>       else if (!force)
>>               die("replace ref '%s' already exists", ref);
>>
>> -     lock = lock_any_ref_for_update(ref, prev, 0, NULL);
>> -     if (!lock)
>> -             die("%s: cannot lock the ref", ref);
>> -     if (write_ref_sha1(lock, repl, NULL) < 0)
>> -             die("%s: cannot update the ref", ref);
>> +     transaction = ref_transaction_begin();
>> +     if (!transaction ||
>> +         ref_transaction_update(transaction, ref, repl, prev,
>> +                                0, !is_null_sha1(prev)) ||
>> +         ref_transaction_commit(transaction, NULL, &err))
>> +             die(_("%s: failed to replace ref: %s"), ref, err.buf);
>
> Same question about the !transaction case.
>
> This makes the message translated, which is a nice change but not
> mentioned in the commit message.  (Generally speaking, I don't mind
> either way about adding or not adding _() to new messages in files
> that have not already undergone a pass of marking everything for
> translation.)

Removed the _. This series is long enough as is so lets not start worrying about translations too.

Same opinion about the ref_transaction_begin() case as before. I think it will be better to just add err to it since it is likely this will be useful for future non-file based ref backends.

Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 13/42] commit.c: use ref transactions for updates

Change commit.c to use ref transactions for all ref updates. Make sure we pass a NULL pointer to ref_transaction_update if have_old is false.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/commit.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)
Show changes to builtin/commit.c +10 −13
diff --git a/builtin/commit.c b/builtin/commit.c
index f0b7906..6117123 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1541,11 +1541,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	const char *index_file, *reflog_msg;
 	char *nl;
 	unsigned char sha1[20];
-	struct ref_lock *ref_lock;
 	struct commit_list *parents = NULL, **pptr = &parents;
 	struct stat statbuf;
 	struct commit *current_head = NULL;
 	struct commit_extra_header *extra = NULL;
+	struct ref_transaction *transaction;
+	struct strbuf err = STRBUF_INIT;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_commit_usage, builtin_commit_options);
@@ -1667,16 +1668,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	strbuf_release(&author_ident);
 	free_commit_extra_headers(extra);
 
-	ref_lock = lock_any_ref_for_update("HEAD",
-					   !current_head
-					   ? NULL
-					   : current_head->object.sha1,
-					   0, NULL);
-	if (!ref_lock) {
-		rollback_index_files();
-		die(_("cannot lock HEAD ref"));
-	}
-
 	nl = strchr(sb.buf, '\n');
 	if (nl)
 		strbuf_setlen(&sb, nl + 1 - sb.buf);
@@ -1685,9 +1676,15 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
 	strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
 
-	if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_update(transaction, "HEAD", sha1,
+				   current_head ?
+				   current_head->object.sha1 : NULL,
+				   0, !!current_head) ||
+	    ref_transaction_commit(transaction, sb.buf, &err)) {
 		rollback_index_files();
-		die(_("cannot update HEAD ref"));
+		die(_("HEAD: cannot update ref: %s"), err.buf);
 	}
 
 	unlink(git_path("CHERRY_PICK_HEAD"));

-- 
2.0.0.rc1.351.g4d2c8e4
Jonathan Nieder· May 15, 2014, 01:11 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 13/42] commit.c: use ref transactions for updates

Ronnie Sahlberg wrote:
[...]
> +++ b/builtin/commit.c
> @@ -1541,11 +1541,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
[...]
Show 33 quoted lines
> @@ -1667,16 +1668,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	strbuf_release(&author_ident);
>  	free_commit_extra_headers(extra);
>  
> -	ref_lock = lock_any_ref_for_update("HEAD",
> -					   !current_head
> -					   ? NULL
> -					   : current_head->object.sha1,
> -					   0, NULL);
> -	if (!ref_lock) {
> -		rollback_index_files();
> -		die(_("cannot lock HEAD ref"));
> -	}
> -
>  	nl = strchr(sb.buf, '\n');
>  	if (nl)
>  		strbuf_setlen(&sb, nl + 1 - sb.buf);
> 	else
> 		strbuf_addch(&sb, '\n');
>  	strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
>  	strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
>  
> -	if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
> +	transaction = ref_transaction_begin();
> +	if (!transaction ||
> +	    ref_transaction_update(transaction, "HEAD", sha1,
> +				   current_head ?
> +				   current_head->object.sha1 : NULL,
> +				   0, !!current_head) ||
> +	    ref_transaction_commit(transaction, sb.buf, &err)) {
>  		rollback_index_files();
> -		die(_("cannot update HEAD ref"));
> +		die(_("HEAD: cannot update ref: %s"), err.buf);

Same question about !transaction (it also applies to later patches but I won't mention it any more).

The error message changed from
	fatal: cannot lock HEAD ref
to
	fatal: HEAD: cannot update ref: Cannot lock the ref 'HEAD'.

Does the message from ref_transaction_commit always say what ref was being updated when it failed? If so, it's tempting to just use the message as-is:

	fatal: Cannot lock the ref 'HEAD'

If the caller should add to the message, it could say something about the context --- e.g.,

	fatal: cannot update HEAD with new commit: cannot lock the ref 'HEAD'
Looking at that,
	die("%s", err.buf)

seems simplest since even if "git commit" was being called in a loop, it's already clear that git was trying to lock HEAD to advance it.

Thanks, Jonathan

Ronnie Sahlberg· May 15, 2014, 16:53 UTC · re: Jonathan Nieder · lore

Re: [PATCH v6 13/42] commit.c: use ref transactions for updates

On Wed, May 14, 2014 at 6:11 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 68 quoted lines
> Ronnie Sahlberg wrote:
>
> [...]
>> +++ b/builtin/commit.c
>> @@ -1541,11 +1541,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> [...]
>> @@ -1667,16 +1668,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>       strbuf_release(&author_ident);
>>       free_commit_extra_headers(extra);
>>
>> -     ref_lock = lock_any_ref_for_update("HEAD",
>> -                                        !current_head
>> -                                        ? NULL
>> -                                        : current_head->object.sha1,
>> -                                        0, NULL);
>> -     if (!ref_lock) {
>> -             rollback_index_files();
>> -             die(_("cannot lock HEAD ref"));
>> -     }
>> -
>>       nl = strchr(sb.buf, '\n');
>>       if (nl)
>>               strbuf_setlen(&sb, nl + 1 - sb.buf);
>>       else
>>               strbuf_addch(&sb, '\n');
>>       strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
>>       strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
>>
>> -     if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
>> +     transaction = ref_transaction_begin();
>> +     if (!transaction ||
>> +         ref_transaction_update(transaction, "HEAD", sha1,
>> +                                current_head ?
>> +                                current_head->object.sha1 : NULL,
>> +                                0, !!current_head) ||
>> +         ref_transaction_commit(transaction, sb.buf, &err)) {
>>               rollback_index_files();
>> -             die(_("cannot update HEAD ref"));
>> +             die(_("HEAD: cannot update ref: %s"), err.buf);
>
> Same question about !transaction (it also applies to later patches but I
> won't mention it any more).
>
> The error message changed from
>
>         fatal: cannot lock HEAD ref
>
> to
>
>         fatal: HEAD: cannot update ref: Cannot lock the ref 'HEAD'.
>
> Does the message from ref_transaction_commit always say what ref
> was being updated when it failed?  If so, it's tempting to just use
> the message as-is:
>
>         fatal: Cannot lock the ref 'HEAD'
>
> If the caller should add to the message, it could say something about
> the context --- e.g.,
>
>         fatal: cannot update HEAD with new commit: cannot lock the ref 'HEAD'
>
> Looking at that,
>
>         die("%s", err.buf)
>
> seems simplest since even if "git commit" was being called in a loop,
> it's already clear that git was trying to lock HEAD to advance it.
Changed it to
>         die("%s", err.buf)
as you suggested.
Many thanks for the reviews so far!

regards ronnie sahlberg

Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 14/42] sequencer.c: use ref transactions for all ref updates

Change to use ref transactions for all updates to refs.
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 sequencer.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)
Show changes to sequencer.c +16 −8
diff --git a/sequencer.c b/sequencer.c
index 0a80c58..9282a12 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,23 +272,31 @@ static int error_dirty_index(struct replay_opts *opts)
 static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 			int unborn, struct replay_opts *opts)
 {
-	struct ref_lock *ref_lock;
+	struct ref_transaction *transaction;
 	struct strbuf sb = STRBUF_INIT;
-	int ret;
+	struct strbuf err = STRBUF_INIT;
 
 	read_cache();
 	if (checkout_fast_forward(from, to, 1))
 		exit(1); /* the callee should have complained already */
-	ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from,
-					   0, NULL);
-	if (!ref_lock)
-		return error(_("Failed to lock HEAD during fast_forward_to"));
 
 	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
-	ret = write_ref_sha1(ref_lock, to, sb.buf);
+
+	transaction = ref_transaction_begin();
+	if ((!transaction ||
+	    ref_transaction_update(transaction, "HEAD", to, from,
+				   0, !unborn)) ||
+	    (ref_transaction_commit(transaction, sb.buf, &err) &&
+	     !(transaction = NULL))) {
+		ref_transaction_rollback(transaction);
+		error(_("HEAD: Could not fast-forward: %s\n"), err.buf);
+		strbuf_release(&sb);
+		strbuf_release(&err);
+		return -1;
+	}
 
 	strbuf_release(&sb);
-	return ret;
+	return 0;
 }
 
 static int do_recursive_merge(struct commit *base, struct commit *next,

-- 
2.0.0.rc1.351.g4d2c8e4
Jonathan Nieder· May 15, 2014, 17:35 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 14/42] sequencer.c: use ref transactions for all ref updates

Ronnie Sahlberg wrote:
Show 27 quoted lines
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -272,23 +272,31 @@ static int error_dirty_index(struct replay_opts *opts)
>  static int fast_forward_to(const unsigned char *to, const unsigned char *from,
>  			int unborn, struct replay_opts *opts)
>  {
> -	struct ref_lock *ref_lock;
> +	struct ref_transaction *transaction;
>  	struct strbuf sb = STRBUF_INIT;
> -	int ret;
> +	struct strbuf err = STRBUF_INIT;
>  
>  	read_cache();
>  	if (checkout_fast_forward(from, to, 1))
>  		exit(1); /* the callee should have complained already */
> -	ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from,
> -					   0, NULL);
> -	if (!ref_lock)
> -		return error(_("Failed to lock HEAD during fast_forward_to"));
>  
>  	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
> -	ret = write_ref_sha1(ref_lock, to, sb.buf);
> +
> +	transaction = ref_transaction_begin();
> +	if ((!transaction ||
> +	    ref_transaction_update(transaction, "HEAD", to, from,
> +				   0, !unborn)) ||

These parentheses make it harder to parse. Other patches in this series do

	if (!transaction ||
	    ref_transaction_update(...) ||
	    ref_transaction_commit(...)) {
so this could do
	if (!transaction ||
	    ref_transaction_update(...) ||
	    (ref_transaction_commit(...) && !(transaction = NULL))) {
> +	    (ref_transaction_commit(transaction, sb.buf, &err) &&
> +	     !(transaction = NULL))) {
> +		ref_transaction_rollback(transaction);

Earlier patches in the series didn't bother rolling back. Should they have?

Thanks, Jonathan

Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 15/42] fast-import.c: change update_branch to use ref transactions

Change update_branch() to use ref transactions for updates.
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 fast-import.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)
Show changes to fast-import.c +14 −8
diff --git a/fast-import.c b/fast-import.c
index 6707a66..79d219b 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1679,39 +1679,45 @@ found_entry:
 static int update_branch(struct branch *b)
 {
 	static const char *msg = "fast-import";
-	struct ref_lock *lock;
+	struct ref_transaction *transaction;
 	unsigned char old_sha1[20];
+	struct strbuf err = STRBUF_INIT;
 
 	if (read_ref(b->name, old_sha1))
 		hashclr(old_sha1);
+
 	if (is_null_sha1(b->sha1)) {
 		if (b->delete)
 			delete_ref(b->name, old_sha1, 0);
 		return 0;
 	}
-	lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL);
-	if (!lock)
-		return error("Unable to lock %s", b->name);
 	if (!force_update && !is_null_sha1(old_sha1)) {
 		struct commit *old_cmit, *new_cmit;
 
 		old_cmit = lookup_commit_reference_gently(old_sha1, 0);
 		new_cmit = lookup_commit_reference_gently(b->sha1, 0);
 		if (!old_cmit || !new_cmit) {
-			unlock_ref(lock);
 			return error("Branch %s is missing commits.", b->name);
 		}
 
 		if (!in_merge_bases(old_cmit, new_cmit)) {
-			unlock_ref(lock);
 			warning("Not updating %s"
 				" (new tip %s does not contain %s)",
 				b->name, sha1_to_hex(b->sha1), sha1_to_hex(old_sha1));
 			return -1;
 		}
 	}
-	if (write_ref_sha1(lock, b->sha1, msg) < 0)
-		return error("Unable to update %s", b->name);
+	transaction = ref_transaction_begin();
+	if ((!transaction ||
+	    ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
+				   0, 1)) ||
+	    (ref_transaction_commit(transaction, msg, &err) &&
+	     !(transaction = NULL))) {
+		ref_transaction_rollback(transaction);
+		error("Unable to update branch %s: %s", b->name, err.buf);
+		strbuf_release(&err);
+		return -1;
+	}
 	return 0;
 }
 

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 16/42] branch.c: use ref transaction for all ref updates

Change create_branch to use a ref transaction when creating the new branch. ref_transaction_create will check that the ref does not already exist and fail otherwise meaning that we no longer need to keep a lock on the ref during the setup_tracking. This simplifies the code since we can now do the transaction in one single step.

If the forcing flag is false then use ref_transaction_create since this will fail if the ref already exist. Otherwise use ref_transaction_update.

This also fixes a race condition in the old code where two concurrent create_branch could race since the lock_any_ref_for_update/write_ref_sha1 did not protect against the ref already existsing. I.e. one thread could end up overwriting a branch even if the forcing flag is false.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 branch.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)
Show changes to branch.c +16 −14
diff --git a/branch.c b/branch.c
index 660097b..2aa5c82 100644
--- a/branch.c
+++ b/branch.c
@@ -226,7 +226,6 @@ void create_branch(const char *head,
 		   int force, int reflog, int clobber_head,
 		   int quiet, enum branch_track track)
 {
-	struct ref_lock *lock = NULL;
 	struct commit *commit;
 	unsigned char sha1[20];
 	char *real_ref, msg[PATH_MAX + 20];
@@ -285,15 +284,6 @@ void create_branch(const char *head,
 		die(_("Not a valid branch point: '%s'."), start_name);
 	hashcpy(sha1, commit->object.sha1);
 
-	if (!dont_change_ref) {
-		lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
-		if (!lock)
-			die_errno(_("Failed to lock ref for update"));
-	}
-
-	if (reflog)
-		log_all_ref_updates = 1;
-
 	if (forcing)
 		snprintf(msg, sizeof msg, "branch: Reset to %s",
 			 start_name);
@@ -301,13 +291,25 @@ void create_branch(const char *head,
 		snprintf(msg, sizeof msg, "branch: Created from %s",
 			 start_name);
 
+	if (reflog)
+		log_all_ref_updates = 1;
+
+	if (!dont_change_ref) {
+		struct ref_transaction *transaction;
+		struct strbuf err = STRBUF_INIT;
+
+		transaction = ref_transaction_begin();
+		if (!transaction ||
+			ref_transaction_update(transaction, ref.buf, sha1,
+					       null_sha1, 0, !forcing) ||
+			ref_transaction_commit(transaction, msg, &err))
+				die_errno(_("%s: failed to write ref: %s"),
+					  ref.buf, err.buf);
+	}
+
 	if (real_ref && track)
 		setup_tracking(ref.buf + 11, real_ref, track, quiet);
 
-	if (!dont_change_ref)
-		if (write_ref_sha1(lock, sha1, msg) < 0)
-			die_errno(_("Failed to write ref"));
-
 	strbuf_release(&ref);
 	free(real_ref);
 }

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 17/42] refs.c: change update_ref to use a transaction

Change the update_ref helper function to use a ref transaction internally.
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)
Show changes to refs.c +21 −4
diff --git a/refs.c b/refs.c
index 0a4e28e..563f336 100644
--- a/refs.c
+++ b/refs.c
@@ -3396,11 +3396,28 @@ int update_ref(const char *action, const char *refname,
 	       const unsigned char *sha1, const unsigned char *oldval,
 	       int flags, enum action_on_err onerr)
 {
-	struct ref_lock *lock;
-	lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
-	if (!lock)
+	struct ref_transaction *t;
+	struct strbuf err = STRBUF_INIT;
+
+	t = ref_transaction_begin();
+	if ((!t ||
+	    ref_transaction_update(t, refname, sha1, oldval, flags,
+				   !!oldval)) ||
+	    (ref_transaction_commit(t, action, &err) && !(t = NULL))) {
+		const char *str = "update_ref failed for ref '%s': %s";
+
+		ref_transaction_rollback(t);
+		switch (onerr) {
+		case UPDATE_REFS_MSG_ON_ERR:
+			error(str, refname, err.buf); break;
+		case UPDATE_REFS_DIE_ON_ERR:
+			die(str, refname, err.buf); break;
+		case UPDATE_REFS_QUIET_ON_ERR: break;
+		}
+		strbuf_release(&err);
 		return 1;
-	return update_ref_write(action, refname, sha1, lock, NULL, onerr);
+	}
+	return 0;
 }
 
 static int ref_update_compare(const void *r1, const void *r2)

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 18/42] refs.c: free the transaction before returning when number of updates is 0

We have to free the transaction before returning in the early check for
'return early if number of updates == 0' or else the following code would
create a memory leak with the transaction never being freed :
   t = ref_transaction_begin()
   ref_transaction_commit(t)
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to refs.c +3 −1
diff --git a/refs.c b/refs.c
index 563f336..c6ee43c 100644
--- a/refs.c
+++ b/refs.c
@@ -3451,8 +3451,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
 
-	if (!n)
+	if (!n) {
+		ref_transaction_free(transaction);
 		return 0;
+	}
 
 	/* Allocate work space */
 	delnames = xmalloc(sizeof(*delnames) * n);

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 19/42] refs.c: ref_transaction_commit should not free the transaction

Change ref_transaction_commit so that it does not free the transaction. Instead require that a caller will end a transaction by either calling ref_transaction_rollback or ref_transaction_free.

By having the transaction object remaining valid after _commit returns allows
us to write much nicer code and still be able to call ref_transaction_rollback
safely. Instead of this horribleness
	t = ref_transaction_begin();
	if ((!t ||
	    ref_transaction_update(t, refname, sha1, oldval, flags,
				   !!oldval)) ||
	    (ref_transaction_commit(t, action, &err) && !(t = NULL))) {
		ref_transaction_rollback(t);
we can now just do the much nicer
	t = ref_transaction_begin();
	if (!t ||
	    ref_transaction_update(t, refname, sha1, oldval, flags,
				   !!oldval) ||
	    ref_transaction_commit(&t, action, &err)) {
		ref_transaction_rollback(t);
		... die/return ...
	ref_transaction_free(transaction);
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 branch.c             |  1 +
 builtin/commit.c     |  1 +
 builtin/replace.c    |  1 +
 builtin/tag.c        |  1 +
 builtin/update-ref.c |  1 +
 fast-import.c        |  8 ++++----
 refs.c               | 14 ++++++--------
 refs.h               | 14 +++++++++-----
 sequencer.c          |  8 ++++----
 9 files changed, 28 insertions(+), 21 deletions(-)
Show changes to 9 files +28 −21

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

diff --git a/branch.c b/branch.c
index 2aa5c82..8e58908 100644
--- a/branch.c
+++ b/branch.c
@@ -305,6 +305,7 @@ void create_branch(const char *head,
 			ref_transaction_commit(transaction, msg, &err))
 				die_errno(_("%s: failed to write ref: %s"),
 					  ref.buf, err.buf);
+		ref_transaction_free(transaction);
 	}
 
 	if (real_ref && track)
diff --git a/builtin/commit.c b/builtin/commit.c
index 6117123..5bdcf9e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1686,6 +1686,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		rollback_index_files();
 		die(_("HEAD: cannot update ref: %s"), err.buf);
 	}
+	ref_transaction_free(transaction);
 
 	unlink(git_path("CHERRY_PICK_HEAD"));
 	unlink(git_path("REVERT_HEAD"));
diff --git a/builtin/replace.c b/builtin/replace.c
index b037b29..5999f7d 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -165,6 +165,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,
 	    ref_transaction_commit(transaction, NULL, &err))
 		die(_("%s: failed to replace ref: %s"), ref, err.buf);
 
+	ref_transaction_free(transaction);
 	return 0;
 }
 
diff --git a/builtin/tag.c b/builtin/tag.c
index 1cf5650..d59154a 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -708,6 +708,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 				   0, !is_null_sha1(prev)) ||
 	    ref_transaction_commit(transaction, NULL, &err))
 		die(_("%s: cannot update the ref: %s"), ref.buf, err.buf);
+	ref_transaction_free(transaction);
 	if (force && !is_null_sha1(prev) && hashcmp(prev, object))
 		printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));
 
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index fc3512f..7fe9c11 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -373,6 +373,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 		update_refs_stdin();
 		if (ref_transaction_commit(transaction, msg, &err))
 			die("%s", err.buf);
+		ref_transaction_free(transaction);
 		return 0;
 	}
 
diff --git a/fast-import.c b/fast-import.c
index 79d219b..3e356da 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1708,16 +1708,16 @@ static int update_branch(struct branch *b)
 		}
 	}
 	transaction = ref_transaction_begin();
-	if ((!transaction ||
+	if (!transaction ||
 	    ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
-				   0, 1)) ||
-	    (ref_transaction_commit(transaction, msg, &err) &&
-	     !(transaction = NULL))) {
+				   0, 1) ||
+	    ref_transaction_commit(transaction, msg, &err)) {
 		ref_transaction_rollback(transaction);
 		error("Unable to update branch %s: %s", b->name, err.buf);
 		strbuf_release(&err);
 		return -1;
 	}
+	ref_transaction_free(transaction);
 	return 0;
 }
 
diff --git a/refs.c b/refs.c
index c6ee43c..fcbdf9b 100644
--- a/refs.c
+++ b/refs.c
@@ -3302,7 +3302,7 @@ struct ref_transaction *ref_transaction_begin(void)
 	return xcalloc(1, sizeof(struct ref_transaction));
 }
 
-static void ref_transaction_free(struct ref_transaction *transaction)
+void ref_transaction_free(struct ref_transaction *transaction)
 {
 	int i;
 
@@ -3400,10 +3400,10 @@ int update_ref(const char *action, const char *refname,
 	struct strbuf err = STRBUF_INIT;
 
 	t = ref_transaction_begin();
-	if ((!t ||
+	if (!t ||
 	    ref_transaction_update(t, refname, sha1, oldval, flags,
-				   !!oldval)) ||
-	    (ref_transaction_commit(t, action, &err) && !(t = NULL))) {
+				   !!oldval) ||
+	    ref_transaction_commit(t, action, &err)) {
 		const char *str = "update_ref failed for ref '%s': %s";
 
 		ref_transaction_rollback(t);
@@ -3417,6 +3417,7 @@ int update_ref(const char *action, const char *refname,
 		strbuf_release(&err);
 		return 1;
 	}
+	ref_transaction_free(t);
 	return 0;
 }
 
@@ -3451,10 +3452,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
 
-	if (!n) {
-		ref_transaction_free(transaction);
+	if (!n)
 		return 0;
-	}
 
 	/* Allocate work space */
 	delnames = xmalloc(sizeof(*delnames) * n);
@@ -3520,7 +3519,6 @@ cleanup:
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
 	free(delnames);
-	ref_transaction_free(transaction);
 	return ret;
 }
 
diff --git a/refs.h b/refs.h
index 5eb500e..fcde43e 100644
--- a/refs.h
+++ b/refs.h
@@ -210,8 +210,8 @@ enum action_on_err {
 
 /*
  * Begin a reference transaction.  The reference transaction must
- * eventually be commited using ref_transaction_commit() or rolled
- * back using ref_transaction_rollback().
+ * eventually be freed by either calling ref_transaction_rollback()
+ * or ref_transaction_free().
  */
 struct ref_transaction *ref_transaction_begin(void);
 
@@ -267,13 +267,17 @@ 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.  The ref_transaction is freed by this function.
- * If err is non-NULL we will add an error string to it to explain why
- * the transaction failed.
+ * problem. If err is non-NULL we will add an error string to it to explain
+ * why the transaction failed.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   const char *msg, struct strbuf *err);
 
+/*
+ * Free an existing transaction.
+ */
+void ref_transaction_free(struct ref_transaction *transaction);
+
 /** Lock a ref and then write its file */
 int update_ref(const char *action, const char *refname,
 		const unsigned char *sha1, const unsigned char *oldval,
diff --git a/sequencer.c b/sequencer.c
index 9282a12..0cfdaf0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -283,17 +283,17 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
 
 	transaction = ref_transaction_begin();
-	if ((!transaction ||
+	if (!transaction ||
 	    ref_transaction_update(transaction, "HEAD", to, from,
-				   0, !unborn)) ||
-	    (ref_transaction_commit(transaction, sb.buf, &err) &&
-	     !(transaction = NULL))) {
+				   0, !unborn) ||
+	    ref_transaction_commit(transaction, sb.buf, &err)) {
 		ref_transaction_rollback(transaction);
 		error(_("HEAD: Could not fast-forward: %s\n"), err.buf);
 		strbuf_release(&sb);
 		strbuf_release(&err);
 		return -1;
 	}
+	ref_transaction_free(transaction);
 
 	strbuf_release(&sb);
 	return 0;

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 20/42] fetch.c: clear errno before calling functions that might set it

In s_update_ref there are two calls that when they fail we return an error based on the errno value. In particular we want to return a specific error if ENOTDIR happened. Both these functions do have failure modes where they may return an error without updating errno, in which case a previous and unrelated ENOTDIT may cause us to return the wrong error. Clear errno before calling any functions if we check errno afterwards.

Also skip initializing a static variable to 0. Statics live in .bss and are all automatically initialized to 0.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/fetch.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to builtin/fetch.c +3 −1
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..a93c893 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -44,7 +44,7 @@ static struct transport *gtransport;
 static struct transport *gsecondary;
 static const char *submodule_prefix = "";
 static const char *recurse_submodules_default;
-static int shown_url = 0;
+static int shown_url;
 
 static int option_parse_recurse_submodules(const struct option *opt,
 				   const char *arg, int unset)
@@ -382,6 +382,8 @@ static int s_update_ref(const char *action,
 	if (!rla)
 		rla = default_rla.buf;
 	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
+
+	errno = 0;
 	lock = lock_any_ref_for_update(ref->name,
 				       check_old ? ref->old_sha1 : NULL,
 				       0, NULL);

-- 
2.0.0.rc1.351.g4d2c8e4
Eric Sunshine· May 2, 2014, 04:11 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 20/42] fetch.c: clear errno before calling functions that might set it

On Thu, May 1, 2014 at 4:37 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
Show 5 quoted lines
> In s_update_ref there are two calls that when they fail we return an error
> based on the errno value. In particular we want to return a specific error
> if ENOTDIR happened. Both these functions do have failure modes where they
> may return an error without updating errno, in which case a previous and
> unrelated ENOTDIT may cause us to return the wrong error. Clear errno before
s/ENOTDIT/ENOTDIR/
Show 34 quoted lines
> calling any functions if we check errno afterwards.
>
> Also skip initializing a static variable to 0. Statics live in .bss and
> are all automatically initialized to 0.
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/fetch.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 55f457c..a93c893 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -44,7 +44,7 @@ static struct transport *gtransport;
>  static struct transport *gsecondary;
>  static const char *submodule_prefix = "";
>  static const char *recurse_submodules_default;
> -static int shown_url = 0;
> +static int shown_url;
>
>  static int option_parse_recurse_submodules(const struct option *opt,
>                                    const char *arg, int unset)
> @@ -382,6 +382,8 @@ static int s_update_ref(const char *action,
>         if (!rla)
>                 rla = default_rla.buf;
>         snprintf(msg, sizeof(msg), "%s: %s", rla, action);
> +
> +       errno = 0;
>         lock = lock_any_ref_for_update(ref->name,
>                                        check_old ? ref->old_sha1 : NULL,
>                                        0, NULL);
> --
> 2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 2, 2014, 14:48 UTC · re: Eric Sunshine · lore

Re: [PATCH v6 20/42] fetch.c: clear errno before calling functions that might set it

Fixed. Thanks.
On Thu, May 1, 2014 at 9:11 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 43 quoted lines
> On Thu, May 1, 2014 at 4:37 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
>> In s_update_ref there are two calls that when they fail we return an error
>> based on the errno value. In particular we want to return a specific error
>> if ENOTDIR happened. Both these functions do have failure modes where they
>> may return an error without updating errno, in which case a previous and
>> unrelated ENOTDIT may cause us to return the wrong error. Clear errno before
>
> s/ENOTDIT/ENOTDIR/
>
>> calling any functions if we check errno afterwards.
>>
>> Also skip initializing a static variable to 0. Statics live in .bss and
>> are all automatically initialized to 0.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  builtin/fetch.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 55f457c..a93c893 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -44,7 +44,7 @@ static struct transport *gtransport;
>>  static struct transport *gsecondary;
>>  static const char *submodule_prefix = "";
>>  static const char *recurse_submodules_default;
>> -static int shown_url = 0;
>> +static int shown_url;
>>
>>  static int option_parse_recurse_submodules(const struct option *opt,
>>                                    const char *arg, int unset)
>> @@ -382,6 +382,8 @@ static int s_update_ref(const char *action,
>>         if (!rla)
>>                 rla = default_rla.buf;
>>         snprintf(msg, sizeof(msg), "%s: %s", rla, action);
>> +
>> +       errno = 0;
>>         lock = lock_any_ref_for_update(ref->name,
>>                                        check_old ? ref->old_sha1 : NULL,
>>                                        0, NULL);
>> --
>> 2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 21/42] 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 | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
Show changes to builtin/fetch.c +9 −8
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a93c893..b41d7b7 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -375,7 +375,7 @@ 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;
 
 	if (dry_run)
 		return 0;
@@ -384,15 +384,16 @@ static int s_update_ref(const char *action,
 	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
 
 	errno = 0;
-	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)
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_update(transaction, ref->name, ref->new_sha1,
+				   ref->old_sha1, 0, check_old) ||
+	    ref_transaction_commit(transaction, msg, NULL)) {
+		ref_transaction_rollback(transaction);
 		return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
 					  STORE_REF_ERROR_OTHER;
+	}
+	ref_transaction_free(transaction);
 	return 0;
 }
 

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 22/42] fetch.c: use a single ref transaction for all ref updates

Change store_updated_refs to use a single ref transaction for all refs that are updated during the fetch. This makes the fetch more atomic when update failures occur.

Since ref update failures will now no longer occur in the code path for updating a single ref in s_update_ref, we no longer have as detailed error message logging the exact reference and the ref log action as in the old cod Instead since we fail the entire transaction we log a much more generic message. But since we commit the transaction using MSG_ON_ERR we will log an error containing the ref name if either locking of writing the ref would so the regression in the log message is minor.

This will also change the order in which errors are checked for and logged which may alter which error will be logged if there are multiple errors occuring during a fetch.

For example, assume we have a fetch for two refs that both would fail.
Where the first ref would fail with ENOTDIR due to a directory in the ref
path not existing, and the second ref in the fetch would fail due to
the check in update_logical_ref():
        if (current_branch &&
            !strcmp(ref->name, current_branch->name) &&
            !(update_head_ok || is_bare_repository()) &&
            !is_null_sha1(ref->old_sha1)) {
                /*
                 * If this is the head, and it's not okay to update
                 * the head, and the old value of the head isn't empty...
                 */

In the old code since we would update the refs one ref at a time we would first fail the ENOTDIR and then fail the second update of HEAD as well. But since the first ref failed with ENOTDIR we would eventually fail the who fetch with STORE_REF_ERROR_DF_CONFLICT

In the new code, since we defer committing the transaction until all refs have been processed, we would now detect that the second ref was bad and rollback the transaction before we would even try start writing the update t disk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.

I think this new behaviour is more correct, since if there was a problem we would not even try to commit the transaction but need to highlight this change in how/what errors are reported. This change in what error is returned only occurs if there are multiple refs that fail to update and only some, but not all, of them fail due to ENOTDIR.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/fetch.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)
Show changes to builtin/fetch.c +16 −18
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b41d7b7..5dbd0f0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -45,6 +45,7 @@ static struct transport *gsecondary;
 static const char *submodule_prefix = "";
 static const char *recurse_submodules_default;
 static int shown_url;
+struct ref_transaction *transaction;
 
 static int option_parse_recurse_submodules(const struct option *opt,
 				   const char *arg, int unset)
@@ -373,27 +374,13 @@ static int s_update_ref(const char *action,
 			struct ref *ref,
 			int check_old)
 {
-	char msg[1024];
-	char *rla = getenv("GIT_REFLOG_ACTION");
-	struct ref_transaction *transaction;
-
 	if (dry_run)
 		return 0;
-	if (!rla)
-		rla = default_rla.buf;
-	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
 
-	errno = 0;
-	transaction = ref_transaction_begin();
-	if (!transaction ||
-	    ref_transaction_update(transaction, ref->name, ref->new_sha1,
-				   ref->old_sha1, 0, check_old) ||
-	    ref_transaction_commit(transaction, msg, NULL)) {
-		ref_transaction_rollback(transaction);
-		return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
-					  STORE_REF_ERROR_OTHER;
-	}
-	ref_transaction_free(transaction);
+	if (ref_transaction_update(transaction, ref->name, ref->new_sha1,
+			       ref->old_sha1, 0, check_old))
+		return STORE_REF_ERROR_OTHER;
+
 	return 0;
 }
 
@@ -565,6 +552,13 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		goto abort;
 	}
 
+	errno = 0;
+	transaction = ref_transaction_begin();
+	if (!transaction) {
+		rc = error(_("cannot start ref transaction\n"));
+		goto abort;
+	}
+
 	/*
 	 * We do a pass for each fetch_head_status type in their enum order, so
 	 * merged entries are written before not-for-merge. That lets readers
@@ -676,6 +670,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 			}
 		}
 	}
+	if (ref_transaction_commit(transaction, "fetch_ref transaction", NULL))
+		rc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
+		  STORE_REF_ERROR_OTHER;
+	ref_transaction_free(transaction);
 
 	if (rc & STORE_REF_ERROR_DF_CONFLICT)
 		error(_("some local refs could not be updated; try running\n"

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 23/42] receive-pack.c: use a reference transaction for updating the refs

Wrap all the ref updates inside a transaction to make the update atomic.
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/receive-pack.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)
Show changes to builtin/receive-pack.c +10 −10
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..d580176 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -46,6 +46,8 @@ static void *head_name_to_free;
 static int sent_capabilities;
 static int shallow_update;
 static const char *alt_shallow_file;
+struct strbuf err = STRBUF_INIT;
+struct ref_transaction *transaction;
 
 static enum deny_action parse_deny_action(const char *var, const char *value)
 {
@@ -475,7 +477,6 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 	const char *namespaced_name;
 	unsigned char *old_sha1 = cmd->old_sha1;
 	unsigned char *new_sha1 = cmd->new_sha1;
-	struct ref_lock *lock;
 
 	/* only refs/... are allowed */
 	if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
@@ -580,15 +581,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 		    update_shallow_ref(cmd, si))
 			return "shallow error";
 
-		lock = lock_any_ref_for_update(namespaced_name, old_sha1,
-					       0, NULL);
-		if (!lock) {
-			rp_error("failed to lock %s", name);
-			return "failed to lock";
-		}
-		if (write_ref_sha1(lock, new_sha1, "push")) {
-			return "failed to write"; /* error() already called */
-		}
+		if (ref_transaction_update(transaction, namespaced_name,
+					   new_sha1, old_sha1, 0, 1))
+			return "failed to update";
 		return NULL; /* good */
 	}
 }
@@ -812,6 +807,7 @@ static void execute_commands(struct command *commands,
 	head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
 
 	checked_connectivity = 1;
+	transaction = ref_transaction_begin();
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (cmd->error_string)
 			continue;
@@ -827,6 +823,10 @@ static void execute_commands(struct command *commands,
 			checked_connectivity = 0;
 		}
 	}
+	if (ref_transaction_commit(transaction, "push", &err))
+		error("%s", err.buf);
+	ref_transaction_free(transaction);
+	strbuf_release(&err);
 
 	if (shallow_update && !checked_connectivity)
 		error("BUG: run 'git fsck' for safety.\n"

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 24/42] fast-import.c: use a ref transaction when dumping tags

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 fast-import.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
Show changes to fast-import.c +12 −5
diff --git a/fast-import.c b/fast-import.c
index 3e356da..5587cf6 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1736,15 +1736,22 @@ static void dump_tags(void)
 {
 	static const char *msg = "fast-import";
 	struct tag *t;
-	struct ref_lock *lock;
 	char ref_name[PATH_MAX];
+	struct strbuf err = STRBUF_INIT;
+	struct ref_transaction *transaction;
 
+	transaction = ref_transaction_begin();
 	for (t = first_tag; t; t = t->next_tag) {
-		sprintf(ref_name, "tags/%s", t->name);
-		lock = lock_ref_sha1(ref_name, NULL);
-		if (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)
-			failure |= error("Unable to update %s", ref_name);
+		sprintf(ref_name, "refs/tags/%s", t->name);
+
+		if (ref_transaction_update(transaction, ref_name, t->sha1,
+					   NULL, 0, 0))
+			failure |= error("Unable to update %s", err.buf);
 	}
+	if (failure || ref_transaction_commit(transaction, msg, &err))
+		failure |= error("Unable to update %s", err.buf);
+	ref_transaction_free(transaction);
+	strbuf_release(&err);
 }
 
 static void dump_marks_helper(FILE *f,

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 25/42] walker.c: use ref transaction for ref updates

Switch to using ref transactions in walker_fetch(). As part of the refactoring to use ref transactions we also fix a potential memory leak where in the original code if write_ref_sha1() would fail we would end up returning from the function without free()ing the msg string.

This changes the locking slightly for walker_fetch. Previously the code would lock all refs before writing them but now we do not lock the refs until the commit stage. There is thus a very short window where changes could be done locally during the fetch which would be overwritten when the fetch completes and commits its transaction. But this window should be reasonably short. Even if this race does trigger, since both the old code and the new code just overwrites the refs to the new values without checking or comparing them with the previous value, this is not too dissimilar to a similar scenario where you first do a ref change locally and then later do a fetch that overwrites the local change. With this in mind I do not see the change in locking semantics to be critical.

Note that this function is only called when fetching from a remote HTTP repository onto the local (most of the time single-user) repository which likely means that the type of collissions that the previous locking would protect against and cause the fetch to fail for to be even more rare.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 walker.c | 51 ++++++++++++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 25 deletions(-)
Show changes to walker.c +26 −25
diff --git a/walker.c b/walker.c
index 1dd86b8..6044ccf 100644
--- a/walker.c
+++ b/walker.c
@@ -251,24 +251,18 @@ void walker_targets_free(int targets, char **target, const char **write_ref)
 int walker_fetch(struct walker *walker, int targets, char **target,
 		 const char **write_ref, const char *write_ref_log_details)
 {
-	struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
+	char ref_name[PATH_MAX];
+	struct strbuf err = STRBUF_INIT;
+	struct ref_transaction *transaction;
 	unsigned char *sha1 = xmalloc(targets * 20);
-	char *msg;
-	int ret;
+	char *msg = NULL;
 	int i;
 
 	save_commit_buffer = 0;
 
-	for (i = 0; i < targets; i++) {
-		if (!write_ref || !write_ref[i])
-			continue;
-
-		lock[i] = lock_ref_sha1(write_ref[i], NULL);
-		if (!lock[i]) {
-			error("Can't lock ref %s", write_ref[i]);
-			goto unlock_and_fail;
-		}
-	}
+	transaction = ref_transaction_begin();
+	if (!transaction)
+		return -1;
 
 	if (!walker->get_recover)
 		for_each_ref(mark_complete, NULL);
@@ -276,14 +270,14 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 	for (i = 0; i < targets; i++) {
 		if (interpret_target(walker, target[i], &sha1[20 * i])) {
 			error("Could not interpret response from server '%s' as something to pull", target[i]);
-			goto unlock_and_fail;
+			goto rollback_and_fail;
 		}
 		if (process(walker, lookup_unknown_object(&sha1[20 * i])))
-			goto unlock_and_fail;
+			goto rollback_and_fail;
 	}
 
 	if (loop(walker))
-		goto unlock_and_fail;
+		goto rollback_and_fail;
 
 	if (write_ref_log_details) {
 		msg = xmalloc(strlen(write_ref_log_details) + 12);
@@ -294,19 +288,26 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 	for (i = 0; i < targets; i++) {
 		if (!write_ref || !write_ref[i])
 			continue;
-		ret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : "fetch (unknown)");
-		lock[i] = NULL;
-		if (ret)
-			goto unlock_and_fail;
+		sprintf(ref_name, "refs/%s", write_ref[i]);
+		if (ref_transaction_update(transaction, ref_name,
+					   &sha1[20 * i], NULL,
+					   0, 0))
+			goto rollback_and_fail;
+	}
+
+	if (ref_transaction_commit(transaction, msg ? msg : "fetch (unknown)",
+				   &err)) {
+		error("%s", err.buf);
+		goto rollback_and_fail;
 	}
-	free(msg);
 
+	free(msg);
 	return 0;
 
-unlock_and_fail:
-	for (i = 0; i < targets; i++)
-		if (lock[i])
-			unlock_ref(lock[i]);
+rollback_and_fail:
+	free(msg);
+	strbuf_release(&err);
+	ref_transaction_free(transaction);
 
 	return -1;
 }

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 26/42] 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 | 4 +++-
 refs.h | 3 ---
 2 files changed, 3 insertions(+), 4 deletions(-)
Show changes to 2 files +3 −4

refs.c, refs.h

diff --git a/refs.c b/refs.c
index fcbdf9b..020eadf 100644
--- a/refs.c
+++ b/refs.c
@@ -251,6 +251,8 @@ struct ref_entry {
 };
 
 static void read_loose_refs(const char *dirname, struct ref_dir *dir);
+static int write_ref_sha1(struct ref_lock *lock,
+			  const unsigned char *sha1, const char *logmsg);
 
 static struct ref_dir *get_ref_dir(struct ref_entry *entry)
 {
@@ -2798,7 +2800,7 @@ static int is_branch(const char *refname)
 	return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/");
 }
 
-int write_ref_sha1(struct ref_lock *lock,
+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 fcde43e..bc8a95b 100644
--- a/refs.h
+++ b/refs.h
@@ -150,9 +150,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. **/
 int log_ref_setup(const char *refname, char *logfile, int bufsize);
 

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 27/42] refs.c: make lock_ref_sha1 static

No external callers reference lock_ref_sha1 any more so lets declare it static.
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 2 +-
 refs.h | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)
Show changes to 2 files +1 −4

refs.c, refs.h

diff --git a/refs.c b/refs.c
index 020eadf..a82d6ba 100644
--- a/refs.c
+++ b/refs.c
@@ -2130,7 +2130,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	return NULL;
 }
 
-struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1)
+static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1)
 {
 	char refpath[PATH_MAX];
 	if (check_refname_format(refname, 0))
diff --git a/refs.h b/refs.h
index bc8a95b..b14fe86 100644
--- a/refs.h
+++ b/refs.h
@@ -132,9 +132,6 @@ extern int ref_exists(const char *);
  */
 extern int peel_ref(const char *refname, unsigned char *sha1);
 
-/** Locks a "refs/" ref returning the lock on success and NULL on failure. **/
-extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1);
-
 /** Locks any ref (for 'HEAD' type refs). */
 #define REF_NODEREF	0x01
 extern struct ref_lock *lock_any_ref_for_update(const char *refname,

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 28/42] refs.c: add transaction.status and track OPEN/CLOSED/ERROR

Track the status of a transaction in a new status field. Check the field for sanity, i.e. that status must be OPEN when _commit/_create/_delete or _update is called or else die(BUG:...)

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)
Show changes to refs.c +33 −4
diff --git a/refs.c b/refs.c
index a82d6ba..206e1a2 100644
--- a/refs.c
+++ b/refs.c
@@ -3288,6 +3288,12 @@ struct ref_update {
 	const char refname[FLEX_ARRAY];
 };
 
+enum ref_transaction_status {
+	REF_TRANSACTION_OPEN   = 0,
+	REF_TRANSACTION_CLOSED = 1,
+	REF_TRANSACTION_ERROR  = 2,
+};
+
 /*
  * Data structure for holding a reference transaction, which can
  * consist of checks and updates to multiple references, carried out
@@ -3297,6 +3303,7 @@ struct ref_transaction {
 	struct ref_update **updates;
 	size_t alloc;
 	size_t nr;
+	enum ref_transaction_status status;
 };
 
 struct ref_transaction *ref_transaction_begin(void)
@@ -3320,6 +3327,11 @@ void ref_transaction_free(struct ref_transaction *transaction)
 
 void ref_transaction_rollback(struct ref_transaction *transaction)
 {
+	if (!transaction)
+		return;
+
+	transaction->status = REF_TRANSACTION_ERROR;
+
 	ref_transaction_free(transaction);
 }
 
@@ -3344,7 +3356,10 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	struct ref_update *update;
 
 	if (have_old && !old_sha1)
-		die("have_old is true but old_sha1 is NULL");
+		die("BUG: have_old is true but old_sha1 is NULL");
+
+	if (transaction->status != REF_TRANSACTION_OPEN)
+		die("BUG: update on transaction that is not open");
 
 	update = add_update(transaction, refname);
 	hashcpy(update->new_sha1, new_sha1);
@@ -3363,7 +3378,10 @@ int ref_transaction_create(struct ref_transaction *transaction,
 	struct ref_update *update;
 
 	if (!new_sha1 || is_null_sha1(new_sha1))
-		die("create ref with null new_sha1");
+		die("BUG: create ref with null new_sha1");
+
+	if (transaction->status != REF_TRANSACTION_OPEN)
+		die("BUG: create on transaction that is not open");
 
 	update = add_update(transaction, refname);
 
@@ -3382,7 +3400,10 @@ int ref_transaction_delete(struct ref_transaction *transaction,
 	struct ref_update *update;
 
 	if (have_old && !old_sha1)
-		die("have_old is true but old_sha1 is NULL");
+		die("BUG: have_old is true but old_sha1 is NULL");
+
+	if (transaction->status != REF_TRANSACTION_OPEN)
+		die("BUG: delete on transaction that is not open");
 
 	update = add_update(transaction, refname);
 	update->flags = flags;
@@ -3454,8 +3475,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
 
-	if (!n)
+	if (transaction->status != REF_TRANSACTION_OPEN)
+		die("BUG: commit on transaction that is not open");
+
+	if (!n) {
+		transaction->status = REF_TRANSACTION_CLOSED;
 		return 0;
+	}
 
 	/* Allocate work space */
 	delnames = xmalloc(sizeof(*delnames) * n);
@@ -3517,6 +3543,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	clear_loose_ref_cache(&ref_cache);
 
 cleanup:
+	transaction->status = ret ? REF_TRANSACTION_ERROR
+	  : REF_TRANSACTION_CLOSED;
+
 	for (i = 0; i < n; i++)
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 29/42] refs.c: remove the update_ref_lock function

Since we now only call update_ref_lock with onerr==QUIET_ON_ERR we no longer need this function and can replace it with just calling lock_any_ref_for_update directly.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)
Show changes to refs.c +6 −24
diff --git a/refs.c b/refs.c
index 206e1a2..9f5435d 100644
--- a/refs.c
+++ b/refs.c
@@ -3235,24 +3235,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
 	return retval;
 }
 
-static struct ref_lock *update_ref_lock(const char *refname,
-					const unsigned char *oldval,
-					int flags, int *type_p,
-					enum action_on_err onerr)
-{
-	struct ref_lock *lock;
-	lock = lock_any_ref_for_update(refname, oldval, flags, type_p);
-	if (!lock) {
-		const char *str = "Cannot lock the ref '%s'.";
-		switch (onerr) {
-		case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
-		case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
-		case UPDATE_REFS_QUIET_ON_ERR: break;
-		}
-	}
-	return lock;
-}
-
 static int update_ref_write(const char *action, const char *refname,
 			    const unsigned char *sha1, struct ref_lock *lock,
 			    struct strbuf *err, enum action_on_err onerr)
@@ -3496,12 +3478,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
-		update->lock = update_ref_lock(update->refname,
-					       (update->have_old ?
-						update->old_sha1 : NULL),
-					       update->flags,
-					       &update->type,
-					       UPDATE_REFS_QUIET_ON_ERR);
+		update->lock = lock_any_ref_for_update(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.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 30/42] refs.c: remove the update_ref_write function

Since we only call update_ref_write from a single place and we only call it with onerr==QUIET_ON_ERR we can just as well get rid of it and just call write_ref_sha1 directly.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 35 +++++++++--------------------------
 1 file changed, 9 insertions(+), 26 deletions(-)
Show changes to refs.c +9 −26
diff --git a/refs.c b/refs.c
index 9f5435d..875178b 100644
--- a/refs.c
+++ b/refs.c
@@ -3235,25 +3235,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
 	return retval;
 }
 
-static int update_ref_write(const char *action, const char *refname,
-			    const unsigned char *sha1, struct ref_lock *lock,
-			    struct strbuf *err, enum action_on_err onerr)
-{
-	if (write_ref_sha1(lock, sha1, action) < 0) {
-		const char *str = "Cannot update the ref '%s'.";
-		if (err)
-			strbuf_addf(err, str, refname);
-
-		switch (onerr) {
-		case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
-		case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
-		case UPDATE_REFS_QUIET_ON_ERR: break;
-		}
-		return 1;
-	}
-	return 0;
-}
-
 /**
  * Information needed for a single ref update.  Set new_sha1 to the
  * new value or to zero to delete the ref.  To check the old value
@@ -3498,14 +3479,16 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		struct ref_update *update = updates[i];
 
 		if (!is_null_sha1(update->new_sha1)) {
-			ret = update_ref_write(msg,
-					       update->refname,
-					       update->new_sha1,
-					       update->lock, err,
-					       UPDATE_REFS_QUIET_ON_ERR);
-			update->lock = NULL; /* freed by update_ref_write */
-			if (ret)
+			ret = write_ref_sha1(update->lock, update->new_sha1,
+					     msg);
+			update->lock = NULL; /* freed by write_ref_sha1 */
+			if (ret) {
+				const char *str = "Cannot update the ref '%s'.";
+
+				if (err)
+					strbuf_addf(err, str, update->refname);
 				goto cleanup;
+			}
 		}
 	}
 

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 31/42] refs.c: remove lock_ref_sha1

lock_ref_sha1 was only called from one place in refc.c and only provided a check that the refname was sane before adding back the initial "refs/" part of the ref path name, the initial "refs/" that this caller had already stripped off before calling lock_ref_sha1.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
Show changes to refs.c +5 −10
diff --git a/refs.c b/refs.c
index 875178b..a55cc38 100644
--- a/refs.c
+++ b/refs.c
@@ -2130,15 +2130,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	return NULL;
 }
 
-static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1)
-{
-	char refpath[PATH_MAX];
-	if (check_refname_format(refname, 0))
-		return NULL;
-	strcpy(refpath, mkpath("refs/%s", refname));
-	return lock_ref_sha1_basic(refpath, old_sha1, 0, NULL);
-}
-
 struct ref_lock *lock_any_ref_for_update(const char *refname,
 					 const unsigned char *old_sha1,
 					 int flags, int *type_p)
@@ -2339,8 +2330,12 @@ static void try_remove_empty_parents(char *name)
 /* make sure nobody touched the ref, and unlink */
 static void prune_ref(struct ref_to_prune *r)
 {
-	struct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);
+	struct ref_lock *lock;
+
+	if (check_refname_format(r->name + 5, 0))
+		return;
 
+	lock = lock_ref_sha1_basic(r->name, r->sha1, 0, NULL);
 	if (lock) {
 		unlink_or_warn(git_path("%s", r->name));
 		unlock_ref(lock);

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 32/42] refs.c: make prune_ref use a transaction to delete the ref

Change prune_ref to delete the ref using a ref transaction. To do this we also need to add a new flag REF_ISPRUNING that will tell the transaction that we do not want to delete this ref from the packed refs.

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

refs.c, refs.h

diff --git a/refs.c b/refs.c
index a55cc38..2b82dce 100644
--- a/refs.c
+++ b/refs.c
@@ -2330,17 +2330,24 @@ static void try_remove_empty_parents(char *name)
 /* make sure nobody touched the ref, and unlink */
 static void prune_ref(struct ref_to_prune *r)
 {
-	struct ref_lock *lock;
+	struct ref_transaction *transaction;
+	struct strbuf err = STRBUF_INIT;
 
 	if (check_refname_format(r->name + 5, 0))
 		return;
 
-	lock = lock_ref_sha1_basic(r->name, r->sha1, 0, NULL);
-	if (lock) {
-		unlink_or_warn(git_path("%s", r->name));
-		unlock_ref(lock);
-		try_remove_empty_parents(r->name);
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_delete(transaction, r->name, r->sha1,
+				   REF_ISPRUNING, 1) ||
+	    ref_transaction_commit(transaction, NULL, &err)) {
+		ref_transaction_rollback(transaction);
+		warning("prune_ref: %s", err.buf);
+		strbuf_release(&err);
+		return;
 	}
+	ref_transaction_free(transaction);
+	try_remove_empty_parents(r->name);
 }
 
 static void prune_refs(struct ref_to_prune *r)
@@ -3492,8 +3499,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		struct ref_update *update = updates[i];
 
 		if (update->lock) {
-			delnames[delnum++] = update->lock->ref_name;
 			ret |= delete_ref_loose(update->lock, update->type);
+			if (!(update->flags & REF_ISPRUNING))
+				delnames[delnum++] = update->lock->ref_name;
 		}
 	}
 
diff --git a/refs.h b/refs.h
index b14fe86..340e11a 100644
--- a/refs.h
+++ b/refs.h
@@ -134,6 +134,8 @@ extern int peel_ref(const char *refname, unsigned char *sha1);
 
 /** Locks any ref (for 'HEAD' type refs). */
 #define REF_NODEREF	0x01
+/** Deleting a loose ref during prune */
+#define REF_ISPRUNING	0x02
 extern struct ref_lock *lock_any_ref_for_update(const char *refname,
 						const unsigned char *old_sha1,
 						int flags, int *type_p);

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 33/42] refs.c: make delete_ref use a transaction

Change delete_ref to use a ref transaction for the deletion. At the same time since we no longer have any callers of repack_without_ref we can now delete this function.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)
Show changes to refs.c +10 −21
diff --git a/refs.c b/refs.c
index 2b82dce..96b03e5 100644
--- a/refs.c
+++ b/refs.c
@@ -2481,11 +2481,6 @@ static int repack_without_refs(const char **refnames, int n)
 	return commit_packed_refs();
 }
 
-static int repack_without_ref(const char *refname)
-{
-	return repack_without_refs(&refname, 1);
-}
-
 static int delete_ref_loose(struct ref_lock *lock, int flag)
 {
 	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
@@ -2503,24 +2498,18 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)
 
 int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 {
-	struct ref_lock *lock;
-	int ret = 0, flag = 0;
+	struct ref_transaction *transaction;
 
-	lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);
-	if (!lock)
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_delete(transaction, refname, sha1, delopt,
+				   sha1 && !is_null_sha1(sha1)) ||
+	    ref_transaction_commit(transaction, NULL, NULL)) {
+		ref_transaction_rollback(transaction);
 		return 1;
-	ret |= delete_ref_loose(lock, flag);
-
-	/* removing the loose one could have resurrected an earlier
-	 * packed one.  Also, if it was not loose we need to repack
-	 * without it.
-	 */
-	ret |= repack_without_ref(lock->ref_name);
-
-	unlink_or_warn(git_path("logs/%s", lock->ref_name));
-	clear_loose_ref_cache(&ref_cache);
-	unlock_ref(lock);
-	return ret;
+	}
+	ref_transaction_free(transaction);
+	return 0;
 }
 
 /*

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 34/42] 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.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 branch.c               |  6 +++---
 builtin/commit.c       |  4 ++--
 builtin/fetch.c        | 10 ++++++++--
 builtin/receive-pack.c |  4 ++--
 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               |  6 +++---
 12 files changed, 60 insertions(+), 45 deletions(-)
Show changes to 12 files +60 −45

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 8e58908..74d55e7 100644
--- a/branch.c
+++ b/branch.c
@@ -300,9 +300,9 @@ void create_branch(const char *head,
 
 		transaction = ref_transaction_begin();
 		if (!transaction ||
-			ref_transaction_update(transaction, ref.buf, sha1,
-					       null_sha1, 0, !forcing) ||
-			ref_transaction_commit(transaction, msg, &err))
+		    ref_transaction_update(transaction, ref.buf, sha1,
+					   null_sha1, 0, !forcing, msg) ||
+		    ref_transaction_commit(transaction, &err))
 				die_errno(_("%s: failed to write ref: %s"),
 					  ref.buf, err.buf);
 		ref_transaction_free(transaction);
diff --git a/builtin/commit.c b/builtin/commit.c
index 5bdcf9e..0f4e1fc 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1681,8 +1681,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) ||
-	    ref_transaction_commit(transaction, sb.buf, &err)) {
+				   0, !!current_head, sb.buf) ||
+	    ref_transaction_commit(transaction, &err)) {
 		rollback_index_files();
 		die(_("HEAD: cannot update ref: %s"), err.buf);
 	}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 5dbd0f0..3a849b0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -374,11 +374,17 @@ static int s_update_ref(const char *action,
 			struct ref *ref,
 			int check_old)
 {
+	char msg[1024];
+	char *rla = getenv("GIT_REFLOG_ACTION");
+
 	if (dry_run)
 		return 0;
+	if (!rla)
+		rla = default_rla.buf;
+	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
 
 	if (ref_transaction_update(transaction, ref->name, ref->new_sha1,
-			       ref->old_sha1, 0, check_old))
+				   ref->old_sha1, 0, check_old, msg))
 		return STORE_REF_ERROR_OTHER;
 
 	return 0;
@@ -670,7 +676,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 			}
 		}
 	}
-	if (ref_transaction_commit(transaction, "fetch_ref transaction", NULL))
+	if (ref_transaction_commit(transaction, NULL))
 		rc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
 		  STORE_REF_ERROR_OTHER;
 	ref_transaction_free(transaction);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d580176..991c659 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -582,7 +582,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 			return "shallow error";
 
 		if (ref_transaction_update(transaction, namespaced_name,
-					   new_sha1, old_sha1, 0, 1))
+					   new_sha1, old_sha1, 0, 1, "push"))
 			return "failed to update";
 		return NULL; /* good */
 	}
@@ -823,7 +823,7 @@ static void execute_commands(struct command *commands,
 			checked_connectivity = 0;
 		}
 	}
-	if (ref_transaction_commit(transaction, "push", &err))
+	if (ref_transaction_commit(transaction, &err))
 		error("%s", err.buf);
 	ref_transaction_free(transaction);
 	strbuf_release(&err);
diff --git a/builtin/replace.c b/builtin/replace.c
index 5999f7d..47c360c 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -161,8 +161,8 @@ static int replace_object(const char *object_ref, const char *replace_ref,
 	transaction = ref_transaction_begin();
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref, repl, prev,
-				   0, !is_null_sha1(prev)) ||
-	    ref_transaction_commit(transaction, NULL, &err))
+				   0, !is_null_sha1(prev), NULL) ||
+	    ref_transaction_commit(transaction, &err))
 		die(_("%s: failed to replace ref: %s"), ref, err.buf);
 
 	ref_transaction_free(transaction);
diff --git a/builtin/tag.c b/builtin/tag.c
index d59154a..c039355 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();
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, object, prev,
-				   0, !is_null_sha1(prev)) ||
-	    ref_transaction_commit(transaction, NULL, &err))
+				   0, !is_null_sha1(prev), NULL) ||
+	    ref_transaction_commit(transaction, &err))
 		die(_("%s: cannot update the ref: %s"), ref.buf, 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 7fe9c11..c5aff92 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;
 
 /*
  * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
@@ -198,7 +199,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))
+				   update_flags, have_old, msg))
 		die("update %s: failed", refname);
 
 	update_flags = 0;
@@ -226,7 +227,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))
+				   update_flags, msg))
 		die("failed transaction create for %s", refname);
 
 	update_flags = 0;
@@ -258,7 +259,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))
+				   update_flags, have_old, msg))
 		die("failed transaction delete for %s", refname);
 
 	update_flags = 0;
@@ -291,7 +292,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))
+				   update_flags, have_old, msg))
 		die("failed transaction update for %s", refname);
 
 	update_flags = 0;
@@ -344,7 +345,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 strbuf err = STRBUF_INIT;
@@ -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 5587cf6..099e71b 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1710,8 +1710,8 @@ static int update_branch(struct branch *b)
 	transaction = ref_transaction_begin();
 	if (!transaction ||
 	    ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
-				   0, 1) ||
-	    ref_transaction_commit(transaction, msg, &err)) {
+				   0, 1, msg) ||
+	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_rollback(transaction);
 		error("Unable to update branch %s: %s", b->name, err.buf);
 		strbuf_release(&err);
@@ -1745,10 +1745,10 @@ static void dump_tags(void)
 		sprintf(ref_name, "refs/tags/%s", t->name);
 
 		if (ref_transaction_update(transaction, ref_name, t->sha1,
-					   NULL, 0, 0))
+					   NULL, 0, 0, msg))
 			failure |= error("Unable to update %s", err.buf);
 	}
-	if (failure || ref_transaction_commit(transaction, msg, &err))
+	if (failure || ref_transaction_commit(transaction, &err))
 		failure |= error("Unable to update %s", err.buf);
 	ref_transaction_free(transaction);
 	strbuf_release(&err);
diff --git a/refs.c b/refs.c
index 96b03e5..7e58693 100644
--- a/refs.c
+++ b/refs.c
@@ -2339,8 +2339,8 @@ static void prune_ref(struct ref_to_prune *r)
 	transaction = ref_transaction_begin();
 	if (!transaction ||
 	    ref_transaction_delete(transaction, r->name, r->sha1,
-				   REF_ISPRUNING, 1) ||
-	    ref_transaction_commit(transaction, NULL, &err)) {
+				   REF_ISPRUNING, 1, NULL) ||
+	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_rollback(transaction);
 		warning("prune_ref: %s", err.buf);
 		strbuf_release(&err);
@@ -2503,8 +2503,8 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 	transaction = ref_transaction_begin();
 	if (!transaction ||
 	    ref_transaction_delete(transaction, refname, sha1, delopt,
-				   sha1 && !is_null_sha1(sha1)) ||
-	    ref_transaction_commit(transaction, NULL, NULL)) {
+				   sha1 && !is_null_sha1(sha1), NULL) ||
+	    ref_transaction_commit(transaction, NULL)) {
 		ref_transaction_rollback(transaction);
 		return 1;
 	}
@@ -3239,6 +3239,7 @@ struct ref_update {
 	int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
 	struct ref_lock *lock;
 	int type;
+	const char *msg;
 	const char refname[FLEX_ARRAY];
 };
 
@@ -3272,9 +3273,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((char *)transaction->updates[i]->msg);
 		free(transaction->updates[i]);
-
+	}
 	free(transaction->updates);
 	free(transaction);
 }
@@ -3305,7 +3307,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 ref_update *update;
 
@@ -3321,13 +3323,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 ref_update *update;
 
@@ -3343,13 +3347,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 ref_update *update;
 
@@ -3366,6 +3372,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;
 }
 
@@ -3379,8 +3387,8 @@ int update_ref(const char *action, const char *refname,
 	t = ref_transaction_begin();
 	if (!t ||
 	    ref_transaction_update(t, refname, sha1, oldval, flags,
-				   !!oldval) ||
-	    ref_transaction_commit(t, action, &err)) {
+				   !!oldval, action) ||
+	    ref_transaction_commit(t, &err)) {
 		const char *str = "update_ref failed for ref '%s': %s";
 
 		ref_transaction_rollback(t);
@@ -3422,7 +3430,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;
@@ -3471,7 +3479,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 340e11a..7a89415 100644
--- a/refs.h
+++ b/refs.h
@@ -237,7 +237,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);
 
 /*
  * Add a reference creation to transaction.  new_sha1 is the value
@@ -248,7 +248,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);
 
 /*
  * Add a reference deletion to transaction.  If have_old is true, then
@@ -258,7 +258,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);
 
 /*
  * Commit all of the changes that have been queued in transaction, as
@@ -267,7 +267,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,
  * why the transaction failed.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
-			   const char *msg, struct strbuf *err);
+			   struct strbuf *err);
 
 /*
  * Free an existing transaction.
diff --git a/sequencer.c b/sequencer.c
index 0cfdaf0..3a0ee09 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -285,8 +285,8 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 	transaction = ref_transaction_begin();
 	if (!transaction ||
 	    ref_transaction_update(transaction, "HEAD", to, from,
-				   0, !unborn) ||
-	    ref_transaction_commit(transaction, sb.buf, &err)) {
+				   0, !unborn, sb.buf) ||
+	    ref_transaction_commit(transaction, &err)) {
 		ref_transaction_rollback(transaction);
 		error(_("HEAD: Could not fast-forward: %s\n"), err.buf);
 		strbuf_release(&sb);
diff --git a/walker.c b/walker.c
index 6044ccf..c2a1266 100644
--- a/walker.c
+++ b/walker.c
@@ -291,12 +291,12 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 		sprintf(ref_name, "refs/%s", write_ref[i]);
 		if (ref_transaction_update(transaction, ref_name,
 					   &sha1[20 * i], NULL,
-					   0, 0))
+					   0, 0,
+					   msg ? msg : "fetch (unknown)"))
 			goto rollback_and_fail;
 	}
 
-	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.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 35/42] 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.

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 7e58693..e1f919e 100644
--- a/refs.c
+++ b/refs.c
@@ -2600,7 +2600,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.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 36/42] refs.c: pack all refs before we start to rename a ref

This means that most loose refs will no longer be present after the rename which triggered a test failure since it assumes the file for an unrelated ref would still be present after the rename.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c            | 3 +++
 t/t3200-branch.sh | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)
Show changes to 2 files +4 −1

refs.c, t/t3200-branch.sh

diff --git a/refs.c b/refs.c
index e1f919e..14d1573 100644
--- a/refs.c
+++ b/refs.c
@@ -2595,6 +2595,9 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 		return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
 			oldrefname, strerror(errno));
 
+	if (pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE))
+		return error("unable to pack refs");
+
 	if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
 		error("unable to delete old %s", oldrefname);
 		goto rollback;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index ac31b71..fafd38a 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -289,7 +289,7 @@ test_expect_success 'renaming a symref is not allowed' '
 	git symbolic-ref refs/heads/master2 refs/heads/master &&
 	test_must_fail git branch -m master2 master3 &&
 	git symbolic-ref refs/heads/master2 &&
-	test_path_is_file .git/refs/heads/master &&
+	test_path_is_missing .git/refs/heads/master &&
 	test_path_is_missing .git/refs/heads/master3
 '
 

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 37/42] 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.

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.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Show changes to refs.c +3 −2
diff --git a/refs.c b/refs.c
index 14d1573..8d82742 100644
--- a/refs.c
+++ b/refs.c
@@ -2043,6 +2043,9 @@ 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))
+		return NULL;
+
 	lock = xcalloc(1, sizeof(struct ref_lock));
 	lock->lock_fd = -1;
 
@@ -2134,8 +2137,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.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 38/42] 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.

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 8d82742..51cd41e 100644
--- a/refs.c
+++ b/refs.c
@@ -3462,12 +3462,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.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 39/42] refs.c: add a new flag for transaction delete for refs we know are packed only

Add a new flag REF_ISPACKONLY that we can use in ref_transaction_delete. This flag indicates that the ref does not exist as a loose ref andf only as a packed ref. If this is the case we then change the commit code so that we skip taking out a lock file and we skip calling delete_ref_loose. Check for this flag and die(BUG:...) if used with _update or _create.

At the start of the transaction, before we even start locking any refs, we add all such REF_ISPACKONLY refs to delnames so that we have a list of all pack only refs that we will be deleting during this transaction.

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

refs.c, refs.h

diff --git a/refs.c b/refs.c
index 51cd41e..b525076 100644
--- a/refs.c
+++ b/refs.c
@@ -3321,6 +3321,9 @@ int ref_transaction_update(struct ref_transaction *transaction,
 	if (transaction->status != REF_TRANSACTION_OPEN)
 		die("BUG: update on transaction that is not open");
 
+	if (flags & REF_ISPACKONLY)
+		die("BUG: REF_ISPACKONLY can not be used with updates");
+
 	update = add_update(transaction, refname);
 	hashcpy(update->new_sha1, new_sha1);
 	update->flags = flags;
@@ -3345,6 +3348,9 @@ int ref_transaction_create(struct ref_transaction *transaction,
 	if (transaction->status != REF_TRANSACTION_OPEN)
 		die("BUG: create on transaction that is not open");
 
+	if (flags & REF_ISPACKONLY)
+		die("BUG: REF_ISPACKONLY can not be used with creates");
+
 	update = add_update(transaction, refname);
 
 	hashcpy(update->new_sha1, new_sha1);
@@ -3458,10 +3464,20 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	if (ret)
 		goto cleanup;
 
+	for (i = 0; i < n; i++) {
+		struct ref_update *update = updates[i];
+
+		if (update->flags & REF_ISPACKONLY)
+			delnames[delnum++] = update->refname;
+	}
+
 	/* Acquire all locks while verifying old values */
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
+		if (update->flags & REF_ISPACKONLY)
+			continue;
+
 		update->lock = lock_ref_sha1_basic(update->refname,
 						   (update->have_old ?
 						    update->old_sha1 :
@@ -3499,6 +3515,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	for (i = 0; i < n; i++) {
 		struct ref_update *update = updates[i];
 
+		if (update->flags & REF_ISPACKONLY)
+			continue;
+
 		if (update->lock) {
 			ret |= delete_ref_loose(update->lock, update->type);
 			if (!(update->flags & REF_ISPRUNING))
diff --git a/refs.h b/refs.h
index 7a89415..71e39b9 100644
--- a/refs.h
+++ b/refs.h
@@ -136,6 +136,8 @@ extern int peel_ref(const char *refname, unsigned char *sha1);
 #define REF_NODEREF	0x01
 /** Deleting a loose ref during prune */
 #define REF_ISPRUNING	0x02
+/** Deletion of a ref that only exists as a packed ref */
+#define REF_ISPACKONLY	0x04
 extern struct ref_lock *lock_any_ref_for_update(const char *refname,
 						const unsigned char *old_sha1,
 						int flags, int *type_p);

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 40/42] refs.c: pass a skip list to name_conflict_fn

Allow passing a list of refs to ckip 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 anc 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 | 43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)
Show changes to refs.c +33 −10
diff --git a/refs.c b/refs.c
index b525076..eb75927 100644
--- a/refs.c
+++ b/refs.c
@@ -789,11 +789,19 @@ 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;
+	int i;
+	for(i = 0; i < data->skipnum; i++) {
+		if (!strcmp(entry->name, data->skip[i])) {
+			return 0;
+		}
+	}
 	if (data->oldrefname && !strcmp(data->oldrefname, entry->name))
 		return 0;
 	if (names_conflict(data->refname, entry->name)) {
@@ -808,15 +816,21 @@ 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. Refs may be skipped due to us knowing that it will
+ * be deleted later during a transaction that deletes one reference and then
+ * creates a new conflicting reference. For example a rename from m to m/m.
  */
 static int is_refname_available(const char *refname, const char *oldrefname,
-				struct ref_dir *dir)
+				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)) {
@@ -2032,7 +2046,8 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 
 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;
@@ -2079,7 +2094,9 @@ 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, NULL,
+				   get_packed_refs(&ref_cache),
+				   skip, skipnum)) {
 		last_errno = ENOTDIR;
 		goto error_return;
 	}
@@ -2137,7 +2154,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);
 }
 
 /*
@@ -2576,6 +2593,9 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
 	const char *symref = NULL;
 
+	if (!strcmp(oldrefname, newrefname))
+		return 0;
+
 	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldrefname);
 
@@ -2586,10 +2606,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, oldrefname,
+				  get_packed_refs(&ref_cache), NULL, 0))
 		return 1;
 
-	if (!is_refname_available(newrefname, oldrefname, get_loose_refs(&ref_cache)))
+	if (!is_refname_available(newrefname, oldrefname,
+				  get_loose_refs(&ref_cache), NULL, 0))
 		return 1;
 
 	if (log && rename(git_path("logs/%s", oldrefname), git_path(TMP_RENAMED_LOG)))
@@ -2622,7 +2644,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;
@@ -2637,7 +2659,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;
@@ -3483,7 +3505,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.0.rc1.351.g4d2c8e4
Eric Sunshine· May 2, 2014, 04:22 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 40/42] refs.c: pass a skip list to name_conflict_fn

On Thu, May 1, 2014 at 4:37 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
> Allow passing a list of refs to ckip checking to name_conflict_fn.
s/ckip/skip/
Show 7 quoted lines
> 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 anc create m/m since we know that the
s/anc/and/
Show 146 quoted lines
> 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 | 43 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index b525076..eb75927 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -789,11 +789,19 @@ 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;
> +       int i;
> +       for(i = 0; i < data->skipnum; i++) {
> +               if (!strcmp(entry->name, data->skip[i])) {
> +                       return 0;
> +               }
> +       }
>         if (data->oldrefname && !strcmp(data->oldrefname, entry->name))
>                 return 0;
>         if (names_conflict(data->refname, entry->name)) {
> @@ -808,15 +816,21 @@ 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. Refs may be skipped due to us knowing that it will
> + * be deleted later during a transaction that deletes one reference and then
> + * creates a new conflicting reference. For example a rename from m to m/m.
>   */
>  static int is_refname_available(const char *refname, const char *oldrefname,
> -                               struct ref_dir *dir)
> +                               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)) {
> @@ -2032,7 +2046,8 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
>
>  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;
> @@ -2079,7 +2094,9 @@ 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, NULL,
> +                                  get_packed_refs(&ref_cache),
> +                                  skip, skipnum)) {
>                 last_errno = ENOTDIR;
>                 goto error_return;
>         }
> @@ -2137,7 +2154,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);
>  }
>
>  /*
> @@ -2576,6 +2593,9 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>         int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
>         const char *symref = NULL;
>
> +       if (!strcmp(oldrefname, newrefname))
> +               return 0;
> +
>         if (log && S_ISLNK(loginfo.st_mode))
>                 return error("reflog for %s is a symlink", oldrefname);
>
> @@ -2586,10 +2606,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, oldrefname,
> +                                 get_packed_refs(&ref_cache), NULL, 0))
>                 return 1;
>
> -       if (!is_refname_available(newrefname, oldrefname, get_loose_refs(&ref_cache)))
> +       if (!is_refname_available(newrefname, oldrefname,
> +                                 get_loose_refs(&ref_cache), NULL, 0))
>                 return 1;
>
>         if (log && rename(git_path("logs/%s", oldrefname), git_path(TMP_RENAMED_LOG)))
> @@ -2622,7 +2644,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;
> @@ -2637,7 +2659,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;
> @@ -3483,7 +3505,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.0.rc1.351.g4d2c8e4
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ronnie Sahlberg· May 2, 2014, 14:49 UTC · re: Eric Sunshine · lore

Re: [PATCH v6 40/42] refs.c: pass a skip list to name_conflict_fn

Fixed. Thanks.
On Thu, May 1, 2014 at 9:22 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 161 quoted lines
> On Thu, May 1, 2014 at 4:37 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
>> Allow passing a list of refs to ckip checking to name_conflict_fn.
>
> s/ckip/skip/
>
>> 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 anc create m/m since we know that the
>
> s/anc/and/
>
>> 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 | 43 +++++++++++++++++++++++++++++++++----------
>>  1 file changed, 33 insertions(+), 10 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index b525076..eb75927 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -789,11 +789,19 @@ 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;
>> +       int i;
>> +       for(i = 0; i < data->skipnum; i++) {
>> +               if (!strcmp(entry->name, data->skip[i])) {
>> +                       return 0;
>> +               }
>> +       }
>>         if (data->oldrefname && !strcmp(data->oldrefname, entry->name))
>>                 return 0;
>>         if (names_conflict(data->refname, entry->name)) {
>> @@ -808,15 +816,21 @@ 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. Refs may be skipped due to us knowing that it will
>> + * be deleted later during a transaction that deletes one reference and then
>> + * creates a new conflicting reference. For example a rename from m to m/m.
>>   */
>>  static int is_refname_available(const char *refname, const char *oldrefname,
>> -                               struct ref_dir *dir)
>> +                               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)) {
>> @@ -2032,7 +2046,8 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
>>
>>  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;
>> @@ -2079,7 +2094,9 @@ 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, NULL,
>> +                                  get_packed_refs(&ref_cache),
>> +                                  skip, skipnum)) {
>>                 last_errno = ENOTDIR;
>>                 goto error_return;
>>         }
>> @@ -2137,7 +2154,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);
>>  }
>>
>>  /*
>> @@ -2576,6 +2593,9 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>>         int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
>>         const char *symref = NULL;
>>
>> +       if (!strcmp(oldrefname, newrefname))
>> +               return 0;
>> +
>>         if (log && S_ISLNK(loginfo.st_mode))
>>                 return error("reflog for %s is a symlink", oldrefname);
>>
>> @@ -2586,10 +2606,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, oldrefname,
>> +                                 get_packed_refs(&ref_cache), NULL, 0))
>>                 return 1;
>>
>> -       if (!is_refname_available(newrefname, oldrefname, get_loose_refs(&ref_cache)))
>> +       if (!is_refname_available(newrefname, oldrefname,
>> +                                 get_loose_refs(&ref_cache), NULL, 0))
>>                 return 1;
>>
>>         if (log && rename(git_path("logs/%s", oldrefname), git_path(TMP_RENAMED_LOG)))
>> @@ -2622,7 +2644,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;
>> @@ -2637,7 +2659,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;
>> @@ -3483,7 +3505,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.0.rc1.351.g4d2c8e4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 41/42] refs.c: make rename_ref use a transaction

Change rename_ref to use a single transaction to perform the ref rename.
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 73 ++++++++++++++++++------------------------------------------------
 1 file changed, 20 insertions(+), 53 deletions(-)
Show changes to refs.c +20 −53
diff --git a/refs.c b/refs.c
index eb75927..810a4db 100644
--- a/refs.c
+++ b/refs.c
@@ -2586,9 +2586,10 @@ static int rename_tmp_log(const char *newrefname)
 
 int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)
 {
-	unsigned char sha1[20], orig_sha1[20];
-	int flag = 0, logmoved = 0;
-	struct ref_lock *lock;
+	unsigned char sha1[20];
+	int flag = 0;
+	struct ref_transaction *transaction;
+	struct strbuf err = STRBUF_INIT;
 	struct stat loginfo;
 	int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
 	const char *symref = NULL;
@@ -2599,7 +2600,7 @@ 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, sha1, 1, &flag);
 	if (flag & REF_ISSYMREF)
 		return error("refname %s is a symbolic ref, renaming it is not supported",
 			oldrefname);
@@ -2621,62 +2622,28 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	if (pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE))
 		return error("unable to pack refs");
 
-	if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
-		error("unable to delete old %s", oldrefname);
-		goto rollback;
-	}
-
-	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))) {
-				error("Directory not empty: %s", newrefname);
-				goto rollback;
-			}
-		} else {
-			error("unable to delete existing %s", newrefname);
-			goto rollback;
-		}
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_delete(transaction, oldrefname, sha1,
+				   REF_NODEREF | REF_ISPACKONLY,
+				   1, NULL) ||
+	    ref_transaction_update(transaction, newrefname, sha1,
+				   NULL, 0, 0, logmsg) ||
+	    ref_transaction_commit(transaction, &err)) {
+		ref_transaction_rollback(transaction);
+		error("rename_ref failed: %s", err.buf);
+		strbuf_release(&err);
+		goto rollbacklog;
 	}
+	ref_transaction_free(transaction);
 
 	if (log && rename_tmp_log(newrefname))
-		goto rollback;
-
-	logmoved = log;
-
-	lock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL, NULL, 0);
-	if (!lock) {
-		error("unable to lock %s for update", newrefname);
-		goto rollback;
-	}
-	lock->force_write = 1;
-	hashcpy(lock->old_sha1, orig_sha1);
-	if (write_ref_sha1(lock, orig_sha1, logmsg)) {
-		error("unable to write current sha1 into %s", newrefname);
-		goto rollback;
-	}
-
-	return 0;
-
- rollback:
-	lock = lock_ref_sha1_basic(oldrefname, NULL, 0, NULL, NULL, 0);
-	if (!lock) {
-		error("unable to lock %s for rollback", oldrefname);
 		goto rollbacklog;
-	}
 
-	lock->force_write = 1;
-	flag = log_all_ref_updates;
-	log_all_ref_updates = 0;
-	if (write_ref_sha1(lock, orig_sha1, NULL))
-		error("unable to write current sha1 into %s", oldrefname);
-	log_all_ref_updates = flag;
+	return 0;
 
  rollbacklog:
-	if (logmoved && rename(git_path("logs/%s", newrefname), git_path("logs/%s", oldrefname)))
-		error("unable to restore logfile %s from %s: %s",
-			oldrefname, newrefname, strerror(errno));
-	if (!logmoved && log &&
+	if (log &&
 	    rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", oldrefname)))
 		error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
 			oldrefname, strerror(errno));

-- 
2.0.0.rc1.351.g4d2c8e4
Ronnie Sahlberg· May 1, 2014, 20:37 UTC · re: Ronnie Sahlberg · lore

[PATCH v6 42/42] refs.c: remove forward declaraion of write_ref_sha1

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 2 --
 1 file changed, 2 deletions(-)
Show changes to refs.c +0 −2
diff --git a/refs.c b/refs.c
index 810a4db..e59bc18 100644
--- a/refs.c
+++ b/refs.c
@@ -251,8 +251,6 @@ struct ref_entry {
 };
 
 static void read_loose_refs(const char *dirname, struct ref_dir *dir);
-static int write_ref_sha1(struct ref_lock *lock,
-			  const unsigned char *sha1, const char *logmsg);
 
 static struct ref_dir *get_ref_dir(struct ref_entry *entry)
 {

-- 
2.0.0.rc1.351.g4d2c8e4
Michael Haggerty· May 5, 2014, 12:57 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 00/42] Use ref transactions for all ref updates

On 05/01/2014 10:37 PM, Ronnie Sahlberg wrote:
> This patch series is based on next and expands on the transaction API. [...]
Meta-comment:
Ronnie,

It seems like successive versions of this patch series are growing not only in maturity but also in breadth. That makes it harder to review them.

I, for one, would prefer that a patch series cover a roughly fixed set of changes [1], so that all of the patches in a version of the series are at roughly the same level of maturity. That way, the whole series can progress from "is this a good idea?" to "is the implementation correct?" to "are all the details right?" at roughly the same time, and then Junio can merge the branch, locking in that bit of progress. While this is happening, other series can be making their way through other stages of the pipeline.

When new patches are added to an old series, then they delay the merge of the older patches, even if those are ripe. Plus, it makes it harder for reviewers to keep track of the maturity level of each patch and to read off how the older patches have changed. It makes the patch series a moving target.

There's no need to re-split this patch series, but please take this wish into account in the future.

Thanks, Michael

[1] Of course, if a patch series has to grow to make the *existing* changes correct, then that's perfectly OK.

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Ronnie Sahlberg· May 5, 2014, 15:09 UTC · re: Michael Haggerty · lore

Re: [PATCH v6 00/42] Use ref transactions for all ref updates

On Mon, May 5, 2014 at 5:57 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 28 quoted lines
> On 05/01/2014 10:37 PM, Ronnie Sahlberg wrote:
>> This patch series is based on next and expands on the transaction API. [...]
>
> Meta-comment:
>
> Ronnie,
>
> It seems like successive versions of this patch series are growing not
> only in maturity but also in breadth.  That makes it harder to review them.
>
> I, for one, would prefer that a patch series cover a roughly fixed set
> of changes [1], so that all of the patches in a version of the series
> are at roughly the same level of maturity.  That way, the whole series
> can progress from "is this a good idea?" to "is the implementation
> correct?" to "are all the details right?" at roughly the same time, and
> then Junio can merge the branch, locking in that bit of progress.  While
> this is happening, other series can be making their way through other
> stages of the pipeline.
>
> When new patches are added to an old series, then they delay the merge
> of the older patches, even if those are ripe.  Plus, it makes it harder
> for reviewers to keep track of the maturity level of each patch and to
> read off how the older patches have changed.  It makes the patch series
> a moving target.
>
> There's no need to re-split this patch series, but please take this wish
> into account in the future.
>
Understood.
Jonathan Nieder· May 13, 2014, 20:25 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH v6 00/42] Use ref transactions for all ref updates

Hi,
Ronnie Sahlberg wrote:
> This patch series is based on next and expands on the transaction API.
Sorry to take so long to get to this.

For the future, it's easier to review patches based on some particular branch that got merged into next, since next is a moving target (series come and go from there depending on what seems to need testing at a given moment). Is mh/ref-transaction the relevant branch to build on?

Trying to apply the series to mh/ref-transaction, I get conflicts in patch 13 due to absence of rs/ref-update-check-errors-early.

Trying to apply the series to a merge of mh/ref-transaction and rs/ref-update-check-errors-early, I get a minor conflict in patch 15 but it is easy to resolve and the rest goes smoothly.

Looking forward to reading the rest. Thanks. Jonathan

← back to recent threads