threads / patch / 43392

patch, 2 partsRe: [PATCH 1/2] Allow '-' in config variable names

Subject: Re: [PATCH 1/2] Allow '-' in config variable names

## tl;dr

9 messages between Oct 30, 2006 and Oct 31, 2006. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Linus Torvalds· Oct 30, 2006, 16:25 UTC · lore

[PATCH 1/2] Allow '-' in config variable names

I need this in order to allow aliases of the same form as "ls-tree", "rev-parse" etc, so that I can use

	[alias]
		my-cat=--paginate cat-file -p
to add a "git my-cat" command.
Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---

This will be followed by another patch to "builtin-push.c" to allow me to make a useful (to me) "push-all" alias.

Show changes to config.c +8 −3
diff --git a/config.c b/config.c
index e8f0caf..3cae390 100644
--- a/config.c
+++ b/config.c
@@ -103,6 +103,11 @@ static char *parse_value(void)
 	}
 }
 
+static inline int iskeychar(int c)
+{
+	return isalnum(c) || c == '-';
+}
+
 static int get_value(config_fn_t fn, char *name, unsigned int len)
 {
 	int c;
@@ -113,7 +118,7 @@ static int get_value(config_fn_t fn, cha
 		c = get_next_char();
 		if (c == EOF)
 			break;
-		if (!isalnum(c))
+		if (!iskeychar(c))
 			break;
 		name[len++] = tolower(c);
 		if (len >= MAXNAME)
@@ -181,7 +186,7 @@ static int get_base_var(char *name)
 			return baselen;
 		if (isspace(c))
 			return get_extended_base_var(name, baselen, c);
-		if (!isalnum(c) && c != '.')
+		if (!iskeychar(c) && c != '.')
 			return -1;
 		if (baselen > MAXNAME / 2)
 			return -1;
@@ -573,7 +578,7 @@ int git_config_set_multivar(const char*
 			dot = 1;
 		/* Leave the extended basename untouched.. */
 		if (!dot || i > store.baselen) {
-			if (!isalnum(c) || (i == store.baselen+1 && !isalpha(c))) {
+			if (!iskeychar(c) || (i == store.baselen+1 && !isalpha(c))) {
 				fprintf(stderr, "invalid key: %s\n", key);
 				free(store.key);
Linus Torvalds· Oct 30, 2006, 16:28 UTC · re: Linus Torvalds · lore

[PATCH 2/2] git push: add verbose flag and allow overriding of default target repository

This adds a command line flag "-v" to enable a more verbose mode, and "--repo=" to override the default target repository for "git push" (which otherwise always defaults to "origin").

This, together with the patch to allow dashes in config variable names, allows me to do

	[alias]
		push-all = push -v --repo=all

in my user-global config file, and then I can (for any project I maintain) add to the project-local config file

	[remote "all"]
		url=one.target.repo:/directory
		url=another.target:/pub/somewhere/else

and now "git push-all" just updates all the target repositories, and shows me what it does - regardless of which repo I am in.

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---

Maybe this is just useful to me? I dunno. I have long had a per-repository "push-all" script that I have in the root directory of the repos I maintain, but this is much more practical. With this, I can do things like

	git push-all --tags

and it will push all the new tags to all the public repositories I maintain for that particular repository.

Special Linus-only behaviour? Maybe. On the other hand, I think this is a pretty clean patch regardless.

Show changes to builtin-push.c +11 −1
diff --git a/builtin-push.c b/builtin-push.c
index f5150ed..3151fb7 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -10,7 +10,7 @@
 
 static const char push_usage[] = "git-push [--all] [--tags] [-f | --force] <repository> [<refspec>...]";
 
-static int all, tags, force, thin = 1;
+static int all, tags, force, thin = 1, verbose;
 static const char *execute;
 
 #define BUF_SIZE (2084)
@@ -248,6 +248,8 @@ static int do_push(const char *repo)
 		while (dest_refspec_nr--)
 			argv[dest_argc++] = *dest_refspec++;
 		argv[dest_argc] = NULL;
+		if (verbose)
+			fprintf(stderr, "Pushing to %s\n", dest);
 		err = run_command_v(argc, argv);
 		if (!err)
 			continue;
@@ -281,6 +283,14 @@ int cmd_push(int argc, const char **argv
 			i++;
 			break;
 		}
+		if (!strcmp(arg, "-v")) {
+			verbose=1;
+			continue;
+		}
+		if (!strncmp(arg, "--repo=", 7)) {
+			repo = arg+7;
+			continue;
+		}
 		if (!strcmp(arg, "--all")) {
 			all = 1;
Junio C Hamano· Oct 30, 2006, 23:02 UTC · re: Linus Torvalds · lore

Re: [PATCH 2/2] git push: add verbose flag and allow overriding of default target repository

Linus Torvalds <torvalds@osdl.org> writes:
Show 24 quoted lines
> This adds a command line flag "-v" to enable a more verbose mode, and 
> "--repo=" to override the default target repository for "git push" (which 
> otherwise always defaults to "origin").
>
> This, together with the patch to allow dashes in config variable names, 
> allows me to do
>
> 	[alias]
> 		push-all = push -v --repo=all
>
> in my user-global config file, and then I can (for any project I maintain) 
> add to the project-local config file
>
> 	[remote "all"]
> 		url=one.target.repo:/directory
> 		url=another.target:/pub/somewhere/else
>
> and now "git push-all" just updates all the target repositories, and shows 
> me what it does - regardless of which repo I am in.
>
> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
> ---
>
> Maybe this is just useful to me?  I dunno.

I would say if something is useful to you it is useful to your subsystem people and anybody who has a public tree and more than one machines to verify the tips of his branches on, including me.

Junio C Hamano· Oct 31, 2006, 03:09 UTC · re: Linus Torvalds · lore

Re: [PATCH 2/2] git push: add verbose flag and allow overriding of default target repository

Linus Torvalds <torvalds@osdl.org> writes:
Show 13 quoted lines
> diff --git a/builtin-push.c b/builtin-push.c
> index f5150ed..3151fb7 100644
> --- a/builtin-push.c
> +++ b/builtin-push.c
> @@ -10,7 +10,7 @@
>  
>  static const char push_usage[] = "git-push [--all] [--tags] [-f | --force] <repository> [<refspec>...]";
>  
> -static int all, tags, force, thin = 1;
> +static int all, tags, force, thin = 1, verbose;
>  static const char *execute;
>  
>  #define BUF_SIZE (2084)

By the way, has anybody noticed we use a funny value for the buffer size?

Junio C Hamano· Oct 30, 2006, 22:45 UTC · re: Linus Torvalds · lore
Linus Torvalds <torvalds@osdl.org> writes:
Show 7 quoted lines
> I need this in order to allow aliases of the same form as "ls-tree", 
> "rev-parse" etc, so that I can use
>
> 	[alias]
> 		my-cat=--paginate cat-file -p
>
> to add a "git my-cat" command.

I do not have problem with this (and would perhaps also want to add '_' to keychar set), but people who envisioned parsing config from scripts (i.e. Perly git) might prefer if we stayed within alnum, since I'd suspect then they may be able to reuse existing .ini parsers. I do not much care about that myself, but I am bringing it up just in case other people might.

Other than that, this sounds nice.

By the way, everybody seems to do "alias.xxx = -p cat-file -p" (I have it as "git less"). Maybe we would want to make a built-in alias for that?

Junio C Hamano· Oct 31, 2006, 03:02 UTC · re: Junio C Hamano · lore
Junio C Hamano <junkio@cox.net> writes:
Show 18 quoted lines
> Linus Torvalds <torvalds@osdl.org> writes:
>
>> I need this in order to allow aliases of the same form as "ls-tree", 
>> "rev-parse" etc, so that I can use
>>
>> 	[alias]
>> 		my-cat=--paginate cat-file -p
>>
>> to add a "git my-cat" command.
>
> I do not have problem with this (and would perhaps also want to
> add '_' to keychar set), but people who envisioned parsing
> config from scripts (i.e. Perly git) might prefer if we stayed
> within alnum, since I'd suspect then they may be able to reuse
> existing .ini parsers.  I do not much care about that myself,
> but I am bringing it up just in case other people might.
>
> Other than that, this sounds nice.

One thing I forgot to add. Just like we downcase what user has written in config file, it might make sense to also remove '-' (and if we add '_' to keychar set, that one as well) to when canonicalizing the key value. That way, somewhat awkward long configuration variables we currently have can be written more readably, e.g. repack.use-delta-base-offset

Likes, dislikes? It is not strictly needed, since we can do CamelCase as well in the configuration file.

> By the way, everybody seems to do "alias.xxx = -p cat-file -p"
> (I have it as "git less").  Maybe we would want to make a
> built-in alias for that?
Seconds?
J. Bruce Fields· Oct 31, 2006, 03:05 UTC · re: Junio C Hamano · lore
On Mon, Oct 30, 2006 at 07:02:27PM -0800, Junio C Hamano wrote:
Show 6 quoted lines
> Junio C Hamano <junkio@cox.net> writes:
> > By the way, everybody seems to do "alias.xxx = -p cat-file -p"
> > (I have it as "git less").  Maybe we would want to make a
> > built-in alias for that?
> 
> Seconds?
Yes!
I'd vote for calling it just "git cat".
Linus Torvalds· Oct 31, 2006, 03:23 UTC · re: Junio C Hamano · lore
On Mon, 30 Oct 2006, Junio C Hamano wrote:
Show 7 quoted lines
> 
> One thing I forgot to add.  Just like we downcase what user has
> written in config file, it might make sense to also remove '-'
> (and if we add '_' to keychar set, that one as well) to when
> canonicalizing the key value.  That way, somewhat awkward long
> configuration variables we currently have can be written more
> readably, e.g. repack.use-delta-base-offset

Well, then we need to teach "git <alias>" that "git <al-i-as>" is the same thing as "alias".

Show 5 quoted lines
> > By the way, everybody seems to do "alias.xxx = -p cat-file -p"
> > (I have it as "git less").  Maybe we would want to make a
> > built-in alias for that?
> 
> Seconds?

I like it mainly as a great example of how to use aliases, not because I actually much -use- the thing, so I don't personally care.

Johannes Schindelin· Oct 31, 2006, 09:49 UTC · re: Junio C Hamano · lore
Hi,
On Mon, 30 Oct 2006, Junio C Hamano wrote:
Show 13 quoted lines
> Junio C Hamano <junkio@cox.net> writes:
> 
> > Linus Torvalds <torvalds@osdl.org> writes:
> >
> >> I need this in order to allow aliases of the same form as "ls-tree", 
> >> "rev-parse" etc, so that I can use
> >>
> >> 	[alias]
> >> 		my-cat=--paginate cat-file -p
> >>
> >> to add a "git my-cat" command.
> >
> Seconds?

I personally dislike any name with "_" or "-" in it, since I am stuck with different keyboard layouts and keep mistyping them. I even often find myself hitting <TAB> just to complete a _single_ "-", since the position of the <TAB> key is not wildly jumping around between different keyboard layouts.

So I can live without it.

Ciao, Dscho

← back to recent threads