[PATCH v3 00/19] Use ref transactions from most callers

[PATCH v3 00/19] Use ref transactions from most callers

[Ronnie Sahlberg]

This patch series changes most of the places where the ref functions for
locking and writing refs to instead use the new ref transaction API. There
are still three more places where write_ref_sha1() is called from outside
of refs.c but those all will require more complex work and review so those
changes belong in a different patch series.

I think I have covered all issues raised on the previous reviews and also
done a bunch of cleanups and improvements to the transaction API.


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 (19):
  refs.c: constify the sha arguments for
    ref_transaction_create|delete|update
  refs.c: allow passing NULL to ref_transaction_free
  refs.c: make ref_transaction_commit return an error string
  refs.c: return error string from ref_update_reject_duplicates on
    failure
  update-ref.c: use the error string from _commit to print better
    message
  refs.c: make update_ref_write to return error string 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: pass **transaction to commit and have it clear the pointer

 branch.c              |  39 ++++++++++------
 builtin/commit.c      |  23 +++++-----
 builtin/replace.c     |  14 +++---
 builtin/tag.c         |  14 +++---
 builtin/update-ref.c  |  30 ++++++++-----
 fast-import.c         |  18 ++++----
 refs.c                | 122 +++++++++++++++++++++++++++++++++++---------------
 refs.h                |  32 +++++++------
 sequencer.c           |  21 ++++++---
 t/t1400-update-ref.sh |  16 +++----
 10 files changed, 206 insertions(+), 123 deletions(-)

-- 
1.9.1.521.g5dc89fa

[PATCH v3 01/19] refs.c: constify the sha arguments for ref_transaction_create|delete|update

[Ronnie Sahlberg]

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

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);
 
 /*
-- 
1.9.1.521.g5dc89fa

[PATCH v3 02/19] refs.c: allow passing NULL to ref_transaction_free

[Ronnie Sahlberg]

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(+)

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]);
 
-- 
1.9.1.521.g5dc89fa

[PATCH v3 03/19] refs.c: make ref_transaction_commit return an error string

[Ronnie Sahlberg]

Let ref_transaction_commit return an optional error string that describes
the transaction failure. Start by returning the same error as update_ref_lock
returns, modulo the initial error:/fatal: preamble.

This will make it easier for callers to craft better error messages when
a transaction call fails.

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

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..0712912 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, char **err,
+			   enum action_on_err onerr)
 {
 	int ret = 0, delnum = 0, i;
 	const char **delnames;
@@ -3424,6 +3425,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 	if (!n)
 		return 0;
 
+	if (err)
+		*err = NULL;
+
 	/* Allocate work space */
 	delnames = xmalloc(sizeof(*delnames) * n);
 
@@ -3443,6 +3447,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 					       update->flags,
 					       &update->type, onerr);
 		if (!update->lock) {
+			if (err) {
+				const char *str = "Cannot lock the ref '%s'.";
+				*err = xmalloc(PATH_MAX + 24);
+				snprintf(*err, PATH_MAX + 24, str,
+					 update->refname);
+			}
 			ret = 1;
 			goto cleanup;
 		}
diff --git a/refs.h b/refs.h
index 892c5b6..615c73d 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 error is non-NULL it will return an error string that describes
+ * why a commit failed. This string must be free()ed by the caller.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
-			   const char *msg, enum action_on_err onerr);
+			   const char *msg, char **err,
+			   enum action_on_err onerr);
 
 /** Lock a ref and then write its file */
 int update_ref(const char *action, const char *refname,
-- 
1.9.1.521.g5dc89fa

Re: [PATCH v3 03/19] refs.c: make ref_transaction_commit return an error string

[Jonathan Nieder]

Ronnie Sahlberg wrote:

> Let ref_transaction_commit return an optional error string that describes
> the transaction failure.  Start by returning the same error as update_ref_lock
> returns, modulo the initial error:/fatal: preamble.

s/returns/prints/?

> This will make it easier for callers to craft better error messages when
> a transaction call fails.

Interesting.  Can you give an example?  What kind of behavior are we
expecting in callers other than die()-ing or cleaning up and then
die()-ing?

I like this more than having the caller pass in a flag/callback/etc to
decide how noisy to be and whether to gracefully accept errors or exit.
So it seems like an improvement, but may always returning error()
would be better --- more context would help in clarifying this.

> --- 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 error is non-NULL it will return an error string that describes
> + * why a commit failed. This string must be free()ed by the caller.
>   */
>  int ref_transaction_commit(struct ref_transaction *transaction,
> -			   const char *msg, enum action_on_err onerr);
> +			   const char *msg, char **err,
> +			   enum action_on_err onerr);

Is the idea that if I pass in a pointer &err then
ref_transaction_commit will take the action described by onerr *and*
write its error message to err?

Probably squashing with patch 07 would make this easier to read (and
wouldn't require changing any messages at that point).

[...]
> --- a/refs.c
> +++ b/refs.c
[...]
> @@ -3443,6 +3447,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  					       update->flags,
>  					       &update->type, onerr);
>  		if (!update->lock) {
> +			if (err) {
> +				const char *str = "Cannot lock the ref '%s'.";
> +				*err = xmalloc(PATH_MAX + 24);
> +				snprintf(*err, PATH_MAX + 24, str,
> +					 update->refname);
> +			}

Might be clearer to use a helper similar to path.c::mkpathdup

	char *aprintf(const char *fmt, ...)
	{
		char *result;
		struct strbuf sb = STRBUF_INIT;
		va_list args;

		va_start(args, fmt);
		strbuf_vaddf(&sb, fmt, args);
		va_end(args);

		return strbuf_detach(&sb);
	}

or to have the caller pass in a pointer to strbuf instead of char *.

The rest looks good to me.

Thanks,
Jonathan

Re: [PATCH v3 03/19] refs.c: make ref_transaction_commit return an error string

[Ronnie Sahlberg]

Good points.



On Fri, Apr 25, 2014 at 3:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ronnie Sahlberg wrote:
>
>> Let ref_transaction_commit return an optional error string that describes
>> the transaction failure.  Start by returning the same error as update_ref_lock
>> returns, modulo the initial error:/fatal: preamble.
>
> s/returns/prints/?

Done, and then was deleted when I reworded the message.

>
>> This will make it easier for callers to craft better error messages when
>> a transaction call fails.
>
> Interesting.  Can you give an example?  What kind of behavior are we
> expecting in callers other than die()-ing or cleaning up and then
> die()-ing?

I was thinking a bit too far ahead. You could in theory keep logging multiple
lock failures during the _commit() and then when the transaction fails
and returns
it will have appended a list of all refs that failed and not just the
first ref that failed.


>
> I like this more than having the caller pass in a flag/callback/etc to
> decide how noisy to be and whether to gracefully accept errors or exit.
> So it seems like an improvement, but may always returning error()
> would be better --- more context would help in clarifying this.
>
>> --- 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 error is non-NULL it will return an error string that describes
>> + * why a commit failed. This string must be free()ed by the caller.
>>   */
>>  int ref_transaction_commit(struct ref_transaction *transaction,
>> -                        const char *msg, enum action_on_err onerr);
>> +                        const char *msg, char **err,
>> +                        enum action_on_err onerr);
>
> Is the idea that if I pass in a pointer &err then
> ref_transaction_commit will take the action described by onerr *and*
> write its error message to err?

Temporarily, yes.
Shortly after this patch I remove the onerr argument completely.
But I want to keep the "pass error back to caller" and "get rid of
onerr" as two separate patches.
I think it is easier to follow the flow of changes if they are done in
two separate steps.

