[PATCH] More readable 'Not a git repository' messages

[PATCH] More readable 'Not a git repository' messages

[Ralf Ebert]

Stating the folder concerned with the operation and wording the
messages differently make them slightly more readable.

old: Not a git repository (or any of the parent directories): .git
new: Not in a git repository: /home/bob/somefolder

old: Not a git repository (or any parent up to mount parent /home/bob)
old: Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM 
not set).
new: Not in a git repository: /home/bob/somefolder
new: (stopped searching at /home because GIT_DISCOVERY_ACROSS_FILESYSTEM 
is not set)

Signed-off-by: Ralf Ebert <info@ralfebert.de>
---
  setup.c |   11 +++++++----
  1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index 2769160..e27fbf1 100644
--- a/setup.c
+++ b/setup.c
@@ -321,6 +321,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
  {
  	const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
  	const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
+	char cwd_orig[PATH_MAX];
  	static char cwd[PATH_MAX+1];
  	const char *gitdirenv;
  	const char *gitfile_dir;
@@ -376,8 +377,9 @@ const char *setup_git_directory_gently(int *nongit_ok)
  		die("Not a git repository: '%s'", gitdirenv);
  	}
  -	if (!getcwd(cwd, sizeof(cwd)-1))
+	if (!getcwd(cwd_orig, sizeof(cwd_orig)))
  		die_errno("Unable to read current working directory");
+	strcpy(cwd, cwd_orig);
   	ceil_offset = longest_ancestor_length(cwd, env_ceiling_dirs);
  	if (ceil_offset < 0 && has_dos_drive_prefix(cwd))
@@ -431,7 +433,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
  				*nongit_ok = 1;
  				return NULL;
  			}
-			die("Not a git repository (or any of the parent directories): %s", 
DEFAULT_GIT_DIR_ENVIRONMENT);
+			die("Not in a git repository: %s", cwd_orig);
  		}
  		if (one_filesystem) {
  			if (stat("..", &buf)) {
@@ -446,8 +448,9 @@ const char *setup_git_directory_gently(int *nongit_ok)
  					return NULL;
  				}
  				cwd[offset] = '\0';
-				die("Not a git repository (or any parent up to mount parent %s)\n"
-				"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM 
not set).", cwd);
+				die("Not in a git repository: %s\n"
+				"(stopped searching at %s because "
+				"GIT_DISCOVERY_ACROSS_FILESYSTEM is not set)", cwd_orig, cwd);
  			}
  		}
  		if (chdir("..")) {
-- 
1.7.2.1.44.g721e7.dirty

Re: [PATCH] More readable 'Not a git repository' messages

[Nguyen Thai Ngoc Duy]

On Tue, Aug 10, 2010 at 3:24 AM, Ralf Ebert <info@ralfebert.de> wrote:
> Stating the folder concerned with the operation and wording the
> messages differently make them slightly more readable.
>
> old: Not a git repository (or any of the parent directories): .git
> new: Not in a git repository: /home/bob/somefolder
>
> old: Not a git repository (or any parent up to mount parent /home/bob)
> old: Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not
> set).
> new: Not in a git repository: /home/bob/somefolder
> new: (stopped searching at /home because GIT_DISCOVERY_ACROSS_FILESYSTEM is
> not set)
>
> Signed-off-by: Ralf Ebert <info@ralfebert.de>
> ---
>  setup.c |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 2769160..e27fbf1 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -321,6 +321,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  {
>        const char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);
>        const char *env_ceiling_dirs =
> getenv(CEILING_DIRECTORIES_ENVIRONMENT);
> +       char cwd_orig[PATH_MAX];
>        static char cwd[PATH_MAX+1];
>        const char *gitdirenv;
>        const char *gitfile_dir;
> @@ -376,8 +377,9 @@ const char *setup_git_directory_gently(int *nongit_ok)
>                die("Not a git repository: '%s'", gitdirenv);
>        }
>  -      if (!getcwd(cwd, sizeof(cwd)-1))
> +       if (!getcwd(cwd_orig, sizeof(cwd_orig)))
>                die_errno("Unable to read current working directory");
> +       strcpy(cwd, cwd_orig);
>        ceil_offset = longest_ancestor_length(cwd, env_ceiling_dirs);
>        if (ceil_offset < 0 && has_dos_drive_prefix(cwd))
> @@ -431,7 +433,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
>                                *nongit_ok = 1;
>                                return NULL;
>                        }
> -                       die("Not a git repository (or any of the parent
> directories): %s", DEFAULT_GIT_DIR_ENVIRONMENT);
> +                       die("Not in a git repository: %s", cwd_orig);

