Fix UX issue with commit-graph.lock

The commit-graph file lives in the .git/objects/info directory. Previously, a failure to acquire the commit-graph.lock file was assumed to be due to the lack of the info directory, so a mkdir() was called. This gave incorrect messaging if instead the lockfile was open by another process:

"fatal: cannot mkdir .git/objects/info: File exists"

Rearrange the expectations of this directory existing to avoid this error, and instead show the typical message when a lockfile already exists.

commit-graph.c +0 -10

@@ -754,23 +755,14 @@ void write_commit_graph(const char *obj_dir,
 	compute_generation_numbers(&commits);
 
 	graph_name = get_commit_graph_filename(obj_dir);
-	fd = hold_lock_file_for_update(&lk, graph_name, 0);
 
-	if (fd < 0) {
-		struct strbuf folder = STRBUF_INIT;
-		strbuf_addstr(&folder, graph_name);
-		strbuf_setlen(&folder, strrchr(folder.buf, '/') - folder.buf);
-
-		if (mkdir(folder.buf, 0777) < 0)
-			die_errno(_("cannot mkdir %s"), folder.buf);
-		strbuf_release(&folder);
-

Quoted 8 lines · Derrick Stolee

The commit-graph file lives in the .git/objects/info directory. Previously, a failure to acquire the commit-graph.lock file was assumed to be due to the lack of the info directory, so a mkdir() was called. This gave incorrect messaging if instead the lockfile was open by another process:

"fatal: cannot mkdir .git/objects/info: File exists"

Quoted 3 lines

Rearrange the expectations of this directory existing to avoid this error, and instead show the typical message when a lockfile already exists.

Sounds like a reasonable bug fix.

Your cover letter is way longer than this description. Should some of that background perhaps go in the commit message?

(I would go so far as to say that sending a cover letter for a single patch is an anti-pattern, because the commit message should be able to stand on its own).

Quoted 15 lines

commit-graph.c +0 -10

@@ -754,23 +755,14 @@ void write_commit_graph(const char *obj_dir,
  	compute_generation_numbers(&commits);
 
  	graph_name = get_commit_graph_filename(obj_dir);
-	fd = hold_lock_file_for_update(&lk, graph_name, 0);
 
-	if (fd < 0) {
-		struct strbuf folder = STRBUF_INIT;
-		strbuf_addstr(&folder, graph_name);
-		strbuf_setlen(&folder, strrchr(folder.buf, '/') - folder.buf);
-
-		if (mkdir(folder.buf, 0777) < 0)
-			die_errno(_("cannot mkdir %s"), folder.buf);
-		strbuf_release(&folder);
-

commit-graph.c void write_commit_graph(const char *obj_dir, · line 763

Quoted 10 lines

commit-graph.c +5 -5

-		fd = hold_lock_file_for_update(&lk, graph_name, LOCK_DIE_ON_ERROR);
-
-		if (fd < 0)
-			die_errno("unable to create '%s'", graph_name);
-	}
+	strbuf_addstr(&folder, graph_name);
+	strbuf_setlen(&folder, strrchr(folder.buf, '/') - folder.buf);
+	if (!file_exists(folder.buf) && mkdir(folder.buf, 0777) < 0)
+		die_errno(_("cannot mkdir %s"), folder.buf);
+	strbuf_release(&folder);

The result is racy if somebody else is trying to create the directory at the same time. For that you'd want to notice EEXIST coming from mkdir and consider that a success.

I think you probably ought to be calling adjust_shared_perm() on the result, too, in case core.sharedrepository is configured.

If you use safe_create_leading_directories(), it should handle both. Something like:

if (safe_create_leading_directories(graph_name))

	die_errno(_("unable to create leading directories of %s"),
		  graph_name));

I think I'm holding it right; that function is a little short on documentation, but it's the standard way to do this in Git's codebase, and you can find lots of example callers.

Quoted 2 lines

Your cover letter is way longer than this description. Should some of that background perhaps go in the commit message?

I did want a place to include the full die() message in the new behavior, but that seemed like overkill for the commit message.

We use the lockfile API to avoid multiple Git processes from writing to the commit-graph file in the .git/objects/info directory. In some cases, this directory may not exist, so we check for its existence.

The existing code does the following when acquiring the lock:

1. Try to acquire the lock.
2. If it fails, try to create the .git/object/info directory.
3. Try to acquire the lock, failing if necessary.

The problem is that if the lockfile exists, then the mkdir fails, giving an error that doesn't help the user:

"fatal: cannot mkdir .git/objects/info: File exists"

While technically this honors the lockfile, it does not help the user.

Instead, do the following:

1. Check for existence of .git/objects/info; create if necessary.
2. Try to acquire the lock, failing if necessary.

The new output looks like:

fatal: Unable to create '/home/stolee/git/.git/objects/info/commit-graph.lock': File exists.

Another git process seems to be running in this repository, e.g. an editor opened by 'git commit'. Please make sure all processes are terminated then try again. If it still fails, a git process may have crashed in this repository earlier:
remove the file manually to continue.

We use the lockfile API to avoid multiple Git processes from writing to the commit-graph file in the .git/objects/info directory. In some cases, this directory may not exist, so we check for its existence.

The existing code does the following when acquiring the lock:

1. Try to acquire the lock.
2. If it fails, try to create the .git/object/info directory.
3. Try to acquire the lock, failing if necessary.

The problem is that if the lockfile exists, then the mkdir fails, giving an error that doesn't help the user:

"fatal: cannot mkdir .git/objects/info: File exists"

Quoted 6 lines · Derrick Stolee

We use the lockfile API to avoid multiple Git processes from writing to the commit-graph file in the .git/objects/info directory. In some cases, this directory may not exist, so we check for its existence.

The existing code does the following when acquiring the lock:

Quoted 8 lines

1. Try to acquire the lock.
2. If it fails, try to create the .git/object/info directory.
3. Try to acquire the lock, failing if necessary.

The problem is that if the lockfile exists, then the mkdir fails, giving an error that doesn't help the user:

"fatal: cannot mkdir .git/objects/info: File exists"

Isn't a better immediate fix to make the second step pay attention to errno? If mkdir() failed due to EEXIST, then we know we tried to aquire the lock already, so we can die with an appropriate message.

That way, we can keep the "optimize for the normal case" that the approach to assume object/info/ directory is already there, instead of always checking its existence which is almost always true beforehand.

Also, can't we tell why we failed to acquire the lock at step #1? Do we only get a NULL that says "I am not telling you why, but we failed to lock"?

What I am getting at is that the ideal sequence
would be more like:
1. Try to acquire the lock.
2-a. if #1 succeeds, we are happy. ignore the rest and return
the lock.
2-b. if #1 failed because object/info/ did not exist,

mkdir() it, and die if we cannot, saying "cannot mkdir".
if mkdir() succeeds, jump t 3.
2-c. if #1 failed but that is not due to missing object/info/,
die saying "cannot lock".
3. Try to acquire the lock.
4-a. if #3 succeeds, we are happy.ignore the rest and return
the lock.
4-b. die saying "cannot lock".

Quoted 3 lines

Also, can't we tell why we failed to acquire the lock at step #1? Do we only get a NULL that says "I am not telling you why, but we failed to lock"?

To tell why we failed to acquire the lock, we could inspect "errno". However, this requires whitebox knowledge of both the lockfile API and the tempfile API to know that the last system call to set errno was an open() or adjust_shared_perm().

We use the lockfile API to avoid multiple Git processes from writing to the commit-graph file in the .git/objects/info directory. In some cases, this directory may not exist, so we check for its existence. [...]