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

2018-12-04 Thread Junio C Hamano
Elijah Newren  writes:

> What depends on stage#2 coming from the commit that will become the
> first parent?

How about "git diff --cc" for a starter?  What came from HEAD's
ancestry should appear first and then what came from the side branch
that is merged into.



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

2018-12-04 Thread Elijah Newren
On Tue, Dec 4, 2018 at 6:26 PM Junio C Hamano  wrote:
>
> Duy Nguyen  writes:
>
> > On Tue, Dec 4, 2018 at 6:43 PM Elijah Newren  wrote:
> >> > > > +--ours::
> >> > > > +--theirs::
> >> ...
> >> go away.  Maybe it can still be fixed (I haven't dug too deeply into
> >> it), but if so, the only fix needed here would be to remove this long
> >> explanation about why the tool gets things totally backward.
> >
> > Aha. I' not really deep in this merge business to know if stages 2 and
> > 3 can be swapped. This is right up your alley. I'll just leave it to
> > you.
>
> Please don't show stage#2 and stage#3 swapped to the end user,
> unless that is protected behind an option (not per-repo config).
> It is pretty much ingrained that stage#2 is what came from the
> commit that will become the first parent of the commit being
> prepared, and changing it without an explicit command line option
> will break tools.

What depends on stage#2 coming from the commit that will become the
first parent?  I wasn't thinking in terms of modifying
checkout/restore-files/diff/etc in a way that would make them show
things different than what was recorded in the index, I was rather
musing on whether it was feasible to have rebase tell the merge
machinery to treat HEAD as stage #3 and the other commit as stage #2
so that it was swapping what was actually recorded in the index.

I know the merge machinery implicitly assumes HEAD == stage #2 in
multiple places, and it'd obviously need a fair amount of fixing to
handle this.  I wasn't immediately aware of other things that would
break.  If you know of some, I'm happy to hear.  Otherwise, I might go
and learn the hard way (after I get around to the merge rewrite) why
my idea is crazy.  :-)

> > I'm actually still not sure how to move it here (I guess 'here' is
> > restore-files since we won't move HEAD). All the --mixed, --merge and
> > --hard are confusing. But maybe we could just make 'git restore-files
> > --from HEAD -f :/" behave just like "git reset --hard HEAD" (but with
> > some safety net) But we can leave it for discussion in the next round.
>
> Perhaps you two should pay a bit closer attention to what Thomas
> Gummerer is working on.  I've touched above in my earlier comments,
> too, e.g.  

Indeed, I'm excited about his changes now; I'll keep an eye out.


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

2018-12-04 Thread Elijah Newren
On Tue, Dec 4, 2018 at 6:14 PM Junio C Hamano  wrote:
>
> Duy Nguyen  writes:
>
> >> 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= " 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.

Oh, sweet, that's awesome.  I saw some of the other emails as I was
scanning through and looked at the --overlay stuff since it sounded
relevant but something in the explanation made me think it was about
whether the command would write to the index or the working tree,
rather than being about adding+overwriting vs. just overwriting.

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

Generally what was being discussed was just that this manpage was
rather complicated for the standard base-case due to the need to
explain all the behavior associated with --guess since that option is
on by default in checkout.  And since --guess is controlled by
checkout.defaultRemote, that was part of the extra complexity that had
to be learned in the basic explanation, rather than letting users
learn it when they learn a new flag.

The critical part you were missing was part of the original text just
before the quoted part was:

>>> So switch-branch will be controlled by checkout.* config variables?
>>> That probably makes the most sense, but it does dilute the advantage
>>> of adding these simpler commands.

Duy is responding to that even if it wasn't included in his quoting.

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

Ah, indeed.  Thanks for pointing out what I missed here.

> >> > +--orphan ::
> >> > +   Create a new 'orphan' branch, named , started from
> >> > +and switch to it.  The first commit made on this
> >>
> >> What??  started from ?  The whole point of --orphan is
> >> 

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

2018-12-04 Thread Junio C Hamano
Duy Nguyen  writes:

> On Tue, Dec 4, 2018 at 6:43 PM Elijah Newren  wrote:
>> > > > +--ours::
>> > > > +--theirs::
>> ...
>> go away.  Maybe it can still be fixed (I haven't dug too deeply into
>> it), but if so, the only fix needed here would be to remove this long
>> explanation about why the tool gets things totally backward.
>
> Aha. I' not really deep in this merge business to know if stages 2 and
> 3 can be swapped. This is right up your alley. I'll just leave it to
> you.

Please don't show stage#2 and stage#3 swapped to the end user,
unless that is protected behind an option (not per-repo config).
It is pretty much ingrained that stage#2 is what came from the
commit that will become the first parent of the commit being
prepared, and changing it without an explicit command line option
will break tools.

> I'm actually still not sure how to move it here (I guess 'here' is
> restore-files since we won't move HEAD). All the --mixed, --merge and
> --hard are confusing. But maybe we could just make 'git restore-files
> --from HEAD -f :/" behave just like "git reset --hard HEAD" (but with
> some safety net) But we can leave it for discussion in the next round.

Perhaps you two should pay a bit closer attention to what Thomas
Gummerer is working on.  I've touched above in my earlier comments,
too, e.g.  


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

2018-12-04 Thread Junio C Hamano
Duy Nguyen  writes:

>> 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= " 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.

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

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

>> > +--orphan ::
>> > +   Create a new 'orphan' branch, named , started from
>> > +and switch to it.  The first commit made on this
>>
>> What??  started from ?  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  first,
> then prepare the worktree so that the first commit will have no
> parent". Or something along that line.

It should be a , 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.


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

