Git development
 help / color / mirror / Atom feed
* 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] 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 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

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

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

* 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

* Re: [PATCH v3 3/6] Change 'git' to 'Git' whenever the whole system is referred to #2
From: David Aguilar @ 2013-01-23  3:44 UTC (permalink / raw)
  To: Thomas Ackermann; +Cc: gitster, git
In-Reply-To: <2009548606.632825.1358795980319.JavaMail.ngmail@webmail20.arcor-online.net>

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

^ permalink raw reply

* Re: [PATCH v3 5/6] Change 'git' to 'Git' whenever the whole system is referred to #4
From: Junio C Hamano @ 2013-01-23  3:16 UTC (permalink / raw)
  To: Thomas Ackermann; +Cc: davvid, git
In-Reply-To: <1199035912.632874.1358796172804.JavaMail.ngmail@webmail20.arcor-online.net>

Leftover in paths touched by [5/6].

 Documentation/gitcvs-migration.txt          | 2 +-
 Documentation/technical/api-builtin.txt     | 2 +-
 Documentation/technical/api-credentials.txt | 4 ++--
 Documentation/technical/api-remote.txt      | 2 +-
 Documentation/urls.txt                      | 2 +-
 Documentation/user-manual.txt               | 4 ++--
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/gitcvs-migration.txt b/Documentation/gitcvs-migration.txt
index 2934ac2..5ab5b07 100644
--- a/Documentation/gitcvs-migration.txt
+++ b/Documentation/gitcvs-migration.txt
@@ -3,7 +3,7 @@ gitcvs-migration(7)
 
 NAME
 ----
-gitcvs-migration - git for CVS users
+gitcvs-migration - Git for CVS users
 
 SYNOPSIS
 --------
diff --git a/Documentation/technical/api-builtin.txt b/Documentation/technical/api-builtin.txt
index 2d27ff1..4a4228b 100644
--- a/Documentation/technical/api-builtin.txt
+++ b/Documentation/technical/api-builtin.txt
@@ -5,7 +5,7 @@ Adding a new built-in
 ---------------------
 
 There are 4 things to do to add a built-in command implementation to
-git:
+Git:
 
 . Define the implementation of the built-in command `foo` with
   signature:
diff --git a/Documentation/technical/api-credentials.txt b/Documentation/technical/api-credentials.txt
index f0c39e1..516fda7 100644
--- a/Documentation/technical/api-credentials.txt
+++ b/Documentation/technical/api-credentials.txt
@@ -18,7 +18,7 @@ Typical setup
 
 ------------
 +-----------------------+
-| git code (C)          |--- to server requiring --->
+| Git code (C)          |--- to server requiring --->
 |                       |        authentication
 |.......................|
 | C credential API      |--- prompt ---> User
@@ -27,7 +27,7 @@ Typical setup
 	| pipe |
 	|      v
 +-----------------------+
-| git credential helper |
+| Git credential helper |
 +-----------------------+
 ------------
 
diff --git a/Documentation/technical/api-remote.txt b/Documentation/technical/api-remote.txt
index 2819d3a..4be8776 100644
--- a/Documentation/technical/api-remote.txt
+++ b/Documentation/technical/api-remote.txt
@@ -45,7 +45,7 @@ struct remote
 `receivepack`, `uploadpack`::
 
 	The configured helper programs to run on the remote side, for
-	git-native protocols.
+	Git-native protocols.
 
 `http_proxy`::
 
diff --git a/Documentation/urls.txt b/Documentation/urls.txt
index a2cf68b..539c0a0 100644
--- a/Documentation/urls.txt
+++ b/Documentation/urls.txt
@@ -29,7 +29,7 @@ The ssh and git protocols additionally support ~username expansion:
 - git://host.xz{startsb}:port{endsb}/~{startsb}user{endsb}/path/to/repo.git/
 - {startsb}user@{endsb}host.xz:/~{startsb}user{endsb}/path/to/repo.git/
 
-For local repositories, also supported by git natively, the following
+For local repositories, also supported by Git natively, the following
 syntaxes may be used:
 
 - /path/to/repo.git/
diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index dda262a..5077e7c 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -1439,7 +1439,7 @@ fundamentally different ways to fix the problem:
 
 	2. You can go back and modify the old commit.  You should
 	never do this if you have already made the history public;
-	git does not normally expect the "history" of a project to
+	Git does not normally expect the "history" of a project to
 	change, and cannot correctly perform repeated merges from
 	a branch that has had its history changed.
 
@@ -3671,7 +3671,7 @@ Unable to checkout '261dfac35cb99d380eb966e102c1197139f7fa24' in submodule path
 
 In older Git versions it could be easily forgotten to commit new or modified
 files in a submodule, which silently leads to similar problems as not pushing
