[1/2] upload-pack: fix ambiguous error message

Commit Message

Elijah Newren Nov. 4, 2024, 7:02 p.m. UTC

From: Elijah Newren <[e-mail]>

upload-pack.c takes any --shallow-exclude argument(s) from
clone/fetch/etc. and passes them through expand_ref().  If it does not
get back exactly one ref from the call to expand_ref(), it will die with
the following error:

    fatal: git upload-pack: ambiguous deepen-not: %s

Given that the documentation suggests to users that --shallow-exclude
accepts a revision rather than a ref (which will be corrected in a
subsequent commit), users may try to pass a revision.  In such a case,
expand_ref() will return 0 matches, but the error message we print will
be misleading since "ambiguous" suggests there are multiple matches.
Provide a clearer error message for such a case.

Signed-off-by: Elijah Newren <[e-mail]>
---
 t/t5500-fetch-pack.sh | 7 +++++++
 upload-pack.c         | 6 +++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

Comments

Patrick Steinhardt Nov. 5, 2024, 6:27 a.m. UTC | #1

On Mon, Nov 04, 2024 at 07:02:43PM +0000, Elijah Newren via GitGitGadget wrote:
> diff --git a/upload-pack.c b/upload-pack.c
> index 6d6e0f9f980..640d45295e1 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1025,10 +1025,14 @@ static int process_deepen_not(const char *line, struct oidset *deepen_not, int *
>  {
>  	const char *arg;
>  	if (skip_prefix(line, "deepen-not ", &arg)) {
> +		int cnt;
>  		char *ref = NULL;
>  		struct object_id oid;
> -		if (expand_ref(the_repository, arg, strlen(arg), &oid, &ref) != 1)
> +		cnt = expand_ref(the_repository, arg, strlen(arg), &oid, &ref);
> +		if (cnt > 1)
>  			die("git upload-pack: ambiguous deepen-not: %s", line);
> +		if (cnt < 1)
> +			die("git upload-pack: deepen-not is not a ref: %s", line);

I was wondering whether `expand_ref()` could ever return negative, but
after reading through its implementation that doesn't seem to be the
case. It's somewhat misleading that it returns `int`, as `size_t` would
convey the return value in a better spirit.

Anyway, that is not an issue of this patch series, and the change looks
good.

Patrick

Patch

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 8da8e7fe423..6552da78d19 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -926,6 +926,13 @@  test_expect_success 'fetch exclude tag one' '
 	test_cmp expected actual
 '
 
+test_expect_success 'fetch exclude tag one as revision' '
+	test_when_finished rm -f rev err &&
+	git -C shallow-exclude rev-parse one >rev &&
+	test_must_fail git -C shallow12 fetch --shallow-exclude $(cat rev) origin 2>err &&
+	grep "deepen-not is not a ref:" err
+'
+
 test_expect_success 'fetching deepen' '
 	test_create_repo shallow-deepen &&
 	(
diff --git a/upload-pack.c b/upload-pack.c
index 6d6e0f9f980..640d45295e1 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1025,10 +1025,14 @@  static int process_deepen_not(const char *line, struct oidset *deepen_not, int *
 {
 	const char *arg;
 	if (skip_prefix(line, "deepen-not ", &arg)) {
+		int cnt;
 		char *ref = NULL;
 		struct object_id oid;
-		if (expand_ref(the_repository, arg, strlen(arg), &oid, &ref) != 1)
+		cnt = expand_ref(the_repository, arg, strlen(arg), &oid, &ref);
+		if (cnt > 1)
 			die("git upload-pack: ambiguous deepen-not: %s", line);
+		if (cnt < 1)
+			die("git upload-pack: deepen-not is not a ref: %s", line);
 		oidset_insert(deepen_not, &oid);
 		free(ref);
 		*deepen_rev_list = 1;