[PATCH 00/11] Use ref transactions from most callers

[PATCH 00/11] 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.


Ronnie Sahlberg (11):
  refs.c: constify the sha arguments for
    ref_transaction_create|delete|update
  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
  walker.c: use ref transaction for ref updates

 branch.c             | 16 ++++++++++------
 builtin/commit.c     | 22 ++++++++++++----------
 builtin/replace.c    | 15 +++++++++------
 builtin/tag.c        | 15 +++++++++------
 builtin/update-ref.c | 20 +++++++++++++-------
 fast-import.c        | 23 +++++++++++++++--------
 refs.c               | 44 ++++++++++++++++++++++++++++++--------------
 refs.h               | 25 +++++++++++++------------
 sequencer.c          | 17 +++++++++++++----
 walker.c             | 45 ++++++++++++++++++++-------------------------
 10 files changed, 144 insertions(+), 98 deletions(-)

-- 
1.9.1.513.gd486896

[PATCH 01/11] 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.513.gd486896

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

[Michael Haggerty]

On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:
> 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.

Good, thanks.

Michael

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

[PATCH 02/11] 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 | 11 +++++++----
 refs.c               |  9 +++++++--
 refs.h               | 10 +++++-----
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 405267f..12bfacc 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -197,8 +197,10 @@ 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("failed transaction update for %s", refname);
+
 
 	update_flags = 0;
 	free(refname);
@@ -286,8 +288,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 138ab70..da1761d 100644
--- a/refs.c
+++ b/refs.c
@@ -3327,19 +3327,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)
+		return error("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 892c5b6..00e4f7b 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.513.gd486896

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

[Michael Haggerty]

On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:
> Update ref_transaction_update() do some basic error checking and return
> true on error. Update all callers to check ref_transaction_update() for error.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/update-ref.c | 11 +++++++----
>  refs.c               |  9 +++++++--
>  refs.h               | 10 +++++-----
>  3 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 405267f..12bfacc 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -197,8 +197,10 @@ 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("failed transaction update for %s", refname);
> +
>  
>  	update_flags = 0;
>  	free(refname);
> @@ -286,8 +288,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 138ab70..da1761d 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3327,19 +3327,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)
> +		return error("have_old is true but old_sha1 is NULL");

The function documentation doesn't seem to allow old_sha1 == NULL.  That
means that calling it that way is a bug, not an error.  Therefore, I
think this should result in

    die("BUG: 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 892c5b6..00e4f7b 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
> 


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

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

[Jonathan Nieder]

Hi,

Ronnie Sahlberg wrote:

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

Micronit: nonzero, not true.  (true tends to mean '1' while here we
have the usual error return of -1.  It's kind of annoying that C
doesn't have a nice way to distinguish between the usual int return of
0 for success and the usual bool return of true for success.)

Looks like a good change.  Some tiny nitpicks below.

[...]
> --- 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,
> +int ref_transaction_update(struct ref_transaction *transaction,

The comment above the prototype doesn't tell me:

When should the caller expect ref_transaction_update to return an
error?  What does an error mean: is it always a sign of a bug in the
caller, or can it be due to some other problem?  What guarantees does
the caller have about the state after an error --- is it just "Things
will be in a sane state so you can free resources and exit", or will
the ref_transaction_update() have been essentially a no-op allowing
the caller to continue?

[...]
> --- a/refs.c
> +++ b/refs.c
> @@ -3327,19 +3327,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)
> +		return error("have_old is true but old_sha1 is NULL");

I agree with Michael that the error message should start with "BUG:"
so humans encountering this know to contact the list instead of
blaming themselves.

Returning error instead of die-ing seems like a nice thing that make
it easier to make git valgrind-clean some day.  Others might disagree
with me about whether that's worthwhile, but I think it's a good
change. :)

[...]
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -197,8 +197,10 @@ 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("failed transaction update for %s", refname);

ref_transaction_update already printed an error, but of course that's
no guarantee that bugs in ref_transaction_update will not cause it
to fail without printing a message in the future.  And the extra
context for the error might be nice (but why not print refname in
the message from ref_transaction_update instead?).