>
> Probably squashing with patch 07 would make this easier to read (and
> wouldn't require changing any messages at that point).

See above.

>
> [...]
>> --- a/refs.c
>> +++ b/refs.c
> [...]
>> @@ -3443,6 +3447,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>>                                              update->flags,
>>                                              &update->type, onerr);
>>               if (!update->lock) {
>> +                     if (err) {
>> +                             const char *str = "Cannot lock the ref '%s'.";
>> +                             *err = xmalloc(PATH_MAX + 24);
>> +                             snprintf(*err, PATH_MAX + 24, str,
>> +                                      update->refname);
>> +                     }
>
> Might be clearer to use a helper similar to path.c::mkpathdup
>
>         char *aprintf(const char *fmt, ...)
>         {
>                 char *result;
>                 struct strbuf sb = STRBUF_INIT;
>                 va_list args;
>
>                 va_start(args, fmt);
>                 strbuf_vaddf(&sb, fmt, args);
>                 va_end(args);
>
>                 return strbuf_detach(&sb);
>         }
>
> or to have the caller pass in a pointer to strbuf instead of char *.

strbuf as argument is probably the right thing to do. I am doing that change.

>
> The rest looks good to me.
>
> Thanks,
> Jonathan

[PATCH v3 04/19] refs.c: return error string from ref_update_reject_duplicates on failure

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 0712912..9d9eab8 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,
+					char **err,
 					enum action_on_err onerr)
 {
 	int i;
@@ -3400,6 +3401,11 @@ 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) {
+				*err = xmalloc(PATH_MAX + 41);
+				snprintf(*err, PATH_MAX + 41, str,
+					 updates[i]->refname);
+			}
 			switch (onerr) {
 			case UPDATE_REFS_MSG_ON_ERR:
 				error(str, updates[i]->refname); break;
@@ -3433,7 +3439,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;
 
-- 
1.9.1.521.g5dc89fa

Re: [PATCH v3 04/19] refs.c: return error string from ref_update_reject_duplicates on failure

[Jonathan Nieder]

Ronnie Sahlberg wrote:

> --- 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,
> +					char **err,
>  					enum action_on_err onerr)
>  {
>  	int i;
> @@ -3400,6 +3401,11 @@ 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) {
> +				*err = xmalloc(PATH_MAX + 41);
> +				snprintf(*err, PATH_MAX + 41, str,
> +					 updates[i]->refname);
> +			}

Same issues as the previous patch: it's too easy to get the buffer size
wrong when updating the message (or, worse, when making it
translatable).  aprintf or a strbuf should work better.

Otherwise seems sensible.

Re: [PATCH v3 04/19] refs.c: return error string from ref_update_reject_duplicates on failure

[Ronnie Sahlberg]

Thanks.

Replaced with strbuf_addf() instead.

On Fri, Apr 25, 2014 at 3:12 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ronnie Sahlberg wrote:
>
>> --- 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,
>> +                                     char **err,
>>                                       enum action_on_err onerr)
>>  {
>>       int i;
>> @@ -3400,6 +3401,11 @@ 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) {
>> +                             *err = xmalloc(PATH_MAX + 41);
>> +                             snprintf(*err, PATH_MAX + 41, str,
>> +                                      updates[i]->refname);
>> +                     }
>
> Same issues as the previous patch: it's too easy to get the buffer size
> wrong when updating the message (or, worse, when making it
> translatable).  aprintf or a strbuf should work better.
>
> Otherwise seems sensible.

[PATCH v3 05/19] update-ref.c: use the error string from _commit to print better message

[Ronnie Sahlberg]

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

Update the tests to reflect that the log message is now slightly different
  fatal: update_ref failed: Cannot lock the ref 'some ref'
versus from the previous
  fatal: Cannot lock the ref 'some ref'

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/update-ref.c  | 12 +++++++-----
 t/t1400-update-ref.sh | 16 ++++++++--------
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index aaa06aa..47c9b53 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;
+	char *err;
 	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,18 @@ 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 (!transaction)
+			die("failed to update refs");
 		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("update_ref failed: %s", err);
+		return 0;
 	}
 
 	if (end_null)
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 48ccc4d..53ed0cb 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -453,7 +453,7 @@ test_expect_success 'stdin fails with duplicate refs' '
 	create $a $m
 	EOF
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: Multiple updates for ref '"'"'$a'"'"' not allowed." err
+	grep "fatal: update_ref failed: Multiple updates for ref '"'"'$a'"'"' not allowed." err
 '
 
 test_expect_success 'stdin create ref works' '
@@ -511,7 +511,7 @@ test_expect_success 'stdin create ref works with path with space to blob' '
 test_expect_success 'stdin update ref fails with wrong old value' '
 	echo "update $c $m $m~1" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err &&
+	grep "fatal: update_ref failed: Cannot lock the ref '"'"'$c'"'"'" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
@@ -547,7 +547,7 @@ test_expect_success 'stdin update ref works with right old value' '
 test_expect_success 'stdin delete ref fails with wrong old value' '
 	echo "delete $a $m~1" >stdin &&
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: Cannot lock the ref '"'"'$a'"'"'" err &&
+	grep "fatal: update_ref failed: Cannot lock the ref '"'"'$a'"'"'" err &&
 	git rev-parse $m >expect &&
 	git rev-parse $a >actual &&
 	test_cmp expect actual
@@ -634,7 +634,7 @@ test_expect_success 'stdin update refs fails with wrong old value' '
 	update $c  ''
 	EOF
 	test_must_fail git update-ref --stdin <stdin 2>err &&
-	grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err &&
+	grep "fatal: update_ref failed: Cannot lock the ref '"'"'$c'"'"'" err &&
 	git rev-parse $m >expect &&
 	git rev-parse $a >actual &&
 	test_cmp expect actual &&
@@ -807,7 +807,7 @@ test_expect_success 'stdin -z fails option with unknown name' '
 test_expect_success 'stdin -z fails with duplicate refs' '
 	printf $F "create $a" "$m" "create $b" "$m" "create $a" "$m" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: Multiple updates for ref '"'"'$a'"'"' not allowed." err
+	grep "fatal: update_ref failed: Multiple updates for ref '"'"'$a'"'"' not allowed." err
 '
 
 test_expect_success 'stdin -z create ref works' '
@@ -847,7 +847,7 @@ test_expect_success 'stdin -z create ref works with path with space to blob' '
 test_expect_success 'stdin -z update ref fails with wrong old value' '
 	printf $F "update $c" "$m" "$m~1" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err &&
+	grep "fatal: update_ref failed: Cannot lock the ref '"'"'$c'"'"'" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
@@ -883,7 +883,7 @@ test_expect_success 'stdin -z update ref works with right old value' '
 test_expect_success 'stdin -z delete ref fails with wrong old value' '
 	printf $F "delete $a" "$m~1" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: Cannot lock the ref '"'"'$a'"'"'" err &&
+	grep "fatal: update_ref failed: Cannot lock the ref '"'"'$a'"'"'" err &&
 	git rev-parse $m >expect &&
 	git rev-parse $a >actual &&
 	test_cmp expect actual
@@ -952,7 +952,7 @@ test_expect_success 'stdin -z update refs fails with wrong old value' '
 	git update-ref $c $m &&
 	printf $F "update $a" "$m" "$m" "update $b" "$m" "$m" "update $c" "$m" "$Z" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: Cannot lock the ref '"'"'$c'"'"'" err &&
+	grep "fatal: update_ref failed: Cannot lock the ref '"'"'$c'"'"'" err &&
 	git rev-parse $m >expect &&
 	git rev-parse $a >actual &&
 	test_cmp expect actual &&
-- 
1.9.1.521.g5dc89fa

Re: [PATCH v3 05/19] update-ref.c: use the error string from _commit to print better message

[Jonathan Nieder]

Ronnie Sahlberg wrote:

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

Ah, so that's how the transition to a better API happens.  Makes sense.

(A mention of QUIET_ON_ERR in the patch that introduces the &err
parameter could help, or feel free to ignore these comments, since it's
all well by the end of the series.)

> Update the tests to reflect that the log message is now slightly different
>   fatal: update_ref failed: Cannot lock the ref 'some ref'
> versus from the previous
>   fatal: Cannot lock the ref 'some ref'

Makes sense as a demo of what the new code allows, but is this error
message better?  The use of 'git update-ref' is an implementation
detail that the user doesn't need to know about.

I think I'd prefer the result of plain die("%s", err), even though
that's a no-op.

[...]
> +++ b/builtin/update-ref.c
[...]
> @@ -359,17 +360,18 @@ 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 (!transaction)
> +			die("failed to update refs");

This can't happen (xcalloc is defined to die() on malloc failure).
If you want to protect against it anyway, die("BUG: ...")?

Thanks,
Jonathan

Re: [PATCH v3 05/19] update-ref.c: use the error string from _commit to print better message

[Ronnie Sahlberg]

Good ideas.
Applied, thanks.

On Fri, Apr 25, 2014 at 3:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ronnie Sahlberg wrote:
>
>> Call ref_transaction_commit with QUIET_ON_ERR and use the error string
>> that is returned to print a better log message if/after the transaction
>> fails.
>
> Ah, so that's how the transition to a better API happens.  Makes sense.
>
> (A mention of QUIET_ON_ERR in the patch that introduces the &err
> parameter could help, or feel free to ignore these comments, since it's
> all well by the end of the series.)
>
>> Update the tests to reflect that the log message is now slightly different
>>   fatal: update_ref failed: Cannot lock the ref 'some ref'
>> versus from the previous
>>   fatal: Cannot lock the ref 'some ref'
>
> Makes sense as a demo of what the new code allows, but is this error
> message better?  The use of 'git update-ref' is an implementation
> detail that the user doesn't need to know about.
>
> I think I'd prefer the result of plain die("%s", err), even though
> that's a no-op.
>
> [...]
>> +++ b/builtin/update-ref.c
> [...]
>> @@ -359,17 +360,18 @@ 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 (!transaction)
>> +                     die("failed to update refs");
>
> This can't happen (xcalloc is defined to die() on malloc failure).
> If you want to protect against it anyway, die("BUG: ...")?
>
> Thanks,
> Jonathan

