[6/8] checkout: add --cached option
diff mbox series

Message ID 20181209200449.16342-7-t.gummerer@gmail.com
State New
Headers show
Series
  • introduce no-overlay and cached mode in git checkout
Related show

Commit Message

Thomas Gummerer Dec. 9, 2018, 8:04 p.m. UTC
Add a new --cached option to git checkout, which works only on the
index, but not the working tree, similar to what 'git reset <tree-ish>
-- <pathspec>... does.  Indeed the tests are adapted from the 'git
reset' tests.

In the longer term the idea is to potentially deprecate 'git reset
<tree-ish> -- <pathspec>...', so the 'git reset' command becomes only
about re-pointing the HEAD, and not also about copying entries from
<tree-ish> to the index.

Note that 'git checkout' by default works in overlay mode, meaning
files that match the pathspec that don't exist in <tree-ish>, but
exist in the index would not be removed.  'git checkout --no-overlay
--cached' can be used to get the same behaviour as 'git reset
<tree-ish> -- <pathspec>'.

One thing this patch doesn't currently deal with is conflicts.
Currently 'git checkout --{ours,theirs} -- <file-with-conflicts>'
doesn't do anything with the index, so the --cached option just
mirrors that behaviour.  But given it doesn't even deal with
conflicts, the '--cached' option doesn't make much sense when no
<tree-ish> is given.  As it operates only on the index, it's always a
no-op if no tree-ish is given.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---

Maybe we can just disallow --cached without <tree-ish> given for now,
and possibly later allow it with some different behaviour for
conflicts, not sure what the best way forward here is.  We can also
just make it update the index as appropriate, and have it behave
different than 'git checkout' curerntly does when handling conflicts?

 builtin/checkout.c         |  26 ++++++++--
 t/t2016-checkout-patch.sh  |   8 +++
 t/t2026-checkout-cached.sh | 103 +++++++++++++++++++++++++++++++++++++
 t/t9902-completion.sh      |   1 +
 4 files changed, 135 insertions(+), 3 deletions(-)
 create mode 100755 t/t2026-checkout-cached.sh

Comments

Duy Nguyen Dec. 10, 2018, 4:49 p.m. UTC | #1
On Sun, Dec 9, 2018 at 9:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> Add a new --cached option to git checkout, which works only on the
> index, but not the working tree, similar to what 'git reset <tree-ish>
> -- <pathspec>... does.

Elijah wanted another mode (and I agree) that modifies worktree but
leaves the index alone. This is most useful (or least confusing) when
used with <tree-ish> and would be default in restore-files. I'm not
saying you have to implement it, but how do the new command line
options are designed to make sense?

I guess if --cached is "update index only" then --no-cached goes back
to the default "update both worktree and index" and we need a another
option for "worktree only"? Can we have one option with three possible
values (index-only, index-and-worktree, worktree-only) maybe?
Elijah Newren Dec. 10, 2018, 6:42 p.m. UTC | #2
On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> Add a new --cached option to git checkout, which works only on the
> index, but not the working tree, similar to what 'git reset <tree-ish>
> -- <pathspec>... does.  Indeed the tests are adapted from the 'git
> reset' tests.
>
> In the longer term the idea is to potentially deprecate 'git reset
> <tree-ish> -- <pathspec>...', so the 'git reset' command becomes only
> about re-pointing the HEAD, and not also about copying entries from
> <tree-ish> to the index.
>
> Note that 'git checkout' by default works in overlay mode, meaning
> files that match the pathspec that don't exist in <tree-ish>, but
> exist in the index would not be removed.  'git checkout --no-overlay
> --cached' can be used to get the same behaviour as 'git reset
> <tree-ish> -- <pathspec>'.

I think this argues _even more_ that --no-overlay should be the
default.  Your series is valuable even if we don't push on that, I'm
just being noisy about what I think would be an even better world.

Also, I don't think I've mentioned it yet, but I'm really excited
about this series and what you're doing.  It's super cool.  (Which I
expected when I saw the description of the desired behavior, but I'm
also liking and contemplating re-using some code...)

> One thing this patch doesn't currently deal with is conflicts.
> Currently 'git checkout --{ours,theirs} -- <file-with-conflicts>'
> doesn't do anything with the index, so the --cached option just
> mirrors that behaviour.  But given it doesn't even deal with
> conflicts, the '--cached' option doesn't make much sense when no
> <tree-ish> is given.  As it operates only on the index, it's always a
> no-op if no tree-ish is given.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>
> Maybe we can just disallow --cached without <tree-ish> given for now,
> and possibly later allow it with some different behaviour for
> conflicts, not sure what the best way forward here is.  We can also
> just make it update the index as appropriate, and have it behave
> different than 'git checkout' curerntly does when handling conflicts?

Huh?
  "git checkout -- <path>"
