[PATCH] branch: handle errors on setting tracking branches

[PATCH] branch: handle errors on setting tracking branches

[Patrick Steinhardt]

The function `install_branch_config`, which is used to set up
tracking branches, does not verify return codes of
`git_config_set`. Due to this we may incorrectly print that a
tracking branch has been set up when in fact we did not due to an
error.

Fix this by checking the return value of `git_config_set` and
returning early in the case of an error. The
`install_branch_config` function has been changed to return an
error code that reflects whether it has been successful.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---

I've run into this issue when a stale config.lock has been left:

> git branch --set-upstream-to=origin/next
error: could not lock config file /path/to/repo/git/config: File exists
error: could not lock config file /path/to/repo/git/config: File exists
Branch next set up to track remote branch next from origin.

While we spit out some errors, we still claim that we set up the
tracking branch correctly, which is not the case.

Actually, there are quite a few places where we don't check the
return values of git_config_set and related functions. I didn't
go through them yet, but might do so if you deem this to be of
value.

 branch.c | 24 ++++++++++++++++--------
 branch.h |  3 ++-
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/branch.c b/branch.c
index d013374..5ab3ad4 100644
--- a/branch.c
+++ b/branch.c
@@ -48,22 +48,24 @@ static int should_setup_rebase(const char *origin)
 	return 0;
 }
 
-void install_branch_config(int flag, const char *local, const char *origin, const char *remote)
+int install_branch_config(int flag, const char *local, const char *origin, const char *remote)
 {
 	const char *shortname = NULL;
 	struct strbuf key = STRBUF_INIT;
-	int rebasing = should_setup_rebase(origin);
+	int ret = 0, rebasing = should_setup_rebase(origin);
 
 	if (skip_prefix(remote, "refs/heads/", &shortname)
 	    && !strcmp(local, shortname)
 	    && !origin) {
 		warning(_("Not setting branch %s as its own upstream."),
 			local);
-		return;
+		return ret;
 	}
 
 	strbuf_addf(&key, "branch.%s.remote", local);
-	git_config_set(key.buf, origin ? origin : ".");
+	ret = git_config_set(key.buf, origin ? origin : ".");
+	if (ret)
+		goto out;
 
 	strbuf_reset(&key);
 	strbuf_addf(&key, "branch.%s.merge", local);
@@ -72,9 +74,10 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 	if (rebasing) {
 		strbuf_reset(&key);
 		strbuf_addf(&key, "branch.%s.rebase", local);
-		git_config_set(key.buf, "true");
+		ret = git_config_set(key.buf, "true");
+		if (ret)
+			goto out;
 	}
-	strbuf_release(&key);
 
 	if (flag & BRANCH_CONFIG_VERBOSE) {
 		if (shortname) {
@@ -101,6 +104,10 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 					  local, remote);
 		}
 	}
+
+out:
+	strbuf_release(&key);
+	return ret;
 }
 
 /*
@@ -133,8 +140,9 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
 		return error(_("Not tracking: ambiguous information for ref %s"),
 				orig_ref);
 
-	install_branch_config(config_flags, new_ref, tracking.remote,
-			      tracking.src ? tracking.src : orig_ref);
+	if (install_branch_config(config_flags, new_ref, tracking.remote,
+				  tracking.src ? tracking.src : orig_ref))
+		return error(_("Could not setup tracking branch"));
 
 	free(tracking.src);
 	return 0;
diff --git a/branch.h b/branch.h
index d3446ed..d287884 100644
--- a/branch.h
+++ b/branch.h
@@ -43,9 +43,10 @@ void remove_branch_state(void);
 /*
  * Configure local branch "local" as downstream to branch "remote"
  * from remote "origin".  Used by git branch --set-upstream.
+ * Returns zero on success, non-zero otherwise.
  */
 #define BRANCH_CONFIG_VERBOSE 01
-extern void install_branch_config(int flag, const char *local, const char *origin, const char *remote);
+extern int install_branch_config(int flag, const char *local, const char *origin, const char *remote);
 
 /*
  * Read branch description
-- 
2.5.3

Re: [PATCH] branch: handle errors on setting tracking branches

[Jeff King]

On Thu, Sep 24, 2015 at 12:32:01PM +0200, Patrick Steinhardt wrote:

> The function `install_branch_config`, which is used to set up
> tracking branches, does not verify return codes of
> `git_config_set`. Due to this we may incorrectly print that a
> tracking branch has been set up when in fact we did not due to an
> error.
> 
> Fix this by checking the return value of `git_config_set` and
> returning early in the case of an error. The
> `install_branch_config` function has been changed to return an
> error code that reflects whether it has been successful.

I think it's nice to tell the user when something has gone wrong, so in
that sense this is a good change. Though git_config_set does already
print something, so the main improvement here is passing up the error
code from install_branch_config. What happens to that error?

I count 4 callers in the current code, and none of them currently looks
at the return value. Your patch updates setup_tracking() to propagate
the error, which is good. But that is called from exactly one place,
create_branch(), which also ignores the outcome. :-/

I don't think we want to die() when the config fails. That might be the
right thing for "branch --set-upstream-to", but probably not for
"checkout -b". I think we need to look at each call site and see whether
we should be propagating the error back. With the hope that we would
ultimately affect the exit code of the command.

In the case of branch creation, we are in a two-step procedure: create
the branch, then set up its tracking config. We _could_ rollback the
first step when the second step fails, but that is probably not worth
the complication.

> Actually, there are quite a few places where we don't check the
> return values of git_config_set and related functions. I didn't
> go through them yet, but might do so if you deem this to be of
> value.

I think more error-checking is better than less. But I do think it's
worth plumbing through the error codes in each case.

It's tempting to just die() when the operation fails, but as discussed
above, that's not always the most appropriate thing.

>  branch.c | 24 ++++++++++++++++--------
>  branch.h |  3 ++-
>  2 files changed, 18 insertions(+), 9 deletions(-)

The patch itself looks OK to me from a cursory read.

-Peff

Re: [PATCH] branch: handle errors on setting tracking branches

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I count 4 callers in the current code, and none of them currently looks
> at the return value. Your patch updates setup_tracking() to propagate
> the error, which is good. But that is called from exactly one place,
> create_branch(), which also ignores the outcome. :-/
>
> I don't think we want to die() when the config fails. That might be the
> right thing for "branch --set-upstream-to", but probably not for
> "checkout -b". I think we need to look at each call site and see whether
> we should be propagating the error back. With the hope that we would
> ultimately affect the exit code of the command.
>
> In the case of branch creation, we are in a two-step procedure: create
> the branch, then set up its tracking config. We _could_ rollback the
> first step when the second step fails, but that is probably not worth
> the complication.
>
>> Actually, there are quite a few places where we don't check the
>> return values of git_config_set and related functions. I didn't
>> go through them yet, but might do so if you deem this to be of
>> value.
>
> I think more error-checking is better than less. But I do think it's
> worth plumbing through the error codes in each case.
>
> It's tempting to just die() when the operation fails, but as discussed
> above, that's not always the most appropriate thing.
>
>>  branch.c | 24 ++++++++++++++++--------
>>  branch.h |  3 ++-
>>  2 files changed, 18 insertions(+), 9 deletions(-)
>
> The patch itself looks OK to me from a cursory read.

I agree with everything you said in the analysis, including that
this patch is a good first step in the right direction, but at the
same time it is a glorified no-op whose only external effect is to
make the error diagnosis a bit noisier.  A good first step in the
right direction with stride length of zero ;-)