{"thread":{"id":"7144","subject":"[PATCH 4/5] Refactor handling of error_string in receive-pack","startedAt":"2007-03-07T21:51:59Z","lastAt":"2007-03-07T21:51:59Z","messageCount":1,"participants":["Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"36573","messageId":"20070307215159.GD28649@spearce.org","threadId":"7144","inReplyTo":null,"subject":"[PATCH 4/5] Refactor handling of error_string in receive-pack","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-03-07T21:51:59Z","receivedAt":"2007-03-07T21:51:59Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"I discovered we did not send an ng line in the report-status feedback\nif the ref was not updated because the repository has the config\noption receive.denyNonFastForwards enabled.  I think the reason this\nhappened is that it is simply too easy to forget to set error_string\nwhen returning back a failure from update()\n\nWe now return an ng line for a non-fastforward update, which in\nturn will cause send-pack to exit with a non-zero exit status.\nHence the modified test.\n\nThis refactoring changes update to return a const char* describing\nthe error, which execute_commands always loads into error_string.\nThe result is what I think is cleaner code, and allows us to\ninitialize the error_string member to NULL when we read_head_info.\n\nI want error_string to be NULL in all commands before we call\nexecute_commands, so that we can reuse the run_hook function to\nexecute a new pre-receive hook.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n receive-pack.c       |   62 ++++++++++++++++++++++++++-----------------------\n t/t5400-send-pack.sh |    4 +-\n 2 files changed, 35 insertions(+), 31 deletions(-)\n\ndiff --git a/receive-pack.c b/receive-pack.c\nindex c55d905..ff746e7 100644\n--- a/receive-pack.c\n+++ b/receive-pack.c\n@@ -127,24 +127,22 @@ static int run_hook(const char *hook_name,\n \t}\n }\n \n-static int update(struct command *cmd)\n+static const char *update(struct command *cmd)\n {\n \tconst char *name = cmd->ref_name;\n \tunsigned char *old_sha1 = cmd->old_sha1;\n \tunsigned char *new_sha1 = cmd->new_sha1;\n \tstruct ref_lock *lock;\n \n-\tcmd->error_string = NULL;\n \tif (!prefixcmp(name, \"refs/\") && check_ref_format(name + 5)) {\n-\t\tcmd->error_string = \"funny refname\";\n-\t\treturn error(\"refusing to create funny ref '%s' locally\",\n-\t\t\t     name);\n+\t\terror(\"refusing to create funny ref '%s' locally\", name);\n+\t\treturn \"funny refname\";\n \t}\n \n \tif (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {\n-\t\tcmd->error_string = \"bad pack\";\n-\t\treturn error(\"unpack should have generated %s, \"\n-\t\t\t     \"but I can't find it!\", sha1_to_hex(new_sha1));\n+\t\terror(\"unpack should have generated %s, \"\n+\t\t      \"but I can't find it!\", sha1_to_hex(new_sha1));\n+\t\treturn \"bad pack\";\n \t}\n \tif (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&\n \t    !is_null_sha1(old_sha1) &&\n@@ -159,37 +157,39 @@ static int update(struct command *cmd)\n \t\t\tif (!hashcmp(old_sha1, ent->item->object.sha1))\n \t\t\t\tbreak;\n \t\tfree_commit_list(bases);\n-\t\tif (!ent)\n-\t\t\treturn error(\"denying non-fast forward;\"\n-\t\t\t\t     \" you should pull first\");\n+\t\tif (!ent) {\n+\t\t\terror(\"denying non-fast forward %s\"\n+\t\t\t      \" (you should pull first)\", name);\n+\t\t\treturn \"non-fast forward\";\n+\t\t}\n \t}\n \tif (run_hook(update_hook, cmd, 1)) {\n-\t\tcmd->error_string = \"hook declined\";\n-\t\treturn error(\"hook declined to update %s\", name);\n+\t\terror(\"hook declined to update %s\", name);\n+\t\treturn \"hook declined\";\n \t}\n \n \tif (is_null_sha1(new_sha1)) {\n \t\tif (delete_ref(name, old_sha1)) {\n-\t\t\tcmd->error_string = \"failed to delete\";\n-\t\t\treturn error(\"failed to delete %s\", name);\n+\t\t\terror(\"failed to delete %s\", name);\n+\t\t\treturn \"failed to delete\";\n \t\t}\n \t\tfprintf(stderr, \"%s: %s -> deleted\\n\", name,\n \t\t\tsha1_to_hex(old_sha1));\n+\t\treturn NULL; /* good */\n \t}\n \telse {\n \t\tlock = lock_any_ref_for_update(name, old_sha1);\n \t\tif (!lock) {\n-\t\t\tcmd->error_string = \"failed to lock\";\n-\t\t\treturn error(\"failed to lock %s\", name);\n+\t\t\terror(\"failed to lock %s\", name);\n+\t\t\treturn \"failed to lock\";\n \t\t}\n \t\tif (write_ref_sha1(lock, new_sha1, \"push\")) {\n-\t\t\tcmd->error_string = \"failed to write\";\n-\t\t\treturn -1; /* error() already called */\n+\t\t\treturn \"failed to write\"; /* error() already called */\n \t\t}\n \t\tfprintf(stderr, \"%s: %s -> %s\\n\", name,\n \t\t\tsha1_to_hex(old_sha1), sha1_to_hex(new_sha1));\n+\t\treturn NULL; /* good */\n \t}\n-\treturn 0;\n }\n \n static char update_post_hook[] = \"hooks/post-update\";\n@@ -224,15 +224,20 @@ static void run_update_post_hook(struct command *cmd)\n \t\t| RUN_COMMAND_STDOUT_TO_STDERR);\n }\n \n-/*\n- * This gets called after(if) we've successfully\n- * unpacked the data payload.\n- */\n-static void execute_commands(void)\n+static void execute_commands(const char *unpacker_error)\n {\n \tstruct command *cmd = commands;\n+\n+\tif (unpacker_error) {\n+\t\twhile (cmd) {\n+\t\t\tcmd->error_string = \"n/a (unpacker error)\";\n+\t\t\tcmd = cmd->next;\n+\t\t}\n+\t\treturn;\n+\t}\n+\n \twhile (cmd) {\n-\t\tupdate(cmd);\n+\t\tcmd->error_string = update(cmd);\n \t\tcmd = cmd->next;\n \t}\n }\n@@ -270,7 +275,7 @@ static void read_head_info(void)\n \t\thashcpy(cmd->old_sha1, old_sha1);\n \t\thashcpy(cmd->new_sha1, new_sha1);\n \t\tmemcpy(cmd->ref_name, line + 82, len - 81);\n-\t\tcmd->error_string = \"n/a (unpacker error)\";\n+\t\tcmd->error_string = NULL;\n \t\tcmd->next = NULL;\n \t\t*p = cmd;\n \t\tp = &cmd->next;\n@@ -473,8 +478,7 @@ int main(int argc, char **argv)\n \n \t\tif (!delete_only(commands))\n \t\t\tunpack_status = unpack();\n-\t\tif (!unpack_status)\n-\t\t\texecute_commands();\n+\t\texecute_commands(unpack_status);\n \t\tif (pack_lockfile)\n \t\t\tunlink(pack_lockfile);\n \t\tif (report_status)\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex fc4a126..477b267 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -108,8 +108,8 @@ test_expect_success \\\n \tcd victim &&\n \tgit-config receive.denyNonFastforwards true &&\n \tcd .. &&\n-\tgit-update-ref refs/heads/master master^ &&\n-\tgit-send-pack --force ./victim/.git/ master &&\n+\tgit-update-ref refs/heads/master master^ || return 1\n+\tgit-send-pack --force ./victim/.git/ master && return 1\n \t! git diff .git/refs/heads/master victim/.git/refs/heads/master\n '\n \n-- \n1.5.0.3.895.g09890\n"}]}