Re: [PATCH] checkout: verify new branch name's validity early
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 28, 2012, 03:55 UTC
- Message-ID
- <7vr4qroel6.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1346120981-27234-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 22 quoted lines
> If the new branch name to -b/-B happens to be missing, the next > argument may be mistaken as branch name and no longer recognized by > checkout as argument. This may lead to confusing error messages. > > By checking branch name early and printing out the invalid name, users > may realize they forgot to specify branch name. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > On Tue, Aug 28, 2012 at 12:03 AM, Junio C Hamano <gitster@pobox.com> wrote: > > Ideally you would want > > > > fatal: "-t" is not an acceptable name for a branch > > > > in this case; if it is cumbersome to arrange, at the very least, > > > > updating paths is incompatible with checking out the branch "-t". > > > > would be clearer. > > Fair enough. It turns out we do check branch name's validity. It's > just too late.
The surrounding code is somewhat tricky and the code structure is brittle; there are places that update the opts.new_branch so the new location of this check has to be after them, and there is one codepath that having a bad value in it does not matter.
I had to check the code outside the context of this patch a few times to convince myself that this patch does not break them. I'll queue the patch as-is for now, but we probably would want to see how we can structure it to be less brittle.
Thanks.
Show 56 quoted lines
> builtin/checkout.c | 20 ++++++++++----------
> t/t2018-checkout-branch.sh | 5 +++++
> 2 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index d812219..03b0f25 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1049,6 +1049,16 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
> if (opts.track == BRANCH_TRACK_UNSPECIFIED)
> opts.track = git_branch_track;
>
> + if (opts.new_branch) {
> + struct strbuf buf = STRBUF_INIT;
> +
> + opts.branch_exists = validate_new_branchname(opts.new_branch, &buf,
> + !!opts.new_branch_force,
> + !!opts.new_branch_force);
> +
> + strbuf_release(&buf);
> + }
> +
> if (argc) {
> const char **pathspec = get_pathspec(prefix, argv);
>
> @@ -1079,16 +1089,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
> if (patch_mode)
> return interactive_checkout(new.name, NULL, &opts);
>
> - if (opts.new_branch) {
> - struct strbuf buf = STRBUF_INIT;
> -
> - opts.branch_exists = validate_new_branchname(opts.new_branch, &buf,
> - !!opts.new_branch_force,
> - !!opts.new_branch_force);
> -
> - strbuf_release(&buf);
> - }
> -
> if (new.name && !new.commit) {
> die(_("Cannot switch branch to a non-commit."));
> }
> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
> index 2741262..48ab6a2 100755
> --- a/t/t2018-checkout-branch.sh
> +++ b/t/t2018-checkout-branch.sh
> @@ -198,4 +198,9 @@ test_expect_success 'checkout -B to the current branch works' '
> test_dirty_mergeable
> '
>
> +test_expect_success 'checkout -b checks branch validitity early' '
> + test_must_fail git checkout -b -t origin/master 2>err &&
> + grep "not a valid branch name" err
> +'
> +
> test_done