diff mbox series

[v1,06/11] restore: add --worktree and --index

Message ID 20190308101655.9767-7-pclouds@gmail.com (mailing list archive)
State New, archived
Headers show
Series And new command "restore" | expand

Commit Message

Duy Nguyen March 8, 2019, 10:16 a.m. UTC
'git checkout <tree-ish> <pathspec>' updates both index and
worktree. But updating the index when you want to restore worktree
files is non-intuitive. The index contains the data ready for the next
commit, and there's no indication that the user will want to commit
the restored versions.

'git restore' therefore by default only touches worktree. The user has
the option to update either the index with

    git restore --source=<tree> --index <path>  (1)

or update both with

    git restore --source=<tree> --index --worktree <path> (2)

PS. Orignally I wanted to make worktree update default and form (1)
would add index update while also updating the worktree, and the user
would need to do "--index --no-worktree" to update index only. But it
looks really confusing that "--index" option alone updates both. So
now form (2) is used for both, which reads much more obvious.

PPS. Yes form (1) overlaps with "git reset <rev> <path>". I don't know
if we can ever turn "git reset" back to "_always_ reset HEAD and
optionally do something else".

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

Comments

Elijah Newren March 9, 2019, 6:52 p.m. UTC | #1
On Fri, Mar 8, 2019 at 2:17 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>
> 'git checkout <tree-ish> <pathspec>' updates both index and
> worktree. But updating the index when you want to restore worktree
> files is non-intuitive. The index contains the data ready for the next
> commit, and there's no indication that the user will want to commit
> the restored versions.
>
> 'git restore' therefore by default only touches worktree. The user has
> the option to update either the index with
>
>     git restore --source=<tree> --index <path>  (1)
>
> or update both with
>
>     git restore --source=<tree> --index --worktree <path> (2)
>
> PS. Orignally I wanted to make worktree update default and form (1)
> would add index update while also updating the worktree, and the user
> would need to do "--index --no-worktree" to update index only. But it
> looks really confusing that "--index" option alone updates both. So
> now form (2) is used for both, which reads much more obvious.
>
> PPS. Yes form (1) overlaps with "git reset <rev> <path>". I don't know
> if we can ever turn "git reset" back to "_always_ reset HEAD and
> optionally do something else".

I'm really happy with how this series is going generally.  :-)

> +       if (!opts->checkout_worktree && !opts->checkout_index)
> +               die(_("neither '%s' or '%s' is specified"),
> +                   "--index", "--worktree");

Is this die() or BUG()?  I thought --worktree was the default.

> +               else
> +                       die(_("'%s' with only '%s' is not currently supported"),
> +                           "--patch", "--worktree");

:-(

> +               /*
> +                * NEEDSWORK: if --worktree is not specified, we
> +                * should save stat info of checked out files in the
> +                * index to avoid the next (potentially costly)
> +                * refresh. But it's a bit tricker to do...
> +                */
> +               rollback_lock_file(&lock_file);

A total tangent: I see both FIXME and NEEDSWORK in the codebase.  Are
there other 'keywords' of this type that we use?  Is there a
preference for how they are used?
Eric Sunshine March 10, 2019, 8:03 p.m. UTC | #2
On Sat, Mar 9, 2019 at 1:52 PM Elijah Newren <newren@gmail.com> wrote:
> On Fri, Mar 8, 2019 at 2:17 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> > +       if (!opts->checkout_worktree && !opts->checkout_index)
> > +               die(_("neither '%s' or '%s' is specified"),
> > +                   "--index", "--worktree");
>
> Is this die() or BUG()?  I thought --worktree was the default.

The user might type "git restore --no-worktree --no-index" which would
trigger this error message, so definitely die(), not a BUG().
Duy Nguyen March 13, 2019, 10:02 a.m. UTC | #3
On Sun, Mar 10, 2019 at 1:52 AM Elijah Newren <newren@gmail.com> wrote:
> > +               /*
> > +                * NEEDSWORK: if --worktree is not specified, we
> > +                * should save stat info of checked out files in the
> > +                * index to avoid the next (potentially costly)
> > +                * refresh. But it's a bit tricker to do...
> > +                */
> > +               rollback_lock_file(&lock_file);
>
> A total tangent: I see both FIXME and NEEDSWORK in the codebase.  Are
> there other 'keywords' of this type that we use?  Is there a
> preference for how they are used?

I don't think so. I've seen FIXME, NEEDSWORK, TODO and XXX. I think
it's often up to the author to pick one. We could unify and use just
one keyword, which helps spot these. But I don't think we keep future
other keywords out long term. This seems to pick up most of them with
a couple false positives

git grep '^\s*\* [A-Z]\+: ' \*.c
Junio C Hamano March 13, 2019, 10:42 p.m. UTC | #4
Elijah Newren <newren@gmail.com> writes:

>> +               /*
>> +                * NEEDSWORK: if --worktree is not specified, we
>> +                * should save stat info of checked out files in the
>> +                * index to avoid the next (potentially costly)
>> +                * refresh. But it's a bit tricker to do...
>> +                */
>> +               rollback_lock_file(&lock_file);
>
> A total tangent: I see both FIXME and NEEDSWORK in the codebase.
> Are there other 'keywords' of this type that we use?  Is there a
> preference for how they are used?

If it makes it simpler, I can easily declare that NEEDSWORK is the
preferred one (I do not think I ever wrote anything else) to avoid
wasting list bandwidth ;-)
diff mbox series

Patch

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5fb85e7b73..07b431be51 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -62,6 +62,8 @@  struct checkout_opts {
 	int switch_branch_doing_nothing_is_ok;
 	int only_merge_on_switching_branches;
 	int empty_pathspec_ok;
+	int checkout_index;
+	int checkout_worktree;
 
 	const char *new_branch;
 	const char *new_branch_force;
@@ -393,6 +395,7 @@  static int checkout_paths(const struct checkout_opts *opts,
 	struct commit *head;
 	int errs = 0;
 	struct lock_file lock_file = LOCK_INIT;
+	int checkout_index;
 
 	trace2_cmd_mode(opts->patch_mode ? "patch" : "path");
 
@@ -418,9 +421,26 @@  static int checkout_paths(const struct checkout_opts *opts,
 		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);
+	if (!opts->checkout_worktree && !opts->checkout_index)
+		die(_("neither '%s' or '%s' is specified"),
+		    "--index", "--worktree");
+
+	if (!opts->source_tree && !opts->checkout_worktree)
+		die(_("'%s' must be used when '%s' is not specified"),
+		    "--worktree", "--source");
+
+	if (opts->patch_mode) {
+		const char *patch_mode;
+
+		if (opts->checkout_index && opts->checkout_worktree)
+			patch_mode = "--patch=checkout";
+		else if (opts->checkout_index && !opts->checkout_worktree)
+			patch_mode = "--patch=reset";
+		else
+			die(_("'%s' with only '%s' is not currently supported"),
+			    "--patch", "--worktree");
+		return run_add_interactive(revision, patch_mode, &opts->pathspec);
+	}
 
 	repo_hold_locked_index(the_repository, &lock_file, LOCK_DIE_ON_ERROR);
 	if (read_cache_preload(&opts->pathspec) < 0)
@@ -478,10 +498,30 @@  static int checkout_paths(const struct checkout_opts *opts,
 		return 1;
 
 	/* Now we are committed to check them out */
-	errs |= checkout_worktree(opts);
+	if (opts->checkout_worktree)
+		errs |= checkout_worktree(opts);
 
-	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-		die(_("unable to write new index file"));
+	/*
+	 * Allow updating the index when checking out from the index.
+	 * This is to save new stat info.
+	 */
+	if (opts->checkout_worktree && !opts->checkout_index && !opts->source_tree)
+		checkout_index = 1;
+	else
+		checkout_index = opts->checkout_index;
+
+	if (checkout_index) {
+		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
+			die(_("unable to write new index file"));
+	} else {
+		/*
+		 * NEEDSWORK: if --worktree is not specified, we
+		 * should save stat info of checked out files in the
+		 * index to avoid the next (potentially costly)
+		 * refresh. But it's a bit tricker to do...
+		 */
+		rollback_lock_file(&lock_file);
+	}
 
 	read_ref_full("HEAD", 0, &rev, NULL);
 	head = lookup_commit_reference_gently(the_repository, &rev, 1);
@@ -1373,6 +1413,20 @@  static int checkout_main(int argc, const char **argv, const char *prefix,
 	if (opts->overlay_mode == 1 && opts->patch_mode)
 		die(_("-p and --overlay are mutually exclusive"));
 
+	if (opts->checkout_index >= 0 || opts->checkout_worktree >= 0) {
+		if (opts->checkout_index < 0)
+			opts->checkout_index = 0;
+		if (opts->checkout_worktree < 0)
+			opts->checkout_worktree = 0;
+	} else {
+		if (opts->checkout_index < 0)
+			opts->checkout_index = -opts->checkout_index - 1;
+		if (opts->checkout_worktree < 0)
+			opts->checkout_worktree = -opts->checkout_worktree - 1;
+	}
+	if (opts->checkout_index < 0 || opts->checkout_worktree < 0)
+		BUG("these flags should be non-negative by now");
+
 	/*
 	 * From here on, new_branch will contain the branch to be checked out,
 	 * and new_branch_force and new_orphan_branch will tell us which one of
@@ -1527,6 +1581,8 @@  int cmd_checkout(int argc, const char **argv, const char *prefix)
 	opts.implicit_detach = 1;
 	opts.empty_pathspec_ok = 1;
 	opts.overlay_mode = -1;
+	opts.checkout_index = -2;    /* default on */
+	opts.checkout_worktree = -2; /* default on */
 
 	options = parse_options_dup(checkout_options);
 	options = add_common_options(&opts, options);
@@ -1580,6 +1636,10 @@  int cmd_restore(int argc, const char **argv, const char *prefix)
 	struct option restore_options[] = {
 		OPT_STRING('s', "source", &opts.from_treeish, "<tree-ish>",
 			   N_("where the checkout from")),
+		OPT_BOOL('I', "index", &opts.checkout_index,
+			   N_("restore the index")),
+		OPT_BOOL('W', "worktree", &opts.checkout_worktree,
+			   N_("restore the working tree (default)")),
 		OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")),
 		OPT_END()
 	};
@@ -1590,6 +1650,8 @@  int cmd_restore(int argc, const char **argv, const char *prefix)
 	opts.accept_pathspec = 1;
 	opts.empty_pathspec_ok = 0;
 	opts.overlay_mode = 0;
+	opts.checkout_index = -1;    /* default off */
+	opts.checkout_worktree = -2; /* default on */
 
 	options = parse_options_dup(restore_options);
 	options = add_common_options(&opts, options);