[PATCH] git.c: Re-introduce sane error messages on missing commands.

[PATCH] git.c: Re-introduce sane error messages on missing commands.

[Andreas Ericsson]

Somewhere in the alias handling git turned hostile on fat fingers:

	$ git showbranch
	Failed to run command '': Is a directory

This patch fixes that by doing 2 things:
 * The variable git_command[MAX_PATH + 1] is now retired from
   git.c:main() where it was never set. Instead the variable "cmd"
   is used for all error messages.
 * The introduction of the "exec_errno" variable which preserves the
   errno number from the execve() attempt. Later "why did it fail"
   tests evaluate exec_errno, which gives the correct error message
   along with the brief command-list (telling me I missed the dash in
   "show-branch").

It's possible that alias handling can fail and set errno to something
proper, making this change less sane than I think, but handle_alias()
seems to take care of its own error-handling so it shouldn't matter.

Signed-off-by: Andreas Ericsson <ae@op5.se>
---
 git.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/git.c b/git.c
index 94e9a4a..b04424f 100644
--- a/git.c
+++ b/git.c
@@ -206,9 +206,8 @@ int main(int argc, const char **argv, ch
 {
 	const char *cmd = argv[0];
 	char *slash = strrchr(cmd, '/');
-	char git_command[PATH_MAX + 1];
 	const char *exec_path = NULL;
-	int done_alias = 0;
+	int exec_errno = 0, done_alias = 0;
 
 	/*
 	 * Take the basename of argv[0] as the command
@@ -300,6 +299,9 @@ int main(int argc, const char **argv, ch
 		/* .. then try the external ones */
 		execv_git_cmd(argv);
 
+		/* if it's not an alias, the execve() errno is what we want */
+		exec_errno = errno;
+
 		/* It could be an alias -- this works around the insanity
 		 * of overriding "git log" with "git show" by having
 		 * alias.log = show
@@ -309,11 +311,11 @@ int main(int argc, const char **argv, ch
 		done_alias = 1;
 	}
 
-	if (errno == ENOENT)
+	if (exec_errno == ENOENT)
 		cmd_usage(0, exec_path, "'%s' is not a git-command", cmd);
 
 	fprintf(stderr, "Failed to run command '%s': %s\n",
-		git_command, strerror(errno));
+		cmd, strerror(exec_errno));
 
 	return 1;
 }
-- 
1.4.1.rc1.g1ef9

Re: [PATCH] git.c: Re-introduce sane error messages on missing commands.

[Junio C Hamano]

Andreas Ericsson <ae@op5.se> writes:

> Somewhere in the alias handling git turned hostile on fat fingers:
>
> 	$ git showbranch
> 	Failed to run command '': Is a directory

Does not happen here (nor on Cygwin 1.4.1.rc1).  Care to help
reproducing it?

Re: [PATCH] git.c: Re-introduce sane error messages on missing commands.

[Andreas Ericsson]

Junio C Hamano wrote:
> Andreas Ericsson <ae@op5.se> writes:
> 
> 
>>Somewhere in the alias handling git turned hostile on fat fingers:
>>
>>	$ git showbranch
>>	Failed to run command '': Is a directory
> 
> 
> Does not happen here (nor on Cygwin 1.4.1.rc1).  Care to help
> reproducing it?
> 

Here's the complete procedure I used:

	$ git pull && make; # works ok
	$ make strip install; # works ok
	$ git showbranch
	Failed to run command '': Is a directory
	(confusion and puzzlement...)
	$ git checkout master; git reset --hard master; # works ok
	$ git pull; # gets nothing (I try stupid things first ;p)
	$ make clean install; # works ok
	$ git showbranch
	Failed to run command '': Is a directory
	$ git --version
	git version 1.4.1.rc1.g1ef9


It's reliably reproducible here. Notable though is that I have no 
.git/config in my git.git clone and no ~/.gitconfig (or ~/.gitrc or 
whatever), so the handle_alias() function never finds a config file to 
look for aliases in.

Either way, removing the variable "char git_command[MAX_PATH + 1];" from 
git.c:main() is correct since it's never used for anything but printing 
the (erroneous) error message above.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] git.c: Re-introduce sane error messages on missing commands.

[Johannes Schindelin]

Hi,

On Wed, 28 Jun 2006, Andreas Ericsson wrote:

> Junio C Hamano wrote:
> > Andreas Ericsson <ae@op5.se> writes:
> > 
> > 
> > > Somewhere in the alias handling git turned hostile on fat fingers:
> > > 
> > > 	$ git showbranch
> > > 	Failed to run command '': Is a directory
> > 
> > 
> > Does not happen here (nor on Cygwin 1.4.1.rc1).  Care to help
> > reproducing it?

Try this:

$ mkdir 5
$ cd 5
$ git-init-db
$ rm .git/config # yes, really.
$ git abc

Ciao,
Dscho

Re: [PATCH] git.c: Re-introduce sane error messages on missing commands.

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Try this:
>
> $ mkdir 5
> $ cd 5
> $ git-init-db
> $ rm .git/config # yes, really.
> $ git abc

Thanks for trying to help, but not really.

: gitster; mkdir 5
: gitster; cd 5
: gitster; git-init-db
defaulting to local storage area
: gitster; rm .git/config
: gitster; ~/git-master/bin/git abc
git: 'abc' is not a git-command

The most commonly used git commands are:
    add            Add files to the index file
    apply          Apply patch on a git index file and a work
...
    tag            Create a tag object signed with GPG
    verify-tag     Check the GPG signature of tag
(use 'git help -a' to get a list of all installed git commands)
: gitster; ~/git-master/bin/git --version
git version 1.4.1.rc1.g8096
: gitster; ls -ld ~/.gitrc ~/.gitconfig ~/.git-config
: gitster; ls -ld ~/.gitrc ~/.gitconfig ~/.git-config
ls: /home/junio/.gitrc: No such file or directory
ls: /home/junio/.gitconfig: No such file or directory
ls: /home/junio/.git-config: No such file or directory
: gitster; : confused...

Re: [PATCH] git.c: Re-introduce sane error messages on missing commands.

[Johannes Schindelin]

Hi,

On Wed, 28 Jun 2006, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Try this:
> >
> > $ mkdir 5
> > $ cd 5
> > $ git-init-db
> > $ rm .git/config # yes, really.
> > $ git abc
> 
> Thanks for trying to help, but not really.

Okay. Does not happen with 'next' here, too. I have some changes in my 
private repo (which eventually should culminate in the big mmap()ed sooper 
config parsing / writing thingie), which make it break. The following 
patch fixes this (and potentially Andreas' problem, too).

-- cut here --

[PATCH] save errno in handle_alias()

git.c:main() relies on the value of errno being set by the last attempt to 
execute the command. However, if something goes awry in handle_alias(), 
that assumption is wrong. So restore errno before returning from 
handle_alias().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

---

diff --git a/git.c b/git.c
index 94e9a4a..7c7106e 100644
--- a/git.c
+++ b/git.c
@@ -99,7 +99,7 @@ static int split_cmdline(char *cmdline, 
 
 static int handle_alias(int *argcp, const char ***argv)
 {
-	int nongit = 0, ret = 0;
+	int nongit = 0, ret = 0, saved_errno = errno;
 	const char *subdir;
 
 	subdir = setup_git_directory_gently(&nongit);
@@ -137,6 +137,8 @@ static int handle_alias(int *argcp, cons
 	if (subdir)
 		chdir(subdir);
 
+	errno = saved_errno;
+
 	return ret;
 }
 

Re: [PATCH] git.c: Re-introduce sane error messages on missing commands.

[Andreas Ericsson]

Johannes Schindelin wrote:
> Hi,
> 
> On Wed, 28 Jun 2006, Junio C Hamano wrote:
> 
> 
>>Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>>
>>>Try this:
>>>
>>>$ mkdir 5
>>>$ cd 5
>>>$ git-init-db
>>>$ rm .git/config # yes, really.
>>>$ git abc
>>
>>Thanks for trying to help, but not really.
> 
> 
> Okay. Does not happen with 'next' here, too. I have some changes in my 
> private repo (which eventually should culminate in the big mmap()ed sooper 
> config parsing / writing thingie), which make it break. The following 
> patch fixes this (and potentially Andreas' problem, too).
> 

It should, although the command it tried to execute will still be empty 
if it fails for some other reason (file not executable / permission 
denied), since it only does the right thing on ENOENT.

This is also, imo, a bit worse than preserving the errno from the 
execve() call in the caller, since errno is sometimes a macro (yes, only 
in threaded apps atm, but still...), and it will be easy to forget to 
look in handle_alias() if other things change in main() that makes this 
bug resurface.

Oh, and the part of my patch removing the git_command variable from 
git.c:main() still has to be applied for arbitrary error-messages to 
look sane.

$ grep -B1 git_command git.c
         char *slash = strrchr(cmd, '/');
         char git_command[PATH_MAX + 1];
--
         fprintf(stderr, "Failed to run command '%s': %s\n",
                 git_command, strerror(errno));


Btw, Junio, did you try this with 'master' as of yesterday morning (git 
version 1.4.1.rc1.g1ef9)? It's reproducible on every machine I've tried 
so far (well, only five, but still), so it seems odd that you don't see it.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] git.c: Re-introduce sane error messages on missing commands.

[Marco Roeland]

On Wednesday June 28th 2006 Johannes Schindelin wrote:

> [PATCH] save errno in handle_alias()
> 
> git.c:main() relies on the value of errno being set by the last attempt to 
> execute the command. However, if something goes awry in handle_alias(), 
> that assumption is wrong. So restore errno before returning from 
> handle_alias().

If we rely on the value of errno we should always immediately store it's
value anyway. On some neolithic systems like the "MSVCRT.DLL" C runtime
library on Windows (used by e.g. the Mingw compiler, don't know about
Cygwin) a lot of runtime functions actually even reset the value of
errno to 0 on success!
-- 
Marco Roeland

Re: [PATCH] git.c: Re-introduce sane error messages on missing commands.

[Christopher Faylor]

On Wed, Jun 28, 2006 at 02:00:44PM +0200, Marco Roeland wrote:
>On Wednesday June 28th 2006 Johannes Schindelin wrote:
>
>> [PATCH] save errno in handle_alias()
>> 
>> git.c:main() relies on the value of errno being set by the last attempt to 
>> execute the command. However, if something goes awry in handle_alias(), 
>> that assumption is wrong. So restore errno before returning from 
>> handle_alias().
>
>If we rely on the value of errno we should always immediately store it's
>value anyway. On some neolithic systems like the "MSVCRT.DLL" C runtime
>library on Windows (used by e.g. the Mingw compiler, don't know about
>Cygwin) a lot of runtime functions actually even reset the value of
>errno to 0 on success!

Cygwin does not use MSVCRT.DLL and tries to be careful about spurious
resetting of errno.

cgf