{"thread":{"id":"45125","subject":"[PATCH] clean: use warning_errno() when appropriate","startedAt":"2017-02-13T09:27:18Z","lastAt":"2017-02-15T01:36:27Z","messageCount":13,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Jeff King","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"311394","messageId":"20170213092702.10462-1-pclouds@gmail.com","threadId":"45125","inReplyTo":null,"subject":"[PATCH] clean: use warning_errno() when appropriate","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-02-13T09:27:02Z","receivedAt":"2017-02-13T09:27:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"All these warning() calls are preceded by a system call. Report the\nactual error to help the user understand why we fail to remove\nsomething.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/clean.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex d6bc3aaaea..dc1168747e 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -175,7 +175,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \t\tres = dry_run ? 0 : rmdir(path->buf);\n \t\tif (res) {\n \t\t\tquote_path_relative(path->buf, prefix, "ed);\n-\t\t\twarning(_(msg_warn_remove_failed), quoted.buf);\n+\t\t\twarning_errno(_(msg_warn_remove_failed), quoted.buf);\n \t\t\t*dir_gone = 0;\n \t\t}\n \t\treturn res;\n@@ -209,7 +209,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \t\t\t\tstring_list_append(&dels, quoted.buf);\n \t\t\t} else {\n \t\t\t\tquote_path_relative(path->buf, prefix, "ed);\n-\t\t\t\twarning(_(msg_warn_remove_failed), quoted.buf);\n+\t\t\t\twarning_errno(_(msg_warn_remove_failed), quoted.buf);\n \t\t\t\t*dir_gone = 0;\n \t\t\t\tret = 1;\n \t\t\t}\n@@ -231,7 +231,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \t\t\t*dir_gone = 1;\n \t\telse {\n \t\t\tquote_path_relative(path->buf, prefix, "ed);\n-\t\t\twarning(_(msg_warn_remove_failed), quoted.buf);\n+\t\t\twarning_errno(_(msg_warn_remove_failed), quoted.buf);\n \t\t\t*dir_gone = 0;\n \t\t\tret = 1;\n \t\t}\n@@ -982,7 +982,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t\tres = dry_run ? 0 : unlink(abs_path.buf);\n \t\t\tif (res) {\n \t\t\t\tqname = quote_path_relative(item->string, NULL, &buf);\n-\t\t\t\twarning(_(msg_warn_remove_failed), qname);\n+\t\t\t\twarning_errno(_(msg_warn_remove_failed), qname);\n \t\t\t\terrors++;\n \t\t\t} else if (!quiet) {\n \t\t\t\tqname = quote_path_relative(item->string, NULL, &buf);\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"311415","messageId":"xmqqwpcudjoh.fsf@gitster.mtv.corp.google.com","threadId":"45125","inReplyTo":"20170213092702.10462-1-pclouds@gmail.com","subject":"Re: [PATCH] clean: use warning_errno() when appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-13T18:34:06Z","receivedAt":"2017-02-13T18:34:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> All these warning() calls are preceded by a system call. Report the\n> actual error to help the user understand why we fail to remove\n> something.\n\nI think this patch is probably correct in the current code, but I\nsay this only after following what quote_path_relative() and\nrelative_path() that is called from it. These warnings are preceded\nby a call to a system library function, but it is not apparent that\nthey are immediately preceded without anything else that could have\nfailed in between.\n\n Side note. There are many calls into strbuf API in these two\n functions. Any calls to xmalloc() and friends made by strbuf\n functions may see ENOMEM from underlying malloc() and recover by\n releasing cached resources, by which time the original errno is\n unrecoverable. So the above \"probably correct\" is not strictly\n true.\n\nIf we care deeply enough that we want to reliably show the errno we\ngot from the preceding call to a system library function even after\nwhatever comes in between, I think you'd need the usual saved_errno\ntrick. Is that worth it?---I do not offhand have an opinion.\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin/clean.c | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> index d6bc3aaaea..dc1168747e 100644\n> --- a/builtin/clean.c\n> +++ b/builtin/clean.c\n> @@ -175,7 +175,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n> \t\tres = dry_run ? 0 : rmdir(path->buf);\n> \t\tif (res) {\n> \t\t\tquote_path_relative(path->buf, prefix, "ed);\n> -\t\t\twarning(_(msg_warn_remove_failed), quoted.buf);\n> +\t\t\twarning_errno(_(msg_warn_remove_failed), quoted.buf);\n> \t\t\t*dir_gone = 0;\n> \t\t}\n> \t\treturn res;\n> @@ -209,7 +209,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n> \t\t\t\tstring_list_append(&dels, quoted.buf);\n> \t\t\t} else {\n> \t\t\t\tquote_path_relative(path->buf, prefix, "ed);\n> -\t\t\t\twarning(_(msg_warn_remove_failed), quoted.buf);\n> +\t\t\t\twarning_errno(_(msg_warn_remove_failed), quoted.buf);\n> \t\t\t\t*dir_gone = 0;\n> \t\t\t\tret = 1;\n> \t\t\t}\n> @@ -231,7 +231,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n> \t\t\t*dir_gone = 1;\n> \t\telse {\n> \t\t\tquote_path_relative(path->buf, prefix, "ed);\n> -\t\t\twarning(_(msg_warn_remove_failed), quoted.buf);\n> +\t\t\twarning_errno(_(msg_warn_remove_failed), quoted.buf);\n> \t\t\t*dir_gone = 0;\n> \t\t\tret = 1;\n> \t\t}\n> @@ -982,7 +982,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n> \t\t\tres = dry_run ? 0 : unlink(abs_path.buf);\n> \t\t\tif (res) {\n> \t\t\t\tqname = quote_path_relative(item->string, NULL, &buf);\n> -\t\t\t\twarning(_(msg_warn_remove_failed), qname);\n> +\t\t\t\twarning_errno(_(msg_warn_remove_failed), qname);\n> \t\t\t\terrors++;\n> \t\t\t} else if (!quiet) {\n> \t\t\t\tqname = quote_path_relative(item->string, NULL, &buf);\n"},{"id":"311418","messageId":"20170213191433.muwhz7zem64p3rxr@sigill.intra.peff.net","threadId":"45125","inReplyTo":"xmqqwpcudjoh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] clean: use warning_errno() when appropriate","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-13T19:14:33Z","receivedAt":"2017-02-13T19:14:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 13, 2017 at 10:34:06AM -0800, Junio C Hamano wrote:\n\n> Nguyễn Thái Ngọc Duy writes:\n> \n> > All these warning() calls are preceded by a system call. Report the\n> > actual error to help the user understand why we fail to remove\n> > something.\n> \n> I think this patch is probably correct in the current code, but I\n> say this only after following what quote_path_relative() and\n> relative_path() that is called from it. These warnings are preceded\n> by a call to a system library function, but it is not apparent that\n> they are immediately preceded without anything else that could have\n> failed in between.\n> \n> Side note. There are many calls into strbuf API in these two\n> functions. Any calls to xmalloc() and friends made by strbuf\n> functions may see ENOMEM from underlying malloc() and recover by\n> releasing cached resources, by which time the original errno is\n> unrecoverable. So the above \"probably correct\" is not strictly\n> true.\n> \n> If we care deeply enough that we want to reliably show the errno we\n> got from the preceding call to a system library function even after\n> whatever comes in between, I think you'd need the usual saved_errno\n> trick. Is that worth it?---I do not offhand have an opinion.\n\nI wonder if xmalloc() should be the one doing the saved_errno trick.\nAfter all, it has only two outcomes: we successfully allocated the\nmemory, or we called die().\n\nAnd that would transitively make most of the strbuf calls errno-safe\n(except for obvious syscall-related ones like strbuf_read_file). And in\nturn that makes quote_path_relative() pretty safe (at least when writing\nto a strbuf).\n\n-Peff\n"},{"id":"311434","messageId":"xmqqk28tddwo.fsf@gitster.mtv.corp.google.com","threadId":"45125","inReplyTo":"20170213191433.muwhz7zem64p3rxr@sigill.intra.peff.net","subject":"Re: [PATCH] clean: use warning_errno() when appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-13T20:38:47Z","receivedAt":"2017-02-13T20:38:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I wonder if xmalloc() should be the one doing the saved_errno trick.\n> After all, it has only two outcomes: we successfully allocated the\n> memory, or we called die().\n\nI would be lying if I said I did not considered it when I wrote the\nmessage you are responding to, but I rejected it because that would\nbe optimizing for a wrong case, in that most callers of xmalloc()\nand friends do not do so in the error codepath, and we would be\npenalizing them by doing the saved_errno dance unconditionally.\n\n"},{"id":"311445","messageId":"20170213211051.vsnnrtcsvuvfcwyk@sigill.intra.peff.net","threadId":"45125","inReplyTo":"xmqqk28tddwo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] clean: use warning_errno() when appropriate","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-13T21:10:52Z","receivedAt":"2017-02-13T21:11:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 13, 2017 at 12:38:47PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I wonder if xmalloc() should be the one doing the saved_errno trick.\n> > After all, it has only two outcomes: we successfully allocated the\n> > memory, or we called die().\n> \n> I would be lying if I said I did not considered it when I wrote the\n> message you are responding to, but I rejected it because that would\n> be optimizing for a wrong case, in that most callers of xmalloc()\n> and friends do not do so in the error codepath, and we would be\n> penalizing them by doing the saved_errno dance unconditionally.\n\nYes, that also occurred to me. I'm not sure if two integer swaps is\nenough to care about when compared to the cost of a malloc(), though.\n\nIOW, I think this may be a case where we should be optimizing for\nprogrammer time (fewer lines of code, and one less thing to worry about\nin the callers) versus squeezing out every instruction.\n\n-Peff\n"},{"id":"311454","messageId":"xmqqy3x9bvvm.fsf@gitster.mtv.corp.google.com","threadId":"45125","inReplyTo":"20170213211051.vsnnrtcsvuvfcwyk@sigill.intra.peff.net","subject":"Re: [PATCH] clean: use warning_errno() when appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-13T21:53:33Z","receivedAt":"2017-02-13T21:53:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> IOW, I think this may be a case where we should be optimizing for\n> programmer time (fewer lines of code, and one less thing to worry about\n> in the callers) versus squeezing out every instruction.\n\nFair enough.\n\nUnless we do the save_errno dance in all the helper functions we\ncommonly use to safely stash away errno as necessary and tell\ndevelopers that they can depend on it, the code in the patch that\nbegan this discussion still needs its own saved_errno dance to be\nsafe, though. I do not have a feeling that we are not there yet,\neven after we teach xmalloc() and its family to do so.\n\n"},{"id":"311458","messageId":"20170213222210.dtfnxbbi77tsx4a6@sigill.intra.peff.net","threadId":"45125","inReplyTo":"xmqqy3x9bvvm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] clean: use warning_errno() when appropriate","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-13T22:22:10Z","receivedAt":"2017-02-13T22:22:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 13, 2017 at 01:53:33PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > IOW, I think this may be a case where we should be optimizing for\n> > programmer time (fewer lines of code, and one less thing to worry about\n> > in the callers) versus squeezing out every instruction.\n> \n> Fair enough.\n> \n> Unless we do the save_errno dance in all the helper functions we\n> commonly use to safely stash away errno as necessary and tell\n> developers that they can depend on it, the code in the patch that\n> began this discussion still needs its own saved_errno dance to be\n> safe, though. I do not have a feeling that we are not there yet,\n> even after we teach xmalloc() and its family to do so.\n\nYeah, I certainly agree that is a potential blocker. Even if it is true\ntoday, there's nothing guaranteeing that the quote functions don't grow\na new internal detail that violates.\n\nSo in that sense doing the errno dance as close to the caller who cares\nis the most _obvious_ thing, even if it isn't the shortest.\n\nIt would be nice if there was a way to annotate a function as\nerrno-safe, and then transitively compute which other functions were\nerrno-safe when they do not call any errno-unsafe function. I don't know\nif any static analyzers allow that kind of custom annotation, though\n(and also I wonder whether the cost/benefit of maintaining those\nannotations would be worth it).\n\n-Peff\n"},{"id":"311476","messageId":"xmqq1sv1bqko.fsf@gitster.mtv.corp.google.com","threadId":"45125","inReplyTo":"20170213222210.dtfnxbbi77tsx4a6@sigill.intra.peff.net","subject":"Re: [PATCH] clean: use warning_errno() when appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-13T23:48:07Z","receivedAt":"2017-02-13T23:48:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So in that sense doing the errno dance as close to the caller who cares\n> is the most _obvious_ thing, even if it isn't the shortest.\n\nYup.\n\n> It would be nice if there was a way to annotate a function as\n> errno-safe, and then transitively compute which other functions were\n> errno-safe when they do not call any errno-unsafe function. I don't know\n> if any static analyzers allow that kind of custom annotation, though\n> (and also I wonder whether the cost/benefit of maintaining those\n> annotations would be worth it).\n\nDouble yup.\n"},{"id":"311531","messageId":"20170214095449.15585-1-pclouds@gmail.com","threadId":"45125","inReplyTo":"20170213092702.10462-1-pclouds@gmail.com","subject":"[PATCH v2] clean: use warning_errno() when appropriate","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-02-14T09:54:49Z","receivedAt":"2017-02-14T09:56:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"All these warning() calls are preceded by a system call. Report the\nactual error to help the user understand why we fail to remove\nsomething.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n v2 dances with errno\n\n builtin/clean.c | 19 ++++++++++++++-----\n 1 file changed, 14 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex d6bc3aaae..3569736f6 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -154,6 +154,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \tstruct strbuf quoted = STRBUF_INIT;\n \tstruct dirent *e;\n \tint res = 0, ret = 0, gone = 1, original_len = path->len, len;\n+\tint saved_errno;\n \tstruct string_list dels = STRING_LIST_INIT_DUP;\n \n \t*dir_gone = 1;\n@@ -173,9 +174,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \tif (!dir) {\n \t\t/* an empty dir could be removed even if it is unreadble */\n \t\tres = dry_run ? 0 : rmdir(path->buf);\n+\t\tsaved_errno = errno;\n \t\tif (res) {\n \t\t\tquote_path_relative(path->buf, prefix, "ed);\n-\t\t\twarning(_(msg_warn_remove_failed), quoted.buf);\n+\t\t\terrno = saved_errno;\n+\t\t\twarning_errno(_(msg_warn_remove_failed), quoted.buf);\n \t\t\t*dir_gone = 0;\n \t\t}\n \t\treturn res;\n@@ -204,12 +207,14 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \t\t\tcontinue;\n \t\t} else {\n \t\t\tres = dry_run ? 0 : unlink(path->buf);\n+\t\t\tsaved_errno = errno;\n \t\t\tif (!res) {\n \t\t\t\tquote_path_relative(path->buf, prefix, "ed);\n \t\t\t\tstring_list_append(&dels, quoted.buf);\n \t\t\t} else {\n \t\t\t\tquote_path_relative(path->buf, prefix, "ed);\n-\t\t\t\twarning(_(msg_warn_remove_failed), quoted.buf);\n+\t\t\t\terrno = saved_errno;\n+\t\t\t\twarning_errno(_(msg_warn_remove_failed), quoted.buf);\n \t\t\t\t*dir_gone = 0;\n \t\t\t\tret = 1;\n \t\t\t}\n@@ -227,11 +232,13 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \n \tif (*dir_gone) {\n \t\tres = dry_run ? 0 : rmdir(path->buf);\n+\t\tsaved_errno = errno;\n \t\tif (!res)\n \t\t\t*dir_gone = 1;\n \t\telse {\n \t\t\tquote_path_relative(path->buf, prefix, "ed);\n-\t\t\twarning(_(msg_warn_remove_failed), quoted.buf);\n+\t\t\terrno = saved_errno;\n+\t\t\twarning_errno(_(msg_warn_remove_failed), quoted.buf);\n \t\t\t*dir_gone = 0;\n \t\t\tret = 1;\n \t\t}\n@@ -853,7 +860,7 @@ static void interactive_main_loop(void)\n \n int cmd_clean(int argc, const char **argv, const char *prefix)\n {\n-\tint i, res;\n+\tint i, res, saved_errno;\n \tint dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;\n \tint ignored_only = 0, config_set = 0, errors = 0, gone = 1;\n \tint rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;\n@@ -980,9 +987,11 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t} else {\n \t\t\tres = dry_run ? 0 : unlink(abs_path.buf);\n+\t\t\tsaved_errno = errno;\n \t\t\tif (res) {\n \t\t\t\tqname = quote_path_relative(item->string, NULL, &buf);\n-\t\t\t\twarning(_(msg_warn_remove_failed), qname);\n+\t\t\t\terrno = saved_errno;\n+\t\t\t\twarning_errno(_(msg_warn_remove_failed), qname);\n \t\t\t\terrors++;\n \t\t\t} else if (!quiet) {\n \t\t\t\tqname = quote_path_relative(item->string, NULL, &buf);\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"311552","messageId":"xmqqh93wabev.fsf@gitster.mtv.corp.google.com","threadId":"45125","inReplyTo":"20170214095449.15585-1-pclouds@gmail.com","subject":"Re: [PATCH v2] clean: use warning_errno() when appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-14T18:13:12Z","receivedAt":"2017-02-14T18:13:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> All these warning() calls are preceded by a system call. Report the\n> actual error to help the user understand why we fail to remove\n> something.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> v2 dances with errno\n\nThanks.\n\n>\n> builtin/clean.c | 19 ++++++++++++++-----\n> 1 file changed, 14 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> index d6bc3aaae..3569736f6 100644\n> --- a/builtin/clean.c\n> +++ b/builtin/clean.c\n> @@ -154,6 +154,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n> \tstruct strbuf quoted = STRBUF_INIT;\n> \tstruct dirent *e;\n> \tint res = 0, ret = 0, gone = 1, original_len = path->len, len;\n> +\tint saved_errno;\n> \tstruct string_list dels = STRING_LIST_INIT_DUP;\n> \n> \t*dir_gone = 1;\n> @@ -173,9 +174,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n> \tif (!dir) {\n> \t\t/* an empty dir could be removed even if it is unreadble */\n> \t\tres = dry_run ? 0 : rmdir(path->buf);\n> +\t\tsaved_errno = errno;\n> \t\tif (res) {\n> \t\t\tquote_path_relative(path->buf, prefix, "ed);\n\nI think this part should be more like\n\n\t\tres = ... : rmdir(...);\n\t\tif (res) {\n\t\t\tint saved_errno = errno;\n\t\t\t... do other things that can touch errno ...\n\t\t\terrno = saved_errno;\n\t\t\t... now we know what the original error was ...\n\nThe reason to store the errno in saved_errno here is not because we\nwant to help code after \"if (res) {...}\", but the patch sent as-is\ngives that impression and is confusing to the readers. \n\nPerhaps all hunks of this patch share the same issue? I could\nlocally amend, of course, but I'd like to double check before doing\nso myself---perhaps you did it this way for a good reason that I am\nmissing?\n"},{"id":"311626","messageId":"CACsJy8BXSAUr2knrkOfO0gXYAwQoJpL2hCXy44Q37H4GE_-yVA@mail.gmail.com","threadId":"45125","inReplyTo":"xmqqh93wabev.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] clean: use warning_errno() when appropriate","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-02-15T00:49:48Z","receivedAt":"2017-02-15T00:51:28Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 15, 2017 at 1:13 AM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> All these warning() calls are preceded by a system call. Report the\n>> actual error to help the user understand why we fail to remove\n>> something.\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>> v2 dances with errno\n>\n> Thanks.\n>\n>>\n>> builtin/clean.c | 19 ++++++++++++++-----\n>> 1 file changed, 14 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/builtin/clean.c b/builtin/clean.c\n>> index d6bc3aaae..3569736f6 100644\n>> --- a/builtin/clean.c\n>> +++ b/builtin/clean.c\n>> @@ -154,6 +154,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n>> struct strbuf quoted = STRBUF_INIT;\n>> struct dirent *e;\n>> int res = 0, ret = 0, gone = 1, original_len = path->len, len;\n>> + int saved_errno;\n>> struct string_list dels = STRING_LIST_INIT_DUP;\n>>\n>> *dir_gone = 1;\n>> @@ -173,9 +174,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n>> if (!dir) {\n>> /* an empty dir could be removed even if it is unreadble */\n>> res = dry_run ? 0 : rmdir(path->buf);\n>> + saved_errno = errno;\n>> if (res) {\n>> quote_path_relative(path->buf, prefix, "ed);\n>\n> I think this part should be more like\n>\n> res = ... : rmdir(...);\n> if (res) {\n> int saved_errno = errno;\n> ... do other things that can touch errno ...\n> errno = saved_errno;\n> ... now we know what the original error was ...\n>\n> The reason to store the errno in saved_errno here is not because we\n> want to help code after \"if (res) {...}\", but the patch sent as-is\n> gives that impression and is confusing to the readers.\n>\n> Perhaps all hunks of this patch share the same issue? I could\n> locally amend, of course, but I'd like to double check before doing\n> so myself---perhaps you did it this way for a good reason that I am\n> missing?\n\nOne thing I like about putting saved_errno right next to the related\nsyscall is, the syscall is visible from the diff (previously some are\nout of context). This is really minor though. I briefly thought of\nintroducing rmdir_errno() and friends that return -errno on error, so\nwe could do\n\nres = ... : rmdir_errno(..);\nif (res) {\n errno = -res;\n warning_errno(...);\n}\n\nBut that's more work and the errno = -res is not particularly pleasing\nto read. I'm fine with moving saved_errno in the error handling\nblocks.\n-- \nDuy\n"},{"id":"311628","messageId":"xmqqfujg5jjv.fsf@gitster.mtv.corp.google.com","threadId":"45125","inReplyTo":"CACsJy8BXSAUr2knrkOfO0gXYAwQoJpL2hCXy44Q37H4GE_-yVA@mail.gmail.com","subject":"Re: [PATCH v2] clean: use warning_errno() when appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-15T01:28:36Z","receivedAt":"2017-02-15T01:28:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Wed, Feb 15, 2017 at 1:13 AM, Junio C Hamano wrote:\n> ...\n>> The reason to store the errno in saved_errno here is not because we\n>> want to help code after \"if (res) {...}\", but the patch sent as-is\n>> gives that impression and is confusing to the readers.\n>>\n>> Perhaps all hunks of this patch share the same issue? I could\n>> locally amend, of course, but I'd like to double check before doing\n>> so myself---perhaps you did it this way for a good reason that I am\n>> missing?\n>\n> One thing I like about putting saved_errno right next to the related\n> syscall is, the syscall is visible from the diff (previously some are\n> out of context). This is really minor though.\n\nI agree that this is minor.\n\nI care more about looking at errno ONLY after we saw our call to a\nsystem library function indicated an error, and I wanted to avoid\ndoing:\n\n\tres = dry_run ? 0 : rmdir(path);\n saved_errno = errno;\n\tif (res) {\n\t\t... do something else ...\n\t\terrno = saved_errno;\n call_something_that_uses_errno();\n\nWhen our call to rmdir() did not fail, or when we didn't even call\nrmdir() at all, what is in errno has nothing to do with what we are\ndoing, and making a copy of it makes the code doubly confusing.\n\nRather, I'd prefer to see:\n\n\tres = dry_run ? 0 : rmdir(path);\n\tif (res) {\n int saved_errno = errno;\n\t\t... do something else ...\n\t\terrno = saved_errno;\n call_something_that_uses_errno();\n\nwhich makes it clear what is going on when reading the resulting\ncode.\n\nFor now, I'll queue a separate SQUASH??? and wait for others to\ncomment.\n\nThanks.\n"},{"id":"311630","messageId":"20170215013620.uec3fbmrcafwas6s@sigill.intra.peff.net","threadId":"45125","inReplyTo":"xmqqfujg5jjv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] clean: use warning_errno() when appropriate","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-15T01:36:20Z","receivedAt":"2017-02-15T01:36:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 14, 2017 at 05:28:36PM -0800, Junio C Hamano wrote:\n\n> I care more about looking at errno ONLY after we saw our call to a\n> system library function indicated an error, and I wanted to avoid\n> doing:\n> \n> \tres = dry_run ? 0 : rmdir(path);\n> saved_errno = errno;\n> \tif (res) {\n> \t\t... do something else ...\n> \t\terrno = saved_errno;\n> call_something_that_uses_errno();\n> \n> When our call to rmdir() did not fail, or when we didn't even call\n> rmdir() at all, what is in errno has nothing to do with what we are\n> doing, and making a copy of it makes the code doubly confusing.\n> \n> Rather, I'd prefer to see:\n> \n> \tres = dry_run ? 0 : rmdir(path);\n> \tif (res) {\n> int saved_errno = errno;\n> \t\t... do something else ...\n> \t\terrno = saved_errno;\n> call_something_that_uses_errno();\n> \n> which makes it clear what is going on when reading the resulting\n> code.\n> \n> For now, I'll queue a separate SQUASH??? and wait for others to\n> comment.\n\nI don't have a strong feeling either way, but I think your second\nexample there is probably preferable. The reason to save errno is\nbecause of the \"something else\" that may affect it, and it puts the\nsaving close to that.\n\nDuy's version above keeps the saved_errno close to the syscall that\ncaused it, which is nicer for making sure we're saving the right thing,\nand didn't get fooled by:\n\n res = rmdir(path);\n ... some other stuff ...\n if (res) {\n int saved_errno = errno;\n\t ... something else ...\n\t errno = saved_errno;\n\nThat's wrong if \"some other stuff\" touches errno. But I think\n\"saved_errno\" is not the right pattern there. It is \"stuff away the\nresult _and_ errno for this thing so we can use it later\".\n\nIOW, I'd expect it to be more like:\n\n rmdir_result = rmdir(path);\n rmdir_errno = errno;\n ... some other stuff ...\n if (rmdir_result)\n show_error(rmdir_errno);\n\nAnd that leads to the \"gee, why don't we just encode error values as\nnegative integers\" pattern. Which I agree is nicer, but I'm not sure I\nwant to get into wrapping every syscall to give it a better interface.\n\n-Peff\n"}]}