[PATCH v3 06/19] refs.c: make update_ref_write to return error string on failure

[Ronnie Sahlberg]

Change update_ref_write to also return an error string on failure.
This makes the error avaialbel to ref_transaction_commit callers if the
transaction failed dur to update_ref_sha1/write_ref_sha1 failures.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 9d9eab8..7562151 100644
--- a/refs.c
+++ b/refs.c
@@ -3253,10 +3253,14 @@ 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)
+			    char **err, enum action_on_err onerr)
 {
 	if (write_ref_sha1(lock, sha1, action) < 0) {
 		const char *str = "Cannot update the ref '%s'.";
+		if (err) {
+			*err = xmalloc(PATH_MAX + 26);
+			snprintf(*err, PATH_MAX + 26, 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 +3386,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)
@@ -3472,7 +3476,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;
-- 
1.9.1.521.g5dc89fa

Re: [PATCH v3 06/19] refs.c: make update_ref_write to return error string on failure

[Jonathan Nieder]

Ronnie Sahlberg wrote:

> Change update_ref_write to also return an error string on failure.
> This makes the error avaialbel to ref_transaction_commit callers if the
> transaction failed dur to update_ref_sha1/write_ref_sha1 failures.

Nits:

 * available
 * during

Probably should come right after or before patch 3.  Same nit as patch
3 about using asprintf or passing in a pointer to strbuf.

Re: [PATCH v3 06/19] refs.c: make update_ref_write to return error string on failure

[Ronnie Sahlberg]

Done, thanks

On Fri, Apr 25, 2014 at 3:45 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ronnie Sahlberg wrote:
>
>> Change update_ref_write to also return an error string on failure.
>> This makes the error avaialbel to ref_transaction_commit callers if the
>> transaction failed dur to update_ref_sha1/write_ref_sha1 failures.
>
> Nits:
>
>  * available
>  * during
>
> Probably should come right after or before patch 3.  Same nit as patch
> 3 about using asprintf or passing in a pointer to strbuf.

[PATCH v3 07/19] refs.c: remove the onerr argument to ref_transaction_commit

[Ronnie Sahlberg]

Since we now pass meaningful error messages back from ref_transaction_commit
on failures, we no longer need to provide a onerr argument. The callers can now
on commit failures die() with a meaningful, and in most cases even better than
before, error message.

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

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 47c9b53..df3cd92 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -368,8 +368,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("update_ref failed: %s", err);
 		return 0;
 	}
diff --git a/refs.c b/refs.c
index 7562151..0e2df81 100644
--- a/refs.c
+++ b/refs.c
@@ -3397,8 +3397,7 @@ static int ref_update_compare(const void *r1, const void *r2)
 }
 
 static int ref_update_reject_duplicates(struct ref_update **updates, int n,
-					char **err,
-					enum action_on_err onerr)
+					char **err)
 {
 	int i;
 	for (i = 1; i < n; i++)
@@ -3410,22 +3409,13 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 				snprintf(*err, PATH_MAX + 41, 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, char **err,
-			   enum action_on_err onerr)
+			   const char *msg, char **err)
 {
 	int ret = 0, delnum = 0, i;
 	const char **delnames;
@@ -3443,7 +3433,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;
 
@@ -3455,7 +3445,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) {
 				const char *str = "Cannot lock the ref '%s'.";
@@ -3476,7 +3467,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 615c73d..f3db632 100644
--- a/refs.h
+++ b/refs.h
@@ -272,8 +272,7 @@ void ref_transaction_delete(struct ref_transaction *transaction,
  * why a commit failed. This string must be free()ed by the caller.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
-			   const char *msg, char **err,
-			   enum action_on_err onerr);
+			   const char *msg, char **err);
 
 /** Lock a ref and then write its file */
 int update_ref(const char *action, const char *refname,
-- 
1.9.1.521.g5dc89fa

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

[Jonathan Nieder]

Ronnie Sahlberg wrote:

> Since we now pass meaningful error messages back from ref_transaction_commit
> on failures, we no longer need to provide a onerr argument.

Yay!  More precisely, now that all callers use
UPDATE_REFS_QUIET_ON_ERR there's no need to support any other
behavior.

Thanks for cleaning up the error handling here.

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

[Ronnie Sahlberg]

Thanks.

Reworded it that we remove it since all callers now use QUIET_ON_ERR

On Fri, Apr 25, 2014 at 3:47 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ronnie Sahlberg wrote:
>
>> Since we now pass meaningful error messages back from ref_transaction_commit
>> on failures, we no longer need to provide a onerr argument.
>
> Yay!  More precisely, now that all callers use
> UPDATE_REFS_QUIET_ON_ERR there's no need to support any other
> behavior.
>
> Thanks for cleaning up the error handling here.

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

[Ronnie Sahlberg]

Update ref_transaction_update() do some basic error checking and return
true on error. Update all callers to check ref_transaction_update() for error.

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 df3cd92..13d094d 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 0e2df81..65cf407 100644
--- a/refs.c
+++ b/refs.c
@@ -3334,19 +3334,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 f3db632..6a361d7 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
-- 
1.9.1.521.g5dc89fa

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

[Ronnie Sahlberg]

Do basic error checking in ref_transaction_create() and make it return
status. Update all callers to check the result of ref_transaction_create()

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

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 13d094d..5f7dfc1 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 65cf407..762be85 100644
--- a/refs.c
+++ b/refs.c
@@ -3354,18 +3354,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 6a361d7..e984267 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
-- 
1.9.1.521.g5dc89fa

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

[Ronnie Sahlberg]

Change ref_transaction_delete() to do basic error checking and return
status. Update all callers to check the return for ref_transaction_delete()

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

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 5f7dfc1..a600ab3 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 762be85..1557c3c 100644
--- a/refs.c
+++ b/refs.c
@@ -3373,19 +3373,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 e984267..8135131 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
-- 
1.9.1.521.g5dc89fa

[PATCH v3 11/19] tag.c: use ref transactions when doing updates

[Ronnie Sahlberg]

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

diff --git a/builtin/tag.c b/builtin/tag.c
index 40356e3..dd53fb4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -488,7 +488,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;
@@ -496,6 +495,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;
+	char *err = NULL;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
@@ -641,11 +642,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);
 	if (force && !is_null_sha1(prev) && hashcmp(prev, object))
 		printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));
 
-- 
1.9.1.521.g5dc89fa

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

[Michael Haggerty]

On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
> 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(-)
> 
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 40356e3..dd53fb4 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -488,7 +488,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;
> @@ -496,6 +495,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;
> +	char *err = NULL;
>  	struct option options[] = {
>  		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
>  		{ OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
> @@ -641,11 +642,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);

Formatting nit: die() should be indented by two TABs.

>  	if (force && !is_null_sha1(prev) && hashcmp(prev, object))
>  		printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));
>  
> 

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

[Ronnie Sahlberg]

Done.

Thanks.

On Fri, Apr 25, 2014 at 3:58 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
>> 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(-)
>>
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index 40356e3..dd53fb4 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -488,7 +488,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;
>> @@ -496,6 +495,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;
>> +     char *err = NULL;
>>       struct option options[] = {
>>               OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
>>               { OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
>> @@ -641,11 +642,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);
>
> Formatting nit: die() should be indented by two TABs.
>
>>       if (force && !is_null_sha1(prev) && hashcmp(prev, object))
>>               printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));
>>
>>
>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

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

[Ronnie Sahlberg]

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

diff --git a/builtin/replace.c b/builtin/replace.c
index b62420a..cf0f56d 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;
+	char *err = NULL;
 
 	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);
 
 	return 0;
 }
-- 
1.9.1.521.g5dc89fa

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

[Michael Haggerty]

On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
> 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(-)
> 
> diff --git a/builtin/replace.c b/builtin/replace.c
> index b62420a..cf0f56d 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;
> +	char *err = NULL;
>  
>  	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);

die() should be indented by two TABs.

>  
>  	return 0;
>  }
> 

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

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

[Ronnie Sahlberg]

Done.

Thanks.

