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

From: Elijah Newren <newren@gmail.com>
Date: Mar 9, 2019, 18:27 UTC
Message-ID: <CABPp-BENtDUOpKYHZu=4rKFrQMaCE=fEtAWRqHvePjZYm1jTtg@mail.gmail.com>
In-Reply-To: <CACsJy8D4tvm_zLo0DcnjmcBeKDRDR+HGAxd1PsUMSBcgR59DPg@mail.gmail.com>

On Sat, Mar 9, 2019 at 4:16 AM Duy Nguyen <pclouds@gmail.com> wrote:

> On Sat, Mar 9, 2019 at 1:01 AM Elijah Newren <newren@gmail.com> wrote:

Show 16 quoted lines

> > > +-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?
>
> There could be. This is carried over from git-checkout. I suspect this
> is about warnings that we print from time to time.

Why would --quiet squelch warnings? I figured it'd only squelch feedback, informational, or progress messages.

I understand you just carried it over from git-checkout, but as worded it makes me wonder if checkout has suboptimal behavior or perhaps just a suboptimal explanation of its flags ... and if it does, we probably don't want to carry that over.

Show 13 quoted lines

> > > +-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?
>
> Well, this is 'git checkout -f' behavior which only concerns the
> index. So yeah it only matters with --index.

Okay, good to know that this only matters with --index. However, new problem: This makes the explanation feel contradictory, though, because elsewhere you stated that --source=HEAD is implied when --index is given without a source. So, the combination of this description and that fact suggests that -f is either useless (--index is not specified) or ignored (because --source will either default to HEAD or be specified by the user). Maybe that's true and -f should be removed from this new document. If it has actual utility in some cases, then this description needs to be reworked to explain what those circiumstances are somehow.

Show 8 quoted lines

> > 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?
>
> If -m is also specified, then we recreate the conflict. The from code,
> if an unmerged path is skipped, there will be warnings.

Not sure I understand. Are you saying that if -m is not specified and nor is --source or --index, that we print a warning for each unmerged entry specified by the pathspecs?

Show 21 quoted lines

> > > +
> > > +-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?
>
> I will make sure that the code aborts. Not so sure about mentioning
> every incompatible combination though. Will it be too much? I think we
> catch and report plenty invalid combinations but I don't think we have
> mentioned them all (or maybe we have, I didn't check the document
> again)

Making sure the code aborts is probably good enough.

Elijah