clear error message for clone a gitweb URL

PATCH In review Liu Yubao 2 participants 3 messages Aug 11, 2016

Share & tools

Liu Yubao

author

2016-08-11 20:43 UTC #0 lore raw

PATCH 1/1 clear error message for clone a gitweb URL

When clone a gitweb URL, git reports "Can't lock ref", it's not clear for users, this patch adds clear error message for this case.

fetch.c +8 -0

index c426c04..40c5183 100644
@@ -266,6 +266,14 @@ int pull(int targets, char **target, con
 		if (!write_ref || !write_ref[i])
 			continue;
 
+		if (*write_ref[i] == '\0') {
+			if (strncmp(write_ref_log_details, "http", 4) == 0)
+				error("Can't feed empty ref, seems you are fetching from a gitweb URL, "
+				      "check it in web browser for git URL.");
+			else
+				error("Can't feed empty ref");
+			goto unlock_and_fail;
+		}
 		lock[i] = lock_ref_sha1(write_ref[i], NULL);
 		if (!lock[i]) {
 			error("Can't lock ref %s", write_ref[i]);

git-clone.sh +4 -0

index 3f006d1..c8274e0 100755
@@ -55,6 +55,10 @@ Perhaps git-update-server-info needs to
 		else
 			tname=$name
 		fi
+		if [ -z "$tname" -o -z "$name" ]; then
+			die "Cannot feed empty ref or commit-id, seems you are fetching
+from a gitweb URL, check it in web browser for git URL."
+		fi
 		git-http-fetch -v -a -w "$tname" "$name" "$1/" || exit 1
 	done <"$clone_tmp/refs"

Junio C Hamano

2016-08-11 19:27 UTC #1 lore raw

Quoted 8 lines · Liu Yubao

When clone a gitweb URL, git reports "Can't lock ref", it's not clear for users, this patch adds clear error message for this case.
fetch.c +0 -0
index c426c04..40c5183 100644

fetch.c int pull(int targets, char **target, con · line 275

Quoted 10 lines

fetch.c +7 -0

@@ -266,6 +266,14 @@ int pull(int targets, char **target, con
 		if (!write_ref || !write_ref[i])
 			continue;
 
+		if (*write_ref[i] == '\0') {
+			if (strncmp(write_ref_log_details, "http", 4) == 0)
+				error("Can't feed empty ref, seems you are fetching from a gitweb URL, "
+				      "check it in web browser for git URL.");
+			else
+				error("Can't feed empty ref");
+			goto unlock_and_fail;

You might have got that error by feeding an URL for gitweb, but I do not think the code, even with your additions, knows enough to tell that the user's mistake isn't other kinds of errors.

I am afraid that it would cause the user to waste time going wild goose chase if you say "seems you are...". The phrasing makes it sound as if the tool _knows_ with some certainty that it is more plausible cause of the error than other kinds, while it certainly doesn't.

I think the reason it does not notice the breakage much earlier is that git-clone does not notice that gitweb URL gives nonsense to requests to "http://host/gitweb.cgi/$project/info/refs", so your patch to git-clone.sh is probably touching the right place, but I still feel the wording is a bit too strong and definitive than it should be.

Perhaps...

git-clone.sh +6 -3

index 3f006d1..7ae69d9 100755
@@ -46,15 +46,18 @@ Perhaps git-update-server-info needs to
 	do
 		name=`expr "z$refname" : 'zrefs/\(.*\)'` &&
 		case "$name" in
-		*^*)	continue;;
-		esac
+		*^*)	continue ;;
+		'')	false ;;
+		esac &&
 		if test -n "$use_separate_remote" &&
 		   branch_name=`expr "z$name" : 'zheads/\(.*\)'`
 		then
 			tname="remotes/$origin/$branch_name"
 		else
 			tname=$name
-		fi
+		fi || {
+			die "info/refs has nonsense $sha1 $refname, are you pulling from the right repository URL?"
+		}
 		git-http-fetch -v -a -w "$tname" "$name" "$1/" || exit 1
 	done <"$clone_tmp/refs"
 	rm -fr "$clone_tmp"

Liu Yubao

author

2016-08-11 20:22 UTC #2 lore raw

Quoted 25 lines · Junio C Hamano

Quoted 8 lines · Liu Yubao
When clone a gitweb URL, git reports "Can't lock ref", it's not clear for users, this patch adds clear error message for this case.
fetch.c +0 -0
index c426c04..40c5183 100644
fetch.c int pull(int targets, char **target, con · line 275
Quoted 10 lines
fetch.c +7 -0
@@ -266,6 +266,14 @@ int pull(int targets, char **target, con
 		if (!write_ref || !write_ref[i])
 			continue;
 
+		if (*write_ref[i] == '\0') {
+			if (strncmp(write_ref_log_details, "http", 4) == 0)
+				error("Can't feed empty ref, seems you are fetching from a gitweb URL, "
+				      "check it in web browser for git URL.");
+			else
+				error("Can't feed empty ref");
+			goto unlock_and_fail;
You might have got that error by feeding an URL for gitweb, but I do not think the code, even with your additions, knows enough to tell that the user's mistake isn't other kinds of errors.

Quoted 6 lines

I am afraid that it would cause the user to waste time going wild goose chase if you say "seems you are...". The phrasing makes it sound as if the tool _knows_ with some certainty that it is more plausible cause of the error than other kinds, while it certainly doesn't.

I agree with you, it's not a fault of fetch.c.

Quoted 26 lines

I think the reason it does not notice the breakage much earlier is that git-clone does not notice that gitweb URL gives nonsense to requests to "http://host/gitweb.cgi/$project/info/refs", so your patch to git-clone.sh is probably touching the right place, but I still feel the wording is a bit too strong and definitive than it should be.
Perhaps...
git-clone.sh +3 -2
index 3f006d1..7ae69d9 100755
@@ -46,15 +46,18 @@ Perhaps git-update-server-info needs to
 	do
 		name=`expr "z$refname" : 'zrefs/$.*$'` &&
 		case "$name" in
-		*^*)	continue;;
-		esac
+		*^*)	continue ;;
+		'')	false ;;
+		esac &&
 		if test -n "$use_separate_remote" &&
 		   branch_name=`expr "z$name" : 'zheads/$.*$'`
 		then

git-clone.sh Perhaps git-update-server-info needs to · line 63

Quoted 10 lines

git-clone.sh +3 -1

 			tname="remotes/$origin/$branch_name"
 		else
 			tname=$name
-		fi
+		fi || {
+			die "info/refs has nonsense $sha1 $refname, are you pulling from the right repository URL?"
+		}
 		git-http-fetch -v -a -w "$tname" "$name" "$1/" || exit 1
 	done <"$clone_tmp/refs"
 	rm -fr "$clone_tmp"

It works well. Maybe it's better to say "info/refs has nonsense sha1($sha1)