2018-12-04 Thread Duy Nguyen
On Tue, Dec 4, 2018 at 6:43 PM Elijah Newren  wrote:
> > > > +--ours::
> > > > +--theirs::
> > > > +   Check out stage #2 ('ours') or #3 ('theirs') for unmerged
> > > > +   paths.
> > > > ++
> > > > +Note that during `git rebase` and `git pull --rebase`, 'ours' and
> > > > +'theirs' may appear swapped; `--ours` gives the version from the
> > > > +branch the changes are rebased onto, while `--theirs` gives the
> > > > +version from the branch that holds your work that is being rebased.
> > > > ++
> > > > +This is because `rebase` is used in a workflow that treats the
> > > > +history at the remote as the shared canonical one, and treats the
> > > > +work done on the branch you are rebasing as the third-party work to
> > > > +be integrated, and you are temporarily assuming the role of the
> > > > +keeper of the canonical history during the rebase.  As the keeper of
> > > > +the canonical history, you need to view the history from the remote
> > > > +as `ours` (i.e. "our shared canonical history"), while what you did
> > > > +on your side branch as `theirs` (i.e. "one contributor's work on top
> > > > +of it").
> > >
> > > Total aside because I'm not sure what you could change here, but man
> > > do I hate this.
> >
> > Uh it's actually documented? I'm always confused by this too. --ours
> > and --theirs at this point are pretty much tied to stage 2 and 3.
> > Nothing I can do about it. But if you could come up with some other
> > option names, then we could make "new ours" to be stage 3 during
> > rebase, for example.
>
> I don't think it's a naming issue, personally.  Years ago we could
> have defined --ours and --theirs differently based on which kind of
> operation we were in the middle of, but you are probably right that
> they are now tied to stage 2 and 3.  But there's another place that we
> might still be able to address this; I think the brain-damage here may
> have just been due to the fact that the recursive merge machinery was
> rather inflexible and required HEAD to be stage 2.  If it were a
> little more flexible, then we might just be able to make this problem
> go away.  Maybe it can still be fixed (I haven't dug too deeply into
> it), but if so, the only fix needed here would be to remove this long
> explanation about why the tool gets things totally backward.

Aha. I' not really deep in this merge business to know if stages 2 and
3 can be swapped. This is right up your alley. I'll just leave it to
you.

> > > > +'git switch-branch' -c|-C  []::
> > > > +
> > > > +   Specifying `-c` causes a new branch to be created as if
> > > > +   linkgit:git-branch[1] were called and then switched to. In
> > > > +   this case you can use the `--track` or `--no-track` options,
> > > > +   which will be passed to 'git branch'.  As a convenience,
> > > > +   `--track` without `-c` implies branch creation; see the
> > > > +   description of `--track` below.
> > >
> > > Can we get rid of --track/--no-track and just provide a flag (which
> > > takes no arguments) for the user to use?  Problems with --track:
> > >   * it's not even in your synopsis
> > >   * user has to repeat themselves (e.g. 'next' in two places from '-c
> > > next --track origin/next'); this repetition is BOTH laborious AND
> > > error-prone
> > >   * it's rather inconsistent: --track is the default due to
> > > auto-vivify when the user specifies nothing but a branch name that
> > > doesn't exist yet, but when the user realizes the branch doesn't exist
> > > yet and asks to have it created then suddenly tracking is not the
> > > default??
> >
> > I don't think --track is default anymore (maybe I haven't updated the
> > man page correctly). The dwim behavior is only activated in
> > switch-branch when you specify --guess to reduce the amount of magic
> > we throw at the user. With that in mind, do we still hide
> > --track/--no-track from switch-branch?
>
> Ooh, you're adding --guess?  Cool, that addresses my concerns, just in
> a different manner.

No it's always there even in git-checkout, just hidden.

> Personally, I'd leave --track/--no-track out.  It's extra mental
> overhead, git branch has options for setting those if they need some
> special non-default setup, and if there is enough demand for it we can
> add it later.  Removing options once published is much harder.

Slightly less convenient (you would need a combination of git-branch
and git-switch-branch, if you avoid git-checkout). But since I don't
think I have ever used this option, I'm fine with leaving it out and
considering adding it back later.

> > > I'm not sure what's best, but here's some food for thought:
> > >
> > >
> > >git switch-branch 
> > > switches to , if it exists.  Error cases:
> > >   * If  isn't actually a branch but a  or
> > >  or , error out and suggest using
> > > --detach.
> > >   * If  isn't actually a branch but there is a similarly named
> > >  (e.g. origin/), then suggest using
> > > -c.
> >
> > I would make 

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

2018-12-04 Thread Elijah Newren
On Tue, Dec 4, 2018 at 8:22 AM Duy Nguyen  wrote:
>
> Thanks for all the comments! There are still some I haven't replied
> (either I'll agree and do it anyway, or I'll need some more time to
> digest)
>
> On Tue, Dec 4, 2018 at 1:45 AM Elijah Newren  wrote:
> > > +'git restore-files' [--from=] ...
> >
> > Isn't this already inferred by the previous line?  Or was the
> > inclusion of --from on the previous line in error?  Looking at the
> > git-checkout manpage, it looks like you may have just been copying an
> > existing weirdness, but it needs to be fixed.  ;-)
>
> Hehe.
>
> > > +'git restore-files' (-p|--patch) [--from=] [...]
> > > +
> > > +DESCRIPTION
> > > +---
> > > +Updates files in the working tree to match the version in the index
> > > +or the specified tree.
> > > +
> > > +'git restore-files' [--from=] ...::
> >
> >  and ?  I understand  and ,
> > or  but have no clue why it'd be okay to specify 
> > and  together.  What does that even mean?
> >
> > Also, rather than fixing from  to  or ,
> > perhaps we should just use  here?  (I'm thinking of git
> > rev-parse's "Specifying revisions", which suggests "revisions" as a
> > good substitute for "commit-ish" that isn't quite so weird for new
> > users.)
>
> tree-ish is technically more accurate. But I'm ok with just
> . If you give it a blob oid then you should get a nice
> explanation what you're doing wrong anyway.

Documenting as  but having it be more general under the hood
and actually accept  sounds good to me.  I just think the
pain of trying to explain  is too much of a hurdle for
users, especially as I expect it to be very unlikely that users will
ever take advantage of it.