On Fri, Apr 25, 2014 at 4:00 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
>> 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(-)
>>
>> diff --git a/builtin/replace.c b/builtin/replace.c
>> index b62420a..cf0f56d 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;
>> +     char *err = NULL;
>>
>>       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);
>
> die() should be indented by two TABs.
>
>>
>>       return 0;
>>  }
>>
>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

[PATCH v3 13/19] commit.c: use ref transactions for updates

[Ronnie Sahlberg]

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

diff --git a/builtin/commit.c b/builtin/commit.c
index d9550c5..7e4c306 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;
+	char *err = NULL;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_commit_usage, builtin_commit_options);
@@ -1667,12 +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);
-
 	nl = strchr(sb.buf, '\n');
 	if (nl)
 		strbuf_setlen(&sb, nl + 1 - sb.buf);
@@ -1681,13 +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 (!ref_lock) {
-		rollback_index_files();
-		die(_("cannot lock HEAD ref"));
-	}
-	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);
 	}
 
 	unlink(git_path("CHERRY_PICK_HEAD"));
-- 
1.9.1.521.g5dc89fa

[PATCH v3 14/19] sequencer.c: use ref transactions for all ref updates

[Ronnie Sahlberg]

Change to use ref transactions for all updates to refs.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 sequencer.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index bde5f04..7d59f58 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,19 +272,29 @@ 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;
+	char *err = NULL;
 
 	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);
+
 	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);
+		strbuf_release(&sb);
+		return error(_("HEAD: Could not fast-forward: %s\n"), err);
+	}
+
 	strbuf_release(&sb);
-	return ret;
+	return 0;
 }
 
 static int do_recursive_merge(struct commit *base, struct commit *next,
-- 
1.9.1.521.g5dc89fa

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

[Ronnie Sahlberg]

Change update_branch() to use ref transactions for updates.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 fast-import.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index fb4738d..a2b05fa 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1678,36 +1678,39 @@ 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];
+	char *err = NULL;
 
 	if (is_null_sha1(b->sha1))
 		return 0;
 	if (read_ref(b->name, old_sha1))
 		hashclr(old_sha1);
-	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);
+		return error("Unable to update branch %s: %s", b->name, err);
+	}
 	return 0;
 }
 
-- 
1.9.1.521.g5dc89fa

[PATCH v3 16/19] branch.c: use ref transaction for all ref updates

[Ronnie Sahlberg]

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 | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/branch.c b/branch.c
index 660097b..23cde1e 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,34 @@ 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;
+		char *err = NULL;
+
+		transaction = ref_transaction_begin();
+		if (forcing) {
+			if (!transaction ||
+			    ref_transaction_update(transaction, ref.buf, sha1,
+						   NULL, 0, 0) ||
+			    ref_transaction_commit(transaction, msg, &err))
+			  die_errno(_("%s: failed to write ref: %s"),
+				    ref.buf, err);
+		} else {
+			if (!transaction ||
+			    ref_transaction_create(transaction, ref.buf, sha1,
+						   0) ||
+			    ref_transaction_commit(transaction, msg, &err))
+			  die_errno(_("%s: failed to write ref: %s"),
+				    ref.buf, err);
+		}
+	}
+
 	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);
 }
-- 
1.9.1.521.g5dc89fa

Re: [PATCH v3 16/19] branch.c: use ref transaction for all ref updates

[Michael Haggerty]

On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
> 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 | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/branch.c b/branch.c
> index 660097b..23cde1e 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,34 @@ 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;
> +		char *err = NULL;
> +
> +		transaction = ref_transaction_begin();
> +		if (forcing) {
> +			if (!transaction ||
> +			    ref_transaction_update(transaction, ref.buf, sha1,
> +						   NULL, 0, 0) ||
> +			    ref_transaction_commit(transaction, msg, &err))
> +			  die_errno(_("%s: failed to write ref: %s"),
> +				    ref.buf, err);
> +		} else {
> +			if (!transaction ||
> +			    ref_transaction_create(transaction, ref.buf, sha1,
> +						   0) ||
> +			    ref_transaction_commit(transaction, msg, &err))
> +			  die_errno(_("%s: failed to write ref: %s"),
> +				    ref.buf, err);
> +		}

You've got some indentation problems above.

But actually, there seems like a lot of duplicated code here.  Couldn't
you instead do a single block with have_old set based on forcing:

    ref_transaction_update(transaction, ref.buf, sha1,
			   null_sha1, 0, !forcing)

?

> +	}
> +
>  	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);
>  }
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v3 16/19] branch.c: use ref transaction for all ref updates

[Ronnie Sahlberg]

On Fri, Apr 25, 2014 at 4:16 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
>> 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 | 39 +++++++++++++++++++++++++--------------
>>  1 file changed, 25 insertions(+), 14 deletions(-)
>>
>> diff --git a/branch.c b/branch.c
>> index 660097b..23cde1e 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,34 @@ 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;
>> +             char *err = NULL;
>> +
>> +             transaction = ref_transaction_begin();
>> +             if (forcing) {
>> +                     if (!transaction ||
>> +                         ref_transaction_update(transaction, ref.buf, sha1,
>> +                                                NULL, 0, 0) ||
>> +                         ref_transaction_commit(transaction, msg, &err))
>> +                       die_errno(_("%s: failed to write ref: %s"),
>> +                                 ref.buf, err);
>> +             } else {
>> +                     if (!transaction ||
>> +                         ref_transaction_create(transaction, ref.buf, sha1,
>> +                                                0) ||
>> +                         ref_transaction_commit(transaction, msg, &err))
>> +                       die_errno(_("%s: failed to write ref: %s"),
>> +                                 ref.buf, err);
>> +             }
>
> You've got some indentation problems above.
>
> But actually, there seems like a lot of duplicated code here.  Couldn't
> you instead do a single block with have_old set based on forcing:
>
>     ref_transaction_update(transaction, ref.buf, sha1,
>                            null_sha1, 0, !forcing)
>
> ?

Done, thanks.


I am not sure how I feel about using _update to create new refs
since we already have ref_transaction_create for that purpose.

ref_transaction_update can either be used to update an existing ref
or it can be used to create new refs, either by passing have_old==0
or by passing old_sha1==null_sha1 and have_old==1

Maybe the api would be cleaner if we would change it so that update
and create does
not overlap and thus change _update so that it can only modify refs
that must already exist ?



>
>> +     }
>> +
>>       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);
>>  }
>>
>
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

Re: [PATCH v3 16/19] branch.c: use ref transaction for all ref updates

[Michael Haggerty]

On 04/28/2014 09:16 PM, Ronnie Sahlberg wrote:
> On Fri, Apr 25, 2014 at 4:16 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
>>> 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 | 39 +++++++++++++++++++++++++--------------
>>>  1 file changed, 25 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/branch.c b/branch.c
>>> index 660097b..23cde1e 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,34 @@ 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;
>>> +             char *err = NULL;
>>> +
>>> +             transaction = ref_transaction_begin();
>>> +             if (forcing) {
>>> +                     if (!transaction ||
>>> +                         ref_transaction_update(transaction, ref.buf, sha1,
>>> +                                                NULL, 0, 0) ||
>>> +                         ref_transaction_commit(transaction, msg, &err))
>>> +                       die_errno(_("%s: failed to write ref: %s"),
>>> +                                 ref.buf, err);
>>> +             } else {
>>> +                     if (!transaction ||
>>> +                         ref_transaction_create(transaction, ref.buf, sha1,
>>> +                                                0) ||
>>> +                         ref_transaction_commit(transaction, msg, &err))
>>> +                       die_errno(_("%s: failed to write ref: %s"),
>>> +                                 ref.buf, err);
>>> +             }
>>
>> You've got some indentation problems above.
>>
>> But actually, there seems like a lot of duplicated code here.  Couldn't
>> you instead do a single block with have_old set based on forcing:
>>
>>     ref_transaction_update(transaction, ref.buf, sha1,
>>                            null_sha1, 0, !forcing)
>>
>> ?
> 
> Done, thanks.
> 
> 
> I am not sure how I feel about using _update to create new refs
> since we already have ref_transaction_create for that purpose.
> 
> ref_transaction_update can either be used to update an existing ref
> or it can be used to create new refs, either by passing have_old==0
> or by passing old_sha1==null_sha1 and have_old==1

Hold onto your socks then, because I think in the future update() should
get a have_new parameter too.  That way it can also be used to verify
the current value of a reference by passing have_old=1, have_new=0
without also re-setting the reference unnecessarily like now.  Though I
admit, have_old=have_new=0 might *not* be so useful :-)

