[PATCH 0/5] Fix verify_lock() to report errors via strbuf

[PATCH 0/5] Fix verify_lock() to report errors via strbuf

[Michael Haggerty]

verify_lock() is a helper function called while committing reference
transactions. But when it fails, instead of recording its error
message in a strbuf to be passed back to the caller of
ref_transaction_commit(), the error message was being written directly
to stderr.

Instead, report the errors via a strbuf so that they make it back to
the caller of ref_transaction_commit().

The last two patches remove the capitalization of some error messages,
to be consistent with our usual practice. These are a slight backwards
incompatibility; if any scripts are trying to grep for these error
message strings, they might have to be adjusted. So feel free to drop
them if you think consistency across time is more important than
consistency across commands.

This is the patch series that I mentioned here [1]. It applies on top
of mh/ref-directory-file [2] and is thematically a continuation of
that series in the sense that it further cleans up the error handling
within reference transactions. It would be easy to rebase to master if
that is preferred.

These patches are also available on my GitHub account [3] as branch
"verify-lock-strbuf-err".

[1] http://article.gmane.org/gmane.comp.version-control.git/269006
[2] http://thread.gmane.org/gmane.comp.version-control.git/268778
[3] https://github.com/mhagger/git

Michael Haggerty (5):
  verify_lock(): return 0/-1 rather than struct ref_lock *
  verify_lock(): on errors, let the caller unlock the lock
  verify_lock(): report errors via a strbuf
  verify_lock(): do not capitalize error messages
  ref_transaction_commit(): do not capitalize error messages

 refs.c                | 40 ++++++++++++++++++++++++++--------------
 t/t1400-update-ref.sh | 14 +++++++-------
 2 files changed, 33 insertions(+), 21 deletions(-)

-- 
2.1.4

[PATCH 1/5] verify_lock(): return 0/-1 rather than struct ref_lock *

[Michael Haggerty]

Its return value wasn't conveying any extra information, but it made
the reader wonder whether the ref_lock that it returned might be
different than the one that was passed to it. So change the function
to the traditional "return 0 on success or a negative value on error".

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 97043fd..4432bc9 100644
--- a/refs.c
+++ b/refs.c
@@ -2195,9 +2195,14 @@ static void unlock_ref(struct ref_lock *lock)
 	free(lock);
 }
 
-/* This function should make sure errno is meaningful on error */
-static struct ref_lock *verify_lock(struct ref_lock *lock,
-	const unsigned char *old_sha1, int mustexist)
+/*
+ * Verify that the reference locked by lock has the value old_sha1.
+ * Fail if the reference doesn't exist and mustexist is set. Return 0
+ * on success or a negative value on error. This function should make
+ * sure errno is meaningful on error.
+ */
+static int verify_lock(struct ref_lock *lock,
+		       const unsigned char *old_sha1, int mustexist)
 {
 	if (read_ref_full(lock->ref_name,
 			  mustexist ? RESOLVE_REF_READING : 0,
@@ -2206,16 +2211,16 @@ static struct ref_lock *verify_lock(struct ref_lock *lock,
 		error("Can't verify ref %s", lock->ref_name);
 		unlock_ref(lock);
 		errno = save_errno;
-		return NULL;
+		return -1;
 	}
 	if (hashcmp(lock->old_sha1, old_sha1)) {
 		error("Ref %s is at %s but expected %s", lock->ref_name,
 			sha1_to_hex(lock->old_sha1), sha1_to_hex(old_sha1));
 		unlock_ref(lock);
 		errno = EBUSY;
-		return NULL;
+		return -1;
 	}
-	return lock;
+	return 0;
 }
 
 static int remove_empty_directories(const char *file)
@@ -2445,7 +2450,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			goto error_return;
 		}
 	}
-	return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
+	if (old_sha1 && verify_lock(lock, old_sha1, mustexist))
+		return NULL;
+	return lock;
 
  error_return:
 	unlock_ref(lock);
