threads / patch / 4970

patchRe: [PATCH] Allow an alias to start with "-p"

Subject: Re: [PATCH] Allow an alias to start with "-p"

## tl;dr

8 messages between Jul 18, 2006 and Jul 25, 2006. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Johannes Schindelin· Jul 18, 2006, 23:25 UTC · lore

[PATCH] Allow an alias to start with "-p"

Now, something like
	[alias]
		pd = -p diff
works as expected.
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 git.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
Show changes to git.c +6 −0
diff --git a/git.c b/git.c
index 537af3a..a969b1b 100644
--- a/git.c
+++ b/git.c
@@ -115,6 +115,12 @@ static int handle_alias(int *argcp, cons
 
 			count = split_cmdline(alias_string, &new_argv);
 
+			if (count > 0 && !strcmp(new_argv[0], "-p")) {
+				setup_pager();
+				count--;
+				new_argv++;
+			}
+
 			if (count < 1)
 				die("empty alias for %s", alias_command);
 

-- 
1.4.1.g3c58e
Junio C Hamano· Jul 24, 2006, 06:32 UTC · re: Johannes Schindelin · lore
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 6 quoted lines
> Now, something like
>
> 	[alias]
> 		pd = -p diff
>
> works as expected.

I like what it wants to do but I am afraid this leads to an unmaintainable code (a micronit that already shows what I mean is that you can say "git --paginate diff", but you cannot say "pd = --paginate diff" in the configuration file).

Is there a cleaner way to do it without duplicating the argument loop of git.c::main()?

Johannes Schindelin· Jul 24, 2006, 12:10 UTC · re: Junio C Hamano · lore

[PATCH] Allow an alias to start with "-p"

    
Now, something like
	[alias]
		pd = -p diff
works as expected.
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
On Sun, 23 Jul 2006, Junio C Hamano wrote:
Show 16 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Now, something like
> >
> > 	[alias]
> > 		pd = -p diff
> >
> > works as expected.
> 
> I like what it wants to do but I am afraid this leads to an
> unmaintainable code (a micronit that already shows what I mean
> is that you can say "git --paginate diff", but you cannot say
> "pd = --paginate diff" in the configuration file).
> 
> Is there a cleaner way to do it without duplicating the argument
> loop of git.c::main()?

This patch uses a better approach: instead of duplicating the option parsing of the git wrapper, it refactors the code into the new function handle_options().

In related news, this function would be the perfect candidate to set GIT_DIR without environment variables...

 git.c |   37 +++++++++++++++++++++++++++++++------
 1 files changed, 31 insertions(+), 6 deletions(-)
Show changes to git.c +31 −6
diff --git a/git.c b/git.c
index ee5a0e8..8d7c644 100644
--- a/git.c
+++ b/git.c
@@ -35,6 +35,27 @@ static void prepend_to_path(const char *
 	setenv("PATH", path, 1);
 }
 
+static int handle_options(const char*** argv, int* argc)
+{
+	int handled = 0;
+
+	while (*argc > 0) {
+		const char *cmd = (*argv)[0];
+		if (cmd[0] != '-')
+			break;
+
+		if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
+			setup_pager();
+		} else
+			die ("Unknown option: %s", cmd);
+
+		(*argv)++;
+		(*argc)--;
+		handled++;
+	}
+	return handled;
+}
+
 static const char *alias_command;
 static char *alias_string = NULL;
 
