Re: [PATCH v1 01/11] checkout: split part of it to new command 'restore'
- Date: Sat, 9 Mar 2019 10:27:05 -0800
- From: Elijah Newren <newren@xxxxxxxxx>
- Subject: Re: [PATCH v1 01/11] checkout: split part of it to new command 'restore'
On Sat, Mar 9, 2019 at 4:16 AM Duy Nguyen <pclouds@xxxxxxxxx> wrote:
> On Sat, Mar 9, 2019 at 1:01 AM Elijah Newren <newren@xxxxxxxxx> 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
Making sure the code aborts is probably good enough.