-- 
2.1.4

Re: [PATCH 1/5] verify_lock(): return 0/-1 rather than struct ref_lock *

[Stefan Beller]

On Fri, May 22, 2015 at 4:34 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Its return value wasn't conveying any extra information, but it made
> the reader wonder whether the ref_lock that it returned might be
> different than the one that was passed to it. So change the function
> to the traditional "return 0 on success or a negative value on error".
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

Bonus points for the documentation!
Reviewed-by: Stefan Beller <sbeller@google.com>

> ---
>  refs.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 97043fd..4432bc9 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2195,9 +2195,14 @@ static void unlock_ref(struct ref_lock *lock)
>         free(lock);
>  }
>
> -/* This function should make sure errno is meaningful on error */
> -static struct ref_lock *verify_lock(struct ref_lock *lock,
> -       const unsigned char *old_sha1, int mustexist)
> +/*
> + * Verify that the reference locked by lock has the value old_sha1.
> + * Fail if the reference doesn't exist and mustexist is set. Return 0
> + * on success or a negative value on error. This function should make
> + * sure errno is meaningful on error.
> + */
> +static int verify_lock(struct ref_lock *lock,
> +                      const unsigned char *old_sha1, int mustexist)
>  {
>         if (read_ref_full(lock->ref_name,
>                           mustexist ? RESOLVE_REF_READING : 0,
> @@ -2206,16 +2211,16 @@ static struct ref_lock *verify_lock(struct ref_lock *lock,
>                 error("Can't verify ref %s", lock->ref_name);
>                 unlock_ref(lock);
>                 errno = save_errno;
> -               return NULL;
> +               return -1;
>         }
>         if (hashcmp(lock->old_sha1, old_sha1)) {
>                 error("Ref %s is at %s but expected %s", lock->ref_name,
>                         sha1_to_hex(lock->old_sha1), sha1_to_hex(old_sha1));
>                 unlock_ref(lock);
>                 errno = EBUSY;
> -               return NULL;
> +               return -1;
>         }
> -       return lock;
> +       return 0;
>  }
>
>  static int remove_empty_directories(const char *file)
> @@ -2445,7 +2450,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>                         goto error_return;
>                 }
>         }
> -       return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;
> +       if (old_sha1 && verify_lock(lock, old_sha1, mustexist))
> +               return NULL;
> +       return lock;
>
>   error_return:
>         unlock_ref(lock);
> --
> 2.1.4
>

[PATCH 2/5] verify_lock(): on errors, let the caller unlock the lock

[Michael Haggerty]

The caller already knows how to do it, so always do it in the same
place.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 4432bc9..100a767 100644
--- a/refs.c
+++ b/refs.c
@@ -2209,14 +2209,12 @@ static int verify_lock(struct ref_lock *lock,
 			  lock->old_sha1, NULL)) {
 		int save_errno = errno;
 		error("Can't verify ref %s", lock->ref_name);
-		unlock_ref(lock);
 		errno = save_errno;
 		return -1;
 	}
 	if (hashcmp(lock->old_sha1, old_sha1)) {
 		error("Ref %s is at %s but expected %s", lock->ref_name,
 			sha1_to_hex(lock->old_sha1), sha1_to_hex(old_sha1));
-		unlock_ref(lock);
 		errno = EBUSY;
 		return -1;
 	}
@@ -2450,8 +2448,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			goto error_return;
 		}
 	}
-	if (old_sha1 && verify_lock(lock, old_sha1, mustexist))
-		return NULL;
+	if (old_sha1 && verify_lock(lock, old_sha1, mustexist)) {
+		last_errno = errno;
+		goto error_return;
+	}
 	return lock;
 
  error_return:
-- 
2.1.4

[PATCH 3/5] verify_lock(): report errors via a strbuf

[Michael Haggerty]