means update <path> from the index, meaning the index is left alone
(it's the source) and only the working tree is touched.

When you add a flag named --cached to only update the index and not
the working tree, then the index becomes the sole destination.

Now we combine: no tree is specified means the index is the source of
the writing, and --cached being specified means the index is the sole
destination of the writing.  Thus, you have a no-op.  If the user
specifies --cached and no tree, you should immediately exit with a
message along the lines of "Nothing to do; no tree given and --cached
specified."  The presence of conflicts seems completely irrelevant to
me here.

>  builtin/checkout.c         |  26 ++++++++--
>  t/t2016-checkout-patch.sh  |   8 +++
>  t/t2026-checkout-cached.sh | 103 +++++++++++++++++++++++++++++++++++++
>  t/t9902-completion.sh      |   1 +
>  4 files changed, 135 insertions(+), 3 deletions(-)
>  create mode 100755 t/t2026-checkout-cached.sh
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 0aef35bbc4..6ba85e9de5 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -45,6 +45,7 @@ struct checkout_opts {
>         int ignore_other_worktrees;
>         int show_progress;
>         int overlay_mode;
> +       int cached;
>         /*
>          * If new checkout options are added, skip_merge_working_tree
>          * should be updated accordingly.
> @@ -288,6 +289,10 @@ 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 && opts->cached)
> +               return run_add_interactive(revision, "--patch=reset",
> +                                          &opts->pathspec);
> +
>         if (opts->patch_mode)
>                 return run_add_interactive(revision, "--patch=checkout",
>                                            &opts->pathspec);
> @@ -319,7 +324,9 @@ static int checkout_paths(const struct checkout_opts *opts,
>                                  * the current index, which means that it should
>                                  * be removed.
>                                  */
> -                               ce->ce_flags |= CE_MATCHED | CE_REMOVE | CE_WT_REMOVE;
> +                               ce->ce_flags |= CE_MATCHED | CE_REMOVE;
> +                               if (!opts->cached)
> +                                       ce->ce_flags |= CE_WT_REMOVE;
>                                 continue;
>                         } else {
>                                 /*
> @@ -392,6 +399,9 @@ static int checkout_paths(const struct checkout_opts *opts,
>         for (pos = 0; pos < active_nr; pos++) {
>                 struct cache_entry *ce = active_cache[pos];
>                 if (ce->ce_flags & CE_MATCHED) {
> +                       if (opts->cached) {
> +                               continue;
> +                       }
>                         if (!ce_stage(ce)) {
>                                 errs |= checkout_entry(ce, &state, NULL);
>                                 continue;
> @@ -571,6 +581,11 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
>          * not tested here
>          */
>
> +       /*
> +        * opts->cached cannot be used with switching branches so is
> +        * not tested here
> +        */
> +
>         /*
>          * If we aren't creating a new branch any changes or updates will
>          * happen in the existing branch.  Since that could only be updating
> @@ -1207,9 +1222,13 @@ static int checkout_branch(struct checkout_opts *opts,
>                 die(_("'%s' cannot be used with switching branches"),
>                     "--patch");
>
> -       if (!opts->overlay_mode)
> +       if (opts->overlay_mode != -1)
> +               die(_("'%s' cannot be used with switching branches"),
> +                   "--overlay/--no-overlay");
> +
> +       if (opts->cached)
>                 die(_("'%s' cannot be used with switching branches"),
> -                   "--no-overlay");
> +                   "--cached");
>
>         if (opts->writeout_stage)
>                 die(_("'%s' cannot be used with switching branches"),
> @@ -1300,6 +1319,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>                             PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater },
>                 OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")),
>                 OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")),
> +               OPT_BOOL(0, "cached", &opts.cached, N_("work on the index only")),
>                 OPT_END(),
>         };
>
> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
> index 47aeb0b167..e8774046e0 100755
> --- a/t/t2016-checkout-patch.sh
> +++ b/t/t2016-checkout-patch.sh
> @@ -108,6 +108,14 @@ test_expect_success PERL 'path limiting works: foo inside dir' '
>         verify_state dir/foo head head
>  '
>
> +test_expect_success PERL 'git checkout --cached -p' '
> +       set_and_save_state dir/foo work work &&
> +       test_write_lines n y | git checkout --cached -p >output &&
> +       verify_state dir/foo work head &&
> +       verify_saved_state bar &&
> +       test_i18ngrep "Unstage" output
> +'
> +
>  test_expect_success PERL 'none of this moved HEAD' '
>         verify_saved_head
>  '
> diff --git a/t/t2026-checkout-cached.sh b/t/t2026-checkout-cached.sh
> new file mode 100755
> index 0000000000..1b66192727
> --- /dev/null
> +++ b/t/t2026-checkout-cached.sh
> @@ -0,0 +1,103 @@
> +#!/bin/sh
> +
> +test_description='checkout --cached <pathspec>'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'checkout --cached <pathspec>' '
> +       echo 1 >file1 &&
> +       echo 2 >file2 &&
> +       git add file1 file2 &&
> +       test_tick &&
> +       git commit -m files &&
> +       git rm file2 &&
> +       echo 3 >file3 &&
> +       echo 4 >file1 &&
> +       git add file1 file3 &&
> +       git checkout --cached HEAD -- file1 file2 &&
> +       test_must_fail git diff --quiet &&
> +
> +       cat >expect <<-\EOF &&
> +       diff --git a/file1 b/file1
> +       index d00491f..b8626c4 100644
> +       --- a/file1
> +       +++ b/file1
> +       @@ -1 +1 @@
> +       -1
> +       +4
> +       diff --git a/file2 b/file2
> +       deleted file mode 100644
> +       index 0cfbf08..0000000
> +       --- a/file2
> +       +++ /dev/null
> +       @@ -1 +0,0 @@
> +       -2
> +       EOF
> +       git diff >actual &&
> +       test_cmp expect actual &&
> +
> +       cat >expect <<-\EOF &&
> +       diff --git a/file3 b/file3
> +       new file mode 100644
> +       index 0000000..00750ed
> +       --- /dev/null
> +       +++ b/file3
> +       @@ -0,0 +1 @@
> +       +3
> +       EOF
> +       git diff --cached >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'checking out an unmodified path is a no-op' '
> +       git reset --hard &&
> +       git checkout --cached HEAD -- file1 &&
> +       git diff-files --exit-code &&
> +       git diff-index --cached --exit-code HEAD
> +'
> +
> +test_expect_success 'checking out specific path that is unmerged' '
> +       test_commit file3 file3 &&
> +       git rm --cached file2 &&
> +       echo 1234 >file2 &&
> +       F1=$(git rev-parse HEAD:file1) &&
> +       F2=$(git rev-parse HEAD:file2) &&
> +       F3=$(git rev-parse HEAD:file3) &&
> +       {
> +               echo "100644 $F1 1      file2" &&
> +               echo "100644 $F2 2      file2" &&
> +               echo "100644 $F3 3      file2"
> +       } | git update-index --index-info &&
> +       git ls-files -u &&
> +       git checkout --cached HEAD file2 &&
> +       test_must_fail git diff --quiet &&
> +       git diff-index --exit-code --cached HEAD
> +'
> +
> +test_expect_success '--cached without --no-overlay does not remove entry from index' '
> +       test_must_fail git checkout --cached HEAD^ file3 &&
> +       git ls-files --error-unmatch -- file3
> +'
> +
> +test_expect_success 'file is removed from the index with --no-overlay' '
> +       git checkout --cached --no-overlay HEAD^ file3 &&
> +       test_path_is_file file3 &&
> +       test_must_fail git ls-files --error-unmatch -- file3
> +'
> +
> +test_expect_success 'test checkout --cached --no-overlay at given paths' '
> +       mkdir sub &&
> +       >sub/file1 &&
> +       >sub/file2 &&
> +       git update-index --add sub/file1 sub/file2 &&
> +       T=$(git write-tree) &&
> +       git checkout --cached --no-overlay HEAD sub/file2 &&
> +       test_must_fail git diff --quiet &&
> +       U=$(git write-tree) &&

Do we need to worry at all about losing the exit status of write-tree
in either invocation?  In particular, if the second one for U fails
somehow, we'd end up with $U being a blank string and we'd still
probably get "$T" != "$U" below.

You also had some rev-parse invocations hidden in a sub-shell in both
this patch and patch 5, but subsequent commands relied on non-empty
output out of those, so I figured those were fine.  This one might be
too, but I thought I'd at least mention it.

> +       echo "$T" &&
> +       echo "$U" &&
> +       test_must_fail git diff-index --cached --exit-code "$T" &&
> +       test "$T" != "$U"
> +'
> +
> +test_done
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index a3fd9a9630..cbc304ace8 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -1437,6 +1437,7 @@ test_expect_success 'double dash "git checkout"' '
>         --no-quiet Z
>         --no-... Z
>         --overlay Z
> +       --cached Z
>         EOF
>  '
>
> --
> 2.20.0.405.gbc1bbc6f85
Junio C Hamano Dec. 11, 2018, 3:13 a.m. UTC | #3
Duy Nguyen <pclouds@gmail.com> writes:

> Elijah wanted another mode (and I agree) that modifies worktree but
> leaves the index alone. This is most useful (or least confusing) when
> used with <tree-ish> and would be default in restore-files. I'm not
> saying you have to implement it, but how do the new command line
> options are designed to make sense?

I'd model it after "git apply", i.e.

	git restore-files [--tree=<treeish>] <pathspec>

would work only on the working tree files,

	git restore-files --tree=<treeish> --cached <pathspec>

would match the entries in the index that match pathspec to the
given treeish without touching the working tree, and

	git restore-files --tree=<treeish> --index <pathspec>

would be both.

I have never been happy with the phraso, the (arbitrary) distinction
between --cached/--index we use, so in the very longer term (like,
introducing synonym at 3.0 boundary and deprecating older ones at
4.0 boundary), it may not be a bad idea to rename "--index" to
"--index-and-working-tree" and "--cached" to "--index-only".  

But until that happens, it would be better to use these two
consistently.
Elijah Newren Dec. 11, 2018, 6:12 a.m. UTC | #4
On Mon, Dec 10, 2018 at 7:13 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Duy Nguyen <pclouds@gmail.com> writes:
>
> > Elijah wanted another mode (and I agree) that modifies worktree but
> > leaves the index alone. This is most useful (or least confusing) when
> > used with <tree-ish> and would be default in restore-files. I'm not
> > saying you have to implement it, but how do the new command line
> > options are designed to make sense?
>
> I'd model it after "git apply", i.e.
>
>         git restore-files [--tree=<treeish>] <pathspec>
>
> would work only on the working tree files,
>
>         git restore-files --tree=<treeish> --cached <pathspec>
>
> would match the entries in the index that match pathspec to the
> given treeish without touching the working tree, and
>
>         git restore-files --tree=<treeish> --index <pathspec>
>
> would be both.
>
> I have never been happy with the phraso, the (arbitrary) distinction
> between --cached/--index we use, so in the very longer term (like,
> introducing synonym at 3.0 boundary and deprecating older ones at
> 4.0 boundary), it may not be a bad idea to rename "--index" to
> "--index-and-working-tree" and "--cached" to "--index-only".
>
> But until that happens, it would be better to use these two
> consistently.

I agree that consistency is important, but trying to distinguish
between "--cached" and "--index" is _extremely_ painful.  I still
can't keep the distinction straight and have to look it up every time
I want to use either.  I don't know how to possibly teach users the
meaning.  Could we introduce synonyms earlier at least, and make the
synonyms more prominent than the "--cached" and "--index" terms in the
documentation?
Duy Nguyen Dec. 11, 2018, 7:23 p.m. UTC | #5
On Tue, Dec 11, 2018 at 7:12 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Mon, Dec 10, 2018 at 7:13 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Duy Nguyen <pclouds@gmail.com> writes:
> >
> > > Elijah wanted another mode (and I agree) that modifies worktree but
> > > leaves the index alone. This is most useful (or least confusing) when
> > > used with <tree-ish> and would be default in restore-files. I'm not
> > > saying you have to implement it, but how do the new command line
> > > options are designed to make sense?
> >
> > I'd model it after "git apply", i.e.
> >
> >         git restore-files [--tree=<treeish>] <pathspec>
> >
> > would work only on the working tree files,
> >
> >         git restore-files --tree=<treeish> --cached <pathspec>
> >
> > would match the entries in the index that match pathspec to the
> > given treeish without touching the working tree, and
> >
> >         git restore-files --tree=<treeish> --index <pathspec>
> >
> > would be both.
> >
> > I have never been happy with the phraso, the (arbitrary) distinction
> > between --cached/--index we use, so in the very longer term (like,
> > introducing synonym at 3.0 boundary and deprecating older ones at
> > 4.0 boundary), it may not be a bad idea to rename "--index" to
> > "--index-and-working-tree" and "--cached" to "--index-only".
> >
> > But until that happens, it would be better to use these two
> > consistently.
>
> I agree that consistency is important, but trying to distinguish
> between "--cached" and "--index" is _extremely_ painful.  I still
> can't keep the distinction straight and have to look it up every time
> I want to use either.  I don't know how to possibly teach users the
> meaning.  Could we introduce synonyms earlier at least, and make the
> synonyms more prominent than the "--cached" and "--index" terms in the
> documentation?

For git-checkout I think --index and --cached fit. For restore-files,
if you come up with better names, I'll gladly use that. Otherwise I'll
just use these.
Thomas Gummerer Dec. 11, 2018, 10:18 p.m. UTC | #6
On 12/10, Elijah Newren wrote:
> On Sun, Dec 9, 2018 at 12:05 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >
> > Add a new --cached option to git checkout, which works only on the
> > index, but not the working tree, similar to what 'git reset <tree-ish>
> > -- <pathspec>... does.  Indeed the tests are adapted from the 'git
> > reset' tests.
> >
> > In the longer term the idea is to potentially deprecate 'git reset
> > <tree-ish> -- <pathspec>...', so the 'git reset' command becomes only
> > about re-pointing the HEAD, and not also about copying entries from
> > <tree-ish> to the index.
> >
> > Note that 'git checkout' by default works in overlay mode, meaning
> > files that match the pathspec that don't exist in <tree-ish>, but
> > exist in the index would not be removed.  'git checkout --no-overlay
> > --cached' can be used to get the same behaviour as 'git reset
> > <tree-ish> -- <pathspec>'.
> 
> I think this argues _even more_ that --no-overlay should be the
> default.  Your series is valuable even if we don't push on that, I'm
> just being noisy about what I think would be an even better world.

I think just having that mode in 'git restore-files' Duy is working on
may have to be enough for now.

> Also, I don't think I've mentioned it yet, but I'm really excited
> about this series and what you're doing.  It's super cool.  (Which I
> expected when I saw the description of the desired behavior, but I'm
> also liking and contemplating re-using some code...)

Thanks :)

> > One thing this patch doesn't currently deal with is conflicts.
> > Currently 'git checkout --{ours,theirs} -- <file-with-conflicts>'
> > doesn't do anything with the index, so the --cached option just
> > mirrors that behaviour.  But given it doesn't even deal with
> > conflicts, the '--cached' option doesn't make much sense when no
> > <tree-ish> is given.  As it operates only on the index, it's always a
> > no-op if no tree-ish is given.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> >
> > Maybe we can just disallow --cached without <tree-ish> given for now,
> > and possibly later allow it with some different behaviour for
> > conflicts, not sure what the best way forward here is.  We can also
> > just make it update the index as appropriate, and have it behave
> > different than 'git checkout' curerntly does when handling conflicts?
> 
> Huh?
>   "git checkout -- <path>"
> means update <path> from the index, meaning the index is left alone
> (it's the source) and only the working tree is touched.
> 
> When you add a flag named --cached to only update the index and not
> the working tree, then the index becomes the sole destination.
> 
> Now we combine: no tree is specified means the index is the source of
> the writing, and --cached being specified means the index is the sole
> destination of the writing.  Thus, you have a no-op.  If the user
> specifies --cached and no tree, you should immediately exit with a
> message along the lines of "Nothing to do; no tree given and --cached
> specified."  The presence of conflicts seems completely irrelevant to
> me here.

Ah yeah you're right, thanks for a sanity check.  The command I was
most worried about was 'git checkout --cached --{ours,theirs} -- <pathspec>', 
which I thought should update the index.  But as we don't give any
tree-ish, I'm not sure anymore it should.  Maybe just always exiting
with the message you mention above is the right thing to do.

> >  builtin/checkout.c         |  26 ++++++++--
> >  t/t2016-checkout-patch.sh  |   8 +++
> >  t/t2026-checkout-cached.sh | 103 +++++++++++++++++++++++++++++++++++++
> >  t/t9902-completion.sh      |   1 +
> >  4 files changed, 135 insertions(+), 3 deletions(-)
> >  create mode 100755 t/t2026-checkout-cached.sh
> >
> > diff --git a/builtin/checkout.c b/builtin/checkout.c
> > index 0aef35bbc4..6ba85e9de5 100644
> > --- a/builtin/checkout.c
> > +++ b/builtin/checkout.c
> > @@ -45,6 +45,7 @@ struct checkout_opts {
> >         int ignore_other_worktrees;
> >         int show_progress;
> >         int overlay_mode;
> > +       int cached;
> >         /*
> >          * If new checkout options are added, skip_merge_working_tree
> >          * should be updated accordingly.
> > @@ -288,6 +289,10 @@ 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 && opts->cached)
> > +               return run_add_interactive(revision, "--patch=reset",
> > +                                          &opts->pathspec);
> > +
> >         if (opts->patch_mode)
> >                 return run_add_interactive(revision, "--patch=checkout",
> >                                            &opts->pathspec);
> > @@ -319,7 +324,9 @@ static int checkout_paths(const struct checkout_opts *opts,
> >                                  * the current index, which means that it should
> >                                  * be removed.
> >                                  */
> > -                               ce->ce_flags |= CE_MATCHED | CE_REMOVE | CE_WT_REMOVE;
> > +                               ce->ce_flags |= CE_MATCHED | CE_REMOVE;
> > +                               if (!opts->cached)
> > +                                       ce->ce_flags |= CE_WT_REMOVE;
> >                                 continue;
> >                         } else {
> >                                 /*
> > @@ -392,6 +399,9 @@ static int checkout_paths(const struct checkout_opts *opts,
> >         for (pos = 0; pos < active_nr; pos++) {
> >                 struct cache_entry *ce = active_cache[pos];
> >                 if (ce->ce_flags & CE_MATCHED) {
> > +                       if (opts->cached) {
> > +                               continue;
> > +                       }
> >                         if (!ce_stage(ce)) {
> >                                 errs |= checkout_entry(ce, &state, NULL);
> >                                 continue;
> > @@ -571,6 +581,11 @@ static int skip_merge_working_tree(const struct checkout_opts *opts,
> >          * not tested here
> >          */
> >
> > +       /*
> > +        * opts->cached cannot be used with switching branches so is
> > +        * not tested here
> > +        */
> > +
> >         /*
> >          * If we aren't creating a new branch any changes or updates will
> >          * happen in the existing branch.  Since that could only be updating
> > @@ -1207,9 +1222,13 @@ static int checkout_branch(struct checkout_opts *opts,
> >                 die(_("'%s' cannot be used with switching branches"),
> >                     "--patch");
> >
> > -       if (!opts->overlay_mode)
> > +       if (opts->overlay_mode != -1)
> > +               die(_("'%s' cannot be used with switching branches"),
> > +                   "--overlay/--no-overlay");
> > +
> > +       if (opts->cached)
> >                 die(_("'%s' cannot be used with switching branches"),
> > -                   "--no-overlay");
> > +                   "--cached");
> >
> >         if (opts->writeout_stage)
> >                 die(_("'%s' cannot be used with switching branches"),
> > @@ -1300,6 +1319,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
> >                             PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater },
> >                 OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")),
> >                 OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")),
> > +               OPT_BOOL(0, "cached", &opts.cached, N_("work on the index only")),
> >                 OPT_END(),
> >         };
> >
> > diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
> > index 47aeb0b167..e8774046e0 100755
> > --- a/t/t2016-checkout-patch.sh
> > +++ b/t/t2016-checkout-patch.sh
> > @@ -108,6 +108,14 @@ test_expect_success PERL 'path limiting works: foo inside dir' '
> >         verify_state dir/foo head head
> >  '
> >
> > +test_expect_success PERL 'git checkout --cached -p' '
> > +       set_and_save_state dir/foo work work &&
> > +       test_write_lines n y | git checkout --cached -p >output &&
> > +       verify_state dir/foo work head &&
> > +       verify_saved_state bar &&
> > +       test_i18ngrep "Unstage" output
> > +'
> > +
> >  test_expect_success PERL 'none of this moved HEAD' '
> >         verify_saved_head
> >  '
> > diff --git a/t/t2026-checkout-cached.sh b/t/t2026-checkout-cached.sh
> > new file mode 100755
> > index 0000000000..1b66192727
> > --- /dev/null
> > +++ b/t/t2026-checkout-cached.sh
> > @@ -0,0 +1,103 @@
> > +#!/bin/sh
> > +
> > +test_description='checkout --cached <pathspec>'
> > +
> > +. ./test-lib.sh
> > +
> > +test_expect_success 'checkout --cached <pathspec>' '
> > +       echo 1 >file1 &&
> > +       echo 2 >file2 &&
> > +       git add file1 file2 &&
> > +       test_tick &&
> > +       git commit -m files &&
> > +       git rm file2 &&
> > +       echo 3 >file3 &&
> > +       echo 4 >file1 &&
> > +       git add file1 file3 &&
> > +       git checkout --cached HEAD -- file1 file2 &&
> > +       test_must_fail git diff --quiet &&
> > +
> > +       cat >expect <<-\EOF &&
> > +       diff --git a/file1 b/file1
> > +       index d00491f..b8626c4 100644
> > +       --- a/file1
> > +       +++ b/file1
> > +       @@ -1 +1 @@
> > +       -1
> > +       +4
> > +       diff --git a/file2 b/file2
> > +       deleted file mode 100644
> > +       index 0cfbf08..0000000
> > +       --- a/file2
> > +       +++ /dev/null
> > +       @@ -1 +0,0 @@
> > +       -2
> > +       EOF
> > +       git diff >actual &&
> > +       test_cmp expect actual &&
> > +
> > +       cat >expect <<-\EOF &&
> > +       diff --git a/file3 b/file3
> > +       new file mode 100644
> > +       index 0000000..00750ed
> > +       --- /dev/null
> > +       +++ b/file3
> > +       @@ -0,0 +1 @@
> > +       +3
> > +       EOF
> > +       git diff --cached >actual &&
> > +       test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'checking out an unmodified path is a no-op' '
> > +       git reset --hard &&
> > +       git checkout --cached HEAD -- file1 &&
> > +       git diff-files --exit-code &&
> > +       git diff-index --cached --exit-code HEAD
> > +'
> > +
> > +test_expect_success 'checking out specific path that is unmerged' '
> > +       test_commit file3 file3 &&
> > +       git rm --cached file2 &&
> > +       echo 1234 >file2 &&
> > +       F1=$(git rev-parse HEAD:file1) &&
> > +       F2=$(git rev-parse HEAD:file2) &&
> > +       F3=$(git rev-parse HEAD:file3) &&
> > +       {
> > +               echo "100644 $F1 1      file2" &&
> > +               echo "100644 $F2 2      file2" &&
> > +               echo "100644 $F3 3      file2"
> > +       } | git update-index --index-info &&
> > +       git ls-files -u &&
> > +       git checkout --cached HEAD file2 &&
> > +       test_must_fail git diff --quiet &&
> > +       git diff-index --exit-code --cached HEAD
> > +'
> > +
> > +test_expect_success '--cached without --no-overlay does not remove entry from index' '
> > +       test_must_fail git checkout --cached HEAD^ file3 &&
> > +       git ls-files --error-unmatch -- file3
> > +'
> > +
> > +test_expect_success 'file is removed from the index with --no-overlay' '
> > +       git checkout --cached --no-overlay HEAD^ file3 &&
> > +       test_path_is_file file3 &&
> > +       test_must_fail git ls-files --error-unmatch -- file3
> > +'
> > +
> > +test_expect_success 'test checkout --cached --no-overlay at given paths' '
> > +       mkdir sub &&
> > +       >sub/file1 &&
> > +       >sub/file2 &&
> > +       git update-index --add sub/file1 sub/file2 &&
> > +       T=$(git write-tree) &&
> > +       git checkout --cached --no-overlay HEAD sub/file2 &&
> > +       test_must_fail git diff --quiet &&
> > +       U=$(git write-tree) &&
> 
> Do we need to worry at all about losing the exit status of write-tree
> in either invocation?  In particular, if the second one for U fails
> somehow, we'd end up with $U being a blank string and we'd still
> probably get "$T" != "$U" below.

Hmm this seems to be a fairly common pattern in our test suite:

$ git grep -F '$(git write-tree)' t/* | wc -l
112

But maybe it's just something we used to do, but should move away
from.  Just writing the output to a file shouldn't be much harder
either, I'll do that in the next iteration.

> You also had some rev-parse invocations hidden in a sub-shell in both
> this patch and patch 5, but subsequent commands relied on non-empty
> output out of those, so I figured those were fine.  This one might be
> too, but I thought I'd at least mention it.
> 
> > +       echo "$T" &&
> > +       echo "$U" &&
> > +       test_must_fail git diff-index --cached --exit-code "$T" &&
> > +       test "$T" != "$U"
> > +'
> > +
> > +test_done
> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> > index a3fd9a9630..cbc304ace8 100755
> > --- a/t/t9902-completion.sh
> > +++ b/t/t9902-completion.sh
> > @@ -1437,6 +1437,7 @@ test_expect_success 'double dash "git checkout"' '
> >         --no-quiet Z
> >         --no-... Z
> >         --overlay Z
> > +       --cached Z
> >         EOF
> >  '
> >
> > --
> > 2.20.0.405.gbc1bbc6f85
Duy Nguyen Jan. 31, 2019, 5:54 a.m. UTC | #7
On Wed, Dec 12, 2018 at 2:23 AM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Tue, Dec 11, 2018 at 7:12 AM Elijah Newren <newren@gmail.com> wrote:
> >
> > On Mon, Dec 10, 2018 at 7:13 PM Junio C Hamano <gitster@pobox.com> wrote:
> > >
> > > Duy Nguyen <pclouds@gmail.com> writes:
> > >
> > > > Elijah wanted another mode (and I agree) that modifies worktree but
> > > > leaves the index alone. This is most useful (or least confusing) when
> > > > used with <tree-ish> and would be default in restore-files. I'm not
> > > > saying you have to implement it, but how do the new command line
> > > > options are designed to make sense?
> > >
> > > I'd model it after "git apply", i.e.
> > >
> > >         git restore-files [--tree=<treeish>] <pathspec>
> > >
> > > would work only on the working tree files,
> > >
> > >         git restore-files --tree=<treeish> --cached <pathspec>
> > >
> > > would match the entries in the index that match pathspec to the
> > > given treeish without touching the working tree, and
> > >
> > >         git restore-files --tree=<treeish> --index <pathspec>
> > >
> > > would be both.
> > >
> > > I have never been happy with the phraso, the (arbitrary) distinction
> > > between --cached/--index we use, so in the very longer term (like,
> > > introducing synonym at 3.0 boundary and deprecating older ones at
> > > 4.0 boundary), it may not be a bad idea to rename "--index" to
> > > "--index-and-working-tree" and "--cached" to "--index-only".
> > >
> > > But until that happens, it would be better to use these two
> > > consistently.
> >
> > I agree that consistency is important, but trying to distinguish
> > between "--cached" and "--index" is _extremely_ painful.  I still
> > can't keep the distinction straight and have to look it up every time
> > I want to use either.  I don't know how to possibly teach users the
> > meaning.  Could we introduce synonyms earlier at least, and make the
> > synonyms more prominent than the "--cached" and "--index" terms in the
> > documentation?
>
> For git-checkout I think --index and --cached fit. For restore-files,
> if you come up with better names, I'll gladly use that. Otherwise I'll
> just use these.

I've changed my mind. I'm not using --index and --cached for "git
restore" (formerly "git restore-files"). So how about this?

git restore --from=<tree> <pathspec> will update both the index and worktree.

git restore --from=<tree> --keep-index <pathspec> will not update the index

git restore --from=<tree> --keep-worktree <pathspec> will not update worktree

And I'm thinking of adding these to "git reset" too. It will have the
forth form:

git reset [--keep-index] [--keep-worktree] [--keep-nothing] [<commit>]

which is similar to --soft, --mixed and --hard, but with better names:
--soft is --keep-worktree and --keep-index, --mixed is --keep-worktree
and --hard is --keep-nothing (i.e. the only change is HEAD).

Although, if I work it out, I might just ignore "git reset" and make
sure "git switch" and "git restore" can do whatever "git reset" can
then remove its "common command" status. This part is more thinking
out loud, but "git switch" can have a new form

git switch --reset-branch [--keep-worktree] [--keep-index] <start-point>

which resets "HEAD" (and switch of course) but do not enter detached
mode. This covers a common use case of "git reset [options] <commit>".
Then either "git restore" learns about "--abort-in-progress" to abort
cherry-pick/merge/revert, or those commands have a new --abort and
--quit option...
Junio C Hamano Jan. 31, 2019, 7:05 p.m. UTC | #8
Duy Nguyen <pclouds@gmail.com> writes:

> I've changed my mind. I'm not using --index and --cached for "git
> restore" (formerly "git restore-files"). So how about this?
>
> git restore --from=<tree> <pathspec> will update both the index and worktree.
>
> git restore --from=<tree> --keep-index <pathspec> will not update the index
>
> git restore --from=<tree> --keep-worktree <pathspec> will not update worktree

An action to "restore" with an option to "keep" (i.e. "do not
touch") smells strongly of double negation.  We are restoring,
i.e. grabbing something that existed in the past out of another
place (like tree or index) and depositing it to the working tree to
recover its previous state, oh, not but not touching the content of
the working tree (or the index) intact?

It would be great if you can come up with phrasing that avoids
specifying what is *not* done, but instructs the command what is to
be done, perhaps along the lines of "restore --index-only", "restore
--worktree-only" and "restore --index-and-worktree (which would be
the default)".
Duy Nguyen Feb. 1, 2019, 6:48 a.m. UTC | #9
On Fri, Feb 1, 2019 at 2:05 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Duy Nguyen <pclouds@gmail.com> writes:
>
> > I've changed my mind. I'm not using --index and --cached for "git
> > restore" (formerly "git restore-files"). So how about this?
> >
> > git restore --from=<tree> <pathspec> will update both the index and worktree.
> >
> > git restore --from=<tree> --keep-index <pathspec> will not update the index
> >
> > git restore --from=<tree> --keep-worktree <pathspec> will not update worktree
>
> An action to "restore" with an option to "keep" (i.e. "do not
> touch") smells strongly of double negation.  We are restoring,
> i.e. grabbing something that existed in the past out of another
> place (like tree or index) and depositing it to the working tree to
> recover its previous state, oh, not but not touching the content of
> the working tree (or the index) intact?

It is negation (though not double). The thinking was, since "git
restore" means restore everything, extra options will amend that to
"restore everything except ..." and because the parts to keep are more
important (i.e. data loss), highlighting it on the command sounds a
bit better to me: "ok i'm going to need to restore this thing, but I
want this part and that part to stay the same, let's type the
command..."

> It would be great if you can come up with phrasing that avoids
> specifying what is *not* done, but instructs the command what is to
> be done, perhaps along the lines of "restore --index-only", "restore
> --worktree-only" and "restore --index-and-worktree (which would be
> the default)".

My biggest problem with --*-only is that they cannot be combined.
There must be an option for every valid combination. I'm still
thinking about "git reset" where there are three parts to
reset/restore: HEAD, index and worktree. So at least there's another
valid combination "reset HEAD but not index nor worktree", which would
end becoming another option. This route does not look promising.

Of course we could just do --index and --worktree, each option
restores the respective part. Then it's combinable (and extensible in
the future). But then "git restore" means "git restore --index
--worktree" and typing "git restore --index" effectively removes the
default "--worktree", which seems a bit twisted.
Junio C Hamano Feb. 1, 2019, 5:57 p.m. UTC | #10
Duy Nguyen <pclouds@gmail.com> writes:

> Of course we could just do --index and --worktree, each option
> restores the respective part. Then it's combinable (and extensible in
> the future). But then "git restore" means "git restore --index
> --worktree" and typing "git restore --index" effectively removes the
> default "--worktree", which seems a bit twisted.

Or "git restore --no-worktree" (essentially, instead of saying
"keep", say "no" to mean "negation").

Incidentally, "git restore --no-index" does not have a counterpart
in "git checkout", but I think it is probably a good thing to add;
as it has to do far more than "git cat-file blob $tree:$path >$path"
these days.
Duy Nguyen Feb. 2, 2019, 10:57 a.m. UTC | #11
On Sat, Feb 2, 2019 at 12:57 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Duy Nguyen <pclouds@gmail.com> writes:
>
> > Of course we could just do --index and --worktree, each option
> > restores the respective part. Then it's combinable (and extensible in
> > the future). But then "git restore" means "git restore --index
> > --worktree" and typing "git restore --index" effectively removes the
> > default "--worktree", which seems a bit twisted.
>
> Or "git restore --no-worktree" (essentially, instead of saying
> "keep", say "no" to mean "negation").
>
> Incidentally, "git restore --no-index" does not have a counterpart
> in "git checkout", but I think it is probably a good thing to add;
> as it has to do far more than "git cat-file blob $tree:$path >$path"
> these days.

Oh yes, I occasionally need that too.
Duy Nguyen Feb. 19, 2019, 4:20 a.m. UTC | #12
On Sat, Feb 2, 2019 at 12:57 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Duy Nguyen <pclouds@gmail.com> writes:
>
> > Of course we could just do --index and --worktree, each option
> > restores the respective part. Then it's combinable (and extensible in
> > the future). But then "git restore" means "git restore --index
> > --worktree" and typing "git restore --index" effectively removes the
> > default "--worktree", which seems a bit twisted.
>
> Or "git restore --no-worktree" (essentially, instead of saying
> "keep", say "no" to mean "negation").
>
> Incidentally, "git restore --no-index" does not have a counterpart
> in "git checkout", but I think it is probably a good thing to add;
> as it has to do far more than "git cat-file blob $tree:$path >$path"
> these days.

OK this hopefully will be the final design

(git restore) "[--worktree] <paths>" restores worktree paths from index

"--index <paths>" restores the index from HEAD (aka "git reset")

"--source <tree> (--index|--worktree) <paths>" restores index or
worktree (or both) from <tree>

I'm a bit reluctant to support "git restore --index --worktree
<paths>" without --source, which should default to HEAD, since it's a
bit unclear/inconsistent ("git restore --worktree <paths>" defaults to
index as the source, but here we use a different default source).
Elijah Newren Feb. 19, 2019, 2:42 p.m. UTC | #13
Hi Duy,

On Mon, Feb 18, 2019 at 8:21 PM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Sat, Feb 2, 2019 at 12:57 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Duy Nguyen <pclouds@gmail.com> writes:
> >
> > > Of course we could just do --index and --worktree, each option
> > > restores the respective part. Then it's combinable (and extensible in
> > > the future). But then "git restore" means "git restore --index
> > > --worktree" and typing "git restore --index" effectively removes the
> > > default "--worktree", which seems a bit twisted.
> >
> > Or "git restore --no-worktree" (essentially, instead of saying
> > "keep", say "no" to mean "negation").
> >
> > Incidentally, "git restore --no-index" does not have a counterpart
> > in "git checkout", but I think it is probably a good thing to add;
> > as it has to do far more than "git cat-file blob $tree:$path >$path"
> > these days.
>
> OK this hopefully will be the final design
>
> (git restore) "[--worktree] <paths>" restores worktree paths from index
>
> "--index <paths>" restores the index from HEAD (aka "git reset")
>
> "--source <tree> (--index|--worktree) <paths>" restores index or
> worktree (or both) from <tree>
>
> I'm a bit reluctant to support "git restore --index --worktree
> <paths>" without --source, which should default to HEAD, since it's a
> bit unclear/inconsistent ("git restore --worktree <paths>" defaults to
> index as the source, but here we use a different default source).

Sorry for going missing a while from the conversation, and thanks so
much for pushing this forward.

Overall this looks good, but there's just one part that confuses me.
Here you seem to suggest that if you pass --source but neither --index
or --worktree that both the index and working tree will be written to.
Why are "restored" changes considered ready for commit?  That seems
confusing to me (and was one of the bugs of checkout, IMO).  See also
second half of https://public-inbox.org/git/xmqq1s6yezk3.fsf@gitster-ct.c.googlers.com/

Elijah
Duy Nguyen Feb. 19, 2019, 2:57 p.m. UTC | #14
On Tue, Feb 19, 2019 at 9:42 PM Elijah Newren <newren@gmail.com> wrote:
> > OK this hopefully will be the final design
> >
> > (git restore) "[--worktree] <paths>" restores worktree paths from index
> >
> > "--index <paths>" restores the index from HEAD (aka "git reset")
> >
> > "--source <tree> (--index|--worktree) <paths>" restores index or
> > worktree (or both) from <tree>
> >
> > I'm a bit reluctant to support "git restore --index --worktree
> > <paths>" without --source, which should default to HEAD, since it's a
> > bit unclear/inconsistent ("git restore --worktree <paths>" defaults to
> > index as the source, but here we use a different default source).
>
> Sorry for going missing a while from the conversation, and thanks so
> much for pushing this forward.
>
> Overall this looks good, but there's just one part that confuses me.
> Here you seem to suggest that if you pass --source but neither --index
> or --worktree that both the index and working tree will be written to.

No no. Sorry for not being clear. If neither --index or --worktree is
given, the default will be --worktree. --source changes the "source"
but cannot influence target selection. So "git restore --source=HEAD
foo.c" will restore foo.c on the worktree but not the index. I still
remember that point you and Stefan Xenos raised ;-)

Full git-restore.txt is here if you're interested. I think I'll send
it out soon once git-switch part more or less settles.

https://gitlab.com/pclouds/git/blob/switch-and-restore-forever/Documentation/git-restore.txt

> Why are "restored" changes considered ready for commit?  That seems
> confusing to me (and was one of the bugs of checkout, IMO).  See also
> second half of https://public-inbox.org/git/xmqq1s6yezk3.fsf@gitster-ct.c.googlers.com/
>
> Elijah
Junio C Hamano Feb. 19, 2019, 7:02 p.m. UTC | #15
Duy Nguyen <pclouds@gmail.com> writes:

> On Sat, Feb 2, 2019 at 12:57 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Duy Nguyen <pclouds@gmail.com> writes:
>>
>> > Of course we could just do --index and --worktree, each option
>> > restores the respective part. Then it's combinable (and extensible in
>> > the future). But then "git restore" means "git restore --index
>> > --worktree" and typing "git restore --index" effectively removes the
>> > default "--worktree", which seems a bit twisted.
>>
>> Or "git restore --no-worktree" (essentially, instead of saying
>> "keep", say "no" to mean "negation").
>>
>> Incidentally, "git restore --no-index" does not have a counterpart
>> in "git checkout", but I think it is probably a good thing to add;
>> as it has to do far more than "git cat-file blob $tree:$path >$path"
>> these days.
>
> OK this hopefully will be the final design
>
> (git restore) "[--worktree] <paths>" restores worktree paths from index
>
> "--index <paths>" restores the index from HEAD (aka "git reset")
>
> "--source <tree> (--index|--worktree) <paths>" restores index or
> worktree (or both) from <tree>
>
> I'm a bit reluctant to support "git restore --index --worktree
> <paths>" without --source, which should default to HEAD, since it's a
> bit unclear/inconsistent ("git restore --worktree <paths>" defaults to
> index as the source, but here we use a different default source).

Ok, so we grab things from the index by default, but with --source
<tree>, we can tell the command to grab things from the tree.  When
we are explicitly told to update the index with "--index", it would
be nonsense to grab things from the index, so "--source <tree>"
becomes required in that case.  Makes sense.

To summerize and full enumerate all the allowed variants:

 * --index --worktree --source <tree> <path>...

   Update both from a tree; --source <tree> is required.

 * --index --no-worktree --source <tree> <path>...

   Update only the index but not the working tree; --source <tree>
   is required.


 * --no-index --worktree <path>...

   Update only the working tree files without touching the index
   (new feature that cannot be done with the current Git, although
   "cat-file -p >path" may be close enough at times), from the index

 * --no-index --worktree --source <tree> <path>...

   Update only the working tree files without touching the index
   from a tree-ish.

 * --no-index --no-worktree <path>...

   Update nothing, which is a no-op that is not all that useful.

 * --no-index --no-worktree --source <tree> <path>...

   Update nothing, which is a no-op that is not all that useful.

I am getting the impression that to save typing, you would want to
make "--index --worktree" the default (i.e. among the above, only
--no-index and --no-worktree need to be spelled explicitly), but
there is one glitch.  Updating from the index must be spelled
explicitly with "--no-index --worktree".

So perhaps the defaulting rule for the "--index" option must become
a bit more tricky.  Perhaps the rules are:

 * --worktree is the defeault; --no-worktree can be given from the
   command line to countermand it, and --worktree can be given from
   the command line to be more explicit.

 * when --source <tree> is given from the command line, --index is
   the default, and --no-index can be given to countermand it.

 * when --source <tree> is not given from the command line,
   --no-index is the only sensible choice.  It can be given from the
   command line to be more explicit, but giving --index to
   countermand the --no-index default would be an error, as updating
   the index, whether the same update also goes to the working tree,
   must come from a --source <tree>.

Am I following you correctly?

Thanks.
Junio C Hamano Feb. 19, 2019, 7:07 p.m. UTC | #16
Elijah Newren <newren@gmail.com> writes:

> Overall this looks good, but there's just one part that confuses me.
> Here you seem to suggest that if you pass --source but neither --index
> or --worktree that both the index and working tree will be written to.
> Why are "restored" changes considered ready for commit?  That seems
> confusing to me (and was one of the bugs of checkout, IMO).  See also
> second half of https://public-inbox.org/git/xmqq1s6yezk3.fsf@gitster-ct.c.googlers.com/

As long as worktree-only mode does not lose track of a
previously-untracked path in the index (perhaps use the i-t-a bit),
I do not have a strong objection against making the worktree-only
mode the default.
Junio C Hamano Feb. 19, 2019, 7:10 p.m. UTC | #17
Junio C Hamano <gitster@pobox.com> writes:

> I am getting the impression that to save typing, you would want to
> make "--index --worktree" the default (i.e. among the above, only
> --no-index and --no-worktree need to be spelled explicitly), but
> there is one glitch.  Updating from the index must be spelled
> explicitly with "--no-index --worktree".

And after getting reminded by Elijah, the default pair is
<--no-index, --worktree>.

> So perhaps the defaulting rule for the "--index" option must become
> a bit more tricky.  Perhaps the rules are:
>
>  * --worktree is the default; --no-worktree can be given from the
>    command line to countermand it, and --worktree can be given from
>    the command line to be more explicit.
>
>  * when --source <tree> is given from the command line, --index is
>    the default, and --no-index can be given to countermand it.

Correction.

    * when --source <tree> is given, --no-index is the default, but
      --index can be given to countermand it.

>
>  * when --source <tree> is not given from the command line,
>    --no-index is the only sensible choice.  It can be given from the
>    command line to be more explicit, but giving --index to
>    countermand the --no-index default would be an error, as updating
>    the index, whether the same update also goes to the working tree,
>    must come from a --source <tree>.

This is still correct.
Elijah Newren Feb. 19, 2019, 10:04 p.m. UTC | #18
On Tue, Feb 19, 2019 at 11:10 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I am getting the impression that to save typing, you would want to
> > make "--index --worktree" the default (i.e. among the above, only
> > --no-index and --no-worktree need to be spelled explicitly), but
> > there is one glitch.  Updating from the index must be spelled
> > explicitly with "--no-index --worktree".
>
> And after getting reminded by Elijah, the default pair is
> <--no-index, --worktree>.

Why would you want --no-index or --no-worktree as flags?  That seems
to presume a default of modifying both the index and the working tree,
as these names imply undoing pieces of such a default.

I'd rather have a flag like --worktree which alone only modifies the
working tree and is presumed to be the default (but useful to be
explicit or as mentioned later), have a flag for applying the changes
to the index instead (--index?), and treat applying to both the
working tree and the index as unusual and require either both flags
(--worktree --index ?) or some special flag that likely has a longer
name (--worktree-and-index?).

I _think_ Duy does the latter reading over his manpage that he linked
to, but maybe I'm just reading my own biases into it.
Elijah Newren Feb. 19, 2019, 10:13 p.m. UTC | #19
On Tue, Feb 19, 2019 at 11:03 AM Junio C Hamano <gitster@pobox.com> wrote:
>  * --no-index --worktree <path>...
>
>    Update only the working tree files without touching the index
>    (new feature that cannot be done with the current Git, although
>    "cat-file -p >path" may be close enough at times), from the index

I worry slightly that wording like this might paint ourselves into a
similar corner of only correctly handling files as paths, rather than
also handling directories as paths, and/or which presumes the
no-overlay mode that I find problematic.  Maybe your "at times" just
meant "when dealing with a path that is a file that existed in the
given source", but I wonder if you are pushing more than that?
Elijah Newren Feb. 19, 2019, 10:24 p.m. UTC | #20
On Tue, Feb 19, 2019 at 11:07 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > Overall this looks good, but there's just one part that confuses me.
> > Here you seem to suggest that if you pass --source but neither --index
> > or --worktree that both the index and working tree will be written to.
> > Why are "restored" changes considered ready for commit?  That seems
> > confusing to me (and was one of the bugs of checkout, IMO).  See also
> > second half of https://public-inbox.org/git/xmqq1s6yezk3.fsf@gitster-ct.c.googlers.com/
>
> As long as worktree-only mode does not lose track of a
> previously-untracked path in the index (perhaps use the i-t-a bit),
> I do not have a strong objection against making the worktree-only
> mode the default.

Could you unpack that for me a bit?  My assumption is that
worktree-only mode doesn't touch the index (other than maybe reading
from it), and treating the index as read-only means by definition it
can't lose anything from there -- but then you mentioned using the
intent-to-add bit, and I feel like I'm missing an important puzzle
piece somewhere.  Trying to make sense of it, I'm wondering if you are
objecting to using overlay mode in general, or are trying to connect
this to the new "precious" concept being advanced, or if there's
something else you are considering here.
Junio C Hamano Feb. 19, 2019, 10:29 p.m. UTC | #21
Elijah Newren <newren@gmail.com> writes:

> On Tue, Feb 19, 2019 at 11:10 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > I am getting the impression that to save typing, you would want to
>> > make "--index --worktree" the default (i.e. among the above, only
>> > --no-index and --no-worktree need to be spelled explicitly), but
>> > there is one glitch.  Updating from the index must be spelled
>> > explicitly with "--no-index --worktree".
>>
>> And after getting reminded by Elijah, the default pair is
>> <--no-index, --worktree>.
>
> Why would you want --no-index or --no-worktree as flags?  That seems
> to presume a default of modifying both the index and the working tree,
> as these names imply undoing pieces of such a default.

By "flags" I think you mean "treat them as two orthogonal booleans"?

It was just how I read Duy's examples (especially the "both --index
and --worktree given" example where "--source <tree>" becomes
mandatory).  I do not have strong preference either way myself, but
I tend to think that treating these as two independent booleans
would be a way to make it clear that the new design departs from
what we have been doing (i.e. "--index" means "both", "--cached"
means "index only" and if we were to introduce the "cat-file -p >"
variant that would be called "--worktree-only"; in these, there is
no "two orthogonal booleans" that can be mixed---instead they come
premixed).

> I'd rather have a flag like --worktree which alone only modifies the
> working tree and is presumed to be the default (but useful to be
> explicit or as mentioned later), have a flag for applying the changes
> to the index instead (--index?), and treat applying to both the
> working tree and the index as unusual and require either both flags
> (--worktree --index ?) or some special flag that likely has a longer
> name (--worktree-and-index?).

So you are in favor of pre-mixed set of options, all are mutually
exclusive.  Which I am perfectly fine with.


I however do think "both worktree and index" is quite common when
tweaking the tree to prepare for the next commit.  A path with
contents freshly taken out of an existing tree may not match exactly
what you plan to record in your next commit, and you would not be
recording it immediately in a commit as-is.  But having the contents
taken from an existing tree recorded as the baseline in the index
would make "git diff (no tree-ish) <path>" a handy tool to review
your progress toward the next commit since that baseline state, in
addition to "git diff HEAD <path>" that is the usual "how does the
overall change relative to the parent of the commit I am preparing
for look like".

So I'd hesitate to endorse "a deliberately longer and harder to type
option" to populate both the index and the working tree files at the
same time.
Junio C Hamano Feb. 19, 2019, 10:33 p.m. UTC | #22
Elijah Newren <newren@gmail.com> writes:

> On Tue, Feb 19, 2019 at 11:03 AM Junio C Hamano <gitster@pobox.com> wrote:
>>  * --no-index --worktree <path>...
>>
>>    Update only the working tree files without touching the index
>>    (new feature that cannot be done with the current Git, although
>>    "cat-file -p >path" may be close enough at times), from the index
>
> I worry slightly that wording like this might paint ourselves into a
> similar corner of only correctly handling files as paths, rather than
> also handling directories as paths, and/or which presumes the
> no-overlay mode that I find problematic.  Maybe your "at times" just
> meant "when dealing with a path that is a file that existed in the
> given source", but I wonder if you are pushing more than that?

No, I meant "when you are not doing smudge filters (including eol
conversion)", aka "'cat-file -p' gives you raw blob contents, which
may not be what you want in your worktree."
Junio C Hamano Feb. 19, 2019, 10:36 p.m. UTC | #23
Elijah Newren <newren@gmail.com> writes:

>> As long as worktree-only mode does not lose track of a
>> previously-untracked path in the index (perhaps use the i-t-a bit),
>> I do not have a strong objection against making the worktree-only
>> mode the default.
>
> Could you unpack that for me a bit?

Suppose in an ancient version v1.0 there was a file called
README.txt but these days such a file does not exist (there may be
README.md added though).  By default, the command does not stuff the
contents to the index out of the tree and instead operate only on
the working tree.

  $ git restore-path --source v1.0 README.txt

At this point, you are assuming that README.txt will become
untracked and the user needs to manually add it.  I was asking if it
makes sense to at least make the index "aware of" it with I-T-A bit,
and I am leaning towards answering "yes" to that question.
Elijah Newren Feb. 19, 2019, 11 p.m. UTC | #24
On Tue, Feb 19, 2019 at 2:36 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> >> As long as worktree-only mode does not lose track of a
> >> previously-untracked path in the index (perhaps use the i-t-a bit),
> >> I do not have a strong objection against making the worktree-only
> >> mode the default.
> >
> > Could you unpack that for me a bit?
>
> Suppose in an ancient version v1.0 there was a file called
> README.txt but these days such a file does not exist (there may be
> README.md added though).  By default, the command does not stuff the
> contents to the index out of the tree and instead operate only on
> the working tree.
>
>   $ git restore-path --source v1.0 README.txt
>
> At this point, you are assuming that README.txt will become
> untracked and the user needs to manually add it.  I was asking if it
> makes sense to at least make the index "aware of" it with I-T-A bit,
> and I am leaning towards answering "yes" to that question.

Ah, thanks for all the clarifications (for this and the other emails).
Your suggestion here would mean that --worktree alone wouldn't
actually treat the index as read-only, but I too am leaning towards
the thought that it makes sense to set the i-t-a bit for such cases.
Duy Nguyen Feb. 20, 2019, 1:53 a.m. UTC | #25
On Wed, Feb 20, 2019 at 5:36 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> >> As long as worktree-only mode does not lose track of a
> >> previously-untracked path in the index (perhaps use the i-t-a bit),
> >> I do not have a strong objection against making the worktree-only
> >> mode the default.
> >
> > Could you unpack that for me a bit?
>
> Suppose in an ancient version v1.0 there was a file called
> README.txt but these days such a file does not exist (there may be
> README.md added though).  By default, the command does not stuff the
> contents to the index out of the tree and instead operate only on
> the working tree.
>
>   $ git restore-path --source v1.0 README.txt
>
> At this point, you are assuming that README.txt will become
> untracked and the user needs to manually add it.  I was asking if it
> makes sense to at least make the index "aware of" it with I-T-A bit,
> and I am leaning towards answering "yes" to that question.

I completely forgot about i-t-a! Do we want the command to add i-t-a
bit by default, or only when --intent-to-add is specified? So far
i-t-a has been a conscious choice, both "git reset <commit>" and "git
add" require --intent-to-add to use it. The first safe step could be
require --intent-to-add even in git-restore, then we could introduce a
config key that enables --intent-to-add in both "git restore" and "git
reset" (and perhaps "git apply" later, I still need to finish that
part).
Duy Nguyen Feb. 20, 2019, 2:19 a.m. UTC | #26
On Wed, Feb 20, 2019 at 2:10 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I am getting the impression that to save typing, you would want to
> > make "--index --worktree" the default (i.e. among the above, only
> > --no-index and --no-worktree need to be spelled explicitly), but
> > there is one glitch.  Updating from the index must be spelled
> > explicitly with "--no-index --worktree".
>
> And after getting reminded by Elijah, the default pair is
> <--no-index, --worktree>.
>
> > So perhaps the defaulting rule for the "--index" option must become
> > a bit more tricky.  Perhaps the rules are:
> >
> >  * --worktree is the default; --no-worktree can be given from the
> >    command line to countermand it, and --worktree can be given from
> >    the command line to be more explicit.

I originally went with that (--no-worktree to negate default
settings), but after updating docs to use "git restore" it's just too
much to write. My current rules are

 - default is --worktree
 - as soon as any of --[no-]worktree or --[no-]index is specified, the
above line is void. There is no default, what you specify is what you
get.

While it's a bit more twisted than spelling out --no-worktree to
countermand the default, i think it's more natural to think and write
it: ok i'm going to need to restore this and that so I write --this
and --that. Going with --no-worktree you need to think "i need to
write this and that and counter the default which is that other
thing". We essentially have two modes, the default mode and explicit
mode. Too bad?

> >  * when --source <tree> is given from the command line, --index is
> >    the default, and --no-index can be given to countermand it.
>
> Correction.
>
>     * when --source <tree> is given, --no-index is the default, but
>       --index can be given to countermand it.
>
> >
> >  * when --source <tree> is not given from the command line,
> >    --no-index is the only sensible choice.  It can be given from the
> >    command line to be more explicit, but giving --index to
> >    countermand the --no-index default would be an error, as updating
> >    the index, whether the same update also goes to the working tree,
> >    must come from a --source <tree>.
>
> This is still correct.

So I guess I scratch the default source for "--index --no-worktree"
(or just --index in my rules above) then? It does make sense to
default restoring the index from HEAD. And it makes "git status"
suggestion to unstage a bit shorter ("git restore --index <paths>"
instead of "git restore --source=HEAD --index <paths>")
Duy Nguyen Feb. 20, 2019, 2:32 a.m. UTC | #27
On Wed, Feb 20, 2019 at 5:29 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > On Tue, Feb 19, 2019 at 11:10 AM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> Junio C Hamano <gitster@pobox.com> writes:
> >>
> >> > I am getting the impression that to save typing, you would want to
> >> > make "--index --worktree" the default (i.e. among the above, only
> >> > --no-index and --no-worktree need to be spelled explicitly), but
> >> > there is one glitch.  Updating from the index must be spelled
> >> > explicitly with "--no-index --worktree".
> >>
> >> And after getting reminded by Elijah, the default pair is
> >> <--no-index, --worktree>.
> >
> > Why would you want --no-index or --no-worktree as flags?  That seems
> > to presume a default of modifying both the index and the working tree,
> > as these names imply undoing pieces of such a default.
>
> By "flags" I think you mean "treat them as two orthogonal booleans"?
>
> It was just how I read Duy's examples (especially the "both --index
> and --worktree given" example where "--source <tree>" becomes
> mandatory).  I do not have strong preference either way myself, but
> I tend to think that treating these as two independent booleans
> would be a way to make it clear that the new design departs from
> what we have been doing (i.e. "--index" means "both", "--cached"
> means "index only" and if we were to introduce the "cat-file -p >"
> variant that would be called "--worktree-only"; in these, there is
> no "two orthogonal booleans" that can be mixed---instead they come
> premixed).

There is indeed inconsistency in "git status" advice with my patches,
where it suggests both "git restore --index" and "git rm --cached",
both operate only in the index but with different option name.

"--index --no-worktree" is one way to deal with this. Another way may
be avoid --index in git-restore and use another name like
--staging-area or something? That avoids the name clash, and we could
even add --worktree and --staging-area to "git rm" / "git apply"
later, deprecating (but still supporting) --index and --cached there.
Duy Nguyen Feb. 20, 2019, 3:52 a.m. UTC | #28
On Wed, Feb 20, 2019 at 5:05 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Tue, Feb 19, 2019 at 11:10 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> > > I am getting the impression that to save typing, you would want to
> > > make "--index --worktree" the default (i.e. among the above, only
> > > --no-index and --no-worktree need to be spelled explicitly), but
> > > there is one glitch.  Updating from the index must be spelled
> > > explicitly with "--no-index --worktree".
> >
> > And after getting reminded by Elijah, the default pair is
> > <--no-index, --worktree>.
>
> Why would you want --no-index or --no-worktree as flags?  That seems
> to presume a default of modifying both the index and the working tree,
> as these names imply undoing pieces of such a default.
>
> I'd rather have a flag like --worktree which alone only modifies the
> working tree and is presumed to be the default (but useful to be
> explicit or as mentioned later), have a flag for applying the changes
> to the index instead (--index?), and treat applying to both the
> working tree and the index as unusual and require either both flags
> (--worktree --index ?) or some special flag that likely has a longer
> name (--worktree-and-index?).

I'd prefer separate options. I even gave --worktree and --index
shortcuts so we could write "git restore -IW" if "git restore --index
--worktree" is used often (and I think it could be an alternative for
"git reset --hard HEAD" if --force is also specified)

> I _think_ Duy does the latter reading over his manpage that he linked
> to, but maybe I'm just reading my own biases into it.

Nah you read it right. The "examples" section also shows it.

Patch
diff mbox series

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0aef35bbc4..6ba85e9de5 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -45,6 +45,7 @@  struct checkout_opts {
 	int ignore_other_worktrees;
 	int show_progress;
 	int overlay_mode;
+	int cached;
 	/*
 	 * If new checkout options are added, skip_merge_working_tree
 	 * should be updated accordingly.
@@ -288,6 +289,10 @@  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 && opts->cached)
+		return run_add_interactive(revision, "--patch=reset",
+					   &opts->pathspec);
+
 	if (opts->patch_mode)
 		return run_add_interactive(revision, "--patch=checkout",
 					   &opts->pathspec);
@@ -319,7 +324,9 @@  static int checkout_paths(const struct checkout_opts *opts,
 				 * the current index, which means that it should 
 				 * be removed.
 				 */
-				ce->ce_flags |= CE_MATCHED | CE_REMOVE | CE_WT_REMOVE;
+				ce->ce_flags |= CE_MATCHED | CE_REMOVE;
+				if (!opts->cached)
+					ce->ce_flags |= CE_WT_REMOVE;
 				continue;
 			} else {
 				/*
@@ -392,6 +399,9 @@  static int checkout_paths(const struct checkout_opts *opts,
 	for (pos = 0; pos < active_nr; pos++) {
 		struct cache_entry *ce = active_cache[pos];
 		if (ce->ce_flags & CE_MATCHED) {
+			if (opts->cached) {
+				continue;
+			}
 			if (!ce_stage(ce)) {
 				errs |= checkout_entry(ce, &state, NULL);
 				continue;
@@ -571,6 +581,11 @@  static int skip_merge_working_tree(const struct checkout_opts *opts,
 	 * not tested here
 	 */
 
+	/*
+	 * opts->cached cannot be used with switching branches so is
+	 * not tested here
+	 */
+
 	/*
 	 * If we aren't creating a new branch any changes or updates will
 	 * happen in the existing branch.  Since that could only be updating
@@ -1207,9 +1222,13 @@  static int checkout_branch(struct checkout_opts *opts,
 		die(_("'%s' cannot be used with switching branches"),
 		    "--patch");
 
-	if (!opts->overlay_mode)
+	if (opts->overlay_mode != -1)
+		die(_("'%s' cannot be used with switching branches"),
+		    "--overlay/--no-overlay");
+
+	if (opts->cached)
 		die(_("'%s' cannot be used with switching branches"),
-		    "--no-overlay");
+		    "--cached");
 
 	if (opts->writeout_stage)
 		die(_("'%s' cannot be used with switching branches"),
@@ -1300,6 +1319,7 @@  int cmd_checkout(int argc, const char **argv, const char *prefix)
 			    PARSE_OPT_OPTARG, option_parse_recurse_submodules_worktree_updater },
 		OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")),
 		OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode")),
+		OPT_BOOL(0, "cached", &opts.cached, N_("work on the index only")),
 		OPT_END(),
 	};
 
diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index 47aeb0b167..e8774046e0 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh
@@ -108,6 +108,14 @@  test_expect_success PERL 'path limiting works: foo inside dir' '
 	verify_state dir/foo head head
 '
 
+test_expect_success PERL 'git checkout --cached -p' '
+	set_and_save_state dir/foo work work &&
+	test_write_lines n y | git checkout --cached -p >output &&
+	verify_state dir/foo work head &&
+	verify_saved_state bar &&
+	test_i18ngrep "Unstage" output
+'
+
 test_expect_success PERL 'none of this moved HEAD' '
 	verify_saved_head
 '
diff --git a/t/t2026-checkout-cached.sh b/t/t2026-checkout-cached.sh
new file mode 100755
index 0000000000..1b66192727
--- /dev/null
+++ b/t/t2026-checkout-cached.sh
@@ -0,0 +1,103 @@ 
+#!/bin/sh
+
+test_description='checkout --cached <pathspec>'
+
+. ./test-lib.sh
+
+test_expect_success 'checkout --cached <pathspec>' '
+	echo 1 >file1 &&
+	echo 2 >file2 &&
+	git add file1 file2 &&
+	test_tick &&
+	git commit -m files &&
+	git rm file2 &&
+	echo 3 >file3 &&
+	echo 4 >file1 &&
+	git add file1 file3 &&
+	git checkout --cached HEAD -- file1 file2 &&
+	test_must_fail git diff --quiet &&
+
+	cat >expect <<-\EOF &&
+	diff --git a/file1 b/file1
+	index d00491f..b8626c4 100644
+	--- a/file1
+	+++ b/file1
+	@@ -1 +1 @@
+	-1
+	+4
+	diff --git a/file2 b/file2
+	deleted file mode 100644
+	index 0cfbf08..0000000
+	--- a/file2
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-2
+	EOF
+	git diff >actual &&
+	test_cmp expect actual &&
+
+	cat >expect <<-\EOF &&
+	diff --git a/file3 b/file3
+	new file mode 100644
+	index 0000000..00750ed
+	--- /dev/null
+	+++ b/file3
+	@@ -0,0 +1 @@
+	+3
+	EOF
+	git diff --cached >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'checking out an unmodified path is a no-op' '
+	git reset --hard &&
+	git checkout --cached HEAD -- file1 &&
+	git diff-files --exit-code &&
+	git diff-index --cached --exit-code HEAD
+'
+
+test_expect_success 'checking out specific path that is unmerged' '
+	test_commit file3 file3 &&
+	git rm --cached file2 &&
+	echo 1234 >file2 &&
+	F1=$(git rev-parse HEAD:file1) &&
+	F2=$(git rev-parse HEAD:file2) &&
+	F3=$(git rev-parse HEAD:file3) &&
+	{
+		echo "100644 $F1 1	file2" &&
+		echo "100644 $F2 2	file2" &&
+		echo "100644 $F3 3	file2"
+	} | git update-index --index-info &&
+	git ls-files -u &&
+	git checkout --cached HEAD file2 &&
+	test_must_fail git diff --quiet &&
+	git diff-index --exit-code --cached HEAD
+'
+
+test_expect_success '--cached without --no-overlay does not remove entry from index' '
+	test_must_fail git checkout --cached HEAD^ file3 &&
+	git ls-files --error-unmatch -- file3
+'
+
+test_expect_success 'file is removed from the index with --no-overlay' '
+	git checkout --cached --no-overlay HEAD^ file3 &&
+	test_path_is_file file3 &&
+	test_must_fail git ls-files --error-unmatch -- file3
+'
+
+test_expect_success 'test checkout --cached --no-overlay at given paths' '
+	mkdir sub &&
+	>sub/file1 &&
+	>sub/file2 &&
+	git update-index --add sub/file1 sub/file2 &&
+	T=$(git write-tree) &&
+	git checkout --cached --no-overlay HEAD sub/file2 &&
+	test_must_fail git diff --quiet &&
+	U=$(git write-tree) &&
+	echo "$T" &&
+	echo "$U" &&
+	test_must_fail git diff-index --cached --exit-code "$T" &&
+	test "$T" != "$U"
+'
+
+test_done
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a3fd9a9630..cbc304ace8 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1437,6 +1437,7 @@  test_expect_success 'double dash "git checkout"' '
 	--no-quiet Z
 	--no-... Z
 	--overlay Z
+	--cached Z
 	EOF
 '