* Re: [PATCH v3 3/6] Change 'git' to 'Git' whenever the whole system is referred to #2
From: Junio C Hamano @ 2013-01-23 4:17 UTC (permalink / raw)
To: Thomas Ackermann; +Cc: git, David Aguilar
In-Reply-To: <CAJDDKr4fnUp_35ni72XJS_NSp4jxbvQPENLnk3AhFv2FBg3DTg@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
> On Mon, Jan 21, 2013 at 11:19 AM, Thomas Ackermann <th.acker@arcor.de> wrote:
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index b87f744..5a831ad2 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> push.default::
>> - Defines the action git push should take if no refspec is given
>> + Defines the action Git push should take if no refspec is given
>> on the command line, no refspec is configured in the remote, and
>> no refspec is implied by any of the options given on the command
>> line. Possible values are:
>
> This should probably be "git push" in double quotes.
Yeah, or no quotes, or `git push` even.
I've pushed the result of my nitpicks I sent so far as part of the
'pu' branch, on topic 'ta/doc-no-small-caps'. Its tip is now at
bfb8e1e (fixup! Change 'git' to 'Git' whenever the whole system is
referred to #4, 2013-01-22).
Thomas, I do not want to see many rounds of entire rerolls of this
series on the list (nobody will look at the whole series multiple
times with fine toothed comb). I do not think you want to do that
either. Can you collect remaining fixups like David's message, turn
them into patch form when you have collected enough to be reviewed
in one sitting (say, a patchfile at around 200 lines), and send them
over to the list to apply on top of the tree of that commit?
After a week or so, I'll squash the series into two commits (the
first one turns 'GIT' into 'Git', the second one turns selected
'git' into 'Git'), and merge the result to 'next'.
^ permalink raw reply
* [PATCHv2 0/8] config key-parsing cleanups
From: Jeff King @ 2013-01-23 6:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Joachim Schmitz, René Scharfe, git
In-Reply-To: <7va9s6qkkz.fsf@alter.siamese.dyndns.org>
On Fri, Jan 18, 2013 at 12:53:32PM -0800, Junio C Hamano wrote:
> >> ... did you have any comment on
> >> the "struct config_key" alternative I sent as a follow-up?
> >
> > I did read it but I cannot say I did so very carefully. My gut
> > reaction was that the "take the variable name and section name,
> > return the subsection name pointer and length, if there is any, and
> > the key" made it readable enough. The proposed interface to make
> > and lend a copy to the caller does make it more readble, but I do
> > not know if that is worth doing. Neutral-to-slightly-in-favor, I
> > would say.
>
> Now I re-read that "struct config_key" thing, I would have to say
> that the idea of giving split and NUL-terminated strings to the
> callers is good, but the "cheat" looks somewhat brittle for all the
> reasons that come from using a static buffer (which you already
> mentioned). As I do not offhand think of a better alternative, I'd
> say we leave it for another day.
OK. I had the feeling if the config parser provided it to the caller
that more sites could take advantage of it (without adding too many
lines to call the parsing function). But looking again, there aren't
that many sites that would benefit. E.g., git_daemon_config in daemon.c
could use it to avoid using a constant offset. But the current code
there is not hard to read, and saving a few characters there is not
worth the complexity.
So I've re-rolled the original version, taking into account the comments
from you and Jonathan. I also clarified a few of the commit messages,
and modified two more sites to use the new function.
[1/8]: config: add helper function for parsing key names
Same as before, but now called parse_config_key.
[2/8]: archive-tar: use parse_config_key when parsing config
Same (rebased for new name, of course).
[3/8]: convert some config callbacks to parse_config_key
Tweaked confusing "ep" variable name. Fixed missing "!name" check in
userdiff code (which gets removed in the next patch anyway).
[4/8]: userdiff: drop parse_driver function
[5/8]: submodule: use parse_config_key when parsing config
[6/8]: submodule: simplify memory handling in config parsing
Same.
[7/8]: help: use parse_config_key for man config
[8/8]: reflog: use parse_config_key in config callback
Two new callsites. I split these out because unlike the ones in 3/8,
they do not benefit from a reduction in lines of code. However, I think
the results are still more readable. You can judge for yourself; drop
them if you disagree. Or feel free to squash them into 3/8 if that makes
more sense.
-Peff
^ permalink raw reply
* [PATCHv2 1/8] config: add helper function for parsing key names
From: Jeff King @ 2013-01-23 6:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Joachim Schmitz, René Scharfe, git
In-Reply-To: <20130123062132.GA2038@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>
---
cache.h | 15 +++++++++++++++
config.c | 33 +++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/cache.h b/cache.h
index c257953..b19305b 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 parse_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..11bd4d8 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 parse_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.0.2.15.g815dc66
^ permalink raw reply related
* [PATCHv2 2/8] archive-tar: use parse_config_key when parsing config
From: Jeff King @ 2013-01-23 6:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Joachim Schmitz, René Scharfe, git
In-Reply-To: <20130123062132.GA2038@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>
---
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 d1cce46..719b629 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -327,20 +327,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 (parse_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 e7c240f..3fbd366 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -212,7 +212,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.0.2.15.g815dc66
^ permalink raw reply related
* [PATCHv2 3/8] convert some config callbacks to parse_config_key
From: Jeff King @ 2013-01-23 6:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Joachim Schmitz, René Scharfe, git
In-Reply-To: <20130123062132.GA2038@sigill.intra.peff.net>
These callers can drop some inline pointer arithmetic and
magic offset constants, making them more readable and less
error-prone (those constants had to match the lengths of
strings, but there is no automatic verification of that
fact).
The "ep" pointer (presumably for "end pointer"), which
points to the final key segment of the config variable, is
given the more standard name "key" to describe its function
rather than its derivation.
Signed-off-by: Jeff King <peff@peff.net>
---
convert.c | 14 +++++---------
ll-merge.c | 14 +++++---------
userdiff.c | 13 +++----------
3 files changed, 13 insertions(+), 28 deletions(-)
diff --git a/convert.c b/convert.c
index 6602155..3520252 100644
--- a/convert.c
+++ b/convert.c
@@ -457,7 +457,7 @@ static int read_convert_config(const char *var, const char *value, void *cb)
static int read_convert_config(const char *var, const char *value, void *cb)
{
- const char *ep, *name;
+ const char *key, *name;
int namelen;
struct convert_driver *drv;
@@ -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 (parse_config_key(var, "filter", &name, &namelen, &key) < 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:
@@ -490,13 +486,13 @@ static int read_convert_config(const char *var, const char *value, void *cb)
* The command-line will not be interpolated in any way.
*/
- if (!strcmp("smudge", ep))
+ if (!strcmp("smudge", key))
return git_config_string(&drv->smudge, var, value);
- if (!strcmp("clean", ep))
+ if (!strcmp("clean", key))
return git_config_string(&drv->clean, var, value);
- if (!strcmp("required", ep)) {
+ if (!strcmp("required", key)) {
drv->required = git_config_bool(var, value);
return 0;
}
diff --git a/ll-merge.c b/ll-merge.c
index acea33b..fb61ea6 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -222,7 +222,7 @@ static int read_merge_config(const char *var, const char *value, void *cb)
static int read_merge_config(const char *var, const char *value, void *cb)
{
struct ll_merge_driver *fn;
- const char *ep, *name;
+ const char *key, *name;
int namelen;
if (!strcmp(var, "merge.default")) {
@@ -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 (parse_config_key(var, "merge", &name, &namelen, &key) < 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,16 +254,14 @@ 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 (!strcmp("name", key)) {
if (!value)
return error("%s: lacks value", var);
fn->description = xstrdup(value);
return 0;
}
- if (!strcmp("driver", ep)) {
+ if (!strcmp("driver", key)) {
if (!value)
return error("%s: lacks value", var);
/*
@@ -289,7 +285,7 @@ static int read_merge_config(const char *var, const char *value, void *cb)
return 0;
}
- if (!strcmp("recursive", ep)) {
+ if (!strcmp("recursive", key)) {
if (!value)
return error("%s: lacks value", var);
fn->recursive = xstrdup(value);
diff --git a/userdiff.c b/userdiff.c
index ed958ef..a4ea1e9 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 (parse_config_key(var, "diff", &name, &namelen, &key) < 0 ||
+ !name || 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.0.2.15.g815dc66
^ permalink raw reply related
* [PATCHv2 4/8] userdiff: drop parse_driver function
From: Jeff King @ 2013-01-23 6:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Joachim Schmitz, René Scharfe, git
In-Reply-To: <20130123062132.GA2038@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 confirming that the key is a valid userdiff key, we
may accidentally 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>
---
userdiff.c | 50 +++++++++++++++++++++-----------------------------
1 file changed, 21 insertions(+), 29 deletions(-)
diff --git a/userdiff.c b/userdiff.c
index a4ea1e9..ea43a03 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 (parse_config_key(var, "diff", &name, &namelen, &key) < 0 ||
- !name || 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 (parse_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.0.2.15.g815dc66
^ permalink raw reply related
* [PATCHv2 5/8] submodule: use parse_config_key when parsing config
From: Jeff King @ 2013-01-23 6:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Joachim Schmitz, René Scharfe, git
In-Reply-To: <20130123062132.GA2038@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]
$ git -c submodule.foo.fetchrecursesubmodules=bogus checkout
fatal: bad submodule.foo.fetchrecursesubmodules argument: bogus
Signed-off-by: Jeff King <peff@peff.net>
---
submodule.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/submodule.c b/submodule.c
index 2f55436..25413de 100644
--- a/submodule.c
+++ b/submodule.c
@@ -126,15 +126,16 @@ 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 (parse_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 +143,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.0.2.15.g815dc66
^ permalink raw reply related
* [PATCHv2 6/8] submodule: simplify memory handling in config parsing
From: Jeff King @ 2013-01-23 6:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Joachim Schmitz, René Scharfe, git
In-Reply-To: <20130123062132.GA2038@sigill.intra.peff.net>
We keep a strbuf for the name of the submodule, even though
we only ever add one string 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>
---
submodule.c | 30 ++++++++++++++----------------
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/submodule.c b/submodule.c
index 25413de..9ba1496 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;
@@ -135,37 +134,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.0.2.15.g815dc66
^ permalink raw reply related
* [PATCHv2 7/8] help: use parse_config_key for man config
From: Jeff King @ 2013-01-23 6:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Joachim Schmitz, René Scharfe, git
In-Reply-To: <20130123062132.GA2038@sigill.intra.peff.net>
The resulting code ends up about the same length, but it is
a little more self-explanatory. It now explicitly documents
and checks the pre-condition that the incoming var starts
with "man.", and drops the magic offset "4".
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/help.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/builtin/help.c b/builtin/help.c
index bd86253..04cb77d 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -237,21 +237,21 @@ static int add_man_viewer_info(const char *var, const char *value)
static int add_man_viewer_info(const char *var, const char *value)
{
- const char *name = var + 4;
- const char *subkey = strrchr(name, '.');
+ const char *name, *subkey;
+ int namelen;
- if (!subkey)
+ if (parse_config_key(var, "man", &name, &namelen, &subkey) < 0 || !name)
return 0;
- if (!strcmp(subkey, ".path")) {
+ if (!strcmp(subkey, "path")) {
if (!value)
return config_error_nonbool(var);
- return add_man_viewer_path(name, subkey - name, value);
+ return add_man_viewer_path(name, namelen, value);
}
- if (!strcmp(subkey, ".cmd")) {
+ if (!strcmp(subkey, "cmd")) {
if (!value)
return config_error_nonbool(var);
- return add_man_viewer_cmd(name, subkey - name, value);
+ return add_man_viewer_cmd(name, namelen, value);
}
return 0;
--
1.8.0.2.15.g815dc66
^ permalink raw reply related
* [PATCHv2 8/8] reflog: use parse_config_key in config callback
From: Jeff King @ 2013-01-23 6:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Joachim Schmitz, René Scharfe, git
In-Reply-To: <20130123062132.GA2038@sigill.intra.peff.net>
This doesn't save any lines, but does keep us from doing
error-prone pointer arithmetic with constants.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/reflog.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index b3c9e27..1fedf66 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -510,26 +510,27 @@ static int reflog_expire_config(const char *var, const char *value, void *cb)
static int reflog_expire_config(const char *var, const char *value, void *cb)
{
- const char *lastdot = strrchr(var, '.');
+ const char *pattern, *key;
+ int pattern_len;
unsigned long expire;
int slot;
struct reflog_expire_cfg *ent;
- if (!lastdot || prefixcmp(var, "gc."))
+ if (parse_config_key(var, "gc", &pattern, &pattern_len, &key) < 0)
return git_default_config(var, value, cb);
- if (!strcmp(lastdot, ".reflogexpire")) {
+ if (!strcmp(key, "reflogexpire")) {
slot = EXPIRE_TOTAL;
if (parse_expire_cfg_value(var, value, &expire))
return -1;
- } else if (!strcmp(lastdot, ".reflogexpireunreachable")) {
+ } else if (!strcmp(key, "reflogexpireunreachable")) {
slot = EXPIRE_UNREACH;
if (parse_expire_cfg_value(var, value, &expire))
return -1;
} else
return git_default_config(var, value, cb);
- if (lastdot == var + 2) {
+ if (!pattern) {
switch (slot) {
case EXPIRE_TOTAL:
default_reflog_expire = expire;
@@ -541,7 +542,7 @@ static int reflog_expire_config(const char *var, const char *value, void *cb)
return 0;
}
- ent = find_cfg_ent(var + 3, lastdot - (var+3));
+ ent = find_cfg_ent(pattern, pattern_len);
if (!ent)
return -1;
switch (slot) {
--
1.8.0.2.15.g815dc66
^ permalink raw reply related
* Re: [PATCH v2 1/3] push: further clean up fields of "struct ref"
From: Jeff King @ 2013-01-23 6:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Chris Rorvick
In-Reply-To: <1358836230-9197-2-git-send-email-gitster@pobox.com>
On Mon, Jan 21, 2013 at 10:30:28PM -0800, Junio C Hamano wrote:
> The "nonfastforward" and "update" fields are only used while
> deciding what value to assign to the "status" locally in a single
> function. Remove them from the "struct ref".
>
> The "requires_force" field is not used to decide if the proposed
> update requires a --force option to succeed, or to record such a
> decision made elsewhere. It is used by status reporting code that
> the particular update was "forced". Rename it to "forced_udpate",
Typo.
> and move the code to assign to it around to further clarify how it
> is used and what it is used for.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * The "update" removal in v1 has been moved to this.
>
> cache.h | 4 +---
> remote.c | 16 ++++++----------
> transport.c | 2 +-
> 3 files changed, 8 insertions(+), 14 deletions(-)
Looks much better.
I wondered briefly why nonfastforward was even there, as I recall that I
was the one who added it many years ago. It turns out that it used to
serve the purpose of the new forced_update, but Chris's series from a
few months ago split it out to "nonfastforward" and "not_forwardable",
and then added "requires_force" to give a single flag that is set in
either case.
So I think your simplification is correct; the first two can be local
variables, and the only thing that matters to carry forward is
requires_force (and I agree that forced_update is a better name).
-Peff
^ permalink raw reply
* Re: [PATCH] all: new command used for multi-repo operations
From: Junio C Hamano @ 2013-01-23 6:44 UTC (permalink / raw)
To: David Aguilar; +Cc: Lars Hjemli, git
In-Reply-To: <CAJDDKr6exXh14m08HTihxREjSFgyPT0bN1cF8eUryXJHOgFL1A@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
>> +static int walk(struct strbuf *path, int argc, const char **argv)
>> +{
>> + DIR *dir;
>> + struct dirent *ent;
>> + size_t len;
>> +
>> + dir = opendir(path->buf);
>> + if (!dir)
>> + return errno;
>> + strbuf_addstr(path, "/");
>> + len = path->len;
>> + while ((ent = readdir(dir))) {
>> + if (!strcmp(ent->d_name, ".") || !strcmp(ent->d_name, ".."))
>> + continue;
>> + if (!strcmp(ent->d_name, ".git")) {
>> + strbuf_setlen(path, len - 1);
>> + chdir(path->buf);
>> + handle_repo(path->buf, argv);
>> + chdir(root);
>> + strbuf_addstr(path, "/");
>> + continue;
>> + }
>
> Does this section above properly handle .git files (where .git is a
> file, not a directory)?
This scans a directory $D to ask "is there '.git' in you?" and if
the answer is "yes", then hands $D (not "$D/.git") to handle_repo().
That logic will not miss a gitfile that points at the real $GIT_DIR
elsewhere.
There is a recursive call to walk() later in the same loop when the
found entry ent turns out to be a directory, and "$D/" + ent->d_name
is given to this function.
But I do not think the loop structure of this function is right. If
$D has ".git" in it, should it even try to feed other subdirectories
of $D (say "$D/a") to itself in recursion to see if $D/a/.git exists?
I think it should be more like
walk(struct strbuf *path)
{
size_t dirlen = path->len;
int has_git;
strbuf_addstr(path, "/.git");
has_git = !lstat(path->buf);
strbuf_setlen(path, dirlen);
if (has_git) {
handle_repo(path->buf);
return;
}
dir = opendir(path->buf);
while ((ent = readdir(dir))) {
... skip . and .. ...
strbuf_addstr(path, ent->d_name);
walk(path);
strbuf_setlen(path, dirlen);
}
}
The determination of has_git can be a bit fancier than a simple
!lstat() as you mentioned.
^ permalink raw reply
* Re: [PATCH] all: new command used for multi-repo operations
From: Junio C Hamano @ 2013-01-23 6:52 UTC (permalink / raw)
To: Lars Hjemli; +Cc: git
In-Reply-To: <1358889019-4554-1-git-send-email-hjemli@gmail.com>
Lars Hjemli <hjemli@gmail.com> writes:
> +static struct option builtin_all_options[] = {
> + OPT_BOOLEAN('c', "clean", &only_clean, N_("only show clean repositories")),
> + OPT_BOOLEAN('d', "dirty", &only_dirty, N_("only show dirty repositories")),
> + OPT_END(),
> +};
If you were to go in the OPT_SET_INT route, that would give users
the usual "last one wins" semantics, e.g.
$ git for-each-repo --clean --dirty
will look for only dirty repositories. For completeness, we would
probably want "all" to defeat either of them, i.e.
$ git for-each-repo --clean --all
> +static int walk(struct strbuf *path, int argc, const char **argv)
> +{
> + DIR *dir;
> + struct dirent *ent;
> + size_t len;
> +
> + dir = opendir(path->buf);
> + if (!dir)
> + return errno;
> + strbuf_addstr(path, "/");
> + len = path->len;
> + while ((ent = readdir(dir))) {
> + if (!strcmp(ent->d_name, ".") || !strcmp(ent->d_name, ".."))
> + continue;
> + if (!strcmp(ent->d_name, ".git")) {
This only looks for the top of working tree. Have you considered if
this "iterate over directories and list git repositories in them"
may be useful for collection of bare repositories, and if it is, how
to go about implementing the discovery process?
> + if (ent->d_type != DT_DIR)
> + continue;
I think this is wrong.
On platforms that need a NO_D_TYPE_IN_DIRENT build, your compilation
may fail here (you would need to lstat() it yourself). See how
dir.c does this without ugly #ifdef's in the code, especially around
the use of get_dtype() and DTYPE() macro.
^ permalink raw reply
* Re: [PATCH v2 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
From: Jeff King @ 2013-01-23 6:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Chris Rorvick
In-Reply-To: <1358836230-9197-3-git-send-email-gitster@pobox.com>
On Mon, Jan 21, 2013 at 10:30:29PM -0800, Junio C Hamano wrote:
> When we push to update an existing ref, if:
>
> * we do not have the object at the tip of the remote; or
> * the object at the tip of the remote is not a commit; or
> * the object we are pushing is not a commit,
>
> there is no point suggesting to fetch, integrate and push again.
>
> If we do not have the current object at the tip of the remote, we
> should tell the user to fetch first and evaluate the situation
> before deciding what to do next.
Should we? I know that it is more correct to do so, because we do not
even know for sure that the remote object is a commit, and fetching
_might_ lead to us saying "hey, this is not something that can be
fast-forwarded".
But by far the common case will be that it _is_ a commit, and the right
thing is going to be to pull. Adding in the extra steps makes the
workflow longer and more complicated, and most of the time doesn't
matter. For example, imagine that Alice is working on "master", and when
she goes to push, she finds that Bob has already pushed his work. With
the current code, she sees:
$ git push
To ...
! [rejected] HEAD -> master (non-fast-forward)
error: failed to push some refs to '...'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.
and she presumably pulls, and all is well with the follow-up push.
With your patch, she sees:
$ git push
To ...
! [rejected] HEAD -> master (fetch first)
error: failed to push some refs to '...'
hint: Updates were rejected; you need to fetch the destination reference
hint: to decide what to do.
$ git fetch
...
$ git push
To ...
! [rejected] HEAD -> master (non-fast-forward)
error: failed to push some refs to '...'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
which is technically more correct (it's possible that in the second
step, she would find that Bob pushed a tree or something). But in the
common case that it is a commit, we've needlessly added two extra steps
(a fetch and another failed push), both of which involve network access
(so they are slow, and may involve Alice having to type her credentials).
Is the extra hassle in the common case worth it for the off chance that
we might give a more accurate message? Should the "fetch first" message
be some hybrid that covers both cases accurately, but still points the
user towards "git pull" (which will fail anyway if the remote ref is not
a commit)?
-Peff
^ permalink raw reply
* Re: [PATCHv2 8/8] reflog: use parse_config_key in config callback
From: Junio C Hamano @ 2013-01-23 7:04 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, Joachim Schmitz, René Scharfe, git
In-Reply-To: <20130123062737.GH5036@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This doesn't save any lines, but does keep us from doing
> error-prone pointer arithmetic with constants.
Yeah, this and 7/8 shows that the true value of the new parse
function is not line number reduction but clarity of the calling
code. There is really no point making everybody implement "last_dot
is there, so the subsection name must be between the section name
and that last_dot" like the original before this patch.
Thanks. Will read it over again and then apply.
^ permalink raw reply
* Re: [PATCHv2 0/8] config key-parsing cleanups
From: Jonathan Nieder @ 2013-01-23 7:27 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Joachim Schmitz, René Scharfe, git
In-Reply-To: <20130123062132.GA2038@sigill.intra.peff.net>
Jeff King wrote:
> So I've re-rolled the original version
Lovely. :)
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply
* Re: [PATCH] all: new command used for multi-repo operations
From: Lars Hjemli @ 2013-01-23 7:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
In-Reply-To: <7vy5fkiek0.fsf@alter.siamese.dyndns.org>
On Wed, Jan 23, 2013 at 7:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> But I do not think the loop structure of this function is right. If
> $D has ".git" in it, should it even try to feed other subdirectories
> of $D (say "$D/a") to itself in recursion to see if $D/a/.git exists?
Yes, this is needed to meet one of the goals for git-all: to be an
alternative to git-submodule when submodules doesn't fit (the other
goal is to manage a collection of unrelated (not nested) repos).
--
larsh
^ permalink raw reply
* [PATCH v2] all: new command used for multi-repo operations
From: Lars Hjemli @ 2013-01-23 8:12 UTC (permalink / raw)
To: git; +Cc: Lars Hjemli
When working with multiple, unrelated (or loosly related) git repos,
there is often a need to locate all repos with uncommitted work and
perform some action on them (say, commit and push). Before this patch,
such tasks would require manually visiting all repositories, running
`git status` within each one and then decide what to do next.
This mundane task can now be automated by e.g. `git all --dirty status`,
which will find all git repositories below the current directory (even
nested ones), check if they are dirty (as defined by `git diff --quiet &&
git diff --cached --quiet`), and for each dirty repo print the path to the
repo and then execute `git status` within the repo.
The command also honours the option '--clean' which restricts the set of
repos to those which '--dirty' would skip.
Finally, the command to execute within each repo is optional. If none is
given, git-all will just print the path to each repo found.
Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
Changes since v1:
* uses setenv() instead of chdir(), which fixes .gitfile handling
* uses DTYPE() and stat(), which fixes NO_D_TYPE_IN_DIRENT platforms
* uses OPT_SET_INT() instead of OPT_BOOLEAN
* support for --all (complements --clean/--dirty)
* removed from command-list.txt
* added to .gitignore
I've not yet renamed the command. If it should be changed to 'git
for-each-repo', I'm tempted to make a patch which transforms
`git -ad status` into `git for-each-repo -d status`.
.gitignore | 1 +
Documentation/git-all.txt | 42 ++++++++++++++++++
Makefile | 1 +
builtin.h | 1 +
builtin/all.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++
git.c | 1 +
t/t0064-all.sh | 46 +++++++++++++++++++
7 files changed, 202 insertions(+)
create mode 100644 Documentation/git-all.txt
create mode 100644 builtin/all.c
create mode 100755 t/t0064-all.sh
diff --git a/.gitignore b/.gitignore
index aa258a6..27118d7 100644
--- a/.gitignore
+++ b/.gitignore
@@ -10,6 +10,7 @@
/git
/git-add
/git-add--interactive
+/git-all
/git-am
/git-annotate
/git-apply
diff --git a/Documentation/git-all.txt b/Documentation/git-all.txt
new file mode 100644
index 0000000..baaa57e
--- /dev/null
+++ b/Documentation/git-all.txt
@@ -0,0 +1,42 @@
+git-all(1)
+==========
+
+NAME
+----
+git-all - Execute a git command in multiple repositories
+
+SYNOPSIS
+--------
+[verse]
+'git all' [--all|--clean|--dirty] [command]
+
+DESCRIPTION
+-----------
+The git-all command is used to locate all git repositoris within the
+current directory tree, and optionally execute a git command in each
+of the found repos.
+
+OPTIONS
+-------
+-a::
+--all::
+ Include both clean and dirty repositories (this is the default
+ behaviour of `git-all`).
+
+-c::
+--clean::
+ Only include repositories with a clean worktree.
+
+-d::
+--dirty::
+ Only include repositories with a dirty worktree.
+
+NOTES
+-----
+
+For the purpose of `git-all`, a dirty worktree is defined as a worktree
+with uncommitted changes.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 1b30d7b..8bf0583 100644
--- a/Makefile
+++ b/Makefile
@@ -840,6 +840,7 @@ LIB_OBJS += xdiff-interface.o
LIB_OBJS += zlib.o
BUILTIN_OBJS += builtin/add.o
+BUILTIN_OBJS += builtin/all.o
BUILTIN_OBJS += builtin/annotate.o
BUILTIN_OBJS += builtin/apply.o
BUILTIN_OBJS += builtin/archive.o
diff --git a/builtin.h b/builtin.h
index 7e7bbd6..438c265 100644
--- a/builtin.h
+++ b/builtin.h
@@ -41,6 +41,7 @@ void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c);
extern int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, int sha1_valid, char **buf, unsigned long *buf_size);
extern int cmd_add(int argc, const char **argv, const char *prefix);
+extern int cmd_all(int argc, const char **argv, const char *prefix);
extern int cmd_annotate(int argc, const char **argv, const char *prefix);
extern int cmd_apply(int argc, const char **argv, const char *prefix);
extern int cmd_archive(int argc, const char **argv, const char *prefix);
diff --git a/builtin/all.c b/builtin/all.c
new file mode 100644
index 0000000..b170b26
--- /dev/null
+++ b/builtin/all.c
@@ -0,0 +1,110 @@
+/*
+ * "git all" builtin command.
+ *
+ * Copyright (c) 2013 Lars Hjemli <hjemli@gmail.com>
+ */
+#include "cache.h"
+#include "color.h"
+#include "builtin.h"
+#include "run-command.h"
+#include "parse-options.h"
+
+#define ALL 0
+#define DIRTY 1
+#define CLEAN 2
+
+static int match;
+
+static const char * const builtin_all_usage[] = {
+ N_("git all [--all|--clean|--dirty] [cmd]"),
+ NULL
+};
+
+static struct option builtin_all_options[] = {
+ OPT_SET_INT('a', "all", &match, N_("match both clean and dirty repositories"), ALL),
+ OPT_SET_INT('c', "clean", &match, N_("only show clean repositories"), CLEAN),
+ OPT_SET_INT('d', "dirty", &match, N_("only show dirty repositories"), DIRTY),
+ OPT_END(),
+};
+
+static int get_repo_state()
+{
+ const char *diffidx[] = {"diff", "--quiet", "--cached", NULL};
+ const char *diffwd[] = {"diff", "--quiet", NULL};
+
+ if (run_command_v_opt(diffidx, RUN_GIT_CMD) != 0)
+ return DIRTY;
+ if (run_command_v_opt(diffwd, RUN_GIT_CMD) != 0)
+ return DIRTY;
+ return CLEAN;
+}
+
+static void handle_repo(char *path, const char **argv)
+{
+ if (path[0] == '.' && path[1] == '/')
+ path += 2;
+ if (match != ALL && match != get_repo_state())
+ return;
+ if (*argv) {
+ color_fprintf_ln(stdout, GIT_COLOR_YELLOW, "[%s]", path);
+ run_command_v_opt(argv, RUN_GIT_CMD);
+ } else
+ printf("%s\n", path);
+}
+
+static int walk(struct strbuf *path, int argc, const char **argv)
+{
+ DIR *dir;
+ struct dirent *ent;
+ struct stat st;
+ size_t len;
+
+ dir = opendir(path->buf);
+ if (!dir)
+ return errno;
+ strbuf_addstr(path, "/");
+ len = path->len;
+ while ((ent = readdir(dir))) {
+ if (!strcmp(ent->d_name, ".") || !strcmp(ent->d_name, ".."))
+ continue;
+ if (!strcmp(ent->d_name, ".git")) {
+ strbuf_addstr(path, ent->d_name);
+ setenv(GIT_DIR_ENVIRONMENT, path->buf, 1);
+ strbuf_setlen(path, len - 1);
+ setenv(GIT_WORK_TREE_ENVIRONMENT, path->buf, 1);
+ handle_repo(path->buf, argv);
+ strbuf_addstr(path, "/");
+ continue;
+ }
+ strbuf_setlen(path, len);
+ strbuf_addstr(path, ent->d_name);
+ switch (DTYPE(ent)) {
+ case DT_UNKNOWN:
+ /* Use stat() instead of lstat(), since we want to
+ * know if we can follow this path into another
+ * directory - it's not important if it's actually
+ * a symlink which gets us there.
+ */
+ if (stat(path->buf, &st) || !S_ISDIR(st.st_mode))
+ break;
+ /* fallthrough */
+ case DT_DIR:
+ walk(path, argc, argv);
+ break;
+ }
+ strbuf_setlen(path, len);
+ }
+ closedir(dir);
+ return 0;
+}
+
+int cmd_all(int argc, const char **argv, const char *prefix)
+{
+ struct strbuf path = STRBUF_INIT;
+
+ argc = parse_options(argc, argv, prefix, builtin_all_options,
+ builtin_all_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+
+ strbuf_addstr(&path, ".");
+ return walk(&path, argc, argv);
+}
diff --git a/git.c b/git.c
index ed66c66..53fd963 100644
--- a/git.c
+++ b/git.c
@@ -304,6 +304,7 @@ static void handle_internal_command(int argc, const char **argv)
const char *cmd = argv[0];
static struct cmd_struct commands[] = {
{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
+ { "all", cmd_all },
{ "annotate", cmd_annotate, RUN_SETUP },
{ "apply", cmd_apply, RUN_SETUP_GENTLY },
{ "archive", cmd_archive },
diff --git a/t/t0064-all.sh b/t/t0064-all.sh
new file mode 100755
index 0000000..3738ab2
--- /dev/null
+++ b/t/t0064-all.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+#
+# Copyright (c) 2013 Lars Hjemli
+#
+
+test_description='Test the git-all command'
+
+. ./test-lib.sh
+
+test_expect_success "setup" '
+ test_create_repo clean &&
+ (cd clean && test_commit foo) &&
+ git init --separate-git-dir=.cleansub clean/gitfile &&
+ (cd clean/gitfile && test_commit foo && echo bar >>foo.t) &&
+ test_create_repo dirty-wt &&
+ (cd dirty-wt && test_commit foo && rm foo.t) &&
+ test_create_repo dirty-idx &&
+ (cd dirty-idx && test_commit foo && git rm foo.t)
+'
+
+test_expect_success "without flags, all repos are included" '
+ echo "." >expect &&
+ echo "clean" >>expect &&
+ echo "clean/gitfile" >>expect &&
+ echo "dirty-idx" >>expect &&
+ echo "dirty-wt" >>expect &&
+ git all | sort >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "--dirty only includes dirty repos" '
+ echo "clean/gitfile" >expect &&
+ echo "dirty-idx" >>expect &&
+ echo "dirty-wt" >>expect &&
+ git all --dirty | sort >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "--clean only includes clean repos" '
+ echo "." >expect &&
+ echo "clean" >>expect &&
+ git all --clean | sort >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
1.8.1.1.296.g725455c
^ permalink raw reply related
* Re: [PATCH] all: new command used for multi-repo operations
From: Duy Nguyen @ 2013-01-23 8:39 UTC (permalink / raw)
To: Lars Hjemli; +Cc: git
In-Reply-To: <1358889019-4554-1-git-send-email-hjemli@gmail.com>
On Wed, Jan 23, 2013 at 4:10 AM, Lars Hjemli <hjemli@gmail.com> wrote:
> When working with multiple, unrelated (or loosly related) git repos,
> there is often a need to locate all repos with uncommitted work and
> perform some action on them (say, commit and push). Before this patch,
> such tasks would require manually visiting all repositories, running
> `git status` within each one and then decide what to do next.
>
> This mundane task can now be automated by e.g. `git all --dirty status`,
> which will find all git repositories below the current directory (even
> nested ones), check if they are dirty (as defined by `git diff --quiet &&
> git diff --cached --quiet`), and for each dirty repo print the path to the
> repo and then execute `git status` within the repo.
I think it should leave out the execute part. The command, say
ls-repo, lists repositories in specified state. The execute part could
be easily done by
xargs -I{} git --git-dir={} status blah
I haven't thought it through. I know xargs does not support chdir'ing
into a repo, so maybe a new git-xargs could be introduced for that.
But there's still a problem with git-xargs (or git-all), printed paths
are relative to the subrepos, not where the git-all/xargs command is
executed. This could be confusing. Personally I'd like to do it
without chdir. That is
xargs -I{} git --git-dir={}/.git --work-tree={} status blah
should print paths relative to current working directory even if cwd
is outside the repo.
--
Duy
^ permalink raw reply
* Re: [PATCH] all: new command used for multi-repo operations
From: Lars Hjemli @ 2013-01-23 8:46 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <CACsJy8CTJ--u+KCYmK-2+K3NpEn72xnYwh_Pb+3Wn7nEtL1gqQ@mail.gmail.com>
On Wed, Jan 23, 2013 at 9:39 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, Jan 23, 2013 at 4:10 AM, Lars Hjemli <hjemli@gmail.com> wrote:
>> When working with multiple, unrelated (or loosly related) git repos,
>> there is often a need to locate all repos with uncommitted work and
>> perform some action on them (say, commit and push). Before this patch,
>> such tasks would require manually visiting all repositories, running
>> `git status` within each one and then decide what to do next.
>>
>> This mundane task can now be automated by e.g. `git all --dirty status`,
>> which will find all git repositories below the current directory (even
>> nested ones), check if they are dirty (as defined by `git diff --quiet &&
>> git diff --cached --quiet`), and for each dirty repo print the path to the
>> repo and then execute `git status` within the repo.
>
> I think it should leave out the execute part. The command, say
> ls-repo, lists repositories in specified state. The execute part could
> be easily done by
>
> xargs -I{} git --git-dir={} status blah
Not so easily on windows, which I need to use at $WORK :(
--
larsh
^ permalink raw reply
* [PATCH] all: new command used for multi-repo operations
From: Lars Hjemli @ 2013-01-23 8:52 UTC (permalink / raw)
To: Git Mailing List
In-Reply-To: <CAFXTnz51czE5iS_pgZyU7SdUwrmZcmLxjFGpCGVhLGJFvW=HRQ@mail.gmail.com>
[*git@vger.kernel.org accidentally dropped from cc *]
On Wed, Jan 23, 2013 at 7:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Lars Hjemli <hjemli@gmail.com> writes:
>
>> +static int walk(struct strbuf *path, int argc, const char **argv)
>> +{
>> + DIR *dir;
>> + struct dirent *ent;
>> + size_t len;
>> +
>> + dir = opendir(path->buf);
>> + if (!dir)
>> + return errno;
>> + strbuf_addstr(path, "/");
>> + len = path->len;
>> + while ((ent = readdir(dir))) {
>> + if (!strcmp(ent->d_name, ".") || !strcmp(ent->d_name, ".."))
>> + continue;
>> + if (!strcmp(ent->d_name, ".git")) {
>
> This only looks for the top of working tree. Have you considered if
> this "iterate over directories and list git repositories in them"
> may be useful for collection of bare repositories, and if it is, how
> to go about implementing the discovery process?
Yes, occasionally I've needed this, but implementing it in my original
shell script was cumbersome. Doing it in C should be as simple as
invoking is_git_directory() on each subdir.
>
>> + if (ent->d_type != DT_DIR)
>> + continue;
>
> I think this is wrong.
>
> On platforms that need a NO_D_TYPE_IN_DIRENT build, your compilation
> may fail here (you would need to lstat() it yourself). See how
> dir.c does this without ugly #ifdef's in the code, especially around
> the use of get_dtype() and DTYPE() macro.
>
Thanks for the pointer, will fix.
--
larsh
^ permalink raw reply
* Re: [PATCH v2] all: new command used for multi-repo operations
From: Duy Nguyen @ 2013-01-23 8:55 UTC (permalink / raw)
To: Lars Hjemli; +Cc: git
In-Reply-To: <1358928767-16283-1-git-send-email-hjemli@gmail.com>
On Wed, Jan 23, 2013 at 3:12 PM, Lars Hjemli <hjemli@gmail.com> wrote:
> +NAME
> +----
> +git-all - Execute a git command in multiple repositories
I agree with Junio "git-all" is too generic. Maybe "git-for-each-repo"
> +static int get_repo_state()
> +{
> + const char *diffidx[] = {"diff", "--quiet", "--cached", NULL};
> + const char *diffwd[] = {"diff", "--quiet", NULL};
> +
> + if (run_command_v_opt(diffidx, RUN_GIT_CMD) != 0)
> + return DIRTY;
> + if (run_command_v_opt(diffwd, RUN_GIT_CMD) != 0)
> + return DIRTY;
> + return CLEAN;
> +}
Perhaps we could add the subrepo's object data to the in-memory object
database of git-all, then do the diff without launching new commands?
> +static int walk(struct strbuf *path, int argc, const char **argv)
> +{
> + DIR *dir;
> + struct dirent *ent;
> + struct stat st;
> + size_t len;
> +
> + dir = opendir(path->buf);
> + if (!dir)
> + return errno;
> + strbuf_addstr(path, "/");
> + len = path->len;
> + while ((ent = readdir(dir))) {
> + if (!strcmp(ent->d_name, ".") || !strcmp(ent->d_name, ".."))
> + continue;
> + if (!strcmp(ent->d_name, ".git")) {
> + strbuf_addstr(path, ent->d_name);
> + setenv(GIT_DIR_ENVIRONMENT, path->buf, 1);
> + strbuf_setlen(path, len - 1);
> + setenv(GIT_WORK_TREE_ENVIRONMENT, path->buf, 1);
> + handle_repo(path->buf, argv);
> + strbuf_addstr(path, "/");
> + continue;
> + }
> + strbuf_setlen(path, len);
> + strbuf_addstr(path, ent->d_name);
> + switch (DTYPE(ent)) {
> + case DT_UNKNOWN:
> + /* Use stat() instead of lstat(), since we want to
> + * know if we can follow this path into another
> + * directory - it's not important if it's actually
> + * a symlink which gets us there.
> + */
> + if (stat(path->buf, &st) || !S_ISDIR(st.st_mode))
> + break;
> + /* fallthrough */
> + case DT_DIR:
> + walk(path, argc, argv);
> + break;
> + }
> + strbuf_setlen(path, len);
> + }
> + closedir(dir);
> + return 0;
> +}
I'm not a user of this command so this is more of bikeshedding. I
think we should have an option to list repos listed in index. For
directory walk, how about reusing fill_directory() to do the job for
you? You could then limit repositories by name. "ls-files -o" code
should be very similar.
--
Duy
^ permalink raw reply
* Aw: Re: [PATCH v3 3/6] Change 'git' to 'Git' whenever the whole system is referred to #2
From: Thomas Ackermann @ 2013-01-23 8:58 UTC (permalink / raw)
To: gitster, th.acker; +Cc: git, davvid
In-Reply-To: <7v38xsjzxg.fsf@alter.siamese.dyndns.org>
>
> Thomas, I do not want to see many rounds of entire rerolls of this
> series on the list (nobody will look at the whole series multiple
> times with fine toothed comb). I do not think you want to do that
> either. Can you collect remaining fixups like David's message, turn
> them into patch form when you have collected enough to be reviewed
> in one sitting (say, a patchfile at around 200 lines), and send them
> over to the list to apply on top of the tree of that commit?
>
Sure!
---
Thomas
^ permalink raw reply
* Re: [PATCH v2] all: new command used for multi-repo operations
From: Lars Hjemli @ 2013-01-23 9:24 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <CACsJy8DskoCi9Lg+HW0JeQBe4HX-bMXNHUgfrsg+DoqBN9-ntQ@mail.gmail.com>
On Wed, Jan 23, 2013 at 9:55 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> Perhaps we could add the subrepo's object data to the in-memory object
> database of git-all, then do the diff without launching new commands?
The `git all` command is regularly invoked outside of git repos, so
I'm not sure if this would work.
>
> I'm not a user of this command so this is more of bikeshedding. I
> think we should have an option to list repos listed in index.
git-submodule uses something like `git ls-files --stage|grep
"^160000"`. Having a better way to achieve this would be nice, but I
don't think it is a job for 'git [all|for-each-repo|ls-repo].
> For
> directory walk, how about reusing fill_directory() to do the job for
> you? You could then limit repositories by name. "ls-files -o" code
> should be very similar.
A cursory look into dir.c seems to indicate that this could work
(possibly except for get_index_dtype()), but it would also load the
complete directory tree (which could be extremly big) into ram,
including file entries (which is not necessary) while dropping '.git'
entries (which is what we're looking for).
--
larsh
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2013, #08; Tue, 22)
From: John Keeping @ 2013-01-23 9:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric S. Raymond, Chris Rorvick
In-Reply-To: <7vobgglpv4.fsf@alter.siamese.dyndns.org>
On Tue, Jan 22, 2013 at 04:11:59PM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>> Would you mind holding off on this? As it stands there are a couple of
>> issues with the cvsimport-3 script including: ...
>
> Actually I do. I think this, at least the early part of it, should
> be merged to 'next' as soon as possible, *unless*
>
> (1) The cvsimport-2 & cvsps2 combo this series ships gives worse
> experience than cvsimport we ship in v1.8.1 to end users of the
> current cvsimport with cvsps2; and/or
>
> (2) The cvsimport-3 in this series, which is a copy of an older
> version of what Eric has, is so broken that we are better off
> starting cvsimport-3 by getting a fresh copy from Eric which
> has been rewritten in a major way, than applying huge
> incremental update patches that amounts to a total rewrite.
>
> The point (1) is important from "no regression" point of view, and
> in a sense more important between the two because it is the first
> step in the overall transition plan.
>
> Even though there may be remaining issues in cvsimport-3 and cvsps3
> (what new piece of software don't have issues?), my limited
> observation of the exchanges between you and Eric suggests me that
> the problem is not something that requires a total rewrite of how
> cvsimport-3 works, so I do not expect the point (2) to be true,
> either, but if I am mistaken, please let me know.
ESR's cvsimport.py in the cvsps repository has no fixes over what's
here. I think his comment in [1] indicates that he won't do any more
work on git-cvsimport.
[1] http://article.gmane.org/gmane.comp.version-control.git/214057
In my opinion the incremental import support really is substantially
worse in cvsimport-3 than cvsimport-2. cvsimport-2 looks at the output
of git-for-each-ref to calculate the dates from which to continue each
branch. cvsps cannot be told this information and so the cvsimport-3
script just takes the date of the last commit on the current branch.
On top of that, the incremental switch to cvsps-3 just causes it to
output:
from: refs/heads/branch^0
on the first commit for each branch, which I can't see working if a new
branch is created in CVS.
> By advancing the topic to 'next', we will give people a more solid
> (read: not getting rewound) foundation to work with than "if you are
> really interested, grab the tip of 'pu', replace it with even newer
> copy from Eric's repository and try it out", so that more people can
> help us polish the scaffolding to let us ship two versions and also
> find issues in the new cvsimport-3 and help fixing them. At least,
> that is what I've been hoping.
That's what I've done and it's convinced me that cvsps-3 is not ready
for use with incremental imports as it stands.
> I could stop at the first three patches, that is, introducing the
> version switch wrapper that switches between cvsps2+cvsimport-2
> combo and nothing, and then let you and Eric redo the "start adding
> cvsps 3.x support" and later patches when cvsimport-3 is ready.
> That would give you a larger lattitude to rework cvsimport-3. Is
> that preferrable?
My preference would be for something like this, possibly with an
expanded examples section showing how to pipe the output of cvsps-3 or
cvs2git into git-fast-import:
-- >8 --
diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
index 9d5353e..20b846e 100644
--- a/Documentation/git-cvsimport.txt
+++ b/Documentation/git-cvsimport.txt
@@ -18,6 +18,11 @@ SYNOPSIS
DESCRIPTION
-----------
+*WARNING:* `git cvsimport` uses cvsps version 2, which is considered
+deprecated; it does not work with cvsps version 3 and later. If you are
+performing a one-shot import of a CVS repository consider using cvsps-3,
+cvs2git or parsecvs directly.
+
Imports a CVS repository into git. It will either create a new
repository, or incrementally import into an existing one.
-- 8< --
John
^ permalink raw reply related
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