cwd can be used in this case.

>                }
>                if (one_filesystem) {
>                        if (stat("..", &buf)) {
> @@ -446,8 +448,9 @@ const char *setup_git_directory_gently(int *nongit_ok)
>                                        return NULL;
>                                }
>                                cwd[offset] = '\0';
> -                               die("Not a git repository (or any parent up
> to mount parent %s)\n"
> -                               "Stopping at filesystem boundary
> (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).", cwd);
> +                               die("Not in a git repository: %s\n"
> +                               "(stopped searching at %s because "
> +                               "GIT_DISCOVERY_ACROSS_FILESYSTEM is not
> set)", cwd_orig, cwd);

How about this and avoid adding cwd_orig?

-                               cwd[offset] = '\0';
-                               die("Not a git repository (or any
parent up to mount parent %s)\n"
-                               "Stopping at filesystem boundary
(GIT_DISCOVERY_ACROSS_FILESYSTEM not set).", cwd);
+                               die("Not in a git repository: %s\n"
+                               "(stopped searching at %.*s because "
+                               "GIT_DISCOVERY_ACROSS_FILESYSTEM is
not set)", cwd, offset, cwd);
-- 
Duy

Re: [PATCH] More readable 'Not a git repository' messages

[Ralf Ebert]

Hi Duy,

> How about this and avoid adding cwd_orig?
>
> -                               cwd[offset] = '\0';
> -                               die("Not a git repository (or any
> parent up to mount parent %s)\n"
> -                               "Stopping at filesystem boundary
> (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).", cwd);
> +                               die("Not in a git repository: %s\n"
> +                               "(stopped searching at %.*s because "
> +                               "GIT_DISCOVERY_ACROSS_FILESYSTEM is
> not set)", cwd, offset, cwd);

this code is inside a loop that goes up in the directory hierarchy. So 
cwd[offset] = '\0' does not only happen once before dying. Maybe it 
could be done without copying but I thought keeping a copy of the 
unaltered path might be more readable.

--
Ralf

Re: [PATCH] More readable 'Not a git repository' messages

[Jonathan Nieder]

Ralf Ebert wrote:

> this code is inside a loop that goes up in the directory hierarchy.
> So cwd[offset] = '\0' does not only happen once before dying.

I hope not, since then the returned cwd value would be useless
for restoring the original cwd later.

Shameless plug: you may find the version in commit 98937be (i.e.
branch jn/maint-setup-fix in pu) more readable. :)

Re: [PATCH] More readable 'Not a git repository' messages

[Ralf Ebert]

Hi Jonathan,

> I hope not, since then the returned cwd value would be useless
> for restoring the original cwd later.

yes, right, I misread the code, thanks for taking the time to correct 
me. My C knowledge is dated to say least and being succumbed by all the 
syntax sugar of dynamic languages, I misread
if (chdir("..")) { ... }
as "if changing the directory worked ok" :)

> Shameless plug: you may find the version in commit 98937be (i.e.
> branch jn/maint-setup-fix in pu) more readable. :)

I do, I guess it makes sense to rework the patch on top of that.

What I also wanted to try is make the GIT_DISCOVERY_ACROSS_FILESYSTEM 
warning go away for crypto home volumes, because that's kind of common 
and nobody needs git to discover repositories in this case. Only need to 
come up with an idea how to detect this case.

--
Ralf

Re: [PATCH] More readable 'Not a git repository' messages

[Sverre Rabbelier]

Heya,

On Mon, Aug 9, 2010 at 12:24, Ralf Ebert <info@ralfebert.de> wrote:
> Stating the folder concerned with the operation and wording the
> messages differently make them slightly more readable.

FWIW I like these improvements, nice work.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] More readable 'Not a git repository' messages

[Nguyen Thai Ngoc Duy]

On Tue, Aug 10, 2010 at 9:34 AM, Ralf Ebert <info@ralfebert.de> wrote:
> I do, I guess it makes sense to rework the patch on top of that.

While you're working on that, I was thinking that cwd may be long as
it's absolute path. How about rephrase the error message a little bit
and give more space for cwd? Something like

Not a git repository: <reason explained>
Stopped at <current cwd>
-- 
Duy

Re: [PATCH] More readable 'Not a git repository' messages

[Jared Hance]

On Mon, Aug 09, 2010 at 07:24:35PM +0200, Ralf Ebert wrote:
> old: Not a git repository (or any of the parent directories): .git
> new: Not in a git repository: /home/bob/somefolder

