Nguyễn Thái Ngọc Duy <[email protected]> writes:
> 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 <[email protected]>
> ---
> On Tue, Aug 28, 2012 at 12:03 AM, Junio C Hamano <[email protected]> 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.
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
