[PATCH 0/2] remote.c: remove erroneous BUG case

[PATCH 0/2] remote.c: remove erroneous BUG case

[Denton Liu]

In the case where one pushes a non-existent oid to an unqualified
destination, we encounter the following BUG

	error: The destination you provided is not a full refname (i.e.,
	starting with "refs/"). We tried to guess what you meant by:

	- Looking for a ref that matches 'branch' on the remote side.
	- Checking if the <src> being pushed ('0000000000000000000000000000000000000001')
	  is a ref in "refs/{heads,tags}/". If so we add a corresponding
	  refs/{heads,tags}/ prefix on the remote side.

	Neither worked, so we gave up. You must fully qualify the ref.
	BUG: remote.c:1221: '0000000000000000000000000000000000000001' should be commit/tag/tree/blob, is '-1'
	fatal: the remote end hung up unexpectedly
	Aborted (core dumped)

However, this isn't actually a bug so replace it with an advise()
message.

Denton Liu (2):
  t5516: introduce 'push ref expression with non-existent oid src'
  remote.c: remove BUG in show_push_unqualified_ref_name_error()

 remote.c              | 5 +++--
 t/t5516-fetch-push.sh | 7 +++++++
 2 files changed, 10 insertions(+), 2 deletions(-)

-- 
2.50.1

[PATCH 1/2] t5516: introduce 'push ref expression with non-existent oid src'

[Denton Liu]

It is possible to trigger a Git bug by pushing a refspec where the
source is an oid that's non-existent. An example of the error message
produced is as follows:

	error: The destination you provided is not a full refname (i.e.,
	starting with "refs/"). We tried to guess what you meant by:

	- Looking for a ref that matches 'branch' on the remote side.
	- Checking if the <src> being pushed ('0000000000000000000000000000000000000001')
	  is a ref in "refs/{heads,tags}/". If so we add a corresponding
	  refs/{heads,tags}/ prefix on the remote side.

	Neither worked, so we gave up. You must fully qualify the ref.
	BUG: remote.c:1221: '0000000000000000000000000000000000000001' should be commit/tag/tree/blob, is '-1'
	fatal: the remote end hung up unexpectedly
	Aborted (core dumped)

Document this failure in a test case so that it can be confirmed fixed
later.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t5516-fetch-push.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 4e9c27b0f2..c2fcfeca92 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -509,6 +509,13 @@ test_expect_success 'push ref expression with non-existent, incomplete dest' '
 
 '
 
+test_expect_failure 'push ref expression with non-existent oid src' '
+
+	mk_test testrepo &&
+	test_must_fail git push testrepo $(test_oid 001):branch
+
+'
+
 for head in HEAD @
 do
 
-- 
2.50.1

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

[Denton Liu]

In the case where a non-existent oid is given as the <src> for a
refspec and the destination is unqualified, we end up hitting the BUG in
show_push_unqualified_ref_name_error().

This is because before hitting this advise message, the <src> is passed
through repo_get_oid() which, upon receiving a fully qualified oid,
doesn't actually check the existence of the object and just returns
found. This means that it's actually possible for the
odb_read_object_info() call to return not found under normal usage and
thus, it's not actually a bug.

Replace the BUG() with an advise() displaying a helpful message about
the oid possibly not existing.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 remote.c              | 5 +++--
 t/t5516-fetch-push.sh | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/remote.c b/remote.c
index e965f022f1..9fb76049d2 100644
--- a/remote.c
+++ b/remote.c
@@ -1218,8 +1218,9 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
 			 "'%s:refs/tags/%s'?"),
 		       matched_src_name, dst_value);
 	} else {
-		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
-		    matched_src_name, type);
+		advise(_("The <src> part of the refspec is an oid that doesn't exist.\n"
+			 "Please ensure that the oid '%s' is correct."),
+		       matched_src_name);
 	}
 }
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index c2fcfeca92..e064ea7433 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -509,7 +509,7 @@ test_expect_success 'push ref expression with non-existent, incomplete dest' '
 
 '
 
-test_expect_failure 'push ref expression with non-existent oid src' '
+test_expect_success 'push ref expression with non-existent oid src' '
 
 	mk_test testrepo &&
 	test_must_fail git push testrepo $(test_oid 001):branch
-- 
2.50.1

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

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> In the case where a non-existent oid is given as the <src> for a
> refspec and the destination is unqualified, we end up hitting the BUG in
> show_push_unqualified_ref_name_error().
>
> This is because before hitting this advise message, the <src> is passed
> through repo_get_oid() which, upon receiving a fully qualified oid,
> doesn't actually check the existence of the object and just returns
> found.

The tail end of the above sentence does not quite parse for me.
Strike "and just returns found" out, perhaps?

> This means that it's actually possible for the
> odb_read_object_info() call to return not found under normal usage and
> thus, it's not actually a bug.

Again it is unclear what this "not found", used as noun, means.

Often saying "A" and having to follow it with "this means B" is a
sign that both needs to be rewritten to clarify.  The above does it
three times ("A", "this is because B", "this means C").  How about
flowing your thought in a slightly different order, perhaps like
this?

    When "git push <remote> <src>:<dst>" does not spell out the
    destination side of the ref fully, and when <src> is not given
    as a reference but an object name, the code tries to give advice
    messages based on the type of that object.

    The type is determined by calling odb_read_object_info() and
    signalled by its return value.  The code however reported a
    programming error with BUG() when this function said that there
    is no such object, which happens when the object name is given
    as a full hexadecimal (if the object name is given as a partial
    hexadecimal or an non-existing ref, the function would have died
    without returning, so this BUG() wouldn't have triggered).  This
    is wrong.  It is an ordinary end-user mistake to give an object
    name that does not exist and treated as such.

or something?

> Replace the BUG() with an advise() displaying a helpful message about
> the oid possibly not existing.

I briefly thought this may need to be an error(), but with a larger
context, this else clause is at the end of if/else if/... cascade
for different object types, each arm of which emits per object type
advice messages, so the new one being another call to advise() would
make sense.

>  	} else {
> -		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
> -		    matched_src_name, type);
> +		advise(_("The <src> part of the refspec is an oid that doesn't exist.\n"
> +			 "Please ensure that the oid '%s' is correct."),
> +		       matched_src_name);

Unlike the other existing messages, the second line after the
diagnosis in this new message states something that is too obvious
to anybody---even to somebody who may be helped by an advice message
that says "you seem to have a commit, perhaps you meant to create a
branch?"

Thanks.

[PATCH v2 0/2] *** SUBJECT HERE ***

[Denton Liu]

*** BLURB HERE ***

Denton Liu (2):
  t5516: introduce 'push ref expression with non-existent oid src'
  remote.c: remove BUG in show_push_unqualified_ref_name_error()

 remote.c              | 3 +--
 t/t5516-fetch-push.sh | 7 +++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

Range-diff against v1:
1:  d26f355c19 = 1:  d26f355c19 t5516: introduce 'push ref expression with non-existent oid src'
2:  3eb95731ea ! 2:  2bd892b26c remote.c: remove BUG in show_push_unqualified_ref_name_error()
    @@ Metadata
      ## Commit message ##
         remote.c: remove BUG in show_push_unqualified_ref_name_error()
     
    -    In the case where a non-existent oid is given as the <src> for a
    -    refspec and the destination is unqualified, we end up hitting the BUG in
    -    show_push_unqualified_ref_name_error().
    +    When "git push <remote> <src>:<dst>" does not spell out the
    +    destination side of the ref fully, and when <src> is not given
    +    as a reference but an object name, the code tries to give advice
    +    messages based on the type of that object.
     
    -    This is because before hitting this advise message, the <src> is passed
    -    through repo_get_oid() which, upon receiving a fully qualified oid,
    -    doesn't actually check the existence of the object and just returns
    -    found. This means that it's actually possible for the
    -    odb_read_object_info() call to return not found under normal usage and
    -    thus, it's not actually a bug.
    +    The type is determined by calling odb_read_object_info() and
    +    signalled by its return value.  The code however reported a
    +    programming error with BUG() when this function said that there
    +    is no such object, which happens when the object name is given
    +    as a full hexadecimal (if the object name is given as a partial
    +    hexadecimal or an non-existing ref, the function would have died
    +    without returning, so this BUG() wouldn't have triggered).  This
    +    is wrong.  It is an ordinary end-user mistake to give an object
    +    name that does not exist and treated as such.
     
    -    Replace the BUG() with an advise() displaying a helpful message about
    -    the oid possibly not existing.
    +    Helped-by: Junio C Hamano <gitster@pobox.com>
     
      ## remote.c ##
     @@ remote.c: static void show_push_unqualified_ref_name_error(const char *dst_value,
    @@ remote.c: static void show_push_unqualified_ref_name_error(const char *dst_value
      	} else {
     -		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
     -		    matched_src_name, type);
    -+		advise(_("The <src> part of the refspec is an oid that doesn't exist.\n"
    -+			 "Please ensure that the oid '%s' is correct."),
    -+		       matched_src_name);
    ++		advise(_("The <src> part of the refspec is an oid that doesn't exist.\n"));
      	}
      }
      
-- 
2.50.1

[PATCH v2 1/2] t5516: introduce 'push ref expression with non-existent oid src'

[Denton Liu]

It is possible to trigger a Git bug by pushing a refspec where the
source is an oid that's non-existent. An example of the error message
produced is as follows:

	error: The destination you provided is not a full refname (i.e.,
	starting with "refs/"). We tried to guess what you meant by:

	- Looking for a ref that matches 'branch' on the remote side.
	- Checking if the <src> being pushed ('0000000000000000000000000000000000000001')
	  is a ref in "refs/{heads,tags}/". If so we add a corresponding
	  refs/{heads,tags}/ prefix on the remote side.

	Neither worked, so we gave up. You must fully qualify the ref.
	BUG: remote.c:1221: '0000000000000000000000000000000000000001' should be commit/tag/tree/blob, is '-1'
	fatal: the remote end hung up unexpectedly
	Aborted (core dumped)