> Maybe the api would be cleaner if we would change it so that update
> and create does
> not overlap and thus change _update so that it can only modify refs
> that must already exist ?

I have no compunctions about using update() to create or delete a
reference.  My point of view is that update() is the general case, and
create() and delete() are special-cases that exist only for the
convenience of callers.  For example, our future pluggable backends
might only have to implement update(), and the other two functions could
delegate to it at the abstract layer.

Plus, making this stricter would make it impossible to eliminate
duplicate code like in the example above, which is itself evidence that
update() is a useful abstraction.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v3 16/19] branch.c: use ref transaction for all ref updates

[Ronnie Sahlberg]

On Tue, Apr 29, 2014 at 2:35 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 04/28/2014 09:16 PM, Ronnie Sahlberg wrote:
>> On Fri, Apr 25, 2014 at 4:16 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
>>>> 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 | 39 +++++++++++++++++++++++++--------------
>>>>  1 file changed, 25 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/branch.c b/branch.c
>>>> index 660097b..23cde1e 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,34 @@ 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;
>>>> +             char *err = NULL;
>>>> +
>>>> +             transaction = ref_transaction_begin();
>>>> +             if (forcing) {
>>>> +                     if (!transaction ||
>>>> +                         ref_transaction_update(transaction, ref.buf, sha1,
>>>> +                                                NULL, 0, 0) ||
>>>> +                         ref_transaction_commit(transaction, msg, &err))
>>>> +                       die_errno(_("%s: failed to write ref: %s"),
>>>> +                                 ref.buf, err);
>>>> +             } else {
>>>> +                     if (!transaction ||
>>>> +                         ref_transaction_create(transaction, ref.buf, sha1,
>>>> +                                                0) ||
>>>> +                         ref_transaction_commit(transaction, msg, &err))
>>>> +                       die_errno(_("%s: failed to write ref: %s"),
>>>> +                                 ref.buf, err);
>>>> +             }
>>>
>>> You've got some indentation problems above.
>>>
>>> But actually, there seems like a lot of duplicated code here.  Couldn't
>>> you instead do a single block with have_old set based on forcing:
>>>
>>>     ref_transaction_update(transaction, ref.buf, sha1,
>>>                            null_sha1, 0, !forcing)
>>>
>>> ?
>>
>> Done, thanks.
>>
>>
>> I am not sure how I feel about using _update to create new refs
>> since we already have ref_transaction_create for that purpose.
>>
>> ref_transaction_update can either be used to update an existing ref
>> or it can be used to create new refs, either by passing have_old==0
>> or by passing old_sha1==null_sha1 and have_old==1
>
> Hold onto your socks then, because I think in the future update() should
> get a have_new parameter too.  That way it can also be used to verify
> the current value of a reference by passing have_old=1, have_new=0
> without also re-setting the reference unnecessarily like now.  Though I
> admit, have_old=have_new=0 might *not* be so useful :-)
>
>> Maybe the api would be cleaner if we would change it so that update
>> and create does
>> not overlap and thus change _update so that it can only modify refs
>> that must already exist ?
>
> I have no compunctions about using update() to create or delete a
> reference.  My point of view is that update() is the general case, and
> create() and delete() are special-cases that exist only for the
> convenience of callers.  For example, our future pluggable backends
> might only have to implement update(), and the other two functions could
> delegate to it at the abstract layer.
>
> Plus, making this stricter would make it impossible to eliminate
> duplicate code like in the example above, which is itself evidence that
> update() is a useful abstraction.
>

Ok, Fair enough.
In that case, in the future we should change
ref_transaction_create/ref_transaction_delete to just
become wrappers that invoke ref_transaction_update.


> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

Re: [PATCH v3 16/19] branch.c: use ref transaction for all ref updates

[Junio C Hamano]

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

> I have no compunctions about using update() to create or delete a
> reference.  My point of view is that update() is the general case, and
> create() and delete() are special-cases that exist only for the
> convenience of callers.  For example, our future pluggable backends
> might only have to implement update(), and the other two functions could
> delegate to it at the abstract layer.

Sounds like a sensible position.

Thanks for commenting, Michael, and thanks for working on this,
Ronnie.

[PATCH v3 17/19] refs.c: change update_ref to use a transaction

[Ronnie Sahlberg]

Change the update_ref helper function to use a ref transaction internally.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 1557c3c..95df4a9 100644
--- a/refs.c
+++ b/refs.c
@@ -3397,11 +3397,26 @@ 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;
+	char *err = NULL;
+
+	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); break;
+		case UPDATE_REFS_DIE_ON_ERR: die(str, refname, err); break;
+		case UPDATE_REFS_QUIET_ON_ERR: break;
+		free(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)
-- 
1.9.1.521.g5dc89fa

Re: [PATCH v3 17/19] refs.c: change update_ref to use a transaction

[Michael Haggerty]

On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
> Change the update_ref helper function to use a ref transaction internally.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  refs.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 1557c3c..95df4a9 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3397,11 +3397,26 @@ 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;
> +	char *err = NULL;
> +
> +	t = ref_transaction_begin();
> +	if ((!t ||
> +	    ref_transaction_update(t, refname, sha1, oldval, flags,
> +				   !!oldval)) ||
> +	    (ref_transaction_commit(t, action, &err) && !(t = NULL))) {

You seem to have extra parentheses around the first || subexpression.

You also don't need the parentheses around the && expression because &&
binds more tightly than ||.

But using "!(t = NULL)" in the middle of a conditional expression is
pretty obscure.  I think you will change this in a later patch, so I
will defer my comments.

> +	     const char *str = "update_ref failed for ref '%s': %s";

Indentation error.

> +
> +		ref_transaction_rollback(t);
> +		switch (onerr) {
> +		case UPDATE_REFS_MSG_ON_ERR: error(str, refname, err); break;
> +		case UPDATE_REFS_DIE_ON_ERR: die(str, refname, err); break;
> +		case UPDATE_REFS_QUIET_ON_ERR: break;
> +		free(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)
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v3 17/19] refs.c: change update_ref to use a transaction

[Ronnie Sahlberg]

Thanks.

On Fri, Apr 25, 2014 at 4:28 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
>> Change the update_ref helper function to use a ref transaction internally.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  refs.c | 23 +++++++++++++++++++----
>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 1557c3c..95df4a9 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3397,11 +3397,26 @@ 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;
>> +     char *err = NULL;
>> +
>> +     t = ref_transaction_begin();
>> +     if ((!t ||
>> +         ref_transaction_update(t, refname, sha1, oldval, flags,
>> +                                !!oldval)) ||
>> +         (ref_transaction_commit(t, action, &err) && !(t = NULL))) {

This is transient code to just show how ugly this pattern becomes when
ref_transaction_commit()
implicitly also frees the transaction.

Later patches toward the end of the series will replace this with
something nicer.
Currently in this patch series I pass a pointer to pointer for the
transaction to _commit()
but that is perhaps not optimal either.
I will try and see how it would look if _commit would not free the
transaction at all
and the caller would always have to call ref_transaction_free() explicitely.


I am thinking of something like this :

   t = ref_transaction_begin();
   if ((!t ||
       ref_transaction_update(t, ...)) ||
       ref_transaction_commit(t, ...)) {
          ...something...
   }
   ref_transaction_free(t);


>
> You seem to have extra parentheses around the first || subexpression.
>
> You also don't need the parentheses around the && expression because &&
> binds more tightly than ||.
>
> But using "!(t = NULL)" in the middle of a conditional expression is
> pretty obscure.  I think you will change this in a later patch, so I
> will defer my comments.
>
>> +          const char *str = "update_ref failed for ref '%s': %s";
>
> Indentation error.

Fixed.

>
>> +
>> +             ref_transaction_rollback(t);
>> +             switch (onerr) {
>> +             case UPDATE_REFS_MSG_ON_ERR: error(str, refname, err); break;
>> +             case UPDATE_REFS_DIE_ON_ERR: die(str, refname, err); break;
>> +             case UPDATE_REFS_QUIET_ON_ERR: break;
>> +             free(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)
>>
>
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

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

[Ronnie Sahlberg]

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

diff --git a/refs.c b/refs.c
index 95df4a9..ffa9c83 100644
--- a/refs.c
+++ b/refs.c
@@ -3452,8 +3452,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;
+	}
 
 	if (err)
 		*err = NULL;
-- 
1.9.1.521.g5dc89fa

[PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer

[Ronnie Sahlberg]

Change ref_transaction_commit to take a pointer to a pointer for the
transaction. This allows us to clear the transaction pointer from within
ref_transaction_commit so that it becomes NULL in the caller.

This makes transaction handling in the callers much nicer since instead of
having to write horrible code like :
	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);

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 branch.c             |  4 ++--
 builtin/commit.c     |  2 +-
 builtin/replace.c    |  2 +-
 builtin/tag.c        |  2 +-
 builtin/update-ref.c |  2 +-
 fast-import.c        |  7 +++----
 refs.c               | 18 ++++++++++--------
 refs.h               |  3 ++-
 sequencer.c          |  7 +++----
 9 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/branch.c b/branch.c
index 23cde1e..5d68467 100644
--- a/branch.c
+++ b/branch.c
@@ -303,14 +303,14 @@ void create_branch(const char *head,
 			if (!transaction ||
 			    ref_transaction_update(transaction, ref.buf, sha1,
 						   NULL, 0, 0) ||
-			    ref_transaction_commit(transaction, msg, &err))
+			    ref_transaction_commit(&transaction, msg, &err))
 			  die_errno(_("%s: failed to write ref: %s"),
 				    ref.buf, err);
 		} else {
 			if (!transaction ||
 			    ref_transaction_create(transaction, ref.buf, sha1,
 						   0) ||
-			    ref_transaction_commit(transaction, msg, &err))
+			    ref_transaction_commit(&transaction, msg, &err))
 			  die_errno(_("%s: failed to write ref: %s"),
 				    ref.buf, err);
 		}
