{"thread":{"id":"31369","subject":"[PATCH] checkout: verify new branch name's validity early","startedAt":"2012-08-28T02:29:41Z","lastAt":"2012-09-07T20:19:43Z","messageCount":16,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"197959","messageId":"1346120981-27234-1-git-send-email-pclouds@gmail.com","threadId":"31369","inReplyTo":null,"subject":"[PATCH] checkout: verify new branch name's validity early","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-08-28T02:29:41Z","receivedAt":"2012-08-28T02:29:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If the new branch name to -b/-B happens to be missing, the next\nargument may be mistaken as branch name and no longer recognized by\ncheckout as argument. This may lead to confusing error messages.\n\nBy checking branch name early and printing out the invalid name, users\nmay realize they forgot to specify branch name.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n On Tue, Aug 28, 2012 at 12:03 AM, Junio C Hamano <gitster@pobox.com> wrote:\n > Ideally you would want\n >\n >         fatal: \"-t\" is not an acceptable name for a branch\n >\n > in this case; if it is cumbersome to arrange, at the very least,\n >\n >         updating paths is incompatible with checking out the branch \"-t\".\n >\n > would be clearer.\n\n Fair enough. It turns out we do check branch name's validity. It's\n just too late.\n\n builtin/checkout.c         | 20 ++++++++++----------\n t/t2018-checkout-branch.sh |  5 +++++\n 2 files changed, 15 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex d812219..03b0f25 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1049,6 +1049,16 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \tif (opts.track == BRANCH_TRACK_UNSPECIFIED)\n \t\topts.track = git_branch_track;\n \n+\tif (opts.new_branch) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\topts.branch_exists = validate_new_branchname(opts.new_branch, &buf,\n+\t\t\t\t\t\t\t     !!opts.new_branch_force,\n+\t\t\t\t\t\t\t     !!opts.new_branch_force);\n+\n+\t\tstrbuf_release(&buf);\n+\t}\n+\n \tif (argc) {\n \t\tconst char **pathspec = get_pathspec(prefix, argv);\n \n@@ -1079,16 +1089,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \tif (patch_mode)\n \t\treturn interactive_checkout(new.name, NULL, &opts);\n \n-\tif (opts.new_branch) {\n-\t\tstruct strbuf buf = STRBUF_INIT;\n-\n-\t\topts.branch_exists = validate_new_branchname(opts.new_branch, &buf,\n-\t\t\t\t\t\t\t     !!opts.new_branch_force,\n-\t\t\t\t\t\t\t     !!opts.new_branch_force);\n-\n-\t\tstrbuf_release(&buf);\n-\t}\n-\n \tif (new.name && !new.commit) {\n \t\tdie(_(\"Cannot switch branch to a non-commit.\"));\n \t}\ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 2741262..48ab6a2 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -198,4 +198,9 @@ test_expect_success 'checkout -B to the current branch works' '\n \ttest_dirty_mergeable\n '\n \n+test_expect_success 'checkout -b checks branch validitity early' '\n+\ttest_must_fail git checkout -b -t origin/master 2>err &&\n+\tgrep \"not a valid branch name\" err\n+'\n+\n test_done\n-- \n1.7.12.rc2\n"},{"id":"197961","messageId":"7vr4qroel6.fsf@alter.siamese.dyndns.org","threadId":"31369","inReplyTo":"1346120981-27234-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] checkout: verify new branch name's validity early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-28T03:55:17Z","receivedAt":"2012-08-28T03:55:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> If the new branch name to -b/-B happens to be missing, the next\n> argument may be mistaken as branch name and no longer recognized by\n> checkout as argument. This may lead to confusing error messages.\n>\n> By checking branch name early and printing out the invalid name, users\n> may realize they forgot to specify branch name.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n>  On Tue, Aug 28, 2012 at 12:03 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>  > Ideally you would want\n>  >\n>  >         fatal: \"-t\" is not an acceptable name for a branch\n>  >\n>  > in this case; if it is cumbersome to arrange, at the very least,\n>  >\n>  >         updating paths is incompatible with checking out the branch \"-t\".\n>  >\n>  > would be clearer.\n>\n>  Fair enough. It turns out we do check branch name's validity. It's\n>  just too late.\n\nThe surrounding code is somewhat tricky and the code structure is\nbrittle; there are places that update the opts.new_branch so the new\nlocation of this check has to be after them, and there is one\ncodepath that having a bad value in it does not matter.\n\nI had to check the code outside the context of this patch a few\ntimes to convince myself that this patch does not break them.  I'll\nqueue the patch as-is for now, but we probably would want to see how\nwe can structure it to be less brittle.\n\nThanks.\n\n>  builtin/checkout.c         | 20 ++++++++++----------\n>  t/t2018-checkout-branch.sh |  5 +++++\n>  2 files changed, 15 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index d812219..03b0f25 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1049,6 +1049,16 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n>  \tif (opts.track == BRANCH_TRACK_UNSPECIFIED)\n>  \t\topts.track = git_branch_track;\n>  \n> +\tif (opts.new_branch) {\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\t\topts.branch_exists = validate_new_branchname(opts.new_branch, &buf,\n> +\t\t\t\t\t\t\t     !!opts.new_branch_force,\n> +\t\t\t\t\t\t\t     !!opts.new_branch_force);\n> +\n> +\t\tstrbuf_release(&buf);\n> +\t}\n> +\n>  \tif (argc) {\n>  \t\tconst char **pathspec = get_pathspec(prefix, argv);\n>  \n> @@ -1079,16 +1089,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n>  \tif (patch_mode)\n>  \t\treturn interactive_checkout(new.name, NULL, &opts);\n>  \n> -\tif (opts.new_branch) {\n> -\t\tstruct strbuf buf = STRBUF_INIT;\n> -\n> -\t\topts.branch_exists = validate_new_branchname(opts.new_branch, &buf,\n> -\t\t\t\t\t\t\t     !!opts.new_branch_force,\n> -\t\t\t\t\t\t\t     !!opts.new_branch_force);\n> -\n> -\t\tstrbuf_release(&buf);\n> -\t}\n> -\n>  \tif (new.name && !new.commit) {\n>  \t\tdie(_(\"Cannot switch branch to a non-commit.\"));\n>  \t}\n> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\n> index 2741262..48ab6a2 100755\n> --- a/t/t2018-checkout-branch.sh\n> +++ b/t/t2018-checkout-branch.sh\n> @@ -198,4 +198,9 @@ test_expect_success 'checkout -B to the current branch works' '\n>  \ttest_dirty_mergeable\n>  '\n>  \n> +test_expect_success 'checkout -b checks branch validitity early' '\n> +\ttest_must_fail git checkout -b -t origin/master 2>err &&\n> +\tgrep \"not a valid branch name\" err\n> +'\n> +\n>  test_done\n"},{"id":"197986","messageId":"1346161748-25651-1-git-send-email-pclouds@gmail.com","threadId":"31369","inReplyTo":"7vr4qroel6.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/3] Refactor checkout option handling","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-08-28T13:49:05Z","receivedAt":"2012-08-28T13:49:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Aug 28, 2012 at 10:55 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> The surrounding code is somewhat tricky and the code structure is\n> brittle; there are places that update the opts.new_branch so the new\n> location of this check has to be after them, and there is one\n> codepath that having a bad value in it does not matter.\n>\n> I had to check the code outside the context of this patch a few\n> times to convince myself that this patch does not break them.  I'll\n> queue the patch as-is for now, but we probably would want to see how\n> we can structure it to be less brittle.\n\nI'll give it a shot. On top of master, not the \"checkout -b -t\" patch.\nThe test suite passes, but we'll need more eyeballs for code moves\nlike this.\n\nNguyễn Thái Ngọc Duy (3):\n  checkout: pass \"struct checkout_opts *\" as const pointer\n  checkout: reorder option handling\n  checkout: move branch guessing code out as a separate function\n\n builtin/checkout.c | 290 ++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 164 insertions(+), 126 deletions(-)\n\n-- \n1.7.12.rc2.18.g61b472e\n"},{"id":"197987","messageId":"1346161748-25651-2-git-send-email-pclouds@gmail.com","threadId":"31369","inReplyTo":"1346161748-25651-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 1/3] checkout: pass \"struct checkout_opts *\" as const pointer","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-08-28T13:49:06Z","receivedAt":"2012-08-28T13:49:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This struct contains various switches to change checkout behavior and\nit feels somewhat safer to have the compiler reassure us that nowhere\nelse changes it.\n\nOne field that is changed, writeout_error, is split out and passed as\nanother argument.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/checkout.c | 37 +++++++++++++++++++++----------------\n 1 file changed, 21 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 7d922c6..3e3f086 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -33,7 +33,6 @@ struct checkout_opts {\n \tint force;\n \tint force_detach;\n \tint writeout_stage;\n-\tint writeout_error;\n \tint overwrite_ignore;\n \n \t/* not set by parse_options */\n@@ -216,7 +215,7 @@ static int checkout_merged(int pos, struct checkout *state)\n }\n \n static int checkout_paths(struct tree *source_tree, const char **pathspec,\n-\t\t\t  const char *prefix, struct checkout_opts *opts)\n+\t\t\t  const char *prefix, const struct checkout_opts *opts)\n {\n \tint pos;\n \tstruct checkout state;\n@@ -309,7 +308,8 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,\n \treturn errs;\n }\n \n-static void show_local_changes(struct object *head, struct diff_options *opts)\n+static void show_local_changes(struct object *head,\n+\t\t\t       const struct diff_options *opts)\n {\n \tstruct rev_info rev;\n \t/* I think we want full paths, even if we're in a subdirectory. */\n@@ -331,7 +331,8 @@ static void describe_detached_head(const char *msg, struct commit *commit)\n \tstrbuf_release(&sb);\n }\n \n-static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)\n+static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n+\t\t      int worktree, int *writeout_error)\n {\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc tree_desc;\n@@ -350,7 +351,7 @@ static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)\n \tinit_tree_desc(&tree_desc, tree->buffer, tree->size);\n \tswitch (unpack_trees(1, &tree_desc, &opts)) {\n \tcase -2:\n-\t\to->writeout_error = 1;\n+\t\t*writeout_error = 1;\n \t\t/*\n \t\t * We return 0 nevertheless, as the index is all right\n \t\t * and more importantly we have made best efforts to\n@@ -381,8 +382,10 @@ static void setup_branch_path(struct branch_info *branch)\n \tbranch->path = strbuf_detach(&buf, NULL);\n }\n \n-static int merge_working_tree(struct checkout_opts *opts,\n-\t\t\t      struct branch_info *old, struct branch_info *new)\n+static int merge_working_tree(const struct checkout_opts *opts,\n+\t\t\t      struct branch_info *old,\n+\t\t\t      struct branch_info *new,\n+\t\t\t      int *writeout_error)\n {\n \tint ret;\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n@@ -393,7 +396,7 @@ static int merge_working_tree(struct checkout_opts *opts,\n \n \tresolve_undo_clear();\n \tif (opts->force) {\n-\t\tret = reset_tree(new->commit->tree, opts, 1);\n+\t\tret = reset_tree(new->commit->tree, opts, 1, writeout_error);\n \t\tif (ret)\n \t\t\treturn ret;\n \t} else {\n@@ -479,7 +482,8 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\t\to.verbosity = 0;\n \t\t\twork = write_tree_from_memory(&o);\n \n-\t\t\tret = reset_tree(new->commit->tree, opts, 1);\n+\t\t\tret = reset_tree(new->commit->tree, opts, 1,\n+\t\t\t\t\t writeout_error);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t\to.ancestor = old->name;\n@@ -487,7 +491,8 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\t\to.branch2 = \"local\";\n \t\t\tmerge_trees(&o, new->commit->tree, work,\n \t\t\t\told->commit->tree, &result);\n-\t\t\tret = reset_tree(new->commit->tree, opts, 0);\n+\t\t\tret = reset_tree(new->commit->tree, opts, 0,\n+\t\t\t\t\t writeout_error);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t}\n@@ -514,7 +519,7 @@ static void report_tracking(struct branch_info *new)\n \tstrbuf_release(&sb);\n }\n \n-static void update_refs_for_switch(struct checkout_opts *opts,\n+static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t\t   struct branch_info *old,\n \t\t\t\t   struct branch_info *new)\n {\n@@ -701,13 +706,13 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new)\n \tfree(refs.objects);\n }\n \n-static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n+static int switch_branches(const struct checkout_opts *opts, struct branch_info *new)\n {\n \tint ret = 0;\n \tstruct branch_info old;\n \tvoid *path_to_free;\n \tunsigned char rev[20];\n-\tint flag;\n+\tint flag, writeout_error = 0;\n \tmemset(&old, 0, sizeof(old));\n \told.path = path_to_free = resolve_refdup(\"HEAD\", rev, 0, &flag);\n \told.commit = lookup_commit_reference_gently(rev, 1);\n@@ -725,7 +730,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \t\tparse_commit(new->commit);\n \t}\n \n-\tret = merge_working_tree(opts, &old, new);\n+\tret = merge_working_tree(opts, &old, new, &writeout_error);\n \tif (ret) {\n \t\tfree(path_to_free);\n \t\treturn ret;\n@@ -738,7 +743,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \n \tret = post_checkout_hook(old.commit, new->commit, 1);\n \tfree(path_to_free);\n-\treturn ret || opts->writeout_error;\n+\treturn ret || writeout_error;\n }\n \n static int git_checkout_config(const char *var, const char *value, void *cb)\n@@ -910,7 +915,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n \treturn argcount;\n }\n \n-static int switch_unborn_to_new_branch(struct checkout_opts *opts)\n+static int switch_unborn_to_new_branch(const struct checkout_opts *opts)\n {\n \tint status;\n \tstruct strbuf branch_ref = STRBUF_INIT;\n-- \n1.7.12.rc2.18.g61b472e\n"},{"id":"197988","messageId":"1346161748-25651-3-git-send-email-pclouds@gmail.com","threadId":"31369","inReplyTo":"1346161748-25651-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/3] checkout: reorder option handling","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-08-28T13:49:07Z","receivedAt":"2012-08-28T13:49:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"checkout operates in three different modes. On top of that it tries to\nbe smart by guessing the branch name for switching. This results in\nmessy option handling code. This patch reorders it so:\n\n - easy option handling comes first\n - the big chunk of branch name guessing comes next\n - mode detection comes last. Once the mode is known, check again to\n   see if users specify any incompatible options\n - the actual action is done\n\nAnother slight improvement is always print branch name (or commit\nname) when printing errors related ot them. This helps catch the case\nwhere an option is mistaken as branch/commit.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/checkout.c | 153 +++++++++++++++++++++++++++++++----------------------\n 1 file changed, 89 insertions(+), 64 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3e3f086..3f1a436 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -760,8 +760,7 @@ static int git_checkout_config(const char *var, const char *value, void *cb)\n \treturn git_xmerge_config(var, value, NULL);\n }\n \n-static int interactive_checkout(const char *revision, const char **pathspec,\n-\t\t\t\tstruct checkout_opts *opts)\n+static int interactive_checkout(const char *revision, const char **pathspec)\n {\n \treturn run_add_interactive(revision, \"--patch=checkout\", pathspec);\n }\n@@ -937,6 +936,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \tchar *conflict_style = NULL;\n \tint patch_mode = 0;\n \tint dwim_new_local_branch = 1;\n+\tconst char **pathspec = NULL;\n \tstruct option options[] = {\n \t\tOPT__QUIET(&opts.quiet, \"suppress progress reporting\"),\n \t\tOPT_STRING('b', NULL, &opts.new_branch, \"branch\",\n@@ -976,23 +976,45 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, checkout_usage,\n \t\t\t     PARSE_OPT_KEEP_DASHDASH);\n \n-\t/* we can assume from now on new_branch = !new_branch_force */\n-\tif (opts.new_branch && opts.new_branch_force)\n-\t\tdie(_(\"-B cannot be used with -b\"));\n-\n-\t/* copy -B over to -b, so that we can just check the latter */\n-\tif (opts.new_branch_force)\n-\t\topts.new_branch = opts.new_branch_force;\n-\n-\tif (patch_mode && (opts.track > 0 || opts.new_branch\n-\t\t\t   || opts.new_branch_log || opts.merge || opts.force\n-\t\t\t   || opts.force_detach))\n-\t\tdie (_(\"--patch is incompatible with all other options\"));\n-\n+\t/* Easy mode-independent incompatible checks */\n \tif (opts.force_detach && (opts.new_branch || opts.new_orphan_branch))\n \t\tdie(_(\"--detach cannot be used with -b/-B/--orphan\"));\n \tif (opts.force_detach && 0 < opts.track)\n \t\tdie(_(\"--detach cannot be used with -t\"));\n+\tif (opts.force && opts.merge)\n+\t\tdie(_(\"git checkout: -f and -m are incompatible\"));\n+\n+\tif (conflict_style) {\n+\t\topts.merge = 1; /* implied */\n+\t\tgit_xmerge_config(\"merge.conflictstyle\", conflict_style, NULL);\n+\t}\n+\n+\t/*\n+\t * opts.new_branch calculation, it could be from -b or -B or\n+\t * --track or --orphan or other command line arguments.\n+\t *\n+\t * The following code until new branch validation should not\n+\t * update any fields in opts but opts.new_branch.\n+\t */\n+\tif (opts.new_branch_force) {\n+\t\t/* we can assume from now on new_branch = !new_branch_force */\n+\t\tif (opts.new_branch)\n+\t\t\tdie(_(\"New branch '%s' is already specified by other options\"),\n+\t\t\t    opts.new_branch);\n+\n+\t\t/* copy -B over to -b, so that we can just check the latter */\n+\t\tif (opts.new_branch_force)\n+\t\t\topts.new_branch = opts.new_branch_force;\n+\t}\n+\n+\tif (opts.new_orphan_branch) {\n+\t\tif (opts.track > 0)\n+\t\t\tdie(_(\"--orphan cannot be used with -t\"));\n+\t\tif (opts.new_branch)\n+\t\t\tdie(_(\"New branch '%s' is already specified by other options\"),\n+\t\t\t    opts.new_branch);\n+\t\topts.new_branch = opts.new_orphan_branch;\n+\t}\n \n \t/* --track without -b should DWIM */\n \tif (0 < opts.track && !opts.new_branch) {\n@@ -1009,22 +1031,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\topts.new_branch = argv0 + 1;\n \t}\n \n-\tif (opts.new_orphan_branch) {\n-\t\tif (opts.new_branch)\n-\t\t\tdie(_(\"--orphan and -b|-B are mutually exclusive\"));\n-\t\tif (opts.track > 0)\n-\t\t\tdie(_(\"--orphan cannot be used with -t\"));\n-\t\topts.new_branch = opts.new_orphan_branch;\n-\t}\n-\n-\tif (conflict_style) {\n-\t\topts.merge = 1; /* implied */\n-\t\tgit_xmerge_config(\"merge.conflictstyle\", conflict_style, NULL);\n-\t}\n-\n-\tif (opts.force && opts.merge)\n-\t\tdie(_(\"git checkout: -f and -m are incompatible\"));\n-\n \t/*\n \t * Extract branch name from command line arguments, so\n \t * all that is left is pathspecs.\n@@ -1050,39 +1056,29 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\targc -= n;\n \t}\n \n-\tif (opts.track == BRANCH_TRACK_UNSPECIFIED)\n-\t\topts.track = git_branch_track;\n-\n+\t/* After DWIM, these must be pathspec */\n \tif (argc) {\n-\t\tconst char **pathspec = get_pathspec(prefix, argv);\n+\t\tpathspec = get_pathspec(prefix, argv);\n \n \t\tif (!pathspec)\n \t\t\tdie(_(\"invalid path specification\"));\n \n-\t\tif (patch_mode)\n-\t\t\treturn interactive_checkout(new.name, pathspec, &opts);\n-\n-\t\t/* Checkout paths */\n-\t\tif (opts.new_branch) {\n-\t\t\tif (argc == 1) {\n-\t\t\t\tdie(_(\"git checkout: updating paths is incompatible with switching branches.\\nDid you intend to checkout '%s' which can not be resolved as commit?\"), argv[0]);\n-\t\t\t} else {\n-\t\t\t\tdie(_(\"git checkout: updating paths is incompatible with switching branches.\"));\n-\t\t\t}\n-\t\t}\n+\t\t/* Try to give more helpful suggestion, new_branch &&\n+\t\t   argc > 1 will be caught later */\n+\t\tif (opts.new_branch && argc == 1)\n+\t\t\tdie(_(\"Cannot update paths and switch to branch '%s' at the same time.\\n\"\n+\t\t\t      \"Did you intend to checkout '%s' which can not be resolved as commit?\"),\n+\t\t\t    opts.new_branch, argv[0]);\n \n \t\tif (opts.force_detach)\n \t\t\tdie(_(\"git checkout: --detach does not take a path argument\"));\n \n \t\tif (1 < !!opts.writeout_stage + !!opts.force + !!opts.merge)\n-\t\t\tdie(_(\"git checkout: --ours/--theirs, --force and --merge are incompatible when\\nchecking out of the index.\"));\n-\n-\t\treturn checkout_paths(source_tree, pathspec, prefix, &opts);\n+\t\t\tdie(_(\"git checkout: --ours/--theirs, --force and --merge are incompatible when\\n\"\n+\t\t\t      \"checking out of the index.\"));\n \t}\n \n-\tif (patch_mode)\n-\t\treturn interactive_checkout(new.name, NULL, &opts);\n-\n+\t/* new branch calculation done, validate it */\n \tif (opts.new_branch) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n@@ -1093,19 +1089,48 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_release(&buf);\n \t}\n \n-\tif (new.name && !new.commit) {\n-\t\tdie(_(\"Cannot switch branch to a non-commit.\"));\n-\t}\n-\tif (opts.writeout_stage)\n-\t\tdie(_(\"--ours/--theirs is incompatible with switching branches.\"));\n+\tif (patch_mode || pathspec) { /* checking out entries */\n+\t\tif (opts.track > 0)\n+\t\t\tdie(_(\"%s cannot be used while checking out paths\"),\n+\t\t\t    \"--track\");\n+\t\tif (opts.new_branch_log)\n+\t\t\tdie(_(\"%s cannot be used while checking out paths\"),\n+\t\t\t    \"-l\");\n+\t\tif (opts.merge && patch_mode)\n+\t\t\tdie(_(\"--merge cannot be used with --patch\"));\n+\t\tif (opts.force && patch_mode)\n+\t\t\tdie(_(\"%s cannot be used while checking out paths\"),\n+\t\t\t    \"-f\");\n+\t\tif (opts.force_detach)\n+\t\t\tdie(_(\"%s cannot be used while checking out paths\"),\n+\t\t\t    \"--detach\");\n+\t\tif (opts.new_branch)\n+\t\t\tdie(_(\"Cannot update paths and switch to branch '%s' at the same time.\"),\n+\t\t\t    opts.new_branch);\n+\n+\t\tif (patch_mode)\n+\t\t\treturn interactive_checkout(new.name, pathspec);\n+\t\telse\n+\t\t\treturn checkout_paths(source_tree, pathspec, prefix, &opts);\n+\t} else {\t\t/* switch branch */\n+\t\tif (new.name && !new.commit)\n+\t\t\tdie(_(\"Cannot switch branch to a non-commit '%s'.\"),\n+\t\t\t    new.name);\n \n-\tif (!new.commit && opts.new_branch) {\n-\t\tunsigned char rev[20];\n-\t\tint flag;\n+\t\tif (opts.writeout_stage)\n+\t\t\tdie(_(\"--ours/--theirs is incompatible with switching branches.\"));\n \n-\t\tif (!read_ref_full(\"HEAD\", rev, 0, &flag) &&\n-\t\t    (flag & REF_ISSYMREF) && is_null_sha1(rev))\n-\t\t\treturn switch_unborn_to_new_branch(&opts);\n+\t\tif (opts.track == BRANCH_TRACK_UNSPECIFIED)\n+\t\t\topts.track = git_branch_track;\n+\n+\t\tif (!new.commit && opts.new_branch) {\n+\t\t\tunsigned char rev[20];\n+\t\t\tint flag;\n+\n+\t\t\tif (!read_ref_full(\"HEAD\", rev, 0, &flag) &&\n+\t\t\t    (flag & REF_ISSYMREF) && is_null_sha1(rev))\n+\t\t\t\treturn switch_unborn_to_new_branch(&opts);\n+\t\t}\n+\t\treturn switch_branches(&opts, &new);\n \t}\n-\treturn switch_branches(&opts, &new);\n }\n-- \n1.7.12.rc2.18.g61b472e\n"},{"id":"197989","messageId":"1346161748-25651-4-git-send-email-pclouds@gmail.com","threadId":"31369","inReplyTo":"1346161748-25651-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/3] checkout: move branch guessing code out as a separate function","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-08-28T13:49:08Z","receivedAt":"2012-08-28T13:49:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This makes cmd_checkout() shorter, therefore easier to get the big\npicture.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n In general I like it, except that parse_branchname_arg() pulls so many\n options from cmd_checkout(). I would rather have update_new_branch()\n that only takes \"struct checkout_opts *\" and returns a new branch. So\n I'm not sure if we should do this. If we do, perhaps we can merge it\n into the previous patch.\n\n builtin/checkout.c | 184 ++++++++++++++++++++++++++++-------------------------\n 1 file changed, 96 insertions(+), 88 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3f1a436..e9d503d 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -914,6 +914,99 @@ static int parse_branchname_arg(int argc, const char **argv,\n \treturn argcount;\n }\n \n+static const char *update_new_branch(const struct checkout_opts *opts,\n+\t\t\t\t     int argc, const char **argv,\n+\t\t\t\t     const char *prefix, const char ***pathspec,\n+\t\t\t\t     int dwim_ok,\n+\t\t\t\t     struct branch_info *new,\n+\t\t\t\t     struct tree **source_tree,\n+\t\t\t\t     unsigned char *rev)\n+{\n+\tconst char *branch = opts->new_branch;\n+\n+\tif (opts->new_branch_force) {\n+\t\t/* we can assume from now on new_branch = !new_branch_force */\n+\t\tif (branch)\n+\t\t\tdie(_(\"New branch '%s' is already specified by other options\"),\n+\t\t\t    branch);\n+\n+\t\t/* copy -B over to -b, so that we can just check the latter */\n+\t\tif (opts->new_branch_force)\n+\t\t\tbranch = opts->new_branch_force;\n+\t}\n+\n+\tif (opts->new_orphan_branch) {\n+\t\tif (opts->track > 0)\n+\t\t\tdie(_(\"--orphan cannot be used with -t\"));\n+\t\tif (branch)\n+\t\t\tdie(_(\"New branch '%s' is already specified by other options\"),\n+\t\t\t    branch);\n+\t\tbranch = opts->new_orphan_branch;\n+\t}\n+\n+\t/* --track without -b should DWIM */\n+\tif (0 < opts->track && !branch) {\n+\t\tconst char *argv0 = argv[0];\n+\t\tif (!argc || !strcmp(argv0, \"--\"))\n+\t\t\tdie (_(\"--track needs a branch name\"));\n+\t\tif (!prefixcmp(argv0, \"refs/\"))\n+\t\t\targv0 += 5;\n+\t\tif (!prefixcmp(argv0, \"remotes/\"))\n+\t\t\targv0 += 8;\n+\t\targv0 = strchr(argv0, '/');\n+\t\tif (!argv0 || !argv0[1])\n+\t\t\tdie (_(\"Missing branch name; try -b\"));\n+\t\tbranch = argv0 + 1;\n+\t}\n+\n+\t/*\n+\t * Extract branch name from command line arguments, so\n+\t * all that is left is pathspecs.\n+\t *\n+\t * Handle\n+\t *\n+\t *  1) git checkout <tree> -- [<paths>]\n+\t *  2) git checkout -- [<paths>]\n+\t *  3) git checkout <something> [<paths>]\n+\t *\n+\t * including \"last branch\" syntax and DWIM-ery for names of\n+\t * remote branches, erroring out for invalid or ambiguous cases.\n+\t */\n+\tif (argc) {\n+\t\tdwim_ok = dwim_ok &&\n+\t\t\topts->track == BRANCH_TRACK_UNSPECIFIED &&\n+\t\t\t!branch;\n+\t\tint n = parse_branchname_arg(argc, argv, dwim_ok,\n+\t\t\t\t\t     new, source_tree, rev, &branch);\n+\t\targv += n;\n+\t\targc -= n;\n+\t}\n+\n+\t/* After DWIM, these must be pathspec */\n+\tif (argc) {\n+\t\t*pathspec = get_pathspec(prefix, argv);\n+\n+\t\tif (!*pathspec)\n+\t\t\tdie(_(\"invalid path specification\"));\n+\n+\t\t/* Try to give more helpful suggestion, new_branch &&\n+\t\t   argc > 1 will be caught later */\n+\t\tif (branch && argc == 1)\n+\t\t\tdie(_(\"Cannot update paths and switch to branch '%s' at the same time.\\n\"\n+\t\t\t      \"Did you intend to checkout '%s' which can not be resolved as commit?\"),\n+\t\t\t    branch, argv[0]);\n+\n+\t\tif (opts->force_detach)\n+\t\t\tdie(_(\"git checkout: --detach does not take a path argument\"));\n+\n+\t\tif (1 < !!opts->writeout_stage + !!opts->force + !!opts->merge)\n+\t\t\tdie(_(\"git checkout: --ours/--theirs, --force and --merge are incompatible when\\n\"\n+\t\t\t      \"checking out of the index.\"));\n+\t}\n+\n+\treturn branch;\n+}\n+\n static int switch_unborn_to_new_branch(const struct checkout_opts *opts)\n {\n \tint status;\n@@ -989,94 +1082,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\tgit_xmerge_config(\"merge.conflictstyle\", conflict_style, NULL);\n \t}\n \n-\t/*\n-\t * opts.new_branch calculation, it could be from -b or -B or\n-\t * --track or --orphan or other command line arguments.\n-\t *\n-\t * The following code until new branch validation should not\n-\t * update any fields in opts but opts.new_branch.\n-\t */\n-\tif (opts.new_branch_force) {\n-\t\t/* we can assume from now on new_branch = !new_branch_force */\n-\t\tif (opts.new_branch)\n-\t\t\tdie(_(\"New branch '%s' is already specified by other options\"),\n-\t\t\t    opts.new_branch);\n-\n-\t\t/* copy -B over to -b, so that we can just check the latter */\n-\t\tif (opts.new_branch_force)\n-\t\t\topts.new_branch = opts.new_branch_force;\n-\t}\n-\n-\tif (opts.new_orphan_branch) {\n-\t\tif (opts.track > 0)\n-\t\t\tdie(_(\"--orphan cannot be used with -t\"));\n-\t\tif (opts.new_branch)\n-\t\t\tdie(_(\"New branch '%s' is already specified by other options\"),\n-\t\t\t    opts.new_branch);\n-\t\topts.new_branch = opts.new_orphan_branch;\n-\t}\n-\n-\t/* --track without -b should DWIM */\n-\tif (0 < opts.track && !opts.new_branch) {\n-\t\tconst char *argv0 = argv[0];\n-\t\tif (!argc || !strcmp(argv0, \"--\"))\n-\t\t\tdie (_(\"--track needs a branch name\"));\n-\t\tif (!prefixcmp(argv0, \"refs/\"))\n-\t\t\targv0 += 5;\n-\t\tif (!prefixcmp(argv0, \"remotes/\"))\n-\t\t\targv0 += 8;\n-\t\targv0 = strchr(argv0, '/');\n-\t\tif (!argv0 || !argv0[1])\n-\t\t\tdie (_(\"Missing branch name; try -b\"));\n-\t\topts.new_branch = argv0 + 1;\n-\t}\n-\n-\t/*\n-\t * Extract branch name from command line arguments, so\n-\t * all that is left is pathspecs.\n-\t *\n-\t * Handle\n-\t *\n-\t *  1) git checkout <tree> -- [<paths>]\n-\t *  2) git checkout -- [<paths>]\n-\t *  3) git checkout <something> [<paths>]\n-\t *\n-\t * including \"last branch\" syntax and DWIM-ery for names of\n-\t * remote branches, erroring out for invalid or ambiguous cases.\n-\t */\n-\tif (argc) {\n-\t\tint dwim_ok =\n-\t\t\t!patch_mode &&\n-\t\t\tdwim_new_local_branch &&\n-\t\t\topts.track == BRANCH_TRACK_UNSPECIFIED &&\n-\t\t\t!opts.new_branch;\n-\t\tint n = parse_branchname_arg(argc, argv, dwim_ok,\n-\t\t\t\t&new, &source_tree, rev, &opts.new_branch);\n-\t\targv += n;\n-\t\targc -= n;\n-\t}\n-\n-\t/* After DWIM, these must be pathspec */\n-\tif (argc) {\n-\t\tpathspec = get_pathspec(prefix, argv);\n-\n-\t\tif (!pathspec)\n-\t\t\tdie(_(\"invalid path specification\"));\n-\n-\t\t/* Try to give more helpful suggestion, new_branch &&\n-\t\t   argc > 1 will be caught later */\n-\t\tif (opts.new_branch && argc == 1)\n-\t\t\tdie(_(\"Cannot update paths and switch to branch '%s' at the same time.\\n\"\n-\t\t\t      \"Did you intend to checkout '%s' which can not be resolved as commit?\"),\n-\t\t\t    opts.new_branch, argv[0]);\n-\n-\t\tif (opts.force_detach)\n-\t\t\tdie(_(\"git checkout: --detach does not take a path argument\"));\n-\n-\t\tif (1 < !!opts.writeout_stage + !!opts.force + !!opts.merge)\n-\t\t\tdie(_(\"git checkout: --ours/--theirs, --force and --merge are incompatible when\\n\"\n-\t\t\t      \"checking out of the index.\"));\n-\t}\n+\topts.new_branch = update_new_branch(&opts, argc, argv, prefix, &pathspec,\n+\t\t\t\t\t    !patch_mode && dwim_new_local_branch,\n+\t\t\t\t\t    &new, &source_tree, rev);\n \n \t/* new branch calculation done, validate it */\n \tif (opts.new_branch) {\n-- \n1.7.12.rc2.18.g61b472e\n"},{"id":"198013","messageId":"7vipc2zqxg.fsf@alter.siamese.dyndns.org","threadId":"31369","inReplyTo":"1346161748-25651-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/3] checkout: reorder option handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-28T20:45:31Z","receivedAt":"2012-08-28T20:45:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> checkout operates in three different modes. On top of that it tries to\n> be smart by guessing the branch name for switching. This results in\n> messy option handling code. This patch reorders it so:\n>\n>  - easy option handling comes first\n>  - the big chunk of branch name guessing comes next\n>  - mode detection comes last. Once the mode is known, check again to\n>    see if users specify any incompatible options\n>  - the actual action is done\n>\n> Another slight improvement is always print branch name (or commit\n> name) when printing errors related ot them. This helps catch the case\n> where an option is mistaken as branch/commit.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n> ...\n> +\t/* Easy mode-independent incompatible checks */\n>  \tif (opts.force_detach && (opts.new_branch || opts.new_orphan_branch))\n>  \t\tdie(_(\"--detach cannot be used with -b/-B/--orphan\"));\n\nDid you catch \"--detach -B\" combination without checking new_branch_force?\n\n>  \tif (opts.force_detach && 0 < opts.track)\n>  \t\tdie(_(\"--detach cannot be used with -t\"));\n> +\tif (opts.force && opts.merge)\n> +\t\tdie(_(\"git checkout: -f and -m are incompatible\"));\n> +\n> +\tif (conflict_style) {\n> +\t\topts.merge = 1; /* implied */\n> +\t\tgit_xmerge_config(\"merge.conflictstyle\", conflict_style, NULL);\n> +\t}\n\n\"checkout --conflict=diff3 -f <branch>\" would imply combination of\n\"-m\" and \"-f\", which is supposed to be forbidden in the previous\ncheck, no?\n\nI very much like the idea of separating things in phases like your\nproposed log message explains.  But I wonder if the order should be\nto do dwimming and parameter canonicalization first, then decide the\nmode (these might have to be swapped, as the same parameter may dwim\ndown to different things depending on the mode), and finally check\nfor sanity before performing.\n\nTo avoid confusion, it also might not be a bad idea to remove\nnew_branch_force and new_orphan_branch from the structure and\nintroduce \"enum branch_creation_type\" or something, and always have\nthe new branch name in \"new_branch\" field (this needs to get various\npointers into opts out of the parseopt options[] array; parse into\nseparate variables and decide what to put in \"struct checkout_opts\"),\nindependent from how that branch is going to be created (either -b,\n-B, or --orphan).\n"},{"id":"198039","messageId":"CACsJy8Cjbg2tb_E+D7hMwPZNFWhvEF1eNpf5HUBzJnBoQQaMAg@mail.gmail.com","threadId":"31369","inReplyTo":"7vipc2zqxg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] checkout: reorder option handling","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-08-29T12:16:10Z","receivedAt":"2012-08-29T12:16:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 29, 2012 at 3:45 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> +     /* Easy mode-independent incompatible checks */\n>>       if (opts.force_detach && (opts.new_branch || opts.new_orphan_branch))\n>>               die(_(\"--detach cannot be used with -b/-B/--orphan\"));\n>\n> Did you catch \"--detach -B\" combination without checking new_branch_force?\n\nI did not. Will move this check further down once opts.new_branch is final.\n\n>>       if (opts.force_detach && 0 < opts.track)\n>>               die(_(\"--detach cannot be used with -t\"));\n>> +     if (opts.force && opts.merge)\n>> +             die(_(\"git checkout: -f and -m are incompatible\"));\n>> +\n>> +     if (conflict_style) {\n>> +             opts.merge = 1; /* implied */\n>> +             git_xmerge_config(\"merge.conflictstyle\", conflict_style, NULL);\n>> +     }\n>\n> \"checkout --conflict=diff3 -f <branch>\" would imply combination of\n> \"-m\" and \"-f\", which is supposed to be forbidden in the previous\n> check, no?\n\nYep. That last \"if\" must be moved up.\n\n> I very much like the idea of separating things in phases like your\n> proposed log message explains.  But I wonder if the order should be\n> to do dwimming and parameter canonicalization first, then decide the\n> mode (these might have to be swapped, as the same parameter may dwim\n> down to different things depending on the mode), and finally check\n> for sanity before performing.\n\nI do a few sanity checks after the mode has been decided and obviously\nmissed some as you pointed out above. Will move them all down. So\nparameter canonicalization, dwim, then separate code paths for each\nmode, which includes sanity checks.\n\n> To avoid confusion, it also might not be a bad idea to remove\n> new_branch_force and new_orphan_branch from the structure and\n> introduce \"enum branch_creation_type\" or something, and always have\n> the new branch name in \"new_branch\" field (this needs to get various\n> pointers into opts out of the parseopt options[] array; parse into\n> separate variables and decide what to put in \"struct checkout_opts\"),\n> independent from how that branch is going to be created (either -b,\n> -B, or --orphan).\n\nOK\n-- \nDuy\n"},{"id":"198040","messageId":"1346248524-11616-1-git-send-email-pclouds@gmail.com","threadId":"31369","inReplyTo":"CACsJy8Cjbg2tb_E+D7hMwPZNFWhvEF1eNpf5HUBzJnBoQQaMAg@mail.gmail.com","subject":"[PATCH v2 1/3] checkout: pass \"struct checkout_opts *\" as const pointer","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-08-29T13:55:22Z","receivedAt":"2012-08-29T13:55:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This struct contains various switches to system and it feels somewhat\nsafer to have the compiler reassure us that nowhere else changes it.\n\nOne field that is changed, writeout_error, is split out and passed as\nanother argument.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/checkout.c | 37 +++++++++++++++++++++----------------\n 1 file changed, 21 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 7d922c6..3e3f086 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -33,7 +33,6 @@ struct checkout_opts {\n \tint force;\n \tint force_detach;\n \tint writeout_stage;\n-\tint writeout_error;\n \tint overwrite_ignore;\n \n \t/* not set by parse_options */\n@@ -216,7 +215,7 @@ static int checkout_merged(int pos, struct checkout *state)\n }\n \n static int checkout_paths(struct tree *source_tree, const char **pathspec,\n-\t\t\t  const char *prefix, struct checkout_opts *opts)\n+\t\t\t  const char *prefix, const struct checkout_opts *opts)\n {\n \tint pos;\n \tstruct checkout state;\n@@ -309,7 +308,8 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,\n \treturn errs;\n }\n \n-static void show_local_changes(struct object *head, struct diff_options *opts)\n+static void show_local_changes(struct object *head,\n+\t\t\t       const struct diff_options *opts)\n {\n \tstruct rev_info rev;\n \t/* I think we want full paths, even if we're in a subdirectory. */\n@@ -331,7 +331,8 @@ static void describe_detached_head(const char *msg, struct commit *commit)\n \tstrbuf_release(&sb);\n }\n \n-static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)\n+static int reset_tree(struct tree *tree, const struct checkout_opts *o,\n+\t\t      int worktree, int *writeout_error)\n {\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc tree_desc;\n@@ -350,7 +351,7 @@ static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)\n \tinit_tree_desc(&tree_desc, tree->buffer, tree->size);\n \tswitch (unpack_trees(1, &tree_desc, &opts)) {\n \tcase -2:\n-\t\to->writeout_error = 1;\n+\t\t*writeout_error = 1;\n \t\t/*\n \t\t * We return 0 nevertheless, as the index is all right\n \t\t * and more importantly we have made best efforts to\n@@ -381,8 +382,10 @@ static void setup_branch_path(struct branch_info *branch)\n \tbranch->path = strbuf_detach(&buf, NULL);\n }\n \n-static int merge_working_tree(struct checkout_opts *opts,\n-\t\t\t      struct branch_info *old, struct branch_info *new)\n+static int merge_working_tree(const struct checkout_opts *opts,\n+\t\t\t      struct branch_info *old,\n+\t\t\t      struct branch_info *new,\n+\t\t\t      int *writeout_error)\n {\n \tint ret;\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n@@ -393,7 +396,7 @@ static int merge_working_tree(struct checkout_opts *opts,\n \n \tresolve_undo_clear();\n \tif (opts->force) {\n-\t\tret = reset_tree(new->commit->tree, opts, 1);\n+\t\tret = reset_tree(new->commit->tree, opts, 1, writeout_error);\n \t\tif (ret)\n \t\t\treturn ret;\n \t} else {\n@@ -479,7 +482,8 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\t\to.verbosity = 0;\n \t\t\twork = write_tree_from_memory(&o);\n \n-\t\t\tret = reset_tree(new->commit->tree, opts, 1);\n+\t\t\tret = reset_tree(new->commit->tree, opts, 1,\n+\t\t\t\t\t writeout_error);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t\to.ancestor = old->name;\n@@ -487,7 +491,8 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\t\to.branch2 = \"local\";\n \t\t\tmerge_trees(&o, new->commit->tree, work,\n \t\t\t\told->commit->tree, &result);\n-\t\t\tret = reset_tree(new->commit->tree, opts, 0);\n+\t\t\tret = reset_tree(new->commit->tree, opts, 0,\n+\t\t\t\t\t writeout_error);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t}\n@@ -514,7 +519,7 @@ static void report_tracking(struct branch_info *new)\n \tstrbuf_release(&sb);\n }\n \n-static void update_refs_for_switch(struct checkout_opts *opts,\n+static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t\t   struct branch_info *old,\n \t\t\t\t   struct branch_info *new)\n {\n@@ -701,13 +706,13 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new)\n \tfree(refs.objects);\n }\n \n-static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n+static int switch_branches(const struct checkout_opts *opts, struct branch_info *new)\n {\n \tint ret = 0;\n \tstruct branch_info old;\n \tvoid *path_to_free;\n \tunsigned char rev[20];\n-\tint flag;\n+\tint flag, writeout_error = 0;\n \tmemset(&old, 0, sizeof(old));\n \told.path = path_to_free = resolve_refdup(\"HEAD\", rev, 0, &flag);\n \told.commit = lookup_commit_reference_gently(rev, 1);\n@@ -725,7 +730,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \t\tparse_commit(new->commit);\n \t}\n \n-\tret = merge_working_tree(opts, &old, new);\n+\tret = merge_working_tree(opts, &old, new, &writeout_error);\n \tif (ret) {\n \t\tfree(path_to_free);\n \t\treturn ret;\n@@ -738,7 +743,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \n \tret = post_checkout_hook(old.commit, new->commit, 1);\n \tfree(path_to_free);\n-\treturn ret || opts->writeout_error;\n+\treturn ret || writeout_error;\n }\n \n static int git_checkout_config(const char *var, const char *value, void *cb)\n@@ -910,7 +915,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n \treturn argcount;\n }\n \n-static int switch_unborn_to_new_branch(struct checkout_opts *opts)\n+static int switch_unborn_to_new_branch(const struct checkout_opts *opts)\n {\n \tint status;\n \tstruct strbuf branch_ref = STRBUF_INIT;\n-- \n1.7.12.rc2.18.g61b472e\n"},{"id":"198041","messageId":"1346248524-11616-2-git-send-email-pclouds@gmail.com","threadId":"31369","inReplyTo":"1346248524-11616-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 2/3] checkout: move more parameters to struct checkout_opts","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-08-29T13:55:23Z","receivedAt":"2012-08-29T13:55:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/checkout.c | 68 ++++++++++++++++++++++++++++--------------------------\n 1 file changed, 35 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3e3f086..78abaeb 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -28,6 +28,7 @@ static const char * const checkout_usage[] = {\n };\n \n struct checkout_opts {\n+\tint patch_mode;\n \tint quiet;\n \tint merge;\n \tint force;\n@@ -35,15 +36,17 @@ struct checkout_opts {\n \tint writeout_stage;\n \tint overwrite_ignore;\n \n-\t/* not set by parse_options */\n-\tint branch_exists;\n-\n \tconst char *new_branch;\n \tconst char *new_branch_force;\n \tconst char *new_orphan_branch;\n \tint new_branch_log;\n \tenum branch_track track;\n \tstruct diff_options diff_options;\n+\n+\tint branch_exists;\n+\tconst char *prefix;\n+\tconst char **pathspec;\n+\tstruct tree *source_tree;\n };\n \n static int post_checkout_hook(struct commit *old, struct commit *new,\n@@ -214,8 +217,7 @@ static int checkout_merged(int pos, struct checkout *state)\n \treturn status;\n }\n \n-static int checkout_paths(struct tree *source_tree, const char **pathspec,\n-\t\t\t  const char *prefix, const struct checkout_opts *opts)\n+static int checkout_paths(const struct checkout_opts *opts)\n {\n \tint pos;\n \tstruct checkout state;\n@@ -230,34 +232,34 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n \n \tnewfd = hold_locked_index(lock_file, 1);\n-\tif (read_cache_preload(pathspec) < 0)\n+\tif (read_cache_preload(opts->pathspec) < 0)\n \t\treturn error(_(\"corrupt index file\"));\n \n-\tif (source_tree)\n-\t\tread_tree_some(source_tree, pathspec);\n+\tif (opts->source_tree)\n+\t\tread_tree_some(opts->source_tree, opts->pathspec);\n \n-\tfor (pos = 0; pathspec[pos]; pos++)\n+\tfor (pos = 0; opts->pathspec[pos]; pos++)\n \t\t;\n \tps_matched = xcalloc(1, pos);\n \n \tfor (pos = 0; pos < active_nr; pos++) {\n \t\tstruct cache_entry *ce = active_cache[pos];\n-\t\tif (source_tree && !(ce->ce_flags & CE_UPDATE))\n+\t\tif (opts->source_tree && !(ce->ce_flags & CE_UPDATE))\n \t\t\tcontinue;\n-\t\tmatch_pathspec(pathspec, ce->name, ce_namelen(ce), 0, ps_matched);\n+\t\tmatch_pathspec(opts->pathspec, ce->name, ce_namelen(ce), 0, ps_matched);\n \t}\n \n-\tif (report_path_error(ps_matched, pathspec, prefix))\n+\tif (report_path_error(ps_matched, opts->pathspec, opts->prefix))\n \t\treturn 1;\n \n \t/* \"checkout -m path\" to recreate conflicted state */\n \tif (opts->merge)\n-\t\tunmerge_cache(pathspec);\n+\t\tunmerge_cache(opts->pathspec);\n \n \t/* Any unmerged paths? */\n \tfor (pos = 0; pos < active_nr; pos++) {\n \t\tstruct cache_entry *ce = active_cache[pos];\n-\t\tif (match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, NULL)) {\n+\t\tif (match_pathspec(opts->pathspec, ce->name, ce_namelen(ce), 0, NULL)) {\n \t\t\tif (!ce_stage(ce))\n \t\t\t\tcontinue;\n \t\t\tif (opts->force) {\n@@ -282,9 +284,9 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,\n \tstate.refresh_cache = 1;\n \tfor (pos = 0; pos < active_nr; pos++) {\n \t\tstruct cache_entry *ce = active_cache[pos];\n-\t\tif (source_tree && !(ce->ce_flags & CE_UPDATE))\n+\t\tif (opts->source_tree && !(ce->ce_flags & CE_UPDATE))\n \t\t\tcontinue;\n-\t\tif (match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, NULL)) {\n+\t\tif (match_pathspec(opts->pathspec, ce->name, ce_namelen(ce), 0, NULL)) {\n \t\t\tif (!ce_stage(ce)) {\n \t\t\t\terrs |= checkout_entry(ce, &state, NULL);\n \t\t\t\tcontinue;\n@@ -706,7 +708,8 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new)\n \tfree(refs.objects);\n }\n \n-static int switch_branches(const struct checkout_opts *opts, struct branch_info *new)\n+static int switch_branches(const struct checkout_opts *opts,\n+\t\t\t   struct branch_info *new)\n {\n \tint ret = 0;\n \tstruct branch_info old;\n@@ -760,8 +763,7 @@ static int git_checkout_config(const char *var, const char *value, void *cb)\n \treturn git_xmerge_config(var, value, NULL);\n }\n \n-static int interactive_checkout(const char *revision, const char **pathspec,\n-\t\t\t\tstruct checkout_opts *opts)\n+static int interactive_checkout(const char *revision, const char **pathspec)\n {\n \treturn run_add_interactive(revision, \"--patch=checkout\", pathspec);\n }\n@@ -931,11 +933,8 @@ static int switch_unborn_to_new_branch(const struct checkout_opts *opts)\n int cmd_checkout(int argc, const char **argv, const char *prefix)\n {\n \tstruct checkout_opts opts;\n-\tunsigned char rev[20];\n \tstruct branch_info new;\n-\tstruct tree *source_tree = NULL;\n \tchar *conflict_style = NULL;\n-\tint patch_mode = 0;\n \tint dwim_new_local_branch = 1;\n \tstruct option options[] = {\n \t\tOPT__QUIET(&opts.quiet, \"suppress progress reporting\"),\n@@ -957,7 +956,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOLEAN(0, \"overwrite-ignore\", &opts.overwrite_ignore, \"update ignored files (default)\"),\n \t\tOPT_STRING(0, \"conflict\", &conflict_style, \"style\",\n \t\t\t   \"conflict style (merge or diff3)\"),\n-\t\tOPT_BOOLEAN('p', \"patch\", &patch_mode, \"select hunks interactively\"),\n+\t\tOPT_BOOLEAN('p', \"patch\", &opts.patch_mode, \"select hunks interactively\"),\n \t\t{ OPTION_BOOLEAN, 0, \"guess\", &dwim_new_local_branch, NULL,\n \t\t  \"second guess 'git checkout no-such-branch'\",\n \t\t  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },\n@@ -967,6 +966,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \tmemset(&opts, 0, sizeof(opts));\n \tmemset(&new, 0, sizeof(new));\n \topts.overwrite_ignore = 1;\n+\topts.prefix = prefix;\n \n \tgitmodules_config();\n \tgit_config(git_checkout_config, &opts);\n@@ -984,7 +984,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \tif (opts.new_branch_force)\n \t\topts.new_branch = opts.new_branch_force;\n \n-\tif (patch_mode && (opts.track > 0 || opts.new_branch\n+\tif (opts.patch_mode && (opts.track > 0 || opts.new_branch\n \t\t\t   || opts.new_branch_log || opts.merge || opts.force\n \t\t\t   || opts.force_detach))\n \t\tdie (_(\"--patch is incompatible with all other options\"));\n@@ -1039,13 +1039,15 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t * remote branches, erroring out for invalid or ambiguous cases.\n \t */\n \tif (argc) {\n+\t\tunsigned char rev[20];\n \t\tint dwim_ok =\n-\t\t\t!patch_mode &&\n+\t\t\t!opts.patch_mode &&\n \t\t\tdwim_new_local_branch &&\n \t\t\topts.track == BRANCH_TRACK_UNSPECIFIED &&\n \t\t\t!opts.new_branch;\n \t\tint n = parse_branchname_arg(argc, argv, dwim_ok,\n-\t\t\t\t&new, &source_tree, rev, &opts.new_branch);\n+\t\t\t\t\t     &new, &opts.source_tree,\n+\t\t\t\t\t     rev, &opts.new_branch);\n \t\targv += n;\n \t\targc -= n;\n \t}\n@@ -1054,13 +1056,13 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\topts.track = git_branch_track;\n \n \tif (argc) {\n-\t\tconst char **pathspec = get_pathspec(prefix, argv);\n+\t\topts.pathspec = get_pathspec(prefix, argv);\n \n-\t\tif (!pathspec)\n+\t\tif (!opts.pathspec)\n \t\t\tdie(_(\"invalid path specification\"));\n \n-\t\tif (patch_mode)\n-\t\t\treturn interactive_checkout(new.name, pathspec, &opts);\n+\t\tif (opts.patch_mode)\n+\t\t\treturn interactive_checkout(new.name, opts.pathspec);\n \n \t\t/* Checkout paths */\n \t\tif (opts.new_branch) {\n@@ -1077,11 +1079,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\tif (1 < !!opts.writeout_stage + !!opts.force + !!opts.merge)\n \t\t\tdie(_(\"git checkout: --ours/--theirs, --force and --merge are incompatible when\\nchecking out of the index.\"));\n \n-\t\treturn checkout_paths(source_tree, pathspec, prefix, &opts);\n+\t\treturn checkout_paths(&opts);\n \t}\n \n-\tif (patch_mode)\n-\t\treturn interactive_checkout(new.name, NULL, &opts);\n+\tif (opts.patch_mode)\n+\t\treturn interactive_checkout(new.name, NULL);\n \n \tif (opts.new_branch) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n-- \n1.7.12.rc2.18.g61b472e\n"},{"id":"198042","messageId":"1346248524-11616-3-git-send-email-pclouds@gmail.com","threadId":"31369","inReplyTo":"1346248524-11616-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 3/3] checkout: reorder option handling","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-08-29T13:55:24Z","receivedAt":"2012-08-29T13:55:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"checkout operates in three different modes. On top of that it tries to\nbe smart by guessing the branch name for switching. This results in\nmessy option handling code. This patch reorders it so that\n\n - cmd_checkout() is responsible for parsing, preparing input and\n   determinining mode\n\n - Code of each mode is in cmd_checkout_entry() and cmd_switch(),\n   where sanity checks are performed per mode\n\nAnother slight improvement is always print branch name (or commit\nname) when printing errors related ot them. This helps catch the case\nwhere an option is mistaken as branch/commit.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n I'm not entirely sure about this chunk\n\n +\tif (opts->track != BRANCH_TRACK_UNSPECIFIED) {\n +\t\tif (opts->new_orphan_branch)\n +\t\t\tdie(_(\"%s cannot be used with %s\"), \"--orphan\", \"-t\");\n +\t\tif (opts->force_detach)\n +\t\t\tdie(_(\"%s cannot be used with %s\"), \"--detach\", \"-t\");\n +\t} else\n +\t\topts->track = git_branch_track;\n\n If we don't want -t and --orphan/--detach together, then we probably should ignore\n branch.autosetupmerge when --orphan/--detach is specified.\n\n I did not unify new_branch, new_branch_force and new_orphan_branch.\n They touch other parts of the code and should probably be done\n separately.\n\n builtin/checkout.c | 169 ++++++++++++++++++++++++++++++++---------------------\n 1 file changed, 101 insertions(+), 68 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 78abaeb..8289bcd 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -930,6 +930,81 @@ static int switch_unborn_to_new_branch(const struct checkout_opts *opts)\n \treturn status;\n }\n \n+static int cmd_checkout_entry(const struct checkout_opts *opts,\n+\t\t\t      const struct branch_info *new)\n+{\n+\tif (opts->track != BRANCH_TRACK_UNSPECIFIED)\n+\t\tdie(_(\"%s cannot be used with updating paths\"), \"--track\");\n+\n+\tif (opts->new_branch_log)\n+\t\tdie(_(\"%s cannot be used with updating paths\"), \"-l\");\n+\n+\tif (opts->force && opts->patch_mode)\n+\t\tdie(_(\"%s cannot be used with updating paths\"), \"-f\");\n+\n+\tif (opts->force_detach)\n+\t\tdie(_(\"%s cannot be used with updating paths\"), \"--detach\");\n+\n+\tif (opts->merge && opts->patch_mode)\n+\t\tdie(_(\"%s cannot be used with %s\"), \"--merge\", \"--patch\");\n+\n+\tif (opts->force && opts->merge)\n+\t\tdie(_(\"%s cannot be used with %s\"), \"-f\", \"-m\");\n+\n+\tif (opts->new_branch)\n+\t\tdie(_(\"Cannot update paths and switch to branch '%s' at the same time.\"),\n+\t\t    opts->new_branch);\n+\n+\tif (opts->patch_mode)\n+\t\treturn interactive_checkout(new->name, opts->pathspec);\n+\telse\n+\t\treturn checkout_paths(opts);\n+}\n+\n+static int cmd_switch(struct checkout_opts *opts,\n+\t\t      struct branch_info *new)\n+{\n+\tif (opts->pathspec)\n+\t\tdie(_(\"paths cannot be used with switching branches\"));\n+\n+\tif (opts->patch_mode)\n+\t\tdie(_(\"%s cannot be used with switching branches\"),\n+\t\t    \"--patch\");\n+\n+\tif (opts->writeout_stage)\n+\t\tdie(_(\"%s cannot be used with switching branches\"),\n+\t\t    \"--ours/--theirs\");\n+\n+\tif (opts->force && opts->merge)\n+\t\tdie(_(\"%s cannot be used with %s\"), \"-f\", \"-m\");\n+\n+\tif (opts->force_detach && opts->new_branch)\n+\t\tdie(_(\"%s cannot be used with %s\"),\n+\t\t    \"--detach\", \"-b/-B/--orphan\");\n+\n+\tif (opts->track != BRANCH_TRACK_UNSPECIFIED) {\n+\t\tif (opts->new_orphan_branch)\n+\t\t\tdie(_(\"%s cannot be used with %s\"), \"--orphan\", \"-t\");\n+\t\tif (opts->force_detach)\n+\t\t\tdie(_(\"%s cannot be used with %s\"), \"--detach\", \"-t\");\n+\t} else\n+\t\topts->track = git_branch_track;\n+\n+\tif (new->name && !new->commit)\n+\t\tdie(_(\"Cannot switch branch to a non-commit '%s'.\"),\n+\t\t    new->name);\n+\n+\tif (!new->commit && opts->new_branch) {\n+\t\tunsigned char rev[20];\n+\t\tint flag;\n+\n+\t\tif (!read_ref_full(\"HEAD\", rev, 0, &flag) &&\n+\t\t    (flag & REF_ISSYMREF) && is_null_sha1(rev))\n+\t\t\treturn switch_unborn_to_new_branch(opts);\n+\t}\n+\treturn switch_branches(opts, new);\n+}\n+\n int cmd_checkout(int argc, const char **argv, const char *prefix)\n {\n \tstruct checkout_opts opts;\n@@ -976,26 +1051,22 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, checkout_usage,\n \t\t\t     PARSE_OPT_KEEP_DASHDASH);\n \n-\t/* we can assume from now on new_branch = !new_branch_force */\n-\tif (opts.new_branch && opts.new_branch_force)\n-\t\tdie(_(\"-B cannot be used with -b\"));\n+\tif (conflict_style) {\n+\t\topts.merge = 1; /* implied */\n+\t\tgit_xmerge_config(\"merge.conflictstyle\", conflict_style, NULL);\n+\t}\n+\n+\tif ((!!opts.new_branch + !!opts.new_branch_force + !!opts.new_orphan_branch) > 1)\n+\t\tdie(_(\"-b, -B and --orphan are mutually exclusive\"));\n \n-\t/* copy -B over to -b, so that we can just check the latter */\n \tif (opts.new_branch_force)\n \t\topts.new_branch = opts.new_branch_force;\n \n-\tif (opts.patch_mode && (opts.track > 0 || opts.new_branch\n-\t\t\t   || opts.new_branch_log || opts.merge || opts.force\n-\t\t\t   || opts.force_detach))\n-\t\tdie (_(\"--patch is incompatible with all other options\"));\n-\n-\tif (opts.force_detach && (opts.new_branch || opts.new_orphan_branch))\n-\t\tdie(_(\"--detach cannot be used with -b/-B/--orphan\"));\n-\tif (opts.force_detach && 0 < opts.track)\n-\t\tdie(_(\"--detach cannot be used with -t\"));\n+\tif (opts.new_orphan_branch)\n+\t\topts.new_branch = opts.new_orphan_branch;\n \n \t/* --track without -b should DWIM */\n-\tif (0 < opts.track && !opts.new_branch) {\n+\tif (opts.track != BRANCH_TRACK_UNSPECIFIED && !opts.new_branch) {\n \t\tconst char *argv0 = argv[0];\n \t\tif (!argc || !strcmp(argv0, \"--\"))\n \t\t\tdie (_(\"--track needs a branch name\"));\n@@ -1009,22 +1080,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\topts.new_branch = argv0 + 1;\n \t}\n \n-\tif (opts.new_orphan_branch) {\n-\t\tif (opts.new_branch)\n-\t\t\tdie(_(\"--orphan and -b|-B are mutually exclusive\"));\n-\t\tif (opts.track > 0)\n-\t\t\tdie(_(\"--orphan cannot be used with -t\"));\n-\t\topts.new_branch = opts.new_orphan_branch;\n-\t}\n-\n-\tif (conflict_style) {\n-\t\topts.merge = 1; /* implied */\n-\t\tgit_xmerge_config(\"merge.conflictstyle\", conflict_style, NULL);\n-\t}\n-\n-\tif (opts.force && opts.merge)\n-\t\tdie(_(\"git checkout: -f and -m are incompatible\"));\n-\n \t/*\n \t * Extract branch name from command line arguments, so\n \t * all that is left is pathspecs.\n@@ -1052,62 +1107,40 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\targc -= n;\n \t}\n \n-\tif (opts.track == BRANCH_TRACK_UNSPECIFIED)\n-\t\topts.track = git_branch_track;\n-\n \tif (argc) {\n \t\topts.pathspec = get_pathspec(prefix, argv);\n \n \t\tif (!opts.pathspec)\n \t\t\tdie(_(\"invalid path specification\"));\n \n-\t\tif (opts.patch_mode)\n-\t\t\treturn interactive_checkout(new.name, opts.pathspec);\n-\n-\t\t/* Checkout paths */\n-\t\tif (opts.new_branch) {\n-\t\t\tif (argc == 1) {\n-\t\t\t\tdie(_(\"git checkout: updating paths is incompatible with switching branches.\\nDid you intend to checkout '%s' which can not be resolved as commit?\"), argv[0]);\n-\t\t\t} else {\n-\t\t\t\tdie(_(\"git checkout: updating paths is incompatible with switching branches.\"));\n-\t\t\t}\n-\t\t}\n+\t\t/* Try to give more helpful suggestion, new_branch &&\n+\t\t   argc > 1 will be caught later */\n+\t\tif (opts.new_branch && argc == 1)\n+\t\t\tdie(_(\"Cannot update paths and switch to branch '%s' at the same time.\\n\"\n+\t\t\t      \"Did you intend to checkout '%s' which can not be resolved as commit?\"),\n+\t\t\t    opts.new_branch, argv[0]);\n \n \t\tif (opts.force_detach)\n \t\t\tdie(_(\"git checkout: --detach does not take a path argument\"));\n \n \t\tif (1 < !!opts.writeout_stage + !!opts.force + !!opts.merge)\n-\t\t\tdie(_(\"git checkout: --ours/--theirs, --force and --merge are incompatible when\\nchecking out of the index.\"));\n-\n-\t\treturn checkout_paths(&opts);\n+\t\t\tdie(_(\"git checkout: --ours/--theirs, --force and --merge are incompatible when\\n\"\n+\t\t\t      \"checking out of the index.\"));\n \t}\n \n-\tif (opts.patch_mode)\n-\t\treturn interactive_checkout(new.name, NULL);\n-\n \tif (opts.new_branch) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\topts.branch_exists = validate_new_branchname(opts.new_branch, &buf,\n-\t\t\t\t\t\t\t     !!opts.new_branch_force,\n-\t\t\t\t\t\t\t     !!opts.new_branch_force);\n+\t\topts.branch_exists =\n+\t\t\tvalidate_new_branchname(opts.new_branch, &buf,\n+\t\t\t\t\t\t!!opts.new_branch_force,\n+\t\t\t\t\t\t!!opts.new_branch_force);\n \n \t\tstrbuf_release(&buf);\n \t}\n \n-\tif (new.name && !new.commit) {\n-\t\tdie(_(\"Cannot switch branch to a non-commit.\"));\n-\t}\n-\tif (opts.writeout_stage)\n-\t\tdie(_(\"--ours/--theirs is incompatible with switching branches.\"));\n-\n-\tif (!new.commit && opts.new_branch) {\n-\t\tunsigned char rev[20];\n-\t\tint flag;\n-\n-\t\tif (!read_ref_full(\"HEAD\", rev, 0, &flag) &&\n-\t\t    (flag & REF_ISSYMREF) && is_null_sha1(rev))\n-\t\t\treturn switch_unborn_to_new_branch(&opts);\n-\t}\n-\treturn switch_branches(&opts, &new);\n+\tif (opts.patch_mode || opts.pathspec)\n+\t\treturn cmd_checkout_entry(&opts, &new);\n+\telse\n+\t\treturn cmd_switch(&opts, &new);\n }\n-- \n1.7.12.rc2.18.g61b472e\n"},{"id":"198054","messageId":"7v7gshwnmm.fsf@alter.siamese.dyndns.org","threadId":"31369","inReplyTo":"1346248524-11616-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 3/3] checkout: reorder option handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-29T18:37:21Z","receivedAt":"2012-08-29T18:37:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n>  I'm not entirely sure about this chunk\n>\n>  +\tif (opts->track != BRANCH_TRACK_UNSPECIFIED) {\n>  +\t\tif (opts->new_orphan_branch)\n>  +\t\t\tdie(_(\"%s cannot be used with %s\"), \"--orphan\", \"-t\");\n>  +\t\tif (opts->force_detach)\n>  +\t\t\tdie(_(\"%s cannot be used with %s\"), \"--detach\", \"-t\");\n>  +\t} else\n>  +\t\topts->track = git_branch_track;\n>\n>  If we don't want -t and --orphan/--detach together, then we probably should ignore\n>  branch.autosetupmerge when --orphan/--detach is specified.\n\nYeah, I agree.\n\n>  I did not unify new_branch, new_branch_force and new_orphan_branch.\n>  They touch other parts of the code and should probably be done\n>  separately.\n\nThat sounds like a very sensible decision.\n"},{"id":"198055","messageId":"7v3935wms5.fsf@alter.siamese.dyndns.org","threadId":"31369","inReplyTo":"1346248524-11616-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 3/3] checkout: reorder option handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-29T18:55:38Z","receivedAt":"2012-08-29T18:55:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> +\tif (opts.patch_mode || opts.pathspec)\n> +\t\treturn cmd_checkout_entry(&opts, &new);\n> +\telse\n> +\t\treturn cmd_switch(&opts, &new);\n>  }\n\nYeah, patch_mode is really part of checking out paths, so separating\nthe actions into two makes sense.\n\nEven though I am not sure about \"cmd_\" prefix (as these are not\ncommands), I'll rename the former \"cmd_checkout_paths\" to be\nconsistent with the underlying checkout_paths() function, and the.\nlatter \"cmd_checkout_branch\" for consistency with the former (you\neither check out one-or-more paths, or check out a branch).\n"},{"id":"198093","messageId":"1346330750-17937-1-git-send-email-pclouds@gmail.com","threadId":"31369","inReplyTo":"1346248524-11616-3-git-send-email-pclouds@gmail.com","subject":"[PATCH v3 3/3] checkout: reorder option handling","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-08-30T12:45:50Z","receivedAt":"2012-08-30T12:45:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"checkout operates in three different modes. On top of that it tries to\nbe smart by guessing the branch name for switching. This results in\nmessy option handling code. This patch reorders it so that\n\n - cmd_checkout() is responsible for parsing, preparing input and\n   determining mode\n\n - Code of each mode is in checkout_paths() and checkout_branch(),\n   where sanity checks are performed\n\nAnother slight improvement is always print branch name (or commit\nname) when printing errors related ot them. This helps catch the case\nwhere an option is mistaken as branch/commit.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Changes since v2 (the first two patches are not resent):\n\n  - leave track mode unspecified if --detach/--orphan is specified\n  - merge cmd_checkout_entry() into checkout_paths()\n  - rename cmd_switch() to checkout_branch()\n\n builtin/checkout.c | 177 ++++++++++++++++++++++++++++++-----------------------\n 1 tập tin đã bị thay đổi, 102 được thêm vào(+), 75 bị xóa(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 78abaeb..b0c5133 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -217,7 +217,8 @@ static int checkout_merged(int pos, struct checkout *state)\n \treturn status;\n }\n \n-static int checkout_paths(const struct checkout_opts *opts)\n+static int checkout_paths(const struct checkout_opts *opts,\n+\t\t\t  const char *revision)\n {\n \tint pos;\n \tstruct checkout state;\n@@ -229,7 +230,35 @@ static int checkout_paths(const struct checkout_opts *opts)\n \tint stage = opts->writeout_stage;\n \tint merge = opts->merge;\n \tint newfd;\n-\tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n+\tstruct lock_file *lock_file;\n+\n+\tif (opts->track != BRANCH_TRACK_UNSPECIFIED)\n+\t\tdie(_(\"%s cannot be used with updating paths\"), \"--track\");\n+\n+\tif (opts->new_branch_log)\n+\t\tdie(_(\"%s cannot be used with updating paths\"), \"-l\");\n+\n+\tif (opts->force && opts->patch_mode)\n+\t\tdie(_(\"%s cannot be used with updating paths\"), \"-f\");\n+\n+\tif (opts->force_detach)\n+\t\tdie(_(\"%s cannot be used with updating paths\"), \"--detach\");\n+\n+\tif (opts->merge && opts->patch_mode)\n+\t\tdie(_(\"%s cannot be used with %s\"), \"--merge\", \"--patch\");\n+\n+\tif (opts->force && opts->merge)\n+\t\tdie(_(\"%s cannot be used with %s\"), \"-f\", \"-m\");\n+\n+\tif (opts->new_branch)\n+\t\tdie(_(\"Cannot update paths and switch to branch '%s' at the same time.\"),\n+\t\t    opts->new_branch);\n+\n+\tif (opts->patch_mode)\n+\t\treturn run_add_interactive(revision, \"--patch=checkout\",\n+\t\t\t\t\t   opts->pathspec);\n+\n+\tlock_file = xcalloc(1, sizeof(struct lock_file));\n \n \tnewfd = hold_locked_index(lock_file, 1);\n \tif (read_cache_preload(opts->pathspec) < 0)\n@@ -763,11 +792,6 @@ static int git_checkout_config(const char *var, const char *value, void *cb)\n \treturn git_xmerge_config(var, value, NULL);\n }\n \n-static int interactive_checkout(const char *revision, const char **pathspec)\n-{\n-\treturn run_add_interactive(revision, \"--patch=checkout\", pathspec);\n-}\n-\n struct tracking_name_data {\n \tconst char *name;\n \tchar *remote;\n@@ -930,6 +954,51 @@ static int switch_unborn_to_new_branch(const struct checkout_opts *opts)\n \treturn status;\n }\n \n+static int checkout_branch(struct checkout_opts *opts,\n+\t\t\t   struct branch_info *new)\n+{\n+\tif (opts->pathspec)\n+\t\tdie(_(\"paths cannot be used with switching branches\"));\n+\n+\tif (opts->patch_mode)\n+\t\tdie(_(\"%s cannot be used with switching branches\"),\n+\t\t    \"--patch\");\n+\n+\tif (opts->writeout_stage)\n+\t\tdie(_(\"%s cannot be used with switching branches\"),\n+\t\t    \"--ours/--theirs\");\n+\n+\tif (opts->force && opts->merge)\n+\t\tdie(_(\"%s cannot be used with %s\"), \"-f\", \"-m\");\n+\n+\tif (opts->force_detach && opts->new_branch)\n+\t\tdie(_(\"%s cannot be used with %s\"),\n+\t\t    \"--detach\", \"-b/-B/--orphan\");\n+\n+\tif (opts->new_orphan_branch) {\n+\t\tif (opts->track != BRANCH_TRACK_UNSPECIFIED)\n+\t\t\tdie(_(\"%s cannot be used with %s\"), \"--orphan\", \"-t\");\n+\t} else if (opts->force_detach) {\n+\t\tif (opts->track != BRANCH_TRACK_UNSPECIFIED)\n+\t\t\tdie(_(\"%s cannot be used with %s\"), \"--detach\", \"-t\");\n+\t} else if (opts->track == BRANCH_TRACK_UNSPECIFIED)\n+\t\topts->track = git_branch_track;\n+\n+\tif (new->name && !new->commit)\n+\t\tdie(_(\"Cannot switch branch to a non-commit '%s'.\"),\n+\t\t    new->name);\n+\n+\tif (!new->commit && opts->new_branch) {\n+\t\tunsigned char rev[20];\n+\t\tint flag;\n+\n+\t\tif (!read_ref_full(\"HEAD\", rev, 0, &flag) &&\n+\t\t    (flag & REF_ISSYMREF) && is_null_sha1(rev))\n+\t\t\treturn switch_unborn_to_new_branch(opts);\n+\t}\n+\treturn switch_branches(opts, new);\n+}\n+\n int cmd_checkout(int argc, const char **argv, const char *prefix)\n {\n \tstruct checkout_opts opts;\n@@ -976,26 +1045,22 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, checkout_usage,\n \t\t\t     PARSE_OPT_KEEP_DASHDASH);\n \n-\t/* we can assume from now on new_branch = !new_branch_force */\n-\tif (opts.new_branch && opts.new_branch_force)\n-\t\tdie(_(\"-B cannot be used with -b\"));\n+\tif (conflict_style) {\n+\t\topts.merge = 1; /* implied */\n+\t\tgit_xmerge_config(\"merge.conflictstyle\", conflict_style, NULL);\n+\t}\n+\n+\tif ((!!opts.new_branch + !!opts.new_branch_force + !!opts.new_orphan_branch) > 1)\n+\t\tdie(_(\"-b, -B and --orphan are mutually exclusive\"));\n \n-\t/* copy -B over to -b, so that we can just check the latter */\n \tif (opts.new_branch_force)\n \t\topts.new_branch = opts.new_branch_force;\n \n-\tif (opts.patch_mode && (opts.track > 0 || opts.new_branch\n-\t\t\t   || opts.new_branch_log || opts.merge || opts.force\n-\t\t\t   || opts.force_detach))\n-\t\tdie (_(\"--patch is incompatible with all other options\"));\n-\n-\tif (opts.force_detach && (opts.new_branch || opts.new_orphan_branch))\n-\t\tdie(_(\"--detach cannot be used with -b/-B/--orphan\"));\n-\tif (opts.force_detach && 0 < opts.track)\n-\t\tdie(_(\"--detach cannot be used with -t\"));\n+\tif (opts.new_orphan_branch)\n+\t\topts.new_branch = opts.new_orphan_branch;\n \n \t/* --track without -b should DWIM */\n-\tif (0 < opts.track && !opts.new_branch) {\n+\tif (opts.track != BRANCH_TRACK_UNSPECIFIED && !opts.new_branch) {\n \t\tconst char *argv0 = argv[0];\n \t\tif (!argc || !strcmp(argv0, \"--\"))\n \t\t\tdie (_(\"--track needs a branch name\"));\n@@ -1009,22 +1074,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\topts.new_branch = argv0 + 1;\n \t}\n \n-\tif (opts.new_orphan_branch) {\n-\t\tif (opts.new_branch)\n-\t\t\tdie(_(\"--orphan and -b|-B are mutually exclusive\"));\n-\t\tif (opts.track > 0)\n-\t\t\tdie(_(\"--orphan cannot be used with -t\"));\n-\t\topts.new_branch = opts.new_orphan_branch;\n-\t}\n-\n-\tif (conflict_style) {\n-\t\topts.merge = 1; /* implied */\n-\t\tgit_xmerge_config(\"merge.conflictstyle\", conflict_style, NULL);\n-\t}\n-\n-\tif (opts.force && opts.merge)\n-\t\tdie(_(\"git checkout: -f and -m are incompatible\"));\n-\n \t/*\n \t * Extract branch name from command line arguments, so\n \t * all that is left is pathspecs.\n@@ -1052,62 +1101,40 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\targc -= n;\n \t}\n \n-\tif (opts.track == BRANCH_TRACK_UNSPECIFIED)\n-\t\topts.track = git_branch_track;\n-\n \tif (argc) {\n \t\topts.pathspec = get_pathspec(prefix, argv);\n \n \t\tif (!opts.pathspec)\n \t\t\tdie(_(\"invalid path specification\"));\n \n-\t\tif (opts.patch_mode)\n-\t\t\treturn interactive_checkout(new.name, opts.pathspec);\n-\n-\t\t/* Checkout paths */\n-\t\tif (opts.new_branch) {\n-\t\t\tif (argc == 1) {\n-\t\t\t\tdie(_(\"git checkout: updating paths is incompatible with switching branches.\\nDid you intend to checkout '%s' which can not be resolved as commit?\"), argv[0]);\n-\t\t\t} else {\n-\t\t\t\tdie(_(\"git checkout: updating paths is incompatible with switching branches.\"));\n-\t\t\t}\n-\t\t}\n+\t\t/* Try to give more helpful suggestion, new_branch &&\n+\t\t   argc > 1 will be caught later */\n+\t\tif (opts.new_branch && argc == 1)\n+\t\t\tdie(_(\"Cannot update paths and switch to branch '%s' at the same time.\\n\"\n+\t\t\t      \"Did you intend to checkout '%s' which can not be resolved as commit?\"),\n+\t\t\t    opts.new_branch, argv[0]);\n \n \t\tif (opts.force_detach)\n \t\t\tdie(_(\"git checkout: --detach does not take a path argument\"));\n \n \t\tif (1 < !!opts.writeout_stage + !!opts.force + !!opts.merge)\n-\t\t\tdie(_(\"git checkout: --ours/--theirs, --force and --merge are incompatible when\\nchecking out of the index.\"));\n-\n-\t\treturn checkout_paths(&opts);\n+\t\t\tdie(_(\"git checkout: --ours/--theirs, --force and --merge are incompatible when\\n\"\n+\t\t\t      \"checking out of the index.\"));\n \t}\n \n-\tif (opts.patch_mode)\n-\t\treturn interactive_checkout(new.name, NULL);\n-\n \tif (opts.new_branch) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\topts.branch_exists = validate_new_branchname(opts.new_branch, &buf,\n-\t\t\t\t\t\t\t     !!opts.new_branch_force,\n-\t\t\t\t\t\t\t     !!opts.new_branch_force);\n+\t\topts.branch_exists =\n+\t\t\tvalidate_new_branchname(opts.new_branch, &buf,\n+\t\t\t\t\t\t!!opts.new_branch_force,\n+\t\t\t\t\t\t!!opts.new_branch_force);\n \n \t\tstrbuf_release(&buf);\n \t}\n \n-\tif (new.name && !new.commit) {\n-\t\tdie(_(\"Cannot switch branch to a non-commit.\"));\n-\t}\n-\tif (opts.writeout_stage)\n-\t\tdie(_(\"--ours/--theirs is incompatible with switching branches.\"));\n-\n-\tif (!new.commit && opts.new_branch) {\n-\t\tunsigned char rev[20];\n-\t\tint flag;\n-\n-\t\tif (!read_ref_full(\"HEAD\", rev, 0, &flag) &&\n-\t\t    (flag & REF_ISSYMREF) && is_null_sha1(rev))\n-\t\t\treturn switch_unborn_to_new_branch(&opts);\n-\t}\n-\treturn switch_branches(&opts, &new);\n+\tif (opts.patch_mode || opts.pathspec)\n+\t\treturn checkout_paths(&opts, new.name);\n+\telse\n+\t\treturn checkout_branch(&opts, &new);\n }\n-- \n1.7.12.rc2.18.g61b472e\n"},{"id":"198105","messageId":"7v7gsgtl6u.fsf@alter.siamese.dyndns.org","threadId":"31369","inReplyTo":"1346330750-17937-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3 3/3] checkout: reorder option handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-30T16:10:33Z","receivedAt":"2012-08-30T16:10:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n\n>  Changes since v2 (the first two patches are not resent):\n> ...\n>   - merge cmd_checkout_entry() into checkout_paths()\n\nI didn't think of this when I reviewed the previous one; I think it\nmakes sense.  Thanks.\n"},{"id":"198551","messageId":"7vpq5xwppc.fsf@alter.siamese.dyndns.org","threadId":"31369","inReplyTo":"1346330750-17937-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3 3/3] checkout: reorder option handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-07T20:19:43Z","receivedAt":"2012-09-07T20:19:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> +\tif (opts->track != BRANCH_TRACK_UNSPECIFIED)\n> +\t\tdie(_(\"%s cannot be used with updating paths\"), \"--track\");\n\nI think most of the places we try to enclose these literals inside\nquotes, so I'd squash in a patch to make this:\n\n\t\tdie(_(\"'%s' cannot be used with updating paths\"), \"--track\");\n\nand update others to match.\n\n> +\tif (new->name && !new->commit)\n> +\t\tdie(_(\"Cannot switch branch to a non-commit '%s'.\"),\n> +\t\t    new->name);\n\nThis is one of the only few places that end a sentence with a full-stop.\n\n> +\t\t/* Try to give more helpful suggestion, new_branch &&\n> +\t\t   argc > 1 will be caught later */\n\nStyle;\n\n> +\t\tif (opts.new_branch && argc == 1)\n> +\t\t\tdie(_(\"Cannot update paths and switch to branch '%s' at the same time.\\n\"\n> +\t\t\t      \"Did you intend to checkout '%s' which can not be resolved as commit?\"),\n> +\t\t\t    opts.new_branch, argv[0]);\n>  \n>  \t\tif (opts.force_detach)\n>  \t\t\tdie(_(\"git checkout: --detach does not take a path argument\"));\n\nI tried this codepath and the message left me feeling uneasy.  I'd do:\n\n\t\tdie(_(\"git checkout: --detach does not take a path argument '%s'\"),\n\t\t\targv[0]);\n\nOther than these, I didn't find anything obviously wrong with my\nfinal review before merging to 'next', so I'll merge it soonish.\n\nThanks.\n"}]}