[PATCH] builtin/fetch-pack: indicate when we have an exact oid

[PATCH] builtin/fetch-pack: indicate when we have an exact oid

[Nathan Froyd via GitGitGadget]

From: Nathan Froyd <froydnj@gmail.com>

The `git fetch` path, when parsing OIDs, properly sets `exact_oid` on
the relevant refs; the equivalent path for `git fetch-pack` does not.
This oversight results in an invocation of `git fetch-pack $OID`
sending `want-ref $OID`, which results in errors like:

  fatal: unknown ref $OID
  fatal: remote error: unknown ref $OID

Make the two paths equivalent by setting `exact_oid` properly.

Signed-off-by: Nathan Froyd <froydnj@gmail.com>
---
    builtin/fetch-pack: indicate when we have an exact oid

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2420%2Ffroydnj%2Ffroydnj-fetch-pack-exact-oid-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2420/froydnj/froydnj-fetch-pack-exact-oid-v1
Pull-Request: https://github.com/git/git/pull/2420

 builtin/fetch-pack.c               |  4 +-
 t/t5703-upload-pack-ref-in-want.sh | 68 ++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 86754296fa..bef8a3dfc5 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -23,13 +23,14 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,
 	struct ref *ref;
 	struct object_id oid;
 	const char *p;
