[PATCH 0/7] Make alternates affect fetch behavior

[PATCH 0/7] Make alternates affect fetch behavior

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

It used to be that alternate references were not considered "complete"
when fetching via fetch-pack.  This failure was not so obvious because
the big benefit of alternates is seen when cloning, and clone used a
different data path: it put the alternate references into extra refs
(which makes them look like references within the local repository).

This patch series teaches fetch-pack to treat objects that are
available via alternates as "complete".

Once that is fixed, clone doesn't need to use the special extra_refs
kludge, so change that.

And once that is changed, the extra_refs API is no longer needed at
all, so remove it.

Michael Haggerty (7):
  t5700: document a failure of alternates to affect fetch
  clone.c: move more code into the "if (refs)" conditional
  fetch-pack.c: rename some parameters from "path" to "refname"
  fetch-pack.c: inline insert_alternate_refs()
  everything_local(): mark alternate refs as complete
  clone: do not add alternate references to extra_refs
  refs: remove the extra_refs API

 builtin/clone.c            |   51 +++++++++++++++++--------------------------
 builtin/fetch-pack.c       |   23 ++++++++++---------
 refs.c                     |   23 +-------------------
 refs.h                     |    8 -------
 t/t5700-clone-reference.sh |   34 ++++++++++++++++++++++++++--
 5 files changed, 64 insertions(+), 75 deletions(-)

-- 
1.7.9

[PATCH 1/7] t5700: document a failure of alternates to affect fetch

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

If an alternate supplies some, but not all, of the objects needed for
a fetch, fetch-pack nevertheless generates "want" lines for the
alternate objects that are present.  Demonstrate this problem via a
failing test.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t5700-clone-reference.sh |   34 +++++++++++++++++++++++++++++++---
 1 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index c4c375a..2dafee8 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -52,13 +52,13 @@ test_cmp expected current'
 
 cd "$base_dir"
 
-rm -f "$U"
+rm -f "$U.D"
 
 test_expect_success 'cloning with reference (no -l -s)' \
-'GIT_DEBUG_SEND_PACK=3 git clone --reference B "file://$(pwd)/A" D 3>"$U"'
+'GIT_DEBUG_SEND_PACK=3 git clone --reference B "file://$(pwd)/A" D 3>"$U.D"'
 
 test_expect_success 'fetched no objects' \
-'! grep "^want" "$U"'
+'! grep "^want" "$U.D"'
 
 cd "$base_dir"
 
@@ -153,4 +153,32 @@ test_expect_success 'clone with reference from a tagged repository' '
 	git clone --reference=A A I
 '
 
+test_expect_success 'prepare branched repository' '
+	git clone A J &&
+	(
+		cd J &&
+		git checkout -b other master^ &&
+		echo other > otherfile &&
+		git add otherfile &&
+		git commit -m other &&
+		git checkout master
+	)
+'
+
+rm -f "$U.K"
+
+test_expect_failure 'fetch with incomplete alternates' '
+	git init K &&
+	echo "$base_dir/A/.git/objects" >K/.git/objects/info/alternates &&
+	(
+		cd K &&
+		git remote add J "file://$base_dir/J" &&
+		GIT_DEBUG_SEND_PACK=3 git fetch J 3>"$U.K"
+	) &&
+	master_object=$(cd A && git for-each-ref --format="%(objectname)" refs/heads/master) &&
+	! grep "^want $master_object" "$U.K" &&
+	tag_object=$(cd A && git for-each-ref --format="%(objectname)" refs/tags/HEAD) &&
+	! grep "^want $tag_object" "$U.K"
+'
+
 test_done
-- 
1.7.9

[PATCH 2/7] clone.c: move more code into the "if (refs)" conditional

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

The bahavior of a bunch of code before the "if (refs)" statement also
depends on whether refs is set, so make the logic clearer by shifting
this code into the if statement.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/clone.c |   39 ++++++++++++++++++++-------------------
 1 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index c62d4b5..279fdf0 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -813,28 +813,28 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 
 	refs = transport_get_remote_refs(transport);
-	mapped_refs = refs ? wanted_peer_refs(refs, refspec) : NULL;
 