-the submodule changes. Starting with git 1.7.0 both "git status" and "git diff"
+the submodule changes. Starting with Git 1.7.0 both "git status" and "git diff"
 in the superproject show submodules as modified when they contain new or
 modified files to protect against accidentally committing such a state. "git
 diff" will also add a "-dirty" to the work tree side when generating patch
-- 
1.8.1.1.507.g1754052

^ permalink raw reply related

* Re: [PATCH v3 0/2] Make git-svn work with gitdir links
From: Junio C Hamano @ 2013-01-23  2:53 UTC (permalink / raw)
  To: Eric Wong; +Cc: Barry Wardell, git
In-Reply-To: <20130123023235.GA24135@dcvr.yhbt.net>

Eric Wong <normalperson@yhbt.net> writes:

> `git rev-parse --show-cdup` outputs nothing if GIT_DIR is set,
> so I unset GIT_DIR temporarily.
>
> I'm not sure why --show-cdup behaves like this, though..

Setting GIT_DIR is to say "That is the directory that has the
repository objects and refs; I am letting you know the location
explicitly because it does not have any relation with the location
of the working tree.  The $(cwd) is at the root of the working
tree".

If you want to say "That is the directory that has metainformation,
and that other one is the root of the working tree", you use
GIT_WORK_TREE to name the latter.

So by definition, if you only set GIT_DIR without setting
GIT_WORK_TREE, show-cdup must say "you are already at the top".

>
> Does squashing this on top of your changes fix all your failures?
> I plan on squashing both your changes together with the below:
>
> diff --git a/git-svn.perl b/git-svn.perl
> index c232798..e5bd292 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -332,11 +332,13 @@ if ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {
>  		$ENV{GIT_DIR} = command_oneline([qw/rev-parse --git-dir/]);
>  	} "Unable to find .git directory\n";
>  	my $cdup = undef;
> +	my $git_dir = delete $ENV{GIT_DIR};
>  	git_cmd_try {
>  		$cdup = command_oneline(qw/rev-parse --show-cdup/);
>  		chomp $cdup if ($cdup);
>  		$cdup = "." unless ($cdup && length $cdup);
> -	} "Already at toplevel, but $ENV{GIT_DIR} not found\n";
> +	} "Already at toplevel, but $git_dir not found\n";
> +	$ENV{GIT_DIR} = $git_dir;
>  	chdir $cdup or die "Unable to chdir up to '$cdup'\n";
>  	$_repository = Git->repository(Repository => $ENV{GIT_DIR});
>  }

^ permalink raw reply

* Bug in EOL conversion?
From: Stefan Norgren @ 2013-01-23  2:44 UTC (permalink / raw)
  To: git

Hi,

The EOL conversion does not behave as indicated by output message from
add and commit. Here is my test case executed on Windows 7 64 bit.


$ git --version
git version 1.8.0.msysgit.0
$ which git
/cygdrive/c/Program Files (x86)/Git/cmd/git
$ git config --list
core.symlinks=false
core.autocrlf=true
color.diff=auto
color.status=auto
color.branch=auto
color.interactive=true
pack.packsizelimit=2g
help.format=html
http.sslcainfo=/bin/curl-ca-bundle.crt
sendemail.smtpserver=/bin/msmtp.exe
diff.astextplain.textconv=astextplain
rebase.autosquash=true
user.name=Stefan
user.email=stefan@---.com
core.repositoryformatversion=0
core.filemode=false
core.bare=false
core.logallrefupdates=true
core.symlinks=false
core.ignorecase=true
core.hidedotfiles=dotGitOnly

-- Note core.autocrlf=true.
-- Created withcrlf.txt with one character and one CRLF line feed.
File size 3 bytes.
-- Created withlf.txt with one character and one LF line feed. File
size 2 bytes.
-- Now let's init repository.

$ git init
Initialized empty Git repository in D:/Dev/workspaces/gittest/.git/
$ ls -la
total 10
d---------+ 1 Stefan None 0 Jan 23 02:12 .
d---------+ 1 Stefan None 0 Jan 23 02:10 ..
d---------+ 1 Stefan None 0 Jan 23 02:13 .git
----------+ 1 Stefan None 3 Jan 23 01:55 withcrlf.txt
----------+ 1 Stefan None 2 Jan 23 01:55 withlf.txt

-- Note no .gitattributes file that will affect EOL conversion.

$ ls -la .git/info/
total 5
d---------+ 1 Stefan None   0 Jan 23 02:12 .
d---------+ 1 Stefan None   0 Jan 23 02:13 ..
----------+ 1 Stefan None 240 Jan 23 02:12 exclude

