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

[Jeff King <peff@peff.net>]

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 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.

-Peff