{"thread":{"id":"40419","subject":"[PATCH] branch: handle errors on setting tracking branches","startedAt":"2015-09-24T10:32:01Z","lastAt":"2015-09-25T16:52:00Z","messageCount":3,"participants":["Patrick Steinhardt","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"270635","messageId":"1443090721-14519-1-git-send-email-ps@pks.im","threadId":"40419","inReplyTo":null,"subject":"[PATCH] branch: handle errors on setting tracking branches","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-09-24T10:32:01Z","receivedAt":"2015-09-24T10:32:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `install_branch_config`, which is used to set up\ntracking branches, does not verify return codes of\n`git_config_set`. Due to this we may incorrectly print that a\ntracking branch has been set up when in fact we did not due to an\nerror.\n\nFix this by checking the return value of `git_config_set` and\nreturning early in the case of an error. The\n`install_branch_config` function has been changed to return an\nerror code that reflects whether it has been successful.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n\nI've run into this issue when a stale config.lock has been left:\n\n> git branch --set-upstream-to=origin/next\nerror: could not lock config file /path/to/repo/git/config: File exists\nerror: could not lock config file /path/to/repo/git/config: File exists\nBranch next set up to track remote branch next from origin.\n\nWhile we spit out some errors, we still claim that we set up the\ntracking branch correctly, which is not the case.\n\nActually, there are quite a few places where we don't check the\nreturn values of git_config_set and related functions. I didn't\ngo through them yet, but might do so if you deem this to be of\nvalue.\n\n branch.c | 24 ++++++++++++++++--------\n branch.h |  3 ++-\n 2 files changed, 18 insertions(+), 9 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex d013374..5ab3ad4 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -48,22 +48,24 @@ static int should_setup_rebase(const char *origin)\n \treturn 0;\n }\n \n-void install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n+int install_branch_config(int flag, const char *local, const char *origin, const char *remote)\n {\n \tconst char *shortname = NULL;\n \tstruct strbuf key = STRBUF_INIT;\n-\tint rebasing = should_setup_rebase(origin);\n+\tint ret = 0, rebasing = should_setup_rebase(origin);\n \n \tif (skip_prefix(remote, \"refs/heads/\", &shortname)\n \t    && !strcmp(local, shortname)\n \t    && !origin) {\n \t\twarning(_(\"Not setting branch %s as its own upstream.\"),\n \t\t\tlocal);\n-\t\treturn;\n+\t\treturn ret;\n \t}\n \n \tstrbuf_addf(&key, \"branch.%s.remote\", local);\n-\tgit_config_set(key.buf, origin ? origin : \".\");\n+\tret = git_config_set(key.buf, origin ? origin : \".\");\n+\tif (ret)\n+\t\tgoto out;\n \n \tstrbuf_reset(&key);\n \tstrbuf_addf(&key, \"branch.%s.merge\", local);\n@@ -72,9 +74,10 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n \tif (rebasing) {\n \t\tstrbuf_reset(&key);\n \t\tstrbuf_addf(&key, \"branch.%s.rebase\", local);\n-\t\tgit_config_set(key.buf, \"true\");\n+\t\tret = git_config_set(key.buf, \"true\");\n+\t\tif (ret)\n+\t\t\tgoto out;\n \t}\n-\tstrbuf_release(&key);\n \n \tif (flag & BRANCH_CONFIG_VERBOSE) {\n \t\tif (shortname) {\n@@ -101,6 +104,10 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n \t\t\t\t\t  local, remote);\n \t\t}\n \t}\n+\n+out:\n+\tstrbuf_release(&key);\n+\treturn ret;\n }\n \n /*\n@@ -133,8 +140,9 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,\n \t\treturn error(_(\"Not tracking: ambiguous information for ref %s\"),\n \t\t\t\torig_ref);\n \n-\tinstall_branch_config(config_flags, new_ref, tracking.remote,\n-\t\t\t      tracking.src ? tracking.src : orig_ref);\n+\tif (install_branch_config(config_flags, new_ref, tracking.remote,\n+\t\t\t\t  tracking.src ? tracking.src : orig_ref))\n+\t\treturn error(_(\"Could not setup tracking branch\"));\n \n \tfree(tracking.src);\n \treturn 0;\ndiff --git a/branch.h b/branch.h\nindex d3446ed..d287884 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -43,9 +43,10 @@ void remove_branch_state(void);\n /*\n  * Configure local branch \"local\" as downstream to branch \"remote\"\n  * from remote \"origin\".  Used by git branch --set-upstream.\n+ * Returns zero on success, non-zero otherwise.\n  */\n #define BRANCH_CONFIG_VERBOSE 01\n-extern void install_branch_config(int flag, const char *local, const char *origin, const char *remote);\n+extern int install_branch_config(int flag, const char *local, const char *origin, const char *remote);\n \n /*\n  * Read branch description\n-- \n2.5.3\n"},{"id":"270637","messageId":"20150924140538.GA11666@sigill.intra.peff.net","threadId":"40419","inReplyTo":"1443090721-14519-1-git-send-email-ps@pks.im","subject":"Re: [PATCH] branch: handle errors on setting tracking branches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-24T14:05:38Z","receivedAt":"2015-09-24T14:05:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 24, 2015 at 12:32:01PM +0200, Patrick Steinhardt wrote:\n\n> The function `install_branch_config`, which is used to set up\n> tracking branches, does not verify return codes of\n> `git_config_set`. Due to this we may incorrectly print that a\n> tracking branch has been set up when in fact we did not due to an\n> error.\n> \n> Fix this by checking the return value of `git_config_set` and\n> returning early in the case of an error. The\n> `install_branch_config` function has been changed to return an\n> error code that reflects whether it has been successful.\n\nI think it's nice to tell the user when something has gone wrong, so in\nthat sense this is a good change. Though git_config_set does already\nprint something, so the main improvement here is passing up the error\ncode from install_branch_config. What happens to that error?\n\nI count 4 callers in the current code, and none of them currently looks\nat the return value. Your patch updates setup_tracking() to propagate\nthe error, which is good. But that is called from exactly one place,\ncreate_branch(), which also ignores the outcome. :-/\n\nI don't think we want to die() when the config fails. That might be the\nright thing for \"branch --set-upstream-to\", but probably not for\n\"checkout -b\". I think we need to look at each call site and see whether\nwe should be propagating the error back. With the hope that we would\nultimately affect the exit code of the command.\n\nIn the case of branch creation, we are in a two-step procedure: create\nthe branch, then set up its tracking config. We _could_ rollback the\nfirst step when the second step fails, but that is probably not worth\nthe complication.\n\n> Actually, there are quite a few places where we don't check the\n> return values of git_config_set and related functions. I didn't\n> go through them yet, but might do so if you deem this to be of\n> value.\n\nI think more error-checking is better than less. But I do think it's\nworth plumbing through the error codes in each case.\n\nIt's tempting to just die() when the operation fails, but as discussed\nabove, that's not always the most appropriate thing.\n\n>  branch.c | 24 ++++++++++++++++--------\n>  branch.h |  3 ++-\n>  2 files changed, 18 insertions(+), 9 deletions(-)\n\nThe patch itself looks OK to me from a cursory read.\n\n-Peff\n"},{"id":"270757","messageId":"xmqqzj0a4f2n.fsf@gitster.mtv.corp.google.com","threadId":"40419","inReplyTo":"20150924140538.GA11666@sigill.intra.peff.net","subject":"Re: [PATCH] branch: handle errors on setting tracking branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-25T16:52:00Z","receivedAt":"2015-09-25T16:52:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I count 4 callers in the current code, and none of them currently looks\n> at the return value. Your patch updates setup_tracking() to propagate\n> the error, which is good. But that is called from exactly one place,\n> create_branch(), which also ignores the outcome. :-/\n>\n> I don't think we want to die() when the config fails. That might be the\n> right thing for \"branch --set-upstream-to\", but probably not for\n> \"checkout -b\". I think we need to look at each call site and see whether\n> we should be propagating the error back. With the hope that we would\n> ultimately affect the exit code of the command.\n>\n> In the case of branch creation, we are in a two-step procedure: create\n> the branch, then set up its tracking config. We _could_ rollback the\n> first step when the second step fails, but that is probably not worth\n> the complication.\n>\n>> Actually, there are quite a few places where we don't check the\n>> return values of git_config_set and related functions. I didn't\n>> go through them yet, but might do so if you deem this to be of\n>> value.\n>\n> I think more error-checking is better than less. But I do think it's\n> worth plumbing through the error codes in each case.\n>\n> It's tempting to just die() when the operation fails, but as discussed\n> above, that's not always the most appropriate thing.\n>\n>>  branch.c | 24 ++++++++++++++++--------\n>>  branch.h |  3 ++-\n>>  2 files changed, 18 insertions(+), 9 deletions(-)\n>\n> The patch itself looks OK to me from a cursory read.\n\nI agree with everything you said in the analysis, including that\nthis patch is a good first step in the right direction, but at the\nsame time it is a glorified no-op whose only external effect is to\nmake the error diagnosis a bit noisier.  A good first step in the\nright direction with stride length of zero ;-)\n"}]}