[PATCH 4/5] allow deepening of a shallow repository

[PATCH 4/5] allow deepening of a shallow repository

[Johannes Schindelin]

Now, by saying "git fetch -depth <n> <repo>" you can deepen
a shallow repository.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 commit.c      |   13 +++++++++++++
 commit.h      |    3 ++-
 fetch-pack.c  |   22 ++++++++++++++++------
 git-fetch.sh  |   12 +++++++++++-
 shallow.c     |    8 ++++++--
 upload-pack.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++-----------
 6 files changed, 94 insertions(+), 21 deletions(-)

diff --git a/commit.c b/commit.c
index bffa278..d5103cd 100644
--- a/commit.c
+++ b/commit.c
@@ -255,6 +255,19 @@ int write_shallow_commits(int fd, int us
 	return count;
 }
 
+int unregister_shallow(const unsigned char *sha1)
+{
+	int pos = commit_graft_pos(sha1);
+	if (pos < 0)
+		return -1;
+	if (pos + 1 < commit_graft_nr)
+		memcpy(commit_graft + pos, commit_graft + pos + 1,
+				sizeof(struct commit_graft *)
+				* (commit_graft_nr - pos - 1));
+	commit_graft_nr--;
+	return 0;
+}
+
 int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)
 {
 	char *tail = buffer;
diff --git a/commit.h b/commit.h
index c559510..e9e158f 100644
--- a/commit.h
+++ b/commit.h
@@ -108,9 +108,10 @@ int read_graft_file(const char *graft_fi
 extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2, int cleanup);
 
 extern int register_shallow(const unsigned char *sha1);
+extern int unregister_shallow(const unsigned char *sha1);
 extern int write_shallow_commits(int fd, int use_pack_protocol);
 extern int is_repository_shallow();
 extern struct commit_list *get_shallow_commits(struct object_array *heads,
-		int depth);
+		int depth, int shallow_flag, int not_shallow_flag);
 
 #endif /* COMMIT_H */
diff --git a/fetch-pack.c b/fetch-pack.c
index 9619d6e..a82a5ba 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -199,7 +199,17 @@ static int find_common(int fd[2], unsign
 				if (lookup_object(sha1))
 					continue;
 				register_shallow(sha1);
-			}
+			}  else if (!strncmp("unshallow ", line, 10)) {
+				if (get_sha1_hex(line + 10, sha1))
+					die("invalid unshallow line: %s", line);
+				if (!lookup_object(sha1))
+					die("object not found: %s", line);
+				/* make sure that it is parsed as shallow */
+				parse_object(sha1);
+				if (unregister_shallow(sha1))
+					die("no shallow found: %s", line);
+			} else
+				die("expected shallow/unshallow, got %s", line);
 		}
 	}
 
@@ -388,9 +398,11 @@ static int everything_local(struct ref *
 		}
 	}
 