Is the plan for ref_transaction_update to be able to fail for
other reasons some day?  What is the contract --- do we need a
human-readable, translatable message here, or is a "this can't
happen" BUG message fine?

I'd be fine with

		die("BUG: failed transa...

or

		/* ref_transaction_update already printed a message */
		exit(128)

with a slight preference for the former, for what it's worth.

[...]
> @@ -286,8 +288,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);

Likewise.

Thanks,
Jonathan

[PATCH 03/11] 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 12bfacc..1b8a087 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -226,7 +226,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 da1761d..c46249f 100644
--- a/refs.c
+++ b/refs.c
@@ -3347,18 +3347,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))
+		return error("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 00e4f7b..8799e15 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.513.gd486896

Re: [PATCH 03/11] refs.c: change ref_transaction_create to do error checking and return status

[Michael Haggerty]

On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:
> Do basic error checking in ref_transaction_create() and make it return
> status. Update all callers to check the result of ref_transaction_create()
> 
> 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 12bfacc..1b8a087 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -226,7 +226,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))

Space between "if" and opening parenthesis.

> +		die("failed transaction create for %s", refname);
>  
>  	update_flags = 0;
>  	free(refname);
> diff --git a/refs.c b/refs.c
> index da1761d..c46249f 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3347,18 +3347,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))
> +		return error("create ref with null new_sha1");

Please die("BUG: ...").

> +
> +	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 00e4f7b..8799e15 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
> 

Michael

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

[PATCH 04/11] 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 1b8a087..6ff8b86 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -258,8 +258,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 c46249f..9cbcffa 100644
--- a/refs.c
+++ b/refs.c
@@ -3366,19 +3366,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)
+		return error("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 8799e15..7050da2 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.513.gd486896

Re: [PATCH 04/11] refs.c: ref_transaction_delete to check for error and return status

[Michael Haggerty]

On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:
> Change ref_transaction_delete() to do basic error checking and return
> status. Update all callers to check the return for ref_transaction_delete()
> 
> 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 1b8a087..6ff8b86 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -258,8 +258,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 c46249f..9cbcffa 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3366,19 +3366,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)
> +		return error("have_old is true but old_sha1 is NULL");

Ditto.

> +
> +	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 8799e15..7050da2 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
> 

Michael

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

[PATCH 05/11] 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 | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 40356e3..dbeacc5 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,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
 	struct commit_list *with_commit = NULL;
+	struct ref_transaction *transaction;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
@@ -641,11 +641,14 @@ 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)
+		die(_("%s: cannot start transaction"), ref.buf);
+	if (ref_transaction_update(transaction, ref.buf, object, prev,
+				   0, !is_null_sha1(prev)))
+		die(_("%s: cannot update transaction"), ref.buf);
+	if (ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))
+		die(_("%s: cannot commit transaction"), ref.buf);
 	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.513.gd486896

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

[Michael Haggerty]

On 04/17/2014 09:46 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 | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 40356e3..dbeacc5 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,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	const char *msgfile = NULL, *keyid = NULL;
>  	struct msg_arg msg = { 0, STRBUF_INIT };
>  	struct commit_list *with_commit = NULL;
> +	struct ref_transaction *transaction;
>  	struct option options[] = {
>  		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
>  		{ OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
> @@ -641,11 +641,14 @@ 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)
> +		die(_("%s: cannot start transaction"), ref.buf);
> +	if (ref_transaction_update(transaction, ref.buf, object, prev,
> +				   0, !is_null_sha1(prev)))
> +		die(_("%s: cannot update transaction"), ref.buf);
> +	if (ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))
> +		die(_("%s: cannot commit transaction"), ref.buf);

I wonder whether these error messages are meaningful to the user.  To
the user, it's all just a failure to create the tag, right?  Does it
help to tell the user that the error was in the "start transaction",
"update transaction", or "commit transaction" phase?

In other words, maybe it would be just as good to do

