From: Derrick Stolee <stolee@gmail.com>
To: Junio C Hamano <gitster@pobox.com>,
Derrick Stolee <dstolee@microsoft.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [PATCH 0/1] Fix UX issue with commit-graph.lock
Date: Thu, 10 May 2018 08:52:55 -0400 [thread overview]
Message-ID: <3b8005c4-a43d-bd46-c7d9-d6490d500114@gmail.com> (raw)
In-Reply-To: <xmqqfu30j8q7.fsf@gitster-ct.c.googlers.com>
On 5/10/2018 3:00 AM, Junio C Hamano wrote:
> Derrick Stolee <dstolee@microsoft.com> writes:
>
>> 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.
This "optimize for the normal case" is why the existing code is
organized the way it is.
Since this code is only for writing a commit-graph file, this "check the
directory first" option is a very small portion of the full time to
write the file, so the "optimization" has very little effect,
relatively. My personal opinion is to make the code cleaner when the
performance difference is negligible.
I'm willing to concede this point and use the steps you suggest below,
if we think this is the best way forward.
> 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(). To cleanly make decisions based on the
reason the lock failed to acquire, I think we would need to modify the
lockfile and tempfile APIs to return a failure reason. This could be
done by passing an `int *reason`, but the extra noise in these APIs is
likely not worth the change.
> 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".
>
Thanks,
-Stolee
next prev parent reply other threads:[~2018-05-10 12:53 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-09 14:15 [PATCH 0/1] Fix UX issue with commit-graph.lock Derrick Stolee
2018-05-09 14:15 ` [PATCH 1/1] commit-graph: fix UX issue when .lock file exists Derrick Stolee
2018-05-09 14:42 ` Jeff King
2018-05-09 14:53 ` Derrick Stolee
2018-05-10 4:53 ` Jeff King
2018-05-09 16:02 ` [PATCH 0/1] Fix UX issue with commit-graph.lock Duy Nguyen
2018-05-10 7:00 ` Junio C Hamano
2018-05-10 12:52 ` Derrick Stolee [this message]
2018-05-11 1:28 ` Junio C Hamano
2018-05-10 17:42 ` [PATCH v2] commit-graph: fix UX issue when .lock file exists Derrick Stolee
2018-05-11 8:59 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3b8005c4-a43d-bd46-c7d9-d6490d500114@gmail.com \
--to=stolee@gmail.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).