-	for_each_ref(mark_complete, NULL);
-	if (cutoff)
-		mark_recent_complete_commits(cutoff);
+	if (!depth) {
+		for_each_ref(mark_complete, NULL);
+		if (cutoff)
+			mark_recent_complete_commits(cutoff);
+	}
 
 	/*
 	 * Mark all complete remote refs as common refs.
@@ -550,8 +562,6 @@ int main(int argc, char **argv)
 	}
 	if (!dest)
 		usage(fetch_pack_usage);
-	if (is_repository_shallow() && depth > 0)
-		die("Deepening of a shallow repository not yet supported!");
 	pid = git_connect(fd, dest, exec);
 	if (pid < 0)
 		return 1;
diff --git a/git-fetch.sh b/git-fetch.sh
index 539dff6..2a4b7a0 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -21,6 +21,7 @@ update_head_ok=
 exec=
 upload_pack=
 keep=--thin
+depth=
 while case "$#" in 0) break ;; esac
 do
 	case "$1" in
@@ -56,6 +57,13 @@ do
 	--reflog-action=*)
 		rloga=`expr "z$1" : 'z-[^=]*=\(.*\)'`
 		;;
+	--depth=*)
+		depth="--depth=`expr "z$1" : 'z-[^=]*=\(.*\)'`"
+		;;
+	--depth)
+		shift
+		depth="--depth=$1"
+		;;
 	-*)
 		usage
 		;;
@@ -296,6 +304,7 @@ fetch_main () {
       # There are transports that can fetch only one head at a time...
       case "$remote" in
       http://* | https://* | ftp://*)
+	  test -n "$depth" && die "shallow clone with http not supported"
 	  proto=`expr "$remote" : '\([^:]*\):'`
 	  if [ -n "$GIT_SSL_NO_VERIFY" ]; then
 	      curl_extra_args="-k"
@@ -324,6 +333,7 @@ fetch_main () {
 	  git-http-fetch -v -a "$head" "$remote/" || exit
 	  ;;
       rsync://*)
+	  test -n "$depth" && die "shallow clone with rsync not supported"
 	  TMP_HEAD="$GIT_DIR/TMP_HEAD"
 	  rsync -L -q "$remote/$remote_name" "$TMP_HEAD" || exit 1
 	  head=$(git-rev-parse --verify TMP_HEAD)
@@ -370,7 +380,7 @@ fetch_main () {
     ( : subshell because we muck with IFS
       IFS=" 	$LF"
       (
-	  git-fetch-pack $exec $keep "$remote" $rref || echo failed "$remote"
+	  git-fetch-pack $exec $keep $depth "$remote" $rref || echo failed "$remote"
       ) |
       while read sha1 remote_name
       do
diff --git a/shallow.c b/shallow.c
index 3cf2127..58a7b20 100644
--- a/shallow.c
+++ b/shallow.c
@@ -41,7 +41,8 @@ int is_repository_shallow()
 	return is_shallow;
 }
 
-struct commit_list *get_shallow_commits(struct object_array *heads, int depth)
+struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
+		int shallow_flag, int not_shallow_flag)
 {
 	int i = 0, cur_depth = 0;
 	struct commit_list *result = NULL;
@@ -67,6 +68,7 @@ struct commit_list *get_shallow_commits(
 			}
 		}
 		parse_commit(commit);
+		commit->object.flags |= not_shallow_flag;
 		cur_depth++;
 		for (p = commit->parents, commit = NULL; p; p = p->next) {
 			if (!p->item->util) {
@@ -87,8 +89,10 @@ struct commit_list *get_shallow_commits(
 					commit = p->item;
 					cur_depth = *(int *)commit->util;
 				}
-			} else
+			} else {
 				commit_list_insert(p->item, &result);
+				p->item->object.flags |= shallow_flag;
+			}
 		}
 	}
 
diff --git a/upload-pack.c b/upload-pack.c
index ebe1e5a..162dd34 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -134,6 +134,7 @@ static void create_pack_file(void)
 		} else {
 			for (i = 0; i < want_obj.nr; i++) {
 				struct object *o = want_obj.objects[i].item;
+				o->flags &= ~UNINTERESTING;
 				add_pending_object(&revs, o, NULL);
 			}
 			for (i = 0; i < have_obj.nr; i++) {
@@ -490,6 +491,9 @@ static void receive_needs(void)
 	static char line[1000];
 	int len, depth = 0;
 
+#define SHALLOW (1u<<8)
+#define NOT_SHALLOW (1u<<9)
+#define CLIENT_SHALLOW (1u<<10)
 	for (;;) {
 		struct object *o;
 		unsigned char sha1_buf[20];
@@ -501,16 +505,19 @@ static void receive_needs(void)
 		if (!strncmp("shallow ", line, 8)) {
 			unsigned char sha1[20];
 			struct object *object;
+			use_thin_pack = 0;
 			if (get_sha1(line + 8, sha1))
 				die("invalid shallow line: %s", line);
 			object = parse_object(sha1);
 			if (!object)
 				die("did not find object for %s", line);
+			object->flags |= CLIENT_SHALLOW;
 			add_object_array(object, NULL, &shallows);
 			continue;
 		}
 		if (!strncmp("deepen ", line, 7)) {
 			char *end;
+			use_thin_pack = 0;
 			depth = strtol(line + 7, &end, 0);
 			if (end == line + 7 || depth <= 0)
 				die("Invalid deepen: %s", line);
@@ -547,23 +554,51 @@ static void receive_needs(void)
 			add_object_array(o, NULL, &want_obj);
 		}
 	}
+	if (depth == 0 && shallows.nr == 0)
+		return;
 	if (depth > 0) {
 		struct commit_list *result, *backup;
-		if (shallows.nr > 0)
-			die("Deepening a shallow repository not yet supported");
-		backup = result = get_shallow_commits(&want_obj, depth);
+		int i;
+		backup = result = get_shallow_commits(&want_obj, depth,
+			SHALLOW, NOT_SHALLOW);
 		while (result) {
-			packet_write(1, "shallow %s",
-					sha1_to_hex(result->item->object.sha1));
+			struct object *object = &result->item->object;
+			if (!(object->flags & CLIENT_SHALLOW)) {
+				packet_write(1, "shallow %s",
+						sha1_to_hex(object->sha1));
+				register_shallow(object->sha1);
+			}
 			result = result->next;
 		}
 		free_commit_list(backup);
-	}
-	if (shallows.nr > 0) {
-		int i;
-		for (i = 0; i < shallows.nr; i++)
-			register_shallow(shallows.objects[i].item->sha1);
-	}
+		for (i = 0; i < shallows.nr; i++) {
+			struct object *object = shallows.objects[i].item;
+			if (object->flags & NOT_SHALLOW) {
+				struct commit_list *parents;
+				packet_write(1, "unshallow %s",
+					sha1_to_hex(object->sha1));
+				object->flags &= ~CLIENT_SHALLOW;
+				/* make sure the real parents are parsed */
+				unregister_shallow(object->sha1);
+				parse_commit((struct commit *)object);
+				parents = ((struct commit *)object)->parents;
+				while (parents) {
+					add_object_array(&parents->item->object,
+							NULL, &want_obj);
+					parents = parents->next;
+				}
+			}
+			/* make sure commit traversal conforms to client */
+			register_shallow(object->sha1);
+		}
+		packet_flush(1);
+	} else
+		if (shallows.nr > 0) {
+			int i;
+			for (i = 0; i < shallows.nr; i++)
+				register_shallow(shallows.objects[i].item->sha1);
+		}
+	free(shallows.objects);
 }
 
 static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