Instead of writing error messages directly to stderr, write them to a
"strbuf *err". In lock_ref_sha1_basic(), arrange for these errors to
be returned to its caller.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 100a767..625e69f 100644
--- a/refs.c
+++ b/refs.c
@@ -2198,23 +2198,28 @@ static void unlock_ref(struct ref_lock *lock)
 /*
  * Verify that the reference locked by lock has the value old_sha1.
  * Fail if the reference doesn't exist and mustexist is set. Return 0
- * on success or a negative value on error. This function should make
- * sure errno is meaningful on error.
+ * on success. On error, write an error message to err, set errno, and
+ * return a negative value.
  */
 static int verify_lock(struct ref_lock *lock,
-		       const unsigned char *old_sha1, int mustexist)
+		       const unsigned char *old_sha1, int mustexist,
+		       struct strbuf *err)
 {
+	assert(err);
+
 	if (read_ref_full(lock->ref_name,
 			  mustexist ? RESOLVE_REF_READING : 0,
 			  lock->old_sha1, NULL)) {
 		int save_errno = errno;
-		error("Can't verify ref %s", lock->ref_name);
+		strbuf_addf(err, "Can't verify ref %s", lock->ref_name);
 		errno = save_errno;
 		return -1;
 	}
 	if (hashcmp(lock->old_sha1, old_sha1)) {
-		error("Ref %s is at %s but expected %s", lock->ref_name,
-			sha1_to_hex(lock->old_sha1), sha1_to_hex(old_sha1));
+		strbuf_addf(err, "Ref %s is at %s but expected %s",
+			    lock->ref_name,
+			    sha1_to_hex(lock->old_sha1),
+			    sha1_to_hex(old_sha1));
 		errno = EBUSY;
 		return -1;
 	}
@@ -2448,7 +2453,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			goto error_return;
 		}
 	}
