date:20140711

On Fri, Jul 11, 2014 at 01:59:53AM +0100, Ramsay Jones wrote:

  The code you're touching here was trying to make sure that each commit
  gets a unique index, under the assumption that commits only get
  allocated via alloc_commit_node. But I think that assumption is wrong.
  We can also get commit objects by allocating an OBJ_NONE (e.g., via
  lookup_unknown_object) and then converting it into an OBJ_COMMIT when we
  find out what it is.
 
 Hmm, I don't know how the object is converted, but the object allocator
 is actually allocating an 'union any_object', so it's allocating more
 space than for a struct object anyway.

Right, we would generally want to avoid lookup_unknown_object where we
can for that reason.

 If you add an 'index' field to struct object, (and remove it from
 struct commit) it could be set in alloc_object_node(). ie _all_ node
 types get an index field.

That was something I considered when we did the original commit-slab
work, as it would let you do similar tricks for any set of objects, not
just commits. The reasons against it are:

  1. It would bloat the size of blob and tree structs by at least 4
 bytes (probably 8 for alignment). In most repos, commits make up
 only 10-20% of the total objects (so for linux.git, we're talking
 about 25MB extra in the working set).

  2. It makes single types sparse in the index space. In cases where you
 do just want to keep data on commits (and that is the main use),
 you end up allocating a slab entry per object, rather than per
 commit. That wastes memory (much worse than 25MB if your slab items
 are large), and reduces cache locality.

You could probably get around (2) by splitting the index space by type
and allocating them in pools, but that complicates things considerably,
as you have to guess ahead of time at reasonable maximums for each type.

-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 0/7] ensure index is set for all OBJ_COMMIT objects variable

Here's a series to address the bug I mentioned earlier by catching the
conversion of OBJ_NONE to OBJ_COMMIT in a central location and setting
the index there.

I've included your patch 1/2 unchanged in the beginning, as I build on
top of it (and your patch 2/2 is no longer applicable).  The rest is
refactoring leading up to patch 6 to fix the bug. Patch 7 is a bonus
cleanup.

I'd hoped to cap off the series by converting the type field of
struct commit to a const unsigned type : 3, which would avoid any
new callers being added that would touch it without going through the
proper procedure.  However, it's a bitfield, which makes it hard to cast
the constness away in the actual setter function. My best attempt was to
use a union with matching const and non-const members, but that would
mean changing all of the sites which read the field (and there are many)
to use object-type.read.

There may be a clever solution hiding in a dark corner of C, but I
suspect we are entering a realm of portability problems with older
compilers (I even saw one compiler's documentation claim that const
was forbidden on bitfields, even though C99 has an example which does
it).

  [1/7]: alloc.c: remove the alloc_raw_commit_node() function
  [2/7]: move setting of object-type to alloc_* functions
  [3/7]: parse_object_buffer: do not set object type
  [4/7]: add object_as_type helper for casting objects
  [5/7]: alloc: factor out commit index
  [6/7]: object_as_type: set commit index
  [7/7]: 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 1/7] 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 3/7] 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 2/7] 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 | 18 ++
 blob.c  |  2 +-
 builtin/blame.c |  1 -
 commit.c|  2 +-
 object.c|  5 ++---
 object.h|  2 +-
 tag.c   |  2 +-
 tree.c  |  2 +-
 8 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/alloc.c b/alloc.c
index d7c3605..fd5fcb7 100644
--- a/alloc.c
+++ b/alloc.c
@@ -18,11 +18,11 @@
 
 #define BLOCKING 1024
 
-#define DEFINE_ALLOCATOR(name, type)   \
+#define DEFINE_ALLOCATOR(name, flag, type) \
 static struct alloc_state name##_state;\
 void *alloc_##name##_node(void)\
 {  \
-   return alloc_node(name##_state, sizeof(type)); \
+   return alloc_node(name##_state, flag, sizeof(type));   \
 }
 
 union any_object {
@@ -39,7 +39,8 @@ struct alloc_state {
void *p;   /* first free node in current allocation */
 };
 
-static inline void *alloc_node(struct alloc_state *s, size_t node_size)
+static inline void *alloc_node(struct alloc_state *s, enum object_type type,
+  size_t node_size)
 {
void *ret;
 
@@ -52,20 +53,21 @@ static inline void *alloc_node(struct alloc_state *s, 
size_t node_size)
ret = s-p;
s-p = (char *)s-p + node_size;
memset(ret, 0, node_size);
+   ((struct object *)ret)-type = type;
return ret;
 }
 
-DEFINE_ALLOCATOR(blob, struct blob)
-DEFINE_ALLOCATOR(tree, struct tree)
-DEFINE_ALLOCATOR(tag, struct tag)
-DEFINE_ALLOCATOR(object, union any_object)
+DEFINE_ALLOCATOR(blob, OBJ_BLOB, struct blob)
+DEFINE_ALLOCATOR(tree, OBJ_TREE, struct tree)
+DEFINE_ALLOCATOR(tag, OBJ_TAG, struct tag)
+DEFINE_ALLOCATOR(object, OBJ_NONE, union any_object)
 
 static struct alloc_state commit_state;
 
 void *alloc_commit_node(void)
 {
static int commit_count;
-   struct commit *c = alloc_node(commit_state, sizeof(struct commit));
+   struct commit *c = alloc_node(commit_state, OBJ_COMMIT, sizeof(struct 
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 fb7897c..21ed310 100644
--- a/commit.c
+++ b/commit.c
@@ -63,7 +63,7 @@ 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);
+   return create_object(sha1, c);
}
if (!obj-type)
obj-type = OBJ_COMMIT;
diff --git a/object.c b/object.c
index 9c31e9a..a950b85 100644
--- a/object.c
+++ b/object.c
@@ -141,13 +141,12 @@ static void

[PATCH 4/7] 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 21ed310..dd638de 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)
@@ -65,9 +52,7 @@ struct commit *lookup_commit(const unsigned char *sha1)
struct commit *c = alloc_commit_node();
return create_object(sha1, c);
}
-   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 5/7] 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 fd5fcb7..21f3d81 100644
--- a/alloc.c
+++ b/alloc.c
@@ -64,11 +64,16 @@ DEFINE_ALLOCATOR(object, OBJ_NONE, union any_object)
 
 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, OBJ_COMMIT, sizeof(struct 
commit));
-   c-index = commit_count++;
+   c-index = alloc_commit_index();
return c;
 }
 
diff --git a/cache.h b/cache.h
index df65231..42a5e86 100644
--- a/cache.h
+++ b/cache.h
@@ -1376,6 +1376,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 6/7] object_as_type: set commit index

The point of the index field of struct commit is that
every allocated commit would have a uniquely allocated
value. 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
OBJ_NONE 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

[PATCH 7/7] 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; 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

Re: [PATCH v6 02/10] replace: add --graft option

2014-07-11 Thread Christian Couder

On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano gits...@pobox.com wrote:
 Junio C Hamano gits...@pobox.com writes:


 As the user might expect that a new replace ref was created on success
 (0 exit code), and as we should at least warn if we would create a
 commit that is the same as an existing one,...

 Why is it an event that needs a warning?  I do not buy that as we
 should at least at all.

Here you ask why this event needs a warning...

 Ehh, it came a bit differently from what I meant.  Perhaps s/do not
 buy/do not understand/ is closer to what I think---that is, it is
 not like I with a strong conviction think you are wrong.  It is more
 like I do not understand why you think it needs a warning, meaing
 you would need to explain it better.

 If you say make sure A's parent is B and then you asked the same
 thing again when there already is a replacement in place, that
 should be a no-op.

(When there is already a replacement in place we error out in
replace_object_sha1() unless --force is used. What we are talking
about here is what happens if the replacement commit is the same as
the original commit.)

 Making sure A's parent is B would be an
 idempotent operation, no?  Why not just make sure A's parent is
 already B and report Your wish has been granted to the user?

... and here you say we should report your wish has been granted...

 Why would it be simpler for the user to get an error, inspect the
 situation and realize that his wish has been granted after all?

... but for me reporting to the user your wish has been granted and
warning (or errorring out) saying the new commit would be the same as
the old one are nearly the same thing.

So I wonder what exactly you are not happy with.

Is it the fact that I use the error() function, because it would
prefix the message with fatal: and that would be too scary?

Is it because with error() the exit code would not be 0?

Is it because the message new commit is the same as the old one:
'%s' is too scary or unnecessarily technical by itself?

Is it ok if I just print the message on stderr without warning: or
fatal: in front of it?

By the way, when I say As ... and ..., I think it is just simpler to
error out in this case., I mean that it is simpler from the
developer's point of view, not necessarily for the user.

Of course I am ok with improving things for the user even if it
requires more code/work, but it is difficult to properly do that if I
don't see how I could do it.

Thanks,
Christian.
--
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] http: Add Accept-Language header if possible

2014-07-11 Thread Yi, EungJun

2014-07-09 19:40 GMT+09:00 Peter Krefting pe...@softwolves.pp.se:
 Yi EungJun:


 Example:
  LANGUAGE= - 
  LANGUAGE=ko - Accept-Language: ko; q=1.000, *; q=0.001
  LANGUAGE=ko:en - Accept-Language: ko; q=1.000, en; q=0.999, *; q=0.001


 Avoid adding q=1.000. It is redundant (the default for any unqualified
 language names is 1.0, and additionally there has historically been some
 buggy servers that failed if it was included.

Ok, I'll fix it.



 +   p1 = getenv(LANGUAGE);


 You need a fallback mechanism here to parse all the possible language
 variables. I would use the first one I find of these:

  1. LANGUAGE
  2. LC_ALL
  3. LC_MESSAGES
  4. LANG

 Only LANGUAGE holds a colon-separated list, but the same code can parse
 all of them, just yielding a single entry for the others.

I'll use setlocale(LC_MESSAGES, NULL) as well as getenv(LANGUAGE).



 +   strbuf_add(buf, p1, p2 - p1);


 The tokens are on the form language_COUNTRY.encoding@identifier, whereas
 Accept-Language wants language-COUNTRY, so you need to a) replace _ with
 -, and b) chop off anything following a . or @.


 +   strbuf_addf(buf, ; q=%.3f, q);
 +   q -= 0.001;


 Three decimals seems a bit overkill, but some experimentation might be
 necessary.

I'll use three decimals only if there are 100 or more preferred languages.



 +   strbuf_addstr(buf, *; q=0.001\r\n);


 You should probably also add an explicit en here, if none was already
 included. I've seen some servers break horribly if en isn't included.

I'll send Accept-Language only if there is at least one preferred
language. Is it enough?

Thanks for your review.
--
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

[Bug] data loss with cyclic alternates

2014-07-11 Thread Ephrim Khong


Hi,

git seems to have issues with alternates when cycles are present (repo A 
has B/objects as alternates, B has A/objects as alternates). In such 
cases, gc and repack might delete objects that are present in only one 
of the alternates, leading to data loss.


I understand that this is no big use case, and requires manual editing 
of objects/info/alternates. But for the sake of preventing unneccesary 
data loss, and since I found no warning regarding alternate cycles in 
the documentation, it might make sense to handle such cases properly 
(maybe it's a simple after finding all alternates directories, remove 
duplicates?).


Here is a small test case that adds the object storage of a repository 
as alternate to itsself. After git repack -adl, all objects are deleted.


---
rm -rf test_repo 
mkdir test_repo 
cd test_repo 
git init 
touch a 
git add a 
git commit -m c1 
git repack -adl 
echo $PWD/.git/objects  .git/objects/info/alternates 
echo  re-packing... 
git repack -adl 
echo  git fsck... 
git fsck
---

Output:

---
Initialized empty Git repository in /somewhere/test_repo/.git/
[master (root-commit) ab9e123] c1
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 a
Counting objects: 3, done.
Writing objects: 100% (3/3), done.
Total 3 (delta 0), reused 0 (delta 0)
 re-packing...
Nothing new to pack.
error: refs/heads/master does not point to a valid object!
 git fsck...
Checking object directories: 100% (256/256), done.
Checking object directories: 100% (256/256), done.
error: HEAD: invalid sha1 pointer 1494ec24356cbbbd66e19f22cef762dd83de7f54
error: refs/heads/master does not point to a valid object!
notice: No default references
missing blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
---

Thanks
- Eph
--
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 2/2] alloc.c: remove the redundant commit_count variable

2014-07-11 Thread Ramsay Jones

On 11/07/14 09:32, Jeff King wrote:
 On Fri, Jul 11, 2014 at 01:59:53AM +0100, Ramsay Jones wrote:
 
 The code you're touching here was trying to make sure that each commit
 gets a unique index, under the assumption that commits only get
 allocated via alloc_commit_node. But I think that assumption is wrong.
 We can also get commit objects by allocating an OBJ_NONE (e.g., via
 lookup_unknown_object) and then converting it into an OBJ_COMMIT when we
 find out what it is.

 Hmm, I don't know how the object is converted, but the object allocator
 is actually allocating an 'union any_object', so it's allocating more
 space than for a struct object anyway.
 
 Right, we would generally want to avoid lookup_unknown_object where we
 can for that reason.
 
 If you add an 'index' field to struct object, (and remove it from
 struct commit) it could be set in alloc_object_node(). ie _all_ node
 types get an index field.
 
 That was something I considered when we did the original commit-slab
 work, as it would let you do similar tricks for any set of objects, not
 just commits. The reasons against it are:
 
   1. It would bloat the size of blob and tree structs by at least 4
  bytes (probably 8 for alignment). In most repos, commits make up
  only 10-20% of the total objects (so for linux.git, we're talking
  about 25MB extra in the working set).
 
   2. It makes single types sparse in the index space. In cases where you
  do just want to keep data on commits (and that is the main use),
  you end up allocating a slab entry per object, rather than per
  commit. That wastes memory (much worse than 25MB if your slab items
  are large), and reduces cache locality.
 

Ahem, yeah I keep telling myself not to post at 2am ... ;-)

Although I haven't given this too much extra thought, I'm beginning
to think that your best course would be to revert commit 969eba63
and add an convert_object_to_commit() function to commit.c which
would set c-index. Then track down each place an OBJ_NONE gets
converted to an OBJ_COMMIT and insert a call to
convert_object_to_commit(). (which may do more than just set the
index field ...)

Hmm, I've just noticed a new series in my in-box. I guess I will
discover what you decided to do shortly ... ;-P

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

Re: [PATCH] http: Add Accept-Language header if possible

2014-07-11 Thread Yi, EungJun

2014-07-11 5:10 GMT+09:00 Jeff King p...@peff.net:
 On Wed, Jul 09, 2014 at 11:46:14AM +0100, Peter Krefting wrote:

 Jeff King:

 I did some digging, and I think the public API is setlocale with a NULL
 parameter, like:
 
  printf(%s\n, setlocale(LC_MESSAGES, NULL));
 
 That still will end up like en_US.UTF-8, though;

 And it only yields the highest-priority language, I think.

 I wasn't clear on whether POSIX locale variables actually supported
 multiple languages with priorities. I have never seen that, though the
 original commit message indicated that LANGUAGE=x:y was a thing (I
 wasn't sure if that was a made-up thing, or something that libc actually
 supported).

 Debian's website has a nice writeup on the subject:
 http://www.debian.org/intro/cn#howtoset

 That seems to be about language settings in browsers, which are a much
 richer set of preferences than POSIX locales (I think).

 It would not be wrong to have that level of configuration for git's http
 requests, but I do not know if it is worth the effort. Mapping the
 user's gettext locale into an accept-language header seems like a
 straightforward way to communicate to the other side what the client is
 using to show errors (so that errors coming from the server can match).

Thanks for you advice. I'll write a path to use both of
setlocale(LC_MESSAGES, NULL) and getenv(LANGUAGE) to get the user's
preferred language. setlocale(LC_MESSAGES, NULL) is quite nice way
because it takes LC_ALL, LC_MESSAGES and LANG into account, but not
LANGUAGE. I think we should take also LANGUAGE into account as gettext
does. [1]

[1]: 
http://www.gnu.org/software/gettext/manual/gettext.html#Locale-Environment-Variables
--
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] http: Add Accept-Language header if possible

2014-07-11 Thread Yi EungJun

From: Yi EungJun eungjun...@navercorp.com

Add an Accept-Language header which indicates the user's preferred
languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.

Examples:
  LANGUAGE= - 
  LANGUAGE=ko:en - Accept-Language: ko, en; q=0.9, *; q=0.1
  LANGUAGE=ko LANG=en_US.UTF-8 - Accept-Language: ko, *; q=0.1
  LANGUAGE= LANG=en_US.UTF-8 - Accept-Language: en-US, *; q=0.1

This gives git servers a chance to display remote error messages in
the user's preferred language.

Signed-off-by: Yi EungJun eungjun...@navercorp.com
---
 http.c | 125 +
 remote-curl.c  |   2 +
 t/t5550-http-fetch-dumb.sh |  21 
 3 files changed, 148 insertions(+)

diff --git a/http.c b/http.c
index 3a28b21..a20f3e2 100644
--- a/http.c
+++ b/http.c
@@ -983,6 +983,129 @@ static void extract_content_type(struct strbuf *raw, 
struct strbuf *type,
strbuf_addstr(charset, ISO-8859-1);
 }
 
+/*
+ * Guess the user's preferred languages from the value in LANGUAGE environment
+ * variable and LC_MESSAGES locale category.
+ *
+ * The result can be a colon-separated list like ko:ja:en.
+ */
+static const char* get_preferred_languages() {
+const char* retval;
+
+   retval = getenv(LANGUAGE);
+   if (retval != NULL  retval[0] != '\0')
+   return retval;
+
+   retval = setlocale(LC_MESSAGES, NULL);
+   if (retval != NULL  retval[0] != '\0'
+strcmp(retval, C) != 0
+strcmp(retval, POSIX) != 0)
+   return retval;
+
+   return NULL;
+}
+
+/*
+ * Add an Accept-Language header which indicates user's preferred languages.
+ *
+ * Examples:
+ *   LANGUAGE= - 
+ *   LANGUAGE=ko:en - Accept-Language: ko, en; q=0.9, *; q=0.1
+ *   LANGUAGE=ko_KR.UTF-8:sr@latin - Accept-Language: ko-KR, sr; q=0.9, *; 
q=0.1
+ *   LANGUAGE=ko LANG=en_US.UTF-8 - Accept-Language: ko, *; q=0.1
+ *   LANGUAGE= LANG=en_US.UTF-8 - Accept-Language: en-US, *; q=0.1
+ *   LANGUAGE= LANG=C - 
+ */
+static struct curl_slist* add_accept_language(struct curl_slist *headers)
+{
+   const char *p1, *p2, *p3;
+   struct strbuf buf = STRBUF_INIT;
+   float q = 1.0;
+   float q_precision = 0.1;
+   int num_langs = 1;
+   char* q_format = ; q=%.1f;
+
+   p1 = get_preferred_languages();
+
+   /* Don't add Accept-Language header if no language is preferred. */
+   if (p1 == NULL || p1[0] == '\0') {
+   strbuf_release(buf);
+   return headers;
+   }
+
+   /* Count number of preferred languages to decide precision of q-factor 
*/
+   for (p3 = p1; *p3 != '\0'; p3++) {
+   if (*p3 == ':') {
+   num_langs++;
+   }
+   }
+
+   /* 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;
+   }
+
+   strbuf_addstr(buf, Accept-Language: );
+
+   for (p2 = p1; q  q_precision; p2++) {
+   if ((*p2 == ':' || *p2 == '\0')  p1 != p2) {
+   if (q  1.0) {
+   strbuf_addstr(buf, , );
+   }
+
+   for (p3 = p1; p3  p2; p3++) {
+   /* Replace '_' with '-'. */
+   if (*p3 == '_') {
+   strbuf_add(buf, p1, p3 - p1);
+   strbuf_addstr(buf, -);
+   p1 = p3 + 1;
+   }
+
+   /* Chop off anything after '.' or '@'. */
+   if ((*p3 == '.' || *p3 == '@')) {
+   break;
+   }
+   }
+
+   if (p3  p1) {
+   strbuf_add(buf, p1, p3 - p1);
+   }
+
+   /* Put the q factor if only it is less than 1.0. */
+   if (q  1.0) {
+   strbuf_addf(buf, q_format, q);
+   }
+
+   q -= q_precision;
+   p1 = p2 + 1;
+
+   if (*p2 == '\0') {
+   break;
+   }
+   }
+   }
+
+   /* Don't add Accept-Language header if no language is preferred. */
+   if (q = 1.0) {
+   strbuf_release(buf);
+   return headers;
+   }
+
+   /* Add '*' with minimum q-factor greater than 0.0. */
+   strbuf_addstr(buf, , *);
+   strbuf_addf(buf, q_format, q_precision);
+
+   headers = curl_slist_append(headers, buf.buf);
+
+