> > > +   Overwrite paths in the working tree by replacing with the
> > > +   contents in the index or in the  (most often a
> > > +   commit).  When a  is given, the paths that
> > > +   match the  are updated both in the index and in
> > > +   the working tree.
> >
> > Is that the default we really want for this command?  Why do we
> > automatically assume these files are ready for commit?  I understand
> > that it's what checkout did, but I'd find it more natural to only
> > affect the working tree by default.  We can give it an option for
> > affecting the index instead (or perhaps in addition).
>
> Yeah, that behavior of updating the index always bothers me when I use
> it but I seemed to forget when working on this.
>
> > > +--ours::
> > > +--theirs::
> > > +   Check out stage #2 ('ours') or #3 ('theirs') for unmerged
> > > +   paths.
> > > ++
> > > +Note that during `git rebase` and `git pull --rebase`, 'ours' and
> > > +'theirs' may appear swapped; `--ours` gives the version from the
> > > +branch the changes are rebased onto, while `--theirs` gives the
> > > +version from the branch that holds your work that is being rebased.
> > > ++
> > > +This is because `rebase` is used in a workflow that treats the
> > > +history at the remote as the shared canonical one, and treats the
> > > +work done on the branch you are rebasing as the third-party work to
> > > +be integrated, and you are temporarily assuming the role of the
> > > +keeper of the canonical history during the rebase.  As the keeper of
> > > +the canonical history, you need to view the history from the remote
> > > +as `ours` (i.e. "our shared canonical history"), while what you did
> > > +on your side branch as `theirs` (i.e. "one contributor's work on top
> > > +of it").
> >
> > Total aside because I'm not sure what you could change here, but man
> > do I hate this.
>
> Uh it's actually documented? I'm always confused by this too. --ours
> and --theirs at this point are pretty much tied to stage 2 and 3.
> Nothing I can do about it. But if you could come up with some other
> option names, then we could make "new ours" to be stage 3 during
> rebase, for example.

I don't think it's a naming issue, personally.  Years ago we could
have defined --ours and --theirs differently based on which kind of
operation we were in the middle of, but you are probably right that
they are now tied to stage 2 and 3.  But there's another place that we
might still be able to address this; I think the brain-damage here may
have just been due to the fact that the recursive merge machinery was
rather inflexible and required HEAD to be stage 2.  If it were a
little more flexible, then we might just be able to make this problem
go away.  Maybe it can still be fixed (I haven't dug too deeply into
it), but if so, the only fix needed here would be to remove this long
explanation about why the tool gets things totally backward.

> > > +Part of the linkgit:git[1] suite
> >
> >
> > 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 

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

2018-12-04 Thread Duy Nguyen
Thanks for all the comments! There are still some I haven't replied
(either I'll agree and do it anyway, or I'll need some more time to
digest)

On Tue, Dec 4, 2018 at 1:45 AM Elijah Newren  wrote:
> > +'git restore-files' [--from=] ...
>
> Isn't this already inferred by the previous line?  Or was the
> inclusion of --from on the previous line in error?  Looking at the
> git-checkout manpage, it looks like you may have just been copying an
> existing weirdness, but it needs to be fixed.  ;-)

Hehe.

> > +'git restore-files' (-p|--patch) [--from=] [...]
> > +
> > +DESCRIPTION
> > +---
> > +Updates files in the working tree to match the version in the index
> > +or the specified tree.
> > +
> > +'git restore-files' [--from=] ...::
>
>  and ?  I understand  and ,
> or  but have no clue why it'd be okay to specify 
> and  together.  What does that even mean?
>
> Also, rather than fixing from  to  or ,
> perhaps we should just use  here?  (I'm thinking of git
> rev-parse's "Specifying revisions", which suggests "revisions" as a
> good substitute for "commit-ish" that isn't quite so weird for new
> users.)

tree-ish is technically more accurate. But I'm ok with just
. If you give it a blob oid then you should get a nice
explanation what you're doing wrong anyway.


> > +   Overwrite paths in the working tree by replacing with the
> > +   contents in the index or in the  (most often a
> > +   commit).  When a  is given, the paths that
> > +   match the  are updated both in the index and in
> > +   the working tree.
>
> Is that the default we really want for this command?  Why do we
> automatically assume these files are ready for commit?  I understand
> that it's what checkout did, but I'd find it more natural to only
> affect the working tree by default.  We can give it an option for
> affecting the index instead (or perhaps in addition).

Yeah, that behavior of updating the index always bothers me when I use
it but I seemed to forget when working on this.

