[PATCH] git-clone: fetch possibly detached HEAD over dumb http

[PATCH] git-clone: fetch possibly detached HEAD over dumb http

[Sven Verdoolaege]

git-clone supports cloning from a repo with detached HEAD,
but if this HEAD is not behind any branch tip then it
would not have been fetched over dumb http, resulting in a

	fatal: Not a valid object name HEAD

Since 928c210a, this would also happen on a http repo
with a HEAD that is a symbolic link where someone has
forgotton to run update-server-info.

Signed-off-by: Sven Verdoolaege <skimo@liacs.nl>
---
 git-clone.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/git-clone.sh b/git-clone.sh
index bd44ce1..cdbbc20 100755
--- a/git-clone.sh
+++ b/git-clone.sh
@@ -70,7 +70,8 @@ Perhaps git-update-server-info needs to be run there?"
 		git-http-fetch $v -a -w "$tname" "$sha1" "$1" || exit 1
 	done <"$clone_tmp/refs"
 	rm -fr "$clone_tmp"
-	http_fetch "$1/HEAD" "$GIT_DIR/REMOTE_HEAD" ||
+	http_fetch "$1/HEAD" "$GIT_DIR/REMOTE_HEAD" &&
+	git-http-fetch $v -a $(cat "$GIT_DIR/REMOTE_HEAD") "$1" ||
 	rm -f "$GIT_DIR/REMOTE_HEAD"
 }
 
-- 
1.5.2.2.585.g9cc0-dirty

Re: [PATCH] git-clone: fetch possibly detached HEAD over dumb http

[Junio C Hamano]

Sven Verdoolaege <skimo@liacs.nl> writes:

> git-clone supports cloning from a repo with detached HEAD,
> but if this HEAD is not behind any branch tip then it
> would not have been fetched over dumb http, resulting in a
>
> 	fatal: Not a valid object name HEAD
>
> Since 928c210a, this would also happen on a http repo
> with a HEAD that is a symbolic link where someone has
> forgotton to run update-server-info.
>
> Signed-off-by: Sven Verdoolaege <skimo@liacs.nl>

Ok.  But I think the change regresses when the remote side is
actually on a particular branch, and is using symref to
represent $GIT_DIR/HEAD.

>  git-clone.sh |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/git-clone.sh b/git-clone.sh
> index bd44ce1..cdbbc20 100755
> --- a/git-clone.sh
> +++ b/git-clone.sh
> @@ -70,7 +70,8 @@ Perhaps git-update-server-info needs to be run there?"
>  		git-http-fetch $v -a -w "$tname" "$sha1" "$1" || exit 1
>  	done <"$clone_tmp/refs"
>  	rm -fr "$clone_tmp"
> -	http_fetch "$1/HEAD" "$GIT_DIR/REMOTE_HEAD" ||
> +	http_fetch "$1/HEAD" "$GIT_DIR/REMOTE_HEAD" &&
> +	git-http-fetch $v -a $(cat "$GIT_DIR/REMOTE_HEAD") "$1" ||
>  	rm -f "$GIT_DIR/REMOTE_HEAD"
>  }

At this point, "$GIT_DIR/REMOTE_HEAD" is a copy of HEAD obtained
from the remote site via curl.  It can contain:

 (1) raw SHA-1 of the tip commit, if the HEAD is detached, or
     the repository uses a symlink to represent HEAD, or

 (2) "ref: refs/heads/$currentbranch".

You would want to do this extra fetch only in case (1).
I think the additional fetch would fail in case (2), and result
in removal of $GIT_DIR/REMOTE_HEAD.

Hmm?

Re: [PATCH] git-clone: fetch possibly detached HEAD over dumb http

[Sven Verdoolaege]

On Thu, Jun 28, 2007 at 05:02:18PM -0700, Junio C Hamano wrote:
> You would want to do this extra fetch only in case (1).
> I think the additional fetch would fail in case (2), and result
> in removal of $GIT_DIR/REMOTE_HEAD.

You're right.  It looks like I only tested it on symbolic link HEADs.
Sorry about that.  Will send a corrected patch later.

skimo

[PATCH] git-clone: fetch possibly detached HEAD over dumb http

[Sven Verdoolaege]

git-clone supports cloning from a repo with detached HEAD,
but if this HEAD is not behind any branch tip then it
would not have been fetched over dumb http, resulting in a

	fatal: Not a valid object name HEAD

Since 928c210a, this would also happen on a http repo
with a HEAD that is a symbolic link where someone has
forgotton to run update-server-info.

Signed-off-by: Sven Verdoolaege <skimo@liacs.nl>
---
On Thu, Jun 28, 2007 at 05:02:18PM -0700, Junio C Hamano wrote:
> Ok.  But I think the change regresses when the remote side is
> actually on a particular branch, and is using symref to
> represent $GIT_DIR/HEAD.

Updated patch tested on both symbolic links and symrefs.

skimo

 git-clone.sh |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/git-clone.sh b/git-clone.sh
index bd44ce1..4cbf60f 100755
--- a/git-clone.sh
+++ b/git-clone.sh
@@ -72,6 +72,17 @@ Perhaps git-update-server-info needs to be run there?"
 	rm -fr "$clone_tmp"
 	http_fetch "$1/HEAD" "$GIT_DIR/REMOTE_HEAD" ||
 	rm -f "$GIT_DIR/REMOTE_HEAD"
+	if test -f "$GIT_DIR/REMOTE_HEAD"; then
+		head_sha1=`cat "$GIT_DIR/REMOTE_HEAD"`
+		case "$head_sha1" in
+		'ref: refs/'*)
+			;;
+		*)
+			git-http-fetch $v -a "$head_sha1" "$1" ||
+			rm -f "$GIT_DIR/REMOTE_HEAD"
+			;;
+		esac
+	fi
 }
 
 quiet=
-- 
1.5.2.2.585.g9cc0-dirty

Re: [PATCH] git-clone: fetch possibly detached HEAD over dumb http

[Alex Riesen]

Sven Verdoolaege, Fri, Jun 29, 2007 10:31:08 +0200:
> +		head_sha1=`cat "$GIT_DIR/REMOTE_HEAD"`
> +		case "$head_sha1" in
> +		'ref: refs/'*)
> +			;;

And what do you do if the HEAD is a reflink on something not in refs/?
Like "ref: tmp"? Yes, it is unlikely, but is not forbidden.

How about "[0-9a-f]*)" instead:

               case "$head_sha1" in
               [0-9a-f]*)
                       git-http-fetch $v -a "$head_sha1" "$1" ||
                       rm -f "$GIT_DIR/REMOTE_HEAD"
                       ;;
               esac

Re: [PATCH] git-clone: fetch possibly detached HEAD over dumb http

[Sven Verdoolaege]

On Sat, Jun 30, 2007 at 03:33:10PM +0200, Alex Riesen wrote:
> Sven Verdoolaege, Fri, Jun 29, 2007 10:31:08 +0200:
> > +		head_sha1=`cat "$GIT_DIR/REMOTE_HEAD"`
> > +		case "$head_sha1" in
> > +		'ref: refs/'*)
> > +			;;
> 
> And what do you do if the HEAD is a reflink on something not in refs/?
> Like "ref: tmp"? Yes, it is unlikely, but is not forbidden.

It may not be forbidden, but I don't think it would
work with current git-clone either.

skimo

Re: [PATCH] git-clone: fetch possibly detached HEAD over dumb http

[Alex Riesen]

Sven Verdoolaege, Sat, Jun 30, 2007 15:45:44 +0200:
> On Sat, Jun 30, 2007 at 03:33:10PM +0200, Alex Riesen wrote:
> > Sven Verdoolaege, Fri, Jun 29, 2007 10:31:08 +0200:
> > > +		head_sha1=`cat "$GIT_DIR/REMOTE_HEAD"`
> > > +		case "$head_sha1" in
> > > +		'ref: refs/'*)
> > > +			;;
> > 
> > And what do you do if the HEAD is a reflink on something not in refs/?
> > Like "ref: tmp"? Yes, it is unlikely, but is not forbidden.
> 
> It may not be forbidden, but I don't think it would
> work with current git-clone either.
> 

Every command which needs a proper .git will not work, so I take this
back completely.

The check for .git validity includes checking if HEAD contains
something sane, and this check is very simple: the HEAD is read
(readlink(2) or plain read(2)) and tested if it contains a
reference starting with "refs/", which maybe inconsistent with
resolve_gitlink_ref, but probably ok.

Re: [PATCH] git-clone: fetch possibly detached HEAD over dumb http

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> The check for .git validity includes checking if HEAD contains
> something sane, and this check is very simple: the HEAD is read
> (readlink(2) or plain read(2)) and tested if it contains a
> reference starting with "refs/", which maybe inconsistent with
> resolve_gitlink_ref, but probably ok.

Ah, I was not paying close attention to resolve_gitlink_ref();
if it does not require HEAD to point at refs/ I would say it is
a bug.

Come to think of it, I would further say that we probably should
tighten it up a bit: HEAD must be either a valid commit object
name (i.e. detached) or a ref that point at somewhere under
refs/heads hierarchy, not just anywhere in refs/.

Re: [PATCH] git-clone: fetch possibly detached HEAD over dumb http

[Alex Riesen]

Junio C Hamano, Sun, Jul 01, 2007 04:22:04 +0200:
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
> > The check for .git validity includes checking if HEAD contains
> > something sane, and this check is very simple: the HEAD is read
> > (readlink(2) or plain read(2)) and tested if it contains a
> > reference starting with "refs/", which maybe inconsistent with
> > resolve_gitlink_ref, but probably ok.
> 
> Ah, I was not paying close attention to resolve_gitlink_ref();
> if it does not require HEAD to point at refs/ I would say it is
> a bug.

yes, thats why I think its ok.

> Come to think of it, I would further say that we probably should
> tighten it up a bit: HEAD must be either a valid commit object
> name (i.e. detached)

That (HEAD must point to a _valid_ commit) will make accidentally
corrupted repositories harder to fix. The tool which require a valid
repository (cat-file, update-ref, read-tree) are the same tools which
you need to fix small problems which can happen, like the commit
pointed by HEAD is accidentally pruned from parent repo.