-	/*
-	 * transport_get_remote_refs() may return refs with null sha-1
-	 * in mapped_refs (see struct transport->get_refs_list
-	 * comment). In that case we need fetch it early because
-	 * remote_head code below relies on it.
-	 *
-	 * for normal clones, transport_get_remote_refs() should
-	 * return reliable ref set, we can delay cloning until after
-	 * remote HEAD check.
-	 */
-	for (ref = refs; ref; ref = ref->next)
-		if (is_null_sha1(ref->old_sha1)) {
-			complete_refs_before_fetch = 0;
-			break;
-		}
+	if (refs) {
+		mapped_refs = wanted_peer_refs(refs, refspec);
+		/*
+		 * transport_get_remote_refs() may return refs with null sha-1
+		 * in mapped_refs (see struct transport->get_refs_list
+		 * comment). In that case we need fetch it early because
+		 * remote_head code below relies on it.
+		 *
+		 * for normal clones, transport_get_remote_refs() should
+		 * return reliable ref set, we can delay cloning until after
+		 * remote HEAD check.
+		 */
+		for (ref = refs; ref; ref = ref->next)
+			if (is_null_sha1(ref->old_sha1)) {
+				complete_refs_before_fetch = 0;
+				break;
+			}
 
-	if (!is_local && !complete_refs_before_fetch && refs)
-		transport_fetch_refs(transport, mapped_refs);
+		if (!is_local && !complete_refs_before_fetch)
+			transport_fetch_refs(transport, mapped_refs);
 
-	if (refs) {
 		remote_head = find_ref_by_name(refs, "HEAD");
 		remote_head_points_at =
 			guess_remote_head(remote_head, mapped_refs, 0);
@@ -852,6 +852,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 	else {
 		warning(_("You appear to have cloned an empty repository."));
+		mapped_refs = NULL;
 		our_head_points_at = NULL;
 		remote_head_points_at = NULL;
 		remote_head = NULL;
-- 
1.7.9

[PATCH 3/7] fetch-pack.c: rename some parameters from "path" to "refname"

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

The parameters denote reference names, which are no longer 1:1 with
filesystem paths.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/fetch-pack.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 6207ecd..9bd2096 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -58,9 +58,9 @@ static void rev_list_push(struct commit *commit, int mark)
 	}
 }
 
-static int rev_list_insert_ref(const char *path, const unsigned char *sha1, int flag, void *cb_data)
+static int rev_list_insert_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
-	struct object *o = deref_tag(parse_object(sha1), path, 0);
+	struct object *o = deref_tag(parse_object(sha1), refname, 0);
 
 	if (o && o->type == OBJ_COMMIT)
 		rev_list_push((struct commit *)o, SEEN);
@@ -68,9 +68,9 @@ static int rev_list_insert_ref(const char *path, const unsigned char *sha1, int
 	return 0;
 }
 
-static int clear_marks(const char *path, const unsigned char *sha1, int flag, void *cb_data)
+static int clear_marks(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
-	struct object *o = deref_tag(parse_object(sha1), path, 0);
+	struct object *o = deref_tag(parse_object(sha1), refname, 0);
 
 	if (o && o->type == OBJ_COMMIT)
 		clear_commit_marks((struct commit *)o,
@@ -493,7 +493,7 @@ done:
 
 static struct commit_list *complete;
 
-static int mark_complete(const char *path, const unsigned char *sha1, int flag, void *cb_data)
+static int mark_complete(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
 	struct object *o = parse_object(sha1);
 
-- 
1.7.9

[PATCH 4/7] fetch-pack.c: inline insert_alternate_refs()

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

The logic of the (single) caller is clearer without encapsulating this
one line in a function.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/fetch-pack.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 9bd2096..dbe9acb 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -256,11 +256,6 @@ static void insert_one_alternate_ref(const struct ref *ref, void *unused)
 	rev_list_insert_ref(NULL, ref->old_sha1, 0, NULL);
 }
 
-static void insert_alternate_refs(void)
-{
-	for_each_alternate_ref(insert_one_alternate_ref, NULL);
-}
-
 #define INITIAL_FLUSH 16
 #define PIPESAFE_FLUSH 32
 #define LARGE_FLUSH 1024
@@ -295,7 +290,7 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 	marked = 1;
 
 	for_each_ref(rev_list_insert_ref, NULL);
-	insert_alternate_refs();
+	for_each_alternate_ref(insert_one_alternate_ref, NULL);
 
 	fetching = 0;
 	for ( ; refs ; refs = refs->next) {
-- 
1.7.9

[PATCH 5/7] everything_local(): mark alternate refs as complete

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

Objects in an alternate object database are already available to the
local repository and therefore don't need to be fetched.  So mark them
as complete in everything_local().

This fixes a test in t5700.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/fetch-pack.c       |    6 ++++++
 t/t5700-clone-reference.sh |    2 +-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index dbe9acb..0e8560f 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -581,6 +581,11 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
 	*refs = newlist;
 }
 
+static void mark_alternate_complete(const struct ref *ref, void *unused)
+{
+	mark_complete(NULL, ref->old_sha1, 0, NULL);
+}
+
 static int everything_local(struct ref **refs, int nr_match, char **match)
 {
 	struct ref *ref;
@@ -609,6 +614,7 @@ static int everything_local(struct ref **refs, int nr_match, char **match)
 
 	if (!args.depth) {
 		for_each_ref(mark_complete, NULL);
+		for_each_alternate_ref(mark_alternate_complete, NULL);
 		if (cutoff)
 			mark_recent_complete_commits(cutoff);
 	}
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index 2dafee8..783f988 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -167,7 +167,7 @@ test_expect_success 'prepare branched repository' '
 
 rm -f "$U.K"
 
-test_expect_failure 'fetch with incomplete alternates' '
+test_expect_success 'fetch with incomplete alternates' '
 	git init K &&
 	echo "$base_dir/A/.git/objects" >K/.git/objects/info/alternates &&
 	(
-- 
1.7.9

[PATCH 6/7] clone: do not add alternate references to extra_refs

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

Alternate references are directly (and now, correctly) handled by
fetch-pack, so there is no need to inform fetch-pack about them via
the extra_refs back channel.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/clone.c |   12 ------------
 1 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 279fdf0..b15fccb 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -232,9 +232,6 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
 {
 	char *ref_git;
 	struct strbuf alternate = STRBUF_INIT;
-	struct remote *remote;
-	struct transport *transport;
-	const struct ref *extra;
 
 	/* Beware: real_path() and mkpath() return static buffer */
 	ref_git = xstrdup(real_path(item->string));
@@ -249,14 +246,6 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
 	strbuf_addf(&alternate, "%s/objects", ref_git);
 	add_to_alternates_file(alternate.buf);
 	strbuf_release(&alternate);
-
-	remote = remote_get(ref_git);
-	transport = transport_get(remote, ref_git);
-	for (extra = transport_get_remote_refs(transport); extra;
-	     extra = extra->next)
-		add_extra_ref(extra->name, extra->old_sha1, 0);
-
-	transport_disconnect(transport);
 	free(ref_git);
 	return 0;
 }
@@ -500,7 +489,6 @@ static void update_remote_refs(const struct ref *refs,
 			       const char *msg)
 {
 	if (refs) {
-		clear_extra_refs();
 		write_remote_refs(mapped_refs);
 		if (option_single_branch)
 			write_followtags(refs, msg);
-- 
1.7.9

[PATCH 7/7] refs: remove the extra_refs API

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

The extra_refs provided a kludgy way to create fake references at a
global level in the hope that they would only affect some particular
code path.  The last user of this API been rewritten, so strip this
stuff out before somebody else gets the bad idea of using it.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   23 +----------------------
 refs.h |    8 --------
 2 files changed, 1 insertions(+), 30 deletions(-)

diff --git a/refs.c b/refs.c
index b8843bb..c9f6835 100644
--- a/refs.c
+++ b/refs.c
@@ -183,12 +183,6 @@ static struct ref_cache {
 
 static struct ref_entry *current_ref;
 
-/*
- * Never call sort_ref_array() on the extra_refs, because it is
- * allowed to contain entries with duplicate names.
- */
-static struct ref_array extra_refs;
-
 static void clear_ref_array(struct ref_array *array)
 {
 	int i;
@@ -289,16 +283,6 @@ static void read_packed_refs(FILE *f, struct ref_array *array)
 	}
 }
 
-void add_extra_ref(const char *refname, const unsigned char *sha1, int flag)
-{
-	add_ref(&extra_refs, create_ref_entry(refname, sha1, flag, 0));
-}
-
-void clear_extra_refs(void)
-{
-	clear_ref_array(&extra_refs);
-}
-
 static struct ref_array *get_packed_refs(struct ref_cache *refs)
 {
 	if (!refs->did_packed) {
@@ -733,16 +717,11 @@ fallback:
 static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn,
 			   int trim, int flags, void *cb_data)
 {
-	int retval = 0, i, p = 0, l = 0;
+	int retval = 0, p = 0, l = 0;
 	struct ref_cache *refs = get_ref_cache(submodule);
 	struct ref_array *packed = get_packed_refs(refs);
 	struct ref_array *loose = get_loose_refs(refs);
 
-	struct ref_array *extra = &extra_refs;
-
-	for (i = 0; i < extra->nr; i++)
-		retval = do_one_ref(base, fn, trim, flags, cb_data, extra->refs[i]);
-
 	sort_ref_array(packed);
 	sort_ref_array(loose);
 	while (p < packed->nr && l < loose->nr) {
diff --git a/refs.h b/refs.h
index 00ba1e2..33202b0 100644
--- a/refs.h
+++ b/refs.h
@@ -56,14 +56,6 @@ extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refn
  */
 extern void add_packed_ref(const char *refname, const unsigned char *sha1);
 
-/*
- * Extra refs will be listed by for_each_ref() before any actual refs
- * for the duration of this process or until clear_extra_refs() is
- * called. Only extra refs added before for_each_ref() is called will
- * be listed on a given call of for_each_ref().
- */
-extern void add_extra_ref(const char *refname, const unsigned char *sha1, int flags);
-extern void clear_extra_refs(void);
 extern int ref_exists(const char *);
 
 extern int peel_ref(const char *refname, unsigned char *sha1);
-- 
1.7.9

Re: [PATCH 1/7] t5700: document a failure of alternates to affect fetch

[Junio C Hamano]

mhagger@alum.mit.edu writes:

> +test_expect_success 'prepare branched repository' '
> +	git clone A J &&
> +	(
> +		cd J &&
> +		git checkout -b other master^ &&
> +		echo other > otherfile &&

s/ > / >/; but that is nothing I cannot fix locally.

> +test_expect_failure 'fetch with incomplete alternates' '

I am assuming that this "incomplete" means "this alternate helps reducing
the number of objects we need to fetch from the remote, but it does not
have everything objects we need, and we still need to fetch some from the
remote".  Am I correct?

I do not think you meant the alternate repository is in some way corrupt,
but I am just making sure, because I found the phrasing a bit odd.

Re: [PATCH 2/7] clone.c: move more code into the "if (refs)" conditional

[Junio C Hamano]

mhagger@alum.mit.edu writes:

> From: Michael Haggerty <mhagger@alum.mit.edu>
>
> The bahavior of a bunch of code before the "if (refs)" statement also
> depends on whether refs is set, so make the logic clearer by shifting
> this code into the if statement.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  builtin/clone.c |   39 ++++++++++++++++++++-------------------
>  1 files changed, 20 insertions(+), 19 deletions(-)

Nice; the result is much easier to follow.

Re: [PATCH 3/7] fetch-pack.c: rename some parameters from "path" to "refname"

[Junio C Hamano]

mhagger@alum.mit.edu writes:

> From: Michael Haggerty <mhagger@alum.mit.edu>
>
> The parameters denote reference names, which are no longer 1:1 with
> filesystem paths.

These three functions are only used as callback from for_each_ref() so
they always get the full refname and nothing else (like a partial refname
like tags/v1.7.9), so calling them refname makes perfect sense.

Even though I generally try to stay away from this kind of naming churn
patches, but hopefully there is nothing in flight to cause horrible
conflict with it.

Thanks.

Re: [PATCH 6/7] clone: do not add alternate references to extra_refs

[Junio C Hamano]

mhagger@alum.mit.edu writes:

> From: Michael Haggerty <mhagger@alum.mit.edu>
>
> Alternate references are directly (and now, correctly) handled by
> fetch-pack, so there is no need to inform fetch-pack about them via
> the extra_refs back channel.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  builtin/clone.c |   12 ------------
>  1 files changed, 0 insertions(+), 12 deletions(-)

Very nice (the real niceness primarily comes from the previous step ;-).

Thanks.

Re: [PATCH 1/7] t5700: document a failure of alternates to affect fetch

[Michael Haggerty]

On 02/13/2012 04:35 AM, Junio C Hamano wrote:
> mhagger@alum.mit.edu writes:
> 
>> +test_expect_success 'prepare branched repository' '
>> +	git clone A J &&
>> +	(
>> +		cd J &&
>> +		git checkout -b other master^ &&
>> +		echo other > otherfile &&
> 
> s/ > / >/; but that is nothing I cannot fix locally.
> 
>> +test_expect_failure 'fetch with incomplete alternates' '
> 
> I am assuming that this "incomplete" means "this alternate helps reducing
> the number of objects we need to fetch from the remote, but it does not
> have everything objects we need, and we still need to fetch some from the
> remote".  Am I correct?

Correct.  Feel free to improve the description if you can think of a
better way to describe it.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 0/7] Make alternates affect fetch behavior

[Jeff King]

On Sat, Feb 11, 2012 at 07:20:54AM +0100, mhagger@alum.mit.edu wrote:

> It used to be that alternate references were not considered "complete"
> when fetching via fetch-pack.  This failure was not so obvious because
> the big benefit of alternates is seen when cloning, and clone used a
> different data path: it put the alternate references into extra refs
> (which makes them look like references within the local repository).
> 
> This patch series teaches fetch-pack to treat objects that are
> available via alternates as "complete".
> 
> Once that is fixed, clone doesn't need to use the special extra_refs
> kludge, so change that.
> 
> And once that is changed, the extra_refs API is no longer needed at
> all, so remove it.
> 
> Michael Haggerty (7):
>   t5700: document a failure of alternates to affect fetch
>   clone.c: move more code into the "if (refs)" conditional
>   fetch-pack.c: rename some parameters from "path" to "refname"
>   fetch-pack.c: inline insert_alternate_refs()
>   everything_local(): mark alternate refs as complete
>   clone: do not add alternate references to extra_refs
>   refs: remove the extra_refs API

>From my reading, all of these patches look good. Thanks for a
well-organized series.

-Peff