* [PATCH 1/2] fast-export: rename handle_object function
From: Jeff King @ 2013-03-17 8:33 UTC (permalink / raw)
To: git
In-Reply-To: <20130317083235.GA29907@sigill.intra.peff.net>
The handle_object function is rather vaguely named; it only
operates on blobs, and its purpose is to export the blob to
the output stream. Let's call it "export_blob" to make it
more clear what it does.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fast-export.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 77dffd1..3eba852 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -113,7 +113,7 @@ static void show_progress(void)
printf("progress %d objects\n", counter);
}
-static void handle_object(const unsigned char *sha1)
+static void export_blob(const unsigned char *sha1)
{
unsigned long size;
enum object_type type;
@@ -312,7 +312,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)
/* Export the referenced blobs, and remember the marks. */
for (i = 0; i < diff_queued_diff.nr; i++)
if (!S_ISGITLINK(diff_queued_diff.queue[i]->two->mode))
- handle_object(diff_queued_diff.queue[i]->two->sha1);
+ export_blob(diff_queued_diff.queue[i]->two->sha1);
mark_next_object(&commit->object);
if (!is_encoding_utf8(encoding))
@@ -512,7 +512,7 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info,
commit = (struct commit *)tag;
break;
case OBJ_BLOB:
- handle_object(tag->object.sha1);
+ export_blob(tag->object.sha1);
continue;
default: /* OBJ_TAG (nested tags) is already handled */
warning("Tag points to object of unexpected type %s, skipping.",
--
1.8.2.rc2.7.gef06216
^ permalink raw reply related
* [PATCH 0/2] minor fast-export speedup
From: Jeff King @ 2013-03-17 8:32 UTC (permalink / raw)
To: git
While grepping through all of the calls to parse_object (to see how they
handled error conditions, for the other series I just posted), I noticed
this opportunity for a small speedup in fast-export (5-15%). The first
patch is a cleanup, the second is the interesting bit.
[1/2]: fast-export: rename handle_object function
[2/2]: fast-export: do not load blob objects twice
A useful third patch on top might be to stream blobs out rather than
load them into memory, but I didn't want to go there tonight.
-Peff
^ permalink raw reply
* [PATCH v2 4/4] pack-refs: add fully-peeled trait
From: Jeff King @ 2013-03-17 8:28 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael Haggerty
In-Reply-To: <20130317082139.GA29505@sigill.intra.peff.net>
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
* [PATCH v2 3/4] pack-refs: write peeled entry for non-tags
From: Jeff King @ 2013-03-17 8:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael Haggerty
In-Reply-To: <20130317082139.GA29505@sigill.intra.peff.net>
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
* [PATCH v2 2/4] use parse_object_or_die instead of die("bad object")
From: Jeff King @ 2013-03-17 8:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael Haggerty
In-Reply-To: <20130317082139.GA29505@sigill.intra.peff.net>
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
* [PATCH v2 1/4] avoid segfaults on parse_object failure
From: Jeff King @ 2013-03-17 8:22 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael Haggerty
In-Reply-To: <20130317082139.GA29505@sigill.intra.peff.net>
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
* [PATCH v2 0/4] peel-ref optimization fixes
From: Jeff King @ 2013-03-17 8:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Michael Haggerty
In-Reply-To: <20130316090018.GA26708@sigill.intra.peff.net>
Here's a re-roll that takes into account the feedback from round 1:
[1/4]: avoid segfaults on parse_object failure
[2/4]: use parse_object_or_die instead of die("bad object")
These two patches are new; they are conceptually independent of the rest
of the series, but there's a textual dependency in later patches.
[3/4]: pack-refs: write peeled entry for non-tags
Same as before, but rebased on patch 1, and s/``/$()/.
[4/4]: pack-refs: add fully-peeled trait
Rewritten using Michael's approach, which is more readable.
-Peff
^ permalink raw reply
* Re: [PATCH] sha1_name: pass object name length to diagnose_invalid_sha1_path()
From: Junio C Hamano @ 2013-03-17 7:10 UTC (permalink / raw)
To: René Scharfe; +Cc: git discussion list, Matthieu Moy
In-Reply-To: <5144BA0B.2040109@lsrfire.ath.cx>
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> The only caller of diagnose_invalid_sha1_path() extracts a substring from
> an object name by creating a NUL-terminated copy of the interesting part.
> Add a length parameter to the function and thus avoid the need for an
> allocation, thereby simplifying the code.
>
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
> ---
> sha1_name.c | 32 ++++++++++++++------------------
> 1 file changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 95003c7..4cea6d3 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -1137,7 +1137,8 @@ int get_sha1_blob(const char *name, unsigned char *sha1)
> static void diagnose_invalid_sha1_path(const char *prefix,
> const char *filename,
> const unsigned char *tree_sha1,
> - const char *object_name)
> + const char *object_name,
> + int object_name_len)
> {
> struct stat st;
> unsigned char sha1[20];
> @@ -1147,8 +1148,8 @@ static void diagnose_invalid_sha1_path(const char *prefix,
> prefix = "";
>
> if (!lstat(filename, &st))
> - die("Path '%s' exists on disk, but not in '%s'.",
> - filename, object_name);
> + die("Path '%s' exists on disk, but not in '%.*s'.",
> + filename, object_name_len, object_name);
> if (errno == ENOENT || errno == ENOTDIR) {
> char *fullname = xmalloc(strlen(filename)
> + strlen(prefix) + 1);
> @@ -1158,16 +1159,16 @@ static void diagnose_invalid_sha1_path(const char *prefix,
> if (!get_tree_entry(tree_sha1, fullname,
> sha1, &mode)) {
> die("Path '%s' exists, but not '%s'.\n"
> - "Did you mean '%s:%s' aka '%s:./%s'?",
> + "Did you mean '%.*s:%s' aka '.*%.*s:./%s'?",
This is so unlike what I call "Scharfe patch", which I can apply
with my eyes closed and expect everything to be perfect.
Other than that, I see this as a usual "Scharfe patch" ;-) Will
squash an obvious fix in and apply.
Thanks.
> fullname,
> filename,
> - object_name,
> + object_name_len, object_name,
> fullname,
> - object_name,
> + object_name_len, object_name,
> filename);
> }
> - die("Path '%s' does not exist in '%s'",
> - filename, object_name);
> + die("Path '%s' does not exist in '%.*s'",
> + filename, object_name_len, object_name);
> }
> }
>
> @@ -1332,13 +1333,8 @@ static int get_sha1_with_context_1(const char *name,
> }
> if (*cp == ':') {
> unsigned char tree_sha1[20];
> - char *object_name = NULL;
> - if (only_to_die) {
> - object_name = xmalloc(cp-name+1);
> - strncpy(object_name, name, cp-name);
> - object_name[cp-name] = '\0';
> - }
> - if (!get_sha1_1(name, cp-name, tree_sha1, GET_SHA1_TREEISH)) {
> + int len = cp - name;
> + if (!get_sha1_1(name, len, tree_sha1, GET_SHA1_TREEISH)) {
> const char *filename = cp+1;
> char *new_filename = NULL;
>
> @@ -1348,8 +1344,8 @@ static int get_sha1_with_context_1(const char *name,
> ret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);
> if (ret && only_to_die) {
> diagnose_invalid_sha1_path(prefix, filename,
> - tree_sha1, object_name);
> - free(object_name);
> + tree_sha1,
> + name, len);
> }
> hashcpy(oc->tree, tree_sha1);
> strncpy(oc->path, filename,
> @@ -1360,7 +1356,7 @@ static int get_sha1_with_context_1(const char *name,
> return ret;
> } else {
> if (only_to_die)
> - die("Invalid object name '%s'.", object_name);
> + die("Invalid object name '%.*s'.", len, name);
> }
> }
> return ret;
^ permalink raw reply
* Re: [PATCH] safe_create_leading_directories: fix race that could give a false negative
From: Junio C Hamano @ 2013-03-17 6:26 UTC (permalink / raw)
To: Steven Walter; +Cc: git
In-Reply-To: <1363462256-5823-1-git-send-email-stevenrwalter@gmail.com>
Steven Walter <stevenrwalter@gmail.com> writes:
> If two processes are racing to create the same directory tree, they will
> both see that the directory doesn't exist, both try to mkdir(), and one
> of them will fail. This is okay, as we only care that the directory
> gets created. So, we add a check for EEXIST from mkdir, and continue if
> the directory now exists.
> ---
Thanks. Please sign-off your patch.
> sha1_file.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 40b2329..c7b7fec 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -123,6 +123,13 @@ int safe_create_leading_directories(char *path)
> }
> }
> else if (mkdir(path, 0777)) {
> + if (errno == EEXIST) {
> + /* We could be racing with another process to
> + * create the directory. As long as the
> + * directory gets created, we don't care. */
> + if (stat(path, &st) && S_ISDIR(st.st_mode))
> + continue;
/*
* Nice explanation, but we try to format our
* multi-line comments like this, slash-asterisk
* and nothing else on the opening line, and
* asterisk-slash and nothing else on the closing
* line.
*/
Thanks.
> + }
> *pos = '/';
> return -1;
> }
^ permalink raw reply
* Re: [PATCH v1 22/45] archive: convert to use parse_pathspec
From: Junio C Hamano @ 2013-03-17 6:22 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <CACsJy8A3NaA=faQoGhCDWNB7CKHQ2WF_5T0tf9z7mGTYFdreEg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> No, the literal strings are reparsed in path_exists() before being fed
> to read_tree_recursive.
Yuck. OK. Thanks.
^ permalink raw reply
* Re: [PATCH] Preallocate hash tables when the number of inserts are known in advance
From: Junio C Hamano @ 2013-03-17 6:18 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <CACsJy8B=KrmbsqAuvoJTVxmUVYF_fG7XNnKvu3zDVELsWcmtQg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Sun, Mar 17, 2013 at 12:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>
>>> This avoids unnecessary re-allocations and reinsertions. On webkit.git
>>> (i.e. about 182k inserts to the name hash table), this reduces about
>>> 100ms out of 3s user time.
>>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>
>> I think this is a very good idea, but I would prefer the second
>> parameter to the "preallocate" to be "expected number of entries"
>> and have the preallocate, which is a part of the hash API, decide
>> how to inflate that number to adjust to the desired load factor of
>> the hash table. We shouldn't have to adjust the caller when the
>> internal implementation of the hash table changes.
>
> OK will do.
I've squashed it in myself, so no need to resend only for this.
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 8d3d9bb..6c7a72f 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -389,7 +389,7 @@ static int find_exact_renames(struct diff_options *options)
struct hash_table file_table;
init_hash(&file_table);
- preallocate_hash(&file_table, (rename_src_nr + rename_dst_nr) * 2);
+ preallocate_hash(&file_table, rename_src_nr + rename_dst_nr);
for (i = 0; i < rename_src_nr; i++)
insert_file_table(&file_table, -1, i, rename_src[i].p->one);
diff --git a/hash.h b/hash.h
index 244d1fe..1d43ac0 100644
--- a/hash.h
+++ b/hash.h
@@ -40,11 +40,11 @@ static inline void init_hash(struct hash_table *table)
table->array = NULL;
}
-static inline void preallocate_hash(struct hash_table *table, unsigned int size)
+static inline void preallocate_hash(struct hash_table *table, unsigned int elts)
{
assert(table->size == 0 && table->nr == 0 && table->array == NULL);
- table->size = size;
- table->array = xcalloc(sizeof(struct hash_table_entry), size);
+ table->size = elts * 2;
+ table->array = xcalloc(sizeof(struct hash_table_entry), table->size);
}
#endif
diff --git a/name-hash.c b/name-hash.c
index 90c7b99..2a1f108 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -93,7 +93,7 @@ static void lazy_init_name_hash(struct index_state *istate)
if (istate->name_hash_initialized)
return;
if (istate->cache_nr)
- preallocate_hash(&istate->name_hash, istate->cache_nr * 2);
+ preallocate_hash(&istate->name_hash, istate->cache_nr);
for (nr = 0; nr < istate->cache_nr; nr++)
hash_index_entry(istate, istate->cache[nr]);
istate->name_hash_initialized = 1;
^ permalink raw reply related
* Re: [PATCH 0/3] fix unparsed object access in upload-pack
From: Junio C Hamano @ 2013-03-17 6:17 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20130317054039.GA16070@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> > [3/3]: upload-pack: load non-tip "want" objects from disk
>> >
>> > While investigating the bug, I found some weirdness around the
>> > stateless-rpc check_non_tip code. As far as I can tell, that code
>> > never actually gets triggered. It's not too surprising that we
>> > wouldn't have noticed, because it is about falling back due to a
>> > race condition. But please sanity check my explanation and patch.
>>
>> Thanks. That fall-back is Shawn's doing and I suspect that nobody is
>> exercising the codepath (he isn't).
>
> I almost wonder if we should cut it out entirely. It is definitely a
> possible race condition, but I wonder if anybody actually hits it in
> practice (and if they do, the consequence is that the fetch fails and
> needs to be retried). As far as I can tell, the code path has never
> actually been followed, and I do not recall ever seeing a bug report or
> complaint about it (though perhaps it happened once, which spurred the
> initial development?).
If you run multiple servers serving the same repository at the same
URL with a small mirroring lag, one may observe a set of refs from
one server, that are a tad older than the other server you actually
fetch from. k.org may have such an arrangement, but does GitHub
serve the same repository on multiple machines without tying the
same client to the same backend?
^ permalink raw reply
* Re: [RFC/PATCH] Introduce remote.pushdefault
From: Junio C Hamano @ 2013-03-17 6:10 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Jonathan Nieder, Jeff King
In-Reply-To: <CALkWK0m2N=D47WJLk1F4j1GsGGWHyfxVF_WGXBbG3vyrfQ-oLA@mail.gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> How will adding remote.pushdefault have any
> impact, unless I explicitly remove this branch-specific remote
> configuration? Besides, without branch.<name>.remote configured, I
> can't even pull and expect changes to be merged.
If the triangle topology is the norm for your project, I would
expect that it would be pretty common to have all of your branches
pull from one place and have all of them push to another place. In
the central repository workflow, it is very common that all of your
branches pull from one place and all of them push to the same place.
In the bigger picture, forcing to set the branch specific remote is
a mistake in the first place; in the longer term, you should be able
to say "My project uses the central repository workflow" once and
you should be able to pull without branch.master.remote; just record
where the default "origin" for all branches is.
I think the per-branch branch.*.pushremote is actively making things
worse by repeating the same mistake for the triangle topology. You
should be able to say "My project uses the triangle workflow" once
and specify two remotes, where you pull from and where you push to,
no?
A per-branch override, be it branch.*.remote or brnach.*.pushremote,
is useful to give an escape hatch in order to handle special cases
with additional flexibility. But making it the sole mechanism and
forcing the user to repeat the same "which remote does it use" all
over the place does not sound like a solid engineering.
^ permalink raw reply
* Re: [PATCH 2/2] pack-refs: add fully-peeled trait
From: Jeff King @ 2013-03-17 6:04 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git, Junio C Hamano
In-Reply-To: <51447C5E.3050808@alum.mit.edu>
On Sat, Mar 16, 2013 at 03:06:22PM +0100, Michael Haggerty wrote:
> > refname = parse_ref_line(refline, sha1);
> > if (refname) {
> > - last = create_ref_entry(refname, sha1, flag, 1);
> > + /*
> > + * Older git did not write peel lines for anything
> > + * outside of refs/tags/; if the fully-peeled trait
> > + * is not set, we are dealing with such an older
> > + * git and cannot assume an omitted peel value
> > + * means the ref is not a tag object.
> > + */
> > + int this_flag = flag;
> > + if (!fully_peeled && prefixcmp(refname, "refs/tags/"))
> > + this_flag &= ~REF_KNOWS_PEELED;
> > +
> > + last = create_ref_entry(refname, sha1, this_flag, 1);
> > add_ref(dir, last);
> > continue;
> > }
>
> I have to admit that I am partial to my variant of this code [1] because
> the logic makes it clearer when the affirmative decision can be made to
> set the REF_KNOWS_PEELED flag. But this version also looks correct to
> me and equivalent (aside from the idea that a few lines later if a
> peeled value is found then the REF_KNOWS_PEELED bit could also be set).
Yeah, I think they are equivalent, but I agree yours is a little more
readable. I'll switch it in my re-roll, and I will go ahead and set the
REF_KNOWS_PEELED bit when we see a peel line. That code should not be
triggered in general, but it is the sane thing for the reader to do, so
it makes the code more obvious and readable.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] pack-refs: write peeled entry for non-tags
From: Jeff King @ 2013-03-17 6:02 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git, Junio C Hamano
In-Reply-To: <514478C0.6060008@alum.mit.edu>
On Sat, Mar 16, 2013 at 02:50:56PM +0100, Michael Haggerty wrote:
> > @@ -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(sha1);
> > - 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(sha1);
> > + if (o->type == OBJ_TAG) {
>
> You suggested that I add a test (o != NULL) at the equivalent place in
> my code (which was derived from this code). Granted, my code was
> explicitly intending to pass invalid SHA1 values to parse_object(). But
> wouldn't it be a good defensive step to add the same check here?
Hmm, yeah. That is not new code, but rather just reindented from above
("diff -w" makes it much more obvious what is going on).
It is probably worth dying rather than segfaulting, though it should be
a separate patch (and I do not think it is sane to do anything except
die here). I almost wonder if parse_object should die by default on
bogus or missing objects, and the few callers who really want to handle
the error can call parse_object_gently. I do not relish analyzing each
caller, though. It would be simpler to add parse_object_or_die.
> > +# This matches show-ref's output
> > +print_ref() {
> > + echo "`git rev-parse "$1"` $1"
> > +}
> > +
>
> CodingGuidelines prefers $() over ``.
Old habits die hard. :)
I'll re-roll with your suggestions in a moment.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] pack-refs: add fully-peeled trait
From: Jeff King @ 2013-03-17 5:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <7vvc8qshgm.fsf@alter.siamese.dyndns.org>
On Sat, Mar 16, 2013 at 10:50:17PM -0700, Junio C Hamano wrote:
> I however wonder if the above implies it may make sense to add this
> on top? Perhaps it is not worth it, because it makes a difference
> only to a repository with annotated tags outside refs/tags hierarchy
> and still has the packed-refs file that was created with an older
> version of Git, so we can just tell "repack with new Git" to users
> with such a repository.
>
> refs.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 7f84efd..afc4dde 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -847,8 +847,10 @@ 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);
> + last->flag |= REF_KNOWS_PEELED;
> + }
> }
> }
>
Almost. The older version of Git would not have written those peel lines
in the first place. So yes, if we saw such a file, we could assume the
peel lines are valid. But nobody has ever generated that (with the
except of git between my two patches).
I do think it may be worth doing, though, just because it makes the
handling of the flag more obvious; somebody reading it later would
wonder "hey, shouldn't we be setting REF_KNOWS_PEELED here?", and it is
simple and harmless to just do it, rather than confusing a later reader.
I'll re-roll in a second to incorporate the comments from Michael.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] pack-refs: add fully-peeled trait
From: Junio C Hamano @ 2013-03-17 5:50 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty
In-Reply-To: <20130316090116.GB26855@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 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.
I think the patch makes sense and in line with what we already
discussed in the thread.
I however wonder if the above implies it may make sense to add this
on top? Perhaps it is not worth it, because it makes a difference
only to a repository with annotated tags outside refs/tags hierarchy
and still has the packed-refs file that was created with an older
version of Git, so we can just tell "repack with new Git" to users
with such a repository.
refs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 7f84efd..afc4dde 100644
--- a/refs.c
+++ b/refs.c
@@ -847,8 +847,10 @@ 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);
+ last->flag |= REF_KNOWS_PEELED;
+ }
}
}
^ permalink raw reply related
* Re: [RFC/PATCH] Introduce remote.pushdefault
From: Jeff King @ 2013-03-17 5:48 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Junio C Hamano, Git List, Jonathan Nieder
In-Reply-To: <CALkWK0m2N=D47WJLk1F4j1GsGGWHyfxVF_WGXBbG3vyrfQ-oLA@mail.gmail.com>
On Sun, Mar 17, 2013 at 06:00:08AM +0530, Ramkumar Ramachandra wrote:
> >> +remote.pushdefault::
> >> + The remote to push to by default. Overrides the
> >> + branch-specific configuration `branch.<name>.remote`.
> >
> > It feels unexpected to see "I may have said while on this branch I
> > push there and on that branch I push somewhere else, but no, with
> > this single configuration I'm invalidating all these previous
> > statements, and all pushes go to this new place".
> >
> > Shouldn't the default be the default that is to be overridden by
> > other configuration that is more specific? That is, "I would
> > normally push to this remote and unless I say otherwise that is all
> > I have to say, but for this particular branch, I push to somehwere
> > else".
>
> I'm a little confused as to where this configuration variable will be
> useful. On a fresh clone from Github, I get branch.master.remote
> configured to "origin". How will adding remote.pushdefault have any
> impact, unless I explicitly remove this branch-specific remote
> configuration? Besides, without branch.<name>.remote configured, I
> can't even pull and expect changes to be merged. So, really: what is
> the use of remote.pushdefault?
>
> I'm dropping this patch, and just going with branch.<name>.pushremote,
> unless you convince me otherwise.
That is why I described the scheme I did in [1]. It uses the following
two general rules:
1. Per-branch config trumps repo-wide config.
2. Push-specific config (e.g., "remote.pushdefault") trumps
non-specific config (e.g., "remote.default") for pushing.
So the push lookup list is (in order of precedence):
1. branch.*.pushremote
2. remote.pushdefault
3. branch.*.remote
4. remote.default
5. origin
and it solves Junio's issue because the way to say "override my
remote.pushdefault for this branch" is not to set "branch.*.remote", but
to set "branch.*.pushremote".
-Peff
[1] http://article.gmane.org/gmane.comp.version-control.git/215751
^ permalink raw reply
* Re: [PATCH 0/3] fix unparsed object access in upload-pack
From: Jeff King @ 2013-03-17 5:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7gl6txl3.fsf@alter.siamese.dyndns.org>
On Sat, Mar 16, 2013 at 10:16:40PM -0700, Junio C Hamano wrote:
> > ... (I had several bug reports
> > within a few hours of deploying v1.8.1.5 on github.com)
>
> Nice to have a pro at the widely used site ;-) I often wish it had
> a mechanism to deploy the tip of 'master' or 'maint', or even 'next'
> for 0.2% of its users' repositories to give us an early feedback.
We are quite slow and conservative at deploying new git, preferring
instead to let the rest of the world act as our testbed. :)
As seen with this bug, though, we really do get a lot more coverage of
weird cases due to our size. In the cases I looked at, the trigger
seemed to be clients doing the equivalent of of "git clone --depth=X
--no-single-branch". Almost nobody would do that intentionally, but
prior to v1.7.10, we did not have --single-branch; older clients using
--depth on a repository with multiple branches started failing clones
almost immediately.
We do have the capability to roll out to one or a few of our servers
(the granularity is not 0.2%, but it is still small). I'm going to try
to keep us more in sync with upstream git, but I don't know if I will
get to the point of ever deploying "master" or "next", even for a small
portion of the population. We are accumulating more hacks[1] on top of
git, so it is not just "run master for an hour on this server"; I have
to actually merge our fork.
I had been handling our hacks as patch series to be rebased on top of
upstream git releases, but that was getting increasingly unwieldy
(especially as people besides me work on it). Going forward, I'm going
to treat upstream git as a vendor branch and merge in occasionally to
get fixes.
One thing I might try is to keep a local "next-upstream" branch, that is
continually merging what is on upstream master with our local production
version. Then I could graduate it to production just like any other
topic branch when it comes time (instead of doing a gigantic painful
merge when we decide to upgrade upstream git).
That would mean I could test-deploy the "next-upstream" branch to a few
servers on any given day without doing a lot of work. So it might make
sense to do it at key times, like when we are in -rc here, to help shake
out any existing bugs before you make a release.
> > [3/3]: upload-pack: load non-tip "want" objects from disk
> >
> > While investigating the bug, I found some weirdness around the
> > stateless-rpc check_non_tip code. As far as I can tell, that code
> > never actually gets triggered. It's not too surprising that we
> > wouldn't have noticed, because it is about falling back due to a
> > race condition. But please sanity check my explanation and patch.
>
> Thanks. That fall-back is Shawn's doing and I suspect that nobody is
> exercising the codepath (he isn't).
I almost wonder if we should cut it out entirely. It is definitely a
possible race condition, but I wonder if anybody actually hits it in
practice (and if they do, the consequence is that the fetch fails and
needs to be retried). As far as I can tell, the code path has never
actually been followed, and I do not recall ever seeing a bug report or
complaint about it (though perhaps it happened once, which spurred the
initial development?).
-Peff
^ permalink raw reply
* Re: [PATCH] Preallocate hash tables when the number of inserts are known in advance
From: Duy Nguyen @ 2013-03-17 5:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vzjy2si7l.fsf@alter.siamese.dyndns.org>
On Sun, Mar 17, 2013 at 12:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> This avoids unnecessary re-allocations and reinsertions. On webkit.git
>> (i.e. about 182k inserts to the name hash table), this reduces about
>> 100ms out of 3s user time.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> I think this is a very good idea, but I would prefer the second
> parameter to the "preallocate" to be "expected number of entries"
> and have the preallocate, which is a part of the hash API, decide
> how to inflate that number to adjust to the desired load factor of
> the hash table. We shouldn't have to adjust the caller when the
> internal implementation of the hash table changes.
OK will do.
>> ---
>> nd/read-directory-recursive-optim reduces the number of input (from
>> 182k to 11k on webkit) to exclude machinery that all patches in the
>> exclude optimization series I posted seem insignificant. So I won't
>> repost them for inclusion unless you think it has cleanup values.
>
> Sorry, without a pointer, it is unclear what "exclude optimization
> series" you are referring to.
http://thread.gmane.org/gmane.comp.version-control.git/217697/focus=217950
--
Duy
^ permalink raw reply
* Re: [PATCH] Preallocate hash tables when the number of inserts are known in advance
From: Junio C Hamano @ 2013-03-17 5:34 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1363490886-29729-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> This avoids unnecessary re-allocations and reinsertions. On webkit.git
> (i.e. about 182k inserts to the name hash table), this reduces about
> 100ms out of 3s user time.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
I think this is a very good idea, but I would prefer the second
parameter to the "preallocate" to be "expected number of entries"
and have the preallocate, which is a part of the hash API, decide
how to inflate that number to adjust to the desired load factor of
the hash table. We shouldn't have to adjust the caller when the
internal implementation of the hash table changes.
> ---
> nd/read-directory-recursive-optim reduces the number of input (from
> 182k to 11k on webkit) to exclude machinery that all patches in the
> exclude optimization series I posted seem insignificant. So I won't
> repost them for inclusion unless you think it has cleanup values.
Sorry, without a pointer, it is unclear what "exclude optimization
series" you are referring to.
^ permalink raw reply
* Re: [PATCH v1 22/45] archive: convert to use parse_pathspec
From: Duy Nguyen @ 2013-03-17 5:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vehfetycj.fsf@alter.siamese.dyndns.org>
On Sun, Mar 17, 2013 at 12:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Sat, Mar 16, 2013 at 12:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>>
>>>> @@ -232,11 +228,18 @@ static int path_exists(struct tree *tree, const char *path)
>>>> static void parse_pathspec_arg(const char **pathspec,
>>>> struct archiver_args *ar_args)
>>>> {
>>>> - ar_args->pathspec = pathspec = get_pathspec("", pathspec);
>>>> + /*
>>>> + * must be consistent with parse_pathspec in path_exists()
>>>> + * Also if pathspec patterns are dependent, we're in big
>>>> + * trouble as we test each one separately
>>>> + */
>>>> + parse_pathspec(&ar_args->pathspec, 0,
>>>> + PATHSPEC_PREFER_FULL,
>>>> + "", pathspec);
>>>> if (pathspec) {
>>>> while (*pathspec) {
>>>> if (!path_exists(ar_args->tree, *pathspec))
>>>> - die("path not found: %s", *pathspec);
>>>> + die(_("pathspec '%s' did not match any files"), *pathspec);
>>>> pathspec++;
>>>> }
>>>
>>> You do not use ar_args->pathspec even though you used parse_pathspec()
>>> to grok it? What's the point of this change?
>>
>> parse_pathspec() here is needed because write_archive_entries needs it
>> later.
>
> That is not the issue I was pointing out. Even though you parse the
> pathspec into args->pathspec, the "if() { while () {} }" here still
> uses strings contained in **pathspec, as if they are literal strings
> and not ":(glob)Documentation" and such, and will not match the named
> directory.
No, the literal strings are reparsed in path_exists() before being fed
to read_tree_recursive. So ":(glob)Documentation" should match the
tree "Documentation".
> Technically, erroring out saying "':(glob)Documentation' does not exist
> as a path in the tree" is correct, but it would be nicer to have the
> code inspect parse_pathspec() result and independently barf, saying
> "this command does not support magic pathspecs, give me leading paths
> and nothing else", until we do support magic pathspecs, no?
--
Duy
^ permalink raw reply
* Re: [PATCH 0/3] fix unparsed object access in upload-pack
From: Junio C Hamano @ 2013-03-17 5:16 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20130316102428.GA29358@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This series fixes the issue I mentioned recently with upload-pack, where
> we might feed unparsed objects to the revision parser. The bug is in
> 435c833 (the tip of the jk/peel-ref topic), which is in v1.8.1 and up.
Good to see follow-up from a responsible contributor ;-)
> ... (I had several bug reports
> within a few hours of deploying v1.8.1.5 on github.com)
Nice to have a pro at the widely used site ;-) I often wish it had
a mechanism to deploy the tip of 'master' or 'maint', or even 'next'
for 0.2% of its users' repositories to give us an early feedback.
> [3/3]: upload-pack: load non-tip "want" objects from disk
>
> While investigating the bug, I found some weirdness around the
> stateless-rpc check_non_tip code. As far as I can tell, that code
> never actually gets triggered. It's not too surprising that we
> wouldn't have noticed, because it is about falling back due to a
> race condition. But please sanity check my explanation and patch.
Thanks. That fall-back is Shawn's doing and I suspect that nobody is
exercising the codepath (he isn't).
^ permalink raw reply
* Re: [PATCH v1 22/45] archive: convert to use parse_pathspec
From: Junio C Hamano @ 2013-03-17 5:00 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <CACsJy8DUb5j0of=cuQje5cRWLwyi-MiH-d-RsyBiPOjaD1Tntg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Sat, Mar 16, 2013 at 12:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>
>>> @@ -232,11 +228,18 @@ static int path_exists(struct tree *tree, const char *path)
>>> static void parse_pathspec_arg(const char **pathspec,
>>> struct archiver_args *ar_args)
>>> {
>>> - ar_args->pathspec = pathspec = get_pathspec("", pathspec);
>>> + /*
>>> + * must be consistent with parse_pathspec in path_exists()
>>> + * Also if pathspec patterns are dependent, we're in big
>>> + * trouble as we test each one separately
>>> + */
>>> + parse_pathspec(&ar_args->pathspec, 0,
>>> + PATHSPEC_PREFER_FULL,
>>> + "", pathspec);
>>> if (pathspec) {
>>> while (*pathspec) {
>>> if (!path_exists(ar_args->tree, *pathspec))
>>> - die("path not found: %s", *pathspec);
>>> + die(_("pathspec '%s' did not match any files"), *pathspec);
>>> pathspec++;
>>> }
>>
>> You do not use ar_args->pathspec even though you used parse_pathspec()
>> to grok it? What's the point of this change?
>
> parse_pathspec() here is needed because write_archive_entries needs it
> later.
That is not the issue I was pointing out. Even though you parse the
pathspec into args->pathspec, the "if() { while () {} }" here still
uses strings contained in **pathspec, as if they are literal strings
and not ":(glob)Documentation" and such, and will not match the named
directory.
Technically, erroring out saying "':(glob)Documentation' does not exist
as a path in the tree" is correct, but it would be nicer to have the
code inspect parse_pathspec() result and independently barf, saying
"this command does not support magic pathspecs, give me leading paths
and nothing else", until we do support magic pathspecs, no?
^ permalink raw reply
* Re: [PATCH] pull: Apply -q and -v options to rebase mode as well
From: Junio C Hamano @ 2013-03-17 4:53 UTC (permalink / raw)
To: Peter Eisentraut; +Cc: git
In-Reply-To: <1363314368.14066.3.camel@vanquo.pezone.net>
Peter Eisentraut <peter@eisentraut.org> writes:
> git pull passed -q and -v only to git merge, but they can be useful for
> git rebase as well, so pass them there, too. In particular, using -q
> shuts up the "Already up-to-date." message. Add test cases to prove it.
>
> Signed-off-by: Peter Eisentraut <peter@eisentraut.org>
> ---
Looks quite straight-forward.
I wouldn't call our test cases "proving" anything, though. The
reason we add tests is to make sure that others who touch the code
later will not break the feature you add today by documenting the
behaviour we expect out of our code.
> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index 1b06691..aa31abe 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -19,6 +19,17 @@ test_expect_success 'git pull -q' '
> test ! -s out)
> '
>
> +test_expect_success 'git pull -q --rebase' '
> + mkdir clonedqrb &&
> + (cd clonedqrb && git init &&
> + git pull -q --rebase "../parent" >out 2>err &&
> + test ! -s err &&
> + test ! -s out &&
> + git pull -q --rebase "../parent" >out 2>err &&
> + test ! -s err &&
> + test ! -s out)
> +'
Pulling twice is a good thing here, to see how it behaves when there
is something to be fetched, and when you are up to date. I think it
is a good idea to add it to the normal 'pull -q' test.
Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox