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

2012-08-29 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy   writes:

> + if (opts.patch_mode || opts.pathspec)
> + return cmd_checkout_entry(&opts, &new);
> + else
> + return cmd_switch(&opts, &new);
>  }

Yeah, patch_mode is really part of checking out paths, so separating
the actions into two makes sense.

Even though I am not sure about "cmd_" prefix (as these are not
commands), I'll rename the former "cmd_checkout_paths" to be
consistent with the underlying checkout_paths() function, and the.
latter "cmd_checkout_branch" for consistency with the former (you
either check out one-or-more paths, or check out a branch).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-08-29 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy   writes:

>  I'm not entirely sure about this chunk
>
>  +if (opts->track != BRANCH_TRACK_UNSPECIFIED) {
>  +if (opts->new_orphan_branch)
>  +die(_("%s cannot be used with %s"), "--orphan", "-t");
>  +if (opts->force_detach)
>  +die(_("%s cannot be used with %s"), "--detach", "-t");
>  +} else
>  +opts->track = git_branch_track;
>
>  If we don't want -t and --orphan/--detach together, then we probably should 
> ignore
>  branch.autosetupmerge when --orphan/--detach is specified.

Yeah, I agree.

>  I did not unify new_branch, new_branch_force and new_orphan_branch.
>  They touch other parts of the code and should probably be done
>  separately.

That sounds like a very sensible decision.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v2 3/3] checkout: reorder option handling

2012-08-29 Thread Nguyễn Thái Ngọc Duy
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 that

 - cmd_checkout() is responsible for parsing, preparing input and
   determinining mode

 - Code of each mode is in cmd_checkout_entry() and cmd_switch(),
   where sanity checks are performed per mode

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 
---
 I'm not entirely sure about this chunk

 +  if (opts->track != BRANCH_TRACK_UNSPECIFIED) {
 +  if (opts->new_orphan_branch)
 +  die(_("%s cannot be used with %s"), "--orphan", "-t");
 +  if (opts->force_detach)
 +  die(_("%s cannot be used with %s"), "--detach", "-t");
 +  } else
 +  opts->track = git_branch_track;

 If we don't want -t and --orphan/--detach together, then we probably should 
ignore
 branch.autosetupmerge when --orphan/--detach is specified.

 I did not unify new_branch, new_branch_force and new_orphan_branch.
 They touch other parts of the code and should probably be done
 separately.

 builtin/checkout.c | 169 -
 1 file changed, 101 insertions(+), 68 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 78abaeb..8289bcd 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -930,6 +930,81 @@ static int switch_unborn_to_new_branch(const struct 
checkout_opts *opts)
return status;
 }
 
+static int cmd_checkout_entry(const struct checkout_opts *opts,
+ const struct branch_info *new)
+{
+   if (opts->track != BRANCH_TRACK_UNSPECIFIED)
+   die(_("%s cannot be used with updating paths"), "--track");
+
+   if (opts->new_branch_log)
+   die(_("%s cannot be used with updating paths"), "-l");
+
+   if (opts->force && opts->patch_mode)
+   die(_("%s cannot be used with updating paths"), "-f");
+
+   if (opts->force_detach)
+   die(_("%s cannot be used with updating paths"), "--detach");
+
+   if (opts->merge && opts->patch_mode)
+   die(_("%s cannot be used with %s"), "--merge", "--patch");
+
+   if (opts->force && opts->merge)
+   die(_("%s cannot be used with %s"), "-f", "-m");
+
+   if (opts->new_branch)
+   die(_("Cannot update paths and switch to branch '%s' at the 
same time."),
+   opts->new_branch);
+
+   if (opts->patch_mode)
+   return interactive_checkout(new->name, opts->pathspec);
+   else
+   return checkout_paths(opts);
+}
+
+static int cmd_switch(struct checkout_opts *opts,
+ struct branch_info *new)
+{
+   if (opts->pathspec)
+   die(_("paths cannot be used with switching branches"));
+
+   if (opts->patch_mode)
+   die(_("%s cannot be used with switching branches"),
+   "--patch");
+
+   if (opts->writeout_stage)
+   die(_("%s cannot be used with switching branches"),
+   "--ours/--theirs");
+
+   if (opts->force && opts->merge)
+   die(_("%s cannot be used with %s"), "-f", "-m");
+
+   if (opts->force_detach && opts->new_branch)
+   die(_("%s cannot be used with %s"),
+   "--detach", "-b/-B/--orphan");
+
+   if (opts->track != BRANCH_TRACK_UNSPECIFIED) {
+   if (opts->new_orphan_branch)
+   die(_("%s cannot be used with %s"), "--orphan", "-t");
+   if (opts->force_detach)
+   die(_("%s cannot be used with %s"), "--detach", "-t");
+   } else
+   opts->track = git_branch_track;
+
+   if (new->name && !new->commit)
+   die(_("Cannot switch branch to a non-commit '%s'."),
+   new->name);
+
+   if (!new->commit && opts->new_branch) {
+   unsigned char rev[20];
+   int flag;
+
+   if (!read_ref_full("HEAD", rev, 0, &flag) &&
+   (flag & REF_ISSYMREF) && is_null_sha1(rev))
+   return switch_unborn_to_new_branch(opts);
+   }
+   return switch_branches(opts, new);
+}
+
 int cmd_checkout(int argc, const char **argv, const char *prefix)
 {
struct checkout_opts opts;
@@ -976,26 +1051,22 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, prefix, options, checkout_usage,
 PARSE_OPT_KEEP_DASHDASH);
 
-   /* we can assume from now on new_branch = !new_branch_force */
-   if (opts.new_branch && opts.new_branch_force)
-   die(_("-B cannot be used with -b"));
+   if (conflict_