{"thread":{"id":"39342","subject":"[PATCH] builtin/clean.c: Handle disappearing files","startedAt":"2015-05-14T21:16:40Z","lastAt":"2015-05-16T19:21:04Z","messageCount":8,"participants":["David Turner","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"261293","messageId":"1431638200-20449-1-git-send-email-dturner@twitter.com","threadId":"39342","inReplyTo":null,"subject":"[PATCH] builtin/clean.c: Handle disappearing files","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-05-14T21:16:40Z","receivedAt":"2015-05-14T21:16:40Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"During a git clean, some other process might be deleting files as\nwell. If this happens, make git clean no longer die.\n\nSigned-off-by: David Turner \n---\n builtin/clean.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 98c103f..3ae44c2 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -941,8 +941,16 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\tif (!cache_name_is_other(ent->name, ent->len))\n \t\t\tcontinue;\n \n-\t\tif (lstat(ent->name, &st))\n-\t\t\tdie_errno(\"Cannot lstat '%s'\", ent->name);\n+\t\t/*\n+\t\t * Some concurrent process might have already removed\n+\t\t * ent->name.\n+\t\t */\n+\t\tif (lstat(ent->name, &st)) {\n+\t\t\tif (errno == ENOENT || errno == ENOTDIR)\n+\t\t\t\tcontinue;\n+\t\t\telse\n+\t\t\t\tdie_errno(\"Cannot lstat '%s'\", ent->name);\n+\t\t}\n \n \t\tif (pathspec.nr)\n \t\t\tmatches = dir_path_match(ent, &pathspec, 0, NULL);\n-- \n2.0.4.315.gad8727a-twtrsrc\n"},{"id":"261298","messageId":"xmqqk2waj0js.fsf@gitster.dls.corp.google.com","threadId":"39342","inReplyTo":"1431638200-20449-1-git-send-email-dturner@twitter.com","subject":"Re: [PATCH] builtin/clean.c: Handle disappearing files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-14T21:45:27Z","receivedAt":"2015-05-14T21:45:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> During a git clean, some other process might be deleting files as\n> well. If this happens, make git clean no longer die.\n>\n> Signed-off-by: David Turner \n> ---\n\nI am having a hard time to convince myself that this is a good\nchange.\n\nFor this change to be an improvement that matters, you must be\nallowing some other process mucking around with the files in your\nworking tree when you run \"git clean\". The original catches such\nsituation as an anomaly that the user would want to be aware of (and\ninvestigate), but this patch essentially declares that having such a\nrandom process touching your working tree from sideways is a normal\nsituation. I do not think I am willing to make such a declaration\nmyself; I'd rather want to know why other people are touching my\nworking tree while I am working in it.\n\n> builtin/clean.c | 12 ++++++++++--\n> 1 file changed, 10 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> index 98c103f..3ae44c2 100644\n> --- a/builtin/clean.c\n> +++ b/builtin/clean.c\n> @@ -941,8 +941,16 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n> \t\tif (!cache_name_is_other(ent->name, ent->len))\n> \t\t\tcontinue;\n> \n> -\t\tif (lstat(ent->name, &st))\n> -\t\t\tdie_errno(\"Cannot lstat '%s'\", ent->name);\n> +\t\t/*\n> +\t\t * Some concurrent process might have already removed\n> +\t\t * ent->name.\n> +\t\t */\n> +\t\tif (lstat(ent->name, &st)) {\n> +\t\t\tif (errno == ENOENT || errno == ENOTDIR)\n> +\t\t\t\tcontinue;\n> +\t\t\telse\n> +\t\t\t\tdie_errno(\"Cannot lstat '%s'\", ent->name);\n> +\t\t}\n> \n> \t\tif (pathspec.nr)\n> \t\t\tmatches = dir_path_match(ent, &pathspec, 0, NULL);\n"},{"id":"261301","messageId":"1431640373.17436.16.camel@ubuntu","threadId":"39342","inReplyTo":"xmqqk2waj0js.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] builtin/clean.c: Handle disappearing files","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-05-14T21:52:53Z","receivedAt":"2015-05-14T21:52:53Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Thu, 2015-05-14 at 14:45 -0700, Junio C Hamano wrote:\n> David Turner writes:\n> \n> > During a git clean, some other process might be deleting files as\n> > well. If this happens, make git clean no longer die.\n> >\n> > Signed-off-by: David Turner \n> > ---\n> \n> I am having a hard time to convince myself that this is a good\n> change.\n> \n> For this change to be an improvement that matters, you must be\n> allowing some other process mucking around with the files in your\n> working tree when you run \"git clean\". The original catches such\n> situation as an anomaly that the user would want to be aware of (and\n> investigate), but this patch essentially declares that having such a\n> random process touching your working tree from sideways is a normal\n> situation. I do not think I am willing to make such a declaration\n> myself; I'd rather want to know why other people are touching my\n> working tree while I am working in it.\n\nOur build tool[1] stores statistics in a hidden file in the working\ntree. After it runs, it daemonizes and uploads those stats, and then\ndeletes the stats file. Because the upload might take some time, the\nuser might run git clean in the meantime. (I think there might be some\nother deletion that happens in the background too; I haven't really\ninvestigated).\n\nOf course, in interactive use, very little harm is done if clean dies\nhere: the user simply must notice that the clean has failed and retry\nit. But in non-interactive use, scripts could fail.\n\nAt least, I think that's what could be causing us to hit this error; I\nhaven't actually done any research to see if this is true.\n\n[1] https://github.com/pantsbuild/pants\n"},{"id":"261308","messageId":"xmqqbnhmiz78.fsf@gitster.dls.corp.google.com","threadId":"39342","inReplyTo":"1431640373.17436.16.camel@ubuntu","subject":"Re: [PATCH] builtin/clean.c: Handle disappearing files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-14T22:14:35Z","receivedAt":"2015-05-14T22:14:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n>> For this change to be an improvement that matters, you must be\n>> allowing some other process mucking around with the files in your\n>> working tree when you run \"git clean\". The original catches such\n>> situation as an anomaly that the user would want to be aware of (and\n>> investigate), but this patch essentially declares that having such a\n>> random process touching your working tree from sideways is a normal\n>> situation. I do not think I am willing to make such a declaration\n>> myself; I'd rather want to know why other people are touching my\n>> working tree while I am working in it.\n>\n> Our build tool[1] stores statistics in a hidden file in the working\n> tree. After it runs, it daemonizes and uploads those stats, and then\n> deletes the stats file. Because the upload might take some time, the\n> user might run git clean in the meantime. (I think there might be some\n> other deletion that happens in the background too; I haven't really\n> investigated).\n>\n> Of course, in interactive use, very little harm is done if clean dies\n> here: the user simply must notice that the clean has failed and retry\n> it. But in non-interactive use, scripts could fail.\n>\n> At least, I think that's what could be causing us to hit this error; I\n> haven't actually done any research to see if this is true.\n\nI find that the above argues that this patch is a bad idea.\n\nThe change sweeps the problem under the rug, killing the canary in\nthe mine, instead of motivating you to figure out why it is\nhappening to you.\n\nOf course, scripts could choose to ignore errors from \"git clean\" it\nruns. At that point, the \"sweeping under the rug\" ugliness is not\nin Git, which is better ;-).\n"},{"id":"261312","messageId":"1431642839.17436.22.camel@ubuntu","threadId":"39342","inReplyTo":"xmqqbnhmiz78.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] builtin/clean.c: Handle disappearing files","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-05-14T22:33:59Z","receivedAt":"2015-05-14T22:33:59Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Thu, 2015-05-14 at 15:14 -0700, Junio C Hamano wrote:\n> David Turner writes:\n> \n> >> For this change to be an improvement that matters, you must be\n> >> allowing some other process mucking around with the files in your\n> >> working tree when you run \"git clean\". The original catches such\n> >> situation as an anomaly that the user would want to be aware of (and\n> >> investigate), but this patch essentially declares that having such a\n> >> random process touching your working tree from sideways is a normal\n> >> situation. I do not think I am willing to make such a declaration\n> >> myself; I'd rather want to know why other people are touching my\n> >> working tree while I am working in it.\n> >\n> > Our build tool[1] stores statistics in a hidden file in the working\n> > tree. After it runs, it daemonizes and uploads those stats, and then\n> > deletes the stats file. Because the upload might take some time, the\n> > user might run git clean in the meantime. (I think there might be some\n> > other deletion that happens in the background too; I haven't really\n> > investigated).\n> >\n> > Of course, in interactive use, very little harm is done if clean dies\n> > here: the user simply must notice that the clean has failed and retry\n> > it. But in non-interactive use, scripts could fail.\n> >\n> > At least, I think that's what could be causing us to hit this error; I\n> > haven't actually done any research to see if this is true.\n> \n> I find that the above argues that this patch is a bad idea.\n> \n> The change sweeps the problem under the rug, killing the canary in\n> the mine, instead of motivating you to figure out why it is\n> happening to you.\n\nBut it's totally legit for processes to create and delete files in the\nworking tree at any time. Maybe I'm editing during the clean, and my\neditor creates a backup file (or creates a lock file which it's going to\nmove on top of my old file).\n\n> Of course, scripts could choose to ignore errors from \"git clean\" it\n> runs. At that point, the \"sweeping under the rug\" ugliness is not\n> in Git, which is better ;-).\n\nThe problem is that git clean does not finish the job it has done in\nthis case. So my script would have to retry on error (and every script\nthat git clean uses would have to so). \n"},{"id":"261319","messageId":"xmqqy4kqh0n8.fsf@gitster.dls.corp.google.com","threadId":"39342","inReplyTo":"1431642839.17436.22.camel@ubuntu","subject":"Re: [PATCH] builtin/clean.c: Handle disappearing files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-15T05:26:19Z","receivedAt":"2015-05-15T05:26:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n>> > Of course, in interactive use, very little harm is done if clean dies\n>> > here: the user simply must notice that the clean has failed and retry\n>> > it. But in non-interactive use, scripts could fail.\n>> >\n>> > At least, I think that's what could be causing us to hit this error; I\n>> > haven't actually done any research to see if this is true.\n>> \n>> I find that the above argues that this patch is a bad idea.\n>> \n>> The change sweeps the problem under the rug, killing the canary in\n>> the mine, instead of motivating you to figure out why it is\n>> happening to you.\n>\n> But it's totally legit for processes to create and delete files in the\n> working tree at any time. Maybe I'm editing during the clean, and my\n> editor creates a backup file (or creates a lock file which it's going to\n> move on top of my old file).\n\nIsn't that exactly what users need to be aware of the potential\nproblem waiting to bite them? Maybe you are editing during the\nclean, which may notice a funny file your editor creates and removes\nit, and when the editor wants to do something useful using that\nfunny file, it no longer is there. Sure, if your editor is quick\nenough and race with \"clean\" in certain timing, it may cause \"clean\"\nto notice and die. But if \"clean\" does not die, that means it won\nthe race and removed what your editor needed to use, no?\n\nIsn't it even worse for scripts? If your \"build statistics\" thing\ncreated a temporary before \"clean\" started to run (causing 'clean'\nto notice that as a cruft), and if \"clean\" gets to remove it before\nthe \"build statistics\" thing finishes what it was doing accumulating\nwhatever data in that file, when \"build statistics\" thing finally\ntries to use the file, it no longer is there.\n\nAnd isn't it even more worse for scripts that drive \"clean\"? By\nletting \"clean\" what it thinks cruft, while other processes are\n\"creating and deleting files in the working tree at any time\"\nwithout coordination, such a script is actively making the other\nprocesses unreliable. If \"clean\" in such a script stops as soon as\nit notices such a race before doing any more damage, it would be a\ngood thing. Retrying \"clean\" (to finish cleaning) does not sound\nlike a remedy; not running \"clean\" when other people are still using\nthe working tree is. It's like somebody is randomly running \"make\nclean\" in the background every once in a while in a working tree\nwhere I am trying to do a real work. Why is it \"totally legit\"?\nAnd hiding the problem by making \"clean\" ignore such a race would\nnot help the user to fix such a script, would it?\n\nPerhaps there is some assumption you have in the way the working\ntree is being used that I haven't considered, and it is entirely\npossible that this change may make sense under that special case,\nbut without knowing what that untold assumption is, the more I hear\nabout this topic from you, the less I am convinced that this is a\ngood change.\n"},{"id":"261357","messageId":"1431713612.17436.27.camel@ubuntu","threadId":"39342","inReplyTo":"xmqqy4kqh0n8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] builtin/clean.c: Handle disappearing files","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-05-15T18:13:32Z","receivedAt":"2015-05-15T18:13:32Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Thu, 2015-05-14 at 22:26 -0700, Junio C Hamano wrote:\n\n\nOK, I'm sold that this patch is not a great idea. But:\n\n> If your \"build statistics\" thing\n> created a temporary before \"clean\" started to run (causing 'clean'\n> to notice that as a cruft), and if \"clean\" gets to remove it before\n> the \"build statistics\" thing finishes what it was doing accumulating\n> whatever data in that file, when \"build statistics\" thing finally\n> tries to use the file, it no longer is there.\n\nOnly if the build statistics file is included in the pathspec for\nclean. \n\nSo maybe we should just move the lstat below the pathspec check. This\nis probably faster in some cases anyway. What do you think?\n"},{"id":"261395","messageId":"xmqqwq08e3bz.fsf@gitster.dls.corp.google.com","threadId":"39342","inReplyTo":"1431713612.17436.27.camel@ubuntu","subject":"Re: [PATCH] builtin/clean.c: Handle disappearing files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-16T19:21:04Z","receivedAt":"2015-05-16T19:21:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> So maybe we should just move the lstat below the pathspec check. This\n> is probably faster in some cases anyway. What do you think?\n\nI also was wondering about the placement of that lstat; because\nhaving it before the pathspec filtering didn't make sense to me at\nall. If we are not told to clean some parts of the tree, we really\nshouldn't be looking at that part of the tree. I think such a\nchange is desirable and the justification of the change should be\nbased solely on the above---limit first and then check, don't check\nwhat we were not told to look at.\n\nAnd if such a change lets you solve your original issue, that is a\nnice side effect I am happy to see. By excluding the paths that are\nunstable (because other processes randomly muck with them without\ncoordinating with the user) explicitly when running \"git clean\", the\nuser _is_ saying that s/he is aware of the issue and is actively\navoiding modifications by the third-party would not get in the way,\nso I wouldn't call that sweeping it under the rug, like I did for\nyour original one.\n\nThanks.\n"}]}