# [PATCH] More readable 'Not a git repository' messages 13 messages from 2010-08-09 to 2010-08-15. Participants: Ralf Ebert, Nguyen Thai Ngoc Duy, Jonathan Nieder, Sverre Rabbelier, Jared Hance, Joshua Juran. Thread: https://gitlist.dev/t/24683 ## Ralf Ebert, 2010-08-09 17:24 Subject: [PATCH] More readable 'Not a git repository' messages Message-ID: URL: https://gitlist.dev/e/i3pdkj%24hut%241%40dough.gmane.org ``` 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 --- 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 ``` ## Nguyen Thai Ngoc Duy, 2010-08-09 22:14 Subject: Re: [PATCH] More readable 'Not a git repository' messages Message-ID: URL: https://gitlist.dev/e/AANLkTinttmKi4%2BEbE54r-GeRbN3_2wxUHu_qJMn5Nysn%40mail.gmail.com In-Reply-To: ``` On Tue, Aug 10, 2010 at 3:24 AM, Ralf Ebert 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 > --- >  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 ``` ## Ralf Ebert, 2010-08-09 22:44 Subject: Re: [PATCH] More readable 'Not a git repository' messages Message-ID: URL: https://gitlist.dev/e/i3q0d8%24m8f%241%40dough.gmane.org In-Reply-To: ``` 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 ``` ## Jonathan Nieder, 2010-08-09 23:01 Subject: Re: [PATCH] More readable 'Not a git repository' messages Message-ID: <20100809230132.GG4429@burratino> URL: https://gitlist.dev/e/20100809230132.GG4429%40burratino In-Reply-To: ``` 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, 2010-08-09 23:34 Subject: Re: [PATCH] More readable 'Not a git repository' messages Message-ID: URL: https://gitlist.dev/e/i3q3ah%24upb%241%40dough.gmane.org In-Reply-To: <20100809230132.GG4429@burratino> ``` 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 ``` ## Sverre Rabbelier, 2010-08-10 02:49 Subject: Re: [PATCH] More readable 'Not a git repository' messages Message-ID: URL: https://gitlist.dev/e/AANLkTinJ80FT5HMDg6r1Vz%2BJ%2BoX_35xoWrygE42UAzkA%40mail.gmail.com In-Reply-To: ``` Heya, On Mon, Aug 9, 2010 at 12:24, Ralf Ebert 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 ``` ## Nguyen Thai Ngoc Duy, 2010-08-10 11:31 Subject: Re: [PATCH] More readable 'Not a git repository' messages Message-ID: URL: https://gitlist.dev/e/AANLkTimeizUKBrwibUTnVPZWwU_M72uQTn8ucmjK_V70%40mail.gmail.com In-Reply-To: ``` On Tue, Aug 10, 2010 at 9:34 AM, Ralf Ebert 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: Stopped at -- Duy ``` ## Jared Hance, 2010-08-10 15:52 Subject: Re: [PATCH] More readable 'Not a git repository' messages Message-ID: <20100810155240.GA5116@localhost.localdomain> URL: https://gitlist.dev/e/20100810155240.GA5116%40localhost.localdomain In-Reply-To: ``` 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) ``` ## Joshua Juran, 2010-08-10 22:34 Subject: Re: [PATCH] More readable 'Not a git repository' messages Message-ID: URL: https://gitlist.dev/e/D944E855-0ECF-41AF-ACD4-8ED8EABCBC1D%40gmail.com In-Reply-To: <20100810155240.GA5116@localhost.localdomain> ``` 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 ``` ## Ralf Ebert, 2010-08-13 17:25 Subject: [PATCH/RFC v2] More readable 'Not a git repository' messages Message-ID: URL: https://gitlist.dev/e/i43v6e%24q00%241%40dough.gmane.org In-Reply-To: ``` * 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 --- 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 ``` ## Joshua Juran, 2010-08-13 17:30 Subject: Re: [PATCH/RFC v2] More readable 'Not a git repository' messages Message-ID: <4994E665-9084-4479-B44F-4D5562CE4BB0@gmail.com> URL: https://gitlist.dev/e/4994E665-9084-4479-B44F-4D5562CE4BB0%40gmail.com In-Reply-To: ``` 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, 2010-08-15 02:55 Subject: [PATCH] setup.c: Improve 'Not a git repository' messages Message-ID: <1281840921-1996-1-git-send-email-ralf@ralfebert.de> URL: https://gitlist.dev/e/1281840921-1996-1-git-send-email-ralf%40ralfebert.de In-Reply-To: ``` Changed message "Not a git repository (or any of the parent directories)" to the more precise message "No <.git> repository in 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 --- 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 ``` ## Jonathan Nieder, 2010-08-15 03:36 Subject: Re: [PATCH] setup.c: Improve 'Not a git repository' messages Message-ID: <20100815033640.GA20486@burratino> URL: https://gitlist.dev/e/20100815033640.GA20486%40burratino In-Reply-To: <1281840921-1996-1-git-send-email-ralf@ralfebert.de> ``` 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. :) ```