diff mbox series

[v4,5/6] refs: explicitly return failure_errno from parse_loose_ref_contents

Message ID ab147afb38d6e74e0e2fc1fbb83346e6ee32cdd5.1625597757.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series refs: cleanup errno sideband ref related functions | expand

Commit Message

Han-Wen Nienhuys July 6, 2021, 6:55 p.m. UTC 
From: Han-Wen Nienhuys <hanwen@google.com>

The EINVAL error from parse_loose_ref_contents is used in files-backend
to create a custom error message.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Reviewed-By: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs.c               |  8 +++++---
 refs/files-backend.c | 13 +++++++++----
 refs/refs-internal.h |  6 ++++--
 3 files changed, 18 insertions(+), 9 deletions(-)

Comments

Junio C Hamano July 6, 2021, 7:37 p.m. UTC | #1 
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> The EINVAL error from parse_loose_ref_contents is used in files-backend
> to create a custom error message.
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> Reviewed-By: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

I think a -By in the trailer is spelled with lowercase, i.e. "Reviewed-by".

> ---
>  refs.c               |  8 +++++---
>  refs/files-backend.c | 13 +++++++++----
>  refs/refs-internal.h |  6 ++++--
>  3 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 25d80e544d0..eca7310e7a4 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1653,7 +1653,8 @@ int for_each_fullref_in_prefixes(const char *namespace,
>  
>  static int refs_read_special_head(struct ref_store *ref_store,
>  				  const char *refname, struct object_id *oid,
> -				  struct strbuf *referent, unsigned int *type)
> +				  struct strbuf *referent, unsigned int *type,
> +				  int *failure_errno)
>  {
>  	struct strbuf full_path = STRBUF_INIT;
>  	struct strbuf content = STRBUF_INIT;
> @@ -1663,7 +1664,8 @@ static int refs_read_special_head(struct ref_store *ref_store,
>  	if (strbuf_read_file(&content, full_path.buf, 0) < 0)
>  		goto done;
>  
> -	result = parse_loose_ref_contents(content.buf, oid, referent, type);
> +	result = parse_loose_ref_contents(content.buf, oid, referent, type,
> +					  failure_errno);
>  
>  done:
>  	strbuf_release(&full_path);
> @@ -1683,7 +1685,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
>  
>  	if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
>  		return refs_read_special_head(ref_store, refname, oid, referent,
> -					      type);
> +					      type, failure_errno);
>  	}
>  
>  	return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index f38c9703504..b43ec4c66cb 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -455,9 +455,13 @@ stat_ref:
>  	strbuf_rtrim(&sb_contents);
>  	buf = sb_contents.buf;
>  
> -	ret = parse_loose_ref_contents(buf, oid, referent, type);
> -
> +	ret = parse_loose_ref_contents(buf, oid, referent, type, failure_errno);
> +	errno = *failure_errno;

All of the above makes sense.

>  out:
> +	/* Collect all types of failures from errno. Many system calls in this
> +	 * function can fail with ENOTDIR/EISDIR, and we want to collect all of
> +	 * them.
> +	 */

Style.

