git's use of mkdir(2)

git's use of mkdir(2)

[der Mouse]

I've been trying to convince git to run on some of the systems I use.
Of particular relevance at the moment are two BSD systems which have an
important behavioural difference.

Specifically, if /foo/bar does not exist but /foo does,
mkdir("/foo/bar/",...) works on one and fails showing ENOENT on the
other.  (Without the trailing slash, it works on both.)

git (at least in the version I've got here, which appears to be
1.6.4.1) depends on this working, and breaks as a result on the second
of the above two systems.

I've been doing a little digging and I think I might be able to fix
this - but, before I charged ahead with it, I thought I'd float a query
here to ask (a) if this is a known issue and fixed in something more
recent (I had a look at 1.7.2 and a quick read of the code makes me
think it still does this, but I could have missed something) and (b) if
there would be any interest in such fixes if I do come up with them.

Thoughts?

/~\ The ASCII				  Mouse
\ / Ribbon Campaign
 X  Against HTML		mouse@rodents-montreal.org
/ \ Email!	     7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B

Re: git's use of mkdir(2)

[Junio C Hamano]

der Mouse <mouse@Rodents-Montreal.ORG> writes:

> I've been trying to convince git to run on some of the systems I use.
> Of particular relevance at the moment are two BSD systems which have an
> important behavioural difference.
>
> Specifically, if /foo/bar does not exist but /foo does,
> mkdir("/foo/bar/",...) works on one and fails showing ENOENT on the
> other.  (Without the trailing slash, it works on both.)

What vintage of BSD do you have that exhibits the problem?  It smells like
a POSIX violation, considering what "4.12 Pathname Resolution" says on the
matter:

  A pathname that contains at least one non-<slash> character and that
  ends with one or more trailing <slash> characters shall not be resolved
  successfully unless the last pathname component before the trailing
  <slash> characters names an existing directory or a directory entry that
  is to be created for a directory immediately after the pathname is
  resolved.

Not that I am saying that such a system does not deserve to be supported,
but I am curious to know how widespread the damage is.

> here to ask (a) if this is a known issue and fixed in something more
> recent (I had a look at 1.7.2 and a quick read of the code makes me
> think it still does this, but I could have missed something)

I don't think so---we seem to have a compat/ replacement "mkdir(2)" for
MinGW (but that doesn't trim the trailing slash so I would imagine MinGW
does not suffer from such a violation), but not for the flavor of BSD you
have.  It shouldn't be hard to add one, though.

Re: git's use of mkdir(2)

[der Mouse]

>> Specifically, if /foo/bar does not exist but /foo does,
>> mkdir("/foo/bar/",...) works on one and fails showing ENOENT on the
>> other.  (Without the trailing slash, it works on both.)
> What vintage of BSD do you have that exhibits the problem?

NetBSD 1.4T.  (About a decade old at this point.)  It's unlikely that
the machine I most care about this on is going to switch versions
anytime soon, though I may be able to convince its mkdir(2) to strip
trailing slashes - I should look at the other implementation....

> It smells like a POSIX violation, considering what "4.12 Pathname
> Resolution" says on the matter:

>   A pathname that contains at least one non-<slash> character and
>   that ends with one or more trailing <slash> characters shall not be
>   resolved successfully unless the last pathname component before the
>   trailing <slash> characters names an existing directory or a
>   directory entry that is to be created for a directory immediately
>   after the pathname is resolved.

Well, notice that it does not say that it shall be successfully
resolved in either of those exception cases, just that it shall not be
successfully resolved in other cases.  I read this as "you can append a
slash to require that the last component be a directory", with the
second exception so that the mkdir-with-trailing-slash-works behaviour
is permitted; I don't read it as mandating that mkdir work with a
trailing slash.  Is there a rationale of any sort that elaborates on
this choice, or perhaps something about mkdir elsewhere?

> Not that I am saying that such a system does not deserve to be
> supported, but I am curious to know how widespread the damage is.

I don't know.  Of my own experience, I know one version that tolerates
what git does and one version that doesn't; I haven't experimented to
try to determine how widespread either is - though I can infer that
Linux falls in the trailing-slash-allowed camp.

For the moment, I bludgeoned it into working with the patch below, but
I haven't really exercised it very much yet, so it's entirely possible
it's going to trip over the same thing somewhere else - there are a lot
of mkdir calls scattered around and I haven't looked at any outside
builtin-init-db.c.