pitfall with empty commits during git rebase

2014-07-11 Thread Olaf Hering


There is an incorrect message when doing git rebase -i remote/branch.
I have it only in german, see below. what happend is:

#01 make changes on another host
#02 copy patchfile to localhost
#03 apply patchfile
#04 git commit -avs # create commit#1
#05 sleep 123456
#06 make different changes on another host
#07 copy patchfile to localhost
#08 git show | patch -Rp1
#09 git commit -avs # create commit#2
#10 apply patchfile
#11 git commit -avs # create commit#3
#12 git rebase -i remote/branch
 pick commit#1 msg
 fcommit#2 msg
 fcommit#3 msg


.git/rebase-merge/git-rebase-todo 21L, 707C geschrieben
Sie fragten den jüngsten Commit nachzubessern, aber das würde diesen leer
machen. Sie können Ihr Kommando mit --allow-empty wiederholen, oder diesen
Commit mit git reset HEAD^ vollständig entfernen.
Rebase im Gange; auf c105589
Sie sind gerade beim Rebase von Branch 'mybranch' auf 'c105589'.

Keine Änderungen

Could not apply 6c5842320acc797d395afb5cdf373c2bfaebfa34... revert


Its not clear what '--allow-empty' refers to, git rebase does not seem to
understand this option.

I should have skipped step #09 to avoid the trouble.
git version is 2.0.1. Please adjust the error msg above.


Olaf
--
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 0/7] ensure index is set for all OBJ_COMMIT objects variable

2014-07-11 Thread Ramsay Jones

On 11/07/14 09:41, Jeff King wrote:
 Here's a series to address the bug I mentioned earlier by catching the
 conversion of OBJ_NONE to OBJ_COMMIT in a central location and setting
 the index there.
 
 I've included your patch 1/2 unchanged in the beginning, as I build on
 top of it (and your patch 2/2 is no longer applicable).  The rest is
 refactoring leading up to patch 6 to fix the bug. Patch 7 is a bonus
 cleanup.

I have just read this series in my email client (I will apply and test
them later), but this looks very good to me. :)

Only one patch gave me slight pause; see later.

 
 I'd hoped to cap off the series by converting the type field of
 struct commit to a const unsigned type : 3, which would avoid any
 new callers being added that would touch it without going through the
 proper procedure.  However, it's a bitfield, which makes it hard to cast
 the constness away in the actual setter function. My best attempt was to
 use a union with matching const and non-const members, but that would
 mean changing all of the sites which read the field (and there are many)
 to use object-type.read.
 
 There may be a clever solution hiding in a dark corner of C, but I
 suspect we are entering a realm of portability problems with older
 compilers (I even saw one compiler's documentation claim that const
 was forbidden on bitfields, even though C99 has an example which does
 it).

Yes, I've come across such compilers too; I wouldn't go there! ;-P

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

Re: [PATCH 4/7] add object_as_type helper for casting objects

2014-07-11 Thread Ramsay Jones

On 11/07/14 09:48, Jeff King wrote:

[snip]

 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 = lookup_object(sha1);
 diff --git a/object.h b/object.h
 index 8020ace..5e8d8ee 100644
 --- a/object.h
 +++ b/object.h
 @@ -81,6 +81,8 @@ struct object *lookup_object(const unsigned char *sha1);
  
  extern void *create_object(const unsigned char *sha1, void *obj);
  
 +void *object_as_type(struct object *obj, enum object_type type, int quiet);
 +
  /*
   * Returns the object, having parsed it to find out what it is.
   *
 diff --git a/refs.c b/refs.c
 index 20e2bf1..5a18e2d 100644
 --- a/refs.c
 +++ b/refs.c
 @@ -1729,9 +1729,8 @@ static enum peel_status peel_object(const unsigned char 
 *name, unsigned char *sh
  
   if (o-type == OBJ_NONE) {
   int type = sha1_object_info(name, NULL);
 - if (type  0)
 + if (type  0 || !object_as_type(o, type, 0))
^^^

It is not possible here for object_as_type() to issue an error()
report, but I had to go look at the code to check. (It would have
been a change in behaviour, otherwise). So, it doesn't really matter
what you pass to the quiet argument, but setting it to 1 _may_ help
the next reader. (No, I'm not convinced either; the only reason I
knew it had anything to do with error messages was because I had
just read the code ...) Hmm, dunno.

   return PEEL_INVALID;
 - o-type = type;
   }
  
   if (o-type != OBJ_TAG)
 diff --git a/tag.c b/tag.c
 index 79552c7..82d841b 100644
 --- a/tag.c
 +++ b/tag.c
 @@ -41,14 +41,7 @@ struct tag *lookup_tag(const unsigned char *sha1)
   struct object *obj = lookup_object(sha1);
   if (!obj)
   return create_object(sha1, alloc_tag_node());
 - if (!obj-type)
 - obj-type = OBJ_TAG;
 - if (obj-type != OBJ_TAG) {
 - error(Object %s is a %s, not a tag,
 -   sha1_to_hex(sha1), typename(obj-type));
 - return NULL;
 - }
 - return (struct tag *) obj;
 + return object_as_type(obj, OBJ_TAG, 0);
  }
  
  static unsigned long parse_tag_date(const char *buf, const char *tail)
 diff --git a/tree.c b/tree.c
 index ed66575..bb02c1c 100644
 --- a/tree.c
 +++ b/tree.c
 @@ -184,14 +184,7 @@ struct tree *lookup_tree(const unsigned char *sha1)
   struct object *obj = lookup_object(sha1);
   if (!obj)
   return create_object(sha1, alloc_tree_node());
 - if (!obj-type)
 - obj-type = OBJ_TREE;
 - if (obj-type != OBJ_TREE) {
 - error(Object %s is a %s, not a tree,
 -   sha1_to_hex(sha1), typename(obj-type));
 - return NULL;
 - }
 - return (struct tree *) obj;
 + return object_as_type(obj, OBJ_TREE, 0);
  }
  
  int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size)
 

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

git pull crash

2014-07-11 Thread Ronald Bos


Hi all,

I am running git on Windows 8.1 (with all the latest updates installed), 
and it consequently crashes when I run git pull in my cloned working copy.


I attached a screen shot of the message window (it is in Dutch...)

This is my git version:
$ git --version
git version 1.9.4.msysgit.0

Is this a known problem? Is there something I can do to help find out 
what is causing the problem?


Regards,

Ronald

Congratulations

2014-07-11 Thread Qatar Charity

wrote to:

Dear Beneficiary,

This is to inform you that you have been awarded the sum of (USD$1,200,000.00) 
as charity donations/aid from the Qatar Foundation, held on 7th of July 2014 in 
Qatar. Reply for more information via e-mail: qatarharit...@gmail.com

Yours Sincerely,
Sheikh Saad Al Muhannadi.
Contact us via: qatarharit...@gmail.com
--
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: git pull crash

2014-07-11 Thread Torsten Bögershausen

On 2014-07-11 12.49, Ronald Bos wrote:
 Hi all,
 
 I am running git on Windows 8.1 (with all the latest updates installed), and 
 it consequently crashes when I run git pull in my cloned working copy.
 
 I attached a screen shot of the message window (it is in Dutch...)
 
 This is my git version:
 $ git --version
 git version 1.9.4.msysgit.0
 
 Is this a known problem? 
I don't know. It may that your repo is triggering a bug which has been fixed
on a later base-line, or it is unfixed because nobody had the very same problem 
yet.
Is there something I can do to help find out what is causing the problem?
Try to send what the screen shot does in text, after translating it into 
english.
Not everybody understands Dutch (or wants to open attachments)


I can think about 2 different things:
- Back up the repo into which you want to pull into, so that the scanrio can be 
re-run under the same condition,
  But with a different version of msysGit.
- Download the source for msysGit from here: http://msysgit.github.io/
  Compile and test, if the bug is on the latest version, or if it is gone.
  (I remember that some crashes had been fixed, but not all the details)
And/or
- If the repo you try to pull from is public, can you give us the URL ?
  If yes then some helpful people may be able to re-produce it.
  If no, is there a dummy (or better say test) repo, which is public and causes 
git to crash ?

Anyway, I would suggest to compile and re-test with the latest version of 
msysGit,
and let us know the results.
  
--
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 07/19] rebase -i: The replay of root commits is not shown with --verbose

2014-07-11 Thread Fabian Ruch

Hi Chris,

you're the original author of the code touched by this patch. Is the
second -q option really a simple copy-and-paste of the first or am I
overlooking something here? I'd like to confirm this as, in retrospect,
I feel a bit uncertain about the hasty claim in the log message.

Kind regards,
   Fabian

Fabian Ruch writes:
 The command line used to recreate root commits specifies the
 erroneous option `-q` which suppresses the commit summary message.
 However, git-rebase--interactive tends to tell the user about the
 commits it creates, if she wishes (cf. command line option
 `--verbose`). The code parts handling non-root commits or squash
 commits all output commit summary messages. Do not make the replay of
 root commits an exception. Remove the option.
 
 It is OK to suppress the commit summary when git-commit is used to
 initialize the authorship of the sentinel commit because the
 existence of this additional commit is a detail of
 git-rebase--interactive's implementation. The option `-q` was
 probably introduced as a copy-and-paste error stemming from that part
 of the root commit handling code.
 
 Signed-off-by: Fabian Ruch baf...@gmail.com
 ---
  git-rebase--interactive.sh | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
 index 0af96f2..ff04d5d 100644
 --- a/git-rebase--interactive.sh
 +++ b/git-rebase--interactive.sh
 @@ -511,7 +511,7 @@ do_pick () {
  --no-post-rewrite -n -q -C $1 
   pick_one -n $1 
   git commit --allow-empty \
 ---amend --no-post-rewrite -n -q -C $1 \
 +--amend --no-post-rewrite -n -C $1 \
  ${gpg_sign_opt:+$gpg_sign_opt} ||
   die_with_patch $1 Could not apply $1... $2
   else
--
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 v8 1/2] add `config_set` API for caching config-like files

Hi,

I had a closer look at error management (once more, sorry: I should have
done this earlier...), and it seems to me that:

* Not all errors are managed properly

* Most error cases are untested

Among the cases I can think of:

* Syntax error when parsing the file

* Non-existant file

* Unreadable file (chmod -r)

Tanay Abhra tanay...@gmail.com writes:

 +`int git_configset_add_file(struct config_set *cs, const char *filename)`::
 +
 + Parses the file and adds the variable-value pairs to the `config_set`,
 + dies if there is an error in parsing the file.

The return value is undocumented.

If I read correctly, the only codepath from this to a syntax error sets
die_on_error, hence dies if there is an error in parsing the file is
correct.

Still, there are errors like unreadable file or no such file that do
not die (nor emit any error message, which is not very good for the
user), and lead to returning -1 here.

I'm not sure this distinction is right (why die on syntax error and
continue running on unreadable file?).

In any case, it should be documented and tested. I'll send a fixup patch
with a few more example tests (probably insufficient).

 +static int git_config_check_init(void)
 +{
 + int ret = 0;
 + if (the_config_set.hash_initialized)
 + return 0;
 + configset_init(the_config_set);
 + ret = git_config(config_hash_callback, the_config_set);
 + if (ret = 0)
 + return 0;
 + else {
 + hashmap_free(the_config_set.config_hash, 1);
 + the_config_set.hash_initialized = 0;
 + return -1;
 + }
 +}

We have the same convention for errors here. But a more serious issue is
that the return value of this function is ignored most of the time.

It seems git_config should never return a negative value, as it calls
git_config_with_options - git_config_early, which checks for file
existance and permission before calling git_config_from_file. Indeed,
Git's tests still pass after this:

--- a/config.c
+++ b/config.c
@@ -1225,7 +1225,10 @@ int git_config_with_options(config_fn_t fn, void *data,
 
 int git_config(config_fn_t fn, void *data)
 {
-   return git_config_with_options(fn, data, NULL, 1);
+   int ret = git_config_with_options(fn, data, NULL, 1);
+   if (ret  0)
+   die(Negative return value in git_config);
+   return ret;
 }

Still, we can imagine cases like race condition between access_or_die()
and git_config_from_file() in

if (git_config_system()  !access_or_die(git_etc_gitconfig(), R_OK, 
0)) {
ret += git_config_from_file(fn, git_etc_gitconfig(),
data);
found += 1;
}

where the function would indeed return -1. In any case, either we
consider that git_config should never return -1, and we should die in
this case, or we consider that it may happen, and that the else branch
of the if/else above is not dead code, and then we can't just ignore the
return value.

I think we should just do something like this:

diff --git a/config.c b/config.c
index 74adbbd..5c023e8 100644
--- a/config.c
+++ b/config.c
@@ -1428,7 +1428,7 @@ int git_configset_get_pathname(struct config_set *cs, 
const char *key, const cha
return 1;
 }
 
-static int git_config_check_init(void)
+static void git_config_check_init(void)
 {
int ret = 0;
if (the_config_set.hash_initialized)
@@ -1437,11 +1437,8 @@ static int git_config_check_init(void)
ret = git_config(config_hash_callback, the_config_set);
if (ret = 0)
return 0;
-   else {
-   hashmap_free(the_config_set.config_hash, 1);
-   the_config_set.hash_initialized = 0;
-   return -1;
-   }
+   else
+   die(Unknown error when parsing one of the configuration 
files.);
 }
 
If not, a comment should explain what the else branch corresponds to,
and why/when the return value can be safely ignored.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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 02/10] replace: add --graft option

Christian Couder christian.cou...@gmail.com writes:

 On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano gits...@pobox.com wrote:

 Making sure A's parent is B would be an
 idempotent operation, no?  Why not just make sure A's parent is
 already B and report Your wish has been granted to the user?

 ... and here you say we should report your wish has been granted...

Normal way for git replace to report that is to exit with status 0
and without any noise, I would think.

 Why would it be simpler for the user to get an error, inspect the
 situation and realize that his wish has been granted after all?

 ... but for me reporting to the user your wish has been granted and
 warning (or errorring out) saying the new commit would be the same as
 the old one are nearly the same thing.

 So I wonder what exactly you are not happy with.

See above.
--
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 v8 2/2] test-config: add tests for the config_set API

Tanay Abhra tanay...@gmail.com writes:

 diff --git a/test-config.c b/test-config.c
 new file mode 100644
 index 000..dc313c2
 --- /dev/null
 +++ b/test-config.c

 +int main(int argc, char **argv)
 +{
 + int i, val;
 + const char *v;
 + const struct string_list *strptr;
 + struct config_set cs;
 + git_configset_init(cs);

The configset is initialized, but never cleared.

As a result, valgrind --leak-check=full complains with definitely lost
items.

I think it would make sense to apply something like this to get a
valgrind-clean test-config.c (I checked, it now passes without
warnings):

diff --git a/test-config.c b/test-config.c
index dc313c2..07b61ef 100644
--- a/test-config.c
+++ b/test-config.c
@@ -41,17 +41,17 @@ int main(int argc, char **argv)
 
if (argc  2) {
fprintf(stderr, Please, provide a command name on the 
command-line\n);
-   return 1;
+   goto exit1;
} else if (argc == 3  !strcmp(argv[1], get_value)) {
if (!git_config_get_value(argv[2], v)) {
if (!v)
printf((NULL)\n);
else
printf(%s\n, v);
-   return 0;
+   goto exit0;

[...]
 
fprintf(stderr, %s: Please check the syntax and the function name\n, 
argv[0]);
+   goto exit1;
+
+exit0:
+   git_configset_clear(cs);
+   return 0;
+
+exit1:
+   git_configset_clear(cs);
return 1;
+
+exit2:
+   git_configset_clear(cs);
+   return 2;
 }

I'll resend as a proper git am-able patch right after.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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/2] Better tests for error cases

Signed-off-by: Matthieu Moy matthieu@imag.fr
---
Consider squashing this into PATCH 2/2

Probably not sufficient.

 t/t1308-config-set.sh | 22 ++
 test-config.c |  8 ++--
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 87a29f1..f1e9e76 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -167,4 +167,26 @@ test_expect_success 'find value_list for a key from a 
configset' '
test_cmp expect actual
 '
 
+test_expect_success 'proper error on non-existant files' '
+   echo Error reading configuration file non-existant-file. expect 
+   test_must_fail test-config configset_get_value foo.bar 
non-existant-file 2actual 
+   test_cmp expect actual
+'
+
+test_expect_success 'proper error on error in default config files' '
+   cp .git/config .git/config.old 
+   test_when_finished mv .git/config.old .git/config 
+   echo [  .git/config 
+   echo fatal: bad config file line 35 in .git/config expect 
+   test_must_fail test-config get_value foo.bar 2actual 
+   test_cmp expect actual
+'
+
+test_expect_success 'proper error on error in custom config files' '
+   echo [  syntax-error 
+   echo fatal: bad config file line 1 in syntax-error expect 
+   test_must_fail test-config configset_get_value foo.bar syntax-error 
2actual 
+   test_cmp expect actual
+'
+
 test_done
diff --git a/test-config.c b/test-config.c
index 07b61ef..49f8cd7 100644
--- a/test-config.c
+++ b/test-config.c
@@ -86,8 +86,10 @@ int main(int argc, char **argv)
}
} else if (!strcmp(argv[1], configset_get_value)) {
for (i = 3; i  argc; i++) {
-   if (git_configset_add_file(cs, argv[i]))
+   if (git_configset_add_file(cs, argv[i])) {
+   fprintf(stderr, Error reading configuration 
file %s.\n, argv[i]);
goto exit2;
+   }
}
if (!git_configset_get_value(cs, argv[2], v)) {
if (!v)
@@ -101,8 +103,10 @@ int main(int argc, char **argv)
}
} else if (!strcmp(argv[1], configset_get_value_multi)) {
for (i = 3; i  argc; i++) {
-   if (git_configset_add_file(cs, argv[i]))
+   if (git_configset_add_file(cs, argv[i])) {
+   fprintf(stderr, Error reading configuration 
file %s.\n, argv[i]);
goto exit2;
+   }
}
strptr = git_configset_get_value_multi(cs, argv[2]);
if (strptr) {
-- 
2.0.0.262.gdafc651

--
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

[fixup PATCH 1/2] Call configset_clear

Signed-off-by: Matthieu Moy matthieu@imag.fr
---
Consider squashing this into PATCH 2/2.

 test-config.c | 42 +++---
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/test-config.c b/test-config.c
index dc313c2..07b61ef 100644
--- a/test-config.c
+++ b/test-config.c
@@ -41,17 +41,17 @@ int main(int argc, char **argv)
 
if (argc  2) {
fprintf(stderr, Please, provide a command name on the 
command-line\n);
-   return 1;
+   goto exit1;
} else if (argc == 3  !strcmp(argv[1], get_value)) {
if (!git_config_get_value(argv[2], v)) {
if (!v)
printf((NULL)\n);
else
printf(%s\n, v);
-   return 0;
+   goto exit0;
} else {
printf(Value not found for \%s\\n, argv[2]);
-   return 1;
+   goto exit1;
}
} else if (argc == 3  !strcmp(argv[1], get_value_multi)) {
strptr = git_config_get_value_multi(argv[2]);
@@ -63,46 +63,46 @@ int main(int argc, char **argv)
else
printf(%s\n, v);
}
-   return 0;
+   goto exit0;
} else {
printf(Value not found for \%s\\n, argv[2]);
-   return 1;
+   goto exit1;
}
} else if (argc == 3  !strcmp(argv[1], get_int)) {
if (!git_config_get_int(argv[2], val)) {
printf(%d\n, val);
-   return 0;
+   goto exit0;
} else {
printf(Value not found for \%s\\n, argv[2]);
-   return 1;
+   goto exit1;
}
} else if (argc == 3  !strcmp(argv[1], get_bool)) {
if (!git_config_get_bool(argv[2], val)) {
printf(%d\n, val);
-   return 0;
+   goto exit0;
} else {
printf(Value not found for \%s\\n, argv[2]);
-   return 1;
+   goto exit1;
}
} else if (!strcmp(argv[1], configset_get_value)) {
for (i = 3; i  argc; i++) {
if (git_configset_add_file(cs, argv[i]))
-   return 2;
+   goto exit2;
}
if (!git_configset_get_value(cs, argv[2], v)) {
if (!v)
printf((NULL)\n);
else
printf(%s\n, v);
-   return 0;
+   goto exit0;
} else {
printf(Value not found for \%s\\n, argv[2]);
-   return 1;
+   goto exit1;
}
} else if (!strcmp(argv[1], configset_get_value_multi)) {
for (i = 3; i  argc; i++) {
if (git_configset_add_file(cs, argv[i]))
-   return 2;
+   goto exit2;
}
strptr = git_configset_get_value_multi(cs, argv[2]);
if (strptr) {
@@ -113,13 +113,25 @@ int main(int argc, char **argv)
else
printf(%s\n, v);
}
-   return 0;
+   goto exit0;
} else {
printf(Value not found for \%s\\n, argv[2]);
-   return 1;
+   goto exit1;
}
}
 
fprintf(stderr, %s: Please check the syntax and the function name\n, 
argv[0]);
+   goto exit1;
+
+exit0:
+   git_configset_clear(cs);
+   return 0;
+
+exit1:
+   git_configset_clear(cs);
return 1;
+
+exit2:
+   git_configset_clear(cs);
+   return 2;
 }
-- 
2.0.0.262.gdafc651

--
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 v4] tag: support configuring --sort via .gitconfig

Jacob Keller jacob.e.kel...@intel.com writes:

 Add support for configuring default sort ordering for git tags. Command
 line option will override this configured value, using the exact same
 syntax.

 Cc: Jeff King p...@peff.net
 Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
 ---
 - v4
 * base on top of suggested change by Jeff King to use skip_prefix instead

  Documentation/config.txt  |  6 ++
  Documentation/git-tag.txt |  1 +
  builtin/tag.c | 46 --
  t/t7004-tag.sh| 21 +
  4 files changed, 60 insertions(+), 14 deletions(-)

 diff --git a/Documentation/config.txt b/Documentation/config.txt
 index 1d718bdb9662..ad8e75fed988 100644
 --- a/Documentation/config.txt
 +++ b/Documentation/config.txt
 @@ -2354,6 +2354,12 @@ submodule.name.ignore::
   --ignore-submodules option. The 'git submodule' commands are not
   affected by this setting.
  
 +tag.sort::
 + This variable is used to control the sort ordering of tags. It is
 + interpreted precisely the same way as --sort=value. If --sort is
 + given on the command line it will override the selection chosen in the
 + configuration. See linkgit:git-tag[1] for more details.
 +

This is not technically incorrect per-se, but the third sentence
talks about --sort on the command line while this applies only
to the command line of the 'git tag' command and nobody else's
--sort option.

Perhaps rephrasing it like this may be easier to understand?

When git tag command is used to list existing tags,
without --sort=value option on the command line,
the value of this variable is used as the default.

This way, it would be clear, without explicitly saying anything,
that the value will be spelled exactly the same way as you would
spell the value for the --sort option on the command line.

 diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
 index b424a1bc48bb..2d246725aeb5 100644
 --- a/Documentation/git-tag.txt
 +++ b/Documentation/git-tag.txt
 @@ -317,6 +317,7 @@ include::date-formats.txt[]
  SEE ALSO
  
  linkgit:git-check-ref-format[1].
 +linkgit:git-config[1].

It is not particularly friendly to readers to refer to
git-config[1] from any other page, if done without spelling out
which variable the reader is expected to look up.  Some addition
to the description of the --sort option is needed if this SEE ALSO
were to be any useful, I guess?

--sort=type::
... (existing description) ...
When this option is not given, the sort order
defaults to the value configured for the `tag.sort`
variable, if exists, or lexicographic otherwise.

or something like that, perhaps?

 diff --git a/builtin/tag.c b/builtin/tag.c
 index 7ccb6f3c581b..a53e27d4e7e4 100644
 --- a/builtin/tag.c
 +++ b/builtin/tag.c
 @@ -18,6 +18,8 @@
  #include sha1-array.h
  #include column.h
  
 +static int tag_sort = 0;

Please do not initialize variables in bss segment to 0 by hand.

If this variable is meant to take one of these *CMP_SORT values
defined as macro later in this file, it is better to define this
variable somewhere after and close to the definitions of the macros.
Perhaps immediately after the struct tag_filter is declared?

 @@ -346,9 +348,33 @@ static const char tag_template_nocleanup[] =
   Lines starting with '%c' will be kept; you may remove them
yourself if you want to.\n);
  
 +static int parse_sort_string(const char *arg)
 +{
 + int sort = 0;
 + int flags = 0;
 +
 + if (skip_prefix(arg, -, arg))
 + flags |= REVERSE_SORT;
 +
 + if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
 + sort = VERCMP_SORT;
 +
 + if (strcmp(arg, refname))
 + die(_(unsupported sort specification %s), arg);

Hmm.  I _thought_ we try to catch unsupported option value coming
from the command line and die but at the same time we try *not* to
die but warn and whatever is sensible when the value comes from the
configuration, so that .git/config or $HOME/.gitconfig can be shared
by those who use different versions of Git.

Do we already have many precedences where we see configuration value
that our version of Git do not yet understand?

Not a very strong objection; just something that worries me.

 + sort |= flags;
 +
 + return sort;
 +}
 +
  static int git_tag_config(const char *var, const char *value, void *cb)
  {
 - int status = git_gpg_config(var, value, cb);
 + int status;
 +
 + if (!strcmp(var, tag.sort)) {
 + tag_sort = parse_sort_string(value);
 + }
 +

Why doesn't this return success after noticing that the variable is
to be interpreted by this block and nobody else?

When there is no reason to have things in a particular order, it is
customary to add new things at the end, not in the front, unless the
new thing is so much more important than

Re: [PATCH 3/4 v6] cache-tree: subdirectory tests

Eric Sunshine sunsh...@sunshineco.com writes:

 On Thu, Jul 10, 2014 at 8:31 PM, David Turner dtur...@twopensource.com 
 wrote:
 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 

Hmm.  This will always expect that the top-level is invalid, even
when $# is 0.  It is OK if you never need to use this to test that a
cache-tree is fully valid, but is it something we want to check?

Existing tests are mostly about cache-tree is populated fully at a
few strategic, well known and easy places and then it degrades over
time, but after all your series is adding more places to that set
of a few places, so we may want to make sure that future breakages
to the new code paths that repair the cache-tree are caught by
these tests.

 +   test-dump-cache-tree | \

 nit: unnecessary backslash

Good eyes ;-)

 +   sed -n -e s/[0-9]* subtrees// -e '/#(ref)/d' -e '/^invalid /p' 
 actual 

Is the second one to remove #(ref), which appears for a good
reference cache tree entry shown for comparison, necessary?  Do
they ever begin with invalid?  If they ever begin with invalid
that itself may even be a noteworthy breakage to catch, no?

 +   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 3/4 v6] cache-tree: subdirectory tests

Junio C Hamano gits...@pobox.com writes:

 +   sed -n -e s/[0-9]* subtrees// -e '/#(ref)/d' -e '/^invalid /p' 
 actual 

 Is the second one to remove #(ref), which appears for a good
 reference cache tree entry shown for comparison, necessary?  Do
 they ever begin with invalid?  If they ever begin with invalid
 that itself may even be a noteworthy breakage to catch, no?

Answering to myself...

Because test-dump-cache-tree uses DRY_RUN to create only an in-core
copy of tree object, and we notice that the reference cache-tree
created in the tests contains the object name of a tree that does
not yet exist in the object database.  We get invalid #(ref) for
such node.

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.


--
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

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.

--
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 v4] tag: support configuring --sort via .gitconfig

On Fri, 2014-07-11 at 08:04 -0700, Junio C Hamano wrote:
 Jacob Keller jacob.e.kel...@intel.com writes:
 
  Add support for configuring default sort ordering for git tags. Command
  line option will override this configured value, using the exact same
  syntax.
 
  Cc: Jeff King p...@peff.net
  Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
  ---
  - v4
  * base on top of suggested change by Jeff King to use skip_prefix instead
 
   Documentation/config.txt  |  6 ++
   Documentation/git-tag.txt |  1 +
   builtin/tag.c | 46 
  --
   t/t7004-tag.sh| 21 +
   4 files changed, 60 insertions(+), 14 deletions(-)
 
  diff --git a/Documentation/config.txt b/Documentation/config.txt
  index 1d718bdb9662..ad8e75fed988 100644
  --- a/Documentation/config.txt
  +++ b/Documentation/config.txt
  @@ -2354,6 +2354,12 @@ submodule.name.ignore::
  --ignore-submodules option. The 'git submodule' commands are not
  affected by this setting.
   
  +tag.sort::
  +   This variable is used to control the sort ordering of tags. It is
  +   interpreted precisely the same way as --sort=value. If --sort is
  +   given on the command line it will override the selection chosen in the
  +   configuration. See linkgit:git-tag[1] for more details.
  +
 
 This is not technically incorrect per-se, but the third sentence
 talks about --sort on the command line while this applies only
 to the command line of the 'git tag' command and nobody else's
 --sort option.
 
 Perhaps rephrasing it like this may be easier to understand?
 
   When git tag command is used to list existing tags,
 without --sort=value option on the command line,
   the value of this variable is used as the default.
 
 This way, it would be clear, without explicitly saying anything,
 that the value will be spelled exactly the same way as you would
 spell the value for the --sort option on the command line.
 

Makes sense.

  diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
  index b424a1bc48bb..2d246725aeb5 100644
  --- a/Documentation/git-tag.txt
  +++ b/Documentation/git-tag.txt
  @@ -317,6 +317,7 @@ include::date-formats.txt[]
   SEE ALSO
   
   linkgit:git-check-ref-format[1].
  +linkgit:git-config[1].
 
 It is not particularly friendly to readers to refer to
 git-config[1] from any other page, if done without spelling out
 which variable the reader is expected to look up.  Some addition
 to the description of the --sort option is needed if this SEE ALSO
 were to be any useful, I guess?
 
   --sort=type::
 ... (existing description) ...
 When this option is not given, the sort order
 defaults to the value configured for the `tag.sort`
 variable, if exists, or lexicographic otherwise.
 
 or something like that, perhaps?
 

Alright, this sounds good too.

  diff --git a/builtin/tag.c b/builtin/tag.c
  index 7ccb6f3c581b..a53e27d4e7e4 100644
  --- a/builtin/tag.c
  +++ b/builtin/tag.c
  @@ -18,6 +18,8 @@
   #include sha1-array.h
   #include column.h
   
  +static int tag_sort = 0;
 
 Please do not initialize variables in bss segment to 0 by hand.
 
 If this variable is meant to take one of these *CMP_SORT values
 defined as macro later in this file, it is better to define this
 variable somewhere after and close to the definitions of the macros.
 Perhaps immediately after the struct tag_filter is declared?
 

I put it just above the struct tag_filter now, as this puts it right
below the #defines regarding it's value.

  @@ -346,9 +348,33 @@ static const char tag_template_nocleanup[] =
  Lines starting with '%c' will be kept; you may remove them
   yourself if you want to.\n);
   
  +static int parse_sort_string(const char *arg)
  +{
  +   int sort = 0;
  +   int flags = 0;
  +
  +   if (skip_prefix(arg, -, arg))
  +   flags |= REVERSE_SORT;
  +
  +   if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
  +   sort = VERCMP_SORT;
  +
  +   if (strcmp(arg, refname))
  +   die(_(unsupported sort specification %s), arg);
 
 Hmm.  I _thought_ we try to catch unsupported option value coming
 from the command line and die but at the same time we try *not* to
 die but warn and whatever is sensible when the value comes from the
 configuration, so that .git/config or $HOME/.gitconfig can be shared
 by those who use different versions of Git.
 
 Do we already have many precedences where we see configuration value
 that our version of Git do not yet understand?
 
 Not a very strong objection; just something that worries me.
 

I like this change, and I think I have a way to handle it. I will
re-work this so that the die is handled by the normal block.

  +   sort |= flags;
  +
  +   return sort;
  +}
  +
   static int git_tag_config(const char *var, const char *value, void *cb)
   {
  -   int status = git_gpg_config(var, value,

Re: [PATCH v6 02/10] replace: add --graft option

2014-07-11 Thread Christian Couder

On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano gits...@pobox.com wrote:
 Christian Couder christian.cou...@gmail.com writes:

 On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano gits...@pobox.com wrote:

 Making sure A's parent is B would be an
 idempotent operation, no?  Why not just make sure A's parent is
 already B and report Your wish has been granted to the user?

 ... and here you say we should report your wish has been granted...

 Normal way for git replace to report that is to exit with status 0
 and without any noise, I would think.

In a similar case git replace --edit we error out instead of just
exiting (with status 0), see:

f22166b5fee7dc (replace: make sure --edit results in a different object)
--
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 v4] tag: support configuring --sort via .gitconfig

Keller, Jacob E jacob.e.kel...@intel.com writes:

 On Fri, 2014-07-11 at 08:04 -0700, Junio C Hamano wrote:
 ...
  +static int tag_sort = 0;
 
 Please do not initialize variables in bss segment to 0 by hand.
 
 If this variable is meant to take one of these *CMP_SORT values
 defined as macro later in this file, it is better to define this
 variable somewhere after and close to the definitions of the macros.
 Perhaps immediately after the struct tag_filter is declared?

 I put it just above the struct tag_filter now, as this puts it right
 below the #defines regarding it's value.

Either would be fine, but just to clarify.

Because these macro definitions are for the .sort field of that
structure, and the new tag_sort variable is the second user of that
macro, my suggestion to put it _after_ was to be in line with add
new things at the end, when there is no compelling reason not to
below.

 When there is no reason to have things in a particular order, it is
 customary to add new things at the end, not in the front, unless the
 new thing is so much more important than everything else---but then
 we are no longer talking about the case where there is no reason to
 have things in a particular order ;-).

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

Re: [PATCH] http: Add Accept-Language header if possible

2014-07-11 Thread Yi, EungJun

2014-07-12 1:24 GMT+09:00 Eric Sunshine sunsh...@sunshineco.com:
 On Fri, Jul 11, 2014 at 5:22 AM, Yi, EungJun semtlen...@gmail.com wrote:
 2014-07-09 6:52 GMT+09:00 Eric Sunshine sunsh...@sunshineco.com:
 +   grep ^Accept-Language: ko; q=1.000, en; q=0.999, \*; q=0.001 
 actual

 Do you want to \-escape the periods? (Or maybe use 'grep -F'?)

 I just want to match '*' character. I tried 'grep -F' but it does not help.

 I meant that the periods in your grep pattern are matching any
 character. If you want to be very strict, so that they match only
 period, then you should \-escape them.

Oops, I misunderstood you. You are right. I'll \- escape them.
--
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 v4] tag: support configuring --sort via .gitconfig

On Fri, 2014-07-11 at 08:04 -0700, Junio C Hamano wrote:
 Jacob Keller jacob.e.kel...@intel.com writes:
 
  Add support for configuring default sort ordering for git tags. Command
  line option will override this configured value, using the exact same
  syntax.
 
  Cc: Jeff King p...@peff.net
  Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
  ---
  - v4
  * base on top of suggested change by Jeff King to use skip_prefix instead
 
   Documentation/config.txt  |  6 ++
   Documentation/git-tag.txt |  1 +
   builtin/tag.c | 46 
  --
   t/t7004-tag.sh| 21 +
   4 files changed, 60 insertions(+), 14 deletions(-)
 
  diff --git a/Documentation/config.txt b/Documentation/config.txt
  index 1d718bdb9662..ad8e75fed988 100644
  --- a/Documentation/config.txt
  +++ b/Documentation/config.txt
  @@ -2354,6 +2354,12 @@ submodule.name.ignore::
  --ignore-submodules option. The 'git submodule' commands are not
  affected by this setting.
   
  +tag.sort::
  +   This variable is used to control the sort ordering of tags. It is
  +   interpreted precisely the same way as --sort=value. If --sort is
  +   given on the command line it will override the selection chosen in the
  +   configuration. See linkgit:git-tag[1] for more details.
  +
 
 This is not technically incorrect per-se, but the third sentence
 talks about --sort on the command line while this applies only
 to the command line of the 'git tag' command and nobody else's
 --sort option.
 
 Perhaps rephrasing it like this may be easier to understand?
 
   When git tag command is used to list existing tags,
 without --sort=value option on the command line,
   the value of this variable is used as the default.
 
 This way, it would be clear, without explicitly saying anything,
 that the value will be spelled exactly the same way as you would
 spell the value for the --sort option on the command line.
 
  diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
  index b424a1bc48bb..2d246725aeb5 100644
  --- a/Documentation/git-tag.txt
  +++ b/Documentation/git-tag.txt
  @@ -317,6 +317,7 @@ include::date-formats.txt[]
   SEE ALSO
   
   linkgit:git-check-ref-format[1].
  +linkgit:git-config[1].
 
 It is not particularly friendly to readers to refer to
 git-config[1] from any other page, if done without spelling out
 which variable the reader is expected to look up.  Some addition
 to the description of the --sort option is needed if this SEE ALSO
 were to be any useful, I guess?
 
   --sort=type::
 ... (existing description) ...
 When this option is not given, the sort order
 defaults to the value configured for the `tag.sort`
 variable, if exists, or lexicographic otherwise.
 
 or something like that, perhaps?
 
  diff --git a/builtin/tag.c b/builtin/tag.c
  index 7ccb6f3c581b..a53e27d4e7e4 100644
  --- a/builtin/tag.c
  +++ b/builtin/tag.c
  @@ -18,6 +18,8 @@
   #include sha1-array.h
   #include column.h
   
  +static int tag_sort = 0;
 
 Please do not initialize variables in bss segment to 0 by hand.
 
 If this variable is meant to take one of these *CMP_SORT values
 defined as macro later in this file, it is better to define this
 variable somewhere after and close to the definitions of the macros.
 Perhaps immediately after the struct tag_filter is declared?
 
  @@ -346,9 +348,33 @@ static const char tag_template_nocleanup[] =
  Lines starting with '%c' will be kept; you may remove them
   yourself if you want to.\n);
   
  +static int parse_sort_string(const char *arg)
  +{
  +   int sort = 0;
  +   int flags = 0;
  +
  +   if (skip_prefix(arg, -, arg))
  +   flags |= REVERSE_SORT;
  +
  +   if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
  +   sort = VERCMP_SORT;
  +
  +   if (strcmp(arg, refname))
  +   die(_(unsupported sort specification %s), arg);
 
 Hmm.  I _thought_ we try to catch unsupported option value coming
 from the command line and die but at the same time we try *not* to
 die but warn and whatever is sensible when the value comes from the
 configuration, so that .git/config or $HOME/.gitconfig can be shared
 by those who use different versions of Git.
 
 Do we already have many precedences where we see configuration value
 that our version of Git do not yet understand?
 
 Not a very strong objection; just something that worries me.
 
  +   sort |= flags;
  +
  +   return sort;
  +}
  +
   static int git_tag_config(const char *var, const char *value, void *cb)
   {
  -   int status = git_gpg_config(var, value, cb);
  +   int status;
  +
  +   if (!strcmp(var, tag.sort)) {
  +   tag_sort = parse_sort_string(value);
  +   }
  +
 
 Why doesn't this return success after noticing that the variable is
 to be interpreted by this block and nobody else?
 
 When there is no reason to have things in

[PATCH v3] http: Add Accept-Language header if possible

2014-07-11 Thread Yi EungJun

Add an Accept-Language header which indicates the user's preferred
languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.

Examples:
  LANGUAGE= - 
  LANGUAGE=ko:en - Accept-Language: ko, en; q=0.9, *; q=0.1
  LANGUAGE=ko LANG=en_US.UTF-8 - Accept-Language: ko, *; q=0.1
  LANGUAGE= LANG=en_US.UTF-8 - Accept-Language: en-US, *; q=0.1

This gives git servers a chance to display remote error messages in
the user's preferred language.

Signed-off-by: Yi EungJun eungjun...@navercorp.com
---
 http.c | 125 +
 remote-curl.c  |   2 +
 t/t5550-http-fetch-dumb.sh |  21 
 3 files changed, 148 insertions(+)

diff --git a/http.c b/http.c
index 3a28b21..a20f3e2 100644
--- a/http.c
+++ b/http.c
@@ -983,6 +983,129 @@ static void extract_content_type(struct strbuf *raw, 
struct strbuf *type,
strbuf_addstr(charset, ISO-8859-1);
 }
 
+/*
+ * Guess the user's preferred languages from the value in LANGUAGE environment
+ * variable and LC_MESSAGES locale category.
+ *
+ * The result can be a colon-separated list like ko:ja:en.
+ */
+static const char* get_preferred_languages() {
+const char* retval;
+
+   retval = getenv(LANGUAGE);
+   if (retval != NULL  retval[0] != '\0')
+   return retval;
+
+   retval = setlocale(LC_MESSAGES, NULL);
+   if (retval != NULL  retval[0] != '\0'
+strcmp(retval, C) != 0
+strcmp(retval, POSIX) != 0)
+   return retval;
+
+   return NULL;
+}
+
+/*
+ * Add an Accept-Language header which indicates user's preferred languages.
+ *
+ * Examples:
+ *   LANGUAGE= - 
+ *   LANGUAGE=ko:en - Accept-Language: ko, en; q=0.9, *; q=0.1
+ *   LANGUAGE=ko_KR.UTF-8:sr@latin - Accept-Language: ko-KR, sr; q=0.9, *; 
q=0.1
+ *   LANGUAGE=ko LANG=en_US.UTF-8 - Accept-Language: ko, *; q=0.1
+ *   LANGUAGE= LANG=en_US.UTF-8 - Accept-Language: en-US, *; q=0.1
+ *   LANGUAGE= LANG=C - 
+ */
+static struct curl_slist* add_accept_language(struct curl_slist *headers)
+{
+   const char *p1, *p2, *p3;
+   struct strbuf buf = STRBUF_INIT;
+   float q = 1.0;
+   float q_precision = 0.1;
+   int num_langs = 1;
+   char* q_format = ; q=%.1f;
+
+   p1 = get_preferred_languages();
+
+   /* Don't add Accept-Language header if no language is preferred. */
+   if (p1 == NULL || p1[0] == '\0') {
+   strbuf_release(buf);
+   return headers;
+   }
+
+   /* Count number of preferred languages to decide precision of q-factor 
*/
+   for (p3 = p1; *p3 != '\0'; p3++) {
+   if (*p3 == ':') {
+   num_langs++;
+   }
+   }
+
+   /* 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;
+   }
+
+   strbuf_addstr(buf, Accept-Language: );
+
+   for (p2 = p1; q  q_precision; p2++) {
+   if ((*p2 == ':' || *p2 == '\0')  p1 != p2) {
+   if (q  1.0) {
+   strbuf_addstr(buf, , );
+   }
+
+   for (p3 = p1; p3  p2; p3++) {
+   /* Replace '_' with '-'. */
+   if (*p3 == '_') {
+   strbuf_add(buf, p1, p3 - p1);
+   strbuf_addstr(buf, -);
+   p1 = p3 + 1;
+   }
+
+   /* Chop off anything after '.' or '@'. */
+   if ((*p3 == '.' || *p3 == '@')) {
+   break;
+   }
+   }
+
+   if (p3  p1) {
+   strbuf_add(buf, p1, p3 - p1);
+   }
+
+   /* Put the q factor if only it is less than 1.0. */
+   if (q  1.0) {
+   strbuf_addf(buf, q_format, q);
+   }
+
+   q -= q_precision;
+   p1 = p2 + 1;
+
+   if (*p2 == '\0') {
+   break;
+   }
+   }
+   }
+
+   /* Don't add Accept-Language header if no language is preferred. */
+   if (q = 1.0) {
+   strbuf_release(buf);
+   return headers;
+   }
+
+   /* Add '*' with minimum q-factor greater than 0.0. */
+   strbuf_addstr(buf, , *);
+   strbuf_addf(buf, q_format, q_precision);
+
+   headers = curl_slist_append(headers, buf.buf);
+
+   strbuf_release(buf);
+
+   return headers;
+}