-	if (old_sha1 && verify_lock(lock, old_sha1, mustexist)) {
+	if (old_sha1 && verify_lock(lock, old_sha1, mustexist, err)) {
 		last_errno = errno;
 		goto error_return;
 	}
-- 
2.1.4

Re: [PATCH 3/5] verify_lock(): report errors via a strbuf

[Junio C Hamano]

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

> Instead of writing error messages directly to stderr, write them to a
> "strbuf *err". In lock_ref_sha1_basic(), arrange for these errors to
> be returned to its caller.

I had to scratch my head and view long outside the context before
realizing that the caller lock_ref_sha1_basic() already arranges
with its caller that errors from it are passed via the strbuf, and
this change is just turning verify_lock(), a callee from it, follows
that already established pattern.

Looks sensible, but the last sentence was misleading at least to me.

	The caller, lock_ref_sha1_basic(), uses this error reporting
	convention with all the other callees, and reports its error
        this way to its callers.

perhaps?

Re: [PATCH 3/5] verify_lock(): report errors via a strbuf

[Michael Haggerty]

On 05/27/2015 09:48 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Instead of writing error messages directly to stderr, write them to a
>> "strbuf *err". In lock_ref_sha1_basic(), arrange for these errors to
>> be returned to its caller.
> 
> I had to scratch my head and view long outside the context before
> realizing that the caller lock_ref_sha1_basic() already arranges
> with its caller that errors from it are passed via the strbuf, and
> this change is just turning verify_lock(), a callee from it, follows
> that already established pattern.
> 
> Looks sensible, but the last sentence was misleading at least to me.
> 
> 	The caller, lock_ref_sha1_basic(), uses this error reporting
> 	convention with all the other callees, and reports its error
>         this way to its callers.
> 
> perhaps?

+1

Thanks for clarifying this sentence.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

[PATCH 4/5] verify_lock(): do not capitalize error messages

[Michael Haggerty]

Our convention is for error messages to start with a lower-case
letter.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 625e69f..48aff79 100644
--- a/refs.c
+++ b/refs.c
@@ -2211,12 +2211,12 @@ static int verify_lock(struct ref_lock *lock,
 			  mustexist ? RESOLVE_REF_READING : 0,
 			  lock->old_sha1, NULL)) {
 		int save_errno = errno;
-		strbuf_addf(err, "Can't verify ref %s", lock->ref_name);
+		strbuf_addf(err, "can't verify ref %s", lock->ref_name);
 		errno = save_errno;
 		return -1;
 	}
 	if (hashcmp(lock->old_sha1, old_sha1)) {
-		strbuf_addf(err, "Ref %s is at %s but expected %s",
+		strbuf_addf(err, "ref %s is at %s but expected %s",
 			    lock->ref_name,
 			    sha1_to_hex(lock->old_sha1),
 			    sha1_to_hex(old_sha1));
-- 
2.1.4

[PATCH 5/5] ref_transaction_commit(): do not capitalize error messages

[Michael Haggerty]

Our convention is for error messages to start with a lower-case
letter.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c                |  4 ++--
 t/t1400-update-ref.sh | 14 +++++++-------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index 48aff79..1d60fcd 100644
--- a/refs.c
+++ b/refs.c
@@ -3856,7 +3856,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 				? TRANSACTION_NAME_CONFLICT
 				: TRANSACTION_GENERIC_ERROR;
 			reason = strbuf_detach(err, NULL);
-			strbuf_addf(err, "Cannot lock ref '%s': %s",
+			strbuf_addf(err, "cannot lock ref '%s': %s",
 				    update->refname, reason);
 			free(reason);
 			goto cleanup;
@@ -3883,7 +3883,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			} else if (write_ref_sha1(update->lock, update->new_sha1,
 						  update->msg)) {
 				update->lock = NULL; /* freed by write_ref_sha1 */
-				strbuf_addf(err, "Cannot update the ref '%s'.",
+				strbuf_addf(err, "cannot update the ref '%s'.",
 					    update->refname);
 				ret = TRANSACTION_GENERIC_ERROR;
 				goto cleanup;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 86fa468..9c133c1 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -519,7 +519,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 ref '"'"'$c'"'"'" err &&
+	grep "fatal: cannot lock ref '"'"'$c'"'"'" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
@@ -555,7 +555,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 ref '"'"'$a'"'"'" err &&
+	grep "fatal: cannot lock ref '"'"'$a'"'"'" err &&
 	git rev-parse $m >expect &&
 	git rev-parse $a >actual &&
 	test_cmp expect actual
@@ -688,7 +688,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 ref '"'"'$c'"'"'" err &&
+	grep "fatal: cannot lock ref '"'"'$c'"'"'" err &&
 	git rev-parse $m >expect &&
 	git rev-parse $a >actual &&
 	test_cmp expect actual &&
@@ -883,7 +883,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 ref '"'"'$c'"'"'" err &&
+	grep "fatal: cannot lock ref '"'"'$c'"'"'" err &&
 	test_must_fail git rev-parse --verify -q $c
 '
 
@@ -899,7 +899,7 @@ test_expect_success 'stdin -z create ref fails when ref exists' '
 	git rev-parse "$c" >expect &&
 	printf $F "create $c" "$m~1" >stdin &&
 	test_must_fail git update-ref -z --stdin <stdin 2>err &&
-	grep "fatal: Cannot lock ref '"'"'$c'"'"'" err &&
+	grep "fatal: cannot lock ref '"'"'$c'"'"'" err &&
 	git rev-parse "$c" >actual &&
 	test_cmp expect actual
 '
@@ -930,7 +930,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 ref '"'"'$a'"'"'" err &&
+	grep "fatal: cannot lock ref '"'"'$a'"'"'" err &&
 	git rev-parse $m >expect &&
 	git rev-parse $a >actual &&
 	test_cmp expect actual
@@ -1045,7 +1045,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 ref '"'"'$c'"'"'" err &&
+	grep "fatal: cannot lock ref '"'"'$c'"'"'" err &&
 	git rev-parse $m >expect &&
 	git rev-parse $a >actual &&
 	test_cmp expect actual &&
-- 
2.1.4

Re: [PATCH 0/5] Fix verify_lock() to report errors via strbuf

[Stefan Beller]

On Fri, May 22, 2015 at 4:34 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> verify_lock() is a helper function called while committing reference
> transactions. But when it fails, instead of recording its error
> message in a strbuf to be passed back to the caller of
> ref_transaction_commit(), the error message was being written directly
> to stderr.
>
> Instead, report the errors via a strbuf so that they make it back to
> the caller of ref_transaction_commit().
>
> The last two patches remove the capitalization of some error messages,
> to be consistent with our usual practice. These are a slight backwards
> incompatibility; if any scripts are trying to grep for these error
> message strings, they might have to be adjusted. So feel free to drop
> them if you think consistency across time is more important than
> consistency across commands.
>
> This is the patch series that I mentioned here [1]. It applies on top
> of mh/ref-directory-file [2] and is thematically a continuation of
> that series in the sense that it further cleans up the error handling
> within reference transactions. It would be easy to rebase to master if
> that is preferred.

I was confused at first as I only looked at the patches and the corresponding
line numbers did not match with the files as currently open in my editor.
(they are roughly origin/master)

Now that I read the cover letter I can explain the line number being slightly
different. :)

The series looks good to me.

>
> These patches are also available on my GitHub account [3] as branch
> "verify-lock-strbuf-err".
>
> [1] http://article.gmane.org/gmane.comp.version-control.git/269006
> [2] http://thread.gmane.org/gmane.comp.version-control.git/268778
> [3] https://github.com/mhagger/git
>
> Michael Haggerty (5):
>   verify_lock(): return 0/-1 rather than struct ref_lock *
>   verify_lock(): on errors, let the caller unlock the lock
>   verify_lock(): report errors via a strbuf
>   verify_lock(): do not capitalize error messages
>   ref_transaction_commit(): do not capitalize error messages
>
>  refs.c                | 40 ++++++++++++++++++++++++++--------------
>  t/t1400-update-ref.sh | 14 +++++++-------
>  2 files changed, 33 insertions(+), 21 deletions(-)
>
> --
> 2.1.4
>

Re: [PATCH 0/5] Fix verify_lock() to report errors via strbuf

[Michael Haggerty]

On 05/23/2015 01:34 AM, Michael Haggerty wrote:
> verify_lock() is a helper function called while committing reference
> transactions. But when it fails, instead of recording its error
> message in a strbuf to be passed back to the caller of
> ref_transaction_commit(), the error message was being written directly
> to stderr.
> 
> Instead, report the errors via a strbuf so that they make it back to
> the caller of ref_transaction_commit().
> 
> [...]
> 
> This is the patch series that I mentioned here [1]. It applies on top
> of mh/ref-directory-file [2] and is thematically a continuation of
> that series in the sense that it further cleans up the error handling
> within reference transactions. It would be easy to rebase to master if
> that is preferred.

The last sentence is nonsense. This patch series relies on
lock_ref_sha1_basic() having a "strbuf *err" parameter, which is only
the case since

    4a32b2e lock_ref_sha1_basic(): report errors via a "struct strbuf
*err" (2015-05-11)

The latter commit is in mh/ref-directory-file (which has now been merged
to master, so technically the last sentence is now correct again).

Sorry for the confusion.
Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 0/5] Fix verify_lock() to report errors via strbuf

[Junio C Hamano]

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

> The last sentence is nonsense. This patch series relies on
> lock_ref_sha1_basic() having a "strbuf *err" parameter, which is only
> the case since
>
>     4a32b2e lock_ref_sha1_basic(): report errors via a "struct strbuf
> *err" (2015-05-11)
>
> The latter commit is in mh/ref-directory-file (which has now been merged
> to master, so technically the last sentence is now correct again).

[5/5] seems to conflict with the write_ref_sha1() vs write_ref_to_lockfile()
updates; I think I can manage, though ;-)