diff --git a/builtin/commit.c b/builtin/commit.c
index 7e4c306..3142827 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1682,7 +1682,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 				   current_head ? 
 				   current_head->object.sha1 : NULL,
 				   0, !!current_head) ||
-	    ref_transaction_commit(transaction, sb.buf, &err)) {
+	    ref_transaction_commit(&transaction, sb.buf, &err)) {
 		rollback_index_files();
 		die(_("HEAD: cannot update ref: %s"), err);
 	}
diff --git a/builtin/replace.c b/builtin/replace.c
index cf0f56d..51e9ddf 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -162,7 +162,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref, repl, prev,
 				   0, !is_null_sha1(prev)) ||
-	    ref_transaction_commit(transaction, NULL, &err))
+	    ref_transaction_commit(&transaction, NULL, &err))
 	  die(_("%s: failed to replace ref: %s"), ref, err);
 
 	return 0;
diff --git a/builtin/tag.c b/builtin/tag.c
index dd53fb4..60b57a1 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -646,7 +646,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, object, prev,
 				   0, !is_null_sha1(prev)) ||
-	    ref_transaction_commit(transaction, NULL, &err))
+	    ref_transaction_commit(&transaction, NULL, &err))
 	  die(_("%s: cannot update the ref: %s"), ref.buf, err);
 	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 a600ab3..4a0901d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -373,7 +373,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, msg, &err))
 			die("update_ref failed: %s", err);
 		return 0;
 	}
diff --git a/fast-import.c b/fast-import.c
index a2b05fa..3ce2f47 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1703,11 +1703,10 @@ 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);
 		return error("Unable to update branch %s: %s", b->name, err);
 	}
diff --git a/refs.c b/refs.c
index ffa9c83..0b60250 100644
--- a/refs.c
+++ b/refs.c
@@ -3401,10 +3401,10 @@ int update_ref(const char *action, const char *refname,
 	char *err = NULL;
 
 	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);
@@ -3444,16 +3444,17 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 	return 0;
 }
 
