{"thread":{"id":"48440","subject":"[PATCH 0/1] Fix UX issue with commit-graph.lock","startedAt":"2018-05-09T14:15:44Z","lastAt":"2018-05-11T08:59:36Z","messageCount":11,"participants":["Derrick Stolee","Jeff King","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"347079","messageId":"20180509141523.89896-1-dstolee@microsoft.com","threadId":"48440","inReplyTo":null,"subject":"[PATCH 0/1] Fix UX issue with commit-graph.lock","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-05-09T14:15:37Z","receivedAt":"2018-05-09T14:15:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"We use the lockfile API to avoid multiple Git processes from writing to\nthe commit-graph file in the .git/objects/info directory. In some cases,\nthis directory may not exist, so we check for its existence.\n\nThe existing code does the following when acquiring the lock:\n\n1. Try to acquire the lock.\n2. If it fails, try to create the .git/object/info directory.\n3. Try to acquire the lock, failing if necessary.\n\nThe problem is that if the lockfile exists, then the mkdir fails, giving\nan error that doesn't help the user:\n\n \"fatal: cannot mkdir .git/objects/info: File exists\"\n\nWhile technically this honors the lockfile, it does not help the user.\n\nInstead, do the following:\n\n1. Check for existence of .git/objects/info; create if necessary.\n2. Try to acquire the lock, failing if necessary.\n\nThe new output looks like:\n\n fatal: Unable to create '/home/stolee/git/.git/objects/info/commit-graph.lock': File exists.\n\n Another git process seems to be running in this repository, e.g.\n an editor opened by 'git commit'. Please make sure all processes\n are terminated then try again. If it still fails, a git process\n may have crashed in this repository earlier:\n remove the file manually to continue.\n\nThis patch is based on ds/generation-numbers\n\nDerrick Stolee (1):\n commit-graph: fix UX issue when .lock file exists\n\n commit-graph.c | 24 ++++++++----------------\n 1 file changed, 8 insertions(+), 16 deletions(-)\n\n\nbase-commit: 7547b95b4fbb8591726b1d9381c176cc27fc6aea\n-- \n2.17.0.39.g685157f7fb\n\n"},{"id":"347080","messageId":"20180509141523.89896-2-dstolee@microsoft.com","threadId":"48440","inReplyTo":"20180509141523.89896-1-dstolee@microsoft.com","subject":"[PATCH 1/1] commit-graph: fix UX issue when .lock file exists","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-05-09T14:15:38Z","receivedAt":"2018-05-09T14:15:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"The commit-graph file lives in the .git/objects/info directory.\nPreviously, a failure to acquire the commit-graph.lock file was\nassumed to be due to the lack of the info directory, so a mkdir()\nwas called. This gave incorrect messaging if instead the lockfile\nwas open by another process:\n\n \"fatal: cannot mkdir .git/objects/info: File exists\"\n\nRearrange the expectations of this directory existing to avoid\nthis error, and instead show the typical message when a lockfile\nalready exists.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 24 ++++++++----------------\n 1 file changed, 8 insertions(+), 16 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex a8c337dd77..8399194da1 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"config.h\"\n+#include \"dir.h\"\n #include \"git-compat-util.h\"\n #include \"lockfile.h\"\n #include \"pack.h\"\n@@ -640,13 +641,13 @@ void write_commit_graph(const char *obj_dir,\n \tstruct hashfile *f;\n \tuint32_t i, count_distinct = 0;\n \tchar *graph_name;\n-\tint fd;\n \tstruct lock_file lk = LOCK_INIT;\n \tuint32_t chunk_ids[5];\n \tuint64_t chunk_offsets[5];\n \tint num_chunks;\n \tint num_extra_edges;\n \tstruct commit_list *parent;\n+\tstruct strbuf folder = STRBUF_INIT;\n \n \toids.nr = 0;\n \toids.alloc = approximate_object_count() / 4;\n@@ -754,23 +755,14 @@ void write_commit_graph(const char *obj_dir,\n \tcompute_generation_numbers(&commits);\n \n \tgraph_name = get_commit_graph_filename(obj_dir);\n-\tfd = hold_lock_file_for_update(&lk, graph_name, 0);\n \n-\tif (fd < 0) {\n-\t\tstruct strbuf folder = STRBUF_INIT;\n-\t\tstrbuf_addstr(&folder, graph_name);\n-\t\tstrbuf_setlen(&folder, strrchr(folder.buf, '/') - folder.buf);\n-\n-\t\tif (mkdir(folder.buf, 0777) < 0)\n-\t\t\tdie_errno(_(\"cannot mkdir %s\"), folder.buf);\n-\t\tstrbuf_release(&folder);\n-\n-\t\tfd = hold_lock_file_for_update(&lk, graph_name, LOCK_DIE_ON_ERROR);\n-\n-\t\tif (fd < 0)\n-\t\t\tdie_errno(\"unable to create '%s'\", graph_name);\n-\t}\n+\tstrbuf_addstr(&folder, graph_name);\n+\tstrbuf_setlen(&folder, strrchr(folder.buf, '/') - folder.buf);\n+\tif (!file_exists(folder.buf) && mkdir(folder.buf, 0777) < 0)\n+\t\tdie_errno(_(\"cannot mkdir %s\"), folder.buf);\n+\tstrbuf_release(&folder);\n \n+\thold_lock_file_for_update(&lk, graph_name, LOCK_DIE_ON_ERROR);\n \tf = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);\n \n \thashwrite_be32(f, GRAPH_SIGNATURE);\n-- \n2.17.0.39.g685157f7fb\n\n"},{"id":"347082","messageId":"20180509144221.GA14714@sigill.intra.peff.net","threadId":"48440","inReplyTo":"20180509141523.89896-2-dstolee@microsoft.com","subject":"Re: [PATCH 1/1] commit-graph: fix UX issue when .lock file exists","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-09T14:42:22Z","receivedAt":"2018-05-09T14:42:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 09, 2018 at 02:15:38PM +0000, Derrick Stolee wrote:\n\n> The commit-graph file lives in the .git/objects/info directory.\n> Previously, a failure to acquire the commit-graph.lock file was\n> assumed to be due to the lack of the info directory, so a mkdir()\n> was called. This gave incorrect messaging if instead the lockfile\n> was open by another process:\n> \n> \"fatal: cannot mkdir .git/objects/info: File exists\"\n> \n> Rearrange the expectations of this directory existing to avoid\n> this error, and instead show the typical message when a lockfile\n> already exists.\n\nSounds like a reasonable bug fix.\n\nYour cover letter is way longer than this description. Should some of\nthat background perhaps go in the commit message?\n\n(I would go so far as to say that sending a cover letter for a single\npatch is an anti-pattern, because the commit message should be able to\nstand on its own).\n\n> @@ -754,23 +755,14 @@ void write_commit_graph(const char *obj_dir,\n> \tcompute_generation_numbers(&commits);\n> \n> \tgraph_name = get_commit_graph_filename(obj_dir);\n> -\tfd = hold_lock_file_for_update(&lk, graph_name, 0);\n> \n> -\tif (fd < 0) {\n> -\t\tstruct strbuf folder = STRBUF_INIT;\n> -\t\tstrbuf_addstr(&folder, graph_name);\n> -\t\tstrbuf_setlen(&folder, strrchr(folder.buf, '/') - folder.buf);\n> -\n> -\t\tif (mkdir(folder.buf, 0777) < 0)\n> -\t\t\tdie_errno(_(\"cannot mkdir %s\"), folder.buf);\n> -\t\tstrbuf_release(&folder);\n> -\n> -\t\tfd = hold_lock_file_for_update(&lk, graph_name, LOCK_DIE_ON_ERROR);\n> -\n> -\t\tif (fd < 0)\n> -\t\t\tdie_errno(\"unable to create '%s'\", graph_name);\n> -\t}\n> +\tstrbuf_addstr(&folder, graph_name);\n> +\tstrbuf_setlen(&folder, strrchr(folder.buf, '/') - folder.buf);\n> +\tif (!file_exists(folder.buf) && mkdir(folder.buf, 0777) < 0)\n> +\t\tdie_errno(_(\"cannot mkdir %s\"), folder.buf);\n> +\tstrbuf_release(&folder);\n\nThe result is racy if somebody else is trying to create the directory at\nthe same time. For that you'd want to notice EEXIST coming from mkdir\nand consider that a success.\n\nI think you probably ought to be calling adjust_shared_perm() on the\nresult, too, in case core.sharedrepository is configured.\n\nIf you use safe_create_leading_directories(), it should handle both.\nSomething like:\n\n if (safe_create_leading_directories(graph_name))\n\tdie_errno(_(\"unable to create leading directories of %s\"),\n\t\t graph_name));\n\nI think I'm holding it right; that function is a little short on\ndocumentation, but it's the standard way to do this in Git's codebase,\nand you can find lots of example callers.\n\n-Peff\n"},{"id":"347086","messageId":"926ef4a5-83c1-3d5f-6dfc-0b74fc7090bd@gmail.com","threadId":"48440","inReplyTo":"20180509144221.GA14714@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] commit-graph: fix UX issue when .lock file exists","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-05-09T14:53:56Z","receivedAt":"2018-05-09T14:54:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/9/2018 10:42 AM, Jeff King wrote:\n> On Wed, May 09, 2018 at 02:15:38PM +0000, Derrick Stolee wrote:\n>\n>> The commit-graph file lives in the .git/objects/info directory.\n>> Previously, a failure to acquire the commit-graph.lock file was\n>> assumed to be due to the lack of the info directory, so a mkdir()\n>> was called. This gave incorrect messaging if instead the lockfile\n>> was open by another process:\n>>\n>> \"fatal: cannot mkdir .git/objects/info: File exists\"\n>>\n>> Rearrange the expectations of this directory existing to avoid\n>> this error, and instead show the typical message when a lockfile\n>> already exists.\n> Sounds like a reasonable bug fix.\n>\n> Your cover letter is way longer than this description. Should some of\n> that background perhaps go in the commit message?\n\nI did want a place to include the full die() message in the new \nbehavior, but that seemed like overkill for the commit message.\n\n> (I would go so far as to say that sending a cover letter for a single\n> patch is an anti-pattern, because the commit message should be able to\n> stand on its own).\n>\n>> @@ -754,23 +755,14 @@ void write_commit_graph(const char *obj_dir,\n>> \tcompute_generation_numbers(&commits);\n>> \n>> \tgraph_name = get_commit_graph_filename(obj_dir);\n>> -\tfd = hold_lock_file_for_update(&lk, graph_name, 0);\n>> \n>> -\tif (fd < 0) {\n>> -\t\tstruct strbuf folder = STRBUF_INIT;\n>> -\t\tstrbuf_addstr(&folder, graph_name);\n>> -\t\tstrbuf_setlen(&folder, strrchr(folder.buf, '/') - folder.buf);\n>> -\n>> -\t\tif (mkdir(folder.buf, 0777) < 0)\n>> -\t\t\tdie_errno(_(\"cannot mkdir %s\"), folder.buf);\n>> -\t\tstrbuf_release(&folder);\n>> -\n>> -\t\tfd = hold_lock_file_for_update(&lk, graph_name, LOCK_DIE_ON_ERROR);\n>> -\n>> -\t\tif (fd < 0)\n>> -\t\t\tdie_errno(\"unable to create '%s'\", graph_name);\n>> -\t}\n>> +\tstrbuf_addstr(&folder, graph_name);\n>> +\tstrbuf_setlen(&folder, strrchr(folder.buf, '/') - folder.buf);\n>> +\tif (!file_exists(folder.buf) && mkdir(folder.buf, 0777) < 0)\n>> +\t\tdie_errno(_(\"cannot mkdir %s\"), folder.buf);\n>> +\tstrbuf_release(&folder);\n> The result is racy if somebody else is trying to create the directory at\n> the same time. For that you'd want to notice EEXIST coming from mkdir\n> and consider that a success.\n>\n> I think you probably ought to be calling adjust_shared_perm() on the\n> result, too, in case core.sharedrepository is configured.\n>\n> If you use safe_create_leading_directories(), it should handle both.\n> Something like:\n>\n> if (safe_create_leading_directories(graph_name))\n> \tdie_errno(_(\"unable to create leading directories of %s\"),\n> \t\t graph_name));\n>\n> I think I'm holding it right; that function is a little short on\n> documentation, but it's the standard way to do this in Git's codebase,\n> and you can find lots of example callers.\n\nThanks for this method. I was unfamiliar with it. Saves the effort of \ncreating the strbuf, too.\n\nThanks,\n-Stolee\n"},{"id":"347092","messageId":"CACsJy8CLF=sjJTw3ut1q9R3=qASwaDrm_dQGQfGDWar2LMwSkQ@mail.gmail.com","threadId":"48440","inReplyTo":"20180509141523.89896-1-dstolee@microsoft.com","subject":"Re: [PATCH 0/1] Fix UX issue with commit-graph.lock","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-09T16:02:04Z","receivedAt":"2018-05-09T16:02:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 9, 2018 at 4:15 PM, Derrick Stolee wrote:\n> We use the lockfile API to avoid multiple Git processes from writing to\n> the commit-graph file in the .git/objects/info directory. In some cases,\n> this directory may not exist, so we check for its existence.\n>\n> The existing code does the following when acquiring the lock:\n>\n> 1. Try to acquire the lock.\n> 2. If it fails, try to create the .git/object/info directory.\n> 3. Try to acquire the lock, failing if necessary.\n>\n> The problem is that if the lockfile exists, then the mkdir fails, giving\n> an error that doesn't help the user:\n>\n> \"fatal: cannot mkdir .git/objects/info: File exists\"\n>\n> While technically this honors the lockfile, it does not help the user.\n>\n> Instead, do the following:\n>\n> 1. Check for existence of .git/objects/info; create if necessary.\n> 2. Try to acquire the lock, failing if necessary.\n>\n> The new output looks like:\n>\n> fatal: Unable to create '/home/stolee/git/.git/objects/info/commit-graph.lock': File exists.\n>\n> Another git process seems to be running in this repository, e.g.\n> an editor opened by 'git commit'. Please make sure all processes\n> are terminated then try again. If it still fails, a git process\n> may have crashed in this repository earlier:\n> remove the file manually to continue.\n\nThis to me is a much better description than the current commit\nmessage in 1/1 and probably should be the commit message of 1/1.\n-- \nDuy\n"},{"id":"347172","messageId":"20180510045310.GA27259@sigill.intra.peff.net","threadId":"48440","inReplyTo":"926ef4a5-83c1-3d5f-6dfc-0b74fc7090bd@gmail.com","subject":"Re: [PATCH 1/1] commit-graph: fix UX issue when .lock file exists","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-10T04:53:11Z","receivedAt":"2018-05-10T04:53:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 09, 2018 at 10:53:56AM -0400, Derrick Stolee wrote:\n\n> > Your cover letter is way longer than this description. Should some of\n> > that background perhaps go in the commit message?\n> \n> I did want a place to include the full die() message in the new behavior,\n> but that seemed like overkill for the commit message.\n\nI think it would be fine. In general, it's probably a good idea to err\non the side of more information in the commit message than less. The one\nexception is that if your commit message grows very long, make sure it's\norganized well. I find there are two \"modes\" in which I read old commit\nmessages:\n\n 1. Skimming through log, etc, to try to get the gist of what the\n commit is doing and find out whether it might be related to my\n problem.\n\n 2. Once I've identified it as a problem, I want to know every single\n thing in the mind of the writer that might help me.\n\n(I'm not speaking to this particular message or your messages in general\nhere; I just didn't want to claim that a blanket \"longer is better\" is\nwithout limitations).\n\n-Peff\n"},{"id":"347184","messageId":"xmqqfu30j8q7.fsf@gitster-ct.c.googlers.com","threadId":"48440","inReplyTo":"20180509141523.89896-1-dstolee@microsoft.com","subject":"Re: [PATCH 0/1] Fix UX issue with commit-graph.lock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-10T07:00:32Z","receivedAt":"2018-05-10T07:00:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> We use the lockfile API to avoid multiple Git processes from writing to\n> the commit-graph file in the .git/objects/info directory. In some cases,\n> this directory may not exist, so we check for its existence.\n>\n> The existing code does the following when acquiring the lock:\n>\n> 1. Try to acquire the lock.\n> 2. If it fails, try to create the .git/object/info directory.\n> 3. Try to acquire the lock, failing if necessary.\n>\n> The problem is that if the lockfile exists, then the mkdir fails, giving\n> an error that doesn't help the user:\n>\n> \"fatal: cannot mkdir .git/objects/info: File exists\"\n\nIsn't a better immediate fix to make the second step pay attention\nto errno? If mkdir() failed due to EEXIST, then we know we tried to\naquire the lock already, so we can die with an appropriate message.\n\nThat way, we can keep the \"optimize for the normal case\" that the\napproach to assume object/info/ directory is already there, instead\nof always checking its existence which is almost always true\nbeforehand.\n\nAlso, can't we tell why we failed to acquire the lock at step #1?\nDo we only get a NULL that says \"I am not telling you why, but we\nfailed to lock\"? What I am getting at is that the ideal sequence\nwould be more like:\n\n 1. Try to acquire the lock.\n 2-a. if #1 succeeds, we are happy. ignore the rest and return\n the lock.\n 2-b. if #1 failed because object/info/ did not exist,\n mkdir() it, and die if we cannot, saying \"cannot mkdir\".\n\t if mkdir() succeeds, jump t 3.\n 2-c. if #1 failed but that is not due to missing object/info/,\n\t die saying \"cannot lock\".\n 3. Try to acquire the lock.\n 4-a. if #3 succeeds, we are happy.ignore the rest and return\n the lock.\n 4-b. die saying \"cannot lock\".\n\n"},{"id":"347220","messageId":"3b8005c4-a43d-bd46-c7d9-d6490d500114@gmail.com","threadId":"48440","inReplyTo":"xmqqfu30j8q7.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/1] Fix UX issue with commit-graph.lock","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-05-10T12:52:55Z","receivedAt":"2018-05-10T12:53:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/10/2018 3:00 AM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n>\n>> We use the lockfile API to avoid multiple Git processes from writing to\n>> the commit-graph file in the .git/objects/info directory. In some cases,\n>> this directory may not exist, so we check for its existence.\n>>\n>> The existing code does the following when acquiring the lock:\n>>\n>> 1. Try to acquire the lock.\n>> 2. If it fails, try to create the .git/object/info directory.\n>> 3. Try to acquire the lock, failing if necessary.\n>>\n>> The problem is that if the lockfile exists, then the mkdir fails, giving\n>> an error that doesn't help the user:\n>>\n>> \"fatal: cannot mkdir .git/objects/info: File exists\"\n> Isn't a better immediate fix to make the second step pay attention\n> to errno? If mkdir() failed due to EEXIST, then we know we tried to\n> aquire the lock already, so we can die with an appropriate message.\n>\n> That way, we can keep the \"optimize for the normal case\" that the\n> approach to assume object/info/ directory is already there, instead\n> of always checking its existence which is almost always true\n> beforehand.\n\nThis \"optimize for the normal case\" is why the existing code is \norganized the way it is.\n\nSince this code is only for writing a commit-graph file, this \"check the \ndirectory first\" option is a very small portion of the full time to \nwrite the file, so the \"optimization\" has very little effect, \nrelatively. My personal opinion is to make the code cleaner when the \nperformance difference is negligible.\n\nI'm willing to concede this point and use the steps you suggest below, \nif we think this is the best way forward.\n\n> Also, can't we tell why we failed to acquire the lock at step #1?\n> Do we only get a NULL that says \"I am not telling you why, but we\n> failed to lock\"?\n\nTo tell why we failed to acquire the lock, we could inspect \"errno\". \nHowever, this requires whitebox knowledge of both the lockfile API and \nthe tempfile API to know that the last system call to set errno was an \nopen() or adjust_shared_perm(). To cleanly make decisions based on the \nreason the lock failed to acquire, I think we would need to modify the \nlockfile and tempfile APIs to return a failure reason. This could be \ndone by passing an `int *reason`, but the extra noise in these APIs is \nlikely not worth the change.\n\n\n> What I am getting at is that the ideal sequence\n> would be more like:\n> 1. Try to acquire the lock.\n> 2-a. if #1 succeeds, we are happy. ignore the rest and return\n> the lock.\n> 2-b. if #1 failed because object/info/ did not exist,\n> mkdir() it, and die if we cannot, saying \"cannot mkdir\".\n> \t if mkdir() succeeds, jump t 3.\n> 2-c. if #1 failed but that is not due to missing object/info/,\n> \t die saying \"cannot lock\".\n> 3. Try to acquire the lock.\n> 4-a. if #3 succeeds, we are happy.ignore the rest and return\n> the lock.\n> 4-b. die saying \"cannot lock\".\n>\n\nThanks,\n-Stolee\n"},{"id":"347276","messageId":"20180510174242.44346-1-dstolee@microsoft.com","threadId":"48440","inReplyTo":"20180509141523.89896-1-dstolee@microsoft.com","subject":"[PATCH v2] commit-graph: fix UX issue when .lock file exists","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-05-10T17:42:52Z","receivedAt":"2018-05-10T17:42:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"We use the lockfile API to avoid multiple Git processes from writing to\nthe commit-graph file in the .git/objects/info directory. In some cases,\nthis directory may not exist, so we check for its existence.\n\nThe existing code does the following when acquiring the lock:\n\n1. Try to acquire the lock.\n2. If it fails, try to create the .git/object/info directory.\n3. Try to acquire the lock, failing if necessary.\n\nThe problem is that if the lockfile exists, then the mkdir fails, giving\nan error that doesn't help the user:\n\n \"fatal: cannot mkdir .git/objects/info: File exists\"\n\nWhile technically this honors the lockfile, it does not help the user.\n\nInstead, do the following:\n\n1. Check for existence of .git/objects/info; create if necessary.\n2. Try to acquire the lock, failing if necessary.\n\nThe new output looks like:\n\n fatal: Unable to create\n '/.git/objects/info/commit-graph.lock': File exists.\n\n Another git process seems to be running in this repository, e.g.\n an editor opened by 'git commit'. Please make sure all processes\n are terminated then try again. If it still fails, a git process\n may have crashed in this repository earlier:\n remove the file manually to continue.\n\nHelped-by: Jeff King \nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 22 +++++-----------------\n 1 file changed, 5 insertions(+), 17 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex a8c337dd77..bb54c1214c 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"config.h\"\n+#include \"dir.h\"\n #include \"git-compat-util.h\"\n #include \"lockfile.h\"\n #include \"pack.h\"\n@@ -640,7 +641,6 @@ void write_commit_graph(const char *obj_dir,\n \tstruct hashfile *f;\n \tuint32_t i, count_distinct = 0;\n \tchar *graph_name;\n-\tint fd;\n \tstruct lock_file lk = LOCK_INIT;\n \tuint32_t chunk_ids[5];\n \tuint64_t chunk_offsets[5];\n@@ -754,23 +754,11 @@ void write_commit_graph(const char *obj_dir,\n \tcompute_generation_numbers(&commits);\n \n \tgraph_name = get_commit_graph_filename(obj_dir);\n-\tfd = hold_lock_file_for_update(&lk, graph_name, 0);\n-\n-\tif (fd < 0) {\n-\t\tstruct strbuf folder = STRBUF_INIT;\n-\t\tstrbuf_addstr(&folder, graph_name);\n-\t\tstrbuf_setlen(&folder, strrchr(folder.buf, '/') - folder.buf);\n-\n-\t\tif (mkdir(folder.buf, 0777) < 0)\n-\t\t\tdie_errno(_(\"cannot mkdir %s\"), folder.buf);\n-\t\tstrbuf_release(&folder);\n-\n-\t\tfd = hold_lock_file_for_update(&lk, graph_name, LOCK_DIE_ON_ERROR);\n-\n-\t\tif (fd < 0)\n-\t\t\tdie_errno(\"unable to create '%s'\", graph_name);\n-\t}\n+\tif (safe_create_leading_directories(graph_name))\n+\t\tdie_errno(_(\"unable to create leading directories of %s\"),\n+\t\t\t graph_name);\n \n+\thold_lock_file_for_update(&lk, graph_name, LOCK_DIE_ON_ERROR);\n \tf = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);\n \n \thashwrite_be32(f, GRAPH_SIGNATURE);\n\nbase-commit: 34fdd433396ee0e3ef4de02eb2189f8226eafe4e\n-- \n2.16.2.329.gfb62395de6\n\n"},{"id":"347323","messageId":"xmqqk1sbhtf1.fsf@gitster-ct.c.googlers.com","threadId":"48440","inReplyTo":"3b8005c4-a43d-bd46-c7d9-d6490d500114@gmail.com","subject":"Re: [PATCH 0/1] Fix UX issue with commit-graph.lock","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-11T01:28:50Z","receivedAt":"2018-05-11T01:28:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n>> Also, can't we tell why we failed to acquire the lock at step #1?\n>> Do we only get a NULL that says \"I am not telling you why, but we\n>> failed to lock\"?\n>\n> To tell why we failed to acquire the lock, we could inspect\n> \"errno\". However, this requires whitebox knowledge of both the\n> lockfile API and the tempfile API to know that the last system call to\n> set errno was an open() or adjust_shared_perm().\n\nThat depends on your viewpoint. We can make it a part of documented\nAPI so that we keep the promise of preserving errno in the error\ncodepath when we update these APIs and then you do not have to worry\nabout errno being whitebox knowledge ;-).\n\n"},{"id":"347347","messageId":"20180511085932.GD22086@sigill.intra.peff.net","threadId":"48440","inReplyTo":"20180510174242.44346-1-dstolee@microsoft.com","subject":"Re: [PATCH v2] commit-graph: fix UX issue when .lock file exists","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-11T08:59:32Z","receivedAt":"2018-05-11T08:59:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 10, 2018 at 05:42:52PM +0000, Derrick Stolee wrote:\n\n> We use the lockfile API to avoid multiple Git processes from writing to\n> the commit-graph file in the .git/objects/info directory. In some cases,\n> this directory may not exist, so we check for its existence.\n> [...]\n\nThis version looks good to me. Thanks!\n\n-Peff\n"}]}