{"thread":{"id":"38097","subject":"[PATCH/RFC] rerere: error out on autoupdate failure","startedAt":"2014-12-03T04:20:49Z","lastAt":"2014-12-03T18:52:16Z","messageCount":4,"participants":["Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"252919","messageId":"20141203042049.GI6527@google.com","threadId":"38097","inReplyTo":null,"subject":"[PATCH/RFC] rerere: error out on autoupdate failure","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T04:20:49Z","receivedAt":"2014-12-03T04:20:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"We have been silently tolerating errors by returning early with an\nerror that the caller ignores since rerere.autoupdate was introduced\nin v1.6.0-rc0~120^2 (2008-06-22).  So on error (for example if the\nindex is already locked), rerere can return success silently without\nupdating the index or with only some items in the index updated.\n\nBetter to treat such failures as a fatal error so the operator can\nfigure out what is wrong and fix it.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nI ran into this while auditing hold_locked_index callers.  Tests\nstill pass after this change.  Sensible?\n\n rerere.c | 14 +++++---------\n 1 file changed, 5 insertions(+), 9 deletions(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex 1b0555f..195f663 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -477,27 +477,23 @@ out:\n \n static struct lock_file index_lock;\n \n-static int update_paths(struct string_list *update)\n+static void update_paths(struct string_list *update)\n {\n \tint i;\n-\tint fd = hold_locked_index(&index_lock, 0);\n-\tint status = 0;\n \n-\tif (fd < 0)\n-\t\treturn -1;\n+\thold_locked_index(&index_lock, 1);\n \n \tfor (i = 0; i < update->nr; i++) {\n \t\tstruct string_list_item *item = &update->items[i];\n \t\tif (add_file_to_cache(item->string, ADD_CACHE_IGNORE_ERRORS))\n-\t\t\tstatus = -1;\n+\t\t\tdie(\"staging updated %s failed\", item->string);\n \t}\n \n-\tif (!status && active_cache_changed) {\n+\tif (active_cache_changed) {\n \t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n \t\t\tdie(\"Unable to write new index file\");\n-\t} else if (fd >= 0)\n+\t} else\n \t\trollback_lock_file(&index_lock);\n-\treturn status;\n }\n \n static int do_plain_rerere(struct string_list *rr, int fd)\n"},{"id":"252963","messageId":"xmqqy4qoiqmu.fsf@gitster.dls.corp.google.com","threadId":"38097","inReplyTo":"20141203042049.GI6527@google.com","subject":"Re: [PATCH/RFC] rerere: error out on autoupdate failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T17:34:33Z","receivedAt":"2014-12-03T17:34:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> We have been silently tolerating errors by returning early with an\n> error that the caller ignores since rerere.autoupdate was introduced\n> in v1.6.0-rc0~120^2 (2008-06-22).  So on error (for example if the\n> index is already locked), rerere can return success silently without\n> updating the index or with only some items in the index updated.\n>\n> Better to treat such failures as a fatal error so the operator can\n> figure out what is wrong and fix it.\n>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> I ran into this while auditing hold_locked_index callers.  Tests\n> still pass after this change.  Sensible?\n\nI think what the change wants to do is sensible, but \"tests still\npass\" probably does not tell us very much.  You'd need a competing\nwriters (e.g. in real life a human user may open two terminals and\ntry to do things in the same repository from both terminals without\nrealizing that she is stomping on herself), which is expected to\nbreak in unexpected ways ;-), which is not something the existing\ntests would try to catch anyway.\n\n>  rerere.c | 14 +++++---------\n>  1 file changed, 5 insertions(+), 9 deletions(-)\n>\n> diff --git a/rerere.c b/rerere.c\n> index 1b0555f..195f663 100644\n> --- a/rerere.c\n> +++ b/rerere.c\n> @@ -477,27 +477,23 @@ out:\n>  \n>  static struct lock_file index_lock;\n>  \n> -static int update_paths(struct string_list *update)\n> +static void update_paths(struct string_list *update)\n>  {\n>  \tint i;\n> -\tint fd = hold_locked_index(&index_lock, 0);\n> -\tint status = 0;\n>  \n> -\tif (fd < 0)\n> -\t\treturn -1;\n> +\thold_locked_index(&index_lock, 1);\n>  \n>  \tfor (i = 0; i < update->nr; i++) {\n>  \t\tstruct string_list_item *item = &update->items[i];\n>  \t\tif (add_file_to_cache(item->string, ADD_CACHE_IGNORE_ERRORS))\n> -\t\t\tstatus = -1;\n> +\t\t\tdie(\"staging updated %s failed\", item->string);\n\nInstead of crafting a new message, why not just stop passing IGNORE_ERRORS\nand have add_file_to_cache() report the failure?  That is:\n\n\tif (add_file_to_cache(item->string, 0))\n        \treturn -1;\n\nThat way, the user will get more useful diagnosis because there are\ndifferent reasons why an \"add\" may fail and we give different error\nmessages to them.\n"},{"id":"252965","messageId":"xmqqppc0iqb7.fsf@gitster.dls.corp.google.com","threadId":"38097","inReplyTo":"xmqqy4qoiqmu.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFC] rerere: error out on autoupdate failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T17:41:32Z","receivedAt":"2014-12-03T17:41:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Instead of crafting a new message, why not just stop passing IGNORE_ERRORS\n> and have add_file_to_cache() report the failure?  That is:\n>\n> \tif (add_file_to_cache(item->string, 0))\n>         \treturn -1;\n\nErr, that should be exit(128) to mimic die().\n\n> That way, the user will get more useful diagnosis because there are\n> different reasons why an \"add\" may fail and we give different error\n> messages to them.\n"},{"id":"252972","messageId":"20141203185216.GA31609@google.com","threadId":"38097","inReplyTo":"xmqqy4qoiqmu.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2] rerere: error out on autoupdate failure","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T18:52:16Z","receivedAt":"2014-12-03T18:52:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"We have been tolerating errors by returning early with an error that\nthe caller ignores since rerere.autoupdate was introduced in\nv1.6.0-rc0~120^2 (2008-06-22).  So on error (for example if the index\nis already locked), rerere can return success without updating the\nindex or with only some items in the index updated.\n\nBetter to treat such failures as a fatal error so the operator can\nfigure out what is wrong and fix it.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nJunio C Hamano wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>>  \tfor (i = 0; i < update->nr; i++) {\n>>  \t\tstruct string_list_item *item = &update->items[i];\n>>  \t\tif (add_file_to_cache(item->string, ADD_CACHE_IGNORE_ERRORS))\n>> -\t\t\tstatus = -1;\n>> +\t\t\tdie(\"staging updated %s failed\", item->string);\n>\n> Instead of crafting a new message, why not just stop passing IGNORE_ERRORS\n> and have add_file_to_cache() report the failure?  That is:\n> \n> \tif (add_file_to_cache(item->string, 0))\n>         \treturn -1;\n\nGood catch.  It turns out that add_file_to_index does not even pay\nattention to the IGNORE_ERRORS flag.  On error it unconditionally\nprints a message using 'return error()'.\n\nbuiltin/add.c uses the IGNORE_ERRORS to flag to decide whether to\navoid die()-ing on error (and it still lets add_file_to_index print the\nerror to stderr in that case, so --ignore-errors is a bit of a misnomer).\n\n\tif (add_file_to_cache(dir->entries[i]->name, flags)) {\n\t\tif (!ignore_add_errors)\n\t\t\tdie(_(\"adding files failed\"));\n\t\texit_status = 1;\n\t}\n\nThe message always says which file it failed to index, so I agree that\nit makes sense to simply exit(128) here.\n\n rerere.c | 21 ++++++++++-----------\n 1 file changed, 10 insertions(+), 11 deletions(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex 1b0555f..dbb2545 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -477,27 +477,26 @@ out:\n \n static struct lock_file index_lock;\n \n-static int update_paths(struct string_list *update)\n+static void update_paths(struct string_list *update)\n {\n \tint i;\n-\tint fd = hold_locked_index(&index_lock, 0);\n-\tint status = 0;\n \n-\tif (fd < 0)\n-\t\treturn -1;\n+\thold_locked_index(&index_lock, 1);\n \n \tfor (i = 0; i < update->nr; i++) {\n-\t\tstruct string_list_item *item = &update->items[i];\n-\t\tif (add_file_to_cache(item->string, ADD_CACHE_IGNORE_ERRORS))\n-\t\t\tstatus = -1;\n+\t\tif (add_file_to_cache(update->items[i].string, 0))\n+\t\t\t/*\n+\t\t\t * add_file_to_cache() printed an error\n+\t\t\t * with the pathname, so we don't have to.\n+\t\t\t */\n+\t\t\texit(128);\n \t}\n \n-\tif (!status && active_cache_changed) {\n+\tif (active_cache_changed) {\n \t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n \t\t\tdie(\"Unable to write new index file\");\n-\t} else if (fd >= 0)\n+\t} else\n \t\trollback_lock_file(&index_lock);\n-\treturn status;\n }\n \n static int do_plain_rerere(struct string_list *rr, int fd)\n"}]}