-int ref_transaction_commit(struct ref_transaction *transaction,
+int ref_transaction_commit(struct ref_transaction **transaction,
 			   const char *msg, char **err)
 {
 	int ret = 0, delnum = 0, i;
 	const char **delnames;
-	int n = transaction->nr;
-	struct ref_update **updates = transaction->updates;
+	int n = (*transaction)->nr;
+	struct ref_update **updates = (*transaction)->updates;
 
 	if (!n) {
-		ref_transaction_free(transaction);
+		ref_transaction_free(*transaction);
+		*transaction = NULL;
 		return 0;
 	}
 
@@ -3527,7 +3528,8 @@ cleanup:
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
 	free(delnames);
-	ref_transaction_free(transaction);
+	ref_transaction_free(*transaction);
+	*transaction = NULL;
 	return ret;
 }
 
diff --git a/refs.h b/refs.h
index 8135131..85127f2 100644
--- a/refs.h
+++ b/refs.h
@@ -270,8 +270,9 @@ int ref_transaction_delete(struct ref_transaction *transaction,
  * problem.  The ref_transaction is freed by this function.
  * If error is non-NULL it will return an error string that describes
  * why a commit failed. This string must be free()ed by the caller.
+ * *transaction is reset to NULL in this call.
  */
-int ref_transaction_commit(struct ref_transaction *transaction,
+int ref_transaction_commit(struct ref_transaction **transaction,
 			   const char *msg, char **err);
 
 /** Lock a ref and then write its file */
diff --git a/sequencer.c b/sequencer.c
index 7d59f58..3f6eced 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -283,11 +283,10 @@ 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);
 		strbuf_release(&sb);
 		return error(_("HEAD: Could not fast-forward: %s\n"), err);
-- 
1.9.1.521.g5dc89fa

Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer

[Michael Haggerty]

On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
> Change ref_transaction_commit to take a pointer to a pointer for the
> transaction. This allows us to clear the transaction pointer from within
> ref_transaction_commit so that it becomes NULL in the caller.
> 
> This makes transaction handling in the callers much nicer since instead of
> having to write horrible code like :
> 	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);

I understand the motivation for this change, but passing
pointer-to-pointer is unconventional in a case like this.  Unfortunately
I ran out of steam for the night before I could think about alternatives.

Regarding patches 01-18, I agree with Jonathan's comments and made a few
of my own.  I definitely don't think that code like "!(t = NULL)" should
stay in our code base as more than a transitional state.  But I'd have
to play with the alternatives before I can form an opinion on your
suggestion in this patch.

Thanks for working on this!

Michael

> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  branch.c             |  4 ++--
>  builtin/commit.c     |  2 +-
>  builtin/replace.c    |  2 +-
>  builtin/tag.c        |  2 +-
>  builtin/update-ref.c |  2 +-
>  fast-import.c        |  7 +++----
>  refs.c               | 18 ++++++++++--------
>  refs.h               |  3 ++-
>  sequencer.c          |  7 +++----
>  9 files changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/branch.c b/branch.c
> index 23cde1e..5d68467 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -303,14 +303,14 @@ void create_branch(const char *head,
>  			if (!transaction ||
>  			    ref_transaction_update(transaction, ref.buf, sha1,
>  						   NULL, 0, 0) ||
> -			    ref_transaction_commit(transaction, msg, &err))
> +			    ref_transaction_commit(&transaction, msg, &err))
>  			  die_errno(_("%s: failed to write ref: %s"),
>  				    ref.buf, err);
>  		} else {
>  			if (!transaction ||
>  			    ref_transaction_create(transaction, ref.buf, sha1,
>  						   0) ||
> -			    ref_transaction_commit(transaction, msg, &err))
> +			    ref_transaction_commit(&transaction, msg, &err))
>  			  die_errno(_("%s: failed to write ref: %s"),
>  				    ref.buf, err);
>  		}
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 7e4c306..3142827 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1682,7 +1682,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  				   current_head ? 
>  				   current_head->object.sha1 : NULL,
>  				   0, !!current_head) ||
> -	    ref_transaction_commit(transaction, sb.buf, &err)) {
> +	    ref_transaction_commit(&transaction, sb.buf, &err)) {
>  		rollback_index_files();
>  		die(_("HEAD: cannot update ref: %s"), err);
>  	}
> diff --git a/builtin/replace.c b/builtin/replace.c
> index cf0f56d..51e9ddf 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -162,7 +162,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,
>  	if (!transaction ||
>  	    ref_transaction_update(transaction, ref, repl, prev,
>  				   0, !is_null_sha1(prev)) ||
> -	    ref_transaction_commit(transaction, NULL, &err))
> +	    ref_transaction_commit(&transaction, NULL, &err))
>  	  die(_("%s: failed to replace ref: %s"), ref, err);
>  
>  	return 0;
> diff --git a/builtin/tag.c b/builtin/tag.c
> index dd53fb4..60b57a1 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -646,7 +646,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	if (!transaction ||
>  	    ref_transaction_update(transaction, ref.buf, object, prev,
>  				   0, !is_null_sha1(prev)) ||
> -	    ref_transaction_commit(transaction, NULL, &err))
> +	    ref_transaction_commit(&transaction, NULL, &err))
>  	  die(_("%s: cannot update the ref: %s"), ref.buf, err);
>  	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 a600ab3..4a0901d 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -373,7 +373,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, msg, &err))
>  			die("update_ref failed: %s", err);
>  		return 0;
>  	}
> diff --git a/fast-import.c b/fast-import.c
> index a2b05fa..3ce2f47 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1703,11 +1703,10 @@ 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);
>  		return error("Unable to update branch %s: %s", b->name, err);
>  	}
> diff --git a/refs.c b/refs.c
> index ffa9c83..0b60250 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3401,10 +3401,10 @@ int update_ref(const char *action, const char *refname,
>  	char *err = NULL;
>  
>  	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);
> @@ -3444,16 +3444,17 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
>  	return 0;
>  }
>  
> -int ref_transaction_commit(struct ref_transaction *transaction,
> +int ref_transaction_commit(struct ref_transaction **transaction,
>  			   const char *msg, char **err)
>  {
>  	int ret = 0, delnum = 0, i;
>  	const char **delnames;
> -	int n = transaction->nr;
> -	struct ref_update **updates = transaction->updates;
> +	int n = (*transaction)->nr;
> +	struct ref_update **updates = (*transaction)->updates;
>  
>  	if (!n) {
> -		ref_transaction_free(transaction);
> +		ref_transaction_free(*transaction);
> +		*transaction = NULL;
>  		return 0;
>  	}
>  
> @@ -3527,7 +3528,8 @@ cleanup:
>  		if (updates[i]->lock)
>  			unlock_ref(updates[i]->lock);
>  	free(delnames);
> -	ref_transaction_free(transaction);
> +	ref_transaction_free(*transaction);
> +	*transaction = NULL;
>  	return ret;
>  }
>  
> diff --git a/refs.h b/refs.h
> index 8135131..85127f2 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -270,8 +270,9 @@ int ref_transaction_delete(struct ref_transaction *transaction,
>   * problem.  The ref_transaction is freed by this function.
>   * If error is non-NULL it will return an error string that describes
>   * why a commit failed. This string must be free()ed by the caller.
> + * *transaction is reset to NULL in this call.
>   */
> -int ref_transaction_commit(struct ref_transaction *transaction,
> +int ref_transaction_commit(struct ref_transaction **transaction,
>  			   const char *msg, char **err);
>  
>  /** Lock a ref and then write its file */
> diff --git a/sequencer.c b/sequencer.c
> index 7d59f58..3f6eced 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -283,11 +283,10 @@ 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);
>  		strbuf_release(&sb);
>  		return error(_("HEAD: Could not fast-forward: %s\n"), err);
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer

[Ronnie Sahlberg]

On Fri, Apr 25, 2014 at 6:31 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
>> Change ref_transaction_commit to take a pointer to a pointer for the
>> transaction. This allows us to clear the transaction pointer from within
>> ref_transaction_commit so that it becomes NULL in the caller.
>>
>> This makes transaction handling in the callers much nicer since instead of
>> having to write horrible code like :
>>       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);
>
> I understand the motivation for this change, but passing
> pointer-to-pointer is unconventional in a case like this.  Unfortunately
> I ran out of steam for the night before I could think about alternatives.

I see.
Yes passing a pointer to pointer is not ideal.
But I still want to be able to use the pattern
       t = ref_transaction_begin();
       if (!t ||
           ref_transaction_update(t, ...) ||
           ref_transaction_commit(t, ...)) {
               ref_transaction_rollback(t);

Maybe the problem is that ref_transaction_commit() implicitely also
frees the transaction.


What about changing ref_transaction_commit() would NOT free the
transaction and thus a caller would
always have to explicitely free the transaction afterwards?

Something like this :
       t = ref_transaction_begin();
       if (!t ||
           ref_transaction_update(t, ...) ||
           ref_transaction_commit(&t, ...)) {
               ref_transaction_rollback(t);
       }
       ref_transaction_free(t);


>
> Regarding patches 01-18, I agree with Jonathan's comments and made a few
> of my own.  I definitely don't think that code like "!(t = NULL)" should
> stay in our code base as more than a transitional state.  But I'd have
> to play with the alternatives before I can form an opinion on your
> suggestion in this patch.
>
> Thanks for working on this!
>
> Michael
>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  branch.c             |  4 ++--
>>  builtin/commit.c     |  2 +-
>>  builtin/replace.c    |  2 +-
>>  builtin/tag.c        |  2 +-
>>  builtin/update-ref.c |  2 +-
>>  fast-import.c        |  7 +++----
>>  refs.c               | 18 ++++++++++--------
>>  refs.h               |  3 ++-
>>  sequencer.c          |  7 +++----
>>  9 files changed, 24 insertions(+), 23 deletions(-)
>>
>> diff --git a/branch.c b/branch.c
>> index 23cde1e..5d68467 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -303,14 +303,14 @@ void create_branch(const char *head,
>>                       if (!transaction ||
>>                           ref_transaction_update(transaction, ref.buf, sha1,
>>                                                  NULL, 0, 0) ||
>> -                         ref_transaction_commit(transaction, msg, &err))
>> +                         ref_transaction_commit(&transaction, msg, &err))
>>                         die_errno(_("%s: failed to write ref: %s"),
>>                                   ref.buf, err);
>>               } else {
>>                       if (!transaction ||
>>                           ref_transaction_create(transaction, ref.buf, sha1,
>>                                                  0) ||
>> -                         ref_transaction_commit(transaction, msg, &err))
>> +                         ref_transaction_commit(&transaction, msg, &err))
>>                         die_errno(_("%s: failed to write ref: %s"),
>>                                   ref.buf, err);
>>               }
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 7e4c306..3142827 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1682,7 +1682,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>                                  current_head ?
>>                                  current_head->object.sha1 : NULL,
>>                                  0, !!current_head) ||
>> -         ref_transaction_commit(transaction, sb.buf, &err)) {
>> +         ref_transaction_commit(&transaction, sb.buf, &err)) {
>>               rollback_index_files();
>>               die(_("HEAD: cannot update ref: %s"), err);
>>       }
>> diff --git a/builtin/replace.c b/builtin/replace.c
>> index cf0f56d..51e9ddf 100644
>> --- a/builtin/replace.c
>> +++ b/builtin/replace.c
>> @@ -162,7 +162,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,
>>       if (!transaction ||
>>           ref_transaction_update(transaction, ref, repl, prev,
>>                                  0, !is_null_sha1(prev)) ||
>> -         ref_transaction_commit(transaction, NULL, &err))
>> +         ref_transaction_commit(&transaction, NULL, &err))
>>         die(_("%s: failed to replace ref: %s"), ref, err);
>>
>>       return 0;
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index dd53fb4..60b57a1 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -646,7 +646,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>       if (!transaction ||
>>           ref_transaction_update(transaction, ref.buf, object, prev,
>>                                  0, !is_null_sha1(prev)) ||
>> -         ref_transaction_commit(transaction, NULL, &err))
>> +         ref_transaction_commit(&transaction, NULL, &err))
>>         die(_("%s: cannot update the ref: %s"), ref.buf, err);
>>       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 a600ab3..4a0901d 100644
>> --- a/builtin/update-ref.c
>> +++ b/builtin/update-ref.c
>> @@ -373,7 +373,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, msg, &err))
>>                       die("update_ref failed: %s", err);
>>               return 0;
>>       }
>> diff --git a/fast-import.c b/fast-import.c
>> index a2b05fa..3ce2f47 100644
>> --- a/fast-import.c
>> +++ b/fast-import.c
>> @@ -1703,11 +1703,10 @@ 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);
>>               return error("Unable to update branch %s: %s", b->name, err);
>>       }
>> diff --git a/refs.c b/refs.c
>> index ffa9c83..0b60250 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3401,10 +3401,10 @@ int update_ref(const char *action, const char *refname,
>>       char *err = NULL;
>>
>>       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);
>> @@ -3444,16 +3444,17 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
>>       return 0;
>>  }
>>
>> -int ref_transaction_commit(struct ref_transaction *transaction,
>> +int ref_transaction_commit(struct ref_transaction **transaction,
>>                          const char *msg, char **err)
>>  {
>>       int ret = 0, delnum = 0, i;
>>       const char **delnames;
>> -     int n = transaction->nr;
>> -     struct ref_update **updates = transaction->updates;
>> +     int n = (*transaction)->nr;
>> +     struct ref_update **updates = (*transaction)->updates;
>>
>>       if (!n) {
>> -             ref_transaction_free(transaction);
>> +             ref_transaction_free(*transaction);
>> +             *transaction = NULL;
>>               return 0;
>>       }
>>
>> @@ -3527,7 +3528,8 @@ cleanup:
>>               if (updates[i]->lock)
>>                       unlock_ref(updates[i]->lock);
>>       free(delnames);
>> -     ref_transaction_free(transaction);
>> +     ref_transaction_free(*transaction);
>> +     *transaction = NULL;
>>       return ret;
>>  }
>>
>> diff --git a/refs.h b/refs.h
>> index 8135131..85127f2 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -270,8 +270,9 @@ int ref_transaction_delete(struct ref_transaction *transaction,
>>   * problem.  The ref_transaction is freed by this function.
>>   * If error is non-NULL it will return an error string that describes
>>   * why a commit failed. This string must be free()ed by the caller.
>> + * *transaction is reset to NULL in this call.
>>   */
>> -int ref_transaction_commit(struct ref_transaction *transaction,
>> +int ref_transaction_commit(struct ref_transaction **transaction,
>>                          const char *msg, char **err);
>>
>>  /** Lock a ref and then write its file */
>> diff --git a/sequencer.c b/sequencer.c
>> index 7d59f58..3f6eced 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -283,11 +283,10 @@ 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);
>>               strbuf_release(&sb);
>>               return error(_("HEAD: Could not fast-forward: %s\n"), err);
>>
>
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer

[Michael Haggerty]

On 04/28/2014 07:59 PM, Ronnie Sahlberg wrote:
> On Fri, Apr 25, 2014 at 6:31 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
>>> Change ref_transaction_commit to take a pointer to a pointer for the
>>> transaction. This allows us to clear the transaction pointer from within
>>> ref_transaction_commit so that it becomes NULL in the caller.
>>>
>>> This makes transaction handling in the callers much nicer since instead of
>>> having to write horrible code like :
>>>       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);
>>
>> I understand the motivation for this change, but passing
>> pointer-to-pointer is unconventional in a case like this.  Unfortunately
>> I ran out of steam for the night before I could think about alternatives.
> 
> I see.
> Yes passing a pointer to pointer is not ideal.
> But I still want to be able to use the pattern
>        t = ref_transaction_begin();
>        if (!t ||
>            ref_transaction_update(t, ...) ||
>            ref_transaction_commit(t, ...)) {
>                ref_transaction_rollback(t);
> 
> Maybe the problem is that ref_transaction_commit() implicitely also
> frees the transaction.
> 
> 
> What about changing ref_transaction_commit() would NOT free the
> transaction and thus a caller would
> always have to explicitely free the transaction afterwards?
> 
> Something like this :
>        t = ref_transaction_begin();
>        if (!t ||
>            ref_transaction_update(t, ...) ||
>            ref_transaction_commit(&t, ...)) {

You wouldn't need the "&" here then, right?

>                ref_transaction_rollback(t);
>        }
>        ref_transaction_free(t);

That sounds like a better solution.  We would want to make sure that
ref_transaction_commit() / ref_transaction_rollback() leaves the
ref_transaction in a state that if it is accidentally passed to
ref_transaction_update() or its friends, the function calls die("BUG: ...").

Unless we want to make ref_transaction objects reusable.  But then we
would need an explicit "allocation" step in the boilerplate code:

    t = ref_transaction_alloc();
    while (something) {
            if (ref_transaction_begin(t) ||
                 ref_transaction_update(t, ...) ||
                 ref_transaction_commit(t, ...)) {
                    ref_transaction_rollback(t);
            }
    }
    ref_transaction_free(t);

Note that ref_transaction_begin() should in this case be converted to
return 0-on-OK, negative-on-error for consistency.

This would bring us back to the familiar pattern alloc...use...free.

I was going to say that the extra boilerplate is not worth it, and
anyway reusing ref_transaction objects won't save any significant work.
 But then it occurred to me that ref_transaction_alloc() might be a
place to do more expensive work, like creating a connection to a
database, so reuse could potentially be a bigger win.

All in all, either way is OK with me.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer

[Ronnie Sahlberg]

On Tue, Apr 29, 2014 at 2:25 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 04/28/2014 07:59 PM, Ronnie Sahlberg wrote:
>> On Fri, Apr 25, 2014 at 6:31 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
>>>> Change ref_transaction_commit to take a pointer to a pointer for the
>>>> transaction. This allows us to clear the transaction pointer from within
>>>> ref_transaction_commit so that it becomes NULL in the caller.
>>>>
>>>> This makes transaction handling in the callers much nicer since instead of
>>>> having to write horrible code like :
>>>>       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);
>>>
>>> I understand the motivation for this change, but passing
>>> pointer-to-pointer is unconventional in a case like this.  Unfortunately
>>> I ran out of steam for the night before I could think about alternatives.
>>
>> I see.
>> Yes passing a pointer to pointer is not ideal.
>> But I still want to be able to use the pattern
>>        t = ref_transaction_begin();
>>        if (!t ||
>>            ref_transaction_update(t, ...) ||
>>            ref_transaction_commit(t, ...)) {
>>                ref_transaction_rollback(t);
>>
>> Maybe the problem is that ref_transaction_commit() implicitely also
>> frees the transaction.
>>
>>
>> What about changing ref_transaction_commit() would NOT free the
>> transaction and thus a caller would
>> always have to explicitely free the transaction afterwards?
>>
>> Something like this :
>>        t = ref_transaction_begin();
>>        if (!t ||
>>            ref_transaction_update(t, ...) ||
>>            ref_transaction_commit(&t, ...)) {
>
> You wouldn't need the "&" here then, right?
>
>>                ref_transaction_rollback(t);
>>        }
>>        ref_transaction_free(t);
>
> That sounds like a better solution.  We would want to make sure that
> ref_transaction_commit() / ref_transaction_rollback() leaves the
> ref_transaction in a state that if it is accidentally passed to
> ref_transaction_update() or its friends, the function calls die("BUG: ...").

Thanks!

Good idea.
I will add a transaction->status field that can track OPEN/CLOSED/ERROR
and die(BUG:...) appropriately in _commit/_create/_delete/_update if
it has the wrong value.


>
> Unless we want to make ref_transaction objects reusable.  But then we
> would need an explicit "allocation" step in the boilerplate code:
>
>     t = ref_transaction_alloc();
>     while (something) {
>             if (ref_transaction_begin(t) ||
>                  ref_transaction_update(t, ...) ||
>                  ref_transaction_commit(t, ...)) {
>                     ref_transaction_rollback(t);
>             }
>     }
>     ref_transaction_free(t);
>
> Note that ref_transaction_begin() should in this case be converted to
> return 0-on-OK, negative-on-error for consistency.
>
> This would bring us back to the familiar pattern alloc...use...free.
>
> I was going to say that the extra boilerplate is not worth it, and
> anyway reusing ref_transaction objects won't save any significant work.
>  But then it occurred to me that ref_transaction_alloc() might be a
> place to do more expensive work, like creating a connection to a
> database, so reuse could potentially be a bigger win.

ACK, but I don't think we need reusable transaction yet. Once the API
is cleaned up
it should be reasonably easy to add in the future if we see a need for it.
Sounds reasonable to you ?


>
> All in all, either way is OK with me.
>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer

[Michael Haggerty]

On 04/29/2014 08:58 PM, Ronnie Sahlberg wrote:
> On Tue, Apr 29, 2014 at 2:25 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> [...]
>> Unless we want to make ref_transaction objects reusable.  [...]
> ACK, but I don't think we need reusable transaction yet. Once the API
> is cleaned up
> it should be reasonably easy to add in the future if we see a need for it.
> Sounds reasonable to you ?

Yes, reusable transaction objects would be pretty straightforward to
retrofit, so I agree that there is no need to implement it now.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v3 00/19] Use ref transactions from most callers

[Jonathan Nieder]

Ronnie Sahlberg wrote:

> This patch series changes most of the places where the ref functions for
> locking and writing refs to instead use the new ref transaction API.

Thanks.  Is this series based against mh/ref-transaction from "next"?

[...]
> I think I have covered all issues raised on the previous reviews and also
> done a bunch of cleanups and improvements to the transaction API.

Whoops, sorry I replied to an old message.

Re: [PATCH v3 00/19] Use ref transactions from most callers

[Ronnie Sahlberg]

On Fri, Apr 25, 2014 at 2:53 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ronnie Sahlberg wrote:
>
>> This patch series changes most of the places where the ref functions for
>> locking and writing refs to instead use the new ref transaction API.
>
> Thanks.  Is this series based against mh/ref-transaction from "next"?

Yes, against mh/ref-transaction


>
> [...]
>> I think I have covered all issues raised on the previous reviews and also
>> done a bunch of cleanups and improvements to the transaction API.
>
> Whoops, sorry I replied to an old message.