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

Johannes Schindelin [off-list ref] 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()?

    
Now, something like

	[alias]
		pd = -p diff

works as expected.

Signed-off-by: Johannes Schindelin <redacted>

---

On Sun, 23 Jul 2006, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

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(-)

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;

Johannes Schindelin [off-list ref] writes:

Now, something like

	[alias]
		pd = -p diff

works as expected.

Signed-off-by: Johannes Schindelin <redacted>

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

On Mon, Jul 24, 2006 at 02:10:45PM +0200, Johannes Schindelin wrote:

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 <redacted>
---
 git.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

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

Hi,

On Tue, 25 Jul 2006, Jeff King wrote:

On Mon, Jul 24, 2006 at 02:10:45PM +0200, Johannes Schindelin wrote:

quoted

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

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 <redacted>

Acked-by: Johannes Schindelin <redacted>

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

Ciao,
Dscho

Hi,

On Mon, 24 Jul 2006, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

Now, something like

	[alias]
		pd = -p diff

works as expected.

Signed-off-by: Johannes Schindelin <redacted>

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