Document this failure in a test case so that it can be confirmed fixed
later.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t5516-fetch-push.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 4e9c27b0f2..c2fcfeca92 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -509,6 +509,13 @@ test_expect_success 'push ref expression with non-existent, incomplete dest' '
 
 '
 
+test_expect_failure 'push ref expression with non-existent oid src' '
+
+	mk_test testrepo &&
+	test_must_fail git push testrepo $(test_oid 001):branch
+
+'
+
 for head in HEAD @
 do
 
-- 
2.50.1

Re: [PATCH v2 1/2] t5516: introduce 'push ref expression with non-existent oid src'

[Patrick Steinhardt]

On Mon, Aug 04, 2025 at 11:24:37PM -0700, Denton Liu wrote:
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 4e9c27b0f2..c2fcfeca92 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -509,6 +509,13 @@ test_expect_success 'push ref expression with non-existent, incomplete dest' '
>  
>  '
>  
> +test_expect_failure 'push ref expression with non-existent oid src' '
> +
> +	mk_test testrepo &&
> +	test_must_fail git push testrepo $(test_oid 001):branch
> +
> +'
> +
>  for head in HEAD @
>  do

Nit: I don't think it's necessary to implement the test in a separate
commit. Folks who want to check that your fix really does something can
trivially revert the code changes while retaining the test. I used to do
the same in the past, but received the same feedback.

Also, I think we can drop the empty surrounding lines in the test body.
Other tests in this file do the same, but that is not a good reason to
not do better for newly added tests.

Patrick

Re: [PATCH v2 1/2] t5516: introduce 'push ref expression with non-existent oid src'

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, Aug 04, 2025 at 11:24:37PM -0700, Denton Liu wrote:
>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
>> index 4e9c27b0f2..c2fcfeca92 100755
>> --- a/t/t5516-fetch-push.sh
>> +++ b/t/t5516-fetch-push.sh
>> @@ -509,6 +509,13 @@ test_expect_success 'push ref expression with non-existent, incomplete dest' '
>>  
>>  '
>>  
>> +test_expect_failure 'push ref expression with non-existent oid src' '
>> +
>> +	mk_test testrepo &&
>> +	test_must_fail git push testrepo $(test_oid 001):branch
>> +
>> +'
>> +
>>  for head in HEAD @
>>  do
>
> Nit: I don't think it's necessary to implement the test in a separate
> commit. Folks who want to check that your fix really does something can
> trivially revert the code changes while retaining the test. I used to do
> the same in the past, but received the same feedback.

A very good suggestion.

> Also, I think we can drop the empty surrounding lines in the test body.
> Other tests in this file do the same, but that is not a good reason to
> not do better for newly added tests.

Yup.  The style was from a decade ago when the test suite was being
developed, and is very out of style these days.

Thanks.

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

[Denton Liu]

When "git push <remote> <src>:<dst>" does not spell out the
destination side of the ref fully, and when <src> is not given
as a reference but an object name, the code tries to give advice
messages based on the type of that object.

The type is determined by calling odb_read_object_info() and
signalled by its return value.  The code however reported a
programming error with BUG() when this function said that there
is no such object, which happens when the object name is given
as a full hexadecimal (if the object name is given as a partial
hexadecimal or an non-existing ref, the function would have died
without returning, so this BUG() wouldn't have triggered).  This
is wrong.  It is an ordinary end-user mistake to give an object
name that does not exist and treated as such.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Thanks, I liked the way you phrased the commit message so I copied it
wholesale over.

 remote.c              | 3 +--
 t/t5516-fetch-push.sh | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/remote.c b/remote.c
index e965f022f1..4ad20110e9 100644
--- a/remote.c
+++ b/remote.c
@@ -1218,8 +1218,7 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
 			 "'%s:refs/tags/%s'?"),
 		       matched_src_name, dst_value);
 	} else {
-		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
-		    matched_src_name, type);
+		advise(_("The <src> part of the refspec is an oid that doesn't exist.\n"));
 	}
 }
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index c2fcfeca92..e064ea7433 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -509,7 +509,7 @@ test_expect_success 'push ref expression with non-existent, incomplete dest' '
 
 '
 
-test_expect_failure 'push ref expression with non-existent oid src' '
+test_expect_success 'push ref expression with non-existent oid src' '
 
 	mk_test testrepo &&
 	test_must_fail git push testrepo $(test_oid 001):branch
-- 
2.50.1

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

[Patrick Steinhardt]

On Mon, Aug 04, 2025 at 11:24:40PM -0700, Denton Liu wrote:
> When "git push <remote> <src>:<dst>" does not spell out the
> destination side of the ref fully, and when <src> is not given
> as a reference but an object name, the code tries to give advice
> messages based on the type of that object.
> 
> The type is determined by calling odb_read_object_info() and
> signalled by its return value.  The code however reported a
> programming error with BUG() when this function said that there
> is no such object, which happens when the object name is given
> as a full hexadecimal (if the object name is given as a partial
> hexadecimal or an non-existing ref, the function would have died
> without returning, so this BUG() wouldn't have triggered).  This
> is wrong.  It is an ordinary end-user mistake to give an object
> name that does not exist and treated as such.

Yup, makes sense.

> diff --git a/remote.c b/remote.c
> index e965f022f1..4ad20110e9 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1218,8 +1218,7 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
>  			 "'%s:refs/tags/%s'?"),
>  		       matched_src_name, dst_value);
>  	} else {
> -		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
> -		    matched_src_name, type);
> +		advise(_("The <src> part of the refspec is an oid that doesn't exist.\n"));

I think we should rather say "object ID", as "oid" is an abbreviation
that might not be immediately obvious to the user. Also, should we
continue to mention the object ID? Otherwise it might be hard for the
user to figure out which object ID doesn't exist in case they pass
multiple refspecs.

Patrick

[PATCH v3 0/2] remote.c: remove erroneous BUG case

[Denton Liu]

In the case where one pushes a non-existent oid to an unqualified
destination, we encounter the following BUG

	error: The destination you provided is not a full refname (i.e.,
	starting with "refs/"). We tried to guess what you meant by:

	- Looking for a ref that matches 'branch' on the remote side.
	- Checking if the <src> being pushed ('0000000000000000000000000000000000000001')
	  is a ref in "refs/{heads,tags}/". If so we add a corresponding
	  refs/{heads,tags}/ prefix on the remote side.

	Neither worked, so we gave up. You must fully qualify the ref.
	BUG: remote.c:1221: '0000000000000000000000000000000000000001' should be commit/tag/tree/blob, is '-1'
	fatal: the remote end hung up unexpectedly
	Aborted (core dumped)

However, this isn't actually a bug so replace it with an advise()
message.

Changes since v2:

* Add t5516 cleanup patch
* Squash test creation patch into the patch that fixes it
* Include the erroneous object ID in the advise message

Denton Liu (2):
  t5516: remove surrounding empty lines in test bodies
  remote.c: remove BUG in show_push_unqualified_ref_name_error()

 remote.c              |  4 ++--
 t/t5516-fetch-push.sh | 54 ++++---------------------------------------
 2 files changed, 6 insertions(+), 52 deletions(-)

Range-diff against v2:
1:  d26f355c19 ! 1:  82b09af4ca t5516: introduce 'push ref expression with non-existent oid src'
    @@ Metadata
     Author: Denton Liu <liu.denton@gmail.com>
     
      ## Commit message ##
    -    t5516: introduce 'push ref expression with non-existent oid src'
    +    t5516: remove surrounding empty lines in test bodies
     
    -    It is possible to trigger a Git bug by pushing a refspec where the
    -    source is an oid that's non-existent. An example of the error message
    -    produced is as follows:
    -
    -            error: The destination you provided is not a full refname (i.e.,
    -            starting with "refs/"). We tried to guess what you meant by:
    -
    -            - Looking for a ref that matches 'branch' on the remote side.
    -            - Checking if the <src> being pushed ('0000000000000000000000000000000000000001')
    -              is a ref in "refs/{heads,tags}/". If so we add a corresponding
    -              refs/{heads,tags}/ prefix on the remote side.
    -
    -            Neither worked, so we gave up. You must fully qualify the ref.
    -            BUG: remote.c:1221: '0000000000000000000000000000000000000001' should be commit/tag/tree/blob, is '-1'
    -            fatal: the remote end hung up unexpectedly
    -            Aborted (core dumped)
    -
    -    Document this failure in a test case so that it can be confirmed fixed
    -    later.
    +    This style with the empty lines in test bodies was from when the test
    +    suite was being developed. Remove the empty lines to match the modern
    +    test style.
     
      ## t/t5516-fetch-push.sh ##
    -@@ t/t5516-fetch-push.sh: test_expect_success 'push ref expression with non-existent, incomplete dest' '
    +@@ t/t5516-fetch-push.sh: check_push_result () {
    + }
      
    + test_expect_success setup '
    +-
    + 	>path1 &&
    + 	git add path1 &&
    + 	test_tick &&
    +@@ t/t5516-fetch-push.sh: test_expect_success setup '
    + 	test_tick &&
    + 	git commit -a -m second &&
    + 	the_commit=$(git show-ref -s --verify refs/heads/main)
    +-
      '
      
    -+test_expect_failure 'push ref expression with non-existent oid src' '
    -+
    -+	mk_test testrepo &&
    -+	test_must_fail git push testrepo $(test_oid 001):branch
    -+
    -+'
    -+
    - for head in HEAD @
    - do
    + for cmd in push fetch
    +@@ t/t5516-fetch-push.sh: test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf
    + '
      
    + test_expect_success 'push with matching heads' '
    +-
    + 	mk_test testrepo heads/main &&
    + 	git push testrepo : &&
    + 	check_push_result testrepo $the_commit heads/main
    +-
    + '
    + 
    + test_expect_success 'push with matching heads on the command line' '
    +-
    + 	mk_test testrepo heads/main &&
    + 	git push testrepo : &&
    + 	check_push_result testrepo $the_commit heads/main
    +-
    + '
    + 
    + test_expect_success 'failed (non-fast-forward) push with matching heads' '
    +-
    + 	mk_test testrepo heads/main &&
    + 	git push testrepo : &&
    + 	git commit --amend -massaged &&
    + 	test_must_fail git push testrepo &&
    + 	check_push_result testrepo $the_commit heads/main &&
    + 	git reset --hard $the_commit
    +-
    + '
    + 
    + test_expect_success 'push --force with matching heads' '
    +-
    + 	mk_test testrepo heads/main &&
    + 	git push testrepo : &&
    + 	git commit --amend -massaged &&
    + 	git push --force testrepo : &&
    + 	! check_push_result testrepo $the_commit heads/main &&
    + 	git reset --hard $the_commit
    +-
    + '
    + 
    + test_expect_success 'push with matching heads and forced update' '
    +-
    + 	mk_test testrepo heads/main &&
    + 	git push testrepo : &&
    + 	git commit --amend -massaged &&
    + 	git push testrepo +: &&
    + 	! check_push_result testrepo $the_commit heads/main &&
    + 	git reset --hard $the_commit
    +-
    + '
    + 
    + test_expect_success 'push with no ambiguity (1)' '
    +-
    + 	mk_test testrepo heads/main &&
    + 	git push testrepo main:main &&
    + 	check_push_result testrepo $the_commit heads/main
    +-
    + '
    + 
    + test_expect_success 'push with no ambiguity (2)' '
    +-
    + 	mk_test testrepo remotes/origin/main &&
    + 	git push testrepo main:origin/main &&
    + 	check_push_result testrepo $the_commit remotes/origin/main
    +-
    + '
    + 
    + test_expect_success 'push with colon-less refspec, no ambiguity' '
    +-
    + 	mk_test testrepo heads/main heads/t/main &&
    + 	git branch -f t/main main &&
    + 	git push testrepo main &&
    + 	check_push_result testrepo $the_commit heads/main &&
    + 	check_push_result testrepo $the_first_commit heads/t/main
    +-
    + '
    + 
    + test_expect_success 'push with weak ambiguity (1)' '
    +-
    + 	mk_test testrepo heads/main remotes/origin/main &&
    + 	git push testrepo main:main &&
    + 	check_push_result testrepo $the_commit heads/main &&
    + 	check_push_result testrepo $the_first_commit remotes/origin/main
    +-
    + '
    + 
    + test_expect_success 'push with weak ambiguity (2)' '
    +-
    + 	mk_test testrepo heads/main remotes/origin/main remotes/another/main &&
    + 	git push testrepo main:main &&
    + 	check_push_result testrepo $the_commit heads/main &&
    + 	check_push_result testrepo $the_first_commit remotes/origin/main remotes/another/main
    +-
    + '
    + 
    + test_expect_success 'push with ambiguity' '
    +-
    + 	mk_test testrepo heads/frotz tags/frotz &&
    + 	test_must_fail git push testrepo main:frotz &&
    + 	check_push_result testrepo $the_first_commit heads/frotz tags/frotz
    +-
    + '
    + 
    + test_expect_success 'push with onelevel ref' '
    +@@ t/t5516-fetch-push.sh: test_expect_success 'push with onelevel ref' '
    + '
    + 
    + test_expect_success 'push with colon-less refspec (1)' '
    +-
    + 	mk_test testrepo heads/frotz tags/frotz &&
    + 	git branch -f frotz main &&
    + 	git push testrepo frotz &&
    + 	check_push_result testrepo $the_commit heads/frotz &&
    + 	check_push_result testrepo $the_first_commit tags/frotz
    +-
    + '
    + 
    + test_expect_success 'push with colon-less refspec (2)' '
    +-
    + 	mk_test testrepo heads/frotz tags/frotz &&
    + 	if git show-ref --verify -q refs/heads/frotz
    + 	then
    +@@ t/t5516-fetch-push.sh: test_expect_success 'push with colon-less refspec (2)' '
    + 	git push -f testrepo frotz &&
    + 	check_push_result testrepo $the_commit tags/frotz &&
    + 	check_push_result testrepo $the_first_commit heads/frotz
    +-
    + '
    + 
    + test_expect_success 'push with colon-less refspec (3)' '
    +@@ t/t5516-fetch-push.sh: test_expect_success 'push with colon-less refspec (3)' '
    + '
    + 
    + test_expect_success 'push with colon-less refspec (4)' '
    +-
    + 	mk_test testrepo &&
    + 	if git show-ref --verify -q refs/heads/frotz
    + 	then
    +@@ t/t5516-fetch-push.sh: test_expect_success 'push with colon-less refspec (4)' '
    + 	git push testrepo frotz &&
    + 	check_push_result testrepo $the_commit tags/frotz &&
    + 	test 1 = $( cd testrepo && git show-ref | wc -l )
    +-
    + '
    + 
    + test_expect_success 'push head with non-existent, incomplete dest' '
    +-
    + 	mk_test testrepo &&
    + 	git push testrepo main:branch &&
    + 	check_push_result testrepo $the_commit heads/branch
    +-
    + '
    + 
    + test_expect_success 'push tag with non-existent, incomplete dest' '
    +-
    + 	mk_test testrepo &&
    + 	git tag -f v1.0 &&
    + 	git push testrepo v1.0:tag &&
    + 	check_push_result testrepo $the_commit tags/tag
    +-
    + '
    + 
    + test_expect_success 'push oid with non-existent, incomplete dest' '
    +-
    + 	mk_test testrepo &&
    + 	test_must_fail git push testrepo $(git rev-parse main):foo
    +-
    + '
    + 
    + test_expect_success 'push ref expression with non-existent, incomplete dest' '
    +-
    + 	mk_test testrepo &&
    + 	test_must_fail git push testrepo main^:branch
    +-
    + '
    + 
    + for head in HEAD @
    +@@ t/t5516-fetch-push.sh: do
    + 		git checkout main &&
    + 		git push testrepo $head:branch &&
    + 		check_push_result testrepo $the_commit heads/branch
    +-
    + 	'
    + 
    + 	test_expect_success "push with config remote.*.push = $head" '
    +@@ t/t5516-fetch-push.sh: test_expect_success 'push with remote.pushdefault' '
    + '
    + 
    + test_expect_success 'push with config remote.*.pushurl' '
    +-
    + 	mk_test testrepo heads/main &&
    + 	git checkout main &&
    + 	test_config remote.there.url test2repo &&
    +@@ t/t5516-fetch-push.sh: test_expect_success 'push ignores "branch." config without subsection' '
    + '
    + 
    + test_expect_success 'push with dry-run' '
    +-
    + 	mk_test testrepo heads/main &&
    + 	old_commit=$(git -C testrepo show-ref -s --verify refs/heads/main) &&
    + 	git push --dry-run testrepo : &&
    +@@ t/t5516-fetch-push.sh: test_expect_success 'push with dry-run' '
    + '
    + 
    + test_expect_success 'push updates local refs' '
    +-
    + 	mk_test testrepo heads/main &&
    + 	mk_child testrepo child &&
    + 	(
    +@@ t/t5516-fetch-push.sh: test_expect_success 'push updates local refs' '
    + 		test $(git rev-parse main) = \
    + 			$(git rev-parse remotes/origin/main)
    + 	)
    +-
    + '
    + 
    + test_expect_success 'push updates up-to-date local refs' '
    +-
    + 	mk_test testrepo heads/main &&
    + 	mk_child testrepo child1 &&
    + 	mk_child testrepo child2 &&
    +@@ t/t5516-fetch-push.sh: test_expect_success 'push updates up-to-date local refs' '
    + 		test $(git rev-parse main) = \
    + 			$(git rev-parse remotes/origin/main)
    + 	)
    +-
    + '
    + 
    + test_expect_success 'push preserves up-to-date packed refs' '
    +-
    + 	mk_test testrepo heads/main &&
    + 	mk_child testrepo child &&
    + 	(
    +@@ t/t5516-fetch-push.sh: test_expect_success 'push preserves up-to-date packed refs' '
    + 		git push &&
    + 		! test -f .git/refs/remotes/origin/main
    + 	)
    +-
    + '
    + 
    + test_expect_success 'push does not update local refs on failure' '
    +-
    + 	mk_test testrepo heads/main &&
    + 	mk_child testrepo child &&
    + 	echo "#!/no/frobnication/today" >testrepo/.git/hooks/pre-receive &&
    +@@ t/t5516-fetch-push.sh: test_expect_success 'push does not update local refs on failure' '
    + 		test $(git rev-parse main) != \
    + 			$(git rev-parse remotes/origin/main)
    + 	)
    +-
    + '
    + 
    + test_expect_success 'allow deleting an invalid remote ref' '
    +-
    + 	mk_test testrepo heads/branch &&
    + 	rm -f testrepo/.git/objects/??/* &&
    + 	git push testrepo :refs/heads/branch &&
    + 	(cd testrepo && test_must_fail git rev-parse --verify refs/heads/branch)
    +-
    + '
    + 
    + test_expect_success 'pushing valid refs triggers post-receive and post-update hooks' '
2:  2bd892b26c ! 2:  938dfb8d4e remote.c: remove BUG in show_push_unqualified_ref_name_error()
    @@ Commit message
         is wrong.  It is an ordinary end-user mistake to give an object
         name that does not exist and treated as such.
     
    +    An example of the error message produced is as follows:
    +
    +            error: The destination you provided is not a full refname (i.e.,
    +            starting with "refs/"). We tried to guess what you meant by:
    +
    +            - Looking for a ref that matches 'branch' on the remote side.
    +            - Checking if the <src> being pushed ('0000000000000000000000000000000000000001')
    +              is a ref in "refs/{heads,tags}/". If so we add a corresponding
    +              refs/{heads,tags}/ prefix on the remote side.
    +
    +            Neither worked, so we gave up. You must fully qualify the ref.
    +            BUG: remote.c:1221: '0000000000000000000000000000000000000001' should be commit/tag/tree/blob, is '-1'
    +            fatal: the remote end hung up unexpectedly
    +            Aborted (core dumped)
    +
         Helped-by: Junio C Hamano <gitster@pobox.com>
     
      ## remote.c ##
    @@ remote.c: static void show_push_unqualified_ref_name_error(const char *dst_value
      	} else {
     -		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
     -		    matched_src_name, type);
    -+		advise(_("The <src> part of the refspec is an oid that doesn't exist.\n"));
    ++		advise(_("The <src> part of the refspec ('%s') is an object ID that doesn't exist.\n"),
    ++		       matched_src_name);
      	}
      }
      
     
      ## t/t5516-fetch-push.sh ##
     @@ t/t5516-fetch-push.sh: test_expect_success 'push ref expression with non-existent, incomplete dest' '
    - 
    + 	test_must_fail git push testrepo main^:branch
      '
      
    --test_expect_failure 'push ref expression with non-existent oid src' '
     +test_expect_success 'push ref expression with non-existent oid src' '
    ++	mk_test testrepo &&
    ++	test_must_fail git push testrepo $(test_oid 001):branch
    ++'
    ++
    + for head in HEAD @
    + do
      
    - 	mk_test testrepo &&
    - 	test_must_fail git push testrepo $(test_oid 001):branch
-- 
2.50.1

[PATCH v3 1/2] t5516: remove surrounding empty lines in test bodies

[Denton Liu]

This style with the empty lines in test bodies was from when the test
suite was being developed. Remove the empty lines to match the modern
test style.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t5516-fetch-push.sh | 51 -------------------------------------------
 1 file changed, 51 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 4e9c27b0f2..8eddf3e40d 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -105,7 +105,6 @@ check_push_result () {
 }
 
 test_expect_success setup '
-
 	>path1 &&
 	git add path1 &&
 	test_tick &&
@@ -117,7 +116,6 @@ test_expect_success setup '
 	test_tick &&
 	git commit -a -m second &&
 	the_commit=$(git show-ref -s --verify refs/heads/main)
-
 '
 
 for cmd in push fetch
@@ -322,104 +320,82 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf
 '
 
 test_expect_success 'push with matching heads' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo : &&
 	check_push_result testrepo $the_commit heads/main
-
 '
 
 test_expect_success 'push with matching heads on the command line' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo : &&
 	check_push_result testrepo $the_commit heads/main
-
 '
 
 test_expect_success 'failed (non-fast-forward) push with matching heads' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo : &&
 	git commit --amend -massaged &&
 	test_must_fail git push testrepo &&
 	check_push_result testrepo $the_commit heads/main &&
 	git reset --hard $the_commit
-
 '
 
 test_expect_success 'push --force with matching heads' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo : &&
 	git commit --amend -massaged &&
 	git push --force testrepo : &&
 	! check_push_result testrepo $the_commit heads/main &&
 	git reset --hard $the_commit
-
 '
 
 test_expect_success 'push with matching heads and forced update' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo : &&
 	git commit --amend -massaged &&
 	git push testrepo +: &&
 	! check_push_result testrepo $the_commit heads/main &&
 	git reset --hard $the_commit
-
 '
 
 test_expect_success 'push with no ambiguity (1)' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo main:main &&
 	check_push_result testrepo $the_commit heads/main
-
 '
 
 test_expect_success 'push with no ambiguity (2)' '
-
 	mk_test testrepo remotes/origin/main &&
 	git push testrepo main:origin/main &&
 	check_push_result testrepo $the_commit remotes/origin/main
-
 '
 
 test_expect_success 'push with colon-less refspec, no ambiguity' '
-
 	mk_test testrepo heads/main heads/t/main &&
 	git branch -f t/main main &&
 	git push testrepo main &&
 	check_push_result testrepo $the_commit heads/main &&
 	check_push_result testrepo $the_first_commit heads/t/main
-
 '
 
 test_expect_success 'push with weak ambiguity (1)' '
-
 	mk_test testrepo heads/main remotes/origin/main &&
 	git push testrepo main:main &&
 	check_push_result testrepo $the_commit heads/main &&
 	check_push_result testrepo $the_first_commit remotes/origin/main
-
 '
 
 test_expect_success 'push with weak ambiguity (2)' '
-
 	mk_test testrepo heads/main remotes/origin/main remotes/another/main &&
 	git push testrepo main:main &&
 	check_push_result testrepo $the_commit heads/main &&
 	check_push_result testrepo $the_first_commit remotes/origin/main remotes/another/main
-
 '
 
 test_expect_success 'push with ambiguity' '
-
 	mk_test testrepo heads/frotz tags/frotz &&
 	test_must_fail git push testrepo main:frotz &&
 	check_push_result testrepo $the_first_commit heads/frotz tags/frotz
-
 '
 
 test_expect_success 'push with onelevel ref' '
@@ -428,17 +404,14 @@ test_expect_success 'push with onelevel ref' '
 '
 
 test_expect_success 'push with colon-less refspec (1)' '
-
 	mk_test testrepo heads/frotz tags/frotz &&
 	git branch -f frotz main &&
 	git push testrepo frotz &&
 	check_push_result testrepo $the_commit heads/frotz &&
 	check_push_result testrepo $the_first_commit tags/frotz
-
 '
 
 test_expect_success 'push with colon-less refspec (2)' '
-
 	mk_test testrepo heads/frotz tags/frotz &&
 	if git show-ref --verify -q refs/heads/frotz
 	then
@@ -448,7 +421,6 @@ test_expect_success 'push with colon-less refspec (2)' '
 	git push -f testrepo frotz &&
 	check_push_result testrepo $the_commit tags/frotz &&
 	check_push_result testrepo $the_first_commit heads/frotz
-
 '
 
 test_expect_success 'push with colon-less refspec (3)' '
@@ -465,7 +437,6 @@ test_expect_success 'push with colon-less refspec (3)' '
 '
 
 test_expect_success 'push with colon-less refspec (4)' '
-
 	mk_test testrepo &&
 	if git show-ref --verify -q refs/heads/frotz
 	then
@@ -475,38 +446,29 @@ test_expect_success 'push with colon-less refspec (4)' '
 	git push testrepo frotz &&
 	check_push_result testrepo $the_commit tags/frotz &&
 	test 1 = $( cd testrepo && git show-ref | wc -l )
-
 '
 
 test_expect_success 'push head with non-existent, incomplete dest' '
-
 	mk_test testrepo &&
 	git push testrepo main:branch &&
 	check_push_result testrepo $the_commit heads/branch
-
 '
 
 test_expect_success 'push tag with non-existent, incomplete dest' '
-
 	mk_test testrepo &&
 	git tag -f v1.0 &&
 	git push testrepo v1.0:tag &&
 	check_push_result testrepo $the_commit tags/tag
-
 '
 
 test_expect_success 'push oid with non-existent, incomplete dest' '
-
 	mk_test testrepo &&
 	test_must_fail git push testrepo $(git rev-parse main):foo
-
 '
 
 test_expect_success 'push ref expression with non-existent, incomplete dest' '
-
 	mk_test testrepo &&
 	test_must_fail git push testrepo main^:branch
-
 '
 
 for head in HEAD @
@@ -550,7 +512,6 @@ do
 		git checkout main &&
 		git push testrepo $head:branch &&
 		check_push_result testrepo $the_commit heads/branch
-
 	'
 
 	test_expect_success "push with config remote.*.push = $head" '
@@ -596,7 +557,6 @@ test_expect_success 'push with remote.pushdefault' '
 '
 
 test_expect_success 'push with config remote.*.pushurl' '
-
 	mk_test testrepo heads/main &&
 	git checkout main &&
 	test_config remote.there.url test2repo &&
@@ -655,7 +615,6 @@ test_expect_success 'push ignores "branch." config without subsection' '
 '
 
 test_expect_success 'push with dry-run' '
-
 	mk_test testrepo heads/main &&
 	old_commit=$(git -C testrepo show-ref -s --verify refs/heads/main) &&
 	git push --dry-run testrepo : &&
@@ -663,7 +622,6 @@ test_expect_success 'push with dry-run' '
 '
 
 test_expect_success 'push updates local refs' '
-
 	mk_test testrepo heads/main &&
 	mk_child testrepo child &&
 	(
@@ -673,11 +631,9 @@ test_expect_success 'push updates local refs' '
 		test $(git rev-parse main) = \
 			$(git rev-parse remotes/origin/main)
 	)
-
 '
 
 test_expect_success 'push updates up-to-date local refs' '
-
 	mk_test testrepo heads/main &&
 	mk_child testrepo child1 &&
 	mk_child testrepo child2 &&
@@ -689,11 +645,9 @@ test_expect_success 'push updates up-to-date local refs' '
 		test $(git rev-parse main) = \
 			$(git rev-parse remotes/origin/main)
 	)
-
 '
 
 test_expect_success 'push preserves up-to-date packed refs' '
-
 	mk_test testrepo heads/main &&
 	mk_child testrepo child &&
 	(
@@ -701,11 +655,9 @@ test_expect_success 'push preserves up-to-date packed refs' '
 		git push &&
 		! test -f .git/refs/remotes/origin/main
 	)
-
 '
 
 test_expect_success 'push does not update local refs on failure' '
-
 	mk_test testrepo heads/main &&
 	mk_child testrepo child &&
 	echo "#!/no/frobnication/today" >testrepo/.git/hooks/pre-receive &&
@@ -717,16 +669,13 @@ test_expect_success 'push does not update local refs on failure' '
 		test $(git rev-parse main) != \
 			$(git rev-parse remotes/origin/main)
 	)
-
 '
 
 test_expect_success 'allow deleting an invalid remote ref' '
-
 	mk_test testrepo heads/branch &&
 	rm -f testrepo/.git/objects/??/* &&
 	git push testrepo :refs/heads/branch &&
 	(cd testrepo && test_must_fail git rev-parse --verify refs/heads/branch)
-
 '
 
 test_expect_success 'pushing valid refs triggers post-receive and post-update hooks' '
-- 
2.50.1

Re: [PATCH v3 1/2] t5516: remove surrounding empty lines in test bodies

[Patrick Steinhardt]

On Tue, Aug 05, 2025 at 09:53:39PM -0700, Denton Liu wrote:
> This style with the empty lines in test bodies was from when the test
> suite was being developed. Remove the empty lines to match the modern
> test style.

Thanks for going the extra mile. Tacking on while-at-it fixes like this
is what ensures that overall the Git codebase is trending towards our
modern code style.

Patrick

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

[Denton Liu]

When "git push <remote> <src>:<dst>" does not spell out the
destination side of the ref fully, and when <src> is not given
as a reference but an object name, the code tries to give advice
messages based on the type of that object.

The type is determined by calling odb_read_object_info() and
signalled by its return value.  The code however reported a
programming error with BUG() when this function said that there
is no such object, which happens when the object name is given
as a full hexadecimal (if the object name is given as a partial
hexadecimal or an non-existing ref, the function would have died
without returning, so this BUG() wouldn't have triggered).  This
is wrong.  It is an ordinary end-user mistake to give an object
name that does not exist and treated as such.

An example of the error message produced is as follows:

	error: The destination you provided is not a full refname (i.e.,
	starting with "refs/"). We tried to guess what you meant by:

	- Looking for a ref that matches 'branch' on the remote side.
	- Checking if the <src> being pushed ('0000000000000000000000000000000000000001')
	  is a ref in "refs/{heads,tags}/". If so we add a corresponding
	  refs/{heads,tags}/ prefix on the remote side.

	Neither worked, so we gave up. You must fully qualify the ref.
	BUG: remote.c:1221: '0000000000000000000000000000000000000001' should be commit/tag/tree/blob, is '-1'
	fatal: the remote end hung up unexpectedly
	Aborted (core dumped)

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 remote.c              | 4 ++--
 t/t5516-fetch-push.sh | 5 +++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index e965f022f1..465e0ea0eb 100644
--- a/remote.c
+++ b/remote.c
@@ -1218,8 +1218,8 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
 			 "'%s:refs/tags/%s'?"),
 		       matched_src_name, dst_value);
 	} else {
-		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
-		    matched_src_name, type);
+		advise(_("The <src> part of the refspec ('%s') is an object ID that doesn't exist.\n"),
+		       matched_src_name);
 	}
 }
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 8eddf3e40d..46926e7bbd 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -471,6 +471,11 @@ test_expect_success 'push ref expression with non-existent, incomplete dest' '
 	test_must_fail git push testrepo main^:branch
 '
 
+test_expect_success 'push ref expression with non-existent oid src' '
+	mk_test testrepo &&
+	test_must_fail git push testrepo $(test_oid 001):branch
+'
+
 for head in HEAD @
 do
 
-- 
2.50.1

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

[Patrick Steinhardt]

On Tue, Aug 05, 2025 at 09:53:42PM -0700, Denton Liu wrote:
> diff --git a/remote.c b/remote.c
> index e965f022f1..465e0ea0eb 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1218,8 +1218,8 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
>  			 "'%s:refs/tags/%s'?"),
>  		       matched_src_name, dst_value);
>  	} else {
> -		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
> -		    matched_src_name, type);
> +		advise(_("The <src> part of the refspec ('%s') is an object ID that doesn't exist.\n"),
> +		       matched_src_name);
>  	}
>  }

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.

Regarding the logic this looks sensible to me. We have already handled
all valid object types in the cases leading up to this final `else`, so
we can be sure that we weren't able to look up the object. And warning
about that case feels reasonable.

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!

Patrick

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> 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.

[PATCH] remote.c: convert if-else tower to switch

[Denton Liu]

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

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
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

Re: [PATCH] remote.c: convert if-else tower to switch

[Patrick Steinhardt]

On Wed, Aug 06, 2025 at 09:30:20PM -0700, Denton Liu wrote:
> For better readability, convert the if-else tower into a switch
> statement.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> 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);

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

[PATCH v2] remote.c: convert if-else tower to switch

[Denton Liu]

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

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 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

Re: [PATCH v2] remote.c: convert if-else tower to switch

[Ben Knoble]

> Le 7 août 2025 à 05:20, Denton Liu <liu.denton@gmail.com> a écrit :
> 
> For better readability, convert the if-else tower into a switch
> statement.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> 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:

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

Re: [PATCH v2] remote.c: convert if-else tower to switch

[Eric Sunshine]

On Thu, Aug 7, 2025 at 8:35 AM Ben Knoble <ben.knoble@gmail.com> wrote:
> > Le 7 août 2025 à 05:20, Denton Liu <liu.denton@gmail.com> a écrit :
> > For better readability, convert the if-else tower into a switch
> > statement.
> >
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> > 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).

Re: [PATCH] remote.c: convert if-else tower to switch

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

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

The reference to "tower" is something new to me.  A quick search
seems to tell me that "if-else cascade", which is what I've been
using around here, is not popular, either.  "if-else ladder" is the
term more often used, it seems.


> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> 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(-)

OK.  Sitting down and thinking about it, the reason is obvious, but
TIL that switch/case is slightly more verbose ;-).

> +	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"),

This line alone is overly long; it is not part of _this_ patch but
is showing the state after that BUG()->advise() fix, so it should be
fixed there, I think?

>  		       matched_src_name);
> +		break;
>  	}
>  }

[PATCH v4 0/3] remote.c: remove erroneous BUG case

[Denton Liu]

In the case where one pushes a non-existent oid to an unqualified
destination, we encounter the following BUG

	error: The destination you provided is not a full refname (i.e.,
	starting with "refs/"). We tried to guess what you meant by:

	- Looking for a ref that matches 'branch' on the remote side.
	- Checking if the <src> being pushed ('0000000000000000000000000000000000000001')
	  is a ref in "refs/{heads,tags}/". If so we add a corresponding
	  refs/{heads,tags}/ prefix on the remote side.

	Neither worked, so we gave up. You must fully qualify the ref.
	BUG: remote.c:1221: '0000000000000000000000000000000000000001' should be commit/tag/tree/blob, is '-1'
	fatal: the remote end hung up unexpectedly
	Aborted (core dumped)

However, this isn't actually a bug so replace it with an advise()
message.

Changes since v3:

* Include the switch statement refactoring patch as a prelude to the
  functional patch
* Change "if-else tower" to "if-else ladder"
* Shortened the overly long advise() line
* Rebased on latest 'master' to avoid merge conflict introduced earlier
  in the merge cycle (this should be fine since we haven't merged to
  'next' yet right?)

Changes since v2:

* Add t5516 cleanup patch
* Squash test creation patch into the patch that fixes it
* Include the erroneous object ID in the advise message

Denton Liu (3):
  t5516: remove surrounding empty lines in test bodies
  remote.c: convert if-else ladder to switch
  remote.c: remove BUG in show_push_unqualified_ref_name_error()

 remote.c              | 24 +++++++++++--------
 t/t5516-fetch-push.sh | 54 ++++---------------------------------------
 2 files changed, 19 insertions(+), 59 deletions(-)

Range-diff against v3:
1:  82b09af4ca = 1:  d31f320fdb t5516: remove surrounding empty lines in test bodies
2:  938dfb8d4e ! 2:  ee6d69bcaf remote.c: remove BUG in show_push_unqualified_ref_name_error()
[... deleted the diff of diff because it's mostly noise]
-:  ---------- > 3:  3d84072dc7 remote.c: remove BUG in show_push_unqualified_ref_name_error()
-- 
2.50.1

[PATCH v4 1/3] t5516: remove surrounding empty lines in test bodies

[Denton Liu]

This style with the empty lines in test bodies was from when the test
suite was being developed. Remove the empty lines to match the modern
test style.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t5516-fetch-push.sh | 51 -------------------------------------------
 1 file changed, 51 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 4e9c27b0f2..8eddf3e40d 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -105,7 +105,6 @@ check_push_result () {
 }
 
 test_expect_success setup '
-
 	>path1 &&
 	git add path1 &&
 	test_tick &&
@@ -117,7 +116,6 @@ test_expect_success setup '
 	test_tick &&
 	git commit -a -m second &&
 	the_commit=$(git show-ref -s --verify refs/heads/main)
-
 '
 
 for cmd in push fetch
@@ -322,104 +320,82 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf
 '
 
 test_expect_success 'push with matching heads' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo : &&
 	check_push_result testrepo $the_commit heads/main
-
 '
 
 test_expect_success 'push with matching heads on the command line' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo : &&
 	check_push_result testrepo $the_commit heads/main
-
 '
 
 test_expect_success 'failed (non-fast-forward) push with matching heads' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo : &&
 	git commit --amend -massaged &&
 	test_must_fail git push testrepo &&
 	check_push_result testrepo $the_commit heads/main &&
 	git reset --hard $the_commit
-
 '
 
 test_expect_success 'push --force with matching heads' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo : &&
 	git commit --amend -massaged &&
 	git push --force testrepo : &&
 	! check_push_result testrepo $the_commit heads/main &&
 	git reset --hard $the_commit
-
 '
 
 test_expect_success 'push with matching heads and forced update' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo : &&
 	git commit --amend -massaged &&
 	git push testrepo +: &&
 	! check_push_result testrepo $the_commit heads/main &&
 	git reset --hard $the_commit
-
 '
 
 test_expect_success 'push with no ambiguity (1)' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo main:main &&
 	check_push_result testrepo $the_commit heads/main
-
 '
 
 test_expect_success 'push with no ambiguity (2)' '
-
 	mk_test testrepo remotes/origin/main &&
 	git push testrepo main:origin/main &&
 	check_push_result testrepo $the_commit remotes/origin/main
-
 '
 
 test_expect_success 'push with colon-less refspec, no ambiguity' '
-
 	mk_test testrepo heads/main heads/t/main &&
 	git branch -f t/main main &&
 	git push testrepo main &&
 	check_push_result testrepo $the_commit heads/main &&
 	check_push_result testrepo $the_first_commit heads/t/main
-
 '
 
 test_expect_success 'push with weak ambiguity (1)' '
-
 	mk_test testrepo heads/main remotes/origin/main &&
 	git push testrepo main:main &&
 	check_push_result testrepo $the_commit heads/main &&
 	check_push_result testrepo $the_first_commit remotes/origin/main
-
 '
 
 test_expect_success 'push with weak ambiguity (2)' '
-
 	mk_test testrepo heads/main remotes/origin/main remotes/another/main &&
 	git push testrepo main:main &&
 	check_push_result testrepo $the_commit heads/main &&
 	check_push_result testrepo $the_first_commit remotes/origin/main remotes/another/main
-
 '
 
 test_expect_success 'push with ambiguity' '
-
 	mk_test testrepo heads/frotz tags/frotz &&
 	test_must_fail git push testrepo main:frotz &&
 	check_push_result testrepo $the_first_commit heads/frotz tags/frotz
-
 '
 
 test_expect_success 'push with onelevel ref' '
@@ -428,17 +404,14 @@ test_expect_success 'push with onelevel ref' '
 '
 
 test_expect_success 'push with colon-less refspec (1)' '
-
 	mk_test testrepo heads/frotz tags/frotz &&
 	git branch -f frotz main &&
 	git push testrepo frotz &&
 	check_push_result testrepo $the_commit heads/frotz &&
 	check_push_result testrepo $the_first_commit tags/frotz
-
 '
 
 test_expect_success 'push with colon-less refspec (2)' '
-
 	mk_test testrepo heads/frotz tags/frotz &&
 	if git show-ref --verify -q refs/heads/frotz
 	then
@@ -448,7 +421,6 @@ test_expect_success 'push with colon-less refspec (2)' '
 	git push -f testrepo frotz &&
 	check_push_result testrepo $the_commit tags/frotz &&
 	check_push_result testrepo $the_first_commit heads/frotz
-
 '
 
 test_expect_success 'push with colon-less refspec (3)' '
@@ -465,7 +437,6 @@ test_expect_success 'push with colon-less refspec (3)' '
 '
 
 test_expect_success 'push with colon-less refspec (4)' '
-
 	mk_test testrepo &&
 	if git show-ref --verify -q refs/heads/frotz
 	then
@@ -475,38 +446,29 @@ test_expect_success 'push with colon-less refspec (4)' '
 	git push testrepo frotz &&
 	check_push_result testrepo $the_commit tags/frotz &&
 	test 1 = $( cd testrepo && git show-ref | wc -l )
-
 '
 
 test_expect_success 'push head with non-existent, incomplete dest' '
-
 	mk_test testrepo &&
 	git push testrepo main:branch &&
 	check_push_result testrepo $the_commit heads/branch
-
 '
 
 test_expect_success 'push tag with non-existent, incomplete dest' '
-
 	mk_test testrepo &&
 	git tag -f v1.0 &&
 	git push testrepo v1.0:tag &&
 	check_push_result testrepo $the_commit tags/tag
-
 '
 
 test_expect_success 'push oid with non-existent, incomplete dest' '
-
 	mk_test testrepo &&
 	test_must_fail git push testrepo $(git rev-parse main):foo
-
 '
 
 test_expect_success 'push ref expression with non-existent, incomplete dest' '
-
 	mk_test testrepo &&
 	test_must_fail git push testrepo main^:branch
-
 '
 
 for head in HEAD @
@@ -550,7 +512,6 @@ do
 		git checkout main &&
 		git push testrepo $head:branch &&
 		check_push_result testrepo $the_commit heads/branch
-
 	'
 
 	test_expect_success "push with config remote.*.push = $head" '
@@ -596,7 +557,6 @@ test_expect_success 'push with remote.pushdefault' '
 '
 
 test_expect_success 'push with config remote.*.pushurl' '
-
 	mk_test testrepo heads/main &&
 	git checkout main &&
 	test_config remote.there.url test2repo &&
@@ -655,7 +615,6 @@ test_expect_success 'push ignores "branch." config without subsection' '
 '
 
 test_expect_success 'push with dry-run' '
-
 	mk_test testrepo heads/main &&
 	old_commit=$(git -C testrepo show-ref -s --verify refs/heads/main) &&
 	git push --dry-run testrepo : &&
@@ -663,7 +622,6 @@ test_expect_success 'push with dry-run' '
 '
 
 test_expect_success 'push updates local refs' '
-
 	mk_test testrepo heads/main &&
 	mk_child testrepo child &&
 	(
@@ -673,11 +631,9 @@ test_expect_success 'push updates local refs' '
 		test $(git rev-parse main) = \
 			$(git rev-parse remotes/origin/main)
 	)
-
 '
 
 test_expect_success 'push updates up-to-date local refs' '
-
 	mk_test testrepo heads/main &&
 	mk_child testrepo child1 &&
 	mk_child testrepo child2 &&
@@ -689,11 +645,9 @@ test_expect_success 'push updates up-to-date local refs' '
 		test $(git rev-parse main) = \
 			$(git rev-parse remotes/origin/main)
 	)
-
 '
 
 test_expect_success 'push preserves up-to-date packed refs' '
-
 	mk_test testrepo heads/main &&
 	mk_child testrepo child &&
 	(
@@ -701,11 +655,9 @@ test_expect_success 'push preserves up-to-date packed refs' '
 		git push &&
 		! test -f .git/refs/remotes/origin/main
 	)
-
 '
 
 test_expect_success 'push does not update local refs on failure' '
-
 	mk_test testrepo heads/main &&
 	mk_child testrepo child &&
 	echo "#!/no/frobnication/today" >testrepo/.git/hooks/pre-receive &&
@@ -717,16 +669,13 @@ test_expect_success 'push does not update local refs on failure' '
 		test $(git rev-parse main) != \
 			$(git rev-parse remotes/origin/main)
 	)
-
 '
 
 test_expect_success 'allow deleting an invalid remote ref' '
-
 	mk_test testrepo heads/branch &&
 	rm -f testrepo/.git/objects/??/* &&
 	git push testrepo :refs/heads/branch &&
 	(cd testrepo && test_must_fail git rev-parse --verify refs/heads/branch)
-
 '
 
 test_expect_success 'pushing valid refs triggers post-receive and post-update hooks' '
-- 
2.50.1

[PATCH v4 2/3] remote.c: convert if-else ladder to switch

[Denton Liu]

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

Suggested-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 remote.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/remote.c b/remote.c
index 88f991795b..61e2c9951a 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,28 +1195,33 @@ 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:
 		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
 		    matched_src_name, type);
 	}
-- 
2.50.1

Re: [PATCH v4 2/3] remote.c: convert if-else ladder to switch

[Patrick Steinhardt]

On Thu, Aug 07, 2025 at 09:41:11PM -0700, Denton Liu wrote:
> diff --git a/remote.c b/remote.c
> index 88f991795b..61e2c9951a 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;

> @@ -1196,28 +1195,33 @@ 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:
>  		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
>  		    matched_src_name, type);

We can't remove the `type` variable in this patch already -- it's still
used by this call to `BUG()`. But we can drop the variable in the next
patch, where that call is converted to `advise()`.

So I'd recommend to either move this patch to after the next patch or to
keep the `type` variable here and remove it in the next patch.

Patrick

Re: [PATCH v4 2/3] remote.c: convert if-else ladder to switch

[Denton Liu]

On Fri, Aug 08, 2025 at 07:43:12AM +0200, Patrick Steinhardt wrote:
> We can't remove the `type` variable in this patch already -- it's still
> used by this call to `BUG()`. But we can drop the variable in the next
> patch, where that call is converted to `advise()`.

Ugh, that's what I get for rushing this patchset out without doing a
test compile :/

Thanks for catching that. Another patchset incoming

-Denton

[PATCH v4 3/3] remote.c: remove BUG in show_push_unqualified_ref_name_error()

[Denton Liu]

When "git push <remote> <src>:<dst>" does not spell out the
destination side of the ref fully, and when <src> is not given
as a reference but an object name, the code tries to give advice
messages based on the type of that object.

The type is determined by calling odb_read_object_info() and
signalled by its return value.  The code however reported a
programming error with BUG() when this function said that there
is no such object, which happens when the object name is given
as a full hexadecimal (if the object name is given as a partial
hexadecimal or an non-existing ref, the function would have died
without returning, so this BUG() wouldn't have triggered).  This
is wrong.  It is an ordinary end-user mistake to give an object
name that does not exist and treated as such.

An example of the error message produced is as follows:

	error: The destination you provided is not a full refname (i.e.,
	starting with "refs/"). We tried to guess what you meant by:

	- Looking for a ref that matches 'branch' on the remote side.
	- Checking if the <src> being pushed ('0000000000000000000000000000000000000001')
	  is a ref in "refs/{heads,tags}/". If so we add a corresponding
	  refs/{heads,tags}/ prefix on the remote side.

	Neither worked, so we gave up. You must fully qualify the ref.
	BUG: remote.c:1221: '0000000000000000000000000000000000000001' should be commit/tag/tree/blob, is '-1'
	fatal: the remote end hung up unexpectedly
	Aborted (core dumped)

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 remote.c              | 6 ++++--
 t/t5516-fetch-push.sh | 5 +++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index 61e2c9951a..df88914716 100644
--- a/remote.c
+++ b/remote.c
@@ -1222,8 +1222,10 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
 		       matched_src_name, dst_value);
 		break;
 	default:
-		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
-		    matched_src_name, type);
+		advise(_("The <src> part of the refspec ('%s') "
+			 "is an object ID that doesn't exist.\n"),
+		       matched_src_name);
+		break;
 	}
 }
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 8eddf3e40d..46926e7bbd 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -471,6 +471,11 @@ test_expect_success 'push ref expression with non-existent, incomplete dest' '
 	test_must_fail git push testrepo main^:branch
 '
 
+test_expect_success 'push ref expression with non-existent oid src' '
+	mk_test testrepo &&
+	test_must_fail git push testrepo $(test_oid 001):branch
+'
+
 for head in HEAD @
 do
 
-- 
2.50.1

[PATCH v5 0/3] remote.c: remove erroneous BUG case

[Denton Liu]

In the case where one pushes a non-existent oid to an unqualified
destination, we encounter the following BUG

	error: The destination you provided is not a full refname (i.e.,
	starting with "refs/"). We tried to guess what you meant by:

	- Looking for a ref that matches 'branch' on the remote side.
	- Checking if the <src> being pushed ('0000000000000000000000000000000000000001')
	  is a ref in "refs/{heads,tags}/". If so we add a corresponding
	  refs/{heads,tags}/ prefix on the remote side.

	Neither worked, so we gave up. You must fully qualify the ref.
	BUG: remote.c:1221: '0000000000000000000000000000000000000001' should be commit/tag/tree/blob, is '-1'
	fatal: the remote end hung up unexpectedly
	Aborted (core dumped)

However, this isn't actually a bug so replace it with an advise()
message.

Changes since v4:

* Put the switch statement refactoring patch last so that we don't get
  compile errors from a missing variable

Changes since v3:

* Include the switch statement refactoring patch as a prelude to the
  functional patch
* Change "if-else tower" to "if-else ladder"
* Shortened the overly long advise() line
* Rebased on latest 'master' to avoid merge conflict introduced earlier
  in the merge cycle (this should be fine since we haven't merged to
  'next' yet right?)

Changes since v2:

* Add t5516 cleanup patch
* Squash test creation patch into the patch that fixes it
* Include the erroneous object ID in the advise message

Denton Liu (3):
  t5516: remove surrounding empty lines in test bodies
  remote.c: remove BUG in show_push_unqualified_ref_name_error()
  remote.c: convert if-else ladder to switch

 remote.c              | 24 +++++++++++--------
 t/t5516-fetch-push.sh | 54 ++++---------------------------------------
 2 files changed, 19 insertions(+), 59 deletions(-)

Range-diff against v4:
1:  d31f320fdb = 1:  d31f320fdb t5516: remove surrounding empty lines in test bodies
3:  3d84072dc7 ! 2:  d21612fca6 remote.c: remove BUG in show_push_unqualified_ref_name_error()
    @@ Commit message
     
      ## remote.c ##
     @@ remote.c: static void show_push_unqualified_ref_name_error(const char *dst_value,
    + 			 "'%s:refs/tags/%s'?"),
      		       matched_src_name, dst_value);
    - 		break;
    - 	default:
    + 	} else {
     -		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
     -		    matched_src_name, type);
     +		advise(_("The <src> part of the refspec ('%s') "
     +			 "is an object ID that doesn't exist.\n"),
     +		       matched_src_name);
    -+		break;
      	}
      }
      
2:  ee6d69bcaf ! 3:  cbda61af5c remote.c: convert if-else ladder to switch
    @@ remote.c: static void show_push_unqualified_ref_name_error(const char *dst_value
     -	} else {
     +		break;
     +	default:
    - 		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
    - 		    matched_src_name, type);
    + 		advise(_("The <src> part of the refspec ('%s') "
    + 			 "is an object ID that doesn't exist.\n"),
    + 		       matched_src_name);
    ++		break;
      	}
    + }
    + 
-- 
2.50.1

[PATCH v5 1/3] t5516: remove surrounding empty lines in test bodies

[Denton Liu]

This style with the empty lines in test bodies was from when the test
suite was being developed. Remove the empty lines to match the modern
test style.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t5516-fetch-push.sh | 51 -------------------------------------------
 1 file changed, 51 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 4e9c27b0f2..8eddf3e40d 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -105,7 +105,6 @@ check_push_result () {
 }
 
 test_expect_success setup '
-
 	>path1 &&
 	git add path1 &&
 	test_tick &&
@@ -117,7 +116,6 @@ test_expect_success setup '
 	test_tick &&
 	git commit -a -m second &&
 	the_commit=$(git show-ref -s --verify refs/heads/main)
-
 '
 
 for cmd in push fetch
@@ -322,104 +320,82 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf
 '
 
 test_expect_success 'push with matching heads' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo : &&
 	check_push_result testrepo $the_commit heads/main
-
 '
 
 test_expect_success 'push with matching heads on the command line' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo : &&
 	check_push_result testrepo $the_commit heads/main
-
 '
 
 test_expect_success 'failed (non-fast-forward) push with matching heads' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo : &&
 	git commit --amend -massaged &&
 	test_must_fail git push testrepo &&
 	check_push_result testrepo $the_commit heads/main &&
 	git reset --hard $the_commit
-
 '
 
 test_expect_success 'push --force with matching heads' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo : &&
 	git commit --amend -massaged &&
 	git push --force testrepo : &&
 	! check_push_result testrepo $the_commit heads/main &&
 	git reset --hard $the_commit
-
 '
 
 test_expect_success 'push with matching heads and forced update' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo : &&
 	git commit --amend -massaged &&
 	git push testrepo +: &&
 	! check_push_result testrepo $the_commit heads/main &&
 	git reset --hard $the_commit
-
 '
 
 test_expect_success 'push with no ambiguity (1)' '
-
 	mk_test testrepo heads/main &&
 	git push testrepo main:main &&
 	check_push_result testrepo $the_commit heads/main
-
 '
 
 test_expect_success 'push with no ambiguity (2)' '
-
 	mk_test testrepo remotes/origin/main &&
 	git push testrepo main:origin/main &&
 	check_push_result testrepo $the_commit remotes/origin/main
-
 '
 
 test_expect_success 'push with colon-less refspec, no ambiguity' '
-
 	mk_test testrepo heads/main heads/t/main &&
 	git branch -f t/main main &&
 	git push testrepo main &&
 	check_push_result testrepo $the_commit heads/main &&
 	check_push_result testrepo $the_first_commit heads/t/main
-
 '
 
 test_expect_success 'push with weak ambiguity (1)' '
-
 	mk_test testrepo heads/main remotes/origin/main &&
 	git push testrepo main:main &&
 	check_push_result testrepo $the_commit heads/main &&
 	check_push_result testrepo $the_first_commit remotes/origin/main
-
 '
 
 test_expect_success 'push with weak ambiguity (2)' '
-
 	mk_test testrepo heads/main remotes/origin/main remotes/another/main &&
 	git push testrepo main:main &&
 	check_push_result testrepo $the_commit heads/main &&
 	check_push_result testrepo $the_first_commit remotes/origin/main remotes/another/main
-
 '
 
 test_expect_success 'push with ambiguity' '
-
 	mk_test testrepo heads/frotz tags/frotz &&
 	test_must_fail git push testrepo main:frotz &&
 	check_push_result testrepo $the_first_commit heads/frotz tags/frotz
-
 '
 
 test_expect_success 'push with onelevel ref' '
@@ -428,17 +404,14 @@ test_expect_success 'push with onelevel ref' '
 '
 
 test_expect_success 'push with colon-less refspec (1)' '
-
 	mk_test testrepo heads/frotz tags/frotz &&
 	git branch -f frotz main &&
 	git push testrepo frotz &&
 	check_push_result testrepo $the_commit heads/frotz &&
 	check_push_result testrepo $the_first_commit tags/frotz
-
 '
 
 test_expect_success 'push with colon-less refspec (2)' '
-
 	mk_test testrepo heads/frotz tags/frotz &&
 	if git show-ref --verify -q refs/heads/frotz
 	then
@@ -448,7 +421,6 @@ test_expect_success 'push with colon-less refspec (2)' '
 	git push -f testrepo frotz &&
 	check_push_result testrepo $the_commit tags/frotz &&
 	check_push_result testrepo $the_first_commit heads/frotz
-
 '
 
 test_expect_success 'push with colon-less refspec (3)' '
@@ -465,7 +437,6 @@ test_expect_success 'push with colon-less refspec (3)' '
 '
 
 test_expect_success 'push with colon-less refspec (4)' '
-
 	mk_test testrepo &&
 	if git show-ref --verify -q refs/heads/frotz
 	then
@@ -475,38 +446,29 @@ test_expect_success 'push with colon-less refspec (4)' '
 	git push testrepo frotz &&
 	check_push_result testrepo $the_commit tags/frotz &&
 	test 1 = $( cd testrepo && git show-ref | wc -l )
-
 '
 
 test_expect_success 'push head with non-existent, incomplete dest' '
-
 	mk_test testrepo &&
 	git push testrepo main:branch &&
 	check_push_result testrepo $the_commit heads/branch
-
 '
 
 test_expect_success 'push tag with non-existent, incomplete dest' '
-
 	mk_test testrepo &&
 	git tag -f v1.0 &&
 	git push testrepo v1.0:tag &&
 	check_push_result testrepo $the_commit tags/tag
-
 '
 
 test_expect_success 'push oid with non-existent, incomplete dest' '
-
 	mk_test testrepo &&
 	test_must_fail git push testrepo $(git rev-parse main):foo
-
 '
 
 test_expect_success 'push ref expression with non-existent, incomplete dest' '
-
 	mk_test testrepo &&
 	test_must_fail git push testrepo main^:branch
-
 '
 
 for head in HEAD @
@@ -550,7 +512,6 @@ do
 		git checkout main &&
 		git push testrepo $head:branch &&
 		check_push_result testrepo $the_commit heads/branch
-
 	'
 
 	test_expect_success "push with config remote.*.push = $head" '
@@ -596,7 +557,6 @@ test_expect_success 'push with remote.pushdefault' '
 '
 
 test_expect_success 'push with config remote.*.pushurl' '
-
 	mk_test testrepo heads/main &&
 	git checkout main &&
 	test_config remote.there.url test2repo &&
@@ -655,7 +615,6 @@ test_expect_success 'push ignores "branch." config without subsection' '
 '
 
 test_expect_success 'push with dry-run' '
-
 	mk_test testrepo heads/main &&
 	old_commit=$(git -C testrepo show-ref -s --verify refs/heads/main) &&
 	git push --dry-run testrepo : &&
@@ -663,7 +622,6 @@ test_expect_success 'push with dry-run' '
 '
 
 test_expect_success 'push updates local refs' '
-
 	mk_test testrepo heads/main &&
 	mk_child testrepo child &&
 	(
@@ -673,11 +631,9 @@ test_expect_success 'push updates local refs' '
 		test $(git rev-parse main) = \
 			$(git rev-parse remotes/origin/main)
 	)
-
 '
 
 test_expect_success 'push updates up-to-date local refs' '
-
 	mk_test testrepo heads/main &&
 	mk_child testrepo child1 &&
 	mk_child testrepo child2 &&
@@ -689,11 +645,9 @@ test_expect_success 'push updates up-to-date local refs' '
 		test $(git rev-parse main) = \
 			$(git rev-parse remotes/origin/main)
 	)
-
 '
 
 test_expect_success 'push preserves up-to-date packed refs' '
-
 	mk_test testrepo heads/main &&
 	mk_child testrepo child &&
 	(
@@ -701,11 +655,9 @@ test_expect_success 'push preserves up-to-date packed refs' '
 		git push &&
 		! test -f .git/refs/remotes/origin/main
 	)
-
 '
 
 test_expect_success 'push does not update local refs on failure' '
-
 	mk_test testrepo heads/main &&
 	mk_child testrepo child &&
 	echo "#!/no/frobnication/today" >testrepo/.git/hooks/pre-receive &&
@@ -717,16 +669,13 @@ test_expect_success 'push does not update local refs on failure' '
 		test $(git rev-parse main) != \
 			$(git rev-parse remotes/origin/main)
 	)
-
 '
 
 test_expect_success 'allow deleting an invalid remote ref' '
-
 	mk_test testrepo heads/branch &&
 	rm -f testrepo/.git/objects/??/* &&
 	git push testrepo :refs/heads/branch &&
 	(cd testrepo && test_must_fail git rev-parse --verify refs/heads/branch)
-
 '
 
 test_expect_success 'pushing valid refs triggers post-receive and post-update hooks' '
-- 
2.50.1

[PATCH v5 2/3] remote.c: remove BUG in show_push_unqualified_ref_name_error()

[Denton Liu]

When "git push <remote> <src>:<dst>" does not spell out the
destination side of the ref fully, and when <src> is not given
as a reference but an object name, the code tries to give advice
messages based on the type of that object.

The type is determined by calling odb_read_object_info() and
signalled by its return value.  The code however reported a
programming error with BUG() when this function said that there
is no such object, which happens when the object name is given
as a full hexadecimal (if the object name is given as a partial
hexadecimal or an non-existing ref, the function would have died
without returning, so this BUG() wouldn't have triggered).  This
is wrong.  It is an ordinary end-user mistake to give an object
name that does not exist and treated as such.

An example of the error message produced is as follows:

	error: The destination you provided is not a full refname (i.e.,
	starting with "refs/"). We tried to guess what you meant by:

	- Looking for a ref that matches 'branch' on the remote side.
	- Checking if the <src> being pushed ('0000000000000000000000000000000000000001')
	  is a ref in "refs/{heads,tags}/". If so we add a corresponding
	  refs/{heads,tags}/ prefix on the remote side.

	Neither worked, so we gave up. You must fully qualify the ref.
	BUG: remote.c:1221: '0000000000000000000000000000000000000001' should be commit/tag/tree/blob, is '-1'
	fatal: the remote end hung up unexpectedly
	Aborted (core dumped)

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 remote.c              | 5 +++--
 t/t5516-fetch-push.sh | 5 +++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index 88f991795b..00761604a8 100644
--- a/remote.c
+++ b/remote.c
@@ -1218,8 +1218,9 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
 			 "'%s:refs/tags/%s'?"),
 		       matched_src_name, dst_value);
 	} else {
-		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
-		    matched_src_name, type);
+		advise(_("The <src> part of the refspec ('%s') "
+			 "is an object ID that doesn't exist.\n"),
+		       matched_src_name);
 	}
 }
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 8eddf3e40d..46926e7bbd 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -471,6 +471,11 @@ test_expect_success 'push ref expression with non-existent, incomplete dest' '
 	test_must_fail git push testrepo main^:branch
 '
 
+test_expect_success 'push ref expression with non-existent oid src' '
+	mk_test testrepo &&
+	test_must_fail git push testrepo $(test_oid 001):branch
+'
+
 for head in HEAD @
 do
 
-- 
2.50.1

[PATCH v5 3/3] remote.c: convert if-else ladder to switch

[Denton Liu]

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

Suggested-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 remote.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/remote.c b/remote.c
index 00761604a8..df88914716 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,31 +1195,37 @@ 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;
 	}
 }
 
-- 
2.50.1

Re: [PATCH v5 0/3] remote.c: remove erroneous BUG case

[Patrick Steinhardt]

On Fri, Aug 08, 2025 at 12:24:39AM -0700, Denton Liu wrote:
> In the case where one pushes a non-existent oid to an unqualified
> destination, we encounter the following BUG
> 
> 	error: The destination you provided is not a full refname (i.e.,
> 	starting with "refs/"). We tried to guess what you meant by:
> 
> 	- Looking for a ref that matches 'branch' on the remote side.
> 	- Checking if the <src> being pushed ('0000000000000000000000000000000000000001')
> 	  is a ref in "refs/{heads,tags}/". If so we add a corresponding
> 	  refs/{heads,tags}/ prefix on the remote side.
> 
> 	Neither worked, so we gave up. You must fully qualify the ref.
> 	BUG: remote.c:1221: '0000000000000000000000000000000000000001' should be commit/tag/tree/blob, is '-1'
> 	fatal: the remote end hung up unexpectedly
> 	Aborted (core dumped)
> 
> However, this isn't actually a bug so replace it with an advise()
> message.
> 
> Changes since v4:
> 
> * Put the switch statement refactoring patch last so that we don't get
>   compile errors from a missing variable

Thanks, this version looks good to me.

Patrick

Re: [PATCH v5 0/3] remote.c: remove erroneous BUG case

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Aug 08, 2025 at 12:24:39AM -0700, Denton Liu wrote:
>> In the case where one pushes a non-existent oid to an unqualified
>> destination, we encounter the following BUG
>> 
>> 	error: The destination you provided is not a full refname (i.e.,
>> 	starting with "refs/"). We tried to guess what you meant by:
>> 
>> 	- Looking for a ref that matches 'branch' on the remote side.
>> 	- Checking if the <src> being pushed ('0000000000000000000000000000000000000001')
>> 	  is a ref in "refs/{heads,tags}/". If so we add a corresponding
>> 	  refs/{heads,tags}/ prefix on the remote side.
>> 
>> 	Neither worked, so we gave up. You must fully qualify the ref.
>> 	BUG: remote.c:1221: '0000000000000000000000000000000000000001' should be commit/tag/tree/blob, is '-1'
>> 	fatal: the remote end hung up unexpectedly
>> 	Aborted (core dumped)
>> 
>> However, this isn't actually a bug so replace it with an advise()
>> message.
>> 
>> Changes since v4:
>> 
>> * Put the switch statement refactoring patch last so that we don't get
>>   compile errors from a missing variable
>
> Thanks, this version looks good to me.

Yeah, this looks good.  Let's mark it for 'next'.

Thanks, both.