Don't we lose information here? Perhaps print the value of
DEFAULT_GIT_DIR_ENVIRONMENT.

> old: Not a git repository (or any parent up to mount parent /home/bob)
> old: Stopping at filesystem boundary
> (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
> new: Not in a git repository: /home/bob/somefolder
> new: (stopped searching at /home because
> GIT_DISCOVERY_ACROSS_FILESYSTEM is not set)

This certainly looks good, but some people might not realize /home is
a filesystem boundary (perhaps those who had someone else set up their
system and don't know their partition setup. I suggest:

(stopped searching at the filesystem boundary, /home, because
GIT_DISCOVERY_ACROSS_FILESYSTEM is not set)

Re: [PATCH] More readable 'Not a git repository' messages

[Joshua Juran]

On Aug 10, 2010, at 8:52 AM, Jared Hance wrote:

> On Mon, Aug 09, 2010 at 07:24:35PM +0200, Ralf Ebert wrote:
>> old: Not a git repository (or any of the parent directories): .git
>> new: Not in a git repository: /home/bob/somefolder
>
> Don't we lose information here? Perhaps print the value of
> DEFAULT_GIT_DIR_ENVIRONMENT.
>
>> old: Not a git repository (or any parent up to mount parent /home/ 
>> bob)
>> old: Stopping at filesystem boundary
>> (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
>> new: Not in a git repository: /home/bob/somefolder
>> new: (stopped searching at /home because
>> GIT_DISCOVERY_ACROSS_FILESYSTEM is not set)
>
> This certainly looks good, but some people might not realize /home is
> a filesystem boundary (perhaps those who had someone else set up their
> system and don't know their partition setup. I suggest:
>
> (stopped searching at the filesystem boundary, /home, because
> GIT_DISCOVERY_ACROSS_FILESYSTEM is not set)

If the search for .git stops at /home or $HOME, why not say nothing at  
all?  If you've turned /home (or /) into a Git repo, then either you  
know what you're doing or you deserve as much pain as possible.  (I  
haven't decided which.)

Josh

[PATCH/RFC v2] More readable 'Not a git repository' messages

[Ralf Ebert]

* State the folder concerned with the operation
* GIT_DISCOVERY_ACROSS_FILESYSTEM warning goes first
* Supressed 'filesystem boundary' warning for $HOME (crypto home volumes
   are common, having a git repository at $HOME is uncommon)
* Improved wording of the messages

Signed-off-by: Ralf Ebert <info@ralfebert.de>
---
  setup.c |   12 ++++++++----
  1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index 2769160..dc4f2da 100644
--- a/setup.c
+++ b/setup.c
@@ -431,7 +431,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
  				*nongit_ok = 1;
  				return NULL;
  			}
-			die("Not a git repository (or any of the parent directories): %s", 
DEFAULT_GIT_DIR_ENVIRONMENT);
+			die("%s is not in a git repository", cwd);
  		}
  		if (one_filesystem) {
  			if (stat("..", &buf)) {
@@ -445,9 +445,13 @@ const char *setup_git_directory_gently(int *nongit_ok)
  					*nongit_ok = 1;
  					return NULL;
  				}
-				cwd[offset] = '\0';
-				die("Not a git repository (or any parent up to mount parent %s)\n"
-				"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM 
not set).", cwd);
+
+				const char *home = getenv("HOME");
+				if (home == NULL || strncmp(home, cwd, offset) != 0) {
+					warning("Stopped searching for repository at %.*s "
+							"as GIT_DISCOVERY_ACROSS_FILESYSTEM is not set", offset, cwd);
+				}
+				die("%s is not in a git repository", cwd);
  			}
  		}
  		if (chdir("..")) {
-- 
1.7.2.1.96.g07594.dirty

Re: [PATCH/RFC v2] More readable 'Not a git repository' messages

[Joshua Juran]

On Aug 13, 2010, at 10:25 AM, Ralf Ebert wrote:

> * State the folder concerned with the operation
> * GIT_DISCOVERY_ACROSS_FILESYSTEM warning goes first
> * Supressed 'filesystem boundary' warning for $HOME (crypto home  
> volumes

s/Supressed/Suppressed/

>  are common, having a git repository at $HOME is uncommon)
> * Improved wording of the messages

Josh

[PATCH] setup.c: Improve 'Not a git repository' messages

[Ralf Ebert]

Changed message "Not a git repository (or any of the parent directories)"
to the more precise message "No <.git> repository in <path> or its parent
directories".

If a filesystem boundary is encountered with
GIT_DISCOVERY_ACROSS_FILESYSTEM not set, the warning now goes first.
The warning is suppressed when search was stopped at $HOME to not confuse
users using a crypto home volume (assuming that users having a repository
at /home or / know what they are doing).

Thanks to Duy, Jonathan, Sverre and Josh for commenting.

Signed-off-by: Ralf Ebert <ralf@ralfebert.de>
---
 setup.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index 2769160..adabdd8 100644
--- a/setup.c
+++ b/setup.c
@@ -431,7 +431,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 				*nongit_ok = 1;
 				return NULL;
 			}
-			die("Not a git repository (or any of the parent directories): %s", DEFAULT_GIT_DIR_ENVIRONMENT);
+			die("No %s repository in %s or its parent directories",
+				DEFAULT_GIT_DIR_ENVIRONMENT, cwd);
 		}
 		if (one_filesystem) {
 			if (stat("..", &buf)) {
@@ -445,9 +446,14 @@ const char *setup_git_directory_gently(int *nongit_ok)
 					*nongit_ok = 1;
 					return NULL;
 				}
-				cwd[offset] = '\0';
-				die("Not a git repository (or any parent up to mount parent %s)\n"
-				"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).", cwd);
+				const char *home = getenv("HOME");
+				if (home == NULL || strncmp(home, cwd, offset) != 0) {
+					warning("Stopped searching for %s at %.*s "
+						"as GIT_DISCOVERY_ACROSS_FILESYSTEM is not set",
+						DEFAULT_GIT_DIR_ENVIRONMENT, offset, cwd);
+				}
+				die("No %s repository in %s or its parent directories",
+					DEFAULT_GIT_DIR_ENVIRONMENT, cwd);
 			}
 		}
 		if (chdir("..")) {
-- 
1.7.2.1.96.gb740

Re: [PATCH] setup.c: Improve 'Not a git repository' messages

[Jonathan Nieder]

Hi Ralf,

Ralf Ebert wrote:

> +++ b/setup.c
> @@ -431,7 +431,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  				*nongit_ok = 1;
>  				return NULL;
>  			}
> -			die("Not a git repository (or any of the parent directories): %s", DEFAULT_GIT_DIR_ENVIRONMENT);
> +			die("No %s repository in %s or its parent directories",
> +				DEFAULT_GIT_DIR_ENVIRONMENT, cwd);

