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

Jonathan Tan [off-list ref] writes:

quoted

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/ (local)

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.

quoted

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

quoted

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.

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

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.

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 (local)

Thanks!