if (!transaction ||
    ref_transaction_update(transaction, ref.buf, object, prev,
			   0, !is_null_sha1(prev)) ||
    ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))
	die(_("%s: cannot update the ref"), ref.buf);

But then I would also hope that somebody (probably
ref_transaction_commit() now, but later maybe ref_transaction_update())
tells the user whether the problem was the inability to lock the
reference, or the consistency check of old_sha1, or whatever.

The next question is whether the generic error messages that the refs
code generates is good enough, or whether the caller would rather learn
about the reason for an error and generate its own, more specific error
message.  Please pay attention to this as you rewrite callers: by
relying more on centralized code, are we losing out unacceptably on
error message specificity?

Oh and by the way, given that you pass UPDATE_REFS_DIE_ON_ERR,
ref_transaction_commit() should never return a nonzero value, should it?

>  	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 06/11] 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 | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index b62420a..d8bd6ee 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -129,7 +129,7 @@ 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;
 
 	if (get_sha1(object_ref, object))
 		die("Failed to resolve '%s' as a valid ref.", object_ref);
@@ -157,11 +157,14 @@ 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)
+		die(_("%s: cannot start transaction"), ref);
+	if (ref_transaction_update(transaction, ref, repl, prev,
+				   0, !is_null_sha1(prev)))
+		die(_("%s: cannot update transaction"), ref);
+	if (ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))
+		die(_("%s: cannot commit transaction"), ref);
 
 	return 0;
 }
-- 
1.9.1.513.gd486896

Re: [PATCH 06/11] replace.c: use the ref transaction functions for updates

[Michael Haggerty]

On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:
> Update replace.c to use ref transactions for updates.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/replace.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/replace.c b/builtin/replace.c
> index b62420a..d8bd6ee 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -129,7 +129,7 @@ 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;
>  
>  	if (get_sha1(object_ref, object))
>  		die("Failed to resolve '%s' as a valid ref.", object_ref);
> @@ -157,11 +157,14 @@ 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)
> +		die(_("%s: cannot start transaction"), ref);
> +	if (ref_transaction_update(transaction, ref, repl, prev,
> +				   0, !is_null_sha1(prev)))
> +		die(_("%s: cannot update transaction"), ref);
> +	if (ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))
> +		die(_("%s: cannot commit transaction"), ref);

The same comment that I made to 05/11 applies here too (and, I predict,
to other patches yet to come).

>  
>  	return 0;
>  }
> 

Michael

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

[PATCH 07/11] commit.c: use ref transactions for updates

[Ronnie Sahlberg]

Change commit.c to use ref transactions for all ref updates.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/commit.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d9550c5..b8e4389 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1541,11 +1541,11 @@ 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;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_commit_usage, builtin_commit_options);
@@ -1667,12 +1667,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,14 +1675,22 @@ 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) {
+	transaction = ref_transaction_begin();
+	if (!transaction) {
 		rollback_index_files();
-		die(_("cannot lock HEAD ref"));
+		die(_("HEAD: cannot start transaction"));
 	}
-	if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
+	if (ref_transaction_update(transaction, "HEAD", sha1,
+				   current_head->object.sha1,
+				   0, !!current_head)) {
 		rollback_index_files();
 		die(_("cannot update HEAD ref"));
 	}
+	if (ref_transaction_commit(transaction, sb.buf,
+				   UPDATE_REFS_QUIET_ON_ERR)) {
+		rollback_index_files();
+		die(_("cannot commit HEAD ref"));
+	}
 
 	unlink(git_path("CHERRY_PICK_HEAD"));
 	unlink(git_path("REVERT_HEAD"));
-- 
1.9.1.513.gd486896

Re: [PATCH 07/11] commit.c: use ref transactions for updates

[Michael Haggerty]

On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:
> Change commit.c to use ref transactions for all ref updates.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/commit.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index d9550c5..b8e4389 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1541,11 +1541,11 @@ 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;
>  
>  	if (argc == 2 && !strcmp(argv[1], "-h"))
>  		usage_with_options(builtin_commit_usage, builtin_commit_options);
> @@ -1667,12 +1667,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);

The old version, above, contemplates that current_head might be NULL...