[PATCH 2/3] tag: fix --sort tests to use cat-\EOF format

The --sort tests should use the better format for expect to maintain
indenting and ensure that no substitution is occurring. This makes
parsing and understanding the tests a bit easier.

Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
 t/t7004-tag.sh | 40 
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e4ab0f5b6419..66010b0e7066 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1385,41 +1385,41 @@ test_expect_success 'lexical sort' '
git tag foo1.6 
git tag foo1.10 
git tag -l --sort=refname foo* actual 
-   cat expect EOF 
-foo1.10
-foo1.3
-foo1.6
-EOF
+   cat expect -\EOF 
+   foo1.10
+   foo1.3
+   foo1.6
+   EOF
test_cmp expect actual
 '
 
 test_expect_success 'version sort' '
git tag -l --sort=version:refname foo* actual 
-   cat expect EOF 
-foo1.3
-foo1.6
-foo1.10
-EOF
+   cat expect -\EOF 
+   foo1.3
+   foo1.6
+   foo1.10
+   EOF
test_cmp expect actual
 '
 
 test_expect_success 'reverse version sort' '
git tag -l --sort=-version:refname foo* actual 
-   cat expect EOF 
-foo1.10
-foo1.6
-foo1.3
-EOF
+   cat expect -\EOF 
+   foo1.10
+   foo1.6
+   foo1.3
+   EOF
test_cmp expect actual
 '
 
 test_expect_success 'reverse lexical sort' '
git tag -l --sort=-refname foo* actual 
-   cat expect EOF 
-foo1.6
-foo1.3
-foo1.10
-EOF
+   cat expect -\EOF 
+   foo1.6
+   foo1.3
+   foo1.10
+   EOF
test_cmp expect actual
 '
 
-- 
2.0.1.475.g9b8d714

--
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] tag: use skip_prefix instead of magic numbers

Make the parsing of the --sort parameter more readable by having
skip_prefix keep our pointer up to date.

Authored-by: Jeff King p...@peff.net
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
 builtin/tag.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index ef765563388c..7ccb6f3c581b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -524,18 +524,12 @@ static int parse_opt_sort(const struct option *opt, const 
char *arg, int unset)
int *sort = opt-value;
int flags = 0;
 
-   if (*arg == '-') {
+   if (skip_prefix(arg, -, arg))
flags |= REVERSE_SORT;
-   arg++;
-   }
-   if (starts_with(arg, version:)) {
+
+   if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
*sort = VERCMP_SORT;
-   arg += 8;
-   } else if (starts_with(arg, v:)) {
-   *sort = VERCMP_SORT;
-   arg += 2;
-   } else
-   *sort = STRCMP_SORT;
+
if (strcmp(arg, refname))
die(_(unsupported sort specification %s), arg);
*sort |= flags;
-- 
2.0.1.475.g9b8d714

--
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 v8 1/2] add `config_set` API for caching config-like files

