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

From: Glen Choo <chooglen@google.com>
Date: Dec 8, 2021, 18:48 UTC
Message-ID: <kl6l5yrzaq5z.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqqbl1tcptq.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 11 quoted lines

>> As Jonathan noted in v1, the diff is quite large. I could shrink this
>> by forward-declaring setup_tracking() so that the function definitions
>> are in the same order; let me know if that would be preferred.
>
> If you are not making any changes to setup_tracking() and just
> moving, then the patch size inflated by the move is OK.
>
> If you are moving and changing at the same time, well, that would
> make it harder to read what is going on in the patch, so you want to
> find a way to avoid it.  Splitting it the pure move into a separate
> patch or use of forward-declaration may be good ways to do so.

Thanks for the advice! I'll use one of these approaches.

Show 12 quoted lines

> The post context continues with:
>
> 				advise(_(upstream_advice));
> 				exit(1);
> 			}
> 			die(_(upstream_missing), start_name);
>
> This is not a problem with this patch, and it should not be fixed as
> part of this series, but since I noticed it, I'll mention it as a
> leftover low-hanging fruit to be fixed after the dust settles.  The
> exit(1) looks wrong.  We should exit with 128 just like die() does.
> Issuing of an advice message should not affect the exit code.

I'll include an optional cleanup patch to address this and the exit(-1).

Show 7 quoted lines

>> +void setup_tracking(const char *new_ref, const char *orig_ref,
>> +			   enum branch_track track, int quiet, int expand_orig)
>
> It is unclear what expand_orig option is supposed to do and how it
> would help the caller.  Perhaps a comment before the function is in
> order (the comment in branch.h before the declaration of this
> function does not make it clear, either).

Ah, I will add extra clarification.

This boolean parameter controls whether or not we should validate + DWIM orig_ref. This is a performance optimization to avoid expanding an orig_ref that has already been expanded (e.g. because create_branch() has already expanded it).

But, as you point out later on, this may not be a good idea.

Show 17 quoted lines

>> +{
>> +	struct tracking tracking;
>> +	int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
>> +	char *full_orig_ref;
>> +	struct object_id unused_oid;
>> +
>> +	memset(&tracking, 0, sizeof(tracking));
>> +	if (expand_orig)
>> +		validate_branch_start(the_repository, orig_ref, track, &unused_oid, &full_orig_ref);
>
> So, the idea is, because we are setting up a new_ref to "track"
> orig_ref, we may be better off pretending that we are "creating a
> new branch from the orig_ref and tracking it", so that orig_ref that
> is not something we can track will be caught with the same logic?
>
> This will cause full_orig_ref to start with "refs/heads/" or
> "refs/remotes/" if 'track' is something that requires tracking.

Yes.

Show 7 quoted lines

>> +	else
>> +		full_orig_ref = xstrdup(orig_ref);
>
> Even though the variable claims to be FULL orig_ref, when this side
> of if/else is taken, nobody guarantees that full_orig_ref is in fact
> a full ref, or merely the name of the branch, no?  Would that cause
> problems later?

Yes, the assumption is that the caller has already done the work of making sure that orig_ref has been validated and expanded into its full form. As mentioned earlier, this is purely a performance optimzation, but it is not a very safe one because it requires the caller to pick the correct value for expand_orig.

I chose this approach because I anticipate that the only callers of setup_tracking() will be create_branch() (which always wants expand_orig = 0) and cmd_branch() (which always wants expand_orig = 1), so the right course of action is clear for now. I don't think setup_tracking() will be useful to anyone else, but expand_orig is a potential sharp edge for new callers.

My next-preferred option would be to remove "expand_orig" and always call validate_branch_start(). We might waste a few cycles sometimes, but the function becomes impossible to misuse.

Show 23 quoted lines

>> +	tracking.spec.dst = full_orig_ref;
>> +	if (for_each_remote(find_tracked_branch, &tracking))
>> +		goto cleanup;
>> +
>> +	if (!tracking.matches)
>> +		switch (track) {
>> +		case BRANCH_TRACK_ALWAYS:
>> +		case BRANCH_TRACK_EXPLICIT:
>> +		case BRANCH_TRACK_OVERRIDE:
>> +			break;
>> +		default:
>> +			goto cleanup;
>> +		}
>> +
>> +	if (tracking.matches > 1)
>> +		die(_("Not tracking: ambiguous information for ref %s"),
>> +		    full_orig_ref);
>
> What's the next step for the user to take, after seeing this message?
> Do we have the necessary info readily available to help them at this
> point in tracking.* structure (e.g. "it could be following X or Y and
> we cannot decide between the two for you"), or have we discarded the
> information already?

This information is discarded in expand_ref(). From the function signature:

  int expand_ref(struct repository *repo, const char *str, int len,
	       struct object_id *oid, char **ref)

The return value is the number of matched refs and "ref" is an out parameter containing the first matched ref.

Show 10 quoted lines

> If tracking.matches == 0, because we broke out of the switch() for
> some values of track, we will make this install_branch_config()
> using members of the tracking structure, which is a bit unnerving.
>
>> +	if (install_branch_config(config_flags, new_ref, tracking.remote,
>> +			      tracking.src ? tracking.src : full_orig_ref) < 0)
>
> But tracking.src==NULL is substituted with full_orig_ref, so as long
> as the value in that variable is sensible, we would probably be ok
> on the 4th parameter.

When tracking.matches is zero, the assumption is that install_branch_config() should set up tracking based off a local branch. By default, we assume that users only want tracking for remote-tracking branches, so only track local branches if we are confident that the user wants this, aka track = BRANCH_TRACK_{EXPLICIT,ALWAYS,OVERRIDE} (and soon, BRANCH_TRACK_INHERIT).

> I am not sure who set tracking.remote or if it is always set to a
> sensible value. Especially when tracking.matches is zero.

tracking.matches == 0 should imply that tracking.remote == NULL, which gives us the expected behavior of tracking a local branch in install_branch_config().

This is also a bit too implicit for my tastes, but I don't think this is the time for such a refactor :)