> -
>  	nl = strchr(sb.buf, '\n');
>  	if (nl)
>  		strbuf_setlen(&sb, nl + 1 - sb.buf);
> @@ -1681,14 +1675,22 @@ 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) {
> +	transaction = ref_transaction_begin();
> +	if (!transaction) {
>  		rollback_index_files();
> -		die(_("cannot lock HEAD ref"));
> +		die(_("HEAD: cannot start transaction"));
>  	}
> -	if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
> +	if (ref_transaction_update(transaction, "HEAD", sha1,
> +				   current_head->object.sha1,
> +				   0, !!current_head)) {

...but here you dereference current_head without checking it first.

It upsets me that the test suite didn't catch this NULL pointer
dereference.  Either

1. current_head cannot in fact be NULL, in which case the commit message
should explain that fact and the code should be simplified

or

2. the test suite is incomplete.  If so, it would be great if you would
add a test that exercises this branch of the code (and catches your
error), and then fix the error.

>  		rollback_index_files();
>  		die(_("cannot update HEAD ref"));
>  	}
> +	if (ref_transaction_commit(transaction, sb.buf,
> +				   UPDATE_REFS_QUIET_ON_ERR)) {
> +		rollback_index_files();
> +		die(_("cannot commit HEAD ref"));
> +	}
>  
>  	unlink(git_path("CHERRY_PICK_HEAD"));
>  	unlink(git_path("REVERT_HEAD"));
> 

Michael

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

Re: [PATCH 07/11] commit.c: use ref transactions for updates

[Ronnie Sahlberg]

On Sat, Apr 19, 2014 at 12:23 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:
>> Change commit.c to use ref transactions for all ref updates.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  builtin/commit.c | 22 ++++++++++++----------
>>  1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index d9550c5..b8e4389 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1541,11 +1541,11 @@ 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;
>>
>>       if (argc == 2 && !strcmp(argv[1], "-h"))
>>               usage_with_options(builtin_commit_usage, builtin_commit_options);
>> @@ -1667,12 +1667,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);
>
> The old version, above, contemplates that current_head might be NULL...
>
>> -
>>       nl = strchr(sb.buf, '\n');
>>       if (nl)
>>               strbuf_setlen(&sb, nl + 1 - sb.buf);
>> @@ -1681,14 +1675,22 @@ 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) {
>> +     transaction = ref_transaction_begin();
>> +     if (!transaction) {
>>               rollback_index_files();
>> -             die(_("cannot lock HEAD ref"));
>> +             die(_("HEAD: cannot start transaction"));
>>       }
>> -     if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
>> +     if (ref_transaction_update(transaction, "HEAD", sha1,
>> +                                current_head->object.sha1,
>> +                                0, !!current_head)) {
>
> ...but here you dereference current_head without checking it first.
>
> It upsets me that the test suite didn't catch this NULL pointer
> dereference.  Either
>
> 1. current_head cannot in fact be NULL, in which case the commit message
> should explain that fact and the code should be simplified
>
> or
>
> 2. the test suite is incomplete.  If so, it would be great if you would
> add a test that exercises this branch of the code (and catches your
> error), and then fix the error.



Current_head can in fact be NULL here but we never actually
dereference any pointer in this case since !!current_head is 0.


current_head->object.sha1 just computes current_head +
offsetof(commit, object) + offsetof(object, sha1)
so we compute and pass a non-NULL garbage pointer into ref_transaction_update()

But since !!current_head is 0 this mean we never actually dereference
this pointer.
Way to subtle for its own good.


I will change ref_transaction_update and friends to use an additional
test to detect some subset of this kind of bug :

if (!have_old && old_sha1)
   die("have_old is false but old_sha1 is not NULL");

regards
ronnie sahlberg

[PATCH 08/11] 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 | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index bde5f04..fa14ac0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,17 +272,26 @@ 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;
 
 	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);
+
+	transaction = ref_transaction_begin();
+	if (!transaction)
+		return 1;
+	if (ref_transaction_update(transaction, "HEAD", to, from,
+				   0, !unborn)) {
+		ref_transaction_rollback(transaction);
+		return 1;
+	}
+
 	strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
-	ret = write_ref_sha1(ref_lock, to, sb.buf);
+	ret = ref_transaction_commit(transaction, sb.buf,
+				     UPDATE_REFS_MSG_ON_ERR);
 	strbuf_release(&sb);
 	return ret;
 }