>  	*failure_errno = errno;
>  	strbuf_release(&sb_path);
>  	strbuf_release(&sb_contents);
> @@ -465,7 +469,8 @@ out:
>  }
>  
>  int parse_loose_ref_contents(const char *buf, struct object_id *oid,
> -			     struct strbuf *referent, unsigned int *type)
> +			     struct strbuf *referent, unsigned int *type,
> +			     int *failure_errno)
>  {
>  	const char *p;
>  	if (skip_prefix(buf, "ref:", &buf)) {
> @@ -484,7 +489,7 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid,
>  	if (parse_oid_hex(buf, oid, &p) ||
>  	    (*p != '\0' && !isspace(*p))) {
>  		*type |= REF_ISBROKEN;
> -		errno = EINVAL;
> +		*failure_errno = EINVAL;
>  		return -1;
>  	}

OK.

>  	return 0;
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 54f57c6a2df..bf581e70cf6 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -689,10 +689,12 @@ struct ref_store {
>  };
>  
>  /*
> - * Parse contents of a loose ref file.
> + * Parse contents of a loose ref file. *failure_errno maybe be set to EINVAL for
> + * invalid contents.
>   */

OK.

>  int parse_loose_ref_contents(const char *buf, struct object_id *oid,
> -			     struct strbuf *referent, unsigned int *type);
> +			     struct strbuf *referent, unsigned int *type,
> +			     int *failure_errno);
>  
>  /*
>   * Fill in the generic part of refs and add it to our collection of
Han-Wen Nienhuys July 7, 2021, 8:20 a.m. UTC | #2 
On Tue, Jul 6, 2021 at 9:37 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Han-Wen Nienhuys <hanwen@google.com>
> >
> > The EINVAL error from parse_loose_ref_contents is used in files-backend
> > to create a custom error message.
> >
> > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> > Reviewed-By: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> I think a -By in the trailer is spelled with lowercase, i.e. "Reviewed-by".

.. All done.

I also addressed your other fixup.
diff mbox series

Patch

diff --git a/refs.c b/refs.c
index 25d80e544d0..eca7310e7a4 100644
--- a/refs.c
+++ b/refs.c
@@ -1653,7 +1653,8 @@  int for_each_fullref_in_prefixes(const char *namespace,
 
 static int refs_read_special_head(struct ref_store *ref_store,
 				  const char *refname, struct object_id *oid,
-				  struct strbuf *referent, unsigned int *type)
+				  struct strbuf *referent, unsigned int *type,
+				  int *failure_errno)
 {
 	struct strbuf full_path = STRBUF_INIT;
 	struct strbuf content = STRBUF_INIT;
@@ -1663,7 +1664,8 @@  static int refs_read_special_head(struct ref_store *ref_store,
 	if (strbuf_read_file(&content, full_path.buf, 0) < 0)
 		goto done;
 
-	result = parse_loose_ref_contents(content.buf, oid, referent, type);
+	result = parse_loose_ref_contents(content.buf, oid, referent, type,
+					  failure_errno);
 
 done:
 	strbuf_release(&full_path);
@@ -1683,7 +1685,7 @@  int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
 
 	if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
 		return refs_read_special_head(ref_store, refname, oid, referent,
-					      type);
+					      type, failure_errno);
 	}
 
 	return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f38c9703504..b43ec4c66cb 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -455,9 +455,13 @@  stat_ref:
 	strbuf_rtrim(&sb_contents);
 	buf = sb_contents.buf;
 
-	ret = parse_loose_ref_contents(buf, oid, referent, type);
-
+	ret = parse_loose_ref_contents(buf, oid, referent, type, failure_errno);
+	errno = *failure_errno;
 out:
+	/* Collect all types of failures from errno. Many system calls in this
+	 * function can fail with ENOTDIR/EISDIR, and we want to collect all of
+	 * them.
+	 */
 	*failure_errno = errno;
 	strbuf_release(&sb_path);
 	strbuf_release(&sb_contents);
@@ -465,7 +469,8 @@  out:
 }
 
 int parse_loose_ref_contents(const char *buf, struct object_id *oid,
-			     struct strbuf *referent, unsigned int *type)
+			     struct strbuf *referent, unsigned int *type,
+			     int *failure_errno)
 {
 	const char *p;
 	if (skip_prefix(buf, "ref:", &buf)) {
@@ -484,7 +489,7 @@  int parse_loose_ref_contents(const char *buf, struct object_id *oid,
 	if (parse_oid_hex(buf, oid, &p) ||
 	    (*p != '\0' && !isspace(*p))) {
 		*type |= REF_ISBROKEN;
-		errno = EINVAL;
+		*failure_errno = EINVAL;
 		return -1;
 	}
 	return 0;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 54f57c6a2df..bf581e70cf6 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -689,10 +689,12 @@  struct ref_store {
 };
 
 /*
- * Parse contents of a loose ref file.
+ * Parse contents of a loose ref file. *failure_errno maybe be set to EINVAL for
+ * invalid contents.
  */
 int parse_loose_ref_contents(const char *buf, struct object_id *oid,
-			     struct strbuf *referent, unsigned int *type);
+			     struct strbuf *referent, unsigned int *type,
+			     int *failure_errno);
 
 /*
  * Fill in the generic part of refs and add it to our collection of