-- Note no attribute file in .git/info/ that will affect EOL conversion.

$ git add *
warning: LF will be replaced by CRLF in withlf.txt.
The file will have its original line endings in your working directory.
$ git commit -m 'Testing EOL'
[master (root-commit) 9a0b2f5] Testing EOL
 2 files changed, 2 insertions(+)
 create mode 100644 withcrlf.txt
 create mode 100644 withlf.txt
warning: LF will be replaced by CRLF in withlf.txt.
The file will have its original line endings in your working directory.
$ ls -la
total 10
d---------+ 1 Stefan None 0 Jan 23 02:12 .
d---------+ 1 Stefan None 0 Jan 23 02:10 ..
d---------+ 1 Stefan None 0 Jan 23 02:22 .git
----------+ 1 Stefan None 3 Jan 23 01:55 withcrlf.txt
----------+ 1 Stefan None 2 Jan 23 01:55 withlf.txt

-- So no changes (see file size) to files in working directory. This
is expected and according to output warning from add and commit.

-- Now lets look in the repository

$ git ls-tree -l HEAD withcrlf.txt
100644 blob d00491fd7e5bb6fa28c517a0bb32b8b506539d4d       2    withcrlf.txt
$ git ls-tree -l HEAD withlf.txt
100644 blob d00491fd7e5bb6fa28c517a0bb32b8b506539d4d       2    withlf.txt

-- Note that size of withlf.txt is 2 in repository indicating that LF
was not replaced by CRLF in withlf.txt as indicated in output from add
and commit. Also note that size of withcrlf.txt is also 2 in
repository so it looks like CRLF was replaced by LF in withcrlf.txt.
To verify I will delete the files from working directory, turn off EOL
conversion, checkout files and look at file endings in the working
directory.

$ rm with*
$ ls -la
total 8
d---------+ 1 Stefan None 0 Jan 23 02:31 .
d---------+ 1 Stefan None 0 Jan 23 02:10 ..
d---------+ 1 Stefan None 0 Jan 23 02:22 .git
$ git status
# On branch master
# Changes not staged for commit:
#   (use "git add/rm <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#       deleted:    withcrlf.txt
#       deleted:    withlf.txt
#
no changes added to commit (use "git add" and/or "git commit -a")
$ git config --global core.autocrlf false
$ git config --global core.autocrlf
false
$ git checkout -- with*
$ ls -la
total 10
d---------+ 1 Stefan None 0 Jan 23 02:38 .
d---------+ 1 Stefan None 0 Jan 23 02:10 ..
d---------+ 1 Stefan None 0 Jan 23 02:38 .git
----------+ 1 Stefan None 2 Jan 23 02:38 withcrlf.txt
----------+ 1 Stefan None 2 Jan 23 02:38 withlf.txt

-- Both files in working directory files now have LF line endings
confirming that files in repository have LF file endings. Either the
output message of add and commit is wrong or the behavior of the EOL
conversion is wrong... or... have I missed something...?

   /Stefan

^ permalink raw reply

* Re: [PATCH v3 4/6] Change 'git' to 'Git' whenever the whole system is referred to #3
From: Junio C Hamano @ 2013-01-23  2:49 UTC (permalink / raw)
  To: Thomas Ackermann; +Cc: davvid, git
In-Reply-To: <1667947659.632844.1358796098848.JavaMail.ngmail@webmail20.arcor-online.net>

 Documentation/gitcore-tutorial.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/gitcore-tutorial.txt b/Documentation/gitcore-tutorial.txt
index 5d957c2..59c1c17 100644
--- a/Documentation/gitcore-tutorial.txt
+++ b/Documentation/gitcore-tutorial.txt
@@ -1068,7 +1068,7 @@ and requires you to have a log-in privilege over `ssh` to the
 remote machine.  It finds out the set of objects the other side
 lacks by exchanging the head commits both ends have and
 transfers (close to) minimum set of objects.  It is by far the
-most efficient way to exchange git objects between repositories.
+most efficient way to exchange Git objects between repositories.
 
 Local directory::
 	`/path/to/repo.git/`
@@ -1077,7 +1077,7 @@ This transport is the same as SSH transport but uses 'sh' to run
 both ends on the local machine instead of running other end on
 the remote machine via 'ssh'.
 
-git Native::
+Git Native::
 	`git://remote.machine/path/to/repo.git/`
 +
 This transport was designed for anonymous downloading.  Like SSH
-- 
1.8.1.1.507.g1754052

^ permalink raw reply related

