remote.c: remove erroneous BUG case

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

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.

---
 t/t5516-fetch-push.sh | 7 +++++++
 1 file changed, 7 insertions(+)

index 4e9c27b0f2..c2fcfeca92 100755
@@ -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

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.

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

index e965f022f1..9fb76049d2 100644
@@ -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);
 	}
 }
 

index c2fcfeca92..e064ea7433 100755
@@ -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

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

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?

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.

*** 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 <[email protected]>

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

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.

---
 t/t5516-fetch-push.sh | 7 +++++++
 1 file changed, 7 insertions(+)

index 4e9c27b0f2..c2fcfeca92 100755
@@ -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

A very good suggestion.

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

Thanks.

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.

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

index e965f022f1..4ad20110e9 100644
@@ -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"));
 	}
 }
 

index c2fcfeca92..e064ea7433 100755
@@ -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

Yup, makes sense.

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 <[email protected]>

## 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)
+

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

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 | 51 -------------------------------------------
 1 file changed, 51 deletions(-)

index 4e9c27b0f2..8eddf3e40d 100755
@@ -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

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

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)

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

index e965f022f1..465e0ea0eb 100644
@@ -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);
 	}
 }
 

index 8eddf3e40d..46926e7bbd 100755
@@ -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

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.

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

Let's merge the topic to 'next'.

Thanks.

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

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

index 465e0ea0eb..c7ae18fcfa 100644
@@ -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

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

---
 remote.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

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

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

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

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

2.50.1

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

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

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.

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

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

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 | 51 -------------------------------------------
 1 file changed, 51 deletions(-)

index 4e9c27b0f2..8eddf3e40d 100755
@@ -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

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

---
 remote.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

index 88f991795b..61e2c9951a 100644
@@ -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

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

Thanks for catching that. Another patchset incoming

-Denton

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)

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

index 61e2c9951a..df88914716 100644
@@ -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;
 	}
 }
 

index 8eddf3e40d..46926e7bbd 100755
@@ -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

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

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 | 51 -------------------------------------------
 1 file changed, 51 deletions(-)

index 4e9c27b0f2..8eddf3e40d 100755
@@ -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

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)

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

index 88f991795b..00761604a8 100644
@@ -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);
 	}
 }
 

index 8eddf3e40d..46926e7bbd 100755
@@ -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

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

---
 remote.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

index 00761604a8..df88914716 100644
@@ -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

Thanks, this version looks good to me.

Patrick

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

Thanks, both.