Re: [PATCH v2 4/4] pack-refs: add fully-peeled trait
Michael Haggerty writes: > Signed-off-by: Michael Haggerty > > and ACK for the whole series, once Junio's points are addressed. > > Regarding Junio's readability suggestion: I agree that his versions are > a bit more readable, albeit at the expense of having to evaluate a bit > more logic for each reference rather than just once when the header line > is handled. So I don't have a preference either way. The way the conditional is written, in the longer term we will almost always compare "peeled == PEELED_FULLY", and otherwise we will do the same !prefixcmp(refs/tags/), so I do not think there is "more logic" that matters compared to the original. Thanks, both; will replace what was queued with "SQUASH???". -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] pack-refs: add fully-peeled trait
Signed-off-by: Michael Haggerty and ACK for the whole series, once Junio's points are addressed. Regarding Junio's readability suggestion: I agree that his versions are a bit more readable, albeit at the expense of having to evaluate a bit more logic for each reference rather than just once when the header line is handled. So I don't have a preference either way. Michael On 03/17/2013 09:28 AM, Jeff King wrote: > From: Michael Haggerty > > Older versions of pack-refs did not write peel lines for > refs outside of refs/tags. This meant that on reading the > pack-refs file, we might set the REF_KNOWS_PEELED flag for > such a ref, even though we do not know anything about its > peeled value. > > The previous commit updated the writer to always peel, no > matter what the ref is. That means that packed-refs files > written by newer versions of git are fine to be read by both > old and new versions of git. However, we still have the > problem of reading packed-refs files written by older > versions of git, or by other implementations which have not > yet learned the same trick. > > The simplest fix would be to always unset the > REF_KNOWS_PEELED flag for refs outside of refs/tags that do > not have a peel line (if it has a peel line, we know it is > valid, but we cannot assume a missing peel line means > anything). But that loses an important optimization, as > upload-pack should not need to load the object pointed to by > refs/heads/foo to determine that it is not a tag. > > Instead, we add a "fully-peeled" trait to the packed-refs > file. If it is set, we know that we can trust a missing peel > line to mean that a ref cannot be peeled. Otherwise, we fall > back to assuming nothing. > > [commit message and tests by Jeff King ] > > Signed-off-by: Jeff King > --- > This uses Michael's approach for managing the flags within > read_packed_refs, which is more readable. As I picked up his > code and comments, I realized that there was basically > nothing of mine left, so I switched the authorship. But do > note: > > 1. It should have Michael's signoff, which was not present > in the commit I lifted the code from. > > 2. I tweaked the big comment above read_packed_refs to > reduce some ambiguities. Please double-check that I am > not putting inaccurate words in your mouth. :) > > pack-refs.c | 2 +- > refs.c | 43 +-- > t/t3211-peel-ref.sh | 22 ++ > 3 files changed, 64 insertions(+), 3 deletions(-) > > diff --git a/pack-refs.c b/pack-refs.c > index ebde785..4461f71 100644 > --- a/pack-refs.c > +++ b/pack-refs.c > @@ -128,7 +128,7 @@ int pack_refs(unsigned int flags) > die_errno("unable to create ref-pack file structure"); > > /* perhaps other traits later as well */ > - fprintf(cbdata.refs_file, "# pack-refs with: peeled \n"); > + fprintf(cbdata.refs_file, "# pack-refs with: peeled fully-peeled \n"); > > for_each_ref(handle_one_ref, &cbdata); > if (ferror(cbdata.refs_file)) > diff --git a/refs.c b/refs.c > index 175b9fc..bdeac28 100644 > --- a/refs.c > +++ b/refs.c > @@ -803,11 +803,39 @@ static void read_packed_refs(FILE *f, struct ref_dir > *dir) > return line; > } > > +/* > + * Read f, which is a packed-refs file, into dir. > + * > + * A comment line of the form "# pack-refs with: " may contain zero or > + * more traits. We interpret the traits as follows: > + * > + * No traits: > + * > + * Probably no references are peeled. But if the file contains a > + * peeled value for a reference, we will use it. > + * > + * peeled: > + * > + * References under "refs/tags/", if they *can* be peeled, *are* > + * peeled in this file. References outside of "refs/tags/" are > + * probably not peeled even if they could have been, but if we find > + * a peeled value for such a reference we will use it. > + * > + * fully-peeled: > + * > + * All references in the file that can be peeled are peeled. > + * Inversely (and this is more important, any references in the > + * file for which no peeled value is recorded is not peelable. This > + * trait should typically be written alongside "fully-peeled" for > + * compatibility with older clients, but we do not require it > + * (i.e., "peeled" is a no-op if "fully-peeled" is set). > + */ > static void read_packed_refs(FILE *f, struct ref_dir *dir) > { > struct ref_entry *last = NULL; > char refline[PATH_MAX]; > int flag = REF_ISPACKED; > + int refs_tags_peeled = 0; > > while (fgets(refline, sizeof(refline), f)) { > unsigned char sha1[20]; > @@ -816,8 +844,10 @@ static void read_packed_refs(FILE *f, struct ref_dir > *dir) > > if (!strncmp(refline, header, sizeof(header)-1)) { > const char *traits = refline + sizeof(header) - 1; > - if (strstr(traits, " peele
Re: [PATCH v2 4/4] pack-refs: add fully-peeled trait
Jeff King writes: > From: Michael Haggerty > > Older versions of pack-refs did not write peel lines for > refs outside of refs/tags. This meant that on reading the > pack-refs file, we might set the REF_KNOWS_PEELED flag for > such a ref, even though we do not know anything about its > peeled value. > > The previous commit updated the writer to always peel, no > matter what the ref is. That means that packed-refs files > written by newer versions of git are fine to be read by both > old and new versions of git. However, we still have the > problem of reading packed-refs files written by older > versions of git, or by other implementations which have not > yet learned the same trick. > > The simplest fix would be to always unset the > REF_KNOWS_PEELED flag for refs outside of refs/tags that do > not have a peel line (if it has a peel line, we know it is > valid, but we cannot assume a missing peel line means > anything). But that loses an important optimization, as > upload-pack should not need to load the object pointed to by > refs/heads/foo to determine that it is not a tag. > > Instead, we add a "fully-peeled" trait to the packed-refs > file. If it is set, we know that we can trust a missing peel > line to mean that a ref cannot be peeled. Otherwise, we fall > back to assuming nothing. > > [commit message and tests by Jeff King ] > > Signed-off-by: Jeff King > --- > This uses Michael's approach for managing the flags within > read_packed_refs, which is more readable. As I picked up his > code and comments, I realized that there was basically > nothing of mine left, so I switched the authorship. But do > note: > > 1. It should have Michael's signoff, which was not present > in the commit I lifted the code from. > > 2. I tweaked the big comment above read_packed_refs to > reduce some ambiguities. Please double-check that I am > not putting inaccurate words in your mouth. :) > > pack-refs.c | 2 +- > refs.c | 43 +-- > t/t3211-peel-ref.sh | 22 ++ > 3 files changed, 64 insertions(+), 3 deletions(-) > > diff --git a/pack-refs.c b/pack-refs.c > index ebde785..4461f71 100644 > --- a/pack-refs.c > +++ b/pack-refs.c > @@ -128,7 +128,7 @@ int pack_refs(unsigned int flags) > die_errno("unable to create ref-pack file structure"); > > /* perhaps other traits later as well */ > - fprintf(cbdata.refs_file, "# pack-refs with: peeled \n"); > + fprintf(cbdata.refs_file, "# pack-refs with: peeled fully-peeled \n"); > > for_each_ref(handle_one_ref, &cbdata); > if (ferror(cbdata.refs_file)) > diff --git a/refs.c b/refs.c > index 175b9fc..bdeac28 100644 > --- a/refs.c > +++ b/refs.c > @@ -803,11 +803,39 @@ static void read_packed_refs(FILE *f, struct ref_dir > *dir) > return line; > } > > +/* > + * Read f, which is a packed-refs file, into dir. > + * > + * A comment line of the form "# pack-refs with: " may contain zero or > + * more traits. We interpret the traits as follows: > + * > + * No traits: > + * > + * Probably no references are peeled. But if the file contains a > + * peeled value for a reference, we will use it. > + * > + * peeled: > + * > + * References under "refs/tags/", if they *can* be peeled, *are* > + * peeled in this file. References outside of "refs/tags/" are > + * probably not peeled even if they could have been, but if we find > + * a peeled value for such a reference we will use it. > + * > + * fully-peeled: > + * > + * All references in the file that can be peeled are peeled. > + * Inversely (and this is more important, any references in the A missing closing paren after "more important". Also the e-mail quote reveals there is some inconsistent indentation (HTs vs runs of SPs) here. > + * file for which no peeled value is recorded is not peelable. This > + * trait should typically be written alongside "fully-peeled" for Alongside "peeled", no? > @@ -816,8 +844,10 @@ static void read_packed_refs(FILE *f, struct ref_dir > *dir) > > if (!strncmp(refline, header, sizeof(header)-1)) { > const char *traits = refline + sizeof(header) - 1; > - if (strstr(traits, " peeled ")) > + if (strstr(traits, " fully-peeled ")) > flag |= REF_KNOWS_PEELED; > + else if (strstr(traits, " peeled ")) > + refs_tags_peeled = 1; > /* perhaps other traits later as well */ > continue; > } > @@ -825,6 +855,8 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) > refname = parse_ref_line(refline, sha1); > if (refname) { > last = create_ref_entry(refname, sha1, flag, 1); > + if (refs_tags_peeled && !prefixcmp(refname, > "refs/tags/
[PATCH v2 4/4] pack-refs: add fully-peeled trait
From: Michael Haggerty Older versions of pack-refs did not write peel lines for refs outside of refs/tags. This meant that on reading the pack-refs file, we might set the REF_KNOWS_PEELED flag for such a ref, even though we do not know anything about its peeled value. The previous commit updated the writer to always peel, no matter what the ref is. That means that packed-refs files written by newer versions of git are fine to be read by both old and new versions of git. However, we still have the problem of reading packed-refs files written by older versions of git, or by other implementations which have not yet learned the same trick. The simplest fix would be to always unset the REF_KNOWS_PEELED flag for refs outside of refs/tags that do not have a peel line (if it has a peel line, we know it is valid, but we cannot assume a missing peel line means anything). But that loses an important optimization, as upload-pack should not need to load the object pointed to by refs/heads/foo to determine that it is not a tag. Instead, we add a "fully-peeled" trait to the packed-refs file. If it is set, we know that we can trust a missing peel line to mean that a ref cannot be peeled. Otherwise, we fall back to assuming nothing. [commit message and tests by Jeff King ] Signed-off-by: Jeff King --- This uses Michael's approach for managing the flags within read_packed_refs, which is more readable. As I picked up his code and comments, I realized that there was basically nothing of mine left, so I switched the authorship. But do note: 1. It should have Michael's signoff, which was not present in the commit I lifted the code from. 2. I tweaked the big comment above read_packed_refs to reduce some ambiguities. Please double-check that I am not putting inaccurate words in your mouth. :) pack-refs.c | 2 +- refs.c | 43 +-- t/t3211-peel-ref.sh | 22 ++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/pack-refs.c b/pack-refs.c index ebde785..4461f71 100644 --- a/pack-refs.c +++ b/pack-refs.c @@ -128,7 +128,7 @@ int pack_refs(unsigned int flags) die_errno("unable to create ref-pack file structure"); /* perhaps other traits later as well */ - fprintf(cbdata.refs_file, "# pack-refs with: peeled \n"); + fprintf(cbdata.refs_file, "# pack-refs with: peeled fully-peeled \n"); for_each_ref(handle_one_ref, &cbdata); if (ferror(cbdata.refs_file)) diff --git a/refs.c b/refs.c index 175b9fc..bdeac28 100644 --- a/refs.c +++ b/refs.c @@ -803,11 +803,39 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) return line; } +/* + * Read f, which is a packed-refs file, into dir. + * + * A comment line of the form "# pack-refs with: " may contain zero or + * more traits. We interpret the traits as follows: + * + * No traits: + * + * Probably no references are peeled. But if the file contains a + * peeled value for a reference, we will use it. + * + * peeled: + * + * References under "refs/tags/", if they *can* be peeled, *are* + * peeled in this file. References outside of "refs/tags/" are + * probably not peeled even if they could have been, but if we find + * a peeled value for such a reference we will use it. + * + * fully-peeled: + * + * All references in the file that can be peeled are peeled. + * Inversely (and this is more important, any references in the + * file for which no peeled value is recorded is not peelable. This + * trait should typically be written alongside "fully-peeled" for + * compatibility with older clients, but we do not require it + * (i.e., "peeled" is a no-op if "fully-peeled" is set). + */ static void read_packed_refs(FILE *f, struct ref_dir *dir) { struct ref_entry *last = NULL; char refline[PATH_MAX]; int flag = REF_ISPACKED; + int refs_tags_peeled = 0; while (fgets(refline, sizeof(refline), f)) { unsigned char sha1[20]; @@ -816,8 +844,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) if (!strncmp(refline, header, sizeof(header)-1)) { const char *traits = refline + sizeof(header) - 1; - if (strstr(traits, " peeled ")) + if (strstr(traits, " fully-peeled ")) flag |= REF_KNOWS_PEELED; + else if (strstr(traits, " peeled ")) + refs_tags_peeled = 1; /* perhaps other traits later as well */ continue; } @@ -825,6 +855,8 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) refname = parse_ref_line(refline, sha1); if (refname) { last = create_ref_entry(refname, sha1, flag, 1); +