So, before:

 fatal: Not a git repository (or any of the parent directories): .git

and after:

 fatal: No .git repository in /home/jrn/src/some/deeply/nested/path/that/goes/on/for/a/while or its parent directories

The idea being to give a hint to the confused user.  That sort of
makes sense, but I would (warning! nitpicks coming!) prefer something
along the lines of

 fatal: not a git repository: /home/jrn/src/some/deeply/nested/pa
 hint: This means every candidate metadata (.git) directory considered
 hint: was invalid in some way.
 hint: See "git help repository-layout" for more information.

In other words:

 - first, a straightforward error message;
 - cutting off user-specified data at some reasonable length
   (e.g. 50 chars);
 - avoiding overly specific (false) advice and deferring to
   documentation for the details.

> @@ -445,9 +446,14 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  					*nongit_ok = 1;
>  					return NULL;
>  				}
> -				cwd[offset] = '\0';
> -				die("Not a git repository (or any parent up to mount parent %s)\n"
> -				"Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).", cwd);
> +				const char *home = getenv("HOME");
> +				if (home == NULL || strncmp(home, cwd, offset) != 0) {
> +					warning("Stopped searching for %s at %.*s "
> +						"as GIT_DISCOVERY_ACROSS_FILESYSTEM is not set",
> +						DEFAULT_GIT_DIR_ENVIRONMENT, offset, cwd);
> +				}
> +				die("No %s repository in %s or its parent directories",
> +					DEFAULT_GIT_DIR_ENVIRONMENT, cwd);

I don’t understand what’s special about $HOME here.

Would something like the following be okay?

 fatal: not a git repository: /home/jrn/src/some/deeply/nested/pa
 info: search stopped at mount parent /home
 hint: This means every candidate metadata (.git) directory considered
 hint: was invalid in some way.
 hint: See "git help repository-layout" for more information.

I actually suspect overriding with $GIT_DISCOVERY_ACROSS_FILESYSTEM
would be kind of rare, so it might be okay to let the documentation
take care of explaining that (provided we include a clear pointer).

Just my two cents.  Feel free to ignore. :)