-- 
1.9.1.513.gd486896

[PATCH 09/11] 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 | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index fb4738d..466dfe3 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1678,36 +1678,43 @@ 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];
 
 	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);
+	transaction = ref_transaction_begin();
+	if (!transaction)
+		return error("Unable to begin transaction for %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);
+			ref_transaction_rollback(transaction);
 			return error("Branch %s is missing commits.", b->name);
 		}
 
 		if (!in_merge_bases(old_cmit, new_cmit)) {
-			unlock_ref(lock);
+			ref_transaction_rollback(transaction);
 			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);
+	if (ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
+				   0, 1)) {
+		ref_transaction_rollback(transaction);
+		return error("Unable to update transaction for %s", b->name);
+	}
+	if (ref_transaction_commit(transaction, msg,
+				   UPDATE_REFS_QUIET_ON_ERR))
+		return error("Unable to commit transaction for %s", b->name);
+
 	return 0;
 }
 
-- 
1.9.1.513.gd486896

[PATCH 10/11] branch.c: use ref transaction for all ref updates

[Ronnie Sahlberg]

Change branch.c to use ref transactions when doing updates.

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

diff --git a/branch.c b/branch.c
index 660097b..45c7766 100644
--- a/branch.c
+++ b/branch.c
@@ -226,7 +226,7 @@ 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 ref_transaction *transaction;
 	struct commit *commit;
 	unsigned char sha1[20];
 	char *real_ref, msg[PATH_MAX + 20];
@@ -286,9 +286,12 @@ void create_branch(const char *head,
 	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"));
+		transaction = ref_transaction_begin();
+		if (!transaction)
+			die_errno(_("Failed to begin transaction"));
+		if (ref_transaction_update(transaction, ref.buf, sha1, NULL,
+				   0, 0))
+			die_errno(_("Failed to update transaction"));
 	}
 
 	if (reflog)
