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

2012-09-07 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 + if (opts-track != BRANCH_TRACK_UNSPECIFIED)
 + die(_(%s cannot be used with updating paths), --track);

I think most of the places we try to enclose these literals inside
quotes, so I'd squash in a patch to make this:

die(_('%s' cannot be used with updating paths), --track);

and update others to match.

 + if (new-name  !new-commit)
 + die(_(Cannot switch branch to a non-commit '%s'.),
 + new-name);

This is one of the only few places that end a sentence with a full-stop.

 + /* Try to give more helpful suggestion, new_branch 
 +argc  1 will be caught later */

Style;

 + if (opts.new_branch  argc == 1)
 + die(_(Cannot update paths and switch to branch '%s' at 
 the same time.\n
 +   Did you intend to checkout '%s' which can not be 
 resolved as commit?),
 + opts.new_branch, argv[0]);
  
   if (opts.force_detach)
   die(_(git checkout: --detach does not take a path 
 argument));

I tried this codepath and the message left me feeling uneasy.  I'd do:

die(_(git checkout: --detach does not take a path argument 
'%s'),
argv[0]);

Other than these, I didn't find anything obviously wrong with my
final review before merging to 'next', so I'll merge it soonish.

Thanks.
--
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 v3 3/3] checkout: reorder option handling

2012-08-30 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
   determining mode

 - Code of each mode is in checkout_paths() and checkout_branch(),
   where sanity checks are performed

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 pclo...@gmail.com
---
 Changes since v2 (the first two patches are not resent):

  - leave track mode unspecified if --detach/--orphan is specified
  - merge cmd_checkout_entry() into checkout_paths()
  - rename cmd_switch() to checkout_branch()

 builtin/checkout.c | 177 ++---
 1 tập tin đã bị thay đổi, 102 được thêm vào(+), 75 bị xóa(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 78abaeb..b0c5133 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -217,7 +217,8 @@ static int checkout_merged(int pos, struct checkout *state)
return status;
 }
 
-static int checkout_paths(const struct checkout_opts *opts)
+static int checkout_paths(const struct checkout_opts *opts,
+ const char *revision)
 {
int pos;
struct checkout state;
@@ -229,7 +230,35 @@ static int checkout_paths(const struct checkout_opts *opts)
int stage = opts-writeout_stage;
int merge = opts-merge;
int newfd;
-   struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
+   struct lock_file *lock_file;
+
+   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 run_add_interactive(revision, --patch=checkout,
+  opts-pathspec);
+
+   lock_file = xcalloc(1, sizeof(struct lock_file));
 
newfd = hold_locked_index(lock_file, 1);
if (read_cache_preload(opts-pathspec)  0)
@@ -763,11 +792,6 @@ static int git_checkout_config(const char *var, const char 
*value, void *cb)
return git_xmerge_config(var, value, NULL);
 }
 
-static int interactive_checkout(const char *revision, const char **pathspec)
-{
-   return run_add_interactive(revision, --patch=checkout, pathspec);
-}
-
 struct tracking_name_data {
const char *name;
char *remote;
@@ -930,6 +954,51 @@ static int switch_unborn_to_new_branch(const struct 
checkout_opts *opts)
return status;
 }
 
+static int checkout_branch(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-new_orphan_branch) {
+   if (opts-track != BRANCH_TRACK_UNSPECIFIED)
+   die(_(%s cannot be used with %s), --orphan, -t);
+   } else if (opts-force_detach) {
+   if (opts-track != BRANCH_TRACK_UNSPECIFIED)
+   die(_(%s cannot be used with %s), --detach, -t);
+   } else if (opts-track == BRANCH_TRACK_UNSPECIFIED)
+   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);

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

2012-08-30 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:

  Changes since v2 (the first two patches are not resent):
 ...
   - merge cmd_checkout_entry() into checkout_paths()

I didn't think of this when I reviewed the previous one; I think it
makes sense.  Thanks.
--
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