Personally, I'd tend to avoid depending on this detail of mkdir, when
it's easy (which it appears to be in the case of builtin-init-db.c)
just on general principles.  But I don't know whether this is outside
the scope of what git cares about - I just know _I_ care about it. :)

Given the existing code, a mkdir wrapper which strips trailing slashes
might be the least-pain answer; it feels wrong to accrete wrappers like
that all over the place, but sometimes they are right answers....

/~\ The ASCII				  Mouse
\ / Ribbon Campaign
 X  Against HTML		mouse@rodents-montreal.org
/ \ Email!	     7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B

Here's the patch I mentioned above.  (This is relative to 1.6.4.1's
builtin-init-db.c; so far I haven't tried to build anything newer.  I
have a handful more patches which I apply as part of my build script,
but they're not relevant to this particular thread; I don't think any
of them interact with this one.)

--- OLD/builtin-init-db.c	Thu Jan  1 00:00:00 1970
+++ NEW/builtin-init-db.c	Thu Jan  1 00:00:00 1970
@@ -46,6 +46,8 @@
 	 * be really carefully chosen.
 	 */
 	safe_create_dir(path, 1);
+	path[baselen++] = '/';
+	template[template_baselen++] = '/';
 	while ((de = readdir(dir)) != NULL) {
 		struct stat st_git, st_template;
 		int namelen;
@@ -75,8 +77,6 @@
 			int template_baselen_sub = template_baselen + namelen;
 			if (!subdir)
 				die_errno("cannot opendir '%s'", template);
-			path[baselen_sub++] =
-				template[template_baselen_sub++] = '/';
 			path[baselen_sub] =
 				template[template_baselen_sub] = 0;
 			copy_templates_1(path, baselen_sub,
@@ -127,10 +127,6 @@
 	if (PATH_MAX <= (template_len+strlen("/config")))
 		die("insanely long template path %s", template_dir);
 	strcpy(template_path, template_dir);
-	if (template_path[template_len-1] != '/') {
-		template_path[template_len++] = '/';
-		template_path[template_len] = 0;
-	}
 	dir = opendir(template_path);
 	if (!dir) {
 		warning("templates not found %s", template_dir);
@@ -138,7 +134,7 @@
 	}
 
 	/* Make sure that template is from the correct vintage */
-	strcpy(template_path + template_len, "config");
+	strcpy(template_path + template_len, "/config");
 	repository_format_version = 0;
 	git_config_from_file(check_repository_format_version,
 			     template_path, NULL);
@@ -155,8 +151,6 @@
 	}
 
 	memcpy(path, git_dir, len);
-	if (len && path[len - 1] != '/')
-		path[len++] = '/';
 	path[len] = 0;
 	copy_templates_1(path, len,
 			 template_path, template_len,
@@ -217,7 +211,7 @@
 	 * Create the default symlink from ".git/HEAD" to the "master"
 	 * branch, if it does not exist yet.
 	 */
-	strcpy(path + len, "HEAD");
+	strcpy(path + len, "/HEAD");
 	reinit = (!access(path, R_OK)
 		  || readlink(path, junk, sizeof(junk)-1) != -1);
 	if (!reinit) {
@@ -230,7 +224,7 @@
 	git_config_set("core.repositoryformatversion", repo_version_string);
 
 	path[len] = 0;
-	strcpy(path + len, "config");
+	strcpy(path + len, "/config");
 
 	/* Check filemode trustability */
 	filemode = TEST_FILEMODE;
@@ -259,7 +253,7 @@
 	if (!reinit) {
 		/* Check if symlink is supported in the work tree */
 		path[len] = 0;
-		strcpy(path + len, "tXXXXXX");
+		strcpy(path + len, "/tXXXXXX");
 		if (!close(xmkstemp(path)) &&
 		    !unlink(path) &&
 		    !symlink("testing", path) &&
@@ -271,7 +265,7 @@
 
 		/* Check if the filesystem is case-insensitive */
 		path[len] = 0;
-		strcpy(path + len, "CoNfIg");
+		strcpy(path + len, "/CoNfIg");
 		if (!access(path, F_OK))
 			git_config_set("core.ignorecase", "true");
 	}