Web lists-archives.com

Re: [PATCH 1/1] commit-graph: fix UX issue when .lock file exists




On 5/9/2018 10:42 AM, Jeff King wrote:
On Wed, May 09, 2018 at 02:15:38PM +0000, Derrick Stolee wrote:

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.
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 did want a place to include the full die() message in the new behavior, but that seemed like overkill for 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).

@@ -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);
-
-		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.

Thanks for this method. I was unfamiliar with it. Saves the effort of creating the strbuf, too.

Thanks,
-Stolee