* Re: [PATCH] rebase --preserve-merges keeps empty merge commits
From: Matthieu Moy @ 2013-01-14 14:12 UTC (permalink / raw)
To: Phil Hord
Cc: git, phil.hord, Neil Horman, Martin von Zweigbergk,
Junio C Hamano
In-Reply-To: <1358023561-26773-1-git-send-email-hordp@cisco.com>
Phil Hord <hordp@cisco.com> writes:
> Subject: [PATCH] rebase --preserve-merges keeps empty merge commits
I would rephrase it as
rebase --preserve-merges: keep empty merge commits
we usually give orders in commit messages, not state facts (it's not
clear from the existing subject line whether keeping merge commit is the
new behavior or a bug that the commit tries to fix).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH] archive-tar: fix sanity check in config parsing
From: Jeff King @ 2013-01-14 14:58 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114124424.GA14129@sigill.intra.peff.net>
On Mon, Jan 14, 2013 at 04:44:24AM -0800, Jeff King wrote:
> > Wouldn't it then be better ti use strlen("tar") rather than a 3? Or
> > at least a comment?
> [...]
> We could also potentially encapsulate it in a function. I think the diff
> code has a very similar block.
Here's a series that does that, with a few other cleanups on top. The
diffstat actually ends up a few lines longer, but that is mostly because
of comments and function declarations. More importantly, though, the
call-sites are much easier to read.
Having written this series, though, I can't help but wonder if the world
would be a better place if config_fn_t looked more like:
typedef int (*config_fn_t)(const char *full_var,
const char *section,
const char *subsection,
const char *key,
const char *value,
void *data);
It's just as easy for the config parser to do this ahead of time, and by
handing off real C-strings (instead of ending up with a ptr/len pair for
the subsection), it makes the lives of the callbacks much easier (e.g.,
the final patch below contorts a bit to use string_list with the
subsection).
I can look into that, but here is the less invasive cleanup:
[1/6]: config: add helper function for parsing key names
[2/6]: archive-tar: use match_config_key when parsing config
[3/6]: convert some config callbacks to match_config_key
[4/6]: userdiff: drop parse_driver function
[5/6]: submodule: use match_config_key when parsing config
[6/6]: submodule: simplify memory handling in config parsing
-Peff
^ permalink raw reply
* [PATCH 1/6] config: add helper function for parsing key names
From: Jeff King @ 2013-01-14 15:00 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114145845.GA16497@sigill.intra.peff.net>
The config callback functions get keys of the general form:
section.subsection.key
(where the subsection may be contain arbitrary data, or may
be missing). For matching keys without subsections, it is
simple enough to call "strcmp". Matching keys with
subsections is a little more complicated, and each callback
does it in an ad-hoc way, usually involving error-prone
pointer arithmetic.
Let's provide a helper that keeps the pointer arithmetic all
in one place.
Signed-off-by: Jeff King <peff@peff.net>
---
No users yet; they come in future patches.
cache.h | 15 +++++++++++++++
config.c | 33 +++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/cache.h b/cache.h
index c257953..14003b8 100644
--- a/cache.h
+++ b/cache.h
@@ -1164,6 +1164,21 @@ extern int git_config_include(const char *name, const char *value, void *data);
#define CONFIG_INCLUDE_INIT { 0 }
extern int git_config_include(const char *name, const char *value, void *data);
+/*
+ * Match and parse a config key of the form:
+ *
+ * section.(subsection.)?key
+ *
+ * (i.e., what gets handed to a config_fn_t). The caller provides the section;
+ * we return -1 if it does not match, 0 otherwise. The subsection and key
+ * out-parameters are filled by the function (and subsection is NULL if it is
+ * missing).
+ */
+extern int match_config_key(const char *var,
+ const char *section,
+ const char **subsection, int *subsection_len,
+ const char **key);
+
extern int committer_ident_sufficiently_given(void);
extern int author_ident_sufficiently_given(void);
diff --git a/config.c b/config.c
index 7b444b6..18d9c0e 100644
--- a/config.c
+++ b/config.c
@@ -1667,3 +1667,36 @@ int config_error_nonbool(const char *var)
{
return error("Missing value for '%s'", var);
}
+
+int match_config_key(const char *var,
+ const char *section,
+ const char **subsection, int *subsection_len,
+ const char **key)
+{
+ int section_len = strlen(section);
+ const char *dot;
+
+ /* Does it start with "section." ? */
+ if (prefixcmp(var, section) || var[section_len] != '.')
+ return -1;
+
+ /*
+ * Find the key; we don't know yet if we have a subsection, but we must
+ * parse backwards from the end, since the subsection may have dots in
+ * it, too.
+ */
+ dot = strrchr(var, '.');
+ *key = dot + 1;
+
+ /* Did we have a subsection at all? */
+ if (dot == var + section_len) {
+ *subsection = NULL;
+ *subsection_len = 0;
+ }
+ else {
+ *subsection = var + section_len + 1;
+ *subsection_len = dot - *subsection;
+ }
+
+ return 0;
+}
--
1.8.1.rc1.10.g7d71f7b
^ permalink raw reply related
* [PATCH 2/6] archive-tar: use match_config_key when parsing config
From: Jeff King @ 2013-01-14 15:02 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114145845.GA16497@sigill.intra.peff.net>
This is fewer lines of code, but more importantly, fixes a
bogus pointer offset. We are looking for "tar." in the
section, but later assume that the dot we found is at offset
9, not 3. This is a holdover from an earlier iteration of
767cf45 which called the section "tarfilter".
As a result, we could erroneously reject some filters with
dots in their name, as well as read uninitialized memory.
Reported by (and test by) René Scharfe.
Signed-off-by: Jeff King <peff@peff.net>
---
This obviously replaces René's patch. It might make more sense to just
put his patch onto maint, and build this on top; then this patch can get
squashed into the next one (which just updates the uninteresting
callsites).
archive-tar.c | 10 +---------
t/t5000-tar-tree.sh | 3 ++-
2 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/archive-tar.c b/archive-tar.c
index 0ba3f25..68dbe59 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -325,20 +325,12 @@ static int tar_filter_config(const char *var, const char *value, void *data)
static int tar_filter_config(const char *var, const char *value, void *data)
{
struct archiver *ar;
- const char *dot;
const char *name;
const char *type;
int namelen;
- if (prefixcmp(var, "tar."))
+ if (match_config_key(var, "tar", &name, &namelen, &type) < 0 || !name)
return 0;
- dot = strrchr(var, '.');
- if (dot == var + 9)
- return 0;
-
- name = var + 4;
- namelen = dot - name;
- type = dot + 1;
ar = find_tar_filter(name, namelen);
if (!ar) {
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index ecf00ed..517ae04 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -283,7 +283,8 @@ test_expect_success 'setup tar filters' '
test_expect_success 'setup tar filters' '
git config tar.tar.foo.command "tr ab ba" &&
git config tar.bar.command "tr ab ba" &&
- git config tar.bar.remote true
+ git config tar.bar.remote true &&
+ git config tar.invalid baz
'
test_expect_success 'archive --list mentions user filter' '
--
1.8.1.rc1.10.g7d71f7b
^ permalink raw reply related
* [PATCH 3/6] convert some config callbacks to match_config_key
From: Jeff King @ 2013-01-14 15:03 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114145845.GA16497@sigill.intra.peff.net>
This is easier to read and avoids magic offset constants
which need to be in sync with the section-name we provide.
Signed-off-by: Jeff King <peff@peff.net>
---
convert.c | 6 +-----
ll-merge.c | 6 +-----
userdiff.c | 13 +++----------
3 files changed, 5 insertions(+), 20 deletions(-)
diff --git a/convert.c b/convert.c
index 6602155..e3ecb30 100644
--- a/convert.c
+++ b/convert.c
@@ -465,10 +465,8 @@ static int read_convert_config(const char *var, const char *value, void *cb)
* External conversion drivers are configured using
* "filter.<name>.variable".
*/
- if (prefixcmp(var, "filter.") || (ep = strrchr(var, '.')) == var + 6)
+ if (match_config_key(var, "filter", &name, &namelen, &ep) < 0 || !name)
return 0;
- name = var + 7;
- namelen = ep - name;
for (drv = user_convert; drv; drv = drv->next)
if (!strncmp(drv->name, name, namelen) && !drv->name[namelen])
break;
@@ -479,8 +477,6 @@ static int read_convert_config(const char *var, const char *value, void *cb)
user_convert_tail = &(drv->next);
}
- ep++;
-
/*
* filter.<name>.smudge and filter.<name>.clean specifies
* the command line:
diff --git a/ll-merge.c b/ll-merge.c
index acea33b..d4c4ff6 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -236,15 +236,13 @@ static int read_merge_config(const char *var, const char *value, void *cb)
* especially, we do not want to look at variables such as
* "merge.summary", "merge.tool", and "merge.verbosity".
*/
- if (prefixcmp(var, "merge.") || (ep = strrchr(var, '.')) == var + 5)
+ if (match_config_key(var, "merge", &name, &namelen, &ep) < 0 || !name)
return 0;
/*
* Find existing one as we might be processing merge.<name>.var2
* after seeing merge.<name>.var1.
*/
- name = var + 6;
- namelen = ep - name;
for (fn = ll_user_merge; fn; fn = fn->next)
if (!strncmp(fn->name, name, namelen) && !fn->name[namelen])
break;
@@ -256,8 +254,6 @@ static int read_merge_config(const char *var, const char *value, void *cb)
ll_user_merge_tail = &(fn->next);
}
- ep++;
-
if (!strcmp("name", ep)) {
if (!value)
return error("%s: lacks value", var);
diff --git a/userdiff.c b/userdiff.c
index ed958ef..1a6a0fa 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -188,20 +188,13 @@ static struct userdiff_driver *parse_driver(const char *var,
const char *value, const char *type)
{
struct userdiff_driver *drv;
- const char *dot;
- const char *name;
+ const char *name, *key;
int namelen;
- if (prefixcmp(var, "diff."))
- return NULL;
- dot = strrchr(var, '.');
- if (dot == var + 4)
- return NULL;
- if (strcmp(type, dot+1))
+ if (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
+ strcmp(type, key))
return NULL;
- name = var + 5;
- namelen = dot - name;
drv = userdiff_find_by_namelen(name, namelen);
if (!drv) {
ALLOC_GROW(drivers, ndrivers+1, drivers_alloc);
--
1.8.1.rc1.10.g7d71f7b
^ permalink raw reply related
* [PATCH 4/6] userdiff: drop parse_driver function
From: Jeff King @ 2013-01-14 15:04 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114145845.GA16497@sigill.intra.peff.net>
When we parse userdiff config, we generally assume that
diff.name.key
will affect the "key" value of the "name" driver. However,
without checking the key, we conflict with the ancient
"diff.color.*" namespace. The current code is careful not to
even create a driver struct if we do not see a key that is
known by the diff-driver code.
However, this carefulness is unnecessary; the default driver
with no keys set behaves exactly the same as having no
driver at all. We can simply set up the driver struct as
soon as we see we have a config key that looks like a
driver. This makes the code a bit more readable.
Signed-off-by: Jeff King <peff@peff.net>
---
This is not strictly related to the series, but I noticed it as a
cleanup while doing the previous patch.
userdiff.c | 50 +++++++++++++++++++++-----------------------------
1 file changed, 21 insertions(+), 29 deletions(-)
diff --git a/userdiff.c b/userdiff.c
index 1a6a0fa..c6cdec4 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -184,28 +184,6 @@ static struct userdiff_driver *userdiff_find_by_namelen(const char *k, int len)
return NULL;
}
-static struct userdiff_driver *parse_driver(const char *var,
- const char *value, const char *type)
-{
- struct userdiff_driver *drv;
- const char *name, *key;
- int namelen;
-
- if (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
- strcmp(type, key))
- return NULL;
-
- drv = userdiff_find_by_namelen(name, namelen);
- if (!drv) {
- ALLOC_GROW(drivers, ndrivers+1, drivers_alloc);
- drv = &drivers[ndrivers++];
- memset(drv, 0, sizeof(*drv));
- drv->name = xmemdupz(name, namelen);
- drv->binary = -1;
- }
- return drv;
-}
-
static int parse_funcname(struct userdiff_funcname *f, const char *k,
const char *v, int cflags)
{
@@ -233,20 +211,34 @@ int userdiff_config(const char *k, const char *v)
int userdiff_config(const char *k, const char *v)
{
struct userdiff_driver *drv;
+ const char *name, *type;
+ int namelen;
+
+ if (match_config_key(k, "diff", &name, &namelen, &type) || !name)
+ return 0;
+
+ drv = userdiff_find_by_namelen(name, namelen);
+ if (!drv) {
+ ALLOC_GROW(drivers, ndrivers+1, drivers_alloc);
+ drv = &drivers[ndrivers++];
+ memset(drv, 0, sizeof(*drv));
+ drv->name = xmemdupz(name, namelen);
+ drv->binary = -1;
+ }
- if ((drv = parse_driver(k, v, "funcname")))
+ if (!strcmp(type, "funcname"))
return parse_funcname(&drv->funcname, k, v, 0);
- if ((drv = parse_driver(k, v, "xfuncname")))
+ if (!strcmp(type, "xfuncname"))
return parse_funcname(&drv->funcname, k, v, REG_EXTENDED);
- if ((drv = parse_driver(k, v, "binary")))
+ if (!strcmp(type, "binary"))
return parse_tristate(&drv->binary, k, v);
- if ((drv = parse_driver(k, v, "command")))
+ if (!strcmp(type, "command"))
return git_config_string(&drv->external, k, v);
- if ((drv = parse_driver(k, v, "textconv")))
+ if (!strcmp(type, "textconv"))
return git_config_string(&drv->textconv, k, v);
- if ((drv = parse_driver(k, v, "cachetextconv")))
+ if (!strcmp(type, "cachetextconv"))
return parse_bool(&drv->textconv_want_cache, k, v);
- if ((drv = parse_driver(k, v, "wordregex")))
+ if (!strcmp(type, "wordregex"))
return git_config_string(&drv->word_regex, k, v);
return 0;
--
1.8.1.rc1.10.g7d71f7b
^ permalink raw reply related
* [PATCH 5/6] submodule: use match_config_key when parsing config
From: Jeff King @ 2013-01-14 15:04 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114145845.GA16497@sigill.intra.peff.net>
This makes the code a lot simpler to read by dropping a
whole bunch of constant offsets.
As a bonus, it means we also feed the whole config variable
name to our error functions:
[before]
$ git -c submodule.foo.fetchrecursesubmodules=bogus checkout
fatal: bad foo.fetchrecursesubmodules argument: bogus
[after]
fatal: bad submodule.foo.fetchrecursesubmodules argument: bogus
Signed-off-by: Jeff King <peff@peff.net>
---
submodule.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/submodule.c b/submodule.c
index 2f55436..4361207 100644
--- a/submodule.c
+++ b/submodule.c
@@ -126,15 +126,17 @@ int parse_submodule_config_option(const char *var, const char *value)
int parse_submodule_config_option(const char *var, const char *value)
{
- int len;
struct string_list_item *config;
struct strbuf submodname = STRBUF_INIT;
+ const char *name, *key;
+ int namelen;
- var += 10; /* Skip "submodule." */
+ if (match_config_key(var, "submodule", &name, &namelen, &key) < 0 ||
+ !name)
+ return 0;
- len = strlen(var);
- if ((len > 5) && !strcmp(var + len - 5, ".path")) {
- strbuf_add(&submodname, var, len - 5);
+ if (!strcmp(key, "path")) {
+ strbuf_add(&submodname, name, namelen);
config = unsorted_string_list_lookup(&config_name_for_path, value);
if (config)
free(config->util);
@@ -142,22 +144,22 @@ int parse_submodule_config_option(const char *var, const char *value)
config = string_list_append(&config_name_for_path, xstrdup(value));
config->util = strbuf_detach(&submodname, NULL);
strbuf_release(&submodname);
- } else if ((len > 23) && !strcmp(var + len - 23, ".fetchrecursesubmodules")) {
- strbuf_add(&submodname, var, len - 23);
+ } else if (!strcmp(key, "fetchrecursesubmodules")) {
+ strbuf_add(&submodname, name, namelen);
config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf);
if (!config)
config = string_list_append(&config_fetch_recurse_submodules_for_name,
strbuf_detach(&submodname, NULL));
config->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value);
strbuf_release(&submodname);
- } else if ((len > 7) && !strcmp(var + len - 7, ".ignore")) {
+ } else if (!strcmp(key, "ignore")) {
if (strcmp(value, "untracked") && strcmp(value, "dirty") &&
strcmp(value, "all") && strcmp(value, "none")) {
warning("Invalid parameter \"%s\" for config option \"submodule.%s.ignore\"", value, var);
return 0;
}
- strbuf_add(&submodname, var, len - 7);
+ strbuf_add(&submodname, name, namelen);
config = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf);
if (config)
free(config->util);
--
1.8.1.rc1.10.g7d71f7b
^ permalink raw reply related
* [PATCH 6/6] submodule: simplify memory handling in config parsing
From: Jeff King @ 2013-01-14 15:07 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114145845.GA16497@sigill.intra.peff.net>
We keep a strbuf for the name of the submodule, even though
we only ever add one buffer (which we know the length of) to
it. Let's just use xmemdupz instead, which is slightly more
efficient and makes it easier to follow what is going on.
Unfortunately, we still end up having to deal with some
memory ownership issues in some code branches, as we have to
allocate the string in order to do a string list lookup, and
then only sometimes want to hand ownership of that string
over to the string_list. Still, making that explicit in the
code (as opposed to sometimes detaching the strbuf, and then
always releasing it) makes it a little more obvious what is
going on.
Signed-off-by: Jeff King <peff@peff.net>
---
I'm undecided on this one. I think the result is easier to follow, but
others might find the original easier. This would be a lot more
readable if the config parser gave us a broken-out representation with
real C strings. Then we could pass the subsection straight to the
string_list functions for lookup, and using the "dup" flag of string
list, avoid even having to deal with memory duplication at all.
submodule.c | 30 ++++++++++++++----------------
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/submodule.c b/submodule.c
index 4361207..23a8490 100644
--- a/submodule.c
+++ b/submodule.c
@@ -127,7 +127,6 @@ int parse_submodule_config_option(const char *var, const char *value)
int parse_submodule_config_option(const char *var, const char *value)
{
struct string_list_item *config;
- struct strbuf submodname = STRBUF_INIT;
const char *name, *key;
int namelen;
@@ -136,37 +135,36 @@ int parse_submodule_config_option(const char *var, const char *value)
return 0;
if (!strcmp(key, "path")) {
- strbuf_add(&submodname, name, namelen);
config = unsorted_string_list_lookup(&config_name_for_path, value);
if (config)
free(config->util);
else
config = string_list_append(&config_name_for_path, xstrdup(value));
- config->util = strbuf_detach(&submodname, NULL);
- strbuf_release(&submodname);
+ config->util = xmemdupz(name, namelen);
} else if (!strcmp(key, "fetchrecursesubmodules")) {
- strbuf_add(&submodname, name, namelen);
- config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf);
+ char *name_cstr = xmemdupz(name, namelen);
+ config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, name_cstr);
if (!config)
- config = string_list_append(&config_fetch_recurse_submodules_for_name,
- strbuf_detach(&submodname, NULL));
+ config = string_list_append(&config_fetch_recurse_submodules_for_name, name_cstr);
+ else
+ free(name_cstr);
config->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value);
- strbuf_release(&submodname);
} else if (!strcmp(key, "ignore")) {
+ char *name_cstr;
+
if (strcmp(value, "untracked") && strcmp(value, "dirty") &&
strcmp(value, "all") && strcmp(value, "none")) {
warning("Invalid parameter \"%s\" for config option \"submodule.%s.ignore\"", value, var);
return 0;
}
- strbuf_add(&submodname, name, namelen);
- config = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf);
- if (config)
+ name_cstr = xmemdupz(name, namelen);
+ config = unsorted_string_list_lookup(&config_ignore_for_name, name_cstr);
+ if (config) {
free(config->util);
- else
- config = string_list_append(&config_ignore_for_name,
- strbuf_detach(&submodname, NULL));
- strbuf_release(&submodname);
+ free(name_cstr);
+ } else
+ config = string_list_append(&config_ignore_for_name, name_cstr);
config->util = xstrdup(value);
return 0;
}
--
1.8.1.rc1.10.g7d71f7b
^ permalink raw reply related
* Re: git list files
From: Jeff King @ 2013-01-14 15:28 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Стойчо Слепцов,
git, Jakub Narębski, Matthieu Moy
In-Reply-To: <20130114070832.GA4820@elie.Belkin>
On Sun, Jan 13, 2013 at 11:08:32PM -0800, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > As far as I recall, that script works. However, I have a pure-C
> > blame-tree implementation that is much faster, which may also be of
> > interest. I need to clean up and put a few finishing touches on it to
> > send it to the list, but it has been in production at GitHub for a few
> > months. You can find it here:
> >
> > git://github.com/peff/git jk/blame-tree
>
> Oh, neat. Would that make sense as an item in
> <https://git.wiki.kernel.org/index.php/Interfaces,_frontends,_and_tools>?
I'd rather finish cleaning it up and actually get it merged. It's on my
todo list.
-Peff
^ permalink raw reply
* [PATCH] pretty: use prefixcmp instead of memcmp on NUL-terminated strings
From: René Scharfe @ 2013-01-14 16:34 UTC (permalink / raw)
To: git discussion list; +Cc: Junio C Hamano
This conversion avoids the need for magic string length numbers in the
code. And unlike memcmp(), prefixcmp() is careful to not run over the
end of a string.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
pretty.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/pretty.c b/pretty.c
index 92c839f..01795de 100644
--- a/pretty.c
+++ b/pretty.c
@@ -966,7 +966,7 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
if (!end)
return 0;
- if (!memcmp(begin, "auto,", 5)) {
+ if (!prefixcmp(begin, "auto,")) {
if (!want_color(c->pretty_ctx->color))
return end - placeholder + 1;
begin += 5;
@@ -1301,7 +1301,7 @@ static void pp_header(const struct pretty_print_context *pp,
continue;
}
- if (!memcmp(line, "parent ", 7)) {
+ if (!prefixcmp(line, "parent ")) {
if (linelen != 48)
die("bad parent line in commit");
continue;
@@ -1325,11 +1325,11 @@ static void pp_header(const struct pretty_print_context *pp,
* FULL shows both authors but not dates.
* FULLER shows both authors and dates.
*/
- if (!memcmp(line, "author ", 7)) {
+ if (!prefixcmp(line, "author ")) {
strbuf_grow(sb, linelen + 80);
pp_user_info(pp, "Author", sb, line + 7, encoding);
}
- if (!memcmp(line, "committer ", 10) &&
+ if (!prefixcmp(line, "committer ") &&
(pp->fmt == CMIT_FMT_FULL || pp->fmt == CMIT_FMT_FULLER)) {
strbuf_grow(sb, linelen + 80);
pp_user_info(pp, "Commit", sb, line + 10, encoding);
--
1.8.0
^ permalink raw reply related
* Re: [BUG] Possible bug in `remote set-url --add --push`
From: Jonathan Nieder @ 2013-01-14 16:41 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Jardel Weyrich, Sascha Cunz, Junio C Hamano, git
In-Reply-To: <50F40316.7010308@drmicha.warpmail.net>
Michael J Gruber wrote:
> All that "set-url --push --add" does is adding a remote.foo.pushurl
> entry to the config. If there was none, there will be one after that.
>
> If there is no pushurl entry, "push" takes the url entry instead. This
> is the "default URL for push", but not a pushurl entry.
That is how it is implemented, but it is hard for me with a straight
face to say that is what most users expect.
Wouldn't the least confusing thing be to just error out for "set-url
--push --add" when there is no existing pushurl? That way, the
operator can use plain "set-url --push" to clarify whether whether he
meant to include the pull URLs in the new pushurl set.
My two cents,
Jonathan
^ permalink raw reply
* Re: [PATCH 3/6] convert some config callbacks to match_config_key
From: Jonathan Nieder @ 2013-01-14 16:55 UTC (permalink / raw)
To: Jeff King; +Cc: Joachim Schmitz, René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114150322.GC16828@sigill.intra.peff.net>
Jeff King wrote:
> --- a/convert.c
> +++ b/convert.c
> @@ -465,10 +465,8 @@ static int read_convert_config(const char *var, const char *value, void *cb)
> * External conversion drivers are configured using
> * "filter.<name>.variable".
> */
> - if (prefixcmp(var, "filter.") || (ep = strrchr(var, '.')) == var + 6)
> + if (match_config_key(var, "filter", &name, &namelen, &ep) < 0 || !name)
> return 0;
Hm, I actually find the preimage more readable here.
I like the idea of having a function to do this, though. Here are a
couple of ideas for making the meaning obvious again for people like
me:
Rename match_config_key() to something like parse_config_key()?
match_ makes it sound like its main purpose is to match it against a
pattern, but really it is more about decomposing into constituent
parts.
Rename ep to something like 'key' or 'filtertype'? Without the
explicit string processing, it is not obvious what ep is the end of.
[...]
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -188,20 +188,13 @@ static struct userdiff_driver *parse_driver(const char *var,
> const char *value, const char *type)
> {
> struct userdiff_driver *drv;
> - const char *dot;
> - const char *name;
> + const char *name, *key;
> int namelen;
>
> - if (prefixcmp(var, "diff."))
> - return NULL;
> - dot = strrchr(var, '.');
> - if (dot == var + 4)
> - return NULL;
> - if (strcmp(type, dot+1))
> + if (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
> + strcmp(type, key))
> return NULL;
>
> - name = var + 5;
> - namelen = dot - name;
> drv = userdiff_find_by_namelen(name, namelen);
What happens in the !name case? (Honest question --- I haven't checked.)
Generally I like the cleanup. Thanks for tasteful patch.
Jonathan
^ permalink raw reply
* Re: [PATCH 3/6] convert some config callbacks to match_config_key
From: Jeff King @ 2013-01-14 17:06 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Joachim Schmitz, René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114165527.GB3121@elie.Belkin>
On Mon, Jan 14, 2013 at 08:55:28AM -0800, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > --- a/convert.c
> > +++ b/convert.c
> > @@ -465,10 +465,8 @@ static int read_convert_config(const char *var, const char *value, void *cb)
> > * External conversion drivers are configured using
> > * "filter.<name>.variable".
> > */
> > - if (prefixcmp(var, "filter.") || (ep = strrchr(var, '.')) == var + 6)
> > + if (match_config_key(var, "filter", &name, &namelen, &ep) < 0 || !name)
> > return 0;
>
> Hm, I actually find the preimage more readable here.
The big thing to me is getting rid of the manual "6" there, which is bad
for maintainability (it must match the length of "filter", and there is
no compile-time verification).
> Rename match_config_key() to something like parse_config_key()?
> match_ makes it sound like its main purpose is to match it against a
> pattern, but really it is more about decomposing into constituent
> parts.
There is already a git_config_parse_key, but it does something else. I
wanted to avoid confusion there. And I was trying to indicate that this
was not just about parsing, but also about matching the section.
Basically I was trying to encapsulate the current technique and have as
little code change as possible. But maybe:
struct config_key k;
parse_config_key(&k, var);
if (strcmp(k.section, "filter") || k.subsection))
return 0;
would be a better start (or having git_config do the first two lines
itself before triggering the callback).
> Rename ep to something like 'key' or 'filtertype'? Without the
> explicit string processing, it is not obvious what ep is the end of.
Ah, so that is what "ep" stands for. I was thinking it is a terrible
variable name, but I was trying to keep the patch minimal.
> > - if (prefixcmp(var, "diff."))
> > - return NULL;
> > - dot = strrchr(var, '.');
> > - if (dot == var + 4)
> > - return NULL;
> > - if (strcmp(type, dot+1))
> > + if (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
> > + strcmp(type, key))
> > return NULL;
> >
> > - name = var + 5;
> > - namelen = dot - name;
> > drv = userdiff_find_by_namelen(name, namelen);
>
> What happens in the !name case? (Honest question --- I haven't checked.)
Segfault, I expect. Thanks for catching.
I actually wrote this correctly once, coupled with patch 4, but screwed
it up when teasing it apart into two patches. It should be:
if (match_config_key(var, "diff", &name, &namelen, &key) < 0 ||
!name ||
strcmp(type, key))
return NULL;
Patch 4 replaces this with correct code (as it moves it into
userdiff_config).
-Peff
^ permalink raw reply
* Re: [PATCH] rebase --preserve-merges keeps empty merge commits
From: Junio C Hamano @ 2013-01-14 17:15 UTC (permalink / raw)
To: Matthieu Moy
Cc: Phil Hord, git, phil.hord, Neil Horman, Martin von Zweigbergk
In-Reply-To: <vpqpq17zwdl.fsf@grenoble-inp.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Phil Hord <hordp@cisco.com> writes:
>
>> Subject: [PATCH] rebase --preserve-merges keeps empty merge commits
>
> I would rephrase it as
>
> rebase --preserve-merges: keep empty merge commits
>
> we usually give orders in commit messages, not state facts (it's not
> clear from the existing subject line whether keeping merge commit is the
> new behavior or a bug that the commit tries to fix).
Thanks for giving a concise rationale on our use of imperative mood.
Phil, I think you meant to and forgot to sign-off; here is what I'll
queue.
Thanks.
-- >8 --
From: Phil Hord <hordp@cisco.com>
Date: Sat, 12 Jan 2013 15:46:01 -0500
Subject: [PATCH] rebase --preserve-merges: keep all merge commits including empty ones
Since 90e1818f9a (git-rebase: add keep_empty flag, 2012-04-20)
'git rebase --preserve-merges' fails to preserve empty merge commits
unless --keep-empty is also specified. Merge commits should be
preserved in order to preserve the structure of the rebased graph,
even if the merge commit does not introduce changes to the parent.
Teach rebase not to drop merge commits only because they are empty.
A special case which is not handled by this change is for a merge commit
whose parents are now the same commit because all the previous different
parents have been dropped as a result of this rebase or some previous
operation.
Signed-off-by: Phil Hord <hordp@cisco.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
git-rebase--interactive.sh | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0c19b7c..2fed92f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -175,6 +175,11 @@ is_empty_commit() {
test "$tree" = "$ptree"
}
+is_merge_commit()
+{
+ git rev-parse --verify --quiet "$1"^2 >/dev/null 2>&1
+}
+
# Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
# GIT_AUTHOR_DATE exported from the current environment.
do_with_author () {
@@ -796,7 +801,7 @@ git rev-list $merges_option --pretty=oneline --abbrev-commit \
while read -r shortsha1 rest
do
- if test -z "$keep_empty" && is_empty_commit $shortsha1
+ if test -z "$keep_empty" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1
then
comment_out="# "
else
--
1.8.1.1.338.g126d652
^ permalink raw reply related
* Re: git list files
From: Стойчо Слепцов @ 2013-01-14 17:20 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git
In-Reply-To: <20130114152836.GA18370@sigill.intra.peff.net>
I went through your initial thread about blame-tree,
and it is really very very (+very) close to answer my question.
Thanks for writing it,
if it comes one day to git, I will use it.
As for:
'I guess people's eyes and brains are trained by the old school "file
boundaries matter" way of thinking'
-- Junio C Hamano
at http://thread.gmane.org/gmane.comp.version-control.git/168323;
http://article.gmane.org/gmane.comp.version-control.git/168333
I think it's not the case, Mr. Hamano.
>From my point of view, it is just to have a quick picture of "what
came from where in this current directory",
which is a normal reaction of human beings, I think.
Speaking of which I can't help thinking that this feature could be
provided by $git rev-list (HEAD) --no-walk -- <paths>, just don't stop
at first commit,
but at first commit for each of the paths.
Or maybe diff could have an option to not compare against a specific point,
but actually do his job and go downstears and find where the
_diff_erence for _each_ path happened finally.
(... applicable for $git status -l (--list) --porcelain ... but thats
a whim, sorry.)
Anyway,
thank you all for your time, it was a real pleasure for me,
Blind.
2013/1/14 Jeff King <peff@peff.net>:
> On Sun, Jan 13, 2013 at 11:08:32PM -0800, Jonathan Nieder wrote:
>
>> Jeff King wrote:
>>
>> > As far as I recall, that script works. However, I have a pure-C
>> > blame-tree implementation that is much faster, which may also be of
>> > interest. I need to clean up and put a few finishing touches on it to
>> > send it to the list, but it has been in production at GitHub for a few
>> > months. You can find it here:
>> >
>> > git://github.com/peff/git jk/blame-tree
>>
>> Oh, neat. Would that make sense as an item in
>> <https://git.wiki.kernel.org/index.php/Interfaces,_frontends,_and_tools>?
>
> I'd rather finish cleaning it up and actually get it merged. It's on my
> todo list.
>
> -Peff
^ permalink raw reply
* Re: [PATCH v2 2/3] push: Add support for pre-push hooks
From: Junio C Hamano @ 2013-01-14 17:39 UTC (permalink / raw)
To: Aaron Schrab; +Cc: git
In-Reply-To: <1358054224-7710-3-git-send-email-aaron@schrab.com>
Aaron Schrab <aaron@schrab.com> writes:
> Add support for a pre-push hook which can be used to determine if the
> set of refs to be pushed is suitable for the target repository. The
> hook is run with two arguments specifying the name and location of the
> destination repository.
>
> Information about what is to be pushed is provided by sending lines of
> the following form to the hook's standard input:
>
> <local ref> SP <local sha1> SP <remote ref> SP <remote sha1> LF
>
> If the hook exits with a non-zero status, the push will be aborted.
>
> This will allow the script to determine if the push is acceptable based
> on the target repository and branch(es), the commits which are to be
> pushed, and even the source branches in some cases.
>
> Signed-off-by: Aaron Schrab <aaron@schrab.com>
> ---
> Documentation/githooks.txt | 29 ++++++++++
> builtin/push.c | 1 +
> t/t5571-pre-push-hook.sh | 129 +++++++++++++++++++++++++++++++++++++++++++++
> transport.c | 60 +++++++++++++++++++++
> transport.h | 1 +
> 5 files changed, 220 insertions(+)
> create mode 100755 t/t5571-pre-push-hook.sh
>
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index b9003fe..d839233 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -176,6 +176,35 @@ save and restore any form of metadata associated with the working tree
> (eg: permissions/ownership, ACLS, etc). See contrib/hooks/setgitperms.perl
> for an example of how to do this.
>
> +pre-push
> +~~~~~~~~
> +
> +This hook is called by 'git push' and can be used to prevent a push from taking
> +place. The hook is called with two parameters which provide the name and
> +location of the destination remote, if a named remote is not being used both
> +values will be the same.
> +
> +Information about what is to be pushed is provided on the hook's standard
> +input with lines of the form:
> +
> + <local ref> SP <local sha1> SP <remote ref> SP <remote sha1> LF
> +
> +For instance, if the command +git push origin master:foreign+ were run the
Just being curious, but why use +monospace text+ here? Most of the
new text use `monospace text literally` instead in this patch.
> +hook would receive a line like the following:
> +
> + refs/heads/master 67890 refs/heads/foreign 12345
> +
> +although the full, 40-character SHA1s would be supplied.
Perhaps ellipses are called for here?
refs/heads/master 67890... refs/heads/foreign 12345...
(the above abbreviates full 40-hexdigits for illustration purposes only)
> +If the foreign ref
> +does not yet exist the `<remote SHA1>` will be 40 `0`. If a ref is to be
> +deleted, the `<local ref>` will be supplied as `(delete)` and the `<local
> +SHA1>` will be 40 `0`. If the local commit was specified by something other
> +than a name which could be expanded (such as `HEAD~`, or a SHA1) it will be
> +supplied as it was originally given.
> +
> +If this hook exits with a non-zero status, 'git push' will abort without
> +pushing anything. Information about why the push is rejected may be sent
> +to the user by writing to standard error.
s/standard error/& of the hook/; perhaps? It is unclear who does
the writing and it can be misunderstood that git-push will write to
standard error upon seeing your hook that silently exits.
> diff --git a/builtin/push.c b/builtin/push.c
> index 8491e43..b158028 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -407,6 +407,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
> OPT_BOOL(0, "progress", &progress, N_("force progress reporting")),
> OPT_BIT(0, "prune", &flags, N_("prune locally removed refs"),
> TRANSPORT_PUSH_PRUNE),
> + OPT_BIT(0, "no-verify", &flags, N_("bypass pre-push hook"), TRANSPORT_PUSH_NO_HOOK),
> OPT_END()
> };
So to countermand this, you have to say --no-no-verify? Wouldn't it
be more natural to introduce a --verify option that turns the bit
on, which automatically gives you --no-verify to turn it off? A
bit in a flag word can be initialized to true before the flag word
is given to the parse_options() machinery to make the field default
to true, no?
^ permalink raw reply
* Re: [PATCH v2 3/3] Add sample pre-push hook script
From: Junio C Hamano @ 2013-01-14 17:42 UTC (permalink / raw)
To: Aaron Schrab; +Cc: git
In-Reply-To: <1358054224-7710-4-git-send-email-aaron@schrab.com>
Aaron Schrab <aaron@schrab.com> writes:
> Create a sample of a script for a pre-push hook. The main purpose is to
> illustrate how a script may parse the information which is supplied to
> such a hook. The script may also be useful to some people as-is for
> avoiding to push commits which are marked as a work in progress.
>
> Signed-off-by: Aaron Schrab <aaron@schrab.com>
> ---
> templates/hooks--pre-push.sample | 53 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
> create mode 100644 templates/hooks--pre-push.sample
>
> diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample
> new file mode 100644
> index 0000000..15ab6d8
> --- /dev/null
> +++ b/templates/hooks--pre-push.sample
> @@ -0,0 +1,53 @@
> +#!/bin/sh
> +
> +# An example hook script to verify what is about to be pushed. Called by "git
> +# push" after it has checked the remote status, but before anything has been
> +# pushed. If this script exits with a non-zero status nothing will be pushed.
> +#
> +# This hook is called with the following parameters:
> +#
> +# $1 -- Name of the remote to which the push is being done
> +# $2 -- URL to which the push is being done
> +#
> +# If pushing without using a named remote those arguments will be equal.
> +#
> +# Information about the commits which are being pushed is supplied as lines to
> +# the standard input in the form:
> +#
> +# <local ref> <local sha1> <remote ref> <remote sha1>
> +#
> +# This sample shows how to prevent push of commits where the log message starts
> +# with "WIP" (work in progress).
An example for a plausible use case is nice to have. I would prefer
to see any new shell script to follow the Git style, though.
> +remote="$1"
> +url="$2"
> +
> +z40=0000000000000000000000000000000000000000
> +
> +IFS=' '
> +while read local_ref local_sha remote_ref remote_sha
> +do
> + if [ "$local_sha" = $z40 ]
> + then
> + # Handle delete
... by doing what?
> + else
> + if [ "$remote_sha" = $z40 ]
> + then
> + # New branch, examine all commits
> + range="$local_sha"
> + else
> + # Update to existing branch, examine new commits
> + range="$remote_sha..$local_sha"
> + fi
> +
> + # Check for WIP commit
> + commit=`git rev-list -n 1 --grep '^WIP' "$range"`
> + if [ -n "$commit" ]
> + then
> + echo "Found WIP commit in $local_ref, not pushing"
> + exit 1
> + fi
> + fi
> +done
> +
> +exit 0
^ permalink raw reply
* Re: [PATCH v2 0/3] pre-push hook support
From: Junio C Hamano @ 2013-01-14 17:42 UTC (permalink / raw)
To: Aaron Schrab; +Cc: git
In-Reply-To: <1358054224-7710-1-git-send-email-aaron@schrab.com>
Aaron Schrab <aaron@schrab.com> writes:
> Main changes since the initial version:
>
> * The first patch converts the existing hook callers to use the new
> find_hook() function.
> * Information about what is to be pushed is now sent over a pipe rather
> than passed as command-line parameters.
>
> Aaron Schrab (3):
> hooks: Add function to check if a hook exists
> push: Add support for pre-push hooks
> Add sample pre-push hook script
Getting much nicer. Thanks.
^ permalink raw reply
* Re: [PATCH] rebase --preserve-merges keeps empty merge commits
From: Phil Hord @ 2013-01-14 17:50 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthieu Moy, git, phil.hord, Neil Horman, Martin von Zweigbergk
In-Reply-To: <7vwqvfele2.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Phil Hord <hordp@cisco.com> writes:
>>
>>> Subject: [PATCH] rebase --preserve-merges keeps empty merge commits
>> I would rephrase it as
>>
>> rebase --preserve-merges: keep empty merge commits
>>
>> we usually give orders in commit messages, not state facts (it's not
>> clear from the existing subject line whether keeping merge commit is the
>> new behavior or a bug that the commit tries to fix).
> Thanks for giving a concise rationale on our use of imperative mood.
>
> Phil, I think you meant to and forgot to sign-off; here is what I'll
> queue.
>
> Thanks.
>
Looks good. Thanks for the help.
Phil
^ permalink raw reply
* Re: [PATCH] t0050: mark TC merge (case change) as success
From: Torsten Bögershausen @ 2013-01-14 17:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Torsten Bögershausen, git, prohaska
In-Reply-To: <7v7gnghdj2.fsf@alter.siamese.dyndns.org>
On 14.01.13 00:24, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> The test "merge (case change)" passes on a case ignoring file system
>>
>> Use test_expect_success to remove the "known breakage vanished"
>>
>> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
>> ---
>
> Interesting. When did this change? Do you happen to have a
> bisection?
This seems to be the commit:
commit 6aad47dec7a72bb36c64afb6c43f4dbcaa49e7f9
Merge: e13067a 0047dd2
Author: Junio C Hamano <gitster@pobox.com>
Date: Fri May 23 16:05:52 2008 -0700
Merge branch 'sp/ignorecase'
* sp/ignorecase:
t0050: Fix merge test on case sensitive file systems
t0050: Add test for case insensitive add
t0050: Set core.ignorecase case to activate case insensitivity
t0050: Test autodetect core.ignorecase
git-init: autodetect core.ignorecase
Which comes from here:
commit 0047dd2fd1fc1980913901c5fa098357482c2842
Author: Steffen Prohaska <prohaska@zib.de>
Date: Thu May 15 07:19:54 2008 +0200
t0050: Fix merge test on case sensitive file systems
On a case sensitive filesystem, "git reset --hard" might refuse to
overwrite a file whose name differs only by case, even if
core.ignorecase is set. It is not clear which circumstances cause this
behavior. This commit simply works around the problem by removing
the case changing file before running "git reset --hard".
Signed-off-by: Steffen Prohaska <prohaska@zib.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
===========
>Or did the test pass from the very beginning?
Hm, reading the commit, it seems as if the "root problem" still exist:
git reset --hard does not change the case of an existing file
What is the exist behvior?
My feeling is that the test as such deserves some more improvements,
the result of the merge is not checked, files are empty so that
the content is not checked.
Another improvement:
Running under Linux gives:
not ok 6 - add (with different case) # TODO known breakage
(and running under mingw failes)
Please stay tuned for more updates, thanks for reviewing.
/Torsten
^ permalink raw reply
* Re: [PATCH 3/6] convert some config callbacks to match_config_key
From: Jeff King @ 2013-01-14 18:05 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Joachim Schmitz, René Scharfe, Junio C Hamano, git
In-Reply-To: <20130114170610.GB22098@sigill.intra.peff.net>
On Mon, Jan 14, 2013 at 09:06:10AM -0800, Jeff King wrote:
> struct config_key k;
> parse_config_key(&k, var);
> if (strcmp(k.section, "filter") || k.subsection))
> return 0;
>
> would be a better start (or having git_config do the first two lines
> itself before triggering the callback).
Here's what that looks like, along with the cleanups in submodule.c that
are made possible by it.
I "cheat" a little and use a static buffer when parsing the config key,
so that the caller does not have to deal with freeing it. It makes using
the parser literally as simple as the lines above, but it does mean it
isn't re-entrant (and worse, it has to be invoked from a config
callback, since the static buffer is tied to the config file stack).
None of that is a problem for the use here, but it is not a
generally-callable function. For that reason, it might make more sense
to have the config parser just provide the config_key, and not have a
public function at all. The downside to that is that we have to update
the function signature of all of the config callbacks.
---
cache.h | 7 ++++++
config.c | 35 ++++++++++++++++++++++++++++++
submodule.c | 41 ++++++++++++++---------------------
3 files changed, 58 insertions(+), 25 deletions(-)
diff --git a/cache.h b/cache.h
index c257953..df756e6 100644
--- a/cache.h
+++ b/cache.h
@@ -1119,6 +1119,13 @@ extern int update_server_info(int);
#define CONFIG_INVALID_PATTERN 6
#define CONFIG_GENERIC_ERROR 7
+struct config_key {
+ const char *section;
+ const char *subsection;
+ const char *key;
+};
+void config_key_parse(struct config_key *, const char *);
+
typedef int (*config_fn_t)(const char *, const char *, void *);
extern int git_default_config(const char *, const char *, void *);
extern int git_config_from_file(config_fn_t fn, const char *, void *);
diff --git a/config.c b/config.c
index 7b444b6..7b8df3e 100644
--- a/config.c
+++ b/config.c
@@ -18,6 +18,7 @@ typedef struct config_file {
int eof;
struct strbuf value;
struct strbuf var;
+ struct strbuf key_parse_buf;
} config_file;
static config_file *cf;
@@ -899,6 +900,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
top.eof = 0;
strbuf_init(&top.value, 1024);
strbuf_init(&top.var, 1024);
+ strbuf_init(&top.key_parse_buf, 1024);
cf = ⊤
ret = git_parse_file(fn, data);
@@ -906,6 +908,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
/* pop config-file parsing state stack */
strbuf_release(&top.value);
strbuf_release(&top.var);
+ strbuf_release(&top.key_parse_buf);
cf = top.prev;
fclose(f);
@@ -1667,3 +1670,35 @@ int config_error_nonbool(const char *var)
{
return error("Missing value for '%s'", var);
}
+
+void config_key_parse(struct config_key *key, const char *var)
+{
+ /*
+ * We want to use a static buffer so the caller does not have to worry
+ * about memory ownership. But since config parsing can happen
+ * recursively, we must use storage from the stack of config files.
+ */
+ struct strbuf *sb = &cf->key_parse_buf;
+ char *dot;
+ char *rdot;
+
+ strbuf_reset(sb);
+ strbuf_addstr(sb, var);
+
+ dot = strchr(sb->buf, '.');
+ rdot = strrchr(sb->buf, '.');
+ /* Should never happen because our keys come from git_parse_file. */
+ if (!dot)
+ die("BUG: config_key_parse was fed a bogus key");
+ key->section = sb->buf;
+ *dot = '\0';
+ key->key = rdot + 1;
+
+ if (rdot == dot)
+ key->subsection = NULL;
+ else {
+ *rdot = '\0';
+ key->subsection = dot + 1;
+ }
+
+}
diff --git a/submodule.c b/submodule.c
index 2f55436..4894718 100644
--- a/submodule.c
+++ b/submodule.c
@@ -11,9 +11,9 @@
#include "sha1-array.h"
#include "argv-array.h"
-static struct string_list config_name_for_path;
-static struct string_list config_fetch_recurse_submodules_for_name;
-static struct string_list config_ignore_for_name;
+static struct string_list config_name_for_path = STRING_LIST_INIT_DUP;
+static struct string_list config_fetch_recurse_submodules_for_name = STRING_LIST_INIT_DUP;
+static struct string_list config_ignore_for_name = STRING_LIST_INIT_DUP;
static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
static struct string_list changed_submodule_paths;
static int initialized_fetch_ref_tips;
@@ -126,47 +126,38 @@ int parse_submodule_config_option(const char *var, const char *value)
int parse_submodule_config_option(const char *var, const char *value)
{
- int len;
+ struct config_key key;
struct string_list_item *config;
- struct strbuf submodname = STRBUF_INIT;
- var += 10; /* Skip "submodule." */
+ config_key_parse(&key, var);
+ if (strcmp(key.section, "submodule") || !key.subsection)
+ return 0;
- len = strlen(var);
- if ((len > 5) && !strcmp(var + len - 5, ".path")) {
- strbuf_add(&submodname, var, len - 5);
+ if (!strcmp(key.key, "path")) {
config = unsorted_string_list_lookup(&config_name_for_path, value);
if (config)
free(config->util);
else
- config = string_list_append(&config_name_for_path, xstrdup(value));
- config->util = strbuf_detach(&submodname, NULL);
- strbuf_release(&submodname);
- } else if ((len > 23) && !strcmp(var + len - 23, ".fetchrecursesubmodules")) {
- strbuf_add(&submodname, var, len - 23);
- config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf);
+ config = string_list_append(&config_name_for_path, value);
+ config->util = xstrdup(key.subsection);
+ } else if (!strcmp(key.key, "fetchrecursesubmodules")) {
+ config = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, key.subsection);
if (!config)
- config = string_list_append(&config_fetch_recurse_submodules_for_name,
- strbuf_detach(&submodname, NULL));
+ config = string_list_append(&config_fetch_recurse_submodules_for_name, key.subsection);
config->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value);
- strbuf_release(&submodname);
- } else if ((len > 7) && !strcmp(var + len - 7, ".ignore")) {
+ } else if (!strcmp(key.key, "ignore")) {
if (strcmp(value, "untracked") && strcmp(value, "dirty") &&
strcmp(value, "all") && strcmp(value, "none")) {
warning("Invalid parameter \"%s\" for config option \"submodule.%s.ignore\"", value, var);
return 0;
}
- strbuf_add(&submodname, var, len - 7);
- config = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf);
+ config = unsorted_string_list_lookup(&config_ignore_for_name, key.subsection);
if (config)
free(config->util);
else
- config = string_list_append(&config_ignore_for_name,
- strbuf_detach(&submodname, NULL));
- strbuf_release(&submodname);
+ config = string_list_append(&config_ignore_for_name, key.subsection);
config->util = xstrdup(value);
- return 0;
}
return 0;
}
^ permalink raw reply related
* Re: [PATCH 1/6] config: add helper function for parsing key names
From: Junio C Hamano @ 2013-01-14 18:08 UTC (permalink / raw)
To: Jeff King; +Cc: Joachim Schmitz, René Scharfe, git
In-Reply-To: <20130114150012.GA16828@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The config callback functions get keys of the general form:
>
> section.subsection.key
>
> (where the subsection may be contain arbitrary data, or may
> be missing). For matching keys without subsections, it is
> simple enough to call "strcmp". Matching keys with
> subsections is a little more complicated, and each callback
> does it in an ad-hoc way, usually involving error-prone
> pointer arithmetic.
>
> Let's provide a helper that keeps the pointer arithmetic all
> in one place.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> No users yet; they come in future patches.
>
> cache.h | 15 +++++++++++++++
> config.c | 33 +++++++++++++++++++++++++++++++++
> 2 files changed, 48 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index c257953..14003b8 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1164,6 +1164,21 @@ extern int git_config_include(const char *name, const char *value, void *data);
> #define CONFIG_INCLUDE_INIT { 0 }
> extern int git_config_include(const char *name, const char *value, void *data);
>
> +/*
> + * Match and parse a config key of the form:
> + *
> + * section.(subsection.)?key
> + *
> + * (i.e., what gets handed to a config_fn_t). The caller provides the section;
> + * we return -1 if it does not match, 0 otherwise. The subsection and key
> + * out-parameters are filled by the function (and subsection is NULL if it is
> + * missing).
> + */
> +extern int match_config_key(const char *var,
> + const char *section,
> + const char **subsection, int *subsection_len,
> + const char **key);
> +
I agree with Jonathan about the naming s/match/parse/.
After looking at the callers in your later patches, I think the
counted interface to subsection is probably fine. The caller can
check !subsection to see if it is a two- or three- level name, and
if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 ||
!name)
return 0;
is very easy to follow (that is the result of your 5th step).
^ permalink raw reply
* [PATCH] clone: do not export and unexport GIT_CONFIG
From: Junio C Hamano @ 2013-01-14 18:11 UTC (permalink / raw)
To: git
Earlier, dc87183 (use GIT_CONFIG only in "git config", not other
programs, 2008-06-30) made sure that the environment variable is
never used outside "git config", but "git clone", after creating a
directory for the new repository and until the init_db() function
populates its .git/ directory, exported the variable for no good
reason. No hook will run from init_db() and more importantly no
hook can run until init_db() finishes creation of the new
repository, so it cannot be used by any invocation of "git config"
by definition.
Stop doing the useless export/unexport.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/clone.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index a4d8d25..6f0c1c6 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -714,8 +714,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
atexit(remove_junk);
sigchain_push_common(remove_junk_on_signal);
- setenv(CONFIG_ENVIRONMENT, mkpath("%s/config", git_dir), 1);
-
if (safe_create_leading_directories_const(git_dir) < 0)
die(_("could not create leading directories of '%s'"), git_dir);
@@ -732,13 +730,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
init_db(option_template, INIT_DB_QUIET);
write_config(&option_config);
- /*
- * At this point, the config exists, so we do not need the
- * environment variable. We actually need to unset it, too, to
- * re-enable parsing of the global configs.
- */
- unsetenv(CONFIG_ENVIRONMENT);
-
git_config(git_default_config, NULL);
if (option_bare) {
--
1.8.1.407.g91cb4ac
^ permalink raw reply related
* Re: [PATCH] remote-hg: store converted URL
From: Junio C Hamano @ 2013-01-14 18:14 UTC (permalink / raw)
To: Max Horn; +Cc: git, Felipe Contreras
In-Reply-To: <1357760618-81222-1-git-send-email-max@quendi.de>
Max Horn <max@quendi.de> writes:
> From: Felipe Contreras <felipe.contreras@gmail.com>
>
> Mercurial might convert the URL to something more appropriate, like an
> absolute path.
"What it is converted *TO*" is fairly clear with ", like an ...",
but from the first reading it was unclear to me "what it is
converted *FROM*" and "*WHEN* the conversion happens". Do you mean
that the user gives "git clone" an URL "../hg-repo" via the command
line (e.g. the argument to "git clone" is spelled something like
"hg::../hg-repo"), and that "../hg-repo" is rewritten to something
else (an absolute path, e.g. "/srv/project/hg-repo")?
> Lets store that instead of the original URL, which won't
> work from a different working directory if it's relative.
What is lacking from this description is why it even needs to work
from a different working directory. I am guessing that remote-hg
later creates a hidden Hg repository or something in a different
place and still tries to use the URL to interact with the upstream,
and that is what breaks, but with only the above description without
looking at your original report, people who will read the "git log"
output and find this change will not be able to tell why this was
needed, I am afraid.
Of course, the above guess of mine may even be wrong, but then that
is yet another reason that the log needs to explain the change
better.
> Suggested-by: Max Horn <max@quendi.de>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Max Horn <max@quendi.de>
> ---
> For a discussion of the problem, see also
> http://article.gmane.org/gmane.comp.version-control.git/210250
I do not see any discussion; only your problem report.
Was this work done outside the list? I just want to make sure this
patch is not something Felipe did not want to sign off for whatever
reason but you are passing it to the list as a patch signed off by
him.
^ permalink raw reply
* Re: [PATCH 2/3] config: Introduce diff.algorithm variable
From: Junio C Hamano @ 2013-01-14 18:33 UTC (permalink / raw)
To: Michal Privoznik; +Cc: git, peff, trast
In-Reply-To: <72370b372a56cc5bfaa9741eae62eae2854964b2.1358006339.git.mprivozn@redhat.com>
Michal Privoznik <mprivozn@redhat.com> writes:
> Some users or projects prefer different algorithms over others, e.g.
> patience over myers or similar. However, specifying appropriate
> argument every time diff is to be used is impractical. Moreover,
> creating an alias doesn't play nicely with other tools based on diff
> (git-show for instance). Hence, a configuration variable which is able
> to set specific algorithm is needed. For now, these four values are
> accepted: 'myers' (which has the same effect as not setting the config
> variable at all), 'minimal', 'patience' and 'histogram'.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> Documentation/diff-config.txt | 17 +++++++++++++++++
> contrib/completion/git-completion.bash | 1 +
> diff.c | 23 +++++++++++++++++++++++
> 3 files changed, 41 insertions(+)
>
> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
> index 4314ad0..be31276 100644
> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -155,3 +155,20 @@ diff.tool::
> "kompare". Any other value is treated as a custom diff tool,
> and there must be a corresponding `difftool.<tool>.cmd`
> option.
> +
> +diff.algorithm::
> + Choose a diff algorithm. The variants are as follows:
> ++
> +--
> +`myers`;;
> + The basic greedy diff algorithm.
> +`minimal`;;
> + Spend extra time to make sure the smallest possible diff is
> + produced.
> +`patience`;;
> + Use "patience diff" algorithm when generating patches.
> +`histogram`;;
> + This algorithm extends the patience algorithm to "support
> + low-occurrence common elements".
> +--
> ++
Sounds sensible.
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 383518c..33e25dc 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1839,6 +1839,7 @@ _git_config ()
> diff.suppressBlankEmpty
> diff.tool
> diff.wordRegex
> + diff.algorithm
> difftool.
> difftool.prompt
> fetch.recurseSubmodules
> diff --git a/diff.c b/diff.c
> index 732d4c2..ddae5c4 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -36,6 +36,7 @@ static int diff_no_prefix;
> static int diff_stat_graph_width;
> static int diff_dirstat_permille_default = 30;
> static struct diff_options default_diff_options;
> +static long diff_algorithm = 0;
Please do not initialize a static explicitly to a zero and instead
let BSS do its thing.
> static char diff_colors[][COLOR_MAXLEN] = {
> GIT_COLOR_RESET,
> @@ -143,6 +144,20 @@ static int git_config_rename(const char *var, const char *value)
> return git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
> }
>
> +static long parse_algorithm_value(const char *value)
> +{
> + if (!value || !strcasecmp(value, "myers"))
> + return 0;
> + else if (!strcasecmp(value, "minimal"))
> + return XDF_NEED_MINIMAL;
> + else if (!strcasecmp(value, "patience"))
> + return XDF_PATIENCE_DIFF;
> + else if (!strcasecmp(value, "histogram"))
> + return XDF_HISTOGRAM_DIFF;
> + else
> + return -1;
> +}
> +
> /*
> * These are to give UI layer defaults.
> * The core-level commands such as git-diff-files should
> @@ -196,6 +211,13 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
> return 0;
> }
>
> + if (!strcmp(var, "diff.algorithm")) {
> + diff_algorithm = parse_algorithm_value(value);
> + if (diff_algorithm < 0)
> + return -1;
> + return 0;
> + }
> +
> if (git_color_config(var, value, cb) < 0)
> return -1;
>
> @@ -3213,6 +3235,7 @@ void diff_setup(struct diff_options *options)
> options->add_remove = diff_addremove;
> options->use_color = diff_use_color_default;
> options->detect_rename = diff_detect_rename_default;
> + options->xdl_opts |= diff_algorithm;
>
> if (diff_no_prefix) {
> options->a_prefix = options->b_prefix = "";
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox