[PATCH] clear error message for clone a gitweb URL

[PATCH] clear error message for clone a gitweb URL

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

diff --git a/fetch.c b/fetch.c
index c426c04..40c5183 100644
--- a/fetch.c
+++ b/fetch.c
@@ -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]);
diff --git a/git-clone.sh b/git-clone.sh
index 3f006d1..c8274e0 100755
--- a/git-clone.sh
+++ b/git-clone.sh
@@ -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"

Re: [PATCH] clear error message for clone a gitweb URL

[Junio C Hamano]

Liu Yubao <yubao.liu@gmail.com> writes:

> 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.
>
> diff --git a/fetch.c b/fetch.c
> index c426c04..40c5183 100644
> --- a/fetch.c
> +++ b/fetch.c
> @@ -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...

diff --git a/git-clone.sh b/git-clone.sh
index 3f006d1..7ae69d9 100755
--- a/git-clone.sh
+++ b/git-clone.sh
@@ -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"

Re: [PATCH] clear error message for clone a gitweb URL

[Liu Yubao]

Junio C Hamano wrote:
> Liu Yubao <yubao.liu@gmail.com> writes:
> 
>> 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.
>>
>> diff --git a/fetch.c b/fetch.c
>> index c426c04..40c5183 100644
>> --- a/fetch.c
>> +++ b/fetch.c
>> @@ -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 agree with you, it's not a fault of fetch.c.

> 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...
> 
> diff --git a/git-clone.sh b/git-clone.sh
> index 3f006d1..7ae69d9 100755
> --- a/git-clone.sh
> +++ b/git-clone.sh
> @@ -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"
> 
It works well. Maybe it's better to say "info/refs has nonsense sha1($sha1)