> > +--ours::
> > +--theirs::
> > +   Check out stage #2 ('ours') or #3 ('theirs') for unmerged
> > +   paths.
> > ++
> > +Note that during `git rebase` and `git pull --rebase`, 'ours' and
> > +'theirs' may appear swapped; `--ours` gives the version from the
> > +branch the changes are rebased onto, while `--theirs` gives the
> > +version from the branch that holds your work that is being rebased.
> > ++
> > +This is because `rebase` is used in a workflow that treats the
> > +history at the remote as the shared canonical one, and treats the
> > +work done on the branch you are rebasing as the third-party work to
> > +be integrated, and you are temporarily assuming the role of the
> > +keeper of the canonical history during the rebase.  As the keeper of
> > +the canonical history, you need to view the history from the remote
> > +as `ours` (i.e. "our shared canonical history"), while what you did
> > +on your side branch as `theirs` (i.e. "one contributor's work on top
> > +of it").
>
> Total aside because I'm not sure what you could change here, but man
> do I hate this.

Uh it's actually documented? I'm always confused by this too. --ours
and --theirs at this point are pretty much tied to stage 2 and 3.
Nothing I can do about it. But if you could come up with some other
option names, then we could make "new ours" to be stage 3 during
rebase, for example.

> > +Part of the linkgit:git[1] suite
>
>
> 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/
> 
> git add -u
> git commit -m "Rationale for changing files including reverting 
> Documentation/"
>
> In particular, now you have a mixture of files in Documentation/ from
> master~10 (er, now likely master~11) and HEAD~1; any files and
> sub-directories that existed in HEAD~1 still remain and are mixed with
> all other files in Documentation/ from the older commit.
>
> You may think this is a special case, but this particular issue
> results in some pretty bad surprises.  Also, it was pretty surprising
> to me just how difficult it was to implement an svn-like revert in
> EasyGit, in large part because of this 'oversight' in git.  git
> checkout --  to me has always been fundamentally wrong, but I
> just wasn't sure if I wanted to fight the backward compatibility
> battle and suggest changing it.  With a new command, we definitely
> shouldn't be reinforcing this error.  (And maybe we should consider
> taking the time to fix git checkout too.)

What would be the right behavior for

 git restore-files --from=master~10 Documentation/

then? Consider it an error? I often use "git checkout HEAD" and "git
checkout HEAD^" (usually with -p) but not very far back like
master~10.

> > +If the branch exists 

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

2018-12-03 Thread Junio C Hamano
Elijah Newren  writes:

>> +Updates files in the working tree to match the version in the index
>> +or the specified tree.
>> +
>> +'git restore-files' [--from=] ...::
>
>  and ?  I understand  and ,
> or  but have no clue why it'd be okay to specify 
> and  together.  What does that even mean?

I have this tree object v2.6.11-tree that is not enclosed in a
commit object.  I want to take the top-level Makefile out of that
tree, stuff it in the index and overwrite the working tree file.

$ git checkout v2.6.11-tree Makefile
$ git restore-files --from=v2.6.11-tree Makefile

>> +   Overwrite paths in the working tree by replacing with the
>> +   contents in the index or in the  (most often a
>> +   commit).  When a  is given, the paths that
>> +   match the  are updated both in the index and in
>> +   the working tree.
>
> Is that the default we really want for this command?  Why do we
> automatically assume these files are ready for commit?  I understand
> that it's what checkout did, but I'd find it more natural to only
> affect the working tree by default.  We can give it an option for
> affecting the index instead (or perhaps in addition).

Oooah.  Now this is getting juicy.  

I do think supporting "--index" (which would make it more in line
with what Duy wrote), with optionally "--cached" as well, and making
the "working tree only" mode as default may not be a bad idea.  I am
offhand not sure how the "working tree only" mode (similar to the
default mode of "git apply" that mimics the way "patch -p1" works)
should interact with the non-overlay mode of the command, but other
than that, I tend to agree with the idea that restore-files is only
a part of making the contents into committable shape, not exactly
ready for it yet.


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

2018-12-03 Thread Elijah Newren
On Thu, Nov 29, 2018 at 2:03 PM Nguyễn Thái Ngọc Duy  wrote:
>
> "git checkout" doing too many things is a source of confusion for many
> users (and it even bites old timers sometimes). To rememdy that, the
> command is now split in two: switch-branch and checkout-files. The

"checkout-files" here(will comment more on this below)