* Re: [PATCH v3 3/6] Change 'git' to 'Git' whenever the whole system is referred to #2
From: Junio C Hamano @ 2013-01-23  2:48 UTC (permalink / raw)
  To: Thomas Ackermann; +Cc: davvid, git
In-Reply-To: <7va9s0lnxn.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> As before, I'll locally create a fixup commit based on the above.
>
> After I am done with these patches, somebody (not Thomas or I, as
> our eyes are too used to staring at git and Git to notice mistakes)
> has to run "git grep -C3 -e '\<Git\>' Documentation/" and eyeball
> the output to spot mistakes that should have been "git" but was
> converted to "Git", as I am mostly concentrating on finding "git"
> that should have been converted in Thomas's series.

 Documentation/config.txt             | 4 ++--
 Documentation/git-remote-helpers.txt | 2 +-
 Documentation/git-verify-pack.txt    | 2 +-
 Documentation/gitattributes.txt      | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5a831ad2..3652ee0 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2,7 +2,7 @@ CONFIGURATION FILE
 ------------------
 
 The Git configuration file contains a number of variables that affect
-the git command's behavior. The `.git/config` file in each repository
+the Git commands' behavior. The `.git/config` file in each repository
 is used to store the configuration for that repository, and
 `$HOME/.gitconfig` is used to store a per-user configuration as
 fallback values for the `.git/config` file. The file `/etc/gitconfig`
@@ -999,7 +999,7 @@ fetch.fsckObjects::
 	is used instead.
 
 fetch.unpackLimit::
-	If the number of objects fetched over the git native
+	If the number of objects fetched over the Git native
 	transfer is below this
 	limit, then the objects will be unpacked into loose object
 	files. However if the number of received objects equals or
diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
index e18c3b0..e36fdcb 100644
--- a/Documentation/git-remote-helpers.txt
+++ b/Documentation/git-remote-helpers.txt
@@ -39,7 +39,7 @@ INVOCATION
 ----------
 
 Remote helper programs are invoked with one or (optionally) two
-arguments. The first argument specifies a remote repository as in git;
+arguments. The first argument specifies a remote repository as in Git;
 it is either the name of a configured remote or a URL. The second
 argument specifies a URL; it is usually of the form
 '<transport>://<address>', but any arbitrary string is possible.
diff --git a/Documentation/git-verify-pack.txt b/Documentation/git-verify-pack.txt
index d4ae05b..0eb9ffb 100644
--- a/Documentation/git-verify-pack.txt
+++ b/Documentation/git-verify-pack.txt
@@ -3,7 +3,7 @@ git-verify-pack(1)
 
 NAME
 ----
-git-verify-pack - Validate packed git archive files
+git-verify-pack - Validate packed Git archive files
 
 
 SYNOPSIS
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index b9c0eec..b322a26 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -449,7 +449,7 @@ String::
 	specify one or more options, as described in the following
 	section. The options for the diff driver "foo" are defined
 	by the configuration variables in the "diff.foo" section of the
-	git config file.
+	Git config file.
 
 
 Defining an external diff driver
@@ -675,7 +675,7 @@ attribute in the `.gitattributes` file:
 *.ps -diff
 ------------------------
 
