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

Junio C Hamano [off-list ref] writes:

quoted

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.

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).

quoted

+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.

quoted

+{
+	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.

quoted

+	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.

quoted

+	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.

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.

quoted

+	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 :)