@@ -106,7 +127,7 @@ static int handle_alias(int *argcp, cons
 
 	subdir = setup_git_directory_gently(&nongit);
 	if (!nongit) {
-		int count;
+		int count, option_count;
 		const char** new_argv;
 
 		alias_command = (*argv)[0];
@@ -114,6 +135,10 @@ static int handle_alias(int *argcp, cons
 		if (alias_string) {
 
 			count = split_cmdline(alias_string, &new_argv);
+			option_count = handle_options(&new_argv, &count);
+			memmove(new_argv - option_count, new_argv,
+					count * sizeof(char *));
+			new_argv -= option_count;
 
 			if (count < 1)
 				die("empty alias for %s", alias_command);
@@ -264,6 +289,7 @@ int main(int argc, const char **argv, ch
 	if (!strncmp(cmd, "git-", 4)) {
 		cmd += 4;
 		argv[0] = cmd;
+		handle_alias(&argc, &argv);
 		handle_internal_command(argc, argv, envp);
 		die("cannot handle %s internally", cmd);
 	}
@@ -273,13 +299,12 @@ int main(int argc, const char **argv, ch
 
 	/* Look for flags.. */
 	while (argc > 1) {
-		cmd = *++argv;
+		argv++;
 		argc--;
 
-		if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
-			setup_pager();
-			continue;
-		}
+		handle_options(&argv, &argc);
+			
+		cmd = *argv;
 
 		if (strncmp(cmd, "--", 2))
 			break;
Junio C Hamano· Jul 25, 2006, 00:36 UTC · re: Johannes Schindelin · lore
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 8 quoted lines
> Now, something like
>
> 	[alias]
> 		pd = -p diff
>
> works as expected.
>
> Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

This seems to break t5400 among other things (git-clone complains that it is not invoked in a git repository).

Johannes Schindelin· Jul 25, 2006, 06:27 UTC · re: Junio C Hamano · lore
Hi,
On Mon, 24 Jul 2006, Junio C Hamano wrote:
Show 13 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Now, something like
> >
> > 	[alias]
> > 		pd = -p diff
> >
> > works as expected.
> >
> > Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> 
> This seems to break t5400 among other things (git-clone
> complains that it is not invoked in a git repository).

See Peff's mail for a fix (hopefully: I do not see why git-clone should be affected, as it is a script, not a hard link to the git wrapper). The funny thing: t5400 does not break here.

Ciao, Dscho

Jeff King· Jul 25, 2006, 05:03 UTC · re: Johannes Schindelin · lore
On Mon, Jul 24, 2006 at 02:10:45PM +0200, Johannes Schindelin wrote:
Show 8 quoted lines
> @@ -264,6 +289,7 @@ int main(int argc, const char **argv, ch
>  	if (!strncmp(cmd, "git-", 4)) {
>  		cmd += 4;
>  		argv[0] = cmd;
> +		handle_alias(&argc, &argv);
>  		handle_internal_command(argc, argv, envp);
>  		die("cannot handle %s internally", cmd);
>  	}

I believe this change is the source of the breakage in tests. GIT_DIR=foo git-init-db no longer works because handle_alias unconditionally calls setup_git_directory_gently(), which thinks that if GIT_DIR is set, it must exist.

This can be fixed by giving precedence to the internal command over alias checking. This makes sense, anyway, since later in the function, we give precedence to internal commands in the "git init-db" form.

Patch is below (wow, that +++ is kind of ugly!).
-Peff

+++ git: choose internal commands over aliases for git-*

This is especially important because some commands (like init-db) don't require a working GIT_DIR, and alias expansion tries to look at it. It also matches the behavior of "git cmd".

Signed-off-by: Jeff King <peff@peff.net>
---
 git.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to git.c +1 −1
diff --git a/git.c b/git.c
index 8d7c644..68ce826 100644
--- a/git.c
+++ b/git.c
@@ -289,8 +289,8 @@ int main(int argc, const char **argv, ch
 	if (!strncmp(cmd, "git-", 4)) {
 		cmd += 4;
 		argv[0] = cmd;
-		handle_alias(&argc, &argv);
 		handle_internal_command(argc, argv, envp);
+		handle_alias(&argc, &argv);
 		die("cannot handle %s internally", cmd);
 	}
 

-- 
1.4.2.rc1.gc470-dirty
Johannes Schindelin· Jul 25, 2006, 06:18 UTC · re: Jeff King · lore
Hi,
On Tue, 25 Jul 2006, Jeff King wrote:
Show 10 quoted lines
> On Mon, Jul 24, 2006 at 02:10:45PM +0200, Johannes Schindelin wrote:
> 
> > @@ -264,6 +289,7 @@ int main(int argc, const char **argv, ch
> >  	if (!strncmp(cmd, "git-", 4)) {
> >  		cmd += 4;
> >  		argv[0] = cmd;
> > +		handle_alias(&argc, &argv);
> >  		handle_internal_command(argc, argv, envp);
> >  		die("cannot handle %s internally", cmd);
> >  	}

My fault. This was a left-over from my original alias patch. (I did a merge, and just used "git-diff next" instead of "git-diff --merge next". <Clickety-click/> Nope, that would not have worked either.

> Patch is below (wow, that +++ is kind of ugly!).
Same here.
Show 7 quoted lines
> git: choose internal commands over aliases for git-*
> 
> This is especially important because some commands (like init-db) don't
> require a working GIT_DIR, and alias expansion tries to look at it. It
> also matches the behavior of "git cmd".
> 
> Signed-off-by: Jeff King <peff@peff.net>
Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Show 9 quoted lines
> @@ -289,8 +289,8 @@ int main(int argc, const char **argv, ch
>  	if (!strncmp(cmd, "git-", 4)) {
>  		cmd += 4;
>  		argv[0] = cmd;
> -		handle_alias(&argc, &argv);
>  		handle_internal_command(argc, argv, envp);
> +		handle_alias(&argc, &argv);
>  		die("cannot handle %s internally", cmd);
>  	}

Alternatively, you can just delete it. IIRC we decided that aliases with "git-" commands do not make sense.

Ciao, Dscho

Junio C Hamano· Jul 25, 2006, 20:34 UTC · re: Johannes Schindelin · lore
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Patch is below (wow, that +++ is kind of ugly!).
>
> Same here.
Same here ;-).
Show 14 quoted lines
> Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>
>> @@ -289,8 +289,8 @@ int main(int argc, const char **argv, ch
>>  	if (!strncmp(cmd, "git-", 4)) {
>>  		cmd += 4;
>>  		argv[0] = cmd;
>> -		handle_alias(&argc, &argv);
>>  		handle_internal_command(argc, argv, envp);
>> +		handle_alias(&argc, &argv);
>>  		die("cannot handle %s internally", cmd);
>>  	}
>
> Alternatively, you can just delete it. IIRC we decided that aliases with 
> "git-" commands do not make sense.
I think that is reasonable and simpler.

← back to recent threads