Re: [PATCH v3 07/14] checkout: split into switch-branch and restore-files

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 5, 2018, 02:14 UTC
Message-ID: <xmqqtvjsen3r.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CACsJy8BTs+WKzTTEF2XVTT-LVJk_exYCz_hN+hXU1Dw+oquBpA@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

Show 9 quoted lines

>> My single biggest worry about this whole series is that I'm worried
>> you're perpetuating and even further ingraining one of the biggest
>> usability problems with checkout: people suggest and use it for
>> reverting/restoring paths to a previous version, but it doesn't do
>> that:
>
> ...
>
>  git restore-files --from=master~10 Documentation/

The "single biggest worry" could be due to Elijah not being aware of other recent discussions. My understanding of the plan is

 - "git checkout" will learn a new "--[no-]overlay" option, where
   the current behaviour, i.e. "take paths in master~10 that match
   pathspec Documentation/, and overlay them on top of what is in
   the index and the working tree", is explained as "the overlay
   mode" and stays to be the default.  With "checkout --no-overlay
   master~10 Documentation/", the command will become "replace paths
   in the current index and the working tree that match the pathspec
   Documentation/ with paths in master~10 that match pathspec
   Documentation/".

 - "git restore-files --from=<tree> <pathspec>" by default will use
   "--no-overlay" semantics, but the users can still use "--overlay"
   from the command line as an option.

So "restore-files" would become truly "restore the state of Documentation/ to match that of master~10", I would think.

Show 11 quoted lines

>> Also, the fact that we're trying to make a simpler command makes me
>> think that removing the auto-vivify behavior from the default and
>> adding a simple flag which users can pass to request will allow this
>> part of the documentation to be hidden behind the appropriate flag,
>> which may make it easier for users to compartmentalize the command and
>> it's options, enabling them to learn as they go.
>
> Sounds good. I don't know a good name for this new option though so
> unless anybody comes up with some suggestion, I'll just disable
> checkout.defaultRemote in switch-branch. If it comes back as a new
> option, it can always be added later.

Are you two discussing the "checkout --guess" option? I am somewhat lost here.

Show 14 quoted lines

>> > +-f::
>> > +--force::
>> > +       Proceed even if the index or the working tree differs from
>> > +       HEAD.  This is used to throw away local changes.
>>
>> Haven't thought through this thoroughly, but do we really need an
>> option for that instead of telling users to 'git reset --hard HEAD'
>> before switching branches if they want their stuff thrown away?
>
> For me it's just a bit more convenient. Hit an error when switching
> branch? Recall the command from bash history, stick -f in it and run.
> Elsewhere I think both Junio and Thomas (or maybe only Junio) suggests
> moving the "git reset" functionality without moving HEAD to one of
> these commands, which goes the opposite direction...

Isn't there a huge difference? "checkout --force <other-branch>" needs to clobber only the changes that are involved in the switch, i.e. if your README.txt is the same between master and maint while Makefile is different, after editing both files while on master, you can not "switch-branch" to maint without doing something to Makefile (i.e. either discard your local change or wiggle your local change to the context of 'maint' with "checkout -m"). But you can carry the changes to README.txt while checking out 'maint' branch. Running "git reset --hard HEAD" would mean that you will lose the changes to README.txt as well.

Show 12 quoted lines

>> > +--orphan <new_branch>::
>> > +       Create a new 'orphan' branch, named <new_branch>, started from
>> > +       <start_point> and switch to it.  The first commit made on this
>>
>> What??  started from <start_point>?  The whole point of --orphan is
>> you have no parent, i.e. no start point.  Also, why does the
>> explanation reference an argument that wasn't in the immediately
>> preceding synopsis?
>
> I guess bad phrasing. It should be "switch to <start_point> first,
> then prepare the worktree so that the first commit will have no
> parent". Or something along that line.

It should be a <tree-ish>, no? It is not a "point" in history, but is "start with this tree".

I may have more comments on this message but that's it from me for now.