Re: [PATCH 2/4] Add functions get_relative_cwd() and is_inside_dir()

Junio C Hamano [off-list ref] writes:

Johannes Schindelin [off-list ref] writes:

quoted

The function get_relative_cwd() works just as getcwd(), only that it
takes an absolute path as additional parameter, returning the prefix
of the current working directory relative to the given path.  If the
cwd is no subdirectory of the given path, it returns NULL.
...
+/*
+ * get_relative_cwd() gets the prefix of the current working directory
+ * relative to 'dir'.  If we are not inside 'dir', it returns NULL.
+ * As a convenience, it also returns NULL if 'dir' is already NULL.
+ */
+char *get_relative_cwd(char *buffer, int size, const char *dir)
+{
+	char *cwd = buffer;
+
+	if (!dir || !getcwd(buffer, size))
+		return NULL;

When is it not a fatal error if get_relative_cwd() is called
with a NULL dir parameter, or getcwd() fails?

If there is no valid such cases, I would rather have this
die(), former with "BUG" and the latter with strerror(errno).

Heh, it turns out that there is this lazy or clever (depending
on the viewpoint) caller that passes the return value of
get_git_work_tree() to this function and expect this to return
NULL when no work tree is found.

The callers of the is_* functions are much cleaner and in that
sense the series is a definite improvement, but this one
particular obscurity makes me wonder if it is replacing one
unholy mess with a smaller but still unholy mess.

Will apply on "master" and will be part of -rc4, but we probably
would want to have a longer pre-final freeze than usual to
really make sure this one is good.

Hi,

On Tue, 31 Jul 2007, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

The function get_relative_cwd() works just as getcwd(), only that it
takes an absolute path as additional parameter, returning the prefix
of the current working directory relative to the given path.  If the
cwd is no subdirectory of the given path, it returns NULL.
...
+/*
+ * get_relative_cwd() gets the prefix of the current working directory
+ * relative to 'dir'.  If we are not inside 'dir', it returns NULL.
+ * As a convenience, it also returns NULL if 'dir' is already NULL.
+ */
+char *get_relative_cwd(char *buffer, int size, const char *dir)
+{
+	char *cwd = buffer;
+
+	if (!dir || !getcwd(buffer, size))
+		return NULL;

When is it not a fatal error if get_relative_cwd() is called
with a NULL dir parameter, or getcwd() fails?

If there is no valid such cases, I would rather have this
die(), former with "BUG" and the latter with strerror(errno).

Heh, it turns out that there is this lazy or clever (depending
on the viewpoint) caller that passes the return value of
get_git_work_tree() to this function and expect this to return
NULL when no work tree is found.

Right.  I thought I had said that (something along the lines: it is more 
convenient not having to check the directory), but I probably did not.

The callers of the is_* functions are much cleaner and in that sense the 
series is a definite improvement, but this one particular obscurity 
makes me wonder if it is replacing one unholy mess with a smaller but 
still unholy mess.

Will apply on "master" and will be part of -rc4, but we probably would 
want to have a longer pre-final freeze than usual to really make sure 
this one is good.

I'll provide a patch which makes the callers of get_relative_cwd() holy, 
and skip the check in get_relative_cwd(), okay?

Ciao,
Dscho

Earlier, get_relative_cwd() interpreted "dir == NULL" as "outside of the dir",
and therefore returned NULL.  Be more strict now.

Signed-off-by: Johannes Schindelin <redacted>
---

	As promised.

	Okay, I made up my mind.  Allowing "dir == NULL" is not only a matter of
	convenience.  It is the most natural way to say that "dir" is an invalid
	or non-existing directory.

	Besides, this patch adds 14.286% more lines than it removes ;-)

	But ultimately, it is your decision, Junio, and I am d'accord with 
	what you choose.

 dir.c   |    3 ---
 setup.c |   10 +++++++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/dir.c b/dir.c
index b3329f4..cfcde13 100644
--- a/dir.c
+++ b/dir.c

@@ -646,7 +646,6 @@ file_exists(const char *f)
 /*
  * get_relative_cwd() gets the prefix of the current working directory
  * relative to 'dir'.  If we are not inside 'dir', it returns NULL.
- * As a convenience, it also returns NULL if 'dir' is already NULL.
  */
 char *get_relative_cwd(char *buffer, int size, const char *dir)
 {

@@ -656,8 +655,6 @@ char *get_relative_cwd(char *buffer, int size, const char *dir)
 	 * a lazy caller can pass a NULL returned from get_git_work_tree()
 	 * and rely on this function to return NULL.
 	 */
-	if (!dir)
-		return NULL;
 	if (!getcwd(buffer, size))
 		die("can't find the current directory: %s", strerror(errno));
 

diff --git a/setup.c b/setup.c
index 3653092..2f720f8 100644
--- a/setup.c
+++ b/setup.c

@@ -183,8 +183,10 @@ int is_inside_git_dir(void)
 
 int is_inside_work_tree(void)
 {
-	if (inside_work_tree < 0)
-		inside_work_tree = is_inside_dir(get_git_work_tree());
+	if (inside_work_tree < 0) {
+		const char *work_tree = get_git_work_tree();
+		inside_work_tree = work_tree ? is_inside_dir(work_tree) : 0;
+	}
 	return inside_work_tree;
 }
 

@@ -370,10 +372,12 @@ const char *setup_git_directory(void)
 	/* If the work tree is not the default one, recompute prefix */
 	if (inside_work_tree < 0) {
 		static char buffer[PATH_MAX + 1];
+		const char *work_tree;
 		char *rel;
 		if (retval && chdir(retval))
 			die ("Could not jump back into original cwd");
-		rel = get_relative_cwd(buffer, PATH_MAX, get_git_work_tree());
+		work_tree = get_git_work_tree();
+		rel = work_tree ? get_relative_cwd(buffer, PATH_MAX, work_tree) : NULL;
 		return rel && *rel ? strcat(rel, "/") : NULL;
 	}
 

-- 
1.5.3.rc3.112.gf60b6