Glen Choo <chooglen@xxxxxxxxxx> writes:
> 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..."
> +-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.
> - 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.
> @@ -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.)
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).
