Re: git clone --reference not working

Re: git clone --reference not working

[Junio C Hamano]

Andrea Arcangeli <aarcange@redhat.com> writes:

> latest git.git won't clone linux upstream with --reference. Those
> v*^{} tags breaks it. What's that stuff anyway, looks totally ugly
> (two commits with same data contents and header) bah.

They point at commits they tag, and are essential for auto-following. They
have been there forever in ls-remote output and they are not the real
problem.

A recent topic that was merged at 9bd5000 tightened the refname checking
code without thinking and started to needlessly barf upon seeing them. I
think we have discussed about the issue on the list, but I do not think
there were fixes yet.

Thanks for reminding.

Michael, how does this look?

-- >8 --
Subject: refs: loosen over-strict "format" check

The add_extra_ref() interface is used to add an extra-ref that is _not_
our ref for the purpose of helping auto-following of tags and reducing
object transfer from remote repository, and they are typically formatted
as a tagname followed by ^{} to make sure no valid refs match that
pattern. In other words, these entries are deliberately formatted not to
pass check-refname-format test.

A recent series however added a test unconditionally to the add_ref()
function that is called from add_extra_ref(). The check may be sensible
for other two callsites of the add_ref() interface, but definitely is
a wrong thing to do in add_extra_ref(). Disable it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

* In general, we should make things lenient when we are reading from
  existing source, so that the code can be used to recover a repository
  with badly formatted refs left by other tools, and enforce the format
  check when we write things out. We may further need to loosen the checks
  introduced earlier by mh/check-ref-format-3 topic for similar breakages
  as they are discovered.

 refs.c                     |   20 ++++++++++----------
 t/t5700-clone-reference.sh |    7 +++++++
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index e69ba26..e7843eb 100644
--- a/refs.c
+++ b/refs.c
@@ -48,7 +48,7 @@ static const char *parse_ref_line(char *line, unsigned char *sha1)
 }
 
 static void add_ref(const char *name, const unsigned char *sha1,
-		    int flag, struct ref_array *refs,
+		    int flag, int check_name, struct ref_array *refs,
 		    struct ref_entry **new_entry)
 {
 	int len;
@@ -59,7 +59,8 @@ static void add_ref(const char *name, const unsigned char *sha1,
 	entry = xmalloc(sizeof(struct ref_entry) + len);
 	hashcpy(entry->sha1, sha1);
 	hashclr(entry->peeled);
-	if (check_refname_format(name, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT))
+	if (check_name &&
+	    check_refname_format(name, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT))
 		die("Reference has invalid format: '%s'", name);
 	memcpy(entry->name, name, len);
 	entry->flag = flag;
@@ -234,7 +235,7 @@ static void read_packed_refs(FILE *f, struct ref_array *array)
 
 		name = parse_ref_line(refline, sha1);
 		if (name) {
-			add_ref(name, sha1, flag, array, &last);
+			add_ref(name, sha1, flag, 1, array, &last);
 			continue;
 		}
 		if (last &&
@@ -249,7 +250,7 @@ static void read_packed_refs(FILE *f, struct ref_array *array)
 
 void add_extra_ref(const char *name, const unsigned char *sha1, int flag)
 {
-	add_ref(name, sha1, flag, &extra_refs, NULL);
+	add_ref(name, sha1, flag, 0, &extra_refs, NULL);
 }
 
 void clear_extra_refs(void)
@@ -333,12 +334,11 @@ static void get_ref_dir(const char *submodule, const char *base,
 					hashclr(sha1);
 					flag |= REF_ISBROKEN;
 				}
-			} else
-				if (!resolve_ref(ref, sha1, 1, &flag)) {
-					hashclr(sha1);
-					flag |= REF_ISBROKEN;
-				}
-			add_ref(ref, sha1, flag, array, NULL);
+			} else if (!resolve_ref(ref, sha1, 1, &flag)) {
+				hashclr(sha1);
+				flag |= REF_ISBROKEN;
+			}
+			add_ref(ref, sha1, flag, 1, array, NULL);
 		}
 		free(ref);
 		closedir(dir);
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index 895f559..c4c375a 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -146,4 +146,11 @@ test_expect_success 'cloning with reference being subset of source (-l -s)' \
 
 cd "$base_dir"
 
