threads / patch / 24683

patchMore readable 'Not a git repository' messages

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

## tl;dr

13 messages between Aug 9, 2010 and Aug 15, 2010. Diffs are folded; open one to read it.

replies: 12people: 7as markdown or json

Ralf Ebert· Aug 9, 2010, 17:24 UTC · lore

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(-)
Show changes to setup.c +7 −3
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
Nguyen Thai Ngoc Duy· Aug 9, 2010, 22:14 UTC · re: Ralf Ebert · lore

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

On Tue, Aug 10, 2010 at 3:24 AM, Ralf Ebert <info@ralfebert.de> wrote:
Show 47 quoted lines
> 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.
Show 15 quoted lines
>                }
>                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
Ralf Ebert· Aug 9, 2010, 22:44 UTC · re: Nguyen Thai Ngoc Duy · lore

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

Hi Duy,
Show 11 quoted lines
> 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

Jonathan Nieder· Aug 9, 2010, 23:01 UTC · re: Ralf Ebert · lore

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

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. :)

Ralf Ebert· Aug 9, 2010, 23:34 UTC · re: Jonathan Nieder · lore

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

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

Nguyen Thai Ngoc Duy· Aug 10, 2010, 11:31 UTC · re: Ralf Ebert · lore

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

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
Sverre Rabbelier· Aug 10, 2010, 02:49 UTC · re: Ralf Ebert · lore

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

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
Jared Hance· Aug 10, 2010, 15:52 UTC · re: Ralf Ebert · lore

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

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.

Show 6 quoted lines
> 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)

Joshua Juran· Aug 10, 2010, 22:34 UTC · re: Jared Hance · lore

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

On Aug 10, 2010, at 8:52 AM, Jared Hance wrote:
Show 21 quoted lines
> 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
Ralf Ebert· Aug 13, 2010, 17:25 UTC · re: Ralf Ebert · lore

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

* 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(-)
Show changes to setup.c +8 −4
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
Joshua Juran· Aug 13, 2010, 17:30 UTC · re: Ralf Ebert · lore

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

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
Ralf Ebert· Aug 15, 2010, 02:55 UTC · re: Ralf Ebert · lore

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

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(-)
Show changes to setup.c +10 −4
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
Jonathan Nieder· Aug 15, 2010, 03:36 UTC · re: Ralf Ebert · lore

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

Hi Ralf,
Ralf Ebert wrote:
Show 8 quoted lines
> +++ 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.
Show 15 quoted lines
> @@ -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. :)

← back to recent threads