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
