* [PATCH v2 1/4] avoid segfaults on parse_object failure
2013-03-17 8:21 ` [PATCH v2 0/4] peel-ref optimization fixes Jeff King
@ 2013-03-17 8:22 ` Jeff King
2013-03-17 8:23 ` [PATCH v2 2/4] use parse_object_or_die instead of die("bad object") Jeff King
` (2 subsequent siblings)
3 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2013-03-17 8:22 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael Haggerty
Many call-sites of parse_object assume that they will get a
non-NULL return value; this is not the case if we encounter
an error while parsing the object.
This patch adds a wrapper function around parse_object that
handles dying automatically, and uses it anywhere we
immediately try to access the return value as a non-NULL
pointer (i.e., anywhere that we would currently segfault).
This wrapper may also be useful in other places. The most
obvious one is code like:
o = parse_object(sha1);
if (!o)
die(...);
However, these should not be mechanically converted to
parse_object_or_die, as the die message is sometimes
customized. Later patches can address these sites on a
case-by-case basis.
Signed-off-by: Jeff King <peff@peff.net>
---
bundle.c | 6 +++---
object.c | 10 ++++++++++
object.h | 13 ++++++++++++-
pack-refs.c | 2 +-
4 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/bundle.c b/bundle.c
index 8d12816..26ceebd 100644
--- a/bundle.c
+++ b/bundle.c
@@ -279,12 +279,12 @@ int create_bundle(struct bundle_header *header, const char *path,
if (buf.len > 0 && buf.buf[0] == '-') {
write_or_die(bundle_fd, buf.buf, buf.len);
if (!get_sha1_hex(buf.buf + 1, sha1)) {
- struct object *object = parse_object(sha1);
+ struct object *object = parse_object_or_die(sha1, buf.buf);
object->flags |= UNINTERESTING;
add_pending_object(&revs, object, xstrdup(buf.buf));
}
} else if (!get_sha1_hex(buf.buf, sha1)) {
- struct object *object = parse_object(sha1);
+ struct object *object = parse_object_or_die(sha1, buf.buf);
object->flags |= SHOWN;
}
}
@@ -361,7 +361,7 @@ int create_bundle(struct bundle_header *header, const char *path,
* end up triggering "empty bundle"
* error.
*/
- obj = parse_object(sha1);
+ obj = parse_object_or_die(sha1, e->name);
obj->flags |= SHOWN;
add_pending_object(&revs, obj, e->name);
}
diff --git a/object.c b/object.c
index 4af3451..20703f5 100644
--- a/object.c
+++ b/object.c
@@ -185,6 +185,16 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
return obj;
}
+struct object *parse_object_or_die(const unsigned char *sha1,
+ const char *name)
+{
+ struct object *o = parse_object(sha1);
+ if (o)
+ return o;
+
+ die(_("unable to parse object: %s"), name ? name : sha1_to_hex(sha1));
+}
+
struct object *parse_object(const unsigned char *sha1)
{
unsigned long size;
diff --git a/object.h b/object.h
index 6a97b6b..97d384b 100644
--- a/object.h
+++ b/object.h
@@ -54,9 +54,20 @@ struct object *parse_object(const unsigned char *sha1);
extern void *create_object(const unsigned char *sha1, int type, void *obj);
-/** Returns the object, having parsed it to find out what it is. **/
+/*
+ * Returns the object, having parsed it to find out what it is.
+ *
+ * Returns NULL if the object is missing or corrupt.
+ */
struct object *parse_object(const unsigned char *sha1);
+/*
+ * Like parse_object, but will die() instead of returning NULL. If the
+ * "name" parameter is not NULL, it is included in the error message
+ * (otherwise, the sha1 hex is given).
+ */
+struct object *parse_object_or_die(const unsigned char *sha1, const char *name);
+
/* Given the result of read_sha1_file(), returns the object after
* parsing it. eaten_p indicates if the object has a borrowed copy
* of buffer and the caller should not free() it.
diff --git a/pack-refs.c b/pack-refs.c
index f09a054..6a689f3 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -40,7 +40,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
fprintf(cb->refs_file, "%s %s\n", sha1_to_hex(sha1), path);
if (is_tag_ref) {
- struct object *o = parse_object(sha1);
+ struct object *o = parse_object_or_die(sha1, path);
if (o->type == OBJ_TAG) {
o = deref_tag(o, path, 0);
if (o)
--
1.8.2.rc2.7.gef06216
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 2/4] use parse_object_or_die instead of die("bad object")
2013-03-17 8:21 ` [PATCH v2 0/4] peel-ref optimization fixes Jeff King
2013-03-17 8:22 ` [PATCH v2 1/4] avoid segfaults on parse_object failure Jeff King
@ 2013-03-17 8:23 ` Jeff King
2013-03-17 8:23 ` [PATCH v2 3/4] pack-refs: write peeled entry for non-tags Jeff King
2013-03-17 8:28 ` [PATCH v2 4/4] pack-refs: add fully-peeled trait Jeff King
3 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2013-03-17 8:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael Haggerty
Some call-sites do:
o = parse_object(sha1);
if (!o)
die("bad object %s", some_name);
We can now handle that as a one-liner, and get more
consistent output.
In the third case of this patch, it looks like we are losing
information, as the existing message also outputs the sha1
hex; however, parse_object will already have written a more
specific complaint about the sha1, so there is no point in
repeating it here.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/grep.c | 4 +---
builtin/prune.c | 4 +---
reachable.c | 4 +---
3 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 8025964..159e65d 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -820,9 +820,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
unsigned char sha1[20];
/* Is it a rev? */
if (!get_sha1(arg, sha1)) {
- struct object *object = parse_object(sha1);
- if (!object)
- die(_("bad object %s"), arg);
+ struct object *object = parse_object_or_die(sha1, arg);
if (!seen_dashdash)
verify_non_filename(prefix, arg);
add_object_array(object, arg, &list);
diff --git a/builtin/prune.c b/builtin/prune.c
index 8cb8b91..85843d4 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -149,9 +149,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
const char *name = *argv++;
if (!get_sha1(name, sha1)) {
- struct object *object = parse_object(sha1);
- if (!object)
- die("bad object: %s", name);
+ struct object *object = parse_object_or_die(sha1, name);
add_pending_object(&revs, object, "");
}
else
diff --git a/reachable.c b/reachable.c
index bf79706..e7e6a1e 100644
--- a/reachable.c
+++ b/reachable.c
@@ -152,11 +152,9 @@ static int add_one_ref(const char *path, const unsigned char *sha1, int flag, vo
static int add_one_ref(const char *path, const unsigned char *sha1, int flag, void *cb_data)
{
- struct object *object = parse_object(sha1);
+ struct object *object = parse_object_or_die(sha1, path);
struct rev_info *revs = (struct rev_info *)cb_data;
- if (!object)
- die("bad object ref: %s:%s", path, sha1_to_hex(sha1));
add_pending_object(revs, object, "");
return 0;
--
1.8.2.rc2.7.gef06216
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 3/4] pack-refs: write peeled entry for non-tags
2013-03-17 8:21 ` [PATCH v2 0/4] peel-ref optimization fixes Jeff King
2013-03-17 8:22 ` [PATCH v2 1/4] avoid segfaults on parse_object failure Jeff King
2013-03-17 8:23 ` [PATCH v2 2/4] use parse_object_or_die instead of die("bad object") Jeff King
@ 2013-03-17 8:23 ` Jeff King
2013-03-17 8:28 ` [PATCH v2 4/4] pack-refs: add fully-peeled trait Jeff King
3 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2013-03-17 8:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael Haggerty
When we pack an annotated tag ref, we write not only the
sha1 of the tag object along with the ref, but also the sha1
obtained by peeling the tag. This lets readers of the
pack-refs file know the peeled value without having to
actually load the object, speeding up upload-pack's ref
advertisement.
The writer marks a packed-refs file with peeled refs using
the "peeled" trait at the top of the file. When the reader
sees this trait, it knows that each ref is either followed
by its peeled value, or it is not an annotated tag.
However, there is a mismatch between the assumptions of the
reader and writer. The writer will only peel refs under
refs/tags, but the reader does not know this; it will assume
a ref without a peeled value must not be a tag object. Thus
an annotated tag object placed outside of the refs/tags
hierarchy will not have its peeled value printed by
upload-pack.
The simplest way to fix this is to start writing peel values
for all refs. This matches what the reader expects for both
new and old versions of git.
Signed-off-by: Jeff King <peff@peff.net>
---
pack-refs.c | 16 ++++++++--------
t/t3211-peel-ref.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 50 insertions(+), 8 deletions(-)
create mode 100755 t/t3211-peel-ref.sh
diff --git a/pack-refs.c b/pack-refs.c
index 6a689f3..ebde785 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -27,6 +27,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
int flags, void *cb_data)
{
struct pack_refs_cb_data *cb = cb_data;
+ struct object *o;
int is_tag_ref;
/* Do not pack the symbolic refs */
@@ -39,14 +40,13 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
return 0;
fprintf(cb->refs_file, "%s %s\n", sha1_to_hex(sha1), path);
- if (is_tag_ref) {
- struct object *o = parse_object_or_die(sha1, path);
- if (o->type == OBJ_TAG) {
- o = deref_tag(o, path, 0);
- if (o)
- fprintf(cb->refs_file, "^%s\n",
- sha1_to_hex(o->sha1));
- }
+
+ o = parse_object_or_die(sha1, path);
+ if (o->type == OBJ_TAG) {
+ o = deref_tag(o, path, 0);
+ if (o)
+ fprintf(cb->refs_file, "^%s\n",
+ sha1_to_hex(o->sha1));
}
if ((cb->flags & PACK_REFS_PRUNE) && !do_not_prune(flags)) {
diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh
new file mode 100755
index 0000000..85f09be
--- /dev/null
+++ b/t/t3211-peel-ref.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+test_description='tests for the peel_ref optimization of packed-refs'
+. ./test-lib.sh
+
+test_expect_success 'create annotated tag in refs/tags' '
+ test_commit base &&
+ git tag -m annotated foo
+'
+
+test_expect_success 'create annotated tag outside of refs/tags' '
+ git update-ref refs/outside/foo refs/tags/foo
+'
+
+# This matches show-ref's output
+print_ref() {
+ echo "$(git rev-parse "$1") $1"
+}
+
+test_expect_success 'set up expected show-ref output' '
+ {
+ print_ref "refs/heads/master" &&
+ print_ref "refs/outside/foo" &&
+ print_ref "refs/outside/foo^{}" &&
+ print_ref "refs/tags/base" &&
+ print_ref "refs/tags/foo" &&
+ print_ref "refs/tags/foo^{}"
+ } >expect
+'
+
+test_expect_success 'refs are peeled outside of refs/tags (loose)' '
+ git show-ref -d >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'refs are peeled outside of refs/tags (packed)' '
+ git pack-refs --all &&
+ git show-ref -d >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
1.8.2.rc2.7.gef06216
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 4/4] pack-refs: add fully-peeled trait
2013-03-17 8:21 ` [PATCH v2 0/4] peel-ref optimization fixes Jeff King
` (2 preceding siblings ...)
2013-03-17 8:23 ` [PATCH v2 3/4] pack-refs: write peeled entry for non-tags Jeff King
@ 2013-03-17 8:28 ` Jeff King
2013-03-17 20:01 ` Junio C Hamano
2013-03-18 3:12 ` [PATCH v2 " Michael Haggerty
3 siblings, 2 replies; 19+ messages in thread
From: Jeff King @ 2013-03-17 8:28 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael Haggerty
From: Michael Haggerty <mhagger@alum.mit.edu>
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 <peff@peff.net>]
Signed-off-by: Jeff King <peff@peff.net>
---
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);
+ if (refs_tags_peeled && !prefixcmp(refname, "refs/tags/"))
+ last->flag |= REF_KNOWS_PEELED;
add_ref(dir, last);
continue;
}
@@ -832,8 +864,15 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
refline[0] == '^' &&
strlen(refline) == 42 &&
refline[41] == '\n' &&
- !get_sha1_hex(refline + 1, sha1))
+ !get_sha1_hex(refline + 1, sha1)) {
hashcpy(last->u.value.peeled, sha1);
+ /*
+ * Regardless of what the file header said,
+ * we definitely know the value of *this*
+ * reference:
+ */
+ last->flag |= REF_KNOWS_PEELED;
+ }
}
}
diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh
index 85f09be..d4d7792 100755
--- a/t/t3211-peel-ref.sh
+++ b/t/t3211-peel-ref.sh
@@ -39,4 +39,26 @@ test_expect_success 'refs are peeled outside of refs/tags (packed)' '
test_cmp expect actual
'
+test_expect_success 'create old-style pack-refs without fully-peeled' '
+ # Git no longer writes without fully-peeled, so we just write our own
+ # from scratch; we could also munge the existing file to remove the
+ # fully-peeled bits, but that seems even more prone to failure,
+ # especially if the format ever changes again. At least this way we
+ # know we are emulating exactly what an older git would have written.
+ {
+ echo "# pack-refs with: peeled " &&
+ print_ref "refs/heads/master" &&
+ print_ref "refs/outside/foo" &&
+ print_ref "refs/tags/base" &&
+ print_ref "refs/tags/foo" &&
+ echo "^$(git rev-parse "refs/tags/foo^{}")"
+ } >tmp &&
+ mv tmp .git/packed-refs
+'
+
+test_expect_success 'refs are peeled outside of refs/tags (old packed)' '
+ git show-ref -d >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.8.2.rc2.7.gef06216
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 4/4] pack-refs: add fully-peeled trait
2013-03-17 8:28 ` [PATCH v2 4/4] pack-refs: add fully-peeled trait Jeff King
@ 2013-03-17 20:01 ` Junio C Hamano
2013-03-18 11:37 ` [PATCH v3 " Jeff King
2013-03-18 3:12 ` [PATCH v2 " Michael Haggerty
1 sibling, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2013-03-17 20:01 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty
Jeff King <peff@peff.net> writes:
> From: Michael Haggerty <mhagger@alum.mit.edu>
>
> 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 <peff@peff.net>]
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> 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/"))
> + last->flag |= REF_KNOWS_PEELED;
I am not sure why you find this any more readable.
The "flag" is set earlier to contain REF_KNOWS_PEELED only when we
have fully-peeled trait, and peeled trait is recorded as a separate
local variable. The fully-peeled case sets the flag by passing the
flag to create_ref_entry() but the peeled case adds it to last->flag
manually after the fact.
If you set two local variables when you read the traits (iow, no
futzing with "flag" there), this part would become either:
last = create_ref_entry(refname, sha1, REF_ISPACKED, 1);
if (refs_fully_peeled ||
(refs_tags_peeled && !prefixcmp(refname, "refs/tags/")))
last->flag |= REF_KNOWS_PEELED;
or
flag = REF_ISPACKED;
if (refs_fully_peeled ||
(refs_tags_peeled && !prefixcmp(refname, "refs/tags/")))
flag |= REF_KNOWS_PEELED;
last = create_ref_entry(refname, sha1, flag, 1);
either of which would be much more readable at least to me.
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v3 4/4] pack-refs: add fully-peeled trait
2013-03-17 20:01 ` Junio C Hamano
@ 2013-03-18 11:37 ` Jeff King
2013-03-18 16:26 ` Junio C Hamano
0 siblings, 1 reply; 19+ messages in thread
From: Jeff King @ 2013-03-18 11:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
On Sun, Mar 17, 2013 at 01:01:44PM -0700, Junio C Hamano wrote:
> > + * 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.
Thanks, will fix (the whitespace damage is due to me cutting and pasting
from Michael's commit).
>
> > + * file for which no peeled value is recorded is not peelable. This
> > + * trait should typically be written alongside "fully-peeled" for
>
> Alongside "peeled", no?
Urgh, yes. Will fix.
> [...]
> I am not sure why you find this any more readable.
I was trying to avoid the "set PEELED globally, but sometimes unset it
for specific refs" pattern which I think is confusing to the reader. But
I think what you wrote is even better. I used an enum rather than two
variables to make it clear that only ones takes effect. I had wanted to
use a switch, also, but you end up either repeating yourself, or doing
this gross fall-through:
switch (peeled) {
case PEELED_TAGS:
if (prefixcmp(refname, "refs/tags/"))
break;
/* fall-through */
case PEELED_FULLY:
last->ref |= REF_KNOWS_PEELED;
break;
default:
/* we know nothing */
}
So I just stuck with the conditional.
Here's the re-roll.
-- >8 --
From: Michael Haggerty <mhagger@alum.mit.edu>
Subject: [PATCH] pack-refs: add fully-peeled trait
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 <peff@peff.net>]
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Jeff King <peff@peff.net>
---
pack-refs.c | 2 +-
refs.c | 49 ++++++++++++++++++++++++++++++++++++++++++++-----
t/t3211-peel-ref.sh | 22 ++++++++++++++++++++++
3 files changed, 67 insertions(+), 6 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..e2b760d 100644
--- a/refs.c
+++ b/refs.c
@@ -803,11 +803,38 @@ 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 "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;
+ enum { PEELED_NONE, PEELED_TAGS, PEELED_FULLY } peeled = PEELED_NONE;
while (fgets(refline, sizeof(refline), f)) {
unsigned char sha1[20];
@@ -816,15 +843,20 @@ 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 "))
- flag |= REF_KNOWS_PEELED;
+ if (strstr(traits, " fully-peeled "))
+ peeled = PEELED_FULLY;
+ else if (strstr(traits, " peeled "))
+ peeled = PEELED_TAGS;
/* perhaps other traits later as well */
continue;
}
refname = parse_ref_line(refline, sha1);
if (refname) {
- last = create_ref_entry(refname, sha1, flag, 1);
+ last = create_ref_entry(refname, sha1, REF_ISPACKED, 1);
+ if (peeled == PEELED_FULLY ||
+ (peeled == PEELED_TAGS && !prefixcmp(refname, "refs/tags/")))
+ last->flag |= REF_KNOWS_PEELED;
add_ref(dir, last);
continue;
}
@@ -832,8 +864,15 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
refline[0] == '^' &&
strlen(refline) == 42 &&
refline[41] == '\n' &&
- !get_sha1_hex(refline + 1, sha1))
+ !get_sha1_hex(refline + 1, sha1)) {
hashcpy(last->u.value.peeled, sha1);
+ /*
+ * Regardless of what the file header said,
+ * we definitely know the value of *this*
+ * reference:
+ */
+ last->flag |= REF_KNOWS_PEELED;
+ }
}
}
diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh
index 85f09be..d4d7792 100755
--- a/t/t3211-peel-ref.sh
+++ b/t/t3211-peel-ref.sh
@@ -39,4 +39,26 @@ test_expect_success 'refs are peeled outside of refs/tags (packed)' '
test_cmp expect actual
'
+test_expect_success 'create old-style pack-refs without fully-peeled' '
+ # Git no longer writes without fully-peeled, so we just write our own
+ # from scratch; we could also munge the existing file to remove the
+ # fully-peeled bits, but that seems even more prone to failure,
+ # especially if the format ever changes again. At least this way we
+ # know we are emulating exactly what an older git would have written.
+ {
+ echo "# pack-refs with: peeled " &&
+ print_ref "refs/heads/master" &&
+ print_ref "refs/outside/foo" &&
+ print_ref "refs/tags/base" &&
+ print_ref "refs/tags/foo" &&
+ echo "^$(git rev-parse "refs/tags/foo^{}")"
+ } >tmp &&
+ mv tmp .git/packed-refs
+'
+
+test_expect_success 'refs are peeled outside of refs/tags (old packed)' '
+ git show-ref -d >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.8.2.rc2.7.gef06216
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/4] pack-refs: add fully-peeled trait
2013-03-17 8:28 ` [PATCH v2 4/4] pack-refs: add fully-peeled trait Jeff King
2013-03-17 20:01 ` Junio C Hamano
@ 2013-03-18 3:12 ` Michael Haggerty
2013-03-18 15:12 ` Junio C Hamano
1 sibling, 1 reply; 19+ messages in thread
From: Michael Haggerty @ 2013-03-18 3:12 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
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 <mhagger@alum.mit.edu>
>
> 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 <peff@peff.net>]
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> 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);
> + if (refs_tags_peeled && !prefixcmp(refname, "refs/tags/"))
> + last->flag |= REF_KNOWS_PEELED;
> add_ref(dir, last);
> continue;
> }
> @@ -832,8 +864,15 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
> refline[0] == '^' &&
> strlen(refline) == 42 &&
> refline[41] == '\n' &&
> - !get_sha1_hex(refline + 1, sha1))
> + !get_sha1_hex(refline + 1, sha1)) {
> hashcpy(last->u.value.peeled, sha1);
> + /*
> + * Regardless of what the file header said,
> + * we definitely know the value of *this*
> + * reference:
> + */
> + last->flag |= REF_KNOWS_PEELED;
> + }
> }
> }
>
> diff --git a/t/t3211-peel-ref.sh b/t/t3211-peel-ref.sh
> index 85f09be..d4d7792 100755
> --- a/t/t3211-peel-ref.sh
> +++ b/t/t3211-peel-ref.sh
> @@ -39,4 +39,26 @@ test_expect_success 'refs are peeled outside of refs/tags (packed)' '
> test_cmp expect actual
> '
>
> +test_expect_success 'create old-style pack-refs without fully-peeled' '
> + # Git no longer writes without fully-peeled, so we just write our own
> + # from scratch; we could also munge the existing file to remove the
> + # fully-peeled bits, but that seems even more prone to failure,
> + # especially if the format ever changes again. At least this way we
> + # know we are emulating exactly what an older git would have written.
> + {
> + echo "# pack-refs with: peeled " &&
> + print_ref "refs/heads/master" &&
> + print_ref "refs/outside/foo" &&
> + print_ref "refs/tags/base" &&
> + print_ref "refs/tags/foo" &&
> + echo "^$(git rev-parse "refs/tags/foo^{}")"
> + } >tmp &&
> + mv tmp .git/packed-refs
> +'
> +
> +test_expect_success 'refs are peeled outside of refs/tags (old packed)' '
> + git show-ref -d >actual &&
> + test_cmp expect actual
> +'
> +
> test_done
>
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 4/4] pack-refs: add fully-peeled trait
2013-03-18 3:12 ` [PATCH v2 " Michael Haggerty
@ 2013-03-18 15:12 ` Junio C Hamano
0 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2013-03-18 15:12 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Jeff King, git
Michael Haggerty <mhagger@alum.mit.edu> writes:
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>
> 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???".
^ permalink raw reply [flat|nested] 19+ messages in thread