-This will cause git to generate `Binary files differ` (or a binary
+This will cause Git to generate `Binary files differ` (or a binary
 patch, if binary patches are enabled) instead of a regular diff.
 
 However, one may also want to specify other diff driver attributes. For
-- 
1.8.1.1.507.g1754052

^ permalink raw reply related

* Re: [PATCH v3 2/6] Change 'git' to 'Git' whenever the whole system is referred to #1
From: Junio C Hamano @ 2013-01-23  2:32 UTC (permalink / raw)
  To: Thomas Ackermann; +Cc: davvid, git
In-Reply-To: <7vfw1slpig.fsf@alter.siamese.dyndns.org>

In Documentation/git-rev-parse.txt, there is this bit:

 --resolve-git-dir <path>::
        Check if <path> is a valid git-dir or a git-file pointing to
        a valid git-dir. If <path> is a valid git-dir the resolved path to
        git-dir will be printed.

I think the author invented the word "git-dir" to mean what we
ordinarily call $GIT_DIR (i.e. the directory that contains the
repository data), and used "git-file" to mean what the code and
error messages call "gitfile".  I think it is better to leave these
in lowercase, but we would need them in the glossary, probably after
rewriting the latter to "gitfile".  The former may want to be
rewritten to "gitdir" or even "$GIT_DIR".

^ permalink raw reply

* Re: [PATCH v3 0/2] Make git-svn work with gitdir links
From: Eric Wong @ 2013-01-23  2:32 UTC (permalink / raw)
  To: Barry Wardell; +Cc: git, Junio C Hamano
In-Reply-To: <1358731322-44600-1-git-send-email-barry.wardell@gmail.com>

Barry Wardell <barry.wardell@gmail.com> wrote:
> These patches fix a bug which prevented git-svn from working with repositories
> which use gitdir links.
> 
> Changes since v2:
>  - Rebased onto latest master.
>  - Added test case which verifies that the problem has been fixed.
>  - Fixed problems with git svn (init|clone|multi-init).
>  - All git-svn test cases now pass (except two in t9101 which also failed
>    before these patches).

t9101 did not fail for me before your patches.  However I have a
patch on top of your 2/2 which should fix things.

`git rev-parse --show-cdup` outputs nothing if GIT_DIR is set,
so I unset GIT_DIR temporarily.

I'm not sure why --show-cdup behaves like this, though..

Does squashing this on top of your changes fix all your failures?
I plan on squashing both your changes together with the below:

diff --git a/git-svn.perl b/git-svn.perl
index c232798..e5bd292 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -332,11 +332,13 @@ if ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {
 		$ENV{GIT_DIR} = command_oneline([qw/rev-parse --git-dir/]);
 	} "Unable to find .git directory\n";
 	my $cdup = undef;
+	my $git_dir = delete $ENV{GIT_DIR};
 	git_cmd_try {
 		$cdup = command_oneline(qw/rev-parse --show-cdup/);
 		chomp $cdup if ($cdup);
 		$cdup = "." unless ($cdup && length $cdup);
-	} "Already at toplevel, but $ENV{GIT_DIR} not found\n";
+	} "Already at toplevel, but $git_dir not found\n";
+	$ENV{GIT_DIR} = $git_dir;
 	chdir $cdup or die "Unable to chdir up to '$cdup'\n";
 	$_repository = Git->repository(Repository => $ENV{GIT_DIR});
 }

^ permalink raw reply related

* Re: [PATCH v3 2/6] Change 'git' to 'Git' whenever the whole system is referred to #1
From: Junio C Hamano @ 2013-01-23  2:24 UTC (permalink / raw)
  To: Thomas Ackermann; +Cc: davvid, git
In-Reply-To: <7vfw1slpig.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> This is the summary of the review comments in a patch form.
>

And then there still are the ones in the file that was touched by
Thomas's 2/6 that need fixing.

 Documentation/git-apply.txt          | 2 +-
 Documentation/git-archimport.txt     | 2 +-
 Documentation/git-cvsserver.txt      | 8 ++++----
 Documentation/git-help.txt           | 2 +-
 Documentation/git-mergetool--lib.txt | 2 +-
 Documentation/git-p4.txt             | 4 ++--
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index c1704dc..f605327 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -198,7 +198,7 @@ behavior:
 * `fix` outputs warnings for a few such errors, and applies the
   patch after fixing them (`strip` is a synonym --- the tool
   used to consider only trailing whitespace characters as errors, and the
-  fix involved 'stripping' them, but modern gits do more).
+  fix involved 'stripping' them, but modern Gits do more).
 * `error` outputs warnings for a few such errors, and refuses
   to apply the patch.
 * `error-all` is similar to `error` but shows all errors.
diff --git a/Documentation/git-archimport.txt b/Documentation/git-archimport.txt
index 25b1cab..163b9f6 100644
--- a/Documentation/git-archimport.txt
+++ b/Documentation/git-archimport.txt
@@ -3,7 +3,7 @@ git-archimport(1)
 
 NAME
 ----
-git-archimport - Import an Arch repository into git
+git-archimport - Import an Arch repository into Git
 
 
 SYNOPSIS
diff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt
index dc5069f..7ca64b3 100644
--- a/Documentation/git-cvsserver.txt
+++ b/Documentation/git-cvsserver.txt
@@ -3,7 +3,7 @@ git-cvsserver(1)
 
 NAME
 ----
-git-cvsserver - A CVS server emulator for git
+git-cvsserver - A CVS server emulator for Git
 
 SYNOPSIS
 --------
@@ -291,14 +291,14 @@ Variable substitution
 In `dbdriver` and `dbuser` you can use the following variables:
 
 %G::
-	git directory name
+	Git directory name
 %g::
-	git directory name, where all characters except for
+	Git directory name, where all characters except for
 	alpha-numeric ones, `.`, and `-` are replaced with
 	`_` (this should make it easier to use the directory
 	name in a filename if wanted)
 %m::
-	CVS module/git head name
+	CVS module/Git head name
 %a::
 	access method (one of "ext" or "pserver")
 %u::
diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 835fba0..e07b6dc 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -3,7 +3,7 @@ git-help(1)
 
 NAME
 ----
