Re: [PATCH v3 1/5] branch: move --set-upstream-to behavior to setup_tracking()

From: Glen Choo <chooglen@google.com>
Date: Dec 9, 2021, 22:16 UTC
Message-ID: <kl6lh7bha0fp.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20211209211951.498358-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 12 quoted lines

>> This refactor is motivated by a desire to add a "dry_run" parameter to
>> create_branch() that will validate whether or not a branch can be
>> created without actually creating it - this behavior be used in a
>> subsequent commit that adds `git branch --recurse-submodules topic`.
>
> When I said that the patch was doing too much [1], I meant that for a
> patch that is supposed to be about refactoring, there were many lines
> that didn't seem to be a straightforward move. I've attached a version
> of the patch that I expected - notice that a reviewer can see that the
> lines are straightforwardly moved.
>
> [1] https://lore.kernel.org/git/20211123231035.3607109-1-jonathantanmy@google.com/

Thanks, the patch gave me a lot of think about.

Comparing yours and mine side-by-side makes it obvious that what I thought was a 'simple' refactor is not so obvious to readers. I think I can incorporate most of your patch.

Show 20 quoted lines

>> +void setup_tracking(const char *new_ref, const char *orig_ref,
>> +			   enum branch_track track, int quiet, int expand_orig)
>>  {
>>  	struct tracking tracking;
>>  	struct string_list tracking_srcs = STRING_LIST_INIT_DUP;
>>  	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
>> +	char *full_orig_ref;
>> +	struct object_id unused_oid;
>>  
>>  	memset(&tracking, 0, sizeof(tracking));
>> -	tracking.spec.dst = (char *)orig_ref;
>> +	if (expand_orig)
>> +		validate_branch_start(the_repository, orig_ref, track, &unused_oid, &full_orig_ref);
>> +	else
>> +		full_orig_ref = xstrdup(orig_ref);
>
> Having two meanings for a parameter (and which meaning used depending on
> another parameter) is quite confusing - I think it's better to call
> another function to expand the ref if necessary. See my patch below for
> an example.

Since you and Junio have both commented on this inconsistency, I'll drop the extra parameter.

I wonder if you've considered another alternative [1], which is to _always_ expand the ref, instead of _never_ expanding the ref. Always expanding the ref wastes cycles, but it avoids creating an implicit contract between setup_tracking() and validate_branch_start_or_TODO_name().

Show 28 quoted lines

>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>> index 267a624671..18e285a876 100755
>> --- a/t/t3200-branch.sh
>> +++ b/t/t3200-branch.sh
>> @@ -42,6 +42,23 @@ test_expect_success 'git branch abc should create a branch' '
>>  	git branch abc && test_path_is_file .git/refs/heads/abc
>>  '
>>  
>> +test_expect_success 'git branch abc should fail when abc exists' '
>> +	test_must_fail git branch abc
>> +'
>> +
>> +test_expect_success 'git branch --force abc should fail when abc is checked out' '
>> +	test_when_finished git switch main &&
>> +	git switch abc &&
>> +	test_must_fail git branch --force abc HEAD~1
>> +'
>> +
>> +test_expect_success 'git branch --force abc should succeed when abc exists' '
>> +	git rev-parse HEAD~1 >expect &&
>> +	git branch --force abc HEAD~1 &&
>> +	git rev-parse abc >actual &&
>> +	test_cmp expect actual
>> +'
>
> This seems like an unrelated test for a refactoring that doesn't seem to
> touch "force" (in builtin/branch.c, the line removed hardcodes force to
> 0).

I added this test because I was trying to simplify create_branch() by moving around/deleting forcing and clobber_head_ok. It turns out that I had actually broken the behavior of --force, but the test suite didn't catch it.

I think this test is a step in the right direction, but it probably falls into the category of 'incidental fix'. I am not sure where it belongs if not here.

Show 60 quoted lines

> Below is my diff, as mentioned.
>
> ---
> diff --git a/branch.c b/branch.c
> index 07a46430b3..a6803e9900 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -131,8 +131,8 @@ int install_branch_config(int flag, const char *local, const char *origin, const
>   * to infer the settings for branch.<new_ref>.{remote,merge} from the
>   * config.
>   */
> -static void setup_tracking(const char *new_ref, const char *orig_ref,
> -			   enum branch_track track, int quiet)
> +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;
> @@ -243,30 +243,12 @@ N_("\n"
>  "will track its remote counterpart, you may want to use\n"
>  "\"git push -u\" to set the upstream config as you push.");
>  
> -void create_branch(struct repository *r,
> -		   const char *name, const char *start_name,
> -		   int force, int clobber_head_ok, int reflog,
> -		   int quiet, enum branch_track track)
> +void TODO_name(struct repository *r, const char *start_name, int explicit_tracking,
> +	       char **out_real_ref, struct commit **out_commit)
>  {
>  	struct commit *commit;
>  	struct object_id oid;
>  	char *real_ref;
> -	struct strbuf ref = STRBUF_INIT;
> -	int forcing = 0;
> -	int dont_change_ref = 0;
> -	int explicit_tracking = 0;
> -
> -	if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
> -		explicit_tracking = 1;
> -
> -	if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok)
> -	    ? validate_branchname(name, &ref)
> -	    : validate_new_branchname(name, &ref, force)) {
> -		if (!force)
> -			dont_change_ref = 1;
> -		else
> -			forcing = 1;
> -	}
>  
>  	real_ref = NULL;
>  	if (get_oid_mb(start_name, &oid)) {
> @@ -304,6 +286,38 @@ void create_branch(struct repository *r,
>  
>  	if ((commit = lookup_commit_reference(r, &oid)) == NULL)
>  		die(_("Not a valid branch point: '%s'."), start_name);
> +	if (out_real_ref)
> +		*out_real_ref = real_ref;
> +	if (out_commit)
> +		*out_commit = commit;
> +}

Using a separate out_real_ref makes a lot more sense than reusing real_ref. I'll borrow this :)

Show 25 quoted lines

> +
> +void create_branch(struct repository *r,
> +		   const char *name, const char *start_name,
> +		   int force, int clobber_head_ok, int reflog,
> +		   int quiet, enum branch_track track)
> +{
> +	struct commit *commit;
> +	struct object_id oid;
> +	char *real_ref = NULL;
> +	struct strbuf ref = STRBUF_INIT;
> +	int forcing = 0;
> +	int dont_change_ref = 0;
> +	int explicit_tracking = 0;
> +
> +	if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
> +		explicit_tracking = 1;
> +
> +	if ((track == BRANCH_TRACK_OVERRIDE || clobber_head_ok)
> +	    ? validate_branchname(name, &ref)
> +	    : validate_new_branchname(name, &ref, force)) {
> +		if (!force)
> +			dont_change_ref = 1;
> +		else
> +			forcing = 1;
> +	}

This patch just moves things around without performing internal simplification (like removing dont_change_ref). The simplification would probably be done in a later patch.

Makes sense, I see how this is easier to review.

Show 16 quoted lines

> @@ -821,12 +822,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		if (!ref_exists(branch->refname))
>  			die(_("branch '%s' does not exist"), branch->name);
>  
> -		/*
> -		 * create_branch takes care of setting up the tracking
> -		 * info and making sure new_upstream is correct
> -		 */
> -		create_branch(the_repository, branch->name, new_upstream,
> -			      0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);
> +		TODO_name(the_repository, new_upstream, 1, &real_ref, NULL);
> +		setup_tracking(branch->name, real_ref, BRANCH_TRACK_OVERRIDE, quiet);
> +		free(real_ref);
>  	} else if (unset_upstream) {
>  		struct branch *branch = branch_get(argv[0]);
>  		struct strbuf buf = STRBUF_INIT;

See my previous comment and [1] about avoiding the implicit contract of TODO_name() + setup_tracking(), but of course, this is still more consistent than what what I proposed.

[1] https://lore.kernel.org/git/kl6l5yrzaq5z.fsf@chooglen-macbookpro.roam.corp.google.com

Thanks!