git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v4 02/15] branch: die on error in setting up tracking branch

From
Junio C Hamano <gitster@pobox.com>
Date
Feb 2, 2016, 20:49 UTC
Message-ID
<xmqq60y6suqx.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<1454413916-31984-3-git-send-email-ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 8 quoted lines
> The setup_tracking function calls install_branch_config, which
> may fail writing the configuration due to a locked configuration
> file or other error conditions. setup_tracking can also fail when
> trying to track ambiguous information for a reference. While this
> condition is checked for and an error code is returned, this
> error is not checked by the caller.
>
> Fix both issues by dying early when error occur.

Hmph. I think create_branch() is written in such a way that all die() come before the actual ref transaction attempts to create the branch, but this change means that we have already created the branch and then die without undoing the damage that is already done.

"The error is not checked by the caller" is very true, but can the caller do something better than just die? I personally do not think it is such a big deal if we just died here, but I may have overlooked something.

Show 111 quoted lines
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  branch.c          | 19 +++++++++----------
>  branch.h          |  1 +
>  t/t3200-branch.sh |  9 ++++++++-
>  3 files changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 7ff3f20..7106369 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -64,16 +64,16 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
>  	}
>  
>  	strbuf_addf(&key, "branch.%s.remote", local);
> -	git_config_set(key.buf, origin ? origin : ".");
> +	git_config_set_or_die(key.buf, origin ? origin : ".");
>  
>  	strbuf_reset(&key);
>  	strbuf_addf(&key, "branch.%s.merge", local);
> -	git_config_set(key.buf, remote);
> +	git_config_set_or_die(key.buf, remote);
>  
>  	if (rebasing) {
>  		strbuf_reset(&key);
>  		strbuf_addf(&key, "branch.%s.rebase", local);
> -		git_config_set(key.buf, "true");
> +		git_config_set_or_die(key.buf, "true");
>  	}
>  	strbuf_release(&key);
>  
> @@ -109,8 +109,8 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
>   * to infer the settings for branch.<new_ref>.{remote,merge} from the
>   * config.
>   */
> -static int setup_tracking(const char *new_ref, const char *orig_ref,
> -			  enum branch_track track, int quiet)
> +static void setup_tracking(const char *new_ref, const char *orig_ref,
> +			   enum branch_track track, int quiet)
>  {
>  	struct tracking tracking;
>  	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
> @@ -118,7 +118,7 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
>  	memset(&tracking, 0, sizeof(tracking));
>  	tracking.spec.dst = (char *)orig_ref;
>  	if (for_each_remote(find_tracked_branch, &tracking))
> -		return 1;
> +		return;
>  
>  	if (!tracking.matches)
>  		switch (track) {
> @@ -127,18 +127,17 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
>  		case BRANCH_TRACK_OVERRIDE:
>  			break;
>  		default:
> -			return 1;
> +			return;
>  		}
>  
>  	if (tracking.matches > 1)
> -		return error(_("Not tracking: ambiguous information for ref %s"),
> -				orig_ref);
> +		die(_("Not tracking: ambiguous information for ref %s"),
> +		    orig_ref);
>  
>  	install_branch_config(config_flags, new_ref, tracking.remote,
>  			      tracking.src ? tracking.src : orig_ref);
>  
>  	free(tracking.src);
> -	return 0;
>  }
>  
>  int read_branch_desc(struct strbuf *buf, const char *branch_name)
> diff --git a/branch.h b/branch.h
> index 58aa45f..8ce22f8 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -43,6 +43,7 @@ void remove_branch_state(void);
>  /*
>   * Configure local branch "local" as downstream to branch "remote"
>   * from remote "origin".  Used by git branch --set-upstream.
> + * Dies if unable to install branch config.
>   */
>  #define BRANCH_CONFIG_VERBOSE 01
>  extern void install_branch_config(int flag, const char *local, const char *origin, const char *remote);
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index cdaf6f6..dd776b3 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -446,6 +446,13 @@ test_expect_success '--set-upstream-to fails on a non-ref' '
>  	test_must_fail git branch --set-upstream-to HEAD^{}
>  '
>  
> +test_expect_success '--set-upstream-to fails on locked config' '
> +	test_when_finished "rm -f .git/config.lock" &&
> +	>.git/config.lock &&
> +	git branch locked &&
> +	test_must_fail git branch --set-upstream-to locked
> +'
> +
>  test_expect_success 'use --set-upstream-to modify HEAD' '
>  	test_config branch.master.remote foo &&
>  	test_config branch.master.merge foo &&
> @@ -579,7 +586,7 @@ test_expect_success 'avoid ambiguous track' '
>  	git config remote.ambi1.fetch refs/heads/lalala:refs/heads/master &&
>  	git config remote.ambi2.url lilili &&
>  	git config remote.ambi2.fetch refs/heads/lilili:refs/heads/master &&
> -	git branch all1 master &&
> +	test_must_fail git branch all1 master &&
>  	test -z "$(git config branch.all1.merge)"
>  '
Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 4 of 26 in “config: make git_config_set die on failure”
  1. 00/15 config: make git_config_set die on failurePatrick Steinhardt, Feb 2, 2016
  2. 01/15 config: introduce set_or_die wrappersPatrick Steinhardt, Feb 2, 2016
  3. 02/15 branch: die on error in setting up tracking branchPatrick Steinhardt, Feb 2, 2016
  4. Junio C HamanoFeb 2, 2016
  5. Patrick SteinhardtFeb 8, 2016
  6. Patrick SteinhardtFeb 8, 2016
  7. 03/15 branch: die on config error when unsetting upstreamPatrick Steinhardt, Feb 2, 2016
  8. 04/15 branch: die on config error when editing branch descriptionPatrick Steinhardt, Feb 2, 2016
  9. 05/15 submodule: die on config error when linking modulesPatrick Steinhardt, Feb 2, 2016
  10. 06/15 submodule--helper: die on config error when cloning modulePatrick Steinhardt, Feb 2, 2016
  11. Eric SunshineFeb 2, 2016
  12. Patrick SteinhardtFeb 8, 2016
  13. 07/15 remote: die on config error when setting URLPatrick Steinhardt, Feb 2, 2016
  14. Patrick SteinhardtFeb 2, 2016
  15. 08/15 remote: die on config error when setting/adding branchesPatrick Steinhardt, Feb 2, 2016
  16. 09/15 remote: die on config error when manipulating remotesPatrick Steinhardt, Feb 2, 2016
  17. 10/15 clone: die on config error in cmd_clonePatrick Steinhardt, Feb 2, 2016
  18. Junio C HamanoFeb 2, 2016
  19. 11/15 init-db: die on config errors when initializing empty repoPatrick Steinhardt, Feb 2, 2016
  20. 12/15 sequencer: die on config error when saving replay optsPatrick Steinhardt, Feb 2, 2016
  21. 13/15 compat: die when unable to set core.precomposeunicodePatrick Steinhardt, Feb 2, 2016
  22. 14/15 config: rename git_config_set to git_config_set_gentlyPatrick Steinhardt, Feb 2, 2016
  23. 15/15 config: rename git_config_set_or_die to git_config_setPatrick Steinhardt, Feb 2, 2016
  24. Stefan BellerFeb 2, 2016
  25. Junio C HamanoFeb 2, 2016
  26. Patrick SteinhardtFeb 4, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.