+	int exact_oid = 0;
 
 	if (!parse_oid_hex(name, &oid, &p)) {
 		if (*p == ' ') {
 			/* <oid> <ref>, find refname */
 			name = p + 1;
 		} else if (*p == '\0') {
-			; /* <oid>, leave oid as name */
+			exact_oid = 1; /* <oid>, leave oid as name */
 		} else {
 			/* <ref>, clear cruft from oid */
 			oidclr(&oid, the_repository->hash_algo);
@@ -41,6 +42,7 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,
 
 	ref = alloc_ref(name);
 	oidcpy(&ref->old_oid, &oid);
+	ref->exact_oid = exact_oid;
 	(*nr)++;
 	ALLOC_GROW(*sought, *nr, *alloc);
 	(*sought)[*nr - 1] = ref;
diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index 330d049b2c..c02e1fc79c 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -242,6 +242,74 @@ test_expect_success 'fetching with exact OID' '
 	test_grep "want $oid" log
 '
 
+test_expect_success 'fetch-pack with ref and exact OID from arguments' '
+	test_when_finished "rm -f log" &&
+
+	rm -rf local &&
+	cp -r "$LOCAL_PRISTINE" local &&
+	oid=$(git -C "$REPO" rev-parse d) &&
+	main_oid=$(git -C "$REPO" rev-parse main) &&
+	GIT_TRACE_PACKET="$(pwd)/log" git -C local fetch-pack \
+		"$REPO" refs/heads/main "$oid" >actual &&
+	cat >expected <<-EOF &&
+	$oid $oid
+	$main_oid refs/heads/main
+	EOF
+	sort expected >expected.sorted &&
+	sort actual >actual.sorted &&
+	test_cmp expected.sorted actual.sorted &&
+	git -C local cat-file -e "$oid" &&
+	git -C local cat-file -e "$main_oid" &&
+	test_grep "want $oid" log &&
+	test_grep "want-ref refs/heads/main" log
+'
+
+test_expect_success 'fetch-pack with ref and exact OID from stdin' '
+	test_when_finished "rm -f log" &&
+
+	rm -rf local &&
+	cp -r "$LOCAL_PRISTINE" local &&
+	oid=$(git -C "$REPO" rev-parse d) &&
+	main_oid=$(git -C "$REPO" rev-parse main) &&
+	cat >input <<-EOF &&
+	refs/heads/main
+	$oid
+	EOF
+	GIT_TRACE_PACKET="$(pwd)/log" git -C local fetch-pack \
+		--stdin "$REPO" <input >actual &&
+	cat >expected <<-EOF &&
+	$oid $oid
+	$main_oid refs/heads/main
+	EOF
+	sort expected >expected.sorted &&
+	sort actual >actual.sorted &&
+	test_cmp expected.sorted actual.sorted &&
+	git -C local cat-file -e "$oid" &&
+	git -C local cat-file -e "$main_oid" &&
+	test_grep "want $oid" log &&
+	test_grep "want-ref refs/heads/main" log
+'
+
+test_expect_success 'fetch-pack with OID and ref name resolves the ref' '
+	test_when_finished "rm -f log" &&
+
+	rm -rf local &&
+	cp -r "$LOCAL_PRISTINE" local &&
+	oid=$(git -C "$REPO" rev-parse d) &&
+	main_oid=$(git -C "$REPO" rev-parse main) &&
+	cat >input <<-EOF &&
+	$oid refs/heads/main
+	EOF
+	GIT_TRACE_PACKET="$(pwd)/log" git -C local fetch-pack \
+		--stdin "$REPO" <input >actual &&
+	cat >expected <<-EOF &&
+	$main_oid refs/heads/main
+	EOF
+	test_cmp expected actual &&
+	git -C local cat-file -e "$main_oid" &&
+	test_grep "want-ref refs/heads/main" log
+'
+
 test_expect_success 'fetching multiple refs' '
 	test_when_finished "rm -f log" &&
 

base-commit: 3bc0341126508f78f5869cbfc0005e987efdf0c7
-- 
gitgitgadget

Re: [PATCH] builtin/fetch-pack: indicate when we have an exact oid

[Junio C Hamano]

"Nathan Froyd via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Nathan Froyd <froydnj@gmail.com>
>
> The `git fetch` path, when parsing OIDs, properly sets `exact_oid` on
> the relevant refs; the equivalent path for `git fetch-pack` does not.
> This oversight results in an invocation of `git fetch-pack $OID`
> sending `want-ref $OID`, which results in errors like:
>
>   fatal: unknown ref $OID
>   fatal: remote error: unknown ref $OID
>
> Make the two paths equivalent by setting `exact_oid` properly.
>
> Signed-off-by: Nathan Froyd <froydnj@gmail.com>
> ---
>     builtin/fetch-pack: indicate when we have an exact oid
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2420%2Ffroydnj%2Ffroydnj-fetch-pack-exact-oid-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2420/froydnj/froydnj-fetch-pack-exact-oid-v1
> Pull-Request: https://github.com/git/git/pull/2420
>
>  builtin/fetch-pack.c               |  4 +-
>  t/t5703-upload-pack-ref-in-want.sh | 68 ++++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 86754296fa..bef8a3dfc5 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -23,13 +23,14 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,
>  	struct ref *ref;
>  	struct object_id oid;
>  	const char *p;
> +	int exact_oid = 0;
>  
>  	if (!parse_oid_hex(name, &oid, &p)) {
>  		if (*p == ' ') {
>  			/* <oid> <ref>, find refname */
>  			name = p + 1;
>  		} else if (*p == '\0') {
> -			; /* <oid>, leave oid as name */
> +			exact_oid = 1; /* <oid>, leave oid as name */
>  		} else {
>  			/* <ref>, clear cruft from oid */
>  			oidclr(&oid, the_repository->hash_algo);

Unlike "git fetch" that is a higher level wrapper, in "git fetch-pack",
a heuristic dwim like this is unwelcome.  In 

    $ git fetch-pack <repository> <ref>...

these <ref> arguments are meant to be passed exactly as given.

I am not sure if there days there still is a reason to run
"fetch-pack" directly instead of "git fetch", as the manual of "git
fetch-pack" itself suggets.  But if there is, then shouldn't we give
it an explicit way to say "this is not a ref but is an object name"
in a more unambiguous way.  Otherwise, we lose the ability to send
"want-ref 3ce1e94f7d5607971b5de6762bbdb57e86fc5707", even if we
wanted to, which is something "git fetch" does not let you do, and
in turn may be a valid reason why somebody want to use "fetch-pack"
over "fetch" in the first place.

Re: [PATCH] builtin/fetch-pack: indicate when we have an exact oid

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Unlike "git fetch" that is a higher level wrapper, in "git fetch-pack",
> a heuristic dwim like this is unwelcome.  In 
>
>     $ git fetch-pack <repository> <ref>...
>
> these <ref> arguments are meant to be passed exactly as given.

Need a bit of clarification.  I should have said

    ... passed to the other side on "want-ref" packet as given on
    the command line.