-- 
1.4.3.3.gca42

Re: [PATCH 4/5] allow deepening of a shallow repository

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Now, by saying "git fetch -depth <n> <repo>" you can deepen
> a shallow repository.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  commit.c      |   13 +++++++++++++
>  commit.h      |    3 ++-
>  fetch-pack.c  |   22 ++++++++++++++++------
>  git-fetch.sh  |   12 +++++++++++-
>  shallow.c     |    8 ++++++--
>  upload-pack.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++-----------
> diff --git a/upload-pack.c b/upload-pack.c
> index ebe1e5a..162dd34 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -134,6 +134,7 @@ static void create_pack_file(void)
>  		} else {
>  			for (i = 0; i < want_obj.nr; i++) {
>  				struct object *o = want_obj.objects[i].item;
> +				o->flags &= ~UNINTERESTING;
>  				add_pending_object(&revs, o, NULL);
>  			}
>  			for (i = 0; i < have_obj.nr; i++) {

I am puzzled why this is needed in this series.  In other words,
do we have a bug in the current upload-pack that does not have
shallow already by not clearing UNINTERESTING bit?  In yet other
words, I haven't figured out which part of the shallow series
makes it necessary to clear UNINTERESTING bit from wanted
object.

> @@ -547,23 +554,51 @@ static void receive_needs(void)
>...
> +		for (i = 0; i < shallows.nr; i++) {
> +			struct object *object = shallows.objects[i].item;
> +			if (object->flags & NOT_SHALLOW) {
> +				struct commit_list *parents;
> +				packet_write(1, "unshallow %s",
> +					sha1_to_hex(object->sha1));
> +				object->flags &= ~CLIENT_SHALLOW;
> +				/* make sure the real parents are parsed */
> +				unregister_shallow(object->sha1);
> +				parse_commit((struct commit *)object);
> +				parents = ((struct commit *)object)->parents;
> +				while (parents) {
> +					add_object_array(&parents->item->object,
> +							NULL, &want_obj);
> +					parents = parents->next;
> +				}
> +			}

I doubt unregister_shallow() is enough to ensure that the next
parse_commit() re-parses to recover its parents.  parse_commit()
says "if (item->object.parsed) return 0" upfront.  Don't you
need to do:

	object->parsed = 0;

before parse_commit()?

Re: [PATCH 4/5] allow deepening of a shallow repository

[Johannes Schindelin]

Hi,

On Mon, 13 Nov 2006, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Now, by saying "git fetch -depth <n> <repo>" you can deepen
> > a shallow repository.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  commit.c      |   13 +++++++++++++
> >  commit.h      |    3 ++-
> >  fetch-pack.c  |   22 ++++++++++++++++------
> >  git-fetch.sh  |   12 +++++++++++-
> >  shallow.c     |    8 ++++++--
> >  upload-pack.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++-----------
> > diff --git a/upload-pack.c b/upload-pack.c
> > index ebe1e5a..162dd34 100644
> > --- a/upload-pack.c
> > +++ b/upload-pack.c
> > @@ -134,6 +134,7 @@ static void create_pack_file(void)
> >  		} else {
> >  			for (i = 0; i < want_obj.nr; i++) {
> >  				struct object *o = want_obj.objects[i].item;
> > +				o->flags &= ~UNINTERESTING;
> >  				add_pending_object(&revs, o, NULL);
> >  			}
> >  			for (i = 0; i < have_obj.nr; i++) {
> 
> I am puzzled why this is needed in this series.  In other words,
> do we have a bug in the current upload-pack that does not have
> shallow already by not clearing UNINTERESTING bit?  In yet other
> words, I haven't figured out which part of the shallow series
> makes it necessary to clear UNINTERESTING bit from wanted
> object.

IIRC (has been a long time, hasn't it?) the test failed without that line, 
and I was too lazy to investigate why. But ICRI ;-)

> > @@ -547,23 +554,51 @@ static void receive_needs(void)
> >...
> > +		for (i = 0; i < shallows.nr; i++) {
> > +			struct object *object = shallows.objects[i].item;
> > +			if (object->flags & NOT_SHALLOW) {
> > +				struct commit_list *parents;
> > +				packet_write(1, "unshallow %s",
> > +					sha1_to_hex(object->sha1));
> > +				object->flags &= ~CLIENT_SHALLOW;
> > +				/* make sure the real parents are parsed */
> > +				unregister_shallow(object->sha1);
> > +				parse_commit((struct commit *)object);
> > +				parents = ((struct commit *)object)->parents;
> > +				while (parents) {
> > +					add_object_array(&parents->item->object,
> > +							NULL, &want_obj);
> > +					parents = parents->next;
> > +				}
> > +			}
> 
> I doubt unregister_shallow() is enough to ensure that the next
> parse_commit() re-parses to recover its parents.  parse_commit()
> says "if (item->object.parsed) return 0" upfront.  Don't you
> need to do:
> 
> 	object->parsed = 0;
> 
> before parse_commit()?

Yes. Somehow, an important part of unregister_shallow() went missing (yet 
another proof that my poor-man's-StGit does not always work). I think that 
the "object->parsed = 0;" should go into unregister_shallow() like this:

diff --git a/commit.c b/commit.c
index d5103cd..4451376 100644
--- a/commit.c
+++ b/commit.c
@@ -258,6 +258,7 @@ int write_shallow_commits(int fd, int us
 int unregister_shallow(const unsigned char *sha1)
 {
 	int pos = commit_graft_pos(sha1);
+	struct commit *commit = lookup_commit(sha1);
 	if (pos < 0)
 		return -1;
 	if (pos + 1 < commit_graft_nr)
@@ -265,6 +266,8 @@ int unregister_shallow(const unsigned ch
 				sizeof(struct commit_graft *)
 				* (commit_graft_nr - pos - 1));
 	commit_graft_nr--;
+	if (commit)
+		commit->object.parsed = 0;
 	return 0;
 }
 
Ciao,
Dscho

Re: [PATCH 4/5] allow deepening of a shallow repository

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Yes. Somehow, an important part of unregister_shallow() went missing (yet 
> another proof that my poor-man's-StGit does not always work). I think that 
> the "object->parsed = 0;" should go into unregister_shallow() like this:

That does not fly so well.  Your fetch-pack.c does this in
find_common() and dropping the parsed flag inside unregister
causes it to be parsed again later with its true parents, which
defeats what the commented part of the code wanted to do.

	if (!lookup_object(sha1))
		die("object not found: %s", line);
	/* make sure that it is parsed as shallow */
	parse_object(sha1);
	if (unregister_shallow(sha1))
		die("no shallow found: %s", line);
	continue;

Although I am reasonably sure we can eventually make it work, it
is very subtle and fragile -- somebody touching this code can
easily break it.
.

Re: [PATCH 4/5] allow deepening of a shallow repository

[Johannes Schindelin]

Hi,

On Fri, 17 Nov 2006, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Yes. Somehow, an important part of unregister_shallow() went missing (yet 
> > another proof that my poor-man's-StGit does not always work). I think that 
> > the "object->parsed = 0;" should go into unregister_shallow() like this:
> 
> That does not fly so well.  Your fetch-pack.c does this in
> find_common() and dropping the parsed flag inside unregister
> causes it to be parsed again later with its true parents, which
> defeats what the commented part of the code wanted to do.
> 
> 	if (!lookup_object(sha1))
> 		die("object not found: %s", line);
> 	/* make sure that it is parsed as shallow */
> 	parse_object(sha1);
> 	if (unregister_shallow(sha1))
> 		die("no shallow found: %s", line);
> 	continue;

Ah! _That_ was the reason I removed it from unregister_shallow().

> Although I am reasonably sure we can eventually make it work, it
> is very subtle and fragile -- somebody touching this code can
> easily break it.

I fully agree. Even the OA did not understand the code fully ;-)

How about adding "int force_reparse" to the signature of 
unregister_shallow()? (Just like we added "int cleanup" to 
get_merge_bases() to avoid pilot errors.)

Ciao,
Dscho

Re: [PATCH 4/5] allow deepening of a shallow repository

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Although I am reasonably sure we can eventually make it work, it
>> is very subtle and fragile -- somebody touching this code can
>> easily break it.
>
> I fully agree. Even the OA did not understand the code fully ;-)
>
> How about adding "int force_reparse" to the signature of 
> unregister_shallow()? (Just like we added "int cleanup" to 
> get_merge_bases() to avoid pilot errors.)

I do not think that's where its fragility lies.  It is that any
random place can later call parse_object() on the commit, after
you elaborately pre-parsed it with shallow so that it appears to
have fewer parents, with the expectation that nobody ever would
clear the parsed bit and cause it to be reparsed again.  With
that assumption, find_common() manipulated the shallow entry
after setting that scheme up.  A mechanism to prevent the commit
from getting re-parsed would have made it more robust.

Re: [PATCH 4/5] allow deepening of a shallow repository

[Johannes Schindelin]

Hi,

On Fri, 17 Nov 2006, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> Although I am reasonably sure we can eventually make it work, it
> >> is very subtle and fragile -- somebody touching this code can
> >> easily break it.
> >
> > I fully agree. Even the OA did not understand the code fully ;-)
> >
> > How about adding "int force_reparse" to the signature of 
> > unregister_shallow()? (Just like we added "int cleanup" to 
> > get_merge_bases() to avoid pilot errors.)
> 
> I do not think that's where its fragility lies.  It is that any
> random place can later call parse_object() on the commit, after
> you elaborately pre-parsed it with shallow so that it appears to
> have fewer parents, with the expectation that nobody ever would
> clear the parsed bit and cause it to be reparsed again.  With
> that assumption, find_common() manipulated the shallow entry
> after setting that scheme up.  A mechanism to prevent the commit
> >from getting re-parsed would have made it more robust.

How about putting yet more meaning into nr_parents: if it is negative, it 
is a shallow commit, but write_shallow_commits() only writes the SHA1 if 
nr_parents is -1. Hmm?

Ciao,
Dscho