@@ -305,8 +308,9 @@ void create_branch(const char *head,
 		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"));
+		if (ref_transaction_commit(transaction, msg,
+				     UPDATE_REFS_DIE_ON_ERR))
+			die_errno(_("Failed to commit transaction"));
 
 	strbuf_release(&ref);
 	free(real_ref);
-- 
1.9.1.513.gd486896

[PATCH 11/11] walker.c: use ref transaction for ref updates

[Ronnie Sahlberg]

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

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 walker.c | 45 ++++++++++++++++++++-------------------------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/walker.c b/walker.c
index 1dd86b8..fa02d09 100644
--- a/walker.c
+++ b/walker.c
@@ -251,24 +251,16 @@ void walker_targets_free(int targets, char **target, const char **write_ref)
 int walker_fetch(struct walker *walker, int targets, char **target,
 		 const char **write_ref, const char *write_ref_log_details)
 {
-	struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
+	struct ref_transaction *transaction;
 	unsigned char *sha1 = xmalloc(targets * 20);
-	char *msg;
-	int ret;
+	char *msg = NULL;
 	int i;
 
 	save_commit_buffer = 0;
 
-	for (i = 0; i < targets; i++) {
-		if (!write_ref || !write_ref[i])
-			continue;
-
-		lock[i] = lock_ref_sha1(write_ref[i], NULL);
-		if (!lock[i]) {
-			error("Can't lock ref %s", write_ref[i]);
-			goto unlock_and_fail;
-		}
-	}
+	transaction = ref_transaction_begin();
+	if (!transaction)
+		return -1;
 
 	if (!walker->get_recover)
 		for_each_ref(mark_complete, NULL);
@@ -276,14 +268,14 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 	for (i = 0; i < targets; i++) {
 		if (interpret_target(walker, target[i], &sha1[20 * i])) {
 			error("Could not interpret response from server '%s' as something to pull", target[i]);
-			goto unlock_and_fail;
+			goto rollback_and_fail;
 		}
 		if (process(walker, lookup_unknown_object(&sha1[20 * i])))
-			goto unlock_and_fail;
+			goto rollback_and_fail;
 	}
 
 	if (loop(walker))
-		goto unlock_and_fail;
+		goto rollback_and_fail;
 
 	if (write_ref_log_details) {
 		msg = xmalloc(strlen(write_ref_log_details) + 12);
@@ -294,19 +286,22 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 	for (i = 0; i < targets; i++) {
 		if (!write_ref || !write_ref[i])
 			continue;
-		ret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : "fetch (unknown)");
-		lock[i] = NULL;
-		if (ret)
-			goto unlock_and_fail;
+		if (ref_transaction_update(transaction, write_ref[i],
+					   &sha1[20 * i], NULL,
+					   0, 0))
+			goto rollback_and_fail;
 	}
-	free(msg);
 
+	if (ref_transaction_commit(transaction, msg ? msg : "fetch (unknown)",
+				   UPDATE_REFS_QUIET_ON_ERR))
+		goto rollback_and_fail;
+
+	free(msg);
 	return 0;
 
-unlock_and_fail:
-	for (i = 0; i < targets; i++)
-		if (lock[i])
-			unlock_ref(lock[i]);
+rollback_and_fail:
+	free(msg);
+	ref_transaction_rollback(transaction);
 
 	return -1;
 }
-- 
1.9.1.513.gd486896

Re: [PATCH 11/11] walker.c: use ref transaction for ref updates

[Michael Haggerty]

On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:
> Switch to using ref transactions in walker_fetch(). As part of the refactoring
> to use ref transactions we also fix a potential memory leak where in the
> original code if write_ref_sha1() would fail we would end up returning from
> the function without free()ing the msg string.

I don't have time to review this last patch this evening, but one thing
struck me.  It seems like the old code went to extra effort to lock all
the write_ref references early in the function, whereas your modified
version doesn't lock them until later.  Have you verified that you are
not opening a possible race condition?  If so, your commit message
should justify that it isn't a problem.  In other words, what does the
code do between the old time of locking and the new time of locking and
why doesn't it care whether the references are locked?

Aside from my other comments, patches 01-10 in the series looked fine.
Thanks!

Michael

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

Re: [PATCH 11/11] walker.c: use ref transaction for ref updates

[Junio C Hamano]

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

> On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:
>> Switch to using ref transactions in walker_fetch(). As part of the refactoring
>> to use ref transactions we also fix a potential memory leak where in the
>> original code if write_ref_sha1() would fail we would end up returning from
>> the function without free()ing the msg string.
> ...
> Aside from my other comments, patches 01-10 in the series looked fine.
> Thanks!

Thanks, both.

When this is queued on 'pu', it appears that t5550 breaks.  I had to
futz with conflicts with the lockfile topic, so a botched conflict
resolution might be a cause for the breakage, though.

Re: [PATCH 11/11] walker.c: use ref transaction for ref updates

[Ronnie Sahlberg]

I have updated the commit message with some text why I do not think
this change is critical for this case.
I will resend v2 of the patch series in a little while.

On Sat, Apr 19, 2014 at 12:48 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:
>> Switch to using ref transactions in walker_fetch(). As part of the refactoring
>> to use ref transactions we also fix a potential memory leak where in the
>> original code if write_ref_sha1() would fail we would end up returning from
>> the function without free()ing the msg string.
>
> I don't have time to review this last patch this evening, but one thing
> struck me.  It seems like the old code went to extra effort to lock all
> the write_ref references early in the function, whereas your modified
> version doesn't lock them until later.  Have you verified that you are
> not opening a possible race condition?  If so, your commit message
> should justify that it isn't a problem.  In other words, what does the
> code do between the old time of locking and the new time of locking and
> why doesn't it care whether the references are locked?
>
> Aside from my other comments, patches 01-10 in the series looked fine.
> Thanks!
>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/