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

From: Elijah Newren <newren@gmail.com>
Date: Mar 9, 2019, 18:52 UTC
Message-ID: <CABPp-BFv-a3Uw1g+ebLqTHRbCedsv1akZxxJ7QfeyXtXBdQuOw@mail.gmail.com>
In-Reply-To: <20190308101655.9767-7-pclouds@gmail.com>

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

Show 25 quoted lines

>
> '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");

:-(

Show 7 quoted lines

> +               /*
> +                * 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?