Re: [PATCH v3 2/2] remote.c: remove BUG in show_push_unqualified_ref_name_error()

Patrick Steinhardt [off-list ref] writes:

This reads a lot better, thanks. We could arguably convert the
if-else-chain into a switch to make all of this read a bit better, but
that is a subjective style change and definitely not something that you
have to do as part of this series.

I concur.  I admit that using switch never occured to me but I agree
100% with you that it would make the result nicer, and that it does
not have to be part of this series.

One thing I wondered is whether it's okay to not die anymore via
`BUG()`. The other error cases already don't die though, so this ought
to be fine. Going up the callchain shows that we do bubble up the error
as expected until we end up in `match_push_refs()`. There's multiple
callers of that function, and all except one perform error handling for
it.

The only exception is git-remote(1) in `get_push_ref_states()`, where it
gets executed via `git remote show $remote_name`. As far as I understand
we would end up not showing any references that are broken, and we would
print the above advise. Which I think is reasonable.

So all of this looks good to me, thanks!

Nice to see somebody thinks through the potential impact for all the
callers.  Very much appreciated.

Let's merge the topic to 'next'.

Thanks.

For better readability, convert the if-else tower into a switch
statement.

Signed-off-by: Denton Liu <redacted>
---
Thanks for the suggestion, both. Please queue this patch wherever it
makes the most sense to do so (either with the existing series or on its
own separate branch).

 remote.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index 465e0ea0eb..c7ae18fcfa 100644
--- a/remote.c
+++ b/remote.c

@@ -1197,29 +1197,35 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
 		    "match_explicit_lhs() should catch this!",
 		    matched_src_name);
 	type = odb_read_object_info(the_repository->objects, &oid, NULL);
-	if (type == OBJ_COMMIT) {
+	switch (type) {
+	case OBJ_COMMIT:
 		advise(_("The <src> part of the refspec is a commit object.\n"
 			 "Did you mean to create a new branch by pushing to\n"
 			 "'%s:refs/heads/%s'?"),
 		       matched_src_name, dst_value);
-	} else if (type == OBJ_TAG) {
+		break;
+	case OBJ_TAG:
 		advise(_("The <src> part of the refspec is a tag object.\n"
 			 "Did you mean to create a new tag by pushing to\n"
 			 "'%s:refs/tags/%s'?"),
 		       matched_src_name, dst_value);
-	} else if (type == OBJ_TREE) {
+		break;
+	case OBJ_TREE:
 		advise(_("The <src> part of the refspec is a tree object.\n"
 			 "Did you mean to tag a new tree by pushing to\n"
 			 "'%s:refs/tags/%s'?"),
 		       matched_src_name, dst_value);
-	} else if (type == OBJ_BLOB) {
+		break;
+	case OBJ_BLOB:
 		advise(_("The <src> part of the refspec is a blob object.\n"
 			 "Did you mean to tag a new blob by pushing to\n"
 			 "'%s:refs/tags/%s'?"),
 		       matched_src_name, dst_value);
-	} else {
+		break;
+	default:
 		advise(_("The <src> part of the refspec ('%s') is an object ID that doesn't exist.\n"),
 		       matched_src_name);
+		break;
 	}
 }
 

-- 
2.50.1

On Wed, Aug 06, 2025 at 09:30:20PM -0700, Denton Liu wrote:

Nit: we can also drop the `type` variable, we don't need it for anything
but the value of the switch as far as I can see.

Thanks!

Patrick

For better readability, convert the if-else tower into a switch
statement.

Signed-off-by: Denton Liu <redacted>
---
 remote.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/remote.c b/remote.c
index 465e0ea0eb..029b1fa93b 100644
--- a/remote.c
+++ b/remote.c

