From: Derrick Stolee <stolee@gmail.com>
Date: Wed, 09 May 2018 14:53:56 GMT
Subject: Re: [PATCH 1/1] commit-graph: fix UX issue when .lock file exists
Message-ID: <926ef4a5-83c1-3d5f-6dfc-0b74fc7090bd@gmail.com>
In-Reply-To: <20180509144221.GA14714@sigill.intra.peff.net>

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