[PATCH v3 12/14] restore-files: take tree-ish from --from option instead

From: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
To: pclouds@gmail.com
Cc: avarab@gmail.com, git@vger.kernel.org, gitster@pobox.com,
	sbeller@google.com, t.gummerer@gmail.com, sxenos@google.com
Subject: [PATCH v3 12/14] restore-files: take tree-ish from --from option instead
Date: Thu, 29 Nov 2018 22:58:47 +0100	[thread overview]
Message-ID: <20181129215850.7278-13-pclouds@gmail.com> (raw)
In-Reply-To: <20181129215850.7278-1-pclouds@gmail.com>

This is another departure from 'git checkout' syntax, which uses -- to
separate ref and pathspec. The observation is restore-files (or "git
checkout ,, <pathspec>") is most often used to restore some files from
the index. If this is correct, we can simplify it by taking a way the
ref, so that we can write

    git restore-files some-file

without worrying about some-file being a ref and whether we need to do

    git restore-files -- some-file

for safety. If the source of the restore comes from a tree, it will be
in the form of an option with value, e.g.

    git restore-files --from=this-tree some-file

This is of course longer to type than using "--". But hopefully it
will not be used as often, and it is clearly easier to understand.

dwim_new_local_branch is no longer set (or unset) in cmd_restore_files()
because it's irrelevant because we don't really care about dwim-ing.
With accept_ref being unset, dwim can't happen.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout.c | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index fbfebba2d9..7ff9951818 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -39,7 +39,7 @@ static const char * const switch_branch_usage[] = {
 };
 
 static const char * const restore_files_usage[] = {
-	N_("git restore-files [<options>] [<branch>] -- <file>..."),
+	N_("git restore-files [<options>] [--from=<branch>] <file>..."),
 	NULL,
 };
 
@@ -56,6 +56,7 @@ struct checkout_opts {
 	int ignore_other_worktrees;
 	int show_progress;
 	int dwim_new_local_branch;
+	int accept_ref;
 	int accept_pathspec;
 	int switch_branch_doing_nothing_not_ok;
 
@@ -75,6 +76,7 @@ struct checkout_opts {
 	int branch_exists;
 	const char *prefix;
 	struct pathspec pathspec;
+	const char *from_treeish;
 	struct tree *source_tree;
 };
 
@@ -1337,6 +1339,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 {
 	struct branch_info new_branch_info;
 	int dwim_remotes_matched = 0;
+	int parseopt_flags = 0;
 
 	memset(&new_branch_info, 0, sizeof(new_branch_info));
 	opts->overwrite_ignore = 1;
@@ -1347,8 +1350,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 
 	opts->track = BRANCH_TRACK_UNSPECIFIED;
 
-	argc = parse_options(argc, argv, prefix, options, usagestr,
-			     PARSE_OPT_KEEP_DASHDASH);
+	if (!opts->accept_pathspec && !opts->accept_ref)
+		BUG("make up your mind, you need to take _something_");
+	if (opts->accept_pathspec && opts->accept_ref)
+		parseopt_flags = PARSE_OPT_KEEP_DASHDASH;
+
+	argc = parse_options(argc, argv, prefix, options,
+			     usagestr, parseopt_flags);
 
 	if (opts->show_progress < 0) {
 		if (opts->quiet)
@@ -1402,7 +1410,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 	 * including "last branch" syntax and DWIM-ery for names of
 	 * remote branches, erroring out for invalid or ambiguous cases.
 	 */
-	if (argc) {
+	if (argc && opts->accept_ref && opts->accept_pathspec) {
 		struct object_id rev;
 		int dwim_ok =
 			!opts->patch_mode &&
@@ -1414,6 +1422,18 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 					     &dwim_remotes_matched);
 		argv += n;
 		argc -= n;
+	} else if (!opts->accept_ref && opts->from_treeish) {
+		struct object_id rev;
+
+		if (get_oid_mb(opts->from_treeish, &rev))
+			die(_("could not resolve %s"), opts->from_treeish);
+
+		setup_new_branch_info_and_source_tree(&new_branch_info,
+						      opts, &rev,
+						      opts->from_treeish);
+
+		if (!opts->source_tree)
+			die(_("reference is not a tree: %s"), opts->from_treeish);
 	}
 
 	if (argc) {
@@ -1492,6 +1512,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	memset(&opts, 0, sizeof(opts));
 	opts.dwim_new_local_branch = 1;
 	opts.switch_branch_doing_nothing_not_ok = 0;
+	opts.accept_ref = 1;
 	opts.accept_pathspec = 1;
 	opts.implicit_detach = 1;
 
@@ -1520,6 +1541,7 @@ int cmd_switch_branch(int argc, const char **argv, const char *prefix)
 
 	memset(&opts, 0, sizeof(opts));
 	opts.dwim_new_local_branch = 1;
+	opts.accept_ref = 1;
 	opts.accept_pathspec = 0;
 	opts.switch_branch_doing_nothing_not_ok = 1;
 	opts.implicit_detach = 0;
@@ -1537,14 +1559,19 @@ int cmd_switch_branch(int argc, const char **argv, const char *prefix)
 int cmd_restore_files(int argc, const char **argv, const char *prefix)
 {
 	struct checkout_opts opts;
-	struct option *options = NULL;
+	struct option *options;
+	struct option restore_options[] = {
+		OPT_STRING(0, "from", &opts.from_treeish, "<tree-ish>",
+			   N_("where the checkout from")),
+		OPT_END()
+	};
 	int ret;
 
 	memset(&opts, 0, sizeof(opts));
-	opts.dwim_new_local_branch = 1;
+	opts.accept_ref = 0;
 	opts.accept_pathspec = 1;
 
-	options = parse_options_dup(options);
+	options = parse_options_dup(restore_options);
 	options = add_common_options(&opts, options);
 	options = add_checkout_path_options(&opts, options);
 
-- 
2.20.0.rc1.380.g3eb999425c.dirty