@@ -1171,7 +1171,6 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
 						 const char *matched_src_name)
 {
 	struct object_id oid;
-	enum object_type type;
 
 	/*
 	 * TRANSLATORS: "matches '%s'%" is the <dst> part of "git push

@@ -1196,30 +1195,36 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
 		BUG("'%s' is not a valid object, "
 		    "match_explicit_lhs() should catch this!",
 		    matched_src_name);
-	type = odb_read_object_info(the_repository->objects, &oid, NULL);
-	if (type == OBJ_COMMIT) {
+
+	switch (odb_read_object_info(the_repository->objects, &oid, NULL)) {
+	case OBJ_COMMIT:
 		advise(_("The <src> part of the refspec is a commit object.\n"
 			 "Did you mean to create a new branch by pushing to\n"
 			 "'%s:refs/heads/%s'?"),
 		       matched_src_name, dst_value);
-	} else if (type == OBJ_TAG) {
+		break;
+	case OBJ_TAG:
 		advise(_("The <src> part of the refspec is a tag object.\n"
 			 "Did you mean to create a new tag by pushing to\n"
 			 "'%s:refs/tags/%s'?"),
 		       matched_src_name, dst_value);
-	} else if (type == OBJ_TREE) {
+		break;
+	case OBJ_TREE:
 		advise(_("The <src> part of the refspec is a tree object.\n"
 			 "Did you mean to tag a new tree by pushing to\n"
 			 "'%s:refs/tags/%s'?"),
 		       matched_src_name, dst_value);
-	} else if (type == OBJ_BLOB) {
+		break;
+	case OBJ_BLOB:
 		advise(_("The <src> part of the refspec is a blob object.\n"
 			 "Did you mean to tag a new blob by pushing to\n"
 			 "'%s:refs/tags/%s'?"),
 		       matched_src_name, dst_value);
-	} else {
+		break;
+	default:
 		advise(_("The <src> part of the refspec ('%s') is an object ID that doesn't exist.\n"),
 		       matched_src_name);
+		break;
 	}
 }
 

Range-diff against v1:
1:  5866818859 ! 1:  54a16614e2 remote.c: convert if-else tower to switch
    @@ Commit message
     
      ## remote.c ##
     @@ remote.c: static void show_push_unqualified_ref_name_error(const char *dst_value,
    + 						 const char *matched_src_name)
    + {
    + 	struct object_id oid;
    +-	enum object_type type;
    + 
    + 	/*
    + 	 * TRANSLATORS: "matches '%s'%" is the <dst> part of "git push
    +@@ remote.c: static void show_push_unqualified_ref_name_error(const char *dst_value,
    + 		BUG("'%s' is not a valid object, "
      		    "match_explicit_lhs() should catch this!",
      		    matched_src_name);
    - 	type = odb_read_object_info(the_repository->objects, &oid, NULL);
    +-	type = odb_read_object_info(the_repository->objects, &oid, NULL);
     -	if (type == OBJ_COMMIT) {
    -+	switch (type) {
    ++
    ++	switch (odb_read_object_info(the_repository->objects, &oid, NULL)) {
     +	case OBJ_COMMIT:
      		advise(_("The <src> part of the refspec is a commit object.\n"
      			 "Did you mean to create a new branch by pushing to\n"
-- 
2.50.1

Don’t we normally put single-patch notes like a range-diff right after the triple dash? I have a feeling this format breaks git-am on the receiving side, though I haven’t actually tried it. 

1:  5866818859 ! 1:  54a16614e2 remote.c: convert if-else tower to switch
   @@ Commit message

     ## remote.c ##
    @@ remote.c: static void show_push_unqualified_ref_name_error(const char *dst_value,
   +                         const char *matched_src_name)
   + {
   +    struct object_id oid;
   +-    enum object_type type;
   +
   +    /*
   +     * TRANSLATORS: "matches '%s'%" is the <dst> part of "git push
   +@@ remote.c: static void show_push_unqualified_ref_name_error(const char *dst_value,
   +        BUG("'%s' is not a valid object, "
                 "match_explicit_lhs() should catch this!",
                 matched_src_name);
   -    type = odb_read_object_info(the_repository->objects, &oid, NULL);
   +-    type = odb_read_object_info(the_repository->objects, &oid, NULL);
    -    if (type == OBJ_COMMIT) {
   -+    switch (type) {
   ++
   ++    switch (odb_read_object_info(the_repository->objects, &oid, NULL)) {
    +    case OBJ_COMMIT:
             advise(_("The <src> part of the refspec is a commit object.\n"
                  "Did you mean to create a new branch by pushing to\n"
--
2.50.1

On Thu, Aug 7, 2025 at 8:35 AM Ben Knoble [off-list ref] wrote:

quoted

Le 7 août 2025 à 05:20, Denton Liu [off-list ref] a écrit :
For better readability, convert the if-else tower into a switch
statement.

Signed-off-by: Denton Liu <redacted>
---

diff --git a/remote.c b/remote.c
index 465e0ea0eb..029b1fa93b 100644
--- a/remote.c
+++ b/remote.c

@@ -1171,7 +1171,6 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,

Range-diff against v1:

Don’t we normally put single-patch notes like a range-diff right after the triple dash? I have a feeling this format breaks git-am on the receiving side, though I haven’t actually tried it.

Not since 2fa04cebfb (format-patch: move range/inter diff at the end
of a single patch output, 2024-05-24).