+test_expect_success 'clone with reference from a tagged repository' '
+	(
+		cd A && git tag -a -m 'tagged' HEAD
+	) &&
+	git clone --reference=A A I
+'
+
 test_done

Re: git clone --reference not working

[Michael Haggerty]

On 11/17/2011 01:54 AM, Junio C Hamano wrote:
> Andrea Arcangeli <aarcange@redhat.com> writes:
> 
>> latest git.git won't clone linux upstream with --reference. Those
>> v*^{} tags breaks it. What's that stuff anyway, looks totally ugly
>> (two commits with same data contents and header) bah.
> 
> They point at commits they tag, and are essential for auto-following. They
> have been there forever in ls-remote output and they are not the real
> problem.
> 
> A recent topic that was merged at 9bd5000 tightened the refname checking
> code without thinking and started to needlessly barf upon seeing them. I
> think we have discussed about the issue on the list, but I do not think
> there were fixes yet.
> 
> Thanks for reminding.
> 
> Michael, how does this look?
>
> -- >8 --
> Subject: refs: loosen over-strict "format" check
> [...]

I reviewed the patch (and ran the test suite here for good measure).
Looks good.

>From SubmittingPatches it looks like I should authorize

Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>

Is there a standard way to do so?

Michael

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

Re: git clone --reference not working

[Junio C Hamano]

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

> On 11/17/2011 01:54 AM, Junio C Hamano wrote:
> ...
> Looks good.
>
>>From SubmittingPatches it looks like I should authorize
>
> Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>
>
> Is there a standard way to do so?

That's perfect. I asked for an extra set of eyeballs from somebody who is
familiar with the codepath that had a problem, and that person eyeballed
and found the change to be an appropriate fix to the problem.

Either Acked-by or Reviewed-by is fine by me.

As a tentative measure, for tonight's pushout, I am inclined to queue an
equivalent of this patch on top of both mh/ref-api-2 and mh/ref-api-3
topic and merge them to 'next' and 'pu'. I'd appreciate if you can double
check the two merges on master..pu after I push them out in a few hours.

Thanks.

Re: git clone --reference not working

[Michael Haggerty]

On 11/17/2011 06:55 AM, Junio C Hamano wrote:
> As a tentative measure, for tonight's pushout, I am inclined to queue an
> equivalent of this patch on top of both mh/ref-api-2 and mh/ref-api-3
> topic and merge them to 'next' and 'pu'. I'd appreciate if you can double
> check the two merges on master..pu after I push them out in a few hours.

I checked the merges in the following (but didn't run the test suite, as
I assume that you have already done that):

   bc1bbe0..09116a1  master     -> gitster/master
   973592c..ada4ec6  mh/ref-api-2 -> gitster/mh/ref-api-2
   caa8069..37817ba  mh/ref-api-3 -> gitster/mh/ref-api-3
   25e8838..cc76151  next       -> gitster/next
 + 06ad567...b032ac4 pu         -> gitster/pu  (forced update)

They all look fine.

Michael

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

Re: git clone --reference not working

[Junio C Hamano]

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

> On 11/17/2011 06:55 AM, Junio C Hamano wrote:
>> As a tentative measure, for tonight's pushout, I am inclined to queue an
>> equivalent of this patch on top of both mh/ref-api-2 and mh/ref-api-3
>> topic and merge them to 'next' and 'pu'. I'd appreciate if you can double
>> check the two merges on master..pu after I push them out in a few hours.
>
> I checked the merges in the following...

Sorry, what I meant was eyeballing these two merges

  aee9699 Merge branch 'mh/ref-api-3' into jch
  9f8b195 Merge branch 'mh/ref-api-2' into jch

in "git log --oneline --first-parent master..pu" to see if I screwed them
up.

Going forward, I think after 1.7.8 final is tagged, when rewinding and
rebuilding the 'next' branch, it would be ideal to rebase these two topics
(actually, -3 builds on top of -2, so rebasing only the lower one should
be sufficient) on top of 1.7.8 that will include the fix in the patch that
started this thread.