Re: [PATCH 2/7] move setting of object-type to alloc_* functions
On Sat, Jul 12, 2014 at 02:05:39PM -0400, Jeff King wrote: I don't particularly like 'flag' here. (not a massive dislike, mind you:) Perhaps: flag-object_type, type-node_type? Or, if that's too verbose, maybe just: flag-type, type-node? Me either, but as you noticed, type was taken. Your suggestions seem fine. We could also just do away with the macro as discussed earlier (we already do in the commit_node case, anyway...). Thinking on this more, writing out the definitions is the only sane thing to do here, now that alloc_commit_node does not use the macro. Otherwise you are inviting people to modify the macro, but fail to notice that the commit allocator also needs updating. Here's a re-roll. The interesting bit is the addition of the second patch (but the rest needed to be rebased on top). [1/8]: alloc.c: remove the alloc_raw_commit_node() function [2/8]: alloc: write out allocator definitions [3/8]: move setting of object-type to alloc_* functions [4/8]: parse_object_buffer: do not set object type [5/8]: add object_as_type helper for casting objects [6/8]: alloc: factor out commit index [7/8]: object_as_type: set commit index [8/8]: diff-tree: avoid lookup_unknown_object -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/8] alloc.c: remove the alloc_raw_commit_node() function
From: Ramsay Jones ram...@ramsay1.demon.co.uk
In order to encapsulate the setting of the unique commit index, commit
969eba63 (commit: push commit_index update into alloc_commit_node,
10-06-2014) introduced a (logically private) intermediary allocator
function. However, this function (alloc_raw_commit_node()) was declared
as a public function, which undermines its entire purpose.
Introduce an inline function, alloc_node(), which implements the main
logic of the allocator used by DEFINE_ALLOCATOR, and redefine the macro
in terms of the new function. In addition, use the new function in the
implementation of the alloc_commit_node() allocator, rather than the
intermediary allocator, which can now be removed.
Noticed by sparse (symbol 'alloc_raw_commit_node' was not declared.
Should it be static?).
Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk
Signed-off-by: Jeff King p...@peff.net
---
alloc.c | 47 +--
1 file changed, 29 insertions(+), 18 deletions(-)
diff --git a/alloc.c b/alloc.c
index eb22a45..d7c3605 100644
--- a/alloc.c
+++ b/alloc.c
@@ -19,22 +19,10 @@
#define BLOCKING 1024
#define DEFINE_ALLOCATOR(name, type) \
-static unsigned int name##_allocs; \
+static struct alloc_state name##_state;\
void *alloc_##name##_node(void)\
{ \
- static int nr; \
- static type *block; \
- void *ret; \
- \
- if (!nr) { \
- nr = BLOCKING; \
- block = xmalloc(BLOCKING * sizeof(type)); \
- } \
- nr--; \
- name##_allocs++;\
- ret = block++; \
- memset(ret, 0, sizeof(type)); \
- return ret; \
+ return alloc_node(name##_state, sizeof(type)); \
}
union any_object {
@@ -45,16 +33,39 @@ union any_object {
struct tag tag;
};
+struct alloc_state {
+ int count; /* total number of nodes allocated */
+ int nr;/* number of nodes left in current allocation */
+ void *p; /* first free node in current allocation */
+};
+
+static inline void *alloc_node(struct alloc_state *s, size_t node_size)
+{
+ void *ret;
+
+ if (!s-nr) {
+ s-nr = BLOCKING;
+ s-p = xmalloc(BLOCKING * node_size);
+ }
+ s-nr--;
+ s-count++;
+ ret = s-p;
+ s-p = (char *)s-p + node_size;
+ memset(ret, 0, node_size);
+ return ret;
+}
+
DEFINE_ALLOCATOR(blob, struct blob)
DEFINE_ALLOCATOR(tree, struct tree)
-DEFINE_ALLOCATOR(raw_commit, struct commit)
DEFINE_ALLOCATOR(tag, struct tag)
DEFINE_ALLOCATOR(object, union any_object)
+static struct alloc_state commit_state;
+
void *alloc_commit_node(void)
{
static int commit_count;
- struct commit *c = alloc_raw_commit_node();
+ struct commit *c = alloc_node(commit_state, sizeof(struct commit));
c-index = commit_count++;
return c;
}
@@ -66,13 +77,13 @@ static void report(const char *name, unsigned int count,
size_t size)
}
#define REPORT(name, type) \
-report(#name, name##_allocs, name##_allocs * sizeof(type) 10)
+report(#name, name##_state.count, name##_state.count * sizeof(type) 10)
void alloc_report(void)
{
REPORT(blob, struct blob);
REPORT(tree, struct tree);
- REPORT(raw_commit, struct commit);
+ REPORT(commit, struct commit);
REPORT(tag, struct tag);
REPORT(object, union any_object);
}
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/8] alloc: write out allocator definitions
Because the allocator functions for tree, blobs, etc are all
very similar, we originally used a macro to avoid repeating
ourselves. Since the prior commit, though, the heavy lifting
is done by an inline helper function. The macro does still
save us a few lines, but at some readability cost. It
obfuscates the function definitions (and makes them hard to
find via grep).
Much worse, though, is the fact that it isn't used
consistently for all allocators. Somebody coming later may
be tempted to modify DEFINE_ALLOCATOR, but they would miss
alloc_commit_node, which is treated specially.
Let's just drop the macro and write everything out
explicitly.
Signed-off-by: Jeff King p...@peff.net
---
alloc.c | 38 +++---
1 file changed, 27 insertions(+), 11 deletions(-)
diff --git a/alloc.c b/alloc.c
index d7c3605..03e458b 100644
--- a/alloc.c
+++ b/alloc.c
@@ -18,13 +18,6 @@
#define BLOCKING 1024
-#define DEFINE_ALLOCATOR(name, type) \
-static struct alloc_state name##_state;\
-void *alloc_##name##_node(void)\
-{ \
- return alloc_node(name##_state, sizeof(type)); \
-}
-
union any_object {
struct object object;
struct blob blob;
@@ -55,10 +48,33 @@ static inline void *alloc_node(struct alloc_state *s,
size_t node_size)
return ret;
}
-DEFINE_ALLOCATOR(blob, struct blob)
-DEFINE_ALLOCATOR(tree, struct tree)
-DEFINE_ALLOCATOR(tag, struct tag)
-DEFINE_ALLOCATOR(object, union any_object)
+static struct alloc_state blob_state;
+void *alloc_blob_node(void)
+{
+ struct blob *b = alloc_node(blob_state, sizeof(struct blob));
+ return b;
+}
+
+static struct alloc_state tree_state;
+void *alloc_tree_node(void)
+{
+ struct tree *t = alloc_node(tree_state, sizeof(struct tree));
+ return t;
+}
+
+static struct alloc_state tag_state;
+void *alloc_tag_node(void)
+{
+ struct tag *t = alloc_node(tag_state, sizeof(struct tag));
+ return t;
+}
+
+static struct alloc_state object_state;
+void *alloc_object_node(void)
+{
+ struct object *obj = alloc_node(object_state, sizeof(union
any_object));
+ return obj;
+}
static struct alloc_state commit_state;
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/8] parse_object_buffer: do not set object type
The only way that obj can be non-NULL is if it came from one of the lookup_* functions. These functions always ensure that the object has the expected type (and return NULL otherwise), so there is no need for us to set the type. Signed-off-by: Jeff King p...@peff.net --- object.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/object.c b/object.c index a950b85..472aa8d 100644 --- a/object.c +++ b/object.c @@ -213,8 +213,6 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t warning(object %s has unknown type id %d, sha1_to_hex(sha1), type); obj = NULL; } - if (obj obj-type == OBJ_NONE) - obj-type = type; return obj; } -- 2.0.0.566.gfe3e6b2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/8] add object_as_type helper for casting objects
When we call lookup_commit, lookup_tree, etc, the logic goes
something like:
1. Look for an existing object struct. If we don't have
one, allocate and return a new one.
2. Double check that any object we have is the expected
type (and complain and return NULL otherwise).
3. Convert an object with type OBJ_NONE (from a prior
call to lookup_unknown_object) to the expected type.
We can encapsulate steps 2 and 3 in a helper function which
checks whether we have the expected object type, converts
OBJ_NONE as appropriate, and returns the object.
Not only does this shorten the code, but it also provides
one central location for converting OBJ_NONE objects into
objects of other types. Future patches will use that to
enforce type-specific invariants.
Since this is a refactoring, we would want it to behave
exactly as the current code. It takes a little reasoning to
see that this is the case:
- for lookup_{commit,tree,etc} functions, we are just
pulling steps 2 and 3 into a function that does the same
thing.
- for the call in peel_object, we currently only do step 3
(but we want to consolidate it with the others, as
mentioned above). However, step 2 is a noop here, as the
surrounding conditional makes sure we have OBJ_NONE
(which we want to keep to avoid an extraneous call to
sha1_object_info).
- for the call in lookup_commit_reference_gently, we are
currently doing step 2 but not step 3. However, step 3
is a noop here. The object we got will have just come
from deref_tag, which must have figured out the type for
each object in order to know when to stop peeling.
Therefore the type will never be OBJ_NONE.
Signed-off-by: Jeff King p...@peff.net
---
blob.c | 9 +
commit.c | 19 ++-
object.c | 17 +
object.h | 2 ++
refs.c | 3 +--
tag.c| 9 +
tree.c | 9 +
7 files changed, 25 insertions(+), 43 deletions(-)
diff --git a/blob.c b/blob.c
index 5720a38..1fcb8e4 100644
--- a/blob.c
+++ b/blob.c
@@ -8,14 +8,7 @@ struct blob *lookup_blob(const unsigned char *sha1)
struct object *obj = lookup_object(sha1);
if (!obj)
return create_object(sha1, alloc_blob_node());
- if (!obj-type)
- obj-type = OBJ_BLOB;
- if (obj-type != OBJ_BLOB) {
- error(Object %s is a %s, not a blob,
- sha1_to_hex(sha1), typename(obj-type));
- return NULL;
- }
- return (struct blob *) obj;
+ return object_as_type(obj, OBJ_BLOB, 0);
}
int parse_blob_buffer(struct blob *item, void *buffer, unsigned long size)
diff --git a/commit.c b/commit.c
index c1815c8..08816d3 100644
--- a/commit.c
+++ b/commit.c
@@ -18,19 +18,6 @@ int save_commit_buffer = 1;
const char *commit_type = commit;
-static struct commit *check_commit(struct object *obj,
- const unsigned char *sha1,
- int quiet)
-{
- if (obj-type != OBJ_COMMIT) {
- if (!quiet)
- error(Object %s is a %s, not a commit,
- sha1_to_hex(sha1), typename(obj-type));
- return NULL;
- }
- return (struct commit *) obj;
-}
-
struct commit *lookup_commit_reference_gently(const unsigned char *sha1,
int quiet)
{
@@ -38,7 +25,7 @@ struct commit *lookup_commit_reference_gently(const unsigned
char *sha1,
if (!obj)
return NULL;
- return check_commit(obj, sha1, quiet);
+ return object_as_type(obj, OBJ_COMMIT, quiet);
}
struct commit *lookup_commit_reference(const unsigned char *sha1)
@@ -63,9 +50,7 @@ struct commit *lookup_commit(const unsigned char *sha1)
struct object *obj = lookup_object(sha1);
if (!obj)
return create_object(sha1, alloc_commit_node());
- if (!obj-type)
- obj-type = OBJ_COMMIT;
- return check_commit(obj, sha1, 0);
+ return object_as_type(obj, OBJ_COMMIT, 0);
}
struct commit *lookup_commit_reference_by_name(const char *name)
diff --git a/object.c b/object.c
index 472aa8d..b2319f6 100644
--- a/object.c
+++ b/object.c
@@ -158,6 +158,23 @@ void *create_object(const unsigned char *sha1, void *o)
return obj;
}
+void *object_as_type(struct object *obj, enum object_type type, int quiet)
+{
+ if (obj-type == type)
+ return obj;
+ else if (obj-type == OBJ_NONE) {
+ obj-type = type;
+ return obj;
+ }
+ else {
+ if (!quiet)
+ error(object %s is a %s, not a %s,
+ sha1_to_hex(obj-sha1),
+ typename(obj-type), typename(type));
+ return NULL;
+ }
+}
+
struct object *lookup_unknown_object(const unsigned char *sha1)
{
struct object *obj
[PATCH v2 6/8] alloc: factor out commit index
We keep a static counter to set the commit index on newly
allocated objects. However, since we also need to set the
index on any_objects which are converted to commits, let's
make the counter available as a public function.
While we're moving it, let's make sure the counter is
allocated as an unsigned integer to match the index field in
struct commit.
Signed-off-by: Jeff King p...@peff.net
---
alloc.c | 9 +++--
cache.h | 1 +
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/alloc.c b/alloc.c
index fd2e32d..12afadf 100644
--- a/alloc.c
+++ b/alloc.c
@@ -82,12 +82,17 @@ void *alloc_object_node(void)
static struct alloc_state commit_state;
+unsigned int alloc_commit_index(void)
+{
+ static unsigned int count;
+ return count++;
+}
+
void *alloc_commit_node(void)
{
- static int commit_count;
struct commit *c = alloc_node(commit_state, sizeof(struct commit));
c-object.type = OBJ_COMMIT;
- c-index = commit_count++;
+ c-index = alloc_commit_index();
return c;
}
diff --git a/cache.h b/cache.h
index 44aa439..ba68e11 100644
--- a/cache.h
+++ b/cache.h
@@ -1380,6 +1380,7 @@ extern void *alloc_commit_node(void);
extern void *alloc_tag_node(void);
extern void *alloc_object_node(void);
extern void alloc_report(void);
+extern unsigned int alloc_commit_index(void);
/* trace.c */
__attribute__((format (printf, 1, 2)))
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/8] move setting of object-type to alloc_* functions
The struct object type implements basic object
polymorphism. Individual instances are allocated as
concrete types (or as a union type that can store any
object), and a struct object * can be cast into its real
type after examining its type enum. This means it is
dangerous to have a type field that does not match the
allocation (e.g., setting the type field of a struct blob
to OBJ_COMMIT would mean that a reader might read past the
allocated memory).
In most of the current code this is not a problem; the first
thing we do after allocating an object is usually to set its
type field by passing it to create_object. However, the
virtual commits we create in merge-recursive.c do not ever
get their type set. This does not seem to have caused
problems in practice, though (presumably because we always
pass around a struct commit pointer and never even look at
the type).
We can fix this oversight and also make it harder for future
code to get it wrong by setting the type directly in the
object allocation functions.
This will also make it easier to fix problems with commit
index allocation, as we know that any object allocated by
alloc_commit_node will meet the invariant that an object
with an OBJ_COMMIT type field will have a unique index
number.
Signed-off-by: Jeff King p...@peff.net
---
alloc.c | 5 +
blob.c | 2 +-
builtin/blame.c | 1 -
commit.c| 6 ++
object.c| 5 ++---
object.h| 2 +-
tag.c | 2 +-
tree.c | 2 +-
8 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/alloc.c b/alloc.c
index 03e458b..fd2e32d 100644
--- a/alloc.c
+++ b/alloc.c
@@ -52,6 +52,7 @@ static struct alloc_state blob_state;
void *alloc_blob_node(void)
{
struct blob *b = alloc_node(blob_state, sizeof(struct blob));
+ b-object.type = OBJ_BLOB;
return b;
}
@@ -59,6 +60,7 @@ static struct alloc_state tree_state;
void *alloc_tree_node(void)
{
struct tree *t = alloc_node(tree_state, sizeof(struct tree));
+ t-object.type = OBJ_TREE;
return t;
}
@@ -66,6 +68,7 @@ static struct alloc_state tag_state;
void *alloc_tag_node(void)
{
struct tag *t = alloc_node(tag_state, sizeof(struct tag));
+ t-object.type = OBJ_TAG;
return t;
}
@@ -73,6 +76,7 @@ static struct alloc_state object_state;
void *alloc_object_node(void)
{
struct object *obj = alloc_node(object_state, sizeof(union
any_object));
+ obj-type = OBJ_NONE;
return obj;
}
@@ -82,6 +86,7 @@ void *alloc_commit_node(void)
{
static int commit_count;
struct commit *c = alloc_node(commit_state, sizeof(struct commit));
+ c-object.type = OBJ_COMMIT;
c-index = commit_count++;
return c;
}
diff --git a/blob.c b/blob.c
index ae320bd..5720a38 100644
--- a/blob.c
+++ b/blob.c
@@ -7,7 +7,7 @@ struct blob *lookup_blob(const unsigned char *sha1)
{
struct object *obj = lookup_object(sha1);
if (!obj)
- return create_object(sha1, OBJ_BLOB, alloc_blob_node());
+ return create_object(sha1, alloc_blob_node());
if (!obj-type)
obj-type = OBJ_BLOB;
if (obj-type != OBJ_BLOB) {
diff --git a/builtin/blame.c b/builtin/blame.c
index d3b256e..8f3e311 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2287,7 +2287,6 @@ static struct commit *fake_working_tree_commit(struct
diff_options *opt,
commit = alloc_commit_node();
commit-object.parsed = 1;
commit-date = now;
- commit-object.type = OBJ_COMMIT;
parent_tail = commit-parents;
if (!resolve_ref_unsafe(HEAD, head_sha1, 1, NULL))
diff --git a/commit.c b/commit.c
index acb74b5..c1815c8 100644
--- a/commit.c
+++ b/commit.c
@@ -61,10 +61,8 @@ struct commit *lookup_commit_or_die(const unsigned char
*sha1, const char *ref_n
struct commit *lookup_commit(const unsigned char *sha1)
{
struct object *obj = lookup_object(sha1);
- if (!obj) {
- struct commit *c = alloc_commit_node();
- return create_object(sha1, OBJ_COMMIT, c);
- }
+ if (!obj)
+ return create_object(sha1, alloc_commit_node());
if (!obj-type)
obj-type = OBJ_COMMIT;
return check_commit(obj, sha1, 0);
diff --git a/object.c b/object.c
index 9c31e9a..a950b85 100644
--- a/object.c
+++ b/object.c
@@ -141,13 +141,12 @@ static void grow_object_hash(void)
obj_hash_size = new_hash_size;
}
-void *create_object(const unsigned char *sha1, int type, void *o)
+void *create_object(const unsigned char *sha1, void *o)
{
struct object *obj = o;
obj-parsed = 0;
obj-used = 0;
- obj-type = type;
obj-flags = 0;
hashcpy(obj-sha1, sha1);
@@ -163,7 +162,7 @@ struct object *lookup_unknown_object(const unsigned char
*sha1)
{
struct object *obj = lookup_object(sha1);
if (!obj)
- obj = create_object(sha1,
[PATCH v2 8/8] diff-tree: avoid lookup_unknown_object
We generally want to avoid lookup_unknown_object, because it results in allocating more memory for the object than may be strictly necessary. In this case, it is used to check whether we have an already-parsed object before calling parse_object, to save us from reading the object from disk. Using lookup_object would be fine for that purpose, but we can take it a step further. Since this code was written, parse_object already learned the check lookup_object optimization, so we can simply call parse_object directly. Signed-off-by: Jeff King p...@peff.net --- builtin/diff-tree.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c index ce0e019..1c4ad62 100644 --- a/builtin/diff-tree.c +++ b/builtin/diff-tree.c @@ -68,9 +68,7 @@ static int diff_tree_stdin(char *line) line[len-1] = 0; if (get_sha1_hex(line, sha1)) return -1; - obj = lookup_unknown_object(sha1); - if (!obj || !obj-parsed) - obj = parse_object(sha1); + obj = parse_object(sha1); if (!obj) return -1; if (obj-type == OBJ_COMMIT) -- 2.0.0.566.gfe3e6b2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 7/8] object_as_type: set commit index
The point of the index field of struct commit is that
every allocated commit would have one. It is supposed to be
an invariant that whenever object-type is set to
OBJ_COMMIT, we have a unique index.
Commit 969eba6 (commit: push commit_index update into
alloc_commit_node, 2014-06-10) covered this case for
newly-allocated commits. However, we may also allocate an
unknown object via lookup_unknown_object, and only later
convert it to a commit. We must make sure that we set the
commit index when we switch the type field.
Signed-off-by: Jeff King p...@peff.net
---
object.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/object.c b/object.c
index b2319f6..69fbbbf 100644
--- a/object.c
+++ b/object.c
@@ -163,6 +163,8 @@ void *object_as_type(struct object *obj, enum object_type
type, int quiet)
if (obj-type == type)
return obj;
else if (obj-type == OBJ_NONE) {
+ if (type == OBJ_COMMIT)
+ ((struct commit *)obj)-index = alloc_commit_index();
obj-type = type;
return obj;
}
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] http: Add Accept-Language header if possible
2014-07-13 13:26 GMT+09:00 Eric Sunshine sunsh...@sunshineco.com:
+ /* Decide the precision for q-factor on number of preferred
languages. */
+ if (num_langs + 1 100) { /* +1 is for '*' */
+ q_precision = 0.001;
+ q_format = ; q=%.3f;
+ } else if (num_langs + 1 10) { /* +1 is for '*' */
+ q_precision = 0.01;
+ q_format = ; q=%.2f;
+ }
It might make sense to have a final 'else' here which sets these
variables for the 0.1 case so that the reader of the code doesn't have
to refer back to the top of the function to figure out what is going
on.
} else {
q_precision = 0.1;
q_format = ; q=%.1f;
}
Better yet, would it be possible to compute these values rather than
having to set them manually via a cascading if-chain?
I think it is possible like this:
num_langs += 1; /* for '*' */
decimal_places = 1 + (num_langs 10) + (num_langs 100);
snprintf(q_format, sizeof(q_format), ; q=%%.%df, decimal_places);
for (q_precision = 1.0; decimal_places-- 0;) q_precision /= 10;
Does this one look better than before? I'm not sure which one is better.
ps. The last line can be simpler by using pow() but I'm not sure it is
okay to include math.h.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Bug] data loss with cyclic alternates
Am 11.07.14 18:01, schrieb Junio C Hamano:
Ephrim Khong dr.kh...@gmail.com writes:
git seems to have issues with alternates when cycles are present (repo
A has B/objects as alternates, B has A/objects as alternates).
Yeah, don't do that. A thinks eh, the other guy must have it and
B thinks the same. In general, do not prune or gc a repository
other repositories borrow from, even if there is no cycle, because
the borrowee does not know anythning about objects that it itself no
longer needs but are still needed by its borrowers.
It seems that there is a safeguard against this in sha1_file.c,
link_alt_odb_entry(), that doesn't work as intended:
if (!strcmp(ent-base, objdir)) {
free(ent);
return -1;
}
However, printf-debugging tells me that ent-base is absolute and objdir
is relative (.git/objects) at this point, so the strings are different
even though the files are the same.
I never submitted a patch to git. Do you think someone can fix this
hickup, otherwise I'll give it a shot next week.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC v2 03/19] rebase -i: reword executes pre-commit hook on interim commit
Hi Junio,
Junio C Hamano writes:
Fabian Ruch baf...@gmail.com writes:
The to-do list command `reword` replays a commit like `pick` but lets
the user also edit the commit's log message. This happens in two
steps. Firstly, the named commit is cherry-picked. Secondly, the
commit created in the first step is amended using an unchanged index
to edit the log message. The pre-commit hook is meant to verify the
changes introduced by a commit (for instance, catching inserted
trailing white space). Since the committed tree is not changed
between the two steps, do not execute the pre-commit hook in the
second step.
It is not like the second step is built as a child commit of the
result from the first step, recording the same tree without any
change. Why is it a good thing not to run the pre-commit hook (or
other hooks for that matter)? After all, the result of the second
step is what is recorded in the final history; it just feels wrong
not to test that one, even if it were a good idea to test only once.
if I understand correctly, the tree of the (amended) commit isn't tested
at all by git-rebase--interactive because git-cherry-pick, and therefore
pick_one, commits with both the pre-commit and commit-msg hook disabled
(see the git commit -n command line being built in
sequencer.c#run_git_commit since revision 9fa4db5).
The commit --amend we are concerned with here amends using an
untouched tree so that the history ought to record exactly the same
trees as prior to commit --amend. If git-cherry-pick was testing the
tree, then it would be unnecessary to test the tree a second time. Since
git-cherry-pick is not testing as of yet, testing and possibly rejecting
a tree in the second step would actually be wrong. I totally neglected
to look for a test case when I submitted this patch, so I'd like to
supply one late now:
test_expect_success 'reword a commit violating pre-commit' '
mkdir -p .git/hooks
PRE_COMMIT=.git/hooks/pre-commit
cat $PRE_COMMIT EOF
#!/bin/sh
echo running pre-commit
exit 1
EOF
chmod +x $PRE_COMMIT
test_must_fail test_commit file
test_commit --no-verify file
set_fake_editor
FAKE_LINES=pick 1 git rebase -i HEAD^
FAKE_LINES=pick 1 git rebase -i --no-ff HEAD^
FAKE_LINES=reword 1 git rebase -i HEAD^
'
(This addition to t3404-rebase--interactive.sh is based on the test case
rebase a commit violating pre-commit; test_commit --no-verify will
be implemented the obvious way.)
In both cases, it's correct to disable the pre-commit hook when editing
the log message and it just makes sense to test changes where you make
them. Unfortunately, it is not obvious that git commit --amend merely
edits the log message rather than committing a new tree.
For what it's worth, if we were to create an empty child commit and
squash it into the parent instead of amending immediately, then
git-rebase--interactive would disable tree verification in the second
step as well. This behaviour was introduced by commit c5b09fe. Although
the change seems to have been triggered by something completely
different back then, the correctness criterion remains the same, i.e.
recording previously committed trees.
Best regards,
Fabian
Specify the git-commit option `--no-verify` to disable the pre-commit
hook when editing the log message. Because `--no-verify` also skips
the commit-msg hook, execute the hook from within
git-rebase--interactive after the commit is created. Fortunately, the
commit message is still available in `$GIT_DIR/COMMIT_EDITMSG` after
git-commit terminates. Caveat: In case the commit-msg hook finds the
new log message ill-formatted, the user is only notified of the
failed commit-msg hook but the log message is used for the commit
anyway. git-commit ought to offer more fine-grained control over
which hooks are executed.
Signed-off-by: Fabian Ruch baf...@gmail.com
---
git-rebase--interactive.sh | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 689400e..b50770d 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -504,10 +504,19 @@ do_next () {
mark_action_done
do_pick $sha1 $rest
-git commit --allow-empty --amend --no-post-rewrite
${gpg_sign_opt:+$gpg_sign_opt} || {
-warn Could not amend commit after successfully picking
$sha1... $rest
-exit_with_patch $sha1 1
-}
+# TODO: Work around the fact that git-commit lets us
+# disable either both the pre-commit and the commit-msg
+# hook or none. Disable the pre-commit hook because the
+# tree is left unchanged but run the commit-msg hook
+# from here because the log message is altered.
+git commit --allow-empty --amend --no-post-rewrite -n
Re: [PATCH 3/4 v6] cache-tree: subdirectory tests
David Turner dtur...@twopensource.com writes: On Fri, 2014-07-11 at 08:40 -0700, Junio C Hamano wrote: In the ideal world, I think whoever tries to compare two cache-trees (i.e. test-dump-cache-tree) should *not* care, because we are merely trying to show what the correct tree object name for the node would be, but this is only for testing, so the best way forward would be to: - Stop using DRY_RUN in test-dump-cache-tree.c; - Stop the code to support DRY_RUN from cache-tree.c (nobody but the test uses it); and - Drop the -e '#(ref)/d' from the above. I would think. Do you mean that I should do this in this patch set, or that it's a good idea for the future? I have no strong preference either way. Removing DRY_RUN may simplify things in the code that gets used in the real life (as opposed to the code that is only used during the tests), so I do not mind it if it was done before the series as a preparation step. Also, if we don't use DRY_RUN, won't test-dump-cache-tree add trees to the actual ODB, which would be odd for a test program? I do not see it as odd at all; after all, nobody in the real-life uses dry-run and as you can see its use is broken, or at least is inconsistent with the rest of the system. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] http: Add Accept-Language header if possible
Yi, EungJun semtlen...@gmail.com writes: I think it is possible like this: num_langs += 1; /* for '*' */ decimal_places = 1 + (num_langs 10) + (num_langs 100); snprintf(q_format, sizeof(q_format), ; q=%%.%df, decimal_places); for (q_precision = 1.0; decimal_places-- 0;) q_precision /= 10; Does this one look better than before? I'm not sure which one is better. ps. The last line can be simpler by using pow() but I'm not sure it is okay to include math.h. If you do not want floating point (and I think we tend to avoid it when we do not need it), you can realize that in your use of 0.1 and 0.01 and 0.001 there is nothing fundamentally floating-point; you can measure how many digits below the two-byte string zero-dot you would want upfront (by counting num_langs), and show an integer counter zero-padded to the left to that width. That would avoid having to even worry about a possible funny case where subtracting 0.01 ten times from 0.1 may not yield zero (or the result of subtracting nine times may not reach 0.01) due to rounding errors accumulating, which was the first thing that came to my mind when I saw your loop. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig
Jeff King p...@peff.net writes: On Fri, Jul 11, 2014 at 02:54:25PM -0700, Junio C Hamano wrote: Yeah, we're quite inconsistent there. In some cases we silently ignore something unknown (e.g., a color.diff.* slot that we do not understand), but in most cases if it is a config key we understand but a value we do not, we complain and die. Hm, that's bad---we've become less and less careful over time, perhaps? I don't think so. I think we've always been not-very-lenient with parsing values. Two examples: ... So I do not think we have ever had a rule, but if we did, it is probably silently ignore unknown keys, complain on unknown values. Yeah, somehow I was mixing up these two cases fuzzily in my mind while responding. Rejecting unknown keys is bad, but we cannot be perfectly forward compatible nor behave sensibly on unknown values while issuing errors against known-to-be-bad values, so your rule above sounds like the most sensible thing to do. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig
Eric Sunshine sunsh...@sunshineco.com writes: Made parse_sort_string take a var parameter, and if given will only warn about invalid parameter, instead of error. This seems unnecessarily ugly since it's hard-coding specialized knowledge of the callers' error-reporting requirements into what should be a generalized parsing function. If you instead make parse_sort_string() responsible only for attempting to parse the value, but leave error-reporting to the callers, then this ugliness goes away. See below. Yup, you are absolutely right. Thanks for catching my silly. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 4/4] cache-tree: Write updated cache-tree after commit
Eric Sunshine sunsh...@sunshineco.com writes: + (echo p; echo 1; echo; echo s; echo n; echo y; echo q) | git commit --interactive -m foo Broken -chain. Would a printf make this more readable? printf p\n1\n\ns\nn\ny\nq\n | git commt ... Perhaps not. But printf %s\n p 1 s n y q is vastly more readable, I would think. Don't we have test_write_lines which is exactly that, though? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig
On Sun, Jul 13, 2014 at 10:10:27AM -0700, Junio C Hamano wrote: Eric Sunshine sunsh...@sunshineco.com writes: Made parse_sort_string take a var parameter, and if given will only warn about invalid parameter, instead of error. This seems unnecessarily ugly since it's hard-coding specialized knowledge of the callers' error-reporting requirements into what should be a generalized parsing function. If you instead make parse_sort_string() responsible only for attempting to parse the value, but leave error-reporting to the callers, then this ugliness goes away. See below. Yup, you are absolutely right. Thanks for catching my silly. I do not know if it is that silly. The reason we push the error reporting down into reusable library functions, even though it is less flexible or causes us to have quiet flags and such, is that the library function knows more about the specific error. In this case we are just saying your sort specification is not valid, so it is not adding much value, and returning -1 provides enough information for the caller to say that. But would we eventually want to diagnose errors more specifically? For example, in foo:refname, we could complain that foo is not a valid sort function. And in version:bar, we could complain that bar is not a valid sorting atom. We can encode these error types into an enum, but it is often much easier to report them at the time of discovery (e.g., because you have the half-parsed string available that says foo or bar). This is a general problem throughout a lot of our code. In higher level languages you might throw an exception with the error message and let the caller decide how to report it. I wonder if it is too gross to do something like: push_error_handler(collect_errors); /* all calls to error() in will push error text onto a stack */ do_something(); pop_error_handler(); /* traverse the list, calling warning() on each, and clear the stack */ print_errors_as_warnings(); One can imagine print_errors_as_errors, which would be useful when a caller is not sure whether a full operation will succeed (e.g., you try X, then Y, and only when both fail do you report an error). Or a caller which does not call any print_error_* at all (i.e., replacing the quiet flag that many library functions take). I realize that I am reinventing the error-reporting wheel on a sleepy Sunday afternoon without having thought about it much, so there is probably some gotcha or case that makes this ugly, or perhaps it just ends up verbose in practice. But one can dream. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig
On Sun, Jul 13, 2014 at 01:33:56PM -0400, Jeff King wrote:
I realize that I am reinventing the error-reporting wheel on a sleepy
Sunday afternoon without having thought about it much, so there is
probably some gotcha or case that makes this ugly, or perhaps it just
ends up verbose in practice. But one can dream.
Just for fun...
---
diff --git a/builtin/tag.c b/builtin/tag.c
index 5e0744b..0f5be3b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -379,7 +379,10 @@ static int git_tag_config(const char *var, const char
*value, void *cb)
if (!strcmp(var, tag.sort)) {
if (!value)
return config_error_nonbool(var);
- parse_sort_string(value, tag_sort);
+ set_error_routine(collect_errors);
+ if (parse_sort_string(value, tag_sort) 0)
+ print_errors(warning, #{error} in config option
'tag.sort');
+ pop_error_routine();
return 0;
}
diff --git a/git-compat-util.h b/git-compat-util.h
index 9de3180..6bf91a6 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -346,6 +346,11 @@ extern void set_die_routine(NORETURN_PTR void
(*routine)(const char *err, va_lis
extern void set_error_routine(void (*routine)(const char *err, va_list
params));
extern void set_die_is_recursing_routine(int (*routine)(void));
+void pop_error_routine(void);
+void collect_errors(const char *err, va_list params);
+__attribute__((format (printf, 2, 3)))
+void print_errors(void (*func)(const char *, ...), const char *fmt, ...);
+
extern int starts_with(const char *str, const char *prefix);
extern int ends_with(const char *str, const char *suffix);
diff --git a/usage.c b/usage.c
index ed14645..055ccc7 100644
--- a/usage.c
+++ b/usage.c
@@ -57,10 +57,16 @@ static int die_is_recursing_builtin(void)
* (ugh), so keep things static. */
static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) =
usage_builtin;
static NORETURN_PTR void (*die_routine)(const char *err, va_list params) =
die_builtin;
-static void (*error_routine)(const char *err, va_list params) = error_builtin;
static void (*warn_routine)(const char *err, va_list params) = warn_builtin;
static int (*die_is_recursing)(void) = die_is_recursing_builtin;
+struct error_func_list {
+ void (*func)(const char *, va_list);
+ struct error_func_list *next;
+};
+static struct error_func_list default_error_func = { error_builtin };
+static struct error_func_list *error_funcs = default_error_func;
+
void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list
params))
{
die_routine = routine;
@@ -68,7 +74,84 @@ void set_die_routine(NORETURN_PTR void (*routine)(const char
*err, va_list param
void set_error_routine(void (*routine)(const char *err, va_list params))
{
- error_routine = routine;
+ struct error_func_list *efl = xmalloc(sizeof(*efl));
+ efl-func = routine;
+ efl-next = error_funcs;
+ error_funcs = efl;
+}
+
+void pop_error_routine(void)
+{
+ while (error_funcs != default_error_func) {
+ struct error_func_list *efl = error_funcs;
+ error_funcs = efl-next;
+ free(efl);
+ }
+}
+
+struct error_list {
+ struct strbuf buf;
+ struct error_list *next;
+};
+struct error_list *error_list;
+struct error_list **error_list_tail = error_list;
+
+void collect_errors(const char *fmt, va_list params)
+{
+ struct error_list *err = xmalloc(sizeof(*err));
+
+ strbuf_init(err-buf, 0);
+ strbuf_vaddf(err-buf, fmt, params);
+ err-next = NULL;
+ *error_list_tail = err;
+ error_list_tail = err-next;
+}
+
+static void clear_errors(void)
+{
+ while (error_list) {
+ struct error_list *next = error_list-next;
+ strbuf_release(error_list-buf);
+ free(error_list);
+ error_list = next;
+ }
+ error_list_tail = error_list;
+}
+
+void print_errors(void (*func)(const char *, ...), const char *fmt, ...)
+{
+ struct strbuf buf = STRBUF_INIT;
+ va_list params;
+ int prefix_len, suffix_start;
+ const char *p;
+ struct error_list *el;
+
+ va_start(params, fmt);
+ strbuf_vaddf(buf, fmt, params);
+ va_end(params);
+
+ /*
+* The intent here is that callers will put #{error} in their fmt
+* string. Doing two layers of interpolation is gross, because we may
+* accidentally find #{error} in one of the substitutions, not the
+* original fmt. Ideally we would do it all in a single pass (and call
+* #{error} %M or something), but that would require extending
+* vsprintf, and there is no way to do that portably.
+*/
+ p = strstr(buf.buf, #{error});
+ if (!p)
+ die(BUG: error printer does not want to print error!?);
+ prefix_len = p - buf.buf;
+ suffix_start =
Re: [PATCH v7 4/4] cache-tree: Write updated cache-tree after commit
On Sun, Jul 13, 2014 at 1:22 PM, Junio C Hamano gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: + (echo p; echo 1; echo; echo s; echo n; echo y; echo q) | git commit --interactive -m foo Broken -chain. Would a printf make this more readable? printf p\n1\n\ns\nn\ny\nq\n | git commt ... Perhaps not. But printf %s\n p 1 s n y q is vastly more readable, I would think. Yes, that's much nicer (and I should have thought of it myself considering that I made effectively the same suggestion elsewhere [1]). Don't we have test_write_lines which is exactly that, though? Indeed, that's even better. [1]: http://thread.gmane.org/gmane.comp.version-control.git/233260/focus=234499 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] move setting of object-type to alloc_* functions
On 13/07/14 07:41, Jeff King wrote: On Sat, Jul 12, 2014 at 02:05:39PM -0400, Jeff King wrote: I don't particularly like 'flag' here. (not a massive dislike, mind you:) Perhaps: flag-object_type, type-node_type? Or, if that's too verbose, maybe just: flag-type, type-node? Me either, but as you noticed, type was taken. Your suggestions seem fine. We could also just do away with the macro as discussed earlier (we already do in the commit_node case, anyway...). Thinking on this more, writing out the definitions is the only sane thing to do here, now that alloc_commit_node does not use the macro. Otherwise you are inviting people to modify the macro, but fail to notice that the commit allocator also needs updating. Hmm, well I could argue that using the macro for all allocators, apart from alloc_commit_node(), clearly shows which allocator is the odd-man out (and conversely, that all others are the same)! :-P No, I don't think this is a telling advantage; I don't think it makes that much difference. (six of one, half-a-dozen of the other). BTW, I tested the previous series on Linux 32-bit, Cygwin 32-bit, MinGW 32-bit and Cygwin 64-bit. (I can't test on Linux 64-bit, since I can't get Linux installed on my new laptop :( ). Admittedly, the testing on MinGW and Cygwin was only fairly light (it takes *hours* to run the full testsuite, and I just don't have the time). I was slightly concerned, when reading through this new series, that the alloc_node() function may no longer be inlined in the new allocators. However, I have just tested on Linux (only using gcc this time), and it was just fine. I will test the new series on the above systems later (probably tomorrow) but don't expect to find any problems. Here's a re-roll. The interesting bit is the addition of the second patch (but the rest needed to be rebased on top). Yep, this looks good. Thanks! [1/8]: alloc.c: remove the alloc_raw_commit_node() function [2/8]: alloc: write out allocator definitions [3/8]: move setting of object-type to alloc_* functions [4/8]: parse_object_buffer: do not set object type [5/8]: add object_as_type helper for casting objects [6/8]: alloc: factor out commit index [7/8]: object_as_type: set commit index [8/8]: diff-tree: avoid lookup_unknown_object ATB, Ramsay Jones -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 2/4] test-dump-cache-tree: invalid trees are not errors
From: David Turner dtur...@twopensource.com
Do not treat known-invalid trees as errors even when their subtree_nr is
incorrect. Because git already knows that these trees are invalid,
an incorrect subtree_nr will not cause problems.
Add a couple of comments.
Signed-off-by: David Turner dtur...@twitter.com
---
test-dump-cache-tree.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
index 47eab97..cbbbd8e 100644
--- a/test-dump-cache-tree.c
+++ b/test-dump-cache-tree.c
@@ -26,16 +26,16 @@ static int dump_cache_tree(struct cache_tree *it,
return 0;
if (it-entry_count 0) {
+ /* invalid */
dump_one(it, pfx, );
dump_one(ref, pfx, #(ref) );
- if (it-subtree_nr != ref-subtree_nr)
- errs = 1;
}
else {
dump_one(it, pfx, );
if (hashcmp(it-sha1, ref-sha1) ||
ref-entry_count != it-entry_count ||
ref-subtree_nr != it-subtree_nr) {
+ /* claims to be valid but is lying */
dump_one(ref, pfx, #(ref) );
errs = 1;
}
--
2.0.0.390.gcb682f8
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 1/4] cache-tree: Create/update cache-tree on checkout
From: David Turner dtur...@twopensource.com
When git checkout checks out a branch, create or update the
cache-tree so that subsequent operations are faster.
update_main_cache_tree learned a new flag, WRITE_TREE_REPAIR. When
WRITE_TREE_REPAIR is set, portions of the cache-tree which do not
correspond to existing tree objects are invalidated (and portions which
do are marked as valid). No new tree objects are created.
Signed-off-by: David Turner dtur...@twitter.com
---
builtin/checkout.c| 8
cache-tree.c | 12 +++-
cache-tree.h | 1 +
t/t0090-cache-tree.sh | 19 ---
4 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 463cfee..4f08554 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -553,6 +553,14 @@ static int merge_working_tree(const struct checkout_opts
*opts,
}
}
+ if (!active_cache_tree)
+ active_cache_tree = cache_tree();
+
+ if (!cache_tree_fully_valid(active_cache_tree))
+ cache_tree_update(active_cache_tree,
+ (const struct cache_entry * const
*)active_cache,
+ active_nr, WRITE_TREE_SILENT |
WRITE_TREE_REPAIR);
+
if (write_cache(newfd, active_cache, active_nr) ||
commit_locked_index(lock_file))
die(_(unable to write new index file));
diff --git a/cache-tree.c b/cache-tree.c
index 7fa524a..f951d7d 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -239,9 +239,12 @@ static int update_one(struct cache_tree *it,
struct strbuf buffer;
int missing_ok = flags WRITE_TREE_MISSING_OK;
int dryrun = flags WRITE_TREE_DRY_RUN;
+ int repair = flags WRITE_TREE_REPAIR;
int to_invalidate = 0;
int i;
+ assert(!(dryrun repair));
+
*skip_count = 0;
if (0 = it-entry_count has_sha1_file(it-sha1))
@@ -374,7 +377,14 @@ static int update_one(struct cache_tree *it,
#endif
}
- if (dryrun)
+ if (repair) {
+ unsigned char sha1[20];
+ hash_sha1_file(buffer.buf, buffer.len, tree_type, sha1);
+ if (has_sha1_file(sha1))
+ hashcpy(it-sha1, sha1);
+ else
+ to_invalidate = 1;
+ } else if (dryrun)
hash_sha1_file(buffer.buf, buffer.len, tree_type, it-sha1);
else if (write_sha1_file(buffer.buf, buffer.len, tree_type, it-sha1)) {
strbuf_release(buffer);
diff --git a/cache-tree.h b/cache-tree.h
index f1923ad..666d18f 100644
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -39,6 +39,7 @@ int update_main_cache_tree(int);
#define WRITE_TREE_IGNORE_CACHE_TREE 2
#define WRITE_TREE_DRY_RUN 4
#define WRITE_TREE_SILENT 8
+#define WRITE_TREE_REPAIR 16
/* error return codes */
#define WRITE_TREE_UNREADABLE_INDEX (-1)
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 6c33e28..98fb1ab 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -44,14 +44,14 @@ test_expect_success 'read-tree HEAD establishes cache-tree'
'
test_expect_success 'git-add invalidates cache-tree' '
test_when_finished git reset --hard; git read-tree HEAD
- echo I changed this file foo
+ echo I changed this file foo
git add foo
test_invalid_cache_tree
'
test_expect_success 'update-index invalidates cache-tree' '
test_when_finished git reset --hard; git read-tree HEAD
- echo I changed this file foo
+ echo I changed this file foo
git update-index --add foo
test_invalid_cache_tree
'
@@ -85,9 +85,22 @@ test_expect_success 'reset --hard without index gives
cache-tree' '
test_shallow_cache_tree
'
-test_expect_failure 'checkout gives cache-tree' '
+test_expect_success 'checkout gives cache-tree' '
+ git tag current
git checkout HEAD^
test_shallow_cache_tree
'
+test_expect_success 'checkout -b gives cache-tree' '
+ git checkout current
+ git checkout -b prev HEAD^
+ test_shallow_cache_tree
+'
+
+test_expect_success 'checkout -B gives cache-tree' '
+ git checkout current
+ git checkout -B prev HEAD^
+ test_shallow_cache_tree
+'
+
test_done
--
2.0.0.390.gcb682f8
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 4/4] cache-tree: Write updated cache-tree after commit
From: David Turner dtur...@twopensource.com
During the commit process, update the cache-tree. Write this updated
cache-tree so that it's ready for subsequent commands.
Add test code which demonstrates that git commit now writes the cache
tree. Make all tests test the entire cache-tree, not just the root
level.
Signed-off-by: David Turner dtur...@twitter.com
---
builtin/commit.c | 18 +++-
t/t0090-cache-tree.sh | 117 +++---
2 files changed, 119 insertions(+), 16 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 461c3b1..fd4e3bc 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -342,6 +342,17 @@ static char *prepare_index(int argc, const char **argv,
const char *prefix,
discard_cache();
read_cache_from(index_lock.filename);
+ if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {
+ fd = open(index_lock.filename, O_WRONLY);
+ if (fd = 0)
+ if (write_cache(fd, active_cache, active_nr)
0)
+ die(_(unable to write index file));
+ else
+ close_lock_file(index_lock);
+ else
+ die(_(unable to write index file));
+ } else
+ warning(_(Failed to update main cache tree));
commit_style = COMMIT_NORMAL;
return index_lock.filename;
@@ -383,8 +394,12 @@ static char *prepare_index(int argc, const char **argv,
const char *prefix,
if (!only !pathspec.nr) {
fd = hold_locked_index(index_lock, 1);
refresh_cache_or_die(refresh_flags);
- if (active_cache_changed) {
+ if (active_cache_changed
+ || !cache_tree_fully_valid(active_cache_tree)) {
update_main_cache_tree(WRITE_TREE_SILENT);
+ active_cache_changed = 1;
+ }
+ if (active_cache_changed) {
if (write_cache(fd, active_cache, active_nr) ||
commit_locked_index(index_lock))
die(_(unable to write new_index file));
@@ -435,6 +450,7 @@ static char *prepare_index(int argc, const char **argv,
const char *prefix,
fd = hold_locked_index(index_lock, 1);
add_remove_files(partial);
refresh_cache(REFRESH_QUIET);
+ update_main_cache_tree(WRITE_TREE_SILENT);
if (write_cache(fd, active_cache, active_nr) ||
close_lock_file(index_lock))
die(_(unable to write new_index file));
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 3a3342e..48c4240 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -8,7 +8,7 @@ cache-tree extension.
. ./test-lib.sh
cmp_cache_tree () {
- test-dump-cache-tree actual
+ test-dump-cache-tree | sed -e '/#(ref)/d' actual
sed s/$_x40/SHA/ actual filtered
test_cmp $1 filtered
}
@@ -16,14 +16,38 @@ cmp_cache_tree () {
# We don't bother with actually checking the SHA1:
# test-dump-cache-tree already verifies that all existing data is
# correct.
-test_shallow_cache_tree () {
- printf SHA (%d entries, 0 subtrees)\n $(git ls-files|wc -l) expect
+generate_expected_cache_tree_rec () {
+ dir=$1${1:+/}
+ parent=$2
+ # ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
+ # We want to count only foo because it's the only direct child
+ subtrees=$(git ls-files|grep /|cut -d / -f 1|uniq)
+ subtree_count=$(echo $subtrees|awk '$1 {++c} END {print c}')
+ entries=$(git ls-files|wc -l)
+ printf SHA $dir (%d entries, %d subtrees)\n $entries
$subtree_count
+ for subtree in $subtrees
+ do
+ cd $subtree
+ generate_expected_cache_tree_rec $dir$subtree $dir ||
return 1
+ cd ..
+ done
+ dir=$parent
+}
+
+generate_expected_cache_tree () {
+ (
+ generate_expected_cache_tree_rec
+ )
+}
+
+test_cache_tree () {
+ generate_expected_cache_tree expect
cmp_cache_tree expect
}
test_invalid_cache_tree () {
printf invalid %s ()\n $@
expect
- test-dump-cache-tree | \
+ test-dump-cache-tree |
sed -n -e s/[0-9]* subtrees// -e '/#(ref)/d' -e '/^invalid /p'
actual
test_cmp expect actual
}
@@ -33,14 +57,14 @@ test_no_cache_tree () {
cmp_cache_tree expect
}
-test_expect_failure 'initial commit has cache-tree' '
+test_expect_success 'initial commit has cache-tree' '
test_commit foo
- test_shallow_cache_tree
+ test_cache_tree
'
test_expect_success 'read-tree HEAD establishes cache-tree' '
git read-tree HEAD
-
[PATCH v9 3/4] cache-tree: subdirectory tests
From: David Turner dtur...@twopensource.com
Add tests to confirm that invalidation of subdirectories neither over-
nor under-invalidates.
Signed-off-by: David Turner dtur...@twitter.com
---
t/t0090-cache-tree.sh | 26 +++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 98fb1ab..3a3342e 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -22,9 +22,10 @@ test_shallow_cache_tree () {
}
test_invalid_cache_tree () {
- echo invalid (0 subtrees) expect
- printf SHA #(ref) (%d entries, 0 subtrees)\n $(git ls-files|wc -l)
expect
- cmp_cache_tree expect
+ printf invalid %s ()\n $@
expect
+ test-dump-cache-tree | \
+ sed -n -e s/[0-9]* subtrees// -e '/#(ref)/d' -e '/^invalid /p'
actual
+ test_cmp expect actual
}
test_no_cache_tree () {
@@ -49,6 +50,25 @@ test_expect_success 'git-add invalidates cache-tree' '
test_invalid_cache_tree
'
+test_expect_success 'git-add in subdir invalidates cache-tree' '
+ test_when_finished git reset --hard; git read-tree HEAD
+ mkdir dirx
+ echo I changed this file dirx/foo
+ git add dirx/foo
+ test_invalid_cache_tree
+'
+
+test_expect_success 'git-add in subdir does not invalidate sibling cache-tree'
'
+ git tag no-children
+ test_when_finished git reset --hard no-children; git read-tree HEAD
+ mkdir dir1 dir2
+ test_commit dir1/a
+ test_commit dir2/b
+ echo I changed this file dir1/a
+ git add dir1/a
+ test_invalid_cache_tree dir1/
+'
+
test_expect_success 'update-index invalidates cache-tree' '
test_when_finished git reset --hard; git read-tree HEAD
echo I changed this file foo
--
2.0.0.390.gcb682f8
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] http: Add Accept-Language header if possible
2014-07-14 1:57 GMT+09:00 Junio C Hamano gits...@pobox.com: If you do not want floating point (and I think we tend to avoid it when we do not need it), you can realize that in your use of 0.1 and 0.01 and 0.001 there is nothing fundamentally floating-point; you can measure how many digits below the two-byte string zero-dot you would want upfront (by counting num_langs), and show an integer counter zero-padded to the left to that width. That would avoid having to even worry about a possible funny case where subtracting 0.01 ten times from 0.1 may not yield zero (or the result of subtracting nine times may not reach 0.01) due to rounding errors accumulating, which was the first thing that came to my mind when I saw your loop. You're right; We don't need floating point numbers. I'll try to fix it. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/14] Add submodule test harness
Jens Lehmann jens.lehm...@web.de writes:
Perhaps squashing this to 7e8e5af9 instead?
Yes please, this is much better than my first attempt.
One thing that I found troubling is the ../../../ three levels up
is hardcoded. Would it be always true for any value of $1? If
the submodule is bound to the superproject at sub/dir/, not at dir/,
for example, would it have to change?
I am not saying that we must support artibrary cases, but if there
is such a limitation in the implementation, people who will use the
helper in their new tests want it at least documented, I think.
t/lib-submodule-update.sh | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index e441b98..fc1da84 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -110,18 +110,23 @@ replace_gitfile_with_git_dir () {
}
# Test that the .git directory in the submodule is unchanged (except for the
-# core.worktree setting, which we temporarily restore). Call this function
-# before test_submodule_content as the latter might write the index file
-# leading to false positive index differences.
+# core.worktree setting, which appears only in $GIT_DIR/modules/$1/config).
+# Call this function before test_submodule_content as the latter might
+# write the index file leading to false positive index differences.
test_git_directory_is_unchanged () {
(
-cd $1
-git config core.worktree ../../../$1
+cd .git/modules/$1
+# does core.worktree point at the right place?
+test $(git config core.worktree) = ../../../$1
+# remove it temporarily before comparing, as
+# $1/.git/config lacks it...
+git config --unset core.worktree
)
diff -r .git/modules/$1 $1/.git
(
-cd $1
-GIT_WORK_TREE=. git config --unset core.worktree
+# ... and then restore.
+cd .git/modules/$1
+git config core.worktree ../../../$1
)
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Kindly reply
My name is Carlos Ramirez, I am lawyer here in Madrid Spain. I want to transfer an abandoned sum of 13.5 Millions USD to your account.50% will be for you. No risk involved. Contact me for more details. Kindly reply me back to my alternative email address ( carlos...@aol.com ) Regards Carlos Ramirez -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Race condition in git push --mirror can cause silent ref rewinding
On 07/02/2014 07:10 PM, Alex Vandiver wrote: On 07/02/2014 06:20 PM, Junio C Hamano wrote: Alex Vandiver a...@chmrr.net writes: [remote github] url = g...@github.com:bestpractical/rt.git fetch = +refs/*:refs/* mirror = yes git push github master^:master must stay a usable way to update the published repository to an arbitrary commit, so if set to mirror, do not pretend that a fetch in reverse has happened during 'git push' will not be a solution to this issue. Hm? I'm confused, as mirror isn't compatible with refspecs: $ git push github master^:master error: --mirror can't be combined with refspecs Just following up on this -- can you clarify your statement about git push github master^:master in light of the fact that --mirror already disallows such? - Alex -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] dir: remove PATH_MAX limitation
Duy Nguyen pclo...@gmail.com writes: [01/20] dir.c: coding style fix [02/20] dir.h: move struct exclude declaration to top level [03/20] prep_exclude: remove the artificial PATH_MAX limit ...perhaps with s/if (!dir-basebuf.alloc)/if (!dir-basebuf.buf)/ @Duy any reason for not signing off that series? That series still need a lot more work, but for those first three, if you want to fast track, you have my sign-offs. If it is not too much trouble, can you send only the relevant patches (these three) with sign-off again? I'd prefer to give patches a new chance to be eyeballed by people, and they will tend to have a better chance by being a smaller and an isolated topic. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Kindly reply
My name is Carlos Ramirez, I am lawyer here in Madrid Spain. I want to transfer an abandoned sum of 13.5 Millions USD to your account.50% will be for you. No risk involved. Contact me for more details. Kindly reply me back to my alternative email address ( carlos...@aol.com ) Regards Carlos Ramirez -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 28/32] gc: style change -- no SP before closing bracket
On Wed, Jul 9, 2014 at 2:47 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Wed, Jul 9, 2014 at 3:33 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com
wrote:
Yet, there is a space after the opening '{'. So, this is now
inconsistently formatted as:
{ foo, bar}
So, if we drop the other hunk and keep only the one below,
the patch needs to be retitled with s/bracket/parenthesis/...
@@ -298,7 +298,7 @@ int cmd_gc(int argc, const char **argv, const char
*prefix)
argv_array_pushl(pack_refs_cmd, pack-refs, --all, --prune,
NULL);
argv_array_pushl(reflog, reflog, expire, --all, NULL);
argv_array_pushl(repack, repack, -d, -l, NULL);
- argv_array_pushl(prune, prune, --expire, NULL );
+ argv_array_pushl(prune, prune, --expire, NULL);
argv_array_pushl(rerere, rerere, gc, NULL);
git_config(gc_config, NULL);
--
1.9.1.346.ga2b5940
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 00/31] Support multiple checkouts
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: fd = open(git_path(repos/%s/gitdir, id), O_RDONLY); ... - while (path[len - 1] == '\n' || path[len - 1] == '\r') + while (len (path[len - 1] == '\n' || path[len - 1] == '\r')) len--; Do we anticipate (or even allow/endorse) that end users will edit this file with a random editor? And, to a lessor degree, the same question on the reading side. Do we encourage users to directly read from this file to learn something about their repository? If we are the only tool that writes into this file, and if we are the only tool to be used to read (and use) the contents of this file, I do not see the need to cater to LF vs CRLF line endings. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFH 0/3] stable priority-queue
As Junio and I discussed earlier in [1], this series makes the prio_queue struct stable with respect to object insertion (which in turn means we can use it to replace commit_list in more places). I think everything here is correct, but the second commit fails the final test in t5539. I think the test is just flaky (hence the RFH and cc to Duy). That test creates some unrelated commits in two separate repositories, and then fetches from one to the other. Since the commit creation happens in a subshell, the first commit in each ends up with the same test_tick value. When fetch-pack looks at the two root commits unrelated1 and new-too, the exact sequence of ACKs is different depending on which one it pulls out of the queue first. With the current code, it happens to be unrelated1 (though this is not at all guaranteed by the prio_queue data structure, it is deterministic for this particular sequence of input). We see the ready-ACK, and the test succeeds. With the stable queue, we reliably get new-too out (since it is our local tip, it is added to the queue before we even talk to the remote). We never see a ready-ACK, and the test fails due to the grep on the TRACE_PACKET output at the end (the fetch itself succeeds as expected). I'm really not quite clear on what's supposed to be going on in the test. I can make it pass with: diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh index 94553e1..b461188 100755 --- a/t/t5539-fetch-http-shallow.sh +++ b/t/t5539-fetch-http-shallow.sh @@ -54,6 +54,7 @@ EOF test_expect_success 'no shallow lines after receiving ACK ready' ' ( cd shallow + test_tick for i in $(test_seq 15) do git checkout --orphan unrelated$i which just bumps the timestamp for the unrelated* commits (so that they are always more recent than new-too and get picked first). I'm not sure if that is too hacky, or if there's a more robust way to set up the test. Anyway, here are the patches. [1/3]: prio-queue: factor out compare and swap operations [2/3]: prio-queue: make output stable with respect to insertion [3/3]: paint_down_to_common: use prio_queue -Peff [1] http://thread.gmane.org/gmane.comp.version-control.git/252472/focus=252475 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] prio-queue: factor out compare and swap operations
When manipulating the priority queue's heap, we frequently
have to compare and swap heap entries. As we are storing
only void pointers right now, this is quite easy to do
inline in a few lines. However, when we start using a more
complicated heap entry in a future patch, that will get
longer. Factoring out these operations lets us make future
changes in one place. It also makes the code a little
shorter and more readable.
Note that we actually accept indices into the queue array
instead of pointers. This is slightly less flexible than
passing pointers-to-pointers (we could not swap items from
unrelated arrays, but we would not want to), but will make
further refactoring simpler (and lets us avoid repeating
queue-array at each callsite, which led to some long
lines).
And finally, note that we are cleaning up an accidental use
of a struct commit pointer to hold a temporary entry
during swap. Even though we currently only use this code for
commits, it is supposed to be type-agnostic. In practice
this didn't matter anyway because we never dereferenced the
commit pointer (and on most systems, the pointer values
themselves are interchangeable between types).
Signed-off-by: Jeff King p...@peff.net
---
prio-queue.c | 49 +
1 file changed, 25 insertions(+), 24 deletions(-)
diff --git a/prio-queue.c b/prio-queue.c
index c9f8c6d..0f4fcf2 100644
--- a/prio-queue.c
+++ b/prio-queue.c
@@ -1,18 +1,28 @@
#include cache.h
-#include commit.h
#include prio-queue.h
+static inline int compare(struct prio_queue *queue, int i, int j)
+{
+ int cmp = queue-compare(queue-array[i], queue-array[j],
+queue-cb_data);
+ return cmp;
+}
+
+static inline void swap(struct prio_queue *queue, int i, int j)
+{
+ void *tmp = queue-array[i];
+ queue-array[i] = queue-array[j];
+ queue-array[j] = tmp;
+}
+
void prio_queue_reverse(struct prio_queue *queue)
{
int i, j;
if (queue-compare != NULL)
die(BUG: prio_queue_reverse() on non-LIFO queue);
- for (i = 0; i = (j = (queue-nr - 1) - i); i++) {
- struct commit *swap = queue-array[i];
- queue-array[i] = queue-array[j];
- queue-array[j] = swap;
- }
+ for (i = 0; i = (j = (queue-nr - 1) - i); i++)
+ swap(queue, i, j);
}
void clear_prio_queue(struct prio_queue *queue)
@@ -25,37 +35,32 @@ void clear_prio_queue(struct prio_queue *queue)
void prio_queue_put(struct prio_queue *queue, void *thing)
{
- prio_queue_compare_fn compare = queue-compare;
int ix, parent;
/* Append at the end */
ALLOC_GROW(queue-array, queue-nr + 1, queue-alloc);
queue-array[queue-nr++] = thing;
- if (!compare)
+ if (!queue-compare)
return; /* LIFO */
/* Bubble up the new one */
for (ix = queue-nr - 1; ix; ix = parent) {
parent = (ix - 1) / 2;
- if (compare(queue-array[parent], queue-array[ix],
- queue-cb_data) = 0)
+ if (compare(queue, parent, ix) = 0)
break;
- thing = queue-array[parent];
- queue-array[parent] = queue-array[ix];
- queue-array[ix] = thing;
+ swap(queue, parent, ix);
}
}
void *prio_queue_get(struct prio_queue *queue)
{
- void *result, *swap;
+ void *result;
int ix, child;
- prio_queue_compare_fn compare = queue-compare;
if (!queue-nr)
return NULL;
- if (!compare)
+ if (!queue-compare)
return queue-array[--queue-nr]; /* LIFO */
result = queue-array[0];
@@ -67,18 +72,14 @@ void *prio_queue_get(struct prio_queue *queue)
/* Push down the one at the root */
for (ix = 0; ix * 2 + 1 queue-nr; ix = child) {
child = ix * 2 + 1; /* left */
- if ((child + 1 queue-nr)
- (compare(queue-array[child], queue-array[child + 1],
-queue-cb_data) = 0))
+ if (child + 1 queue-nr
+ compare(queue, child, child + 1) = 0)
child++; /* use right child */
- if (compare(queue-array[ix], queue-array[child],
- queue-cb_data) = 0)
+ if (compare(queue, ix, child) = 0)
break;
- swap = queue-array[child];
- queue-array[child] = queue-array[ix];
- queue-array[ix] = swap;
+ swap(queue, child, ix);
}
return result;
}
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] prio-queue: make output stable with respect to insertion
If two items are added to a prio_queue and compare equal,
they currently come out in an apparently random order (this
order is deterministic for a particular sequence of
insertions and removals, but does not necessarily match the
insertion order). This makes it unlike using a date-ordered
commit_list, which is one of the main types we would like to
replace with it (because prio_queue does not suffer from
O(n) insertions).
We can make the priority queue stable by keeping an
insertion counter for each element, and using it to break
ties. This does increase the memory usage of the structure
(one int per element), but in practice it does not seem to
affect runtime. A best-of-five git rev-list --topo-order
on linux.git showed less than 1% difference (well within the
run-to-run noise).
In an ideal world, we would offer both stable and unstable
priority queues (the latter to try to maximize performance).
However, given the lack of a measurable performance
difference, it is not worth the extra code.
Signed-off-by: Jeff King p...@peff.net
---
I actually tried implementing a stable queue on top of the existing
prio-queue. However, it was quite a mess. Because prio_queue only stores
one void pointer to a thing, the stable queue has to allocate its own its
(counter,thing) pair. Doing it in a separate array doesn't work (you do
not pop them in insertion order, so you end up with holes). So you end
up storing an extra pointer, _plus_ per-entry malloc overhead.
If we really want to offer a non-stable queue (and I do not think we
do), we should probably just do two totally separate queues, or
implement the whole thing with a run-time element size member (or even
do it in macros).
I use struct assignment in the swap() function below. Do we want to
replace that with a memcpy? Presumably decent compilers can turn that
into the same code anyway, but I find the assignment more readable, and
IIRC it has been around since C89.
prio-queue.c | 15 ++-
prio-queue.h | 8 +++-
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/prio-queue.c b/prio-queue.c
index 0f4fcf2..e4365b0 100644
--- a/prio-queue.c
+++ b/prio-queue.c
@@ -3,14 +3,16 @@
static inline int compare(struct prio_queue *queue, int i, int j)
{
- int cmp = queue-compare(queue-array[i], queue-array[j],
+ int cmp = queue-compare(queue-array[i].data, queue-array[j].data,
queue-cb_data);
+ if (!cmp)
+ cmp = queue-array[i].ctr - queue-array[j].ctr;
return cmp;
}
static inline void swap(struct prio_queue *queue, int i, int j)
{
- void *tmp = queue-array[i];
+ struct prio_queue_entry tmp = queue-array[i];
queue-array[i] = queue-array[j];
queue-array[j] = tmp;
}
@@ -31,6 +33,7 @@ void clear_prio_queue(struct prio_queue *queue)
queue-nr = 0;
queue-alloc = 0;
queue-array = NULL;
+ queue-insertion_ctr = 0;
}
void prio_queue_put(struct prio_queue *queue, void *thing)
@@ -39,7 +42,9 @@ void prio_queue_put(struct prio_queue *queue, void *thing)
/* Append at the end */
ALLOC_GROW(queue-array, queue-nr + 1, queue-alloc);
- queue-array[queue-nr++] = thing;
+ queue-array[queue-nr].ctr = queue-insertion_ctr++;
+ queue-array[queue-nr].data = thing;
+ queue-nr++;
if (!queue-compare)
return; /* LIFO */
@@ -61,9 +66,9 @@ void *prio_queue_get(struct prio_queue *queue)
if (!queue-nr)
return NULL;
if (!queue-compare)
- return queue-array[--queue-nr]; /* LIFO */
+ return queue-array[--queue-nr].data; /* LIFO */
- result = queue-array[0];
+ result = queue-array[0].data;
if (!--queue-nr)
return result;
diff --git a/prio-queue.h b/prio-queue.h
index 9c3cd1f..d030ec9 100644
--- a/prio-queue.h
+++ b/prio-queue.h
@@ -21,11 +21,17 @@
*/
typedef int (*prio_queue_compare_fn)(const void *one, const void *two, void
*cb_data);
+struct prio_queue_entry {
+ unsigned ctr;
+ void *data;
+};
+
struct prio_queue {
prio_queue_compare_fn compare;
+ unsigned insertion_ctr;
void *cb_data;
int alloc, nr;
- void **array;
+ struct prio_queue_entry *array;
};
/*
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] paint_down_to_common: use prio_queue
When we are traversing to find merge bases, we keep our
usual commit_list of commits to process, sorted by their
commit timestamp. As we add each parent to the list, we have
to spend O(width of history) to do the insertion, where
the width of history is the number of simultaneous lines of
development.
If we instead use a heap-based priority queue, we can do
these insertions in O(log width) time. This provides minor
speedups to merge-base calculations (timings in linux.git,
warm cache, best-of-five):
[before]
$ git merge-base HEAD v2.6.12
real0m3.251s
user0m3.148s
sys 0m0.104s
[after]
$ git merge-base HEAD v2.6.12
real0m3.234s
user0m3.108s
sys 0m0.128s
That's only an 0.5% speedup, but it does help protect us
against pathological cases.
While we are munging the interesting function, we also
take the opportunity to give it a more descriptive name, and
convert the return value to an int (we returned the first
interesting commit, but nobody ever looked at it).
Signed-off-by: Jeff King p...@peff.net
---
Same as what I posted a few weeks ago, but now we do not have to tweak
t6024 to account for the lack of stability.
commit.c | 42 +++---
1 file changed, 19 insertions(+), 23 deletions(-)
diff --git a/commit.c b/commit.c
index acb74b5..1fc60c0 100644
--- a/commit.c
+++ b/commit.c
@@ -786,45 +786,41 @@ void sort_in_topological_order(struct commit_list **list,
enum rev_sort_order so
static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT);
-static struct commit *interesting(struct commit_list *list)
+static int queue_has_nonstale(struct prio_queue *queue)
{
- while (list) {
- struct commit *commit = list-item;
- list = list-next;
- if (commit-object.flags STALE)
- continue;
- return commit;
+ int i;
+ for (i = 0; i queue-nr; i++) {
+ struct commit *commit = queue-array[i].data;
+ if (!(commit-object.flags STALE))
+ return 1;
}
- return NULL;
+ return 0;
}
/* all input commits in one and twos[] must have been parsed! */
static struct commit_list *paint_down_to_common(struct commit *one, int n,
struct commit **twos)
{
- struct commit_list *list = NULL;
+ struct prio_queue queue = { compare_commits_by_commit_date };
struct commit_list *result = NULL;
int i;
one-object.flags |= PARENT1;
- commit_list_insert_by_date(one, list);
- if (!n)
- return list;
+ if (!n) {
+ commit_list_append(one, result);
+ return result;
+ }
+ prio_queue_put(queue, one);
+
for (i = 0; i n; i++) {
twos[i]-object.flags |= PARENT2;
- commit_list_insert_by_date(twos[i], list);
+ prio_queue_put(queue, twos[i]);
}
- while (interesting(list)) {
- struct commit *commit;
+ while (queue_has_nonstale(queue)) {
+ struct commit *commit = prio_queue_get(queue);
struct commit_list *parents;
- struct commit_list *next;
int flags;
- commit = list-item;
- next = list-next;
- free(list);
- list = next;
-
flags = commit-object.flags (PARENT1 | PARENT2 | STALE);
if (flags == (PARENT1 | PARENT2)) {
if (!(commit-object.flags RESULT)) {
@@ -843,11 +839,11 @@ static struct commit_list *paint_down_to_common(struct
commit *one, int n, struc
if (parse_commit(p))
return NULL;
p-object.flags |= flags;
- commit_list_insert_by_date(p, list);
+ prio_queue_put(queue, p);
}
}
- free_commit_list(list);
+ clear_prio_queue(queue);
return result;
}
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] move setting of object-type to alloc_* functions
On Sun, Jul 13, 2014 at 08:27:51PM +0100, Ramsay Jones wrote: Thinking on this more, writing out the definitions is the only sane thing to do here, now that alloc_commit_node does not use the macro. Otherwise you are inviting people to modify the macro, but fail to notice that the commit allocator also needs updating. Hmm, well I could argue that using the macro for all allocators, apart from alloc_commit_node(), clearly shows which allocator is the odd-man out (and conversely, that all others are the same)! :-P No, I don't think this is a telling advantage; I don't think it makes that much difference. (six of one, half-a-dozen of the other). Yeah, I agree with your final statement in parentheses. I am OK with it either way (but I have a slight preference for what I posted). I was slightly concerned, when reading through this new series, that the alloc_node() function may no longer be inlined in the new allocators. However, I have just tested on Linux (only using gcc this time), and it was just fine. I will test the new series on the above systems later (probably tomorrow) but don't expect to find any problems. That should not be due to my patches (which are just expanding macros), but rather to your 1/8, right? I do not know that it matters that much anyway. Yes, we allocate a lot of objects in some workloads. But I think it is not so tight a loop that the extra function call is going to kill us (and we tend to _read_ the allocated objects much more than we allocate them). Here's a re-roll. The interesting bit is the addition of the second patch (but the rest needed to be rebased on top). Yep, this looks good. Thanks! Thanks for reviewing, as usual. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
