Re: [PATCH 3/4] branch: add --dry-run option to branch

Jonathan Tan [off-list ref] writes:

Glen Choo [off-list ref] writes:

quoted

When running "git branch --recurse-submodules topic"

At this point, this argument has not been introduced yet, so better to
just say "A future patch will introduce branch creation that recurses
into submodules, so..."

quoted

+-n::
+--dry-run::
+	Can only be used when creating a branch. If the branch creation
+	would fail, show the relevant error message. If the branch
+	creation would succeed, show nothing.

Right now we only plan to use this internally so it's not worth using a
single character argument for this right now, I think. We can always add
it later if we find it useful.

For the reasons you specified, I didn't intend to add -n. However, -n is
automatically added by OPT__DRY_RUN, so I thought it was appropriate to
document it.

quoted

-	if (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +
-	    list + edit_description + unset_upstream > 1)
+	create = 1 - (!!delete + !!rename + !!copy + !!new_upstream +
+		      !!show_current + !!list + !!edit_description +
+		      !!unset_upstream);
+	if (create < 0)
 		usage_with_options(builtin_branch_usage, options);

Hmm...I think it would be clearer just to call it noncreate_options and
print usage if it is greater than 1. Then whenever you want to check if
it's create, check `!noncreate_options` instead.

Sounds good.

quoted

@@ -852,10 +862,20 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (track == BRANCH_TRACK_OVERRIDE)
 			die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
 
-		create_branch(the_repository,
-			      argv[0], (argc == 2) ? argv[1] : head,
-			      force, 0, reflog, quiet, track);
+		if (dry_run) {
+			struct strbuf buf = STRBUF_INIT;
+			char *unused_full_ref;
+			struct object_id unused_oid;
 
+			validate_new_branchname(branch_name, &buf, force);
+			validate_branch_start(the_repository, start_name, track,
+					      &unused_oid, &unused_full_ref);
+			strbuf_release(&buf);
+			FREE_AND_NULL(unused_full_ref);
+			return 0;
+		}
+		create_branch(the_repository, branch_name, start_name, force, 0,
+			      reflog, quiet, track);
 	} else
 		usage_with_options(builtin_branch_usage, options);
 

I don't think we should use separate code paths for the dry run and the
regular run - could create_branch() take a dry_run parameter instead?
(If there are too many boolean parameters, it might be time to convert
some or all to a struct.)

That sounds reasonable, it would be good not to have duplicate code
paths.

This suggestion would require a reworking of patch 2, which is why I
didn't comment there. But if we are not going to use this suggestion and
are going to stick with patch 2, then my comment on it is that it seems
to be doing too much: I ran "git show --color-moved" on it and saw that
quite a few lines were newly introduced (not just moved around).

I will do the reworking, but the final result will probably look very
similar to the one in patch 2. Does it look more acceptable with
--color-moved-ws=ignore-all-space? Some text had to be reindented
(because I removed one conditional), but I also replaced some functions
with repo_* versions.