-git-help - display help information about git
+git-help - Display help information about Git
 
 SYNOPSIS
 --------
diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt
index b748bdf..055550b 100644
--- a/Documentation/git-mergetool--lib.txt
+++ b/Documentation/git-mergetool--lib.txt
@@ -3,7 +3,7 @@ git-mergetool{litdd}lib(1)
 
 NAME
 ----
-git-mergetool--lib - Common git merge tool shell scriptlets
+git-mergetool--lib - Common Git merge tool shell scriptlets
 
 SYNOPSIS
 --------
diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 96a5b31..1d1b020 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -64,7 +64,7 @@ COMMANDS
 
 Clone
 ~~~~~
-Generally, 'git p4 clone' is used to create a new git directory
+Generally, 'git p4 clone' is used to create a new Git directory
 from an existing p4 repository:
 ------------
 $ git p4 clone //depot/path/project
@@ -494,7 +494,7 @@ git-p4.detectCopiesHarder::
 	Detect copies harder.  See linkgit:git-diff[1].  A boolean.
 
 git-p4.preserveUser::
-	On submit, re-author changes to reflect the git author,
+	On submit, re-author changes to reflect the Git author,
 	regardless of who invokes 'git p4 submit'.
 
 git-p4.allowMissingP4Users::

^ permalink raw reply related

* Re: [PATCH v3 01/10] wildmatch: fix "**" special case
From: Duy Nguyen @ 2013-01-23  1:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7v1udcn84w.fsf@alter.siamese.dyndns.org>

On Wed, Jan 23, 2013 at 5:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> We obviously do not want to set FNM_PATHNAME when we are not
>> substituting fnmatch() with wildmatch(), but I wonder if it may make
>> sense to unconditionally use WM_PATHNAME semantics when we build the
>> system with USE_WILDMATCH and calling wildmatch() in this codepath.
>> Users can always use "*/**/*" in place of "*" in their patterns
>> where they want to ignore directory boundaries.

If we do that, we need to do the same in tree_entry_interesting(). In
other words, pathspec learns the new glob syntax. It's fine for an
experimental flag like USE_WILDMATCH. But after fnmatch is replaced by
wildmatch unconditionally (thus USE_WILDMATCH becomes obsolete), then
what? Should people who expects new syntax with USE_WILDMATCH continue
to have new syntax? How does a user know which syntax may be used in
his friend's "git" binary?

On a related subject, I've been meaning to write a mail about the
other use of patterns in git (e.g. in git-branch, git-tag,
git-apply...) but never got around to. Some use without FNM_PATHNAME,
some do and the document does not go into details about the
differences. We might want to unify the syntax there too. It'll
probably break backward compatibility so we can do the same when we
switch pathspec syntax.

> Another possibility, which _might_ make more practical sense, is to
> update dowild() so that any pattern that has (^|/)**(/|$) in it
> implicitly turns the WM_PATHNAME flag on.  There is no reason for
> the user to feed it a double-asterisk unless it is an attempt to
> defeat some directory boundaries,

They may also put "**" by mistake (or realize they just put "**" but
too lazy to remove one asterisk).

> so we already know that the user
> expects WM_PATHNAME behaviour at that point.  Otherwise, the user
> would have simply said '*' instead, wouldn't he?

The only problem I see is, without the version string, there's no way
to know if "**" is supported. Old git versions will happily take "**"
and interpret as "*". When you advise someone to use "**" you might
need to add "check if you have this version of git". This problem does
not exist with pathspec magic like :(glob)

> I said "_might_" because it sounds a bit too magical to my taste.
> I dunno.
-- 
Duy

^ permalink raw reply

* Re: [PATCH v3 3/6] Change 'git' to 'Git' whenever the whole system is referred to #2
From: Junio C Hamano @ 2013-01-23  0:53 UTC (permalink / raw)
  To: Thomas Ackermann; +Cc: davvid, git
In-Reply-To: <2009548606.632825.1358795980319.JavaMail.ngmail@webmail20.arcor-online.net>

Thomas Ackermann <th.acker@arcor.de> writes:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index b87f744..5a831ad2 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1,14 +1,14 @@
>  CONFIGURATION FILE
>  ------------------
>  
> -The git configuration file contains a number of variables that affect
> +The Git configuration file contains a number of variables that affect
>  the git command's behavior. The `.git/config` file in each repository

