Re: [PATCH 2/3] checkout: reorder option handling

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 28, 2012, 20:45 UTC
Message-ID: <7vipc2zqxg.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1346161748-25651-3-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 20 quoted lines

> checkout operates in three different modes. On top of that it tries to
> be smart by guessing the branch name for switching. This results in
> messy option handling code. This patch reorders it so:
>
>  - easy option handling comes first
>  - the big chunk of branch name guessing comes next
>  - mode detection comes last. Once the mode is known, check again to
>    see if users specify any incompatible options
>  - the actual action is done
>
> Another slight improvement is always print branch name (or commit
> name) when printing errors related ot them. This helps catch the case
> where an option is mistaken as branch/commit.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> ...
> +	/* Easy mode-independent incompatible checks */
>  	if (opts.force_detach && (opts.new_branch || opts.new_orphan_branch))
>  		die(_("--detach cannot be used with -b/-B/--orphan"));

Did you catch "--detach -B" combination without checking new_branch_force?

Show 9 quoted lines

>  	if (opts.force_detach && 0 < opts.track)
>  		die(_("--detach cannot be used with -t"));
> +	if (opts.force && opts.merge)
> +		die(_("git checkout: -f and -m are incompatible"));
> +
> +	if (conflict_style) {
> +		opts.merge = 1; /* implied */
> +		git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
> +	}

"checkout --conflict=diff3 -f <branch>" would imply combination of "-m" and "-f", which is supposed to be forbidden in the previous check, no?

I very much like the idea of separating things in phases like your proposed log message explains. But I wonder if the order should be to do dwimming and parameter canonicalization first, then decide the mode (these might have to be swapped, as the same parameter may dwim down to different things depending on the mode), and finally check for sanity before performing.

To avoid confusion, it also might not be a bad idea to remove new_branch_force and new_orphan_branch from the structure and introduce "enum branch_creation_type" or something, and always have the new branch name in "new_branch" field (this needs to get various pointers into opts out of the parseopt options[] array; parse into separate variables and decide what to put in "struct checkout_opts"), independent from how that branch is going to be created (either -b, -B, or --orphan).