> good old "git checkout" command is still here and will be until all
> (or most of users) are sick of it.
>
> See the new man pages for the final design of these commands. The
> actual implementation though is still pretty much the same as "git
> checkout". Following patches will adjust their behavior to match the
> man pages.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  .gitignore  |   2 +
>  Documentation/git-checkout.txt  |   5 +
>  Documentation/git-restore-files.txt | 167 
>  Documentation/git-switch-branch.txt | 289 
>  Makefile|   2 +
>  builtin.h   |   2 +
>  builtin/checkout.c  |  84 ++--
>  command-list.txt|   2 +
>  git.c   |   2 +
>  9 files changed, 543 insertions(+), 12 deletions(-)
>  create mode 100644 Documentation/git-restore-files.txt
>  create mode 100644 Documentation/git-switch-branch.txt
>
> diff --git a/.gitignore b/.gitignore
> index 0d77ea5894..c63dcb1427 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -143,6 +143,7 @@
>  /git-request-pull
>  /git-rerere
>  /git-reset
> +/git-restore-files

...and "restore-files" here.  Should be consistent with whatever name you pick.

>  /git-rev-list
>  /git-rev-parse
>  /git-revert
> @@ -167,6 +168,7 @@
>  /git-submodule
>  /git-submodule--helper
>  /git-svn
> +/git-switch-branch
>  /git-symbolic-ref
>  /git-tag
>  /git-unpack-file
> diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
> index 25887a6087..25ec7f508f 100644
> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -406,6 +406,11 @@ $ edit frotz
>  $ git add frotz
>  
>
> +SEE ALSO
> +
> +linkgit:git-switch-branch[1]
> +linkgit:git-restore-files[1]
> +
>  GIT
>  ---
>  Part of the linkgit:git[1] suite
> diff --git a/Documentation/git-restore-files.txt 
> b/Documentation/git-restore-files.txt
> new file mode 100644
> index 00..03c1250ad0
> --- /dev/null
> +++ b/Documentation/git-restore-files.txt
> @@ -0,0 +1,167 @@
> +git-restore-files(1)
> +
> +
> +NAME
> +
> +git-restore-files - Restore working tree files
> +
> +SYNOPSIS
> +
> +[verse]
> +'git restore-files' 

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

2018-11-29 Thread Nguyễn Thái Ngọc Duy
"git checkout" doing too many things is a source of confusion for many
users (and it even bites old timers sometimes). To rememdy that, the
command is now split in two: switch-branch and checkout-files. The
good old "git checkout" command is still here and will be until all
(or most of users) are sick of it.

See the new man pages for the final design of these commands. The
actual implementation though is still pretty much the same as "git
checkout". Following patches will adjust their behavior to match the
man pages.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 .gitignore  |   2 +
 Documentation/git-checkout.txt  |   5 +
 Documentation/git-restore-files.txt | 167 
 Documentation/git-switch-branch.txt | 289 
 Makefile|   2 +
 builtin.h   |   2 +
 builtin/checkout.c  |  84 ++--
 command-list.txt|   2 +
 git.c   |   2 +
 9 files changed, 543 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/git-restore-files.txt
 create mode 100644 Documentation/git-switch-branch.txt

diff --git a/.gitignore b/.gitignore
index 0d77ea5894..c63dcb1427 100644
--- a/.gitignore
+++ b/.gitignore
@@ -143,6 +143,7 @@
 /git-request-pull
 /git-rerere
 /git-reset
+/git-restore-files
 /git-rev-list
 /git-rev-parse
 /git-revert
@@ -167,6 +168,7 @@
 /git-submodule
 /git-submodule--helper
 /git-svn
+/git-switch-branch
 /git-symbolic-ref
 /git-tag
 /git-unpack-file
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 25887a6087..25ec7f508f 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -406,6 +406,11 @@ $ edit frotz
 $ git add frotz
 
 
+SEE ALSO
+
+linkgit:git-switch-branch[1]
+linkgit:git-restore-files[1]
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/Documentation/git-restore-files.txt 
b/Documentation/git-restore-files.txt
new file mode 100644
index 00..03c1250ad0
--- /dev/null
+++ b/Documentation/git-restore-files.txt
@@ -0,0 +1,167 @@
+git-restore-files(1)
+
+
+NAME
+
+git-restore-files - Restore working tree files
+
+SYNOPSIS
+
+[verse]
+'git restore-files'