I am torn on this one, in the sense that there are configuration
variables that changes the behaviour of 'git' wrapper itself, but
this explanation is talking more about all the 161 subcommands (no,
I am just kidding and didn't count) in the Git suite in general, so
I think s/git/Git/ is probably more appropriate.

> @@ -1039,7 +1039,7 @@ format.subjectprefix::
>  
>  format.signature::
>  	The default for format-patch is to output a signature containing
> -	the git version number. Use this variable to change that default.
> +	the Git version number. Use this variable to change that default.
>  	Set this variable to the empty string ("") to suppress
>  	signature generation.

Before this hunk, fetch.unpackLimit has this:

	If the number of objects fetched over the git native
	transfer is below...

s/git/Git/ is needed, I think.


> diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
> index 6d696e0..e18c3b0 100644
> --- a/Documentation/git-remote-helpers.txt
> +++ b/Documentation/git-remote-helpers.txt
> @@ -45,9 +45,9 @@ argument specifies a URL; it is usually of the form
>  '<transport>://<address>', but any arbitrary string is possible.
>  The 'GIT_DIR' environment variable is set up for the remote helper
>  and can be used to determine where to store additional data or from
> -which directory to invoke auxiliary git commands.
> +which directory to invoke auxiliary Git commands.

The paragraph begins with this:

 Remote helper programs are invoked with one or (optionally) two
 arguments. The first argument specifies a remote repository as in git;

s/git/Git/ is needed, I think.


> diff --git a/Documentation/git-verify-pack.txt b/Documentation/git-verify-pack.txt
> index cd23076..d4ae05b 100644
> --- a/Documentation/git-verify-pack.txt
> +++ b/Documentation/git-verify-pack.txt
> @@ -14,7 +14,7 @@ SYNOPSIS
>  
>  DESCRIPTION
>  -----------
> -Reads given idx file for packed git archive created with the
> +Reads given idx file for packed Git archive created with the
>  'git pack-objects' command and verifies idx file and the
>  corresponding pack file.

The NAME section lists it as "Validate packed git archive files",
which wants a s/git/Git/, I think.

> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index 2698f63..b9c0eec 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -417,11 +417,11 @@ Generating diff text
>  `diff`
>  ^^^^^^
>  
> -The attribute `diff` affects how 'git' generates diffs for particular
> -files. It can tell git whether to generate a textual patch for the path
> +The attribute `diff` affects how Git generates diffs for particular
> +files. It can tell Git whether to generate a textual patch for the path
>  or to treat the path as a binary file.  It can also affect what line is
> -shown on the hunk header `@@ -k,l +n,m @@` line, tell git to use an
> -external command to generate the diff, or ask git to convert binary
> +shown on the hunk header `@@ -k,l +n,m @@` line, tell Git to use an
> +external command to generate the diff, or ask Git to convert binary
>  files to a text format before generating the diff.
>  
>  Set::

After this hunk, the description of "String" type says this:

	... are defined by the configuration variables in the
        "diff.foo" section of the git config file.

s/git/Git/ is needed, I think.


> @@ -650,7 +650,7 @@ advantages to choosing this method:
>     odt2txt).
>  
>  2. Git diff features. By performing only the transformation step
> -   yourself, you can still utilize many of git's diff features,
> +   yourself, you can still utilize many of Git's diff features,
>     including colorization, word-diff, and combined diffs for merges.
>  
>  3. Caching. Textconv caching can speed up repeated diffs, such as those

After this hunk, in "Marking file as binary" section, you have

	This will cause git to generate `Binary files differ`...

s/git/Git/ is needed, I think.

As before, I'll locally create a fixup commit based on the above.

After I am done with these patches, somebody (not Thomas or I, as
our eyes are too used to staring at git and Git to notice mistakes)
has to run "git grep -C3 -e '\<Git\>' Documentation/" and eyeball
the output to spot mistakes that should have been "git" but was
converted to "Git", as I am mostly concentrating on finding "git"
that should have been converted in Thomas's series.

Thanks.

^ permalink raw reply

* Re: [PATCH] all: new command used for multi-repo operations
From: David Aguilar @ 2013-01-23  0:43 UTC (permalink / raw)
  To: Lars Hjemli; +Cc: git
In-Reply-To: <1358889019-4554-1-git-send-email-hjemli@gmail.com>

