Re: [PATCH v6 00/32] Support multiple checkouts
Tested-by: Dennis Kaarsemaker I've been using this branch for a little while now and have no breakages to report. Max Kirillov reported some bugs in the interaction with submodules which I plan to chase when I have some time unless someone beats me to it :) On wo, 2014-07-09 at 14:32 +0700, Nguyễn Thái Ngọc Duy wrote: > This is basically a reroll from what was parked on 'pu' with two > new patches at the end: one to not share info/sparse-checkout > across worktrees, and one to allow 'checkout --to` from a bare > repository. > > I cherry-picked two patches from jk/xstrfmt (on next) because they > result in non-trivial conflicts. When this series is merged with > jk/xstrfmt, you still get conflicts in environment.c, but you can just > pick up my changes and drop Jeff's. > > Dennis Kaarsemaker (1): > checkout: don't require a work tree when checking out into a new one > > Jeff King (2): > setup_git_env: use git_pathdup instead of xmalloc + sprintf > setup_git_env(): introduce git_path_from_env() helper > > Nguyễn Thái Ngọc Duy (29): > path.c: make get_pathname() return strbuf instead of static buffer > path.c: make get_pathname() call sites return const char * > git_snpath(): retire and replace with strbuf_git_path() > path.c: rename vsnpath() to do_git_path() > path.c: group git_path(), git_pathdup() and strbuf_git_path() together > git_path(): be aware of file relocation in $GIT_DIR > *.sh: respect $GIT_INDEX_FILE > reflog: avoid constructing .lock path with git_path > fast-import: use git_path() for accessing .git dir instead of get_git_dir() > commit: use SEQ_DIR instead of hardcoding "sequencer" > $GIT_COMMON_DIR: a new environment variable > git-sh-setup.sh: use rev-parse --git-path to get $GIT_DIR/objects > *.sh: avoid hardcoding $GIT_DIR/hooks/... > git-stash: avoid hardcoding $GIT_DIR/logs/ > setup.c: convert is_git_directory() to use strbuf > setup.c: detect $GIT_COMMON_DIR in is_git_directory() > setup.c: convert check_repository_format_gently to use strbuf > setup.c: detect $GIT_COMMON_DIR check_repository_format_gently() > setup.c: support multi-checkout repo setup > wrapper.c: wrapper to open a file, fprintf then close > use new wrapper write_file() for simple file writing > checkout: support checking out into a new working directory > checkout: clean up half-prepared directories in --to mode > checkout: detach if the branch is already checked out elsewhere > prune: strategies for linked checkouts > gc: style change -- no SP before closing bracket > gc: support prune --repos > count-objects: report unused files in $GIT_DIR/repos/... > git_path(): keep "info/sparse-checkout" per work-tree > > Documentation/config.txt | 9 ++ > Documentation/git-checkout.txt | 34 + > Documentation/git-prune.txt| 3 + > Documentation/git-rev-parse.txt| 10 ++ > Documentation/git.txt | 9 ++ > Documentation/gitrepository-layout.txt | 75 -- > builtin/branch.c | 4 +- > builtin/checkout.c | 248 > - > builtin/clone.c| 9 +- > builtin/commit.c | 2 +- > builtin/count-objects.c| 4 +- > builtin/fetch.c| 5 +- > builtin/fsck.c | 4 +- > builtin/gc.c | 21 ++- > builtin/init-db.c | 7 +- > builtin/prune.c| 74 ++ > builtin/receive-pack.c | 2 +- > builtin/reflog.c | 2 +- > builtin/remote.c | 2 +- > builtin/repack.c | 8 +- > builtin/rev-parse.c| 11 ++ > cache.h| 17 ++- > daemon.c | 11 +- > environment.c | 45 -- > fast-import.c | 7 +- > git-am.sh | 22 +-- > git-pull.sh| 2 +- > git-rebase--interactive.sh | 6 +- > git-rebase--merge.sh | 6 +- > git-rebase.sh | 4 +- > git-sh-setup.sh| 2 +- > git-stash.sh | 6 +- > git.c | 2 +- > notes-merge.c | 6 +- > path.c | 234 +-- > refs.c | 86 +++- > refs.h | 2 +- > run-command.c | 4 +- > run-command.h | 2 +- > setup.c| 124 + > sha1_file.c| 2 +- > submodule.c| 9 +- > t/t0060-path-util
Re: [PATCH v3 2/2] alloc.c: remove the redundant commit_count variable
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
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
Signed-off-by: Jeff King
---
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
---
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
---
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 @@ sta
[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
---
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 = lookup_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
---
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
---
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 --- 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
On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano wrote: > Junio C Hamano 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-09 19:40 GMT+09:00 Peter Krefting :
> 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
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
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 5:10 GMT+09:00 Jeff King :
> 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
From: 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
---
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(hea
pitfall with empty commits during git rebase
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
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
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
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
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
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
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
> ---
> 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 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 writes: > On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano 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 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
---
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 2>actual &&
+ 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 2>actual &&
+ 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
2>actual &&
+ 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
---
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 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
> Signed-off-by: Jacob Keller
> ---
> - 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..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=". 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=" 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=::
... (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 t
Re: [PATCH 3/4 v6] cache-tree: subdirectory tests
Eric Sunshine writes:
> On Thu, Jul 10, 2014 at 8:31 PM, David Turner
> wrote:
>> Add tests to confirm that invalidation of subdirectories neither over-
>> nor under-invalidates.
>>
>> Signed-off-by: David Turner
>> ---
>> 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 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: [PATCH 4/4 v6] cache-tree: Write updated cache-tree after commit
David Turner 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?
> -test_shallow_cache_tree () {
> - printf "SHA (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >expect
> &&
> +generate_expected_cache_tree_rec () {
> + dir="$1${1:+/}" &&
> + parent="$2" &&
> + # ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
> + # We want to count only foo because it's the only direct child
> + subtrees=$(git ls-files|grep /|cut -d / -f 1|uniq) &&
> + subtree_count=$(echo "$subtrees"|awk '$1 {++c} END {print c}') &&
> + entries=$(git ls-files|wc -l) &&
> + printf "SHA $dir (%d entries, %d subtrees)\n" "$entries"
> "$subtree_count" &&
> + for subtree in $subtrees
> + do
> + cd "$subtree"
> + generate_expected_cache_tree_rec "$dir$subtree" "$dir" || return 1
> + cd ..
> + done &&
> + dir=$parent
> +}
> +
> +generate_expected_cache_tree () {
> +cwd=$(pwd)
> +generate_expected_cache_tree_rec
> +ret="$?"
> +cd "$cwd"
> +return $ret
> +}
As we always have had trouble between $PWD and $(pwd) on other
platforms, I'd prefer something simpler, like:
expected_cache_tree () {
(
# subshell to let it wander around freely
generate_expected_cache_tree_rec
)
}
Other than these two, the new tests were good from a cursory look.
The change to builtin/commit.c looked good, too.
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: [Bug] data loss with cyclic alternates
Ephrim Khong 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 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
> > Signed-off-by: Jacob Keller
> > ---
> > - 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..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=". 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=" 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=::
> ... (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;
> > +
> >
Re: [PATCH v6 02/10] replace: add --graft option
On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano wrote: > Christian Couder writes: > >> On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano 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" 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 v8 1/2] add `config_set` API for caching config-like files
On 7/11/2014 7:51 PM, Matthieu Moy wrote:
> 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)
>
I had seen that there were checks for Syntax error or Non-existant files in
t1300-repo-config, for example,
# malformed configuration files
test_expect_success 'barf on syntax error' '
cat >.git/config <<-\EOF &&
# broken section line
[section]
key garbage
EOF
test_must_fail git config --get section.key >actual 2>error &&
grep " line 3 " error
'
test_expect_success 'barf on incomplete section header' '
cat >.git/config <<-\EOF &&
# broken section line
[section
key = value
EOF
test_must_fail git config --get section.key >actual 2>error &&
grep " line 2 " error
'
test_expect_success 'barf on incomplete string' '
cat >.git/config <<-\EOF &&
# broken section line
[section]
key = "value string
EOF
test_must_fail git config --get section.key >actual 2>error &&
grep " line 3 " error
'
test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' '
test_must_fail git config --file non-existing-config -l
'
They cover the same parsing code which I used for constructing the cache.
Still, more tests will not harm anyone, I will add more testcases accordingly
for the corner cases you raised. :)
> Tanay Abhra 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).
>
I am not sure of this myself, I will have to look into it.
>> +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;
>
Re: [PATCH] http: Add Accept-Language header if possible
2014-07-12 1:24 GMT+09:00 Eric Sunshine : > On Fri, Jul 11, 2014 at 5:22 AM, Yi, EungJun wrote: >> 2014-07-09 6:52 GMT+09:00 Eric Sunshine : + 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 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
> > Signed-off-by: Jacob Keller
> > ---
> > - 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..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=". 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=" 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=::
> ... (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);
> >
[PATCH v3] http: Add Accept-Language header if possible
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
---
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);
+
+
Re: [PATCH 4/7] add object_as_type helper for casting objects
On Fri, Jul 11, 2014 at 11:45:58AM +0100, Ramsay Jones wrote:
> > @@ -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.
Right, as I mentioned in the commit message, the type-check part of
object_type is a noop here. In that sense you could write this as just:
object_as_type(o, type. 1);
and ignore the return value. However, I'd rather err on the side of
checking for and reporting the error, even if we expect it to do nothing
right now. That decouples the two functions, and if object_as_type ever
learns to report other errors, then we automatically do the right
thing here.
-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 1/2] add `config_set` API for caching config-like files
Tanay Abhra writes: > I had seen that there were checks for Syntax error or Non-existant files in > t1300-repo-config, for example, The code raising the syntax error is there, and tested. But the way the error code (eg. return -1 from git_config) is handled by your code is not. -- 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 2/2 v4] tag: support configuring --sort via .gitconfig
On Fri, 2014-07-11 at 08:04 -0700, Junio C Hamano wrote:
> Jacob Keller writes:
>
> > +
> > + 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.
Based on a cursory glance at how the config is parsed, we actually
die_on_error for most options. Does it makes sense to warn or not? I am
not really sure here.. At any rate I updated this to be a bit cleaner
and use error( instead of die, so that it will work within how config is
supposed to.. Plus, this also makes it so that --sort shows the usage if
it's invalid.
Please comment more on the new set I am sending so we can really
determine what the correct course of action is here.
Regards,
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 --- 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 >expectactual && - cat >expect actual && - cat >expect actual && - cat >expect
[PATCH 3/3 v5] 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
Signed-off-by: Jacob Keller
---
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.
Documentation/config.txt | 5 +
Documentation/git-tag.txt | 5 -
builtin/tag.c | 56 +--
t/t7004-tag.sh| 25 +
4 files changed, 74 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..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=" 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[=]::
--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..f14aa346e0d4 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,41 @@ 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);
+ return parse_sort_string(value, &tag_sort);
+ }
+
+ status = git_gpg_config(var, value, cb);
if (status)
return status;
if (starts_with(var, "column."))
@@ -522,18 +556,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 +570,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 msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
@@ -572
[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
Signed-off-by: Jacob Keller
---
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 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 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,
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" 2>stderr &&
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" 2>stderr &&
Re: git p4 diff-tree ambiguous argument error
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 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 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 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 > > 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 > > 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 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 2>&1 &&
> + 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_
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 writes: > On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano wrote: >> Christian Couder writes: >> >>> On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano 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 writes: > > > On Fri, Jul 11, 2014 at 4:22 PM, Junio C Hamano wrote: > >> Christian Couder writes: > >> > >>> On Thu, Jul 10, 2014 at 7:36 PM, Junio C Hamano 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" 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 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 > > 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 > > 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 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 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 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 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" 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 --- 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 >expectactual && - cat >expect actual && - cat >expect actual && - cat >expect
[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
Signed-off-by: Jacob Keller
---
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..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=" 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[=]::
--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 = 0
[PATCH 1/3] tag: use skip_prefix instead of magic numbers
From: Jeff King
Make the parsing of the --sort parameter more readable by having
skip_prefix keep our pointer up to date.
Signed-off-by: Jeff King
Signed-off-by: Jacob Keller
---
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
> Signed-off-by: Jacob Keller
> ---
> 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..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=" 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[=]::
> --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 |= fl
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 --- 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 >expectactual && - cat >expect actual && - cat >expect actual && - cat >expect
[PATCH 1/3] tag: use skip_prefix instead of magic numbers
From: Jeff King
Make the parsing of the --sort parameter more readable by having
skip_prefix keep our pointer up to date.
Signed-off-by: Jeff King
Signed-off-by: Jacob Keller
---
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
Signed-off-by: Jacob Keller
---
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..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=" 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[=]::
--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 msg_arg m
Re: [PATCH 3/3 v5] tag: support configuring --sort via .gitconfig
Jeff King 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?
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 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 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 writes:
> Am 07.07.2014 20:30, schrieb Junio C Hamano:
>> Karsten Blees 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 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
On Fri, Jul 11, 2014 at 1:35 PM, Jeff King 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 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 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 writes:
> From: Jeff King
>
> Make the parsing of the --sort parameter more readable by having
> skip_prefix keep our pointer up to date.
>
> Signed-off-by: Jeff King
> Signed-off-by: Jacob Keller
> ---
> 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 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 writes:
>
> > On Thu, Jul 10, 2014 at 8:31 PM, David Turner
> > wrote:
> >> Add tests to confirm that invalidation of subdirectories neither over-
> >> nor under-invalidates.
> >>
> >> Signed-off-by: David Turner
> >> ---
> >> 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 writes:
>
> > From: Jeff King
> >
> > Make the parsing of the --sort parameter more readable by having
> > skip_prefix keep our pointer up to date.
> >
> > Signed-off-by: Jeff King
> > Signed-off-by: Jacob Keller
> > ---
> > 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 --- 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 >expectactual && - cat >expect actual && - cat >expect actual && - cat >expect
[PATCH 1/3] tag: use skip_prefix instead of magic numbers
From: Jeff King
Make the parsing of the --sort parameter more readable by having
skip_prefix keep our pointer up to date.
Signed-off-by: Jeff King
Signed-off-by: Jacob Keller
---
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] fixup! symlinks: remove PATH_MAX limitation
Rename cache_def_free to cache_def_clear as it doesn't free the struct
cache_def, but just clears its content.
Signed-off-by: Karsten Blees
---
cache.h | 2 +-
preload-index.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index 44aa439..378ee7f 100644
--- a/cache.h
+++ b/cache.h
@@ -1096,7 +1096,7 @@ struct cache_def {
int prefix_len_stat_func;
};
#define CACHE_DEF_INIT { STRBUF_INIT, 0, 0, 0 }
-static inline void cache_def_free(struct cache_def *cache)
+static inline void cache_def_clear(struct cache_def *cache)
{
strbuf_release(&cache->path);
}
diff --git a/preload-index.c b/preload-index.c
index 79ce8a9..c1fe3a3 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -63,7 +63,7 @@ static void *preload_thread(void *_data)
continue;
ce_mark_uptodate(ce);
} while (--nr > 0);
- cache_def_free(&cache);
+ cache_def_clear(&cache);
return NULL;
}
--
2.0.1.563.g66f467c.dirty
--
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
Signed-off-by: Jacob Keller
---
Made parse_sort_string take a "var" parameter, and if given will only warn
about invalid parameter, instead of error.
Documentation/config.txt | 5
Documentation/git-tag.txt | 5 +++-
builtin/tag.c | 66 ++-
t/t7004-tag.sh| 36 ++
4 files changed, 93 insertions(+), 19 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..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=" 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[=]::
--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 9d7643f127e7..97c5317c28e5 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,49 @@ 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 *var, const char *value, int *sort)
+{
+ int type = 0, flags = 0;
+
+ if (skip_prefix(value, "-", &value))
+ flags |= REVERSE_SORT;
+
+ if (skip_prefix(value, "version:", &value) || skip_prefix(value, "v:",
&value))
+ type = VERCMP_SORT;
+ else
+ type = STRCMP_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;
+}
+
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);
+ parse_sort_string(var, value, &tag_sort);
+ return 0;
+ }
+
+ status = git_gpg_config(var, value, cb);
if (status)
return status;
if (starts_with(var, "column."))
@@ -522,20 +564,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;
- else
- *sort = STRCMP_SORT;
-
- if (strcmp(arg, "refname"))
- die(_("unsupported sort specification %s"), arg);
- *sort |= flags;
- return 0;
+ return parse_sort_string(NULL, arg, sort);
}
int cmd_tag(int argc, const char **argv, const char *prefix)
@@ -548,7 +578,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct create_tag_options opt;
char *cleanup_arg = NULL;
int a
[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
---
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 4/4] cache-tree: Write updated cache-tree after commit
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
---
builtin/commit.c | 9 +++-
t/t0090-cache-tree.sh | 117 +++---
2 files changed, 110 insertions(+), 16 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..99c9054 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -342,6 +342,8 @@ static char *prepare_index(int argc, const char **argv,
const char *prefix,
discard_cache();
read_cache_from(index_lock.filename);
+ if (update_main_cache_tree(WRITE_TREE_SILENT) >= 0)
+ write_cache(fd, active_cache, active_nr);
commit_style = COMMIT_NORMAL;
return index_lock.filename;
@@ -383,8 +385,12 @@ static char *prepare_index(int argc, const char **argv,
const char *prefix,
if (!only && !pathspec.nr) {
fd = hold_locked_index(&index_lock, 1);
refresh_cache_or_die(refresh_flags);
- if (active_cache_changed) {
+ if (active_cache_changed
+ || !cache_tree_fully_valid(active_cache_tree)) {
update_main_cache_tree(WRITE_TREE_SILENT);
+ active_cache_changed = 1;
+ }
+ if (active_cache_changed) {
if (write_cache(fd, active_cache, active_nr) ||
commit_locked_index(&index_lock))
die(_("unable to write new_index file"));
@@ -435,6 +441,7 @@ static char *prepare_index(int argc, const char **argv,
const char *prefix,
fd = hold_locked_index(&index_lock, 1);
add_remove_files(&partial);
refresh_cache(REFRESH_QUIET);
+ update_main_cache_tree(WRITE_TREE_SILENT);
if (write_cache(fd, active_cache, active_nr) ||
close_lock_file(&index_lock))
die(_("unable to write new_index file"));
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 3a3342e..57f263f 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -8,7 +8,7 @@ cache-tree extension.
. ./test-lib.sh
cmp_cache_tree () {
- test-dump-cache-tree >actual &&
+ test-dump-cache-tree | sed -e '/#(ref)/d' >actual &&
sed "s/$_x40/SHA/" filtered &&
test_cmp "$1" filtered
}
@@ -16,14 +16,38 @@ cmp_cache_tree () {
# We don't bother with actually checking the SHA1:
# test-dump-cache-tree already verifies that all existing data is
# correct.
-test_shallow_cache_tree () {
- printf "SHA (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >expect
&&
+generate_expected_cache_tree_rec () {
+ dir="$1${1:+/}" &&
+ parent="$2" &&
+ # ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
+ # We want to count only foo because it's the only direct child
+ subtrees=$(git ls-files|grep /|cut -d / -f 1|uniq) &&
+ subtree_count=$(echo "$subtrees"|awk '$1 {++c} END {print c}') &&
+ entries=$(git ls-files|wc -l) &&
+ printf "SHA $dir (%d entries, %d subtrees)\n" "$entries"
"$subtree_count" &&
+ for subtree in $subtrees
+ do
+ cd "$subtree"
+ generate_expected_cache_tree_rec "$dir$subtree" "$dir" ||
return 1
+ cd ..
+ done &&
+ dir=$parent
+}
+
+generate_expected_cache_tree () {
+ (
+ generate_expected_cache_tree_rec
+ )
+}
+
+test_cache_tree () {
+ generate_expected_cache_tree >expect &&
cmp_cache_tree expect
}
test_invalid_cache_tree () {
printf "invalid %s ()\n" "" "$@"
>expect &&
- test-dump-cache-tree | \
+ test-dump-cache-tree |
sed -n -e "s/[0-9]* subtrees//" -e '/#(ref)/d' -e '/^invalid /p'
>actual &&
test_cmp expect actual
}
@@ -33,14 +57,14 @@ test_no_cache_tree () {
cmp_cache_tree expect
}
-test_expect_failure 'initial commit has cache-tree' '
+test_expect_success 'initial commit has cache-tree' '
test_commit foo &&
- test_shallow_cache_tree
+ test_cache_tree
'
test_expect_success 'read-tree HEAD establishes cache-tree' '
git read-tree HEAD &&
- test_shallow_cache_tree
+ test_cache_tree
'
test_expect_success 'git-add invalidates cache-tree' '
@@ -58,6 +82,18 @@ test_expect_success 'git-add in subdir invalidates
cache-tree' '
test_invalid_cache_tree
'
+cat >before <<\EOF
+SHA (3 entries, 2 subtrees)
+SHA dir1/ (1 entries, 0 subtrees)
+SHA dir2/ (1 entries, 0 subtrees)
+EOF
+
+cat >expect <<\EOF
+invalid (2 subtrees)
+invalid dir1/ (0 subtrees
[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
---
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
---
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 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
