Re: [PATCH 0/1] Fix UX issue with commit-graph.lock

From: Junio C Hamano <gitster@pobox.com>
Date: May 10, 2018, 07:00 UTC
Message-ID: <xmqqfu30j8q7.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180509141523.89896-1-dstolee@microsoft.com>

Derrick Stolee <dstolee@microsoft.com> writes:

Show 14 quoted lines

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

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