Tanay Abhra tanay...@gmail.com writes:

 diff --git a/config.c b/config.c
 index ba882a1..aa58275 100644
 --- a/config.c
 +++ b/config.c
 @@ -9,6 +9,8 @@
  #include exec_cmd.h
  #include strbuf.h
  #include quote.h
 +#include hashmap.h
 +#include string-list.h
  
  struct config_source {
   struct config_source *prev;
 @@ -33,10 +35,23 @@ struct config_source {
   long (*do_ftell)(struct config_source *c);
  };
  
 +struct config_hash_entry {
 + struct hashmap_entry ent;
 + char *key;
 + struct string_list value_list;
 +};
 +
  static struct config_source *cf;
  
  static int zlib_compression_seen;
  
 +/*
 + * Default config_set that contains key-value pairs from the usual set of 
 config
 + * config files (i.e repo specific .git/config, user wide ~/.gitconfig, XDG
 + * config file and the global /etc/gitconfig)
 + */
 +static struct config_set the_config_set;
 +
  static int config_file_fgetc(struct config_source *conf)
  {
   return fgetc(conf-u.file);
 @@ -1212,6 +1227,284 @@ int git_config(config_fn_t fn, void *data)
   return git_config_with_options(fn, data, NULL, 1);
  }
  
 +static int config_hash_add_value(const char *, const char *, struct hashmap 
 *);

This is a funny forward declaration.  If you define add-file and
friends that modify the config-set _after_ you define find-entry and
friends that are used to look-up, would you still need to do a
forward declaration?

In any case, please give names to these first two parameters as what
they are for can be unclear; because they are of the same type,
there is one less clue than there usually are.

The function signature makes it sound as if this is not specific to
config; what takes a hashmap, a key and a value and is called add
is a function to add key, value pair to a hashmap.  Why doesn't it
take struct config_set?  In other words, I would have expected

static int config_set_add(struct config_set *, const char *key, const char 
*value)

instead.  Not a complaint, but is puzzled why you chose not to do
so.

I suspect almost everywhere in this patch, you would want to do
s/config_hash/config_set/.  s/config_hash_entry/config_set_element/
might be a good idea, too.  You have the concept of the config
set, and each element of that set is a config-set element, not a
config-hash entry, isn't it?

 +static int config_hash_entry_cmp(const struct config_hash_entry *e1,
 +  const struct config_hash_entry *e2, const void 
 *unused)
 +{
 + return strcmp(e1-key, e2-key);
 +}
 +
 +static void configset_init(struct config_set *cs)
 +{
 + if (!cs-hash_initialized) {
 + hashmap_init(cs-config_hash, 
 (hashmap_cmp_fn)config_hash_entry_cmp, 0);
 + cs-hash_initialized = 1;
 + }
 +}

Have uninitializer here, immediately after you defined the initializer.

 +static int config_hash_callback(const char *key, const char *value, void *cb)
 +{
 + struct config_set *cs = cb;
 + config_hash_add_value(key, value, cs-config_hash);
 + return 0;
 +}
 +
 +int git_configset_add_file(struct config_set *cs, const char *filename)
 +{
 + int ret = 0;
 + configset_init(cs);
 + ret = git_config_from_file(config_hash_callback, filename, cs);
 + if (!ret)
 + return 0;
 + else {
 + hashmap_free(cs-config_hash, 1);
 + cs-hash_initialized = 0;
 + return -1;

Does calling configset_clear() do too much for the purpose of this
error path?  In other words, is it deliberate that you do not touch
any string-list items you may have accumulated by calling the
callback?

 +static struct config_hash_entry *config_hash_find_entry(const char *key,
 + struct hashmap 
 *config_hash)
 +{
 + struct config_hash_entry k;
 + struct config_hash_entry *found_entry;
 + char *normalized_key;
 + int ret;
 + /*
 +  * `key` may come from the user, so normalize it before using it
 +  * for querying entries from the hashmap.
 +  */
 + ret = git_config_parse_key(key, normalized_key, NULL);
 +
 + if (ret)
 + return NULL;
 +
 + hashmap_entry_init(k, strhash(normalized_key));
 + k.key = normalized_key;
 + found_entry = hashmap_get(config_hash, k, NULL);
 + free(normalized_key);
 + return found_entry;
 +}
 +
 +static struct string_list *configset_get_list(const char *key, struct 
 config_set *cs)
 +{
 + struct config_hash_entry *e = config_hash_find_entry(key, 
 cs-config_hash);
 + return e ? e-value_list : NULL;
 +}
 +
 +static int config_hash_add_value(const char *key, const char *value, struct 
 hashmap *config_hash)
 +{
 + struct config_hash_entry *e;
 + e = config_hash_find_entry(key, config_hash);
 + /*
 +  * Since the keys are being fed by git_config*() callback mechanism, 
 they
 +  * are already normalized. So simply add them without any further 
 munging.
 +  */
 + if (!e) {
 +

Re: [PATCH v3] http: Add Accept-Language header if possible

On Sat, Jul 12, 2014 at 01:52:53AM +0900, Yi EungJun wrote:

 Add an Accept-Language header which indicates the user's preferred
 languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.
 
 Examples:
   LANGUAGE= - 
   LANGUAGE=ko:en - Accept-Language: ko, en; q=0.9, *; q=0.1
   LANGUAGE=ko LANG=en_US.UTF-8 - Accept-Language: ko, *; q=0.1
   LANGUAGE= LANG=en_US.UTF-8 - Accept-Language: en-US, *; q=0.1
 
 This gives git servers a chance to display remote error messages in
 the user's preferred language.

Thanks, this is looking much nicer. Most of my comments are on style:

 +static const char* get_preferred_languages() {
 +const char* retval;

A few style nits:

  1. We usually put a function's opening brace on a new line.

  2. We usually put the asterisk in a pointer declaration with the
 variable name (const char *retval). This one appears elsewhere in
 the patch.

  3. This line seems to be indented with spaces instead of tabs.

 + retval = getenv(LANGUAGE);
 + if (retval != NULL  retval[0] != '\0')
 + return retval;
 +
 + retval = setlocale(LC_MESSAGES, NULL);
 + if (retval != NULL  retval[0] != '\0'
 +  strcmp(retval, C) != 0
 +  strcmp(retval, POSIX) != 0)
 + return retval;

More style nits: we usually avoid writing out explicit comparisons with
NULL (and often with zero). So I'd write this as:

  if (retval  *retval 
  strcmp(retval, C) 
  strcmp(retval, POSIX))

but I think the NULL one is the only one we usually enforce explicitly.

 + const char *p1, *p2, *p3;

I had trouble following the logic with these variable names. Is it
possible to give them more descriptive names?

 + /* Don't add Accept-Language header if no language is preferred. */
 + if (p1 == NULL || p1[0] == '\0') {
 + strbuf_release(buf);
 + return headers;
 + }

No need to strbuf_release a buffer that hasn't been used (assigning
STRBUF_INIT does not count as use).

 + /* Count number of preferred languages to decide precision of q-factor 
 */
 + for (p3 = p1; *p3 != '\0'; p3++) {
 + if (*p3 == ':') {
 + num_langs++;
 + }
 + }

Style: we usually omit braces for one-liners. So:

  for (p3 = p1; *p3; p3++)
if (*p3 == ':')
num_langs++;

(and elsewhere).

 + /* 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;
 + }

I don't mind this auto-precision too much, but I'm not sure it buys us
anything. We are still setting a hard-limit at 100, and it just means we
write 0.1 instead of 0.001 sometimes.

 + headers = curl_slist_append(headers, buf.buf);
 +
 + strbuf_release(buf);

Do all versions of curl make a copy of buf.buf here?

I seem to recall that older versions want pointers passed to
curl_easy_setopt to stay around for the duration of the request (I do
not know about curl_slist, though).

 @@ -1020,6 +1143,8 @@ static int http_request(const char *url,
fwrite_buffer);
   }
  
 + headers = add_accept_language(headers);

This means we do the whole parsing routine for each request we make (for
dumb-http, this can be quite a lot, though I suppose the parsing is not
especially expensive). Should we perhaps just cache the result in a
static strbuf? That would also make the pointer-lifetime issue above go
away.

 --- a/remote-curl.c
 +++ b/remote-curl.c
 @@ -946,6 +946,8 @@ int main(int argc, const char **argv)
   struct strbuf buf = STRBUF_INIT;
   int nongit;
  
 + git_setup_gettext();

Oops. We probably should have been doing this all along to localize the
messages on our side.

 +test_expect_success 'git client sends Accept-Language based on LANGUAGE, 
 LC_ALL, LC_MESSAGES and LANG' '
 + test_must_fail env GIT_CURL_VERBOSE=1 LANGUAGE=ko_KR.UTF-8 
 LC_ALL=de_DE.UTF-8 LC_MESSAGES=ja_JP.UTF-8 LANG=en_US.UTF-8 git clone 
 $HTTPD_URL/accept/language 2stderr 

We usually try to avoid long lines (you can break them up with \).

 + grep ^Accept-Language: ko-KR, \\*; q=0.1 stderr 

I see you noticed the extra level of quoting necessary between v2 and
v3. :)

I wonder if these test cases might be more readable with a helper
function like:

  check_language () {
echo Accept-Language: $1 expect 
test_must_fail env \
GIT_CURL_VERBOSE=1 \
LANGUAGE=$2 \
LC_ALL=$3 \
LC_MESSAGES=$4 \
LANG=$5 \
git clone $HTTPD_URL/accept/language 2stderr 
grep -i ^Accept-Language: stderr actual 
test_cmp expect actual
  }

which lets you write:

  check_language de-DE, *; q=0.1

Re: git p4 diff-tree ambiguous argument error

2014-07-11 Thread Bill Door

More data points. I have reproduced the problem on

$ git --version
git version 1.8.5.2 (Apple Git-48)
$ python --version
Python 2.7.5
$ uname -a
Darwin Kernel Version 13.3.0: Tue Jun  3 21:27:35 PDT 2014;
root:xnu-2422.110.17~1/RELEASE_X86_64 x86_64

However, it the command is used on a new empty repository, there is no
failure. Running a second time causes the failure. 

Thanks for any help you can offer in tracking down this problem!



--
View this message in context: 
http://git.661346.n2.nabble.com/git-p4-diff-tree-ambiguous-argument-error-tp7614774p7614882.html
Sent from the git mailing list archive at Nabble.com.
--
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

On Fri, Jul 11, 2014 at 10:24:07AM -0700, Jacob Keller wrote:

 Updated to include changes due to Junio's feedback. This has not resolved
 whether we should fail on a configuration error or simply warn. It appears 
 that
 we actually seem to error out more than warn, so I am unsure what the correct
 action is here.

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.

It's probably user-unfriendly to be silent for those cases, though. The
user gets no feedback on why their config value is doing nothing.

I tend to think that warning is not much better than erroring out. It is
helpful if you are running a single-shot of an old version (which is
something that I do a lot when testing old versions), but would quickly
become irritating if you were actually using an old version of git
day-to-day.

I dunno. Maybe it is worth making life easier for people in the former
category.

 +static int parse_sort_string(const char *arg, int *sort)
 +{
 + int type = 0, flags = 0;
 +
 + if (skip_prefix(arg, -, arg))
 + flags |= REVERSE_SORT;
 +
 + if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
 + type = VERCMP_SORT;
 + else
 + type = STRCMP_SORT;
 +
 + if (strcmp(arg, refname))
 + return error(_(unsupported sort specification %s), arg);
 +
 + *sort = (type | flags);
 +
 + return 0;
 +}

Regardless of how we handle the error, I think this version that
assembles the final bitfield at the end is easier to read than the
original.

-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 1/3] tag: use skip_prefix instead of magic numbers

On Fri, Jul 11, 2014 at 10:24:05AM -0700, Jacob Keller wrote:

 Make the parsing of the --sort parameter more readable by having
 skip_prefix keep our pointer up to date.
 
 Authored-by: Jeff King p...@peff.net

I suspect Junio may just apply this on the version of the commit he has
upstream, so you may not need this as part of your series.

However, for reference, the right way to handle authorship is with a

  From: Jeff King p...@peff.net

at the top of your message body. Git-am will pick that up and turn it
into the author field of the commit.

-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: pitfall with empty commits during git rebase

On Fri, 2014-07-11 at 12:15 +0200, Olaf Hering wrote:
There is an incorrect message when doing git rebase -i remote/branch.
I have it only in german, see below. what happend is:

#01 make changes on another host
#02 copy patchfile to localhost
#03 apply patchfile
#04 git commit -avs # create commit#1
#05 sleep 123456
#06 make different changes on another host
#07 copy patchfile to localhost
#08 git show | patch -Rp1
#09 git commit -avs # create commit#2
#10 apply patchfile
#11 git commit -avs # create commit#3
#12 git rebase -i remote/branch
pick commit#1 msg
fcommit#2 msg
fcommit#3 msg

.git/rebase-merge/git-rebase-todo 21L, 707C geschrieben
Sie fragten den jüngsten Commit nachzubessern, aber das würde diesen leer
machen. Sie können Ihr Kommando mit --allow-empty wiederholen, oder diesen
Commit mit git reset HEAD^ vollständig entfernen.
Rebase im Gange; auf c105589
Sie sind gerade beim Rebase von Branch 'mybranch' auf 'c105589'.

Keine Änderungen

Could not apply 6c5842320acc797d395afb5cdf373c2bfaebfa34... revert

Its not clear what '--allow-empty' refers to, git rebase does not seem to
understand this option.

You should be able to fix this with

git commit --allow-empty
git rebase --continue

But yes the message could possibly be made a little clearer.

Thanks,
Jake

I should have skipped step #09 to avoid the trouble.
git version is 2.0.1. Please adjust the error msg above.

Olaf
--
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

N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf

Re: [Bug] data loss with cyclic alternates

On Fri, 2014-07-11 at 09:01 -0700, Junio C Hamano wrote:
 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.
 

Doesn't gc get run automatically at some points? Is the automatic gc run
setup to avoid this problem? 

Thanks,
Jake

 --
 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 1/3] tag: use skip_prefix instead of magic numbers

On Fri, 2014-07-11 at 13:50 -0400, Jeff King wrote:
 On Fri, Jul 11, 2014 at 10:24:05AM -0700, Jacob Keller wrote:
 
  Make the parsing of the --sort parameter more readable by having
  skip_prefix keep our pointer up to date.
  
  Authored-by: Jeff King p...@peff.net
 
 I suspect Junio may just apply this on the version of the commit he has
 upstream, so you may not need this as part of your series.
 
 However, for reference, the right way to handle authorship is with a
 
   From: Jeff King p...@peff.net
 
 at the top of your message body. Git-am will pick that up and turn it
 into the author field of the commit.
 

Alright, thanks.

Regards,
Jake

 -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 v5] tag: support configuring --sort via .gitconfig

On Fri, 2014-07-11 at 13:46 -0400, Jeff King wrote:
 On Fri, Jul 11, 2014 at 10:24:07AM -0700, Jacob Keller wrote:
 
  Updated to include changes due to Junio's feedback. This has not resolved
  whether we should fail on a configuration error or simply warn. It appears 
  that
  we actually seem to error out more than warn, so I am unsure what the 
  correct
  action is here.
 
 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.
 
 It's probably user-unfriendly to be silent for those cases, though. The
 user gets no feedback on why their config value is doing nothing.
 
 I tend to think that warning is not much better than erroring out. It is
 helpful if you are running a single-shot of an old version (which is
 something that I do a lot when testing old versions), but would quickly
 become irritating if you were actually using an old version of git
 day-to-day.
 
 I dunno. Maybe it is worth making life easier for people in the former
 category.
 
  +static int parse_sort_string(const char *arg, int *sort)
  +{
  +   int type = 0, flags = 0;
  +
  +   if (skip_prefix(arg, -, arg))
  +   flags |= REVERSE_SORT;
  +
  +   if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
  +   type = VERCMP_SORT;
  +   else
  +   type = STRCMP_SORT;
  +
  +   if (strcmp(arg, refname))
  +   return error(_(unsupported sort specification %s), arg);
  +
  +   *sort = (type | flags);
  +
  +   return 0;
  +}
 
 Regardless of how we handle the error, I think this version that
 assembles the final bitfield at the end is easier to read than the
 original.
 

Yes, I figured setting it up all at the end makes more sense, and is
less prone to subtle bugs, since we don't directly modify sort using |=
or relying on particular values of sort initially.

I personally prefer error out on options, even though it can make it a
bit more difficult, though as far as I know unknown fields simply warn
or are ignored. (ie: old versions of git just ignore unknown fields in
configuration).

It's possible we should warn instead though, so that older gits work
with new sorts that they don't understand.

I am ok with warning but I don't know the best practice for how to warn
here instead of failing. Returning error causes a fatal bad config
message. Any thoughts?

Thanks,
Jake

 -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 v8 2/2] test-config: add tests for the config_set API

Tanay Abhra tanay...@gmail.com writes:

 diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
 new file mode 100755
 index 000..87a29f1
 --- /dev/null
 +++ b/t/t1308-config-set.sh
 @@ -0,0 +1,170 @@
 +#!/bin/sh
 +
 +test_description='Test git config-set API in different settings'
 +
 +. ./test-lib.sh
 +
 +# 'check section.key value' verifies that the entry for section.key is
 +# 'value'
 +check() {

(style) SP around both sides of ().

More importantly, will we ever have different kind of check in this
script, perhaps checking if all values for a multi-valued variables
appear in the output, checking if get_bool works, etc. in the
future?  I would imagine the answer is yes, and in that case this
should be renamed to be a bit more specific (i.e. no too generic
helper called check would exist in the final result).  Perhaps
call it check_single, check_get_value or check_value (the last
one assumes that all your future checks are mostly about various
forms of get)?

 + echo $2 expected 
 + test-config get_value $1 actual 21 
 + test_cmp expected actual
 +}
 +
 +test_expect_success 'setup default config' '
 + cat .git/config  EOF

 - No SP after redirection operator.

 - If you are not using variable substition in the here-doc, it is
   more friendly to quote the end-of-here-doc token to tell the
   reader that they do not have to worry about them.

 - There may be core.* variables that are crucial for correct
   operation of the version of Git being tested, so wiping and
   replacing .git/config wholesale is not a good idea.  Appending
   your config items is sufficient for the purpose of these tests.

i.e.

cat .git/config \EOF
...
EOF

 + [core]

I'd feel safer if you did not abuse [core] like this.  All you care
about is the config parsing, and you do not want future versions of
Git introducing core.MixedCase to mean something meaningful that
affects how git config and other commands work.

 + penguin = very blue
 + Movie = BadPhysics
 + UPPERCASE = true
 + MixedCase = true
 + my =
 ...
 + EOF
 +'
 +
 +test_expect_success 'get value for a simple key' '
 + check core.penguin very blue
 +'
 +
 +test_expect_success 'get value for a key with value as an empty string' '
 + check core.my 
 +'
 +
 +test_expect_success 'get value for a key with value as NULL' '
 + check core.foo (NULL)
 +'
 +
 +test_expect_success 'upper case key' '
 + check core.UPPERCASE true
 +'
 +
 +test_expect_success 'mixed case key' '
 + check core.MixedCase true
 +'

You would also need to see how

check core.uppercase
check core.MIXEDCASE

behave (after moving them out of core. hierarchy, of course), like
the following checks for case insensitivity of the first token.  The
first and the last token are both supposed to be case insensitive,
no?

 +test_expect_success 'key with case insensitive section header' '
 + check cores.baz ball 
 + check Cores.baz ball 
 + check CORES.baz ball 
 + check coreS.baz ball
 +'
 +
 +test_expect_success 'find value with misspelled key' '
 + echo Value not found for \my.fOo Bar.hi\ expect 
 + test_must_fail test-config get_value my.fOo Bar.hi actual 

Is test_must_fail suffice here?  git_config_get_value() has two
kinds of non-zero return values (one for error, the other for not
found).  Shouldn't test-config let the caller tell these two kinds
apart in some way?  It would be reasonable to do so with its exit
status, I would imagine, and in that case, test_expect_code may be
more appropriate here.

 + test_cmp expect actual

Are you sure the expect and actual should match here?

Oh by the way, in check() helper shell function you spelled
the correct answer expected but here you use expect (like almost
all the other existing tests).  Perhaps s/expected/expect/ while we
fix the helper function?

 +'
 +
 +test_expect_success 'find value with the highest priority' '
 + check core.baz hask
 +'
 +
 +test_expect_success 'find integer value for a key' '
 + echo 65 expect 
 + test-config get_int lamb.chop actual 
 + test_cmp expect actual
 +'

Perhaps

check_config () {
op=$1 key=$2 
shift 
shift 
printf %s\n $@ expect 
test-config $op $key actual 
test_cmp expect actual
}

and use it like so?

check_config get_value core.mixedcase true
check_config get_int lamb.chop 65
check_config get_bool goat.head 1
check_config get_value_multi core.baz sam bat hask

 +test_expect_success 'find integer if value is non parse-able' '
 + echo 65 expect 
 + test_must_fail test-config get_int lamb.head actual 
 + test_must_fail test_cmp expect actual

Do not use test_must_fail on anything other than git command.
Worse yet, you are not just interested in seeing expect and actual
differ.

Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig

On Fri, Jul 11, 2014 at 06:11:08PM +, Keller, Jacob E wrote:

 I personally prefer error out on options, even though it can make it a
 bit more difficult, though as far as I know unknown fields simply warn
 or are ignored. (ie: old versions of git just ignore unknown fields in
 configuration).

Right, we _have_ to ignore unknown config options, because we
specifically allow other programs built on git to store their config
with us (and anyway, our callback style of parsing means that no single
callback knows about all of the keys).

In the past we have staked out particular areas of the namespace,
though. E.g., the diff code said I own all of color.diff.*, and if you
put in something I don't understand, I'll complain. That ended up being
annoying, and now we ignore slots we don't understand there.

So old gits will always silently ignore tag.sort if they don't know
about it, and we can't change that. The only thing we can change is:

 It's possible we should warn instead though, so that older gits work
 with new sorts that they don't understand.

Right. I think other config variables in similar situations will barf.
This is backwards-compatible as long as the new variables are a superset
(i.e., we only add new understood values, never remove or change the
meaning of existing values). It's just not forwards-compatible.

 I am ok with warning but I don't know the best practice for how to warn
 here instead of failing. Returning error causes a fatal bad config
 message. Any thoughts?

The simplest thing is ignoring the return from parse_sort_string and
just calling return 0. That will still say error:, but continue on.
If you really want it to say warning:, I think you'll have to pass a
flag into parse_sort_string. I'm not sure if it's worth the effort.

-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 v6 02/10] replace: add --graft option

Christian Couder christian.cou...@gmail.com writes:

 On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano gits...@pobox.com wrote:
 Christian Couder christian.cou...@gmail.com writes:

 On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano gits...@pobox.com wrote:

 Making sure A's parent is B would be an
 idempotent operation, no?  Why not just make sure A's parent is
 already B and report Your wish has been granted to the user?

 ... and here you say we should report your wish has been granted...

 Normal way for git replace to report that is to exit with status 0
 and without any noise, I would think.

 In a similar case git replace --edit we error out instead of just
 exiting (with status 0), see:

 f22166b5fee7dc (replace: make sure --edit results in a different object)

I do not care *too* deeply, but if you ask me, that may be a mistake
we may want to fix before the next release.
--
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: pitfall with empty commits during git rebase

On Fri, Jul 11, 2014 at 12:15:47PM +0200, Olaf Hering wrote:

 Could not apply 6c5842320acc797d395afb5cdf373c2bfaebfa34... revert
 
 
 Its not clear what '--allow-empty' refers to, git rebase does not seem to
 understand this option.

I think this is the same problem discussed recently in:

  http://thread.gmane.org/gmane.comp.version-control.git/251365

-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 v6 02/10] replace: add --graft option

On Fri, Jul 11, 2014 at 11:25:43AM -0700, Junio C Hamano wrote:

 Christian Couder christian.cou...@gmail.com writes:
 
  On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano gits...@pobox.com wrote:
  Christian Couder christian.cou...@gmail.com writes:
 
  On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano gits...@pobox.com wrote:
 
  Making sure A's parent is B would be an
  idempotent operation, no?  Why not just make sure A's parent is
  already B and report Your wish has been granted to the user?
 
  ... and here you say we should report your wish has been granted...
 
  Normal way for git replace to report that is to exit with status 0
  and without any noise, I would think.
 
  In a similar case git replace --edit we error out instead of just
  exiting (with status 0), see:
 
  f22166b5fee7dc (replace: make sure --edit results in a different object)
 
 I do not care *too* deeply, but if you ask me, that may be a mistake
 we may want to fix before the next release.

Yeah, I also do not care too deeply, but I mentioned in the earlier
review that I would expect it to just remove the replacement if it ends
up generating the same 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

Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig

Keller, Jacob E jacob.e.kel...@intel.com writes:

 This is not how the rest of the current tests work. I will submit a
 patch which fixes up the current --sort tests (but not every test, for
 now) as well.

I do not want to pile more work that is unrelated to the task at
hand on your plate, i.e. clean-up work, so I would assign a very low
priority to fix up the current tests.  At the same time, existing
mistakes are not excuses to introduce new similar ones, hence my
suggestions to the added code in the patch I saw.

--
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 1/3] tag: use skip_prefix instead of magic numbers

Jeff King p...@peff.net writes:

 On Fri, Jul 11, 2014 at 10:24:05AM -0700, Jacob Keller wrote:

 Make the parsing of the --sort parameter more readable by having
 skip_prefix keep our pointer up to date.
 
 Authored-by: Jeff King p...@peff.net

 I suspect Junio may just apply this on the version of the commit he has
 upstream, so you may not need this as part of your series.

You read me correctly ;-)  That is what I was planning to do, to
fork jk/tag-sort topic on top of the updated jk/skip-prefix topic.

 However, for reference, the right way to handle authorship is with a

   From: Jeff King p...@peff.net

 at the top of your message body. Git-am will pick that up and turn it
 into the author field of the commit.

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

Re: [PATCH 2/2] dir: remove PATH_MAX limitation

Am 05.07.2014 12:48, schrieb Duy Nguyen:
 On Sat, Jul 5, 2014 at 5:42 AM, Karsten Blees karsten.bl...@gmail.com wrote:
 'git status' segfaults if a directory is longer than PATH_MAX, because
 processing .gitignore files in prep_exclude() writes past the end of a
 PATH_MAX-bounded buffer.

 Remove the limitation by using strbuf instead.

 Note: this fix just 'abuses' strbuf as string allocator, len is always 0.
 prep_exclude() can probably be simplified using more strbuf APIs.
 
 FYI I had a similar patch [1] that attempted to lazily strbuf_init()
 instead so that strbuf_ API could be used.
 
 [1] http://article.gmane.org/gmane.comp.version-control.git/248310
 

Sorry, I missed that one.

In my version, strbuf_grow() does the lazy initialization (in fact, many
strbuf_* functions can handle memset(0) strbufs just fine).

I was simply too lazy to understand (again) how prep_exclude works exactly, and
as string calculations like that have the tendency to be just 1 char off, I went
for the obviously correct solution (i.e. s/dir-basebuf/dir-base.buf/ plus
strbuf_grow() before we write the buffer).

But IMO your version is much cleaner already.

However, api-strbuf.txt says that buf != NULL is invariant after init, and
alloc is somehow private :-) , so perhaps you should

-   if (!dir-basebuf.alloc)
+   if (!dir-basebuf.buf)
strbuf_init(dir-basebuf, PATH_MAX);

--
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 1/2] symlinks: remove PATH_MAX limitation

Am 07.07.2014 20:30, schrieb Junio C Hamano:
 Karsten Blees karsten.bl...@gmail.com writes:
 
 The above cache_def_free(cache) does not free the cache itself, but
 only its associated data, so the name cache_def_free() is somewhat
 misleading.
 

You already merged this to master (kb/path-max-must-go lol), should
I send a fixup! s/cache_def_free/cache_def_clear/ or is it OK as is?

--
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 v1 3/4] hashmap: add simplified hashmap_get_from_hash() API

Am 07.07.2014 19:43, schrieb Junio C Hamano:
 Karsten Blees karsten.bl...@gmail.com writes:
 
 Hashmap entries are typically looked up by just a key. The hashmap_get()
 API expects an initialized entry structure instead, to support compound
 keys. This flexibility is currently only needed by find_dir_entry() in
 name-hash.c (and compat/win32/fscache.c in the msysgit fork). All other
 (currently five) call sites of hashmap_get() have to set up a near emtpy
 
 s/emtpy/empty/;
 
 entry structure, resulting in duplicate code like this:

   struct hashmap_entry keyentry;
   hashmap_entry_init(keyentry, hash(key));
   return hashmap_get(map, keyentry, key);

 Add a hashmap_get_from_hash() API that allows hashmap lookups by just
 specifying the key and its hash code, i.e.:

   return hashmap_get_from_hash(map, hash(key), key);
 
 While I think the *_get_from_hash() is an improvement over the
 three-line sequence, I find it somewhat strange that callers of it
 still must compute the hash code themselves, instead of letting the
 hashmap itself to apply the user supplied hash function to the key
 to derive it.  After all, the map must know how to compare two
 entries via a user-supplied cmpfn given at the map initialization
 time, and the algorithm to derive the hash code falls into the same
 category, in the sense that both must stay the same during the
 lifetime of a hashmap, no?  Unless there is a situation where you
 need to be able to initialize a hashmap_entry without knowing which
 hashmap the entry will eventually be stored, it feels dubious API
 that exposes to the outside callers hashmap_entry_init() that takes
 the hash code without taking the hashmap itself.
 
 In other words, why isn't hashmap_get() more like this:
 
 void *hashmap_get(const struct hashmap *map, const void *key)
 {
 struct hashmap_entry keyentry;
 hashmap_entry_init(keyentry, map-hash(key));
 return *find_entry_ptr(map, keyentry, key);
 }
 
 with hashmap_entry_init() purely a static helper in hashmap.c?
 

1. Performance

Calculating hash codes is the most expensive operation when working with
hash tables (unless you already have a good hash such as sha1). And using
hash tables as a cache of some sort is probably the most common use case.
So you'll often have code like this:

1  unsigned int h = hash(key);
2  struct my_entry *e = hashmap_get_from_hash(map, hash, key);
3  if (!e) {
4e = malloc(sizeof(*e));
5hashmap_entry_init(e, h);
6e-key = key;
6hashmap_add(map, e);
7  }

Note that the hash code from line 1 can be reused in line 5. You cannot do
that if calculating the hash code is buried in the implementation.

Callbacks cannot be inlined either...


2. Simplicity

Most APIs take a user defined hashmap_entry structure, so you'd either need
two hash functions, or a hash function that can distinguish between the
'entry' and 'key-only' case, e.g.

  unsigned int my_hash(const struct my_entry *entry, const void *keydata)
  {
if (keydata)
  return strhash(keydata);
else
  return strhash(entry-key);
  }

Hashmap clients will typically provide small, type safe wrappers around the
hashmap API. That's perfect for calculating the hash code without breaking
encapsulation. IMO there's no need to make things more complex by requiring
another callback.


3. Compound keys

The key doesn't always consist of just a single word. E.g. for struct
dir_entry, the key is [char *name, int len]. So an API like this:

  void *hashmap_get(const struct hashmap *map, const void *key)

won't do in the general case (unless you require clients to define their
own key structure in addition to the entry structure...).

--
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

Am 09.07.2014 18:33, schrieb Junio C Hamano:
 Karsten Blees karsten.bl...@gmail.com writes:
 
 'git status' segfaults if a directory is longer than PATH_MAX, because
 processing .gitignore files in prep_exclude() writes past the end of a
 PATH_MAX-bounded buffer.

 Remove the limitation by using strbuf instead.

 Note: this fix just 'abuses' strbuf as string allocator, len is always 0.
 prep_exclude() can probably be simplified using more strbuf APIs.
 
 In addition to what Jeff already said, I am afraid that this may
 make things a lot worse for normal case.  By sizing the strbuf to
 absolute minimum as you dig deeper at each level, wouldn't you copy
 and reallocate the buffer a lot more, compared to the case where
 everything fits in the original buffer?
 

strbuf uses ALLOC_GROW, which resizes in (x+16)*1.5 steps (i.e. 24, 60, 114,
195, 316...). Max path len in git is 85, and linux and WebKit are 170ish. I
don't think three or four extra reallocs per directory traversal will be
noticeable.

Anyways, I'd like to kindly withdraw this patch in favor of Duy's version.

http://article.gmane.org/gmane.comp.version-control.git/248310
--
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: Topic sk/mingw-unicode-spawn-args breaks tests

Am 10.07.2014 22:05, schrieb Johannes Sixt:
 It looks like I totally missed the topic sk/mingw-unicode-spawn-args.
 Now it's in master, and it breaks lots of test cases for me:
 
 t0050-filesystem
 t0110-urlmatch-normalization
 t4014-format-patch
 t4041-diff-submodule-option
 t4120-apply-popt
 t4201-shortlog
 t4205-log-pretty-formats
 t4209-log-pickaxe
 t4210-log-i18n
 (I killed the test run here)
 
 Am I doing something wrong? Does the topic depend on a particular
 version of MSYS (or DLL)?
 
 -- Hannes
 

After commenting out fchmod in config.c, I get similar results.

At first glance, t0050 seems to fail because the unicode file
name patches are still missing.

t4041 tries to pass ISO-8859-1 encoded bytes on the command line,
which simply doesn't work on Windows (all OS APIs 'talk' UTF-16).
We have a fix for this in the msysgit fork [1] (but unfortunately
in another branch, so Stepan couldn't know the patch is related).

I suspect the other failures also fall in these two categories.

[1] https://github.com/msysgit/git/commit/ef4a733c

--
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

On Fri, 2014-07-11 at 14:22 -0400, Jeff King wrote:
 On Fri, Jul 11, 2014 at 06:11:08PM +, Keller, Jacob E wrote:
 
  I personally prefer error out on options, even though it can make it a
  bit more difficult, though as far as I know unknown fields simply warn
  or are ignored. (ie: old versions of git just ignore unknown fields in
  configuration).
 
 Right, we _have_ to ignore unknown config options, because we
 specifically allow other programs built on git to store their config
 with us (and anyway, our callback style of parsing means that no single
 callback knows about all of the keys).
 
 In the past we have staked out particular areas of the namespace,
 though. E.g., the diff code said I own all of color.diff.*, and if you
 put in something I don't understand, I'll complain. That ended up being
 annoying, and now we ignore slots we don't understand there.
 
 So old gits will always silently ignore tag.sort if they don't know
 about it, and we can't change that. The only thing we can change is:
 
  It's possible we should warn instead though, so that older gits work
  with new sorts that they don't understand.
 
 Right. I think other config variables in similar situations will barf.
 This is backwards-compatible as long as the new variables are a superset
 (i.e., we only add new understood values, never remove or change the
 meaning of existing values). It's just not forwards-compatible.
 

So should I respin this so that config option doesn't error out?

  I am ok with warning but I don't know the best practice for how to warn
  here instead of failing. Returning error causes a fatal bad config
  message. Any thoughts?
 
 The simplest thing is ignoring the return from parse_sort_string and
 just calling return 0. That will still say error:, but continue on.
 If you really want it to say warning:, I think you'll have to pass a
 flag into parse_sort_string. I'm not sure if it's worth the effort.
 
 -Peff

Ok this makes sense, I am fine leaving it as error. Should I respin to
make it not die though?

Thanks,
Jake

Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig

On Fri, 2014-07-11 at 11:29 -0700, Junio C Hamano wrote:
 Keller, Jacob E jacob.e.kel...@intel.com writes:
 
  This is not how the rest of the current tests work. I will submit a
  patch which fixes up the current --sort tests (but not every test, for
  now) as well.
 
 I do not want to pile more work that is unrelated to the task at
 hand on your plate, i.e. clean-up work, so I would assign a very low
 priority to fix up the current tests.  At the same time, existing
 mistakes are not excuses to introduce new similar ones, hence my
 suggestions to the added code in the patch I saw.
 

It was trivial to fix at least the --sort tests, so I submitted a patch
that goes before this one to fix those as well.

Thanks,
Jake

[PATCH 2/3] tag: fix --sort tests to use cat-\EOF format

The --sort tests should use the better format for expect to maintain
indenting and ensure that no substitution is occurring. This makes
parsing and understanding the tests a bit easier.

Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
 t/t7004-tag.sh | 40 
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e4ab0f5b6419..66010b0e7066 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1385,41 +1385,41 @@ test_expect_success 'lexical sort' '
git tag foo1.6 
git tag foo1.10 
git tag -l --sort=refname foo* actual 
-   cat expect EOF 
-foo1.10
-foo1.3
-foo1.6
-EOF
+   cat expect -\EOF 
+   foo1.10
+   foo1.3
+   foo1.6
+   EOF
test_cmp expect actual
 '
 
 test_expect_success 'version sort' '
git tag -l --sort=version:refname foo* actual 
-   cat expect EOF 
-foo1.3
-foo1.6
-foo1.10
-EOF
+   cat expect -\EOF 
+   foo1.3
+   foo1.6
+   foo1.10
+   EOF
test_cmp expect actual
 '
 
 test_expect_success 'reverse version sort' '
git tag -l --sort=-version:refname foo* actual 
-   cat expect EOF 
-foo1.10
-foo1.6
-foo1.3
-EOF
+   cat expect -\EOF 
+   foo1.10
+   foo1.6
+   foo1.3
+   EOF
test_cmp expect actual
 '
 
 test_expect_success 'reverse lexical sort' '
git tag -l --sort=-refname foo* actual 
-   cat expect EOF 
-foo1.6
-foo1.3
-foo1.10
-EOF
+   cat expect -\EOF 
+   foo1.6
+   foo1.3
+   foo1.10
+   EOF
test_cmp expect actual
 '
 
-- 
2.0.1.475.g9b8d714

--
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] tag: support configuring --sort via .gitconfig

Add support for configuring default sort ordering for git tags. Command
line option will override this configured value, using the exact same
syntax.

Cc: Jeff King p...@peff.net
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
Updated based on Junio's suggestions, as well as making sure that we don't bail
if we can't understand config's option. We still print error message followed
by a warning about using default order. In addition, added a few more tests to
ensure that these work as expected.

 Documentation/config.txt  |  5 
 Documentation/git-tag.txt |  5 +++-
 builtin/tag.c | 61 ++-
 t/t7004-tag.sh| 36 
 4 files changed, 90 insertions(+), 17 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1d718bdb9662..c55c22ab7be9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2354,6 +2354,11 @@ submodule.name.ignore::
--ignore-submodules option. The 'git submodule' commands are not
affected by this setting.
 
+tag.sort::
+   This variable controls the sort ordering of tags when displayed by
+   linkgit:git-tag[1]. Without the --sort=value option provided, the
+   value of this variable will be used as the default.
+
 tar.umask::
This variable can be used to restrict the permission bits of
tar archive entries.  The default is 0002, which turns off the
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index b424a1bc48bb..320908369f06 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -99,7 +99,9 @@ OPTIONS
Sort in a specific order. Supported type is refname
(lexicographic order), version:refname or v:refname (tag
names are treated as versions). Prepend - to reverse sort
-   order.
+   order. When this option is not given, the sort order defaults to the
+   value configured for the 'tag.sort' variable if it exists, or
+   lexicographic order otherwise. See linkgit:git-config[1].
 
 --column[=options]::
 --no-column::
@@ -317,6 +319,7 @@ include::date-formats.txt[]
 SEE ALSO
 
 linkgit:git-check-ref-format[1].
+linkgit:git-config[1].
 
 GIT
 ---
diff --git a/builtin/tag.c b/builtin/tag.c
index 7ccb6f3c581b..cb37b26159a6 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -32,6 +32,8 @@ static const char * const git_tag_usage[] = {
 #define SORT_MASK   0x7fff
 #define REVERSE_SORT0x8000
 
+static int tag_sort;
+
 struct tag_filter {
const char **patterns;
int lines;
@@ -346,9 +348,46 @@ static const char tag_template_nocleanup[] =
Lines starting with '%c' will be kept; you may remove them
 yourself if you want to.\n);
 
+/*
+ * Parse a sort string, and return 0 if parsed successfully. Will return
+ * non-zero when the sort string does not parse into a known type.
+ */
+static int parse_sort_string(const char *arg, int *sort)
+{
+   int type = 0, flags = 0;
+
+   if (skip_prefix(arg, -, arg))
+   flags |= REVERSE_SORT;
+
+   if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
+   type = VERCMP_SORT;
+   else
+   type = STRCMP_SORT;
+
+   if (strcmp(arg, refname))
+   return error(_(unsupported sort specification %s), arg);
+
+   *sort = (type | flags);
+
+   return 0;
+}
+
 static int git_tag_config(const char *var, const char *value, void *cb)
 {
-   int status = git_gpg_config(var, value, cb);
+   int status;
+
+   if (!strcmp(var, tag.sort)) {
+   if (!value)
+   return config_error_nonbool(var);
+   status = parse_sort_string(value, tag_sort);
+   if (status) {
+   warning(using default lexicographic sort order);
+   tag_sort = STRCMP_SORT;
+   }
+   return 0;
+   }
+
+   status = git_gpg_config(var, value, cb);
if (status)
return status;
if (starts_with(var, column.))
@@ -522,18 +561,8 @@ static int parse_opt_points_at(const struct option *opt 
__attribute__((unused)),
 static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
 {
int *sort = opt-value;
-   int flags = 0;
 
-   if (skip_prefix(arg, -, arg))
-   flags |= REVERSE_SORT;
-
-   if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
-   *sort = VERCMP_SORT;
-
-   if (strcmp(arg, refname))
-   die(_(unsupported sort specification %s), arg);
-   *sort |= flags;
-   return 0;
+   return parse_sort_string(arg, sort);
 }
 
 int cmd_tag(int argc, const char **argv, const char *prefix)
@@ -546,7 +575,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct create_tag_options opt;
char *cleanup_arg = NULL;
int annotate = 0, force

[PATCH 1/3] tag: use skip_prefix instead of magic numbers

From: Jeff King p...@peff.net

Make the parsing of the --sort parameter more readable by having
skip_prefix keep our pointer up to date.

Signed-off-by: Jeff King p...@peff.net
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
Fixed authorship. I don't expect this version to be taken, but it helps me in
review, and I figured it is good to send the whole series.

 builtin/tag.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index ef765563388c..7ccb6f3c581b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -524,18 +524,12 @@ static int parse_opt_sort(const struct option *opt, const 
char *arg, int unset)
int *sort = opt-value;
int flags = 0;
 
-   if (*arg == '-') {
+   if (skip_prefix(arg, -, arg))
flags |= REVERSE_SORT;
-   arg++;
-   }
-   if (starts_with(arg, version:)) {
+
+   if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
*sort = VERCMP_SORT;
-   arg += 8;
-   } else if (starts_with(arg, v:)) {
-   *sort = VERCMP_SORT;
-   arg += 2;
-   } else
-   *sort = STRCMP_SORT;
+
if (strcmp(arg, refname))
die(_(unsupported sort specification %s), arg);
*sort |= flags;
-- 
2.0.1.475.g9b8d714

--
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 Fri, 2014-07-11 at 13:51 -0700, Jacob Keller wrote:
 Add support for configuring default sort ordering for git tags. Command
 line option will override this configured value, using the exact same
 syntax.
 
 Cc: Jeff King p...@peff.net
 Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
 ---
 Updated based on Junio's suggestions, as well as making sure that we don't 
 bail
 if we can't understand config's option. We still print error message followed
 by a warning about using default order. In addition, added a few more tests to
 ensure that these work as expected.
 
  Documentation/config.txt  |  5 
  Documentation/git-tag.txt |  5 +++-
  builtin/tag.c | 61 
 ++-
  t/t7004-tag.sh| 36 
  4 files changed, 90 insertions(+), 17 deletions(-)
 
 diff --git a/Documentation/config.txt b/Documentation/config.txt
 index 1d718bdb9662..c55c22ab7be9 100644
 --- a/Documentation/config.txt
 +++ b/Documentation/config.txt
 @@ -2354,6 +2354,11 @@ submodule.name.ignore::
   --ignore-submodules option. The 'git submodule' commands are not
   affected by this setting.
  
 +tag.sort::
 + This variable controls the sort ordering of tags when displayed by
 + linkgit:git-tag[1]. Without the --sort=value option provided, the
 + value of this variable will be used as the default.
 +
  tar.umask::
   This variable can be used to restrict the permission bits of
   tar archive entries.  The default is 0002, which turns off the
 diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
 index b424a1bc48bb..320908369f06 100644
 --- a/Documentation/git-tag.txt
 +++ b/Documentation/git-tag.txt
 @@ -99,7 +99,9 @@ OPTIONS
   Sort in a specific order. Supported type is refname
   (lexicographic order), version:refname or v:refname (tag
   names are treated as versions). Prepend - to reverse sort
 - order.
 + order. When this option is not given, the sort order defaults to the
 + value configured for the 'tag.sort' variable if it exists, or
 + lexicographic order otherwise. See linkgit:git-config[1].
  
  --column[=options]::
  --no-column::
 @@ -317,6 +319,7 @@ include::date-formats.txt[]
  SEE ALSO
  
  linkgit:git-check-ref-format[1].
 +linkgit:git-config[1].
  
  GIT
  ---
 diff --git a/builtin/tag.c b/builtin/tag.c
 index 7ccb6f3c581b..cb37b26159a6 100644
 --- a/builtin/tag.c
 +++ b/builtin/tag.c
 @@ -32,6 +32,8 @@ static const char * const git_tag_usage[] = {
  #define SORT_MASK   0x7fff
  #define REVERSE_SORT0x8000
  
 +static int tag_sort;
 +
  struct tag_filter {
   const char **patterns;
   int lines;
 @@ -346,9 +348,46 @@ static const char tag_template_nocleanup[] =
   Lines starting with '%c' will be kept; you may remove them
yourself if you want to.\n);
  
 +/*
 + * Parse a sort string, and return 0 if parsed successfully. Will return
 + * non-zero when the sort string does not parse into a known type.
 + */
 +static int parse_sort_string(const char *arg, int *sort)
 +{
 + int type = 0, flags = 0;
 +
 + if (skip_prefix(arg, -, arg))
 + flags |= REVERSE_SORT;
 +
 + if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
 + type = VERCMP_SORT;
 + else
 + type = STRCMP_SORT;
 +
 + if (strcmp(arg, refname))
 + return error(_(unsupported sort specification %s), arg);
 +
 + *sort = (type | flags);
 +
 + return 0;
 +}
 +
  static int git_tag_config(const char *var, const char *value, void *cb)
  {
 - int status = git_gpg_config(var, value, cb);
 + int status;
 +
 + if (!strcmp(var, tag.sort)) {
 + if (!value)
 + return config_error_nonbool(var);
 + status = parse_sort_string(value, tag_sort);
 + if (status) {
 + warning(using default lexicographic sort order);

Oops, I should also use warning(_()) here as well I believe...

Sorry for thrash.

Thanks,
Jake

 + tag_sort = STRCMP_SORT;
 + }
 + return 0;
 + }
 +
 + status = git_gpg_config(var, value, cb);
   if (status)
   return status;
   if (starts_with(var, column.))
 @@ -522,18 +561,8 @@ static int parse_opt_points_at(const struct option *opt 
 __attribute__((unused)),
  static int parse_opt_sort(const struct option *opt, const char *arg, int 
 unset)
  {
   int *sort = opt-value;
 - int flags = 0;
  
 - if (skip_prefix(arg, -, arg))
 - flags |= REVERSE_SORT;
 -
 - if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
 - *sort = VERCMP_SORT;
 -
 - if (strcmp(arg, refname))
 - die(_(unsupported sort specification %s), arg);
 - *sort |= flags;
 - return 0;
 + return parse_sort_string(arg, sort);
  }
  
  int cmd_tag(int argc, const char **argv, const char *prefix)

Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig

On Fri, Jul 11, 2014 at 01:51:35PM -0700, Jacob Keller wrote:

 + if (!strcmp(var, tag.sort)) {
 + if (!value)
 + return config_error_nonbool(var);
 + status = parse_sort_string(value, tag_sort);
 + if (status) {
 + warning(using default lexicographic sort order);
 + tag_sort = STRCMP_SORT;
 + }

I think this is a good compromise of the issues we discussed earlier. As
you noted, it should probably be marked for translation. I'm also not
sure the message content is clear in all situations. If I have a broken
tag.sort, I get:

  $ git config tag.sort bogus
  $ git tag /dev/null
  error: unsupported sort specification bogus
  warning: using default lexicographic sort order

Not too bad, though reminding me that the value bogus came from
tag.sort would be nice. But what if I override it:

  $ git tag --sort=v:refname /dev/null
  error: unsupported sort specification bogus
  warning: using default lexicographic sort order

That's actively wrong, because we are using v:refname from the
command-line. Perhaps we could phrase it like:

  warning: ignoring invalid config option tag.sort

or similar, which helps both cases.

As an aside, I also think the error line could more clearly mark the
literal contents of the variable. E.g., one of:

  error: unsupported sort specification: bogus

or

  error: unsupported sort specification 'bogus'

-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 Fri, 2014-07-11 at 17:06 -0400, Jeff King wrote:
 On Fri, Jul 11, 2014 at 01:51:35PM -0700, Jacob Keller wrote:
 
  +   if (!strcmp(var, tag.sort)) {
  +   if (!value)
  +   return config_error_nonbool(var);
  +   status = parse_sort_string(value, tag_sort);
  +   if (status) {
  +   warning(using default lexicographic sort order);
  +   tag_sort = STRCMP_SORT;
  +   }
 
 I think this is a good compromise of the issues we discussed earlier. As
 you noted, it should probably be marked for translation. I'm also not
 sure the message content is clear in all situations. If I have a broken
 tag.sort, I get:
 
   $ git config tag.sort bogus
   $ git tag /dev/null
   error: unsupported sort specification bogus
   warning: using default lexicographic sort order
 
 Not too bad, though reminding me that the value bogus came from
 tag.sort would be nice. But what if I override it:
 
   $ git tag --sort=v:refname /dev/null
   error: unsupported sort specification bogus
   warning: using default lexicographic sort order
 
 That's actively wrong, because we are using v:refname from the
 command-line. Perhaps we could phrase it like:
 
   warning: ignoring invalid config option tag.sort
 

ok that makes more sense. I can't really avoid printing the warning at
all since config parsing happens before the option parsing.

I like this wording. I will respin again :D

Thanks,
Jake

 or similar, which helps both cases.
 
 As an aside, I also think the error line could more clearly mark the
 literal contents of the variable. E.g., one of:
 
   error: unsupported sort specification: bogus
 
 or
 
   error: unsupported sort specification 'bogus'
 
 -Peff

[PATCH 2/3] tag: fix --sort tests to use cat-\EOF format

The --sort tests should use the better format for expect to maintain
indenting and ensure that no substitution is occurring. This makes
parsing and understanding the tests a bit easier.

Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
 t/t7004-tag.sh | 40 
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e4ab0f5b6419..66010b0e7066 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1385,41 +1385,41 @@ test_expect_success 'lexical sort' '
git tag foo1.6 
git tag foo1.10 
git tag -l --sort=refname foo* actual 
-   cat expect EOF 
-foo1.10
-foo1.3
-foo1.6
-EOF
+   cat expect -\EOF 
+   foo1.10
+   foo1.3
+   foo1.6
+   EOF
test_cmp expect actual
 '
 
 test_expect_success 'version sort' '
git tag -l --sort=version:refname foo* actual 
-   cat expect EOF 
-foo1.3
-foo1.6
-foo1.10
-EOF
+   cat expect -\EOF 
+   foo1.3
+   foo1.6
+   foo1.10
+   EOF
test_cmp expect actual
 '
 
 test_expect_success 'reverse version sort' '
git tag -l --sort=-version:refname foo* actual 
-   cat expect EOF 
-foo1.10
-foo1.6
-foo1.3
-EOF
+   cat expect -\EOF 
+   foo1.10
+   foo1.6
+   foo1.3
+   EOF
test_cmp expect actual
 '
 
 test_expect_success 'reverse lexical sort' '
git tag -l --sort=-refname foo* actual 
-   cat expect EOF 
-foo1.6
-foo1.3
-foo1.10
-EOF
+   cat expect -\EOF 
+   foo1.6
+   foo1.3
+   foo1.10
+   EOF
test_cmp expect actual
 '
 
-- 
2.0.1.475.g9b8d714

--
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] tag: use skip_prefix instead of magic numbers

From: Jeff King p...@peff.net

Make the parsing of the --sort parameter more readable by having
skip_prefix keep our pointer up to date.

Signed-off-by: Jeff King p...@peff.net
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
 builtin/tag.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index ef765563388c..7ccb6f3c581b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -524,18 +524,12 @@ static int parse_opt_sort(const struct option *opt, const 
char *arg, int unset)
int *sort = opt-value;
int flags = 0;
 
-   if (*arg == '-') {
+   if (skip_prefix(arg, -, arg))
flags |= REVERSE_SORT;
-   arg++;
-   }
-   if (starts_with(arg, version:)) {
+
+   if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
*sort = VERCMP_SORT;
-   arg += 8;
-   } else if (starts_with(arg, v:)) {
-   *sort = VERCMP_SORT;
-   arg += 2;
-   } else
-   *sort = STRCMP_SORT;
+
if (strcmp(arg, refname))
die(_(unsupported sort specification %s), arg);
*sort |= flags;
-- 
2.0.1.475.g9b8d714

--
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 v7] tag: support configuring --sort via .gitconfig

Add support for configuring default sort ordering for git tags. Command
line option will override this configured value, using the exact same
syntax.

Cc: Jeff King p...@peff.net
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
Updated warning texts based on Jeff's feedback. Also added translate specifier
to the warning string.

 Documentation/config.txt  |  5 
 Documentation/git-tag.txt |  5 +++-
 builtin/tag.c | 61 ++-
 t/t7004-tag.sh| 36 
 4 files changed, 90 insertions(+), 17 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1d718bdb9662..c55c22ab7be9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2354,6 +2354,11 @@ submodule.name.ignore::
--ignore-submodules option. The 'git submodule' commands are not
affected by this setting.
 
+tag.sort::
+   This variable controls the sort ordering of tags when displayed by
+   linkgit:git-tag[1]. Without the --sort=value option provided, the
+   value of this variable will be used as the default.
+
 tar.umask::
This variable can be used to restrict the permission bits of
tar archive entries.  The default is 0002, which turns off the
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index b424a1bc48bb..320908369f06 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -99,7 +99,9 @@ OPTIONS
Sort in a specific order. Supported type is refname
(lexicographic order), version:refname or v:refname (tag
names are treated as versions). Prepend - to reverse sort
-   order.
+   order. When this option is not given, the sort order defaults to the
+   value configured for the 'tag.sort' variable if it exists, or
+   lexicographic order otherwise. See linkgit:git-config[1].
 
 --column[=options]::
 --no-column::
@@ -317,6 +319,7 @@ include::date-formats.txt[]
 SEE ALSO
 
 linkgit:git-check-ref-format[1].
+linkgit:git-config[1].
 
 GIT
 ---
diff --git a/builtin/tag.c b/builtin/tag.c
index 7ccb6f3c581b..6e0a8ed4d1f9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -32,6 +32,8 @@ static const char * const git_tag_usage[] = {
 #define SORT_MASK   0x7fff
 #define REVERSE_SORT0x8000
 
+static int tag_sort;
+
 struct tag_filter {
const char **patterns;
int lines;
@@ -346,9 +348,46 @@ static const char tag_template_nocleanup[] =
Lines starting with '%c' will be kept; you may remove them
 yourself if you want to.\n);
 
+/*
+ * Parse a sort string, and return 0 if parsed successfully. Will return
+ * non-zero when the sort string does not parse into a known type.
+ */
+static int parse_sort_string(const char *arg, int *sort)
+{
+   int type = 0, flags = 0;
+
+   if (skip_prefix(arg, -, arg))
+   flags |= REVERSE_SORT;
+
+   if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
+   type = VERCMP_SORT;
+   else
+   type = STRCMP_SORT;
+
+   if (strcmp(arg, refname))
+   return error(_(unsupported sort specification '%s'), arg);
+
+   *sort = (type | flags);
+
+   return 0;
+}
+
 static int git_tag_config(const char *var, const char *value, void *cb)
 {
-   int status = git_gpg_config(var, value, cb);
+   int status;
+
+   if (!strcmp(var, tag.sort)) {
+   if (!value)
+   return config_error_nonbool(var);
+   status = parse_sort_string(value, tag_sort);
+   if (status) {
+   warning(_(ignoring configured sort specification from 
'tag.sort'));
+   tag_sort = STRCMP_SORT;
+   }
+   return 0;
+   }
+
+   status = git_gpg_config(var, value, cb);
if (status)
return status;
if (starts_with(var, column.))
@@ -522,18 +561,8 @@ static int parse_opt_points_at(const struct option *opt 
__attribute__((unused)),
 static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
 {
int *sort = opt-value;
-   int flags = 0;
 
-   if (skip_prefix(arg, -, arg))
-   flags |= REVERSE_SORT;
-
-   if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
-   *sort = VERCMP_SORT;
-
-   if (strcmp(arg, refname))
-   die(_(unsupported sort specification %s), arg);
-   *sort |= flags;
-   return 0;
+   return parse_sort_string(arg, sort);
 }
 
 int cmd_tag(int argc, const char **argv, const char *prefix)
@@ -546,7 +575,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct create_tag_options opt;
char *cleanup_arg = NULL;
int annotate = 0, force = 0, lines = -1;
-   int cmdmode = 0, sort = 0;
+   int cmdmode = 0;
const char *msgfile = NULL, *keyid = NULL;
struct

Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig

Jeff King p...@peff.net writes:

 On Fri, Jul 11, 2014 at 10:24:07AM -0700, Jacob Keller wrote:

 Updated to include changes due to Junio's feedback. This has not resolved
 whether we should fail on a configuration error or simply warn. It appears 
 that
 we actually seem to error out more than warn, so I am unsure what the correct
 action is here.

 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?

As we want to be able to enhance semantics of existing configuration
variables without having to introduce new but similar ones, we would
really want to make sure that those who share the same .git/config
or $HOME/.gitconfig across different versions of Git would not have
to suffer too much (i.e. forcing them to config --unset when using
their older Git is not nice).

 It's probably user-unfriendly to be silent for those cases, though. The
 user gets no feedback on why their config value is doing nothing.

 I tend to think that warning is not much better than erroring out. It is
 helpful if you are running a single-shot of an old version (which is
 something that I do a lot when testing old versions), but would quickly
 become irritating if you were actually using an old version of git
 day-to-day.

 I dunno. Maybe it is worth making life easier for people in the former
 category.

... former cat meaning less irritating for single-shot use?  I
dunno...

 +static int parse_sort_string(const char *arg, int *sort)
 +{
 +int type = 0, flags = 0;
 +
 +if (skip_prefix(arg, -, arg))
 +flags |= REVERSE_SORT;
 +
 +if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
 +type = VERCMP_SORT;
 +else
 +type = STRCMP_SORT;
 +
 +if (strcmp(arg, refname))
 +return error(_(unsupported sort specification %s), arg);
 +
 +*sort = (type | flags);
 +
 +return 0;
 +}

 Regardless of how we handle the error, I think this version that
 assembles the final bitfield at the end is easier to read than the
 original.

Yes, this part really is nicely done, I agree.
--
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

git-fast-import bug?

2014-07-11 Thread Duane Murphy

git-fast-import is not writing a commit even after a checkpoint/progress 
command.

See my previous message git p4 diff-tree ambiguous argument error. 

The error in git-p4 is caused by git not writing the commit even after 
git-fast-import has been given a checkpoint and progress command.

On initial use of git p4 to sync a p4 repository, the commits are written 
properly. But on a subsequent run the commit is not flushed to the file system 
during the run. Specifically, I can stop the git-p4 command directly after the 
progress checkpoint command (see the checkpoint() function in git-p4). The file 
is not found. If I abort/exit the application at that point, the file appears. 

There is a pattern of behavior here that is consistent but I am unable to 
understand. A bare repository works fine. An already populated repository does 
not work until the app is quit. What would cause git-fast-import to _NOT_ flush 
the file?

This certainly seems like a bug. But I don't know enough of the git internals 
to reproduce.

Suggestions on how to test or isolate this problem?

Thanks

Reproduced consistently on two systems:

$ git --version 
git version 1.8.5.2 (Apple Git-48) 
$ python --version 
Python 2.7.5 
$ uname -a 
Darwin Kernel Version 13.3.0: Tue Jun  3 21:27:35 PDT 2014; 
root:xnu-2422.110.17~1/RELEASE_X86_64 x86_64 

and 

$ git --version 
git version 1.7.12.4 
$ python --version 
Python 2.6.6 
OS: GNU/Linux 2.6.32-431.el6.x86_64 


--
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

On Fri, 2014-07-11 at 14:54 -0700, Junio C Hamano wrote:
 Jeff King p...@peff.net writes:
 
  On Fri, Jul 11, 2014 at 10:24:07AM -0700, Jacob Keller wrote:
 
  Updated to include changes due to Junio's feedback. This has not resolved
  whether we should fail on a configuration error or simply warn. It appears 
  that
  we actually seem to error out more than warn, so I am unsure what the 
  correct
  action is here.
 
  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?
 
 As we want to be able to enhance semantics of existing configuration
 variables without having to introduce new but similar ones, we would
 really want to make sure that those who share the same .git/config
 or $HOME/.gitconfig across different versions of Git would not have
 to suffer too much (i.e. forcing them to config --unset when using
 their older Git is not nice).
 
  It's probably user-unfriendly to be silent for those cases, though. The
  user gets no feedback on why their config value is doing nothing.
 
  I tend to think that warning is not much better than erroring out. It is
  helpful if you are running a single-shot of an old version (which is
  something that I do a lot when testing old versions), but would quickly
  become irritating if you were actually using an old version of git
  day-to-day.
 
  I dunno. Maybe it is worth making life easier for people in the former
  category.
 
 ... former cat meaning less irritating for single-shot use?  I
 dunno...
 
  +static int parse_sort_string(const char *arg, int *sort)
  +{
  +  int type = 0, flags = 0;
  +
  +  if (skip_prefix(arg, -, arg))
  +  flags |= REVERSE_SORT;
  +
  +  if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
  +  type = VERCMP_SORT;
  +  else
  +  type = STRCMP_SORT;
  +
  +  if (strcmp(arg, refname))
  +  return error(_(unsupported sort specification %s), arg);
  +
  +  *sort = (type | flags);
  +
  +  return 0;
  +}
 
  Regardless of how we handle the error, I think this version that
  assembles the final bitfield at the end is easier to read than the
  original.
 
 Yes, this part really is nicely done, I agree.

The current revision of the patch prints an error and warning about not
using the configured tag value. It does still run. I added a test to
ensure this as well.

The easiest way to change overall behavior is to change the git-config's
die_on_error to be false, so that we no longer die on configuration
errors.

It appears this would resolve the issue for many configuration options
(not all, as I think a few are still hard coded to die) but it would be
a change that is well outside the scope of this patch.

I think that for now behavior I added is good, as we *know* that
tag.sort will add new parameters in the near future, so it makes no
sense to have it die on a config that is only slightly newer than the
git version.

Glad I could help. Junio if you could review the v7 I sent a bit ago for
any possible mistakes that I forgot to clean up that would be great.

Thanks,
Jake
N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf

Re: [PATCH 3/3] tag: support configuring --sort via .gitconfig

Jeff King p...@peff.net writes:

 On Fri, Jul 11, 2014 at 01:51:35PM -0700, Jacob Keller wrote:

 +if (!strcmp(var, tag.sort)) {
 +if (!value)
 +return config_error_nonbool(var);
 +status = parse_sort_string(value, tag_sort);
 +if (status) {
 +warning(using default lexicographic sort order);
 +tag_sort = STRCMP_SORT;
 +}

 I think this is a good compromise of the issues we discussed earlier. As
 you noted, it should probably be marked for translation. I'm also not
 sure the message content is clear in all situations. If I have a broken
 tag.sort, I get:

   $ git config tag.sort bogus
   $ git tag /dev/null
   error: unsupported sort specification bogus
   warning: using default lexicographic sort order

 Not too bad, though reminding me that the value bogus came from
 tag.sort would be nice. But what if I override it:

   $ git tag --sort=v:refname /dev/null
   error: unsupported sort specification bogus
   warning: using default lexicographic sort order

 That's actively wrong, because we are using v:refname from the
 command-line.  Perhaps we could phrase it like:

   warning: ignoring invalid config option tag.sort

 or similar, which helps both cases.

Hmph.  Looks like a mild-enough middle ground, I guess.  As
parse_sort_string() is private for git tag implementation, I
actually would not mind if we taught a bit more context to it and
phrase its messages differently.  Perhaps something like this, where
the config parser will tell what variable it came from with var
and the command line parser will pass NULL there.

static int parse_sort_string(const char *var, const char *value, int *sort)
{
...
if (strcmp(value, refname)) {
if (!var)
return error(_(unsupported sort specification '%s'), 
value);
else {
warning(_(unsupported sort specification '%s' in 
variable '%s'),
var, value);
return -1;
}
}

*sort = (type | flags);

return 0;
}

 As an aside, I also think the error line could more clearly mark the
 literal contents of the variable. E.g., one of:

   error: unsupported sort specification: bogus

 or

   error: unsupported sort specification 'bogus'

Yup.
--
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 1/2] symlinks: remove PATH_MAX limitation

Karsten Blees karsten.bl...@gmail.com writes:

 Am 07.07.2014 20:30, schrieb Junio C Hamano:
 Karsten Blees karsten.bl...@gmail.com writes:
 
 The above cache_def_free(cache) does not free the cache itself, but
 only its associated data, so the name cache_def_free() is somewhat
 misleading.
 

 You already merged this to master (kb/path-max-must-go lol), should
 I send a fixup! s/cache_def_free/cache_def_clear/ or is it OK as is?

I do not think a fix-up would hurt other topics in flight, so
please do so.

--
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 v1 3/4] hashmap: add simplified hashmap_get_from_hash() API

Karsten Blees karsten.bl...@gmail.com writes:

 In other words, why isn't hashmap_get() more like this:
  ...
 with hashmap_entry_init() purely a static helper in hashmap.c?
 
 1. Performance

OK.

 2. Simplicity

 Hashmap clients will typically provide small, type safe wrappers around the
 hashmap API.

OK.

 3. Compound keys

 The key doesn't always consist of just a single word. E.g. for struct
 dir_entry, the key is [char *name, int len]. So an API like this:

   void *hashmap_get(const struct hashmap *map, const void *key)

 won't do in the general case (unless you require clients to define their
 own key structure in addition to the entry structure...).

Yeah, I was being silly.  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

Re: [PATCH v3] http: Add Accept-Language header if possible

2014-07-11 Thread Eric Sunshine

On Fri, Jul 11, 2014 at 1:35 PM, Jeff King p...@peff.net wrote:
 On Sat, Jul 12, 2014 at 01:52:53AM +0900, Yi EungJun wrote:
 Add an Accept-Language header which indicates the user's preferred
 languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.

 Examples:
   LANGUAGE= - 
   LANGUAGE=ko:en - Accept-Language: ko, en; q=0.9, *; q=0.1
   LANGUAGE=ko LANG=en_US.UTF-8 - Accept-Language: ko, *; q=0.1
   LANGUAGE= LANG=en_US.UTF-8 - Accept-Language: en-US, *; q=0.1

 This gives git servers a chance to display remote error messages in
 the user's preferred language.

 Thanks, this is looking much nicer. Most of my comments are on style:

 +static const char* get_preferred_languages() {
 +const char* retval;

 A few style nits:

Also, this is C, not C++, so don't forget void:

static const char *get_preferred_languages(void)
{

   1. We usually put a function's opening brace on a new line.

   2. We usually put the asterisk in a pointer declaration with the
  variable name (const char *retval). This one appears elsewhere in
  the patch.

   3. This line seems to be indented with spaces instead of tabs.
--
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

Karsten Blees karsten.bl...@gmail.com writes:

 Anyways, I'd like to kindly withdraw this patch in favor of Duy's version.

 http://article.gmane.org/gmane.comp.version-control.git/248310

Thanks; I've already reverted it from 'next'.

Is Duy's patch still viable?
--
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 Fri, 2014-07-11 at 15:17 -0700, Junio C Hamano wrote:
 Jeff King p...@peff.net writes:
 
  On Fri, Jul 11, 2014 at 01:51:35PM -0700, Jacob Keller wrote:
 
  +  if (!strcmp(var, tag.sort)) {
  +  if (!value)
  +  return config_error_nonbool(var);
  +  status = parse_sort_string(value, tag_sort);
  +  if (status) {
  +  warning(using default lexicographic sort order);
  +  tag_sort = STRCMP_SORT;
  +  }
 
  I think this is a good compromise of the issues we discussed earlier. As
  you noted, it should probably be marked for translation. I'm also not
  sure the message content is clear in all situations. If I have a broken
  tag.sort, I get:
 
$ git config tag.sort bogus
$ git tag /dev/null
error: unsupported sort specification bogus
warning: using default lexicographic sort order
 
  Not too bad, though reminding me that the value bogus came from
  tag.sort would be nice. But what if I override it:
 
$ git tag --sort=v:refname /dev/null
error: unsupported sort specification bogus
warning: using default lexicographic sort order
 
  That's actively wrong, because we are using v:refname from the
  command-line.  Perhaps we could phrase it like:
 
warning: ignoring invalid config option tag.sort
 
  or similar, which helps both cases.
 
 Hmph.  Looks like a mild-enough middle ground, I guess.  As
 parse_sort_string() is private for git tag implementation, I
 actually would not mind if we taught a bit more context to it and
 phrase its messages differently.  Perhaps something like this, where
 the config parser will tell what variable it came from with var
 and the command line parser will pass NULL there.
 
 static int parse_sort_string(const char *var, const char *value, int *sort)
 {
   ...
   if (strcmp(value, refname)) {
   if (!var)
   return error(_(unsupported sort specification '%s'), 
 value);
   else {
   warning(_(unsupported sort specification '%s' in 
 variable '%s'),
   var, value);
   return -1;
   }
   }
 
   *sort = (type | flags);
 
   return 0;
 }
 

Ok..  I suppose that could be done.

Thanks,
Jake

  As an aside, I also think the error line could more clearly mark the
  literal contents of the variable. E.g., one of:
 
error: unsupported sort specification: bogus
 
  or
 
error: unsupported sort specification 'bogus'
 
 Yup.

Re: [PATCH 1/3] tag: use skip_prefix instead of magic numbers

Jacob Keller jacob.e.kel...@intel.com writes:

 From: Jeff King p...@peff.net

 Make the parsing of the --sort parameter more readable by having
 skip_prefix keep our pointer up to date.

 Signed-off-by: Jeff King p...@peff.net
 Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
 ---
  builtin/tag.c | 14 --
  1 file changed, 4 insertions(+), 10 deletions(-)

 diff --git a/builtin/tag.c b/builtin/tag.c
 index ef765563388c..7ccb6f3c581b 100644
 --- a/builtin/tag.c
 +++ b/builtin/tag.c
 @@ -524,18 +524,12 @@ static int parse_opt_sort(const struct option *opt, 
 const char *arg, int unset)
   int *sort = opt-value;
   int flags = 0;
  
 - if (*arg == '-') {
 + if (skip_prefix(arg, -, arg))
   flags |= REVERSE_SORT;
 - arg++;
 - }
 - if (starts_with(arg, version:)) {
 +
 + if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
   *sort = VERCMP_SORT;
 - arg += 8;
 - } else if (starts_with(arg, v:)) {
 - *sort = VERCMP_SORT;
 - arg += 2;
 - } else
 - *sort = STRCMP_SORT;
 +

By losing *sort = STRCMP_SORT, the version after this patch would
stop clearing what is pointed by opt-value, so

git tag --sort=v:refname --sort=refname

would no longer implement the last one wins semantics, no?

Am I misreading the patch?  I somehow thought Peff's original was
clearing the variable...

   if (strcmp(arg, refname))
   die(_(unsupported sort specification %s), arg);
   *sort |= flags;
--
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/4 v6] cache-tree: subdirectory tests

On Fri, 2014-07-11 at 08:40 -0700, Junio C Hamano wrote:
 Junio C Hamano gits...@pobox.com writes:
 
  +   sed -n -e s/[0-9]* subtrees// -e '/#(ref)/d' -e '/^invalid /p' 
  actual 
 
  Is the second one to remove #(ref), which appears for a good
  reference cache tree entry shown for comparison, necessary?  Do
  they ever begin with invalid?  If they ever begin with invalid
  that itself may even be a noteworthy breakage to catch, no?
 
 Answering to myself...
 
 Because test-dump-cache-tree uses DRY_RUN to create only an in-core
 copy of tree object, and we notice that the reference cache-tree
 created in the tests contains the object name of a tree that does
 not yet exist in the object database.  We get invalid #(ref) for
 such node.
 
 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?

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?

--
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/4 v6] cache-tree: subdirectory tests

On Fri, 2014-07-11 at 08:27 -0700, Junio C Hamano wrote:
 Eric Sunshine sunsh...@sunshineco.com writes:
 
  On Thu, Jul 10, 2014 at 8:31 PM, David Turner dtur...@twopensource.com 
  wrote:
  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 
 
 Hmm.  This will always expect that the top-level is invalid, even
 when $# is 0.  It is OK if you never need to use this to test that a
 cache-tree is fully valid, but is it something we want to check?

We have test_cache_tree to check that it's fully valid.

 Existing tests are mostly about cache-tree is populated fully at a
 few strategic, well known and easy places and then it degrades over
 time, but after all your series is adding more places to that set
 of a few places, so we may want to make sure that future breakages
 to the new code paths that repair the cache-tree are caught by
 these tests.

This patchset un-failed initial commit has cache-tree, and added
commit in child dir has cache-tree and partial commit gives
cache-tree.  I've just added a test for interactive commit; when you
take a look at the next patchset, you can let me know if this seems
sufficient to you.

--
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 1/3] tag: use skip_prefix instead of magic numbers

On Fri, 2014-07-11 at 15:44 -0700, Junio C Hamano wrote:
 Jacob Keller jacob.e.kel...@intel.com writes:
 
  From: Jeff King p...@peff.net
 
  Make the parsing of the --sort parameter more readable by having
  skip_prefix keep our pointer up to date.
 
  Signed-off-by: Jeff King p...@peff.net
  Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
  ---
   builtin/tag.c | 14 --
   1 file changed, 4 insertions(+), 10 deletions(-)
 
  diff --git a/builtin/tag.c b/builtin/tag.c
  index ef765563388c..7ccb6f3c581b 100644
  --- a/builtin/tag.c
  +++ b/builtin/tag.c
  @@ -524,18 +524,12 @@ static int parse_opt_sort(const struct option *opt, 
  const char *arg, int unset)
  int *sort = opt-value;
  int flags = 0;
   
  -   if (*arg == '-') {
  +   if (skip_prefix(arg, -, arg))
  flags |= REVERSE_SORT;
  -   arg++;
  -   }
  -   if (starts_with(arg, version:)) {
  +
  +   if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
  *sort = VERCMP_SORT;
  -   arg += 8;
  -   } else if (starts_with(arg, v:)) {
  -   *sort = VERCMP_SORT;
  -   arg += 2;
  -   } else
  -   *sort = STRCMP_SORT;
  +
 
 By losing *sort = STRCMP_SORT, the version after this patch would
 stop clearing what is pointed by opt-value, so
 
   git tag --sort=v:refname --sort=refname
 
 would no longer implement the last one wins semantics, no?
 
 Am I misreading the patch?  I somehow thought Peff's original was
 clearing the variable...
 
  if (strcmp(arg, refname))
  die(_(unsupported sort specification %s), arg);
  *sort |= flags;

Indeed. My patch fixes this up but I will re-work this so we don't
introduce an inbetween bug :)

Thanks,
Jake

[PATCH 2/3] tag: fix --sort tests to use cat-\EOF format

The --sort tests should use the better format for expect to maintain
indenting and ensure that no substitution is occurring. This makes
parsing and understanding the tests a bit easier.

Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
 t/t7004-tag.sh | 40 
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e4ab0f5b6419..66010b0e7066 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1385,41 +1385,41 @@ test_expect_success 'lexical sort' '
git tag foo1.6 
git tag foo1.10 
git tag -l --sort=refname foo* actual 
-   cat expect EOF 
-foo1.10
-foo1.3
-foo1.6
-EOF
+   cat expect -\EOF 
+   foo1.10
+   foo1.3
+   foo1.6
+   EOF
test_cmp expect actual
 '
 
 test_expect_success 'version sort' '
git tag -l --sort=version:refname foo* actual 
-   cat expect EOF 
-foo1.3
-foo1.6
-foo1.10
-EOF
+   cat expect -\EOF 
+   foo1.3
+   foo1.6
+   foo1.10
+   EOF
test_cmp expect actual
 '
 
 test_expect_success 'reverse version sort' '
git tag -l --sort=-version:refname foo* actual 
-   cat expect EOF 
-foo1.10
-foo1.6
-foo1.3
-EOF
+   cat expect -\EOF 
+   foo1.10
+   foo1.6
+   foo1.3
+   EOF
test_cmp expect actual
 '
 
 test_expect_success 'reverse lexical sort' '
git tag -l --sort=-refname foo* actual 
-   cat expect EOF 
-foo1.6
-foo1.3
-foo1.10
-EOF
+   cat expect -\EOF 
+   foo1.6
+   foo1.3
+   foo1.10
+   EOF
test_cmp expect actual
 '
 
-- 
2.0.1.475.g9b8d714

--
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] tag: use skip_prefix instead of magic numbers

From: Jeff King p...@peff.net

Make the parsing of the --sort parameter more readable by having
skip_prefix keep our pointer up to date.

Signed-off-by: Jeff King p...@peff.net
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
Fixed issue with patch in that we dropped the reset to STRCMP_SORT, discovered
by Junio.

 builtin/tag.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index ef765563388c..9d7643f127e7 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -524,18 +524,14 @@ static int parse_opt_sort(const struct option *opt, const 
char *arg, int unset)
int *sort = opt-value;
int flags = 0;
 
-   if (*arg == '-') {
+   if (skip_prefix(arg, -, arg))
flags |= REVERSE_SORT;
-   arg++;
-   }
-   if (starts_with(arg, version:)) {
+
+   if (skip_prefix(arg, version:, arg) || skip_prefix(arg, v:, arg))
*sort = VERCMP_SORT;
-   arg += 8;
-   } else if (starts_with(arg, v:)) {
-   *sort = VERCMP_SORT;
-   arg += 2;
-   } else
+   else
*sort = STRCMP_SORT;
+
if (strcmp(arg, refname))
die(_(unsupported sort specification %s), arg);
*sort |= flags;
-- 
2.0.1.475.g9b8d714

--
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 v7 1/4] cache-tree: Create/update cache-tree on checkout

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 07cf555..054214f 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 v7 2/4] test-dump-cache-tree: invalid trees are not errors

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 v7 3/4] cache-tree: subdirectory tests

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 4/4 v6] cache-tree: Write updated cache-tree after commit

On Fri, 2014-07-11 at 08:52 -0700, Junio C Hamano wrote:
 David Turner dtur...@twopensource.com writes:
 
  @@ -16,8 +16,34 @@ cmp_cache_tree () {
   # We don't bother with actually checking the SHA1:
   # test-dump-cache-tree already verifies that all existing data is
   # correct.
 
 Is this statement now stale and needs to be removed?

I think it is still accurate; we still don't bother to check SHAs and
test-dump-cache-tree still does the comparison.

--
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

Am 12.07.2014 00:29, schrieb Junio C Hamano:
 Karsten Blees karsten.bl...@gmail.com writes:
 
 Anyways, I'd like to kindly withdraw this patch in favor of Duy's version.

 http://article.gmane.org/gmane.comp.version-control.git/248310
 
 Thanks; I've already reverted it from 'next'.
 
 Is Duy's patch still viable?
 

I think so. It fixes the segfault with long paths on Windows as well
(Tested-by: me), uses strbuf APIs as Peff suggested, and initializes the
strbuf with PATH_MAX (i.e. no reallocs in the common case either ;-) ).

AFAICT the first two patches of that series are also completely unrelated
to the untracked-cache, so we may want to fast-track these?

[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?

--
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

2014-07-11 Thread Eric Sunshine

On Fri, Jul 11, 2014 at 7:22 PM, David Turner dtur...@twopensource.com wrote:
 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
 diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
 index 3a3342e..57f263f 100755
 --- a/t/t0090-cache-tree.sh
 +++ b/t/t0090-cache-tree.sh
 @@ -90,37 +128,86 @@ test_expect_success 'test-scrap-cache-tree works' '

  test_expect_success 'second commit has cache-tree' '
 test_commit bar 
 -   test_shallow_cache_tree
 +   test_cache_tree
 +'
 +
 +test_expect_success 'commit -i gives cache-tree' '
 +   git checkout current 
 +   cat -\EOT foo.c 
 +   int foo()
 +   {
 +   return 42;
 +   }
 +   int bar()
 +   {
 +   return 42;
 +   }
 +   EOT
 +   git add foo.c
 +   test_invalid_cache_tree
 +   git commit -m add a file
 +   test_cache_tree

Broken -chain on all four lines above.

 +   cat -\EOT foo.c 
 +   int foo()
 +   {
 +   return 43;
 +   }
 +   int bar()
 +   {
 +   return 44;
 +   }
 +   EOT
 +   (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.

 +   test_cache_tree
 +'
 +
 +test_expect_success 'commit in child dir has cache-tree' '
 +   mkdir dir 
 +   dir/child.t 
 +   git add dir/child.t 
 +   git commit -m dir/child.t 
 +   test_cache_tree
  '

  test_expect_success 'reset --hard gives cache-tree' '
 test-scrap-cache-tree 
 git reset --hard 
 -   test_shallow_cache_tree
 +   test_cache_tree
  '

  test_expect_success 'reset --hard without index gives cache-tree' '
 rm -f .git/index 
 git reset --hard 
 -   test_shallow_cache_tree
 +   test_cache_tree
  '

  test_expect_success 'checkout gives cache-tree' '
 git tag current 
 git checkout HEAD^ 
 -   test_shallow_cache_tree
 +   test_cache_tree
  '

  test_expect_success 'checkout -b gives cache-tree' '
 git checkout current 
 git checkout -b prev HEAD^ 
 -   test_shallow_cache_tree
 +   test_cache_tree
  '

  test_expect_success 'checkout -B gives cache-tree' '
 git checkout current 
 git checkout -B prev HEAD^ 
 -   test_shallow_cache_tree
 +   test_cache_tree
 +'
 +
 +test_expect_success 'partial commit gives cache-tree' '
 +   git checkout -b partial no-children 
 +   test_commit one 
 +   test_commit two 
 +   echo some change one.t 
 +   git add one.t 
 +   echo some other change two.t 
 +   git commit two.t -m partial 
 +   test_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 v8 00/17] add performance tracing facility

Changes since v7:
[04]: Fixed -Wextra compiler warnings, thanks to Ramsay Jones.
[11]: Added #ifndef TRACE_CONTEXT, explained why __FILE__ :
  __FUNCTION__ doesn't work.
[17]: New Documentation/technical/api-trace.txt


Karsten Blees (17):
  trace: move trace declarations from cache.h to new trace.h
  trace: consistently name the format parameter
  trace: remove redundant printf format attribute
  trace: improve trace performance
  Documentation/git.txt: improve documentation of 'GIT_TRACE*' variables
  sha1_file: change GIT_TRACE_PACK_ACCESS logging to use trace API
  trace: add infrastructure to augment trace output with additional info
  trace: disable additional trace output for unit tests
  trace: add current timestamp to all trace output
  trace: move code around, in preparation to file:line output
  trace: add 'file:line' to all trace output
  trace: add high resolution timer function to debug performance issues
  trace: add trace_performance facility to debug performance issues
  git: add performance tracing for git's main() function to debug
scripts
  wt-status: simplify performance measurement by using getnanotime()
  progress: simplify performance measurement by using getnanotime()
  api-trace.txt: add trace API documentation

 Documentation/git.txt |  59 --
 Documentation/technical/api-trace.txt |  97 +
 Makefile  |   7 +
 builtin/receive-pack.c|   2 +-
 cache.h   |  13 +-
 commit.h  |   1 +
 config.mak.uname  |   1 +
 git-compat-util.h |   4 +
 git.c |   2 +
 pkt-line.c|   8 +-
 progress.c|  71 +++
 sha1_file.c   |  30 +--
 shallow.c |  10 +-
 t/test-lib.sh |   4 +
 trace.c   | 369 --
 trace.h   | 113 +++
 wt-status.c   |  14 +-
 17 files changed, 629 insertions(+), 176 deletions(-)
 create mode 100644 Documentation/technical/api-trace.txt
 create mode 100644 trace.h

-- 
2.0.0.406.g2e9ef9b

--
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 v8 01/17] trace: move trace declarations from cache.h to new trace.h

Also include direct dependencies (strbuf.h and git-compat-util.h for
__attribute__) so that trace.h can be used independently of cache.h, e.g.
in test programs.

Signed-off-by: Karsten Blees bl...@dcon.de
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 cache.h | 13 ++---
 trace.h | 17 +
 2 files changed, 19 insertions(+), 11 deletions(-)
 create mode 100644 trace.h

diff --git a/cache.h b/cache.h
index cbe1935..466f6b3 100644
--- a/cache.h
+++ b/cache.h
@@ -7,6 +7,7 @@
 #include advice.h
 #include gettext.h
 #include convert.h
+#include trace.h
 
 #include SHA1_HEADER
 #ifndef git_SHA_CTX
@@ -1377,17 +1378,7 @@ extern void *alloc_tag_node(void);
 extern void *alloc_object_node(void);
 extern void alloc_report(void);
 
-/* trace.c */
-__attribute__((format (printf, 1, 2)))
-extern void trace_printf(const char *format, ...);
-__attribute__((format (printf, 2, 3)))
-extern void trace_argv_printf(const char **argv, const char *format, ...);
-extern void trace_repo_setup(const char *prefix);
-extern int trace_want(const char *key);
-__attribute__((format (printf, 2, 3)))
-extern void trace_printf_key(const char *key, const char *fmt, ...);
-extern void trace_strbuf(const char *key, const struct strbuf *buf);
-
+/* pkt-line.c */
 void packet_trace_identity(const char *prog);
 
 /* add */
diff --git a/trace.h b/trace.h
new file mode 100644
index 000..6cc4541
--- /dev/null
+++ b/trace.h
@@ -0,0 +1,17 @@
+#ifndef TRACE_H
+#define TRACE_H
+
+#include git-compat-util.h
+#include strbuf.h
+
+__attribute__((format (printf, 1, 2)))
+extern void trace_printf(const char *format, ...);
+__attribute__((format (printf, 2, 3)))
+extern void trace_argv_printf(const char **argv, const char *format, ...);
+extern void trace_repo_setup(const char *prefix);
+extern int trace_want(const char *key);
+__attribute__((format (printf, 2, 3)))
+extern void trace_printf_key(const char *key, const char *fmt, ...);
+extern void trace_strbuf(const char *key, const struct strbuf *buf);
+
+#endif /* TRACE_H */
-- 
2.0.0.406.g2e9ef9b

--
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 v8 02/17] trace: consistently name the format parameter

The format parameter to trace_printf functions is sometimes abbreviated
'fmt'. Rename to 'format' everywhere (consistent with POSIX' printf
specification).

Signed-off-by: Karsten Blees bl...@dcon.de
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 trace.c | 22 +++---
 trace.h |  2 +-
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/trace.c b/trace.c
index 08180a9..37a7fa9 100644
--- a/trace.c
+++ b/trace.c
@@ -62,7 +62,7 @@ static int get_trace_fd(const char *key, int *need_close)
 static const char err_msg[] = Could not trace into fd given by 
GIT_TRACE environment variable;
 
-static void trace_vprintf(const char *key, const char *fmt, va_list ap)
+static void trace_vprintf(const char *key, const char *format, va_list ap)
 {
struct strbuf buf = STRBUF_INIT;
 
@@ -70,25 +70,25 @@ static void trace_vprintf(const char *key, const char *fmt, 
va_list ap)
return;
 
set_try_to_free_routine(NULL);  /* is never reset */
-   strbuf_vaddf(buf, fmt, ap);
+   strbuf_vaddf(buf, format, ap);
trace_strbuf(key, buf);
strbuf_release(buf);
 }
 
 __attribute__((format (printf, 2, 3)))
-void trace_printf_key(const char *key, const char *fmt, ...)
+void trace_printf_key(const char *key, const char *format, ...)
 {
va_list ap;
-   va_start(ap, fmt);
-   trace_vprintf(key, fmt, ap);
+   va_start(ap, format);
+   trace_vprintf(key, format, ap);
va_end(ap);
 }
 
-void trace_printf(const char *fmt, ...)
+void trace_printf(const char *format, ...)
 {
va_list ap;
-   va_start(ap, fmt);
-   trace_vprintf(GIT_TRACE, fmt, ap);
+   va_start(ap, format);
+   trace_vprintf(GIT_TRACE, format, ap);
va_end(ap);
 }
 
@@ -106,7 +106,7 @@ void trace_strbuf(const char *key, const struct strbuf *buf)
close(fd);
 }
 
-void trace_argv_printf(const char **argv, const char *fmt, ...)
+void trace_argv_printf(const char **argv, const char *format, ...)
 {
struct strbuf buf = STRBUF_INIT;
va_list ap;
@@ -117,8 +117,8 @@ void trace_argv_printf(const char **argv, const char *fmt, 
...)
return;
 
set_try_to_free_routine(NULL);  /* is never reset */
-   va_start(ap, fmt);
-   strbuf_vaddf(buf, fmt, ap);
+   va_start(ap, format);
+   strbuf_vaddf(buf, format, ap);
va_end(ap);
 
sq_quote_argv(buf, argv, 0);
diff --git a/trace.h b/trace.h
index 6cc4541..8fea50b 100644
--- a/trace.h
+++ b/trace.h
@@ -11,7 +11,7 @@ extern void trace_argv_printf(const char **argv, const char 
*format, ...);
 extern void trace_repo_setup(const char *prefix);
 extern int trace_want(const char *key);
 __attribute__((format (printf, 2, 3)))
-extern void trace_printf_key(const char *key, const char *fmt, ...);
+extern void trace_printf_key(const char *key, const char *format, ...);
 extern void trace_strbuf(const char *key, const struct strbuf *buf);
 
 #endif /* TRACE_H */
-- 
2.0.0.406.g2e9ef9b

--
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 v8 03/17] trace: remove redundant printf format attribute

trace_printf_key() is the only non-static function that duplicates the
printf format attribute in the .c file, remove it for consistency.

Signed-off-by: Karsten Blees bl...@dcon.de
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 trace.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/trace.c b/trace.c
index 37a7fa9..3e31558 100644
--- a/trace.c
+++ b/trace.c
@@ -75,7 +75,6 @@ static void trace_vprintf(const char *key, const char 
*format, va_list ap)
strbuf_release(buf);
 }
 
-__attribute__((format (printf, 2, 3)))
 void trace_printf_key(const char *key, const char *format, ...)
 {
va_list ap;
-- 
2.0.0.406.g2e9ef9b

--
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 v8 04/17] trace: improve trace performance