Re: [PATCH v1 01/11] checkout: split part of it to new command 'restore'

[Elijah Newren <newren@gmail.com>]

Thanks for working on this; overall looks really good.  I've got a few
comments here and there on the wording and combinations of options...

On Fri, Mar 8, 2019 at 2:17 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> +SYNOPSIS

It might be worth adding some words somewhere to differentiate between
`reset` and `restore`.  E.g.

`git restore` modifies the working tree (and maybe index) to change
file content to match some other (usually older) version, but does not
update your branch.  `git reset` is about modifying which commit your
branch points to, meaning possibly removing and/or adding many commits
to your branch.

It may also make sense to add whatever description you use to the reset manpage.

> +--------
> +[verse]
> +'git restore' [<options>] [--source=<revision>] <pathspec>...
> +'git restore' (-p|--patch) [--source=<revision>] [<pathspec>...]

So one cannot specify any special options with -p?  Does that mean one
cannot use it with --index (i.e. this command cannot replace 'git
reset -p')?  Or is this an oversight in the synopsis?

> +DESCRIPTION
> +-----------
> +Restore paths in the working tree by replacing with the contents in
> +the restore source or remove if the paths do not exist in the restore
> +source. The source is by default the index but could be from a commit.
> +The command can also optionally restore content in the index from a
> +commit.

The first sentence makes it sound like two different operations, when
I think it is one. Perhaps:

Restore paths in the working tree by replacing with the contents in
the restore source (and if the restore source is missing paths found
in the working tree, that means deleting those paths from the working
tree).

> +
> +When a `<revision>` is given, the paths that match the `<pathspec>` are
> +updated both in the index and in the working tree.

I thought the default was --worktree.  Is this sentence from an older
version of your patch series that you forgot to update?

> +
> +OPTIONS
> +-------
> +-s<tree>::
> +--source=<tree>::
> +       Restore the working tree files with the content from the given
> +       tree or any revision that leads to a tree (e.g. a commit or a
> +       branch).

I think that's a little hard to parse.  We may not be able to avoid
"working tree" vs. "tree" confusion, but the spelling of <tree> feels
like it should be a separate sentence.  Maybe:

Restore the working tree files with the content from the given tree.
It is common to specify the source tree by naming a commit, branch, or
tag.

?

> +
> +-p::
> +--patch::
> +       Interactively select hunks in the difference between the
> +       `<revision>` (or the index, if unspecified) and the working
> +       tree. See the ``Interactive Mode'' section of linkgit:git-add[1]
> +       to learn how to operate the `--patch` mode.
> +
> +-W::
> +--worktree::
> +-I::
> +--index::
> +       Specify the restore location. If neither option is specified,
> +       by default the working tree is restored. If `--index` is
> +       specified without `--worktree` or `--source`, `--source=HEAD`
> +       is implied. These options only make sense to use with
> +       `--source`.

Seems like this contains a minor contradiction.  Perhaps start the
final sentence with: "Otherwise, ..." ?

> +-q::
> +--quiet::
> +       Quiet, suppress feedback messages.
> +
> +--progress::
> +--no-progress::
> +       Progress status is reported on the standard error stream
> +       by default when it is attached to a terminal, unless `--quiet`
> +       is specified. This flag enables progress reporting even if not
> +       attached to a terminal, regardless of `--quiet`.

I'm assuming this means there are feedback messages other than
progress feedback?

> +-f::
> +--force::
> +       If `--source` is not specified, unmerged entries are left alone
> +       and will not fail the operation. Unmerged entries are always
> +       replaced if `--source` is specified, regardless of `--force`.

This may be slightly confusing, in particular it suggests that --index
(or --worktree and --index) are the default.  Is --force only useful
when --index is specified?  If it has utility with --worktree only,
what does it do?  Also, what happens when there are unmerged entries
in the index and someone tries to restore just working tree files --
are the ones corresponding to unmerged entries skipped (if so,
silently or with warnings printed for the user?), or does something
else happen?

> +--ours::
> +--theirs::
> +       Check out stage #2 ('ours') or #3 ('theirs') for unmerged
> +       paths.
> ++
> +Note that during `git rebase` and `git pull --rebase`, 'ours' and
> +'theirs' may appear swapped. See the explanation of the same options
> +in linkgit:git-checkout[1] for details.

So sad, but yes you need to mention it.  I'm curious what we say for
cherry-pick; it's not clear to me whether people think of the current
branch as "ours" or the commit they wrote themselves and are trying to
bring to this branch as "ours".  There are probably examples of both.

Not that I think you can do anything about that here or need to change
your description.  I'm just very sad about --ours and --theirs in
general.

> +
> +-m::
> +--merge::
> +       Recreate the conflicted merge in the specified paths.
> +
> +--conflict=<style>::
> +       The same as `--merge` option above, but changes the way the
> +       conflicting hunks are presented, overriding the merge.conflictStyle
> +       configuration variable.  Possible values are "merge" (default)
> +       and "diff3" (in addition to what is shown by "merge" style,
> +       shows the original contents).

Should you mention that these are incompatible with --source and
--index?  And perhaps also make sure the code aborts if either of
these options are combined with either of those?

> +--ignore-skip-worktree-bits::
> +       In sparse checkout mode, by default update only entries
> +       matched by `<pathspec>` and sparse patterns in
> +       $GIT_DIR/info/sparse-checkout. This option ignores the sparse
> +       patterns and unconditionally restores any files in `<pathspec>`.

The first sentence is slightly confusing; it sounds like you are
saying what the flag does rather than what the default is without the
flag.  Perhaps:

"In sparse checkout mode, the default is to only update entries
matched by `<pathspec>` and sparse patterns in
$GIT_DIR/info/sparse-checkout...."

> +
> +--recurse-submodules::
> +--no-recurse-submodules::
> +       Using `--recurse-submodules` will update the content of all initialized
> +       submodules according to the commit recorded in the superproject. If
> +       local modifications in a submodule would be overwritten the checkout
> +       will fail unless `-f` is used.

This suggests that your documentation for -f/--force is incomplete.

> +       If nothing (or `--no-recurse-submodules`)
> +       is used, the work trees of submodules will not be updated.

This seems slightly awkward.  Perhaps "If `--no-recurse-submodules`
(which is the default) is used, the work trees of submodules will not
be updated.

> +       Just like linkgit:git-submodule[1], this will detach the
> +       submodules HEAD.
> +
> +--overlay::
> +--no-overlay::
> +       In overlay mode, `git checkout` never removes files from the

Why are you talking about `git checkout` here?  Shouldn't this be `git restore`?

> +       index or the working tree. In no-overlay mode, files that
> +       appear in the index and working tree, but not in `--source` tree
> +       are removed, to make them match `<tree-ish>` exactly. The
> +       default is no-overlay mode.

A minor comment: This seems slightly weird because it feels like you
start out with a negative ("never removes") and then describe the
opposite option which adds another negation to the negative.  But the
wording avoids an explicit double negative (which is good), and I
don't have a better suggestion for wording here.  Just thought I'd
flag it in case anyone else reading over this notices it too and has a
wording suggestion.

> +
> +EXAMPLES
> +--------
> +
> +The following sequence checks out the `master` branch, reverts
> +the `Makefile` to two revisions back, deletes hello.c by
> +mistake, and gets it back from the index.
> +
> +------------
> +$ git switch master
> +$ git restore --source master~2 Makefile  <1>
> +$ rm -f hello.c
> +$ git restore hello.c                   <2>
> +------------
> +
> +<1> take a file out of another commit
> +<2> restore hello.c from the index
> +
> +If you want to check out _all_ C source files out of the index,
> +you can say
> +
> +------------
> +$ git restore '*.c'
> +------------
> +
> +Note the quotes around `*.c`.  The file `hello.c` will also be
> +checked out, even though it is no longer in the working tree,
> +because the file globbing is used to match entries in the index
> +(not in the working tree by the shell).
> +
> +To restore all files in the current directory
> +
> +------------
> +$ git restore .
> +------------
> +
> +or to restore all working tree files with 'top' pathspec magic (see
> +linkgit::gitglossary[7])
> +
> +------------
> +$ git restore :/
> +------------
> +
> +To restore a file in the index only (this is the same as using
> +"git reset")
> +
> +------------
> +$ git restore --index hello.c
> +------------
> +
> +or you can restore both the index and the working tree
> +
> +------------
> +$ git restore --source=HEAD --index --worktree hello.c
> +------------
> +
> +SEE ALSO
> +--------
> +linkgit:git-checkout[1]
> +
> +GIT
> +---
> +Part of the linkgit:git[1] suite
> diff --git a/Makefile b/Makefile
> index 8e91db73ad..ffe7e4f58f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -799,6 +799,7 @@ BUILT_INS += git-format-patch$X
>  BUILT_INS += git-fsck-objects$X
>  BUILT_INS += git-init$X
>  BUILT_INS += git-merge-subtree$X
> +BUILT_INS += git-restore$X
>  BUILT_INS += git-show$X
>  BUILT_INS += git-stage$X
>  BUILT_INS += git-status$X
> diff --git a/builtin.h b/builtin.h
> index c64e44450e..6830000e14 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -214,6 +214,7 @@ extern int cmd_remote_fd(int argc, const char **argv, const char *prefix);
>  extern int cmd_repack(int argc, const char **argv, const char *prefix);
>  extern int cmd_rerere(int argc, const char **argv, const char *prefix);
>  extern int cmd_reset(int argc, const char **argv, const char *prefix);
> +extern int cmd_restore(int argc, const char **argv, const char *prefix);
>  extern int cmd_rev_list(int argc, const char **argv, const char *prefix);
>  extern int cmd_rev_parse(int argc, const char **argv, const char *prefix);
>  extern int cmd_revert(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 4903359b49..11dd2ae44c 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -37,6 +37,11 @@ static const char * const switch_branch_usage[] = {
>         NULL,
>  };
>
> +static const char * const restore_files_usage[] = {
> +       N_("git restore [<options>] [<branch>] -- <file>..."),
> +       NULL,
> +};
> +
>  struct checkout_opts {
>         int patch_mode;
>         int quiet;
> @@ -1528,3 +1533,24 @@ int cmd_switch(int argc, const char **argv, const char *prefix)
>         FREE_AND_NULL(options);
>         return ret;
>  }
> +
> +int cmd_restore(int argc, const char **argv, const char *prefix)
> +{
> +       struct checkout_opts opts;
> +       struct option *options = NULL;
> +       int ret;
> +
> +       memset(&opts, 0, sizeof(opts));
> +       opts.dwim_new_local_branch = 1;
> +       opts.switch_branch_doing_nothing_is_ok = 0;
> +       opts.accept_pathspec = 1;
> +
> +       options = parse_options_dup(options);
> +       options = add_common_options(&opts, options);
> +       options = add_checkout_path_options(&opts, options);
> +
> +       ret = checkout_main(argc, argv, prefix, &opts,
> +                           options, restore_files_usage);
> +       FREE_AND_NULL(options);
> +       return ret;
> +}
> diff --git a/command-list.txt b/command-list.txt
> index 13317f47d4..b9eae1c258 100644
> --- a/command-list.txt
> +++ b/command-list.txt
> @@ -151,6 +151,7 @@ git-replace                             ancillarymanipulators           complete
>  git-request-pull                        foreignscminterface             complete
>  git-rerere                              ancillaryinterrogators
>  git-reset                               mainporcelain           worktree
> +git-restore                             mainporcelain           worktree
>  git-revert                              mainporcelain
>  git-rev-list                            plumbinginterrogators
>  git-rev-parse                           plumbinginterrogators
> diff --git a/git.c b/git.c
> index 39582cf511..6d439e723f 100644
> --- a/git.c
> +++ b/git.c
> @@ -558,6 +558,7 @@ static struct cmd_struct commands[] = {
>         { "replace", cmd_replace, RUN_SETUP },
>         { "rerere", cmd_rerere, RUN_SETUP },
>         { "reset", cmd_reset, RUN_SETUP },
> +       { "restore", cmd_restore, RUN_SETUP | NEED_WORK_TREE },
>         { "rev-list", cmd_rev_list, RUN_SETUP | NO_PARSEOPT },
>         { "rev-parse", cmd_rev_parse, NO_PARSEOPT },
>         { "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE },
> --
> 2.21.0.rc1.337.gdf7f8d0522
>