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

[Elijah Newren <newren@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:

> > > +-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.

> > > +-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.

> > 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?

> > > +
> > > +-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