On Tue, Jan 22, 2013 at 1:10 PM, 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.
>
> 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>
> ---
>  Documentation/git-all.txt |  37 ++++++++++++++++
>  Makefile                  |   1 +
>  builtin.h                 |   1 +
>  builtin/all.c             | 105 ++++++++++++++++++++++++++++++++++++++++++++++
>  command-list.txt          |   1 +
>  git.c                     |   1 +
>  t/t0064-all.sh            |  42 +++++++++++++++++++
>  7 files changed, 188 insertions(+)
>  create mode 100644 Documentation/git-all.txt
>  create mode 100644 builtin/all.c
>  create mode 100755 t/t0064-all.sh
>
> diff --git a/Documentation/git-all.txt b/Documentation/git-all.txt
> new file mode 100644
> index 0000000..b25f23c
> --- /dev/null
> +++ b/Documentation/git-all.txt
> @@ -0,0 +1,37 @@
> +git-all(1)
> +==========
> +
> +NAME
> +----
> +git-all - Execute a git command in multiple repositories
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'git all' [--dirty|--clean] [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
> +-------
> +-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..ee9270d
> --- /dev/null
> +++ b/builtin/all.c
> @@ -0,0 +1,105 @@
> +/*
> + * "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"
> +
> +static int only_dirty;
> +static int only_clean;
> +char root[PATH_MAX];
> +
> +static const char * const builtin_all_usage[] = {
> +       N_("git all [options] [cmd]"),
> +       NULL
> +};
> +
> +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(),
> +};
> +
> +static int is_dirty()
> +{
> +       const char *diffidx[] = {"diff", "--quiet", "--cached", NULL};
> +       const char *diffwd[] = {"diff", "--quiet", NULL};
> +
> +       if (run_command_v_opt(diffidx, RUN_GIT_CMD) != 0)
> +               return 1;
> +       if (run_command_v_opt(diffwd, RUN_GIT_CMD) != 0)
> +               return 1;
> +       return 0;
> +}
> +
> +static void handle_repo(char *path, const char **argv)
> +{
> +       int dirty;
> +
> +       if (path[0] == '.' && path[1] == '/')
> +               path += 2;
> +       if (only_dirty || only_clean) {
> +               dirty = is_dirty();
> +               if ((dirty && only_clean) ||
> +                    (!dirty && only_dirty))
> +                       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;
> +       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)?  I wonder whether the check should be tighter,
e.g. something closer to what's done in setup.c:is_git_repository().

The name of this function leads me to believe that this walks
everything below the current directory looking for Git repos.  How
deep does it walk?  Does this handle nested repositories, e.g.
foo/bar/ and foo/bar/baz/ when you are inside foo/ ?  After re-reading
the documentation I am led to believe that it only walks one level
deep.

I did not notice a test for nested repos which is what sparked my
curiosity. ;-)  If we do not expect to handle them in the first
version then we should have a test to ensure the expected behavior.
It would also be nice to see a test with a .git file.

I do wonder what the end user experience is with this command when
used alongside other Git aggregate commands such as "repo" or "git
submodule".  This command is basically "git submodule foreach ..."
without needing to buy into the whole submodule thing.  This is an
argument for naming it something like "git foreach-repo" since it
would be named more closely to the "foreach" submodule command.

While "all" is less to type than "foreach-repo", most of the extra
work can be eliminated by installing the wonderful git completion
scripts for bash/zsh.

All that said, I have very real use cases for this command.  Thanks
for writing it.

> +               if (ent->d_type != DT_DIR)
> +                       continue;
> +               strbuf_setlen(path, len);
> +               strbuf_addstr(path, ent->d_name);
> +               walk(path, argc, argv);
> +       }
> +       closedir(dir);
> +       return 0;
> +}
> +
> +int cmd_all(int argc, const char **argv, const char *prefix)
> +{
> +       struct strbuf path = STRBUF_INIT;
> +
> +       if (!getcwd(root, sizeof(root)))
> +               return 1;
> +
> +       argc = parse_options(argc, argv, prefix, builtin_all_options,
> +                            builtin_all_usage, PARSE_OPT_STOP_AT_NON_OPTION);
> +
> +       unsetenv(GIT_DIR_ENVIRONMENT);
> +       unsetenv(GIT_WORK_TREE_ENVIRONMENT);
> +
> +       strbuf_addstr(&path, ".");
> +       return walk(&path, argc, argv);
> +}
> diff --git a/command-list.txt b/command-list.txt
> index 7e8cfec..f955895 100644
> --- a/command-list.txt
> +++ b/command-list.txt
> @@ -1,6 +1,7 @@
>  # List of known git commands.
>  # command name                         category [deprecated] [common]
>  git-add                                 mainporcelain common
> +git-all                                 mainporcelain
>  git-am                                  mainporcelain
>  git-annotate                            ancillaryinterrogators
>  git-apply                               plumbingmanipulators
> 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..932e374
> --- /dev/null
> +++ b/t/t0064-all.sh
> @@ -0,0 +1,42 @@
> +#!/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) &&
> +       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 "dirty-idx" >>expect &&
> +       echo "dirty-wt" >>expect &&
> +       git all | sort >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success "--dirty only includes dirty repos" '
> +       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
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
David

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox