threads / patch / 8757

patchgit-clone: fetch possibly detached HEAD over dumb http

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

## tl;dr

9 messages between Jun 28, 2007 and Jul 1, 2007. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Sven Verdoolaege· Jun 28, 2007, 10:52 UTC · lore

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(-)

Show changes to git-clone.sh +2 −1

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

Junio C Hamano· Jun 29, 2007, 00:02 UTC · re: Sven Verdoolaege · lore

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

Sven Verdoolaege <skimo@liacs.nl> writes:

Show 11 quoted lines

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

Show 16 quoted lines

>  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?

Sven Verdoolaege· Jun 29, 2007, 08:11 UTC · re: Junio C Hamano · lore

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

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

Sven Verdoolaege· Jun 29, 2007, 08:31 UTC · re: Junio C Hamano · lore

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(-)

Show changes to git-clone.sh +11 −0

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

Alex Riesen· Jun 30, 2007, 13:33 UTC · re: Sven Verdoolaege · lore

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

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

Sven Verdoolaege· Jun 30, 2007, 13:45 UTC · re: Alex Riesen · lore

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

On Sat, Jun 30, 2007 at 03:33:10PM +0200, Alex Riesen wrote:

Show 8 quoted lines

> 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

Alex Riesen· Jun 30, 2007, 22:23 UTC · re: Sven Verdoolaege · lore

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

Sven Verdoolaege, Sat, Jun 30, 2007 15:45:44 +0200:

Show 13 quoted lines

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

Junio C Hamano· Jul 1, 2007, 02:22 UTC · re: Alex Riesen · lore

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

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

Show 5 quoted lines

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

Alex Riesen· Jul 1, 2007, 16:40 UTC · re: Junio C Hamano · lore

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

Junio C Hamano, Sun, Jul 01, 2007 04:22:04 +0200:

Show 11 quoted lines

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