[PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Christoph Michelbach]

While technically in the documentation, the fact that changes
introduced by a checkout <tree-ish> are staged automatically, was
not obvious when reading its documentation. It is now specifically
pointed out.

Signed-off-by: Christoph Michelbach <michelbach94@gmail.com>
---
 Documentation/git-checkout.txt | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 8e2c066..cfd7b18 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -85,9 +85,10 @@ Omitting <branch> detaches HEAD at the tip of the current branch.
 	from the index file or from a named <tree-ish> (most often a
 	commit).  In this case, the `-b` and `--track` options are
 	meaningless and giving either of them results in an error.  The
-	<tree-ish> argument can be used to specify a specific tree-ish
-	(i.e.  commit, tag or tree) to update the index for the given
-	paths before updating the working tree.
+	<tree-ish> argument can be used to specify the tree-ish (i.e.
+	commit,	tag, or tree) to update the index to for the given paths
+	before updating the working tree accordingly.  Note that this means
+	that the changes this command introduces are staged automatically.
 +
 'git checkout' with <paths> or `--patch` is used to restore modified or
 deleted paths to their original contents from the index or replace paths
-- 
2.7.4

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Philip Oakley]

From: "Christoph Michelbach" <michelbach94@gmail.com>
> While technically in the documentation, the fact that changes
> introduced by a checkout <tree-ish> are staged automatically, was
> not obvious when reading its documentation. It is now specifically
> pointed out.
>
> Signed-off-by: Christoph Michelbach <michelbach94@gmail.com>
> ---
> Documentation/git-checkout.txt | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-checkout.txt 
> b/Documentation/git-checkout.txt
> index 8e2c066..cfd7b18 100644
> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -85,9 +85,10 @@ Omitting <branch> detaches HEAD at the tip of the 
> current branch.
> from the index file or from a named <tree-ish> (most often a
> commit). In this case, the `-b` and `--track` options are
> meaningless and giving either of them results in an error. The
> - <tree-ish> argument can be used to specify a specific tree-ish
> - (i.e. commit, tag or tree) to update the index for the given

Do these lines above actually need reflowing? Their content hasn't changed 
making it more difficult to spot the significant changes below here.

> - paths before updating the working tree.
> + <tree-ish> argument can be used to specify the tree-ish (i.e.
> + commit, tag, or tree) to update the index to for the given paths
> + before updating the working tree accordingly.

> +                               Note that this means
> + that the changes this command introduces are staged automatically.

Does this actually capture the intent of the user confusion it's meant to 
cover? I may have missed the original discussions.

For a clean commit checkout my mental model is not one of anything new being 
actively staged i.e. different from what was in the commit. I can see that 
if a particular tree is checkout then it's implicit staging could well be a 
surprise.

I am in favour of improving the documentation to avoid user surprise

> +
> 'git checkout' with <paths> or `--patch` is used to restore modified or
> deleted paths to their original contents from the index or replace paths
> -- 
> 2.7.4
>
--
Philip 

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Christoph Michelbach]

On Sun, 2017-04-16 at 00:28 +0100, Philip Oakley wrote:
> From: "Christoph Michelbach" <michelbach94@gmail.com>
> > 
> > While technically in the documentation, the fact that changes
> > introduced by a checkout <tree-ish> are staged automatically, was
> > not obvious when reading its documentation. It is now specifically
> > pointed out.
> > 
> > Signed-off-by: Christoph Michelbach <michelbach94@gmail.com>
> > ---
> > Documentation/git-checkout.txt | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/git-checkout.txt 
> > b/Documentation/git-checkout.txt
> > index 8e2c066..cfd7b18 100644
> > --- a/Documentation/git-checkout.txt
> > +++ b/Documentation/git-checkout.txt
> > @@ -85,9 +85,10 @@ Omitting <branch> detaches HEAD at the tip of the 
> > current branch.
> > from the index file or from a named <tree-ish> (most often a
> > commit). In this case, the `-b` and `--track` options are
> > meaningless and giving either of them results in an error. The
> > - <tree-ish> argument can be used to specify a specific tree-ish
> > - (i.e. commit, tag or tree) to update the index for the given
> Do these lines above actually need reflowing? Their content hasn't changed 
> making it more difficult to spot the significant changes below here.

They're just part of the context of the automatically created patch.


> > +                               Note that this means
> > + that the changes this command introduces are staged automatically.
> Does this actually capture the intent of the user confusion it's meant to 
> cover? I may have missed the original discussions.

There is no original discussion in this mailing list. I got surprised the command
automatically modified my staging area even though I didn't remember to have read it in
the corresponding man page. Upon reading the relevant part of the man page again, I
noticed that it in fact can be inferred from the half sentence "to update the index for
the given paths before updating the working tree." but it isn't pointed out explicitly.
Doing so doesn't take a lot more text and can avoid such surprises. So yes, if it's
pointed out explicitly, the confusion is removed.


> For a clean commit checkout my mental model is not one of anything new being 
> actively staged i.e. different from what was in the commit.

Note that this is not about something like `git checkout 925b29` but about something like
`git checkout 925b29 src`.


> I can see that 
> if a particular tree is checkout then it's implicit staging could well be a 
> surprise.

And it is actually documented. Just not explicitly mentioned / pointed out.

--
Christoph

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Philip Oakley]

From: "Christoph Michelbach" <michelbach94@gmail.com>
> On Sun, 2017-04-16 at 00:28 +0100, Philip Oakley wrote:
>> From: "Christoph Michelbach" <michelbach94@gmail.com>
>> >
>> > While technically in the documentation, the fact that changes
>> > introduced by a checkout <tree-ish> are staged automatically, was
>> > not obvious when reading its documentation. It is now specifically
>> > pointed out.
>> >
>> > Signed-off-by: Christoph Michelbach <michelbach94@gmail.com>
>> > ---
>> > Documentation/git-checkout.txt | 7 ++++---
>> > 1 file changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/Documentation/git-checkout.txt
>> > b/Documentation/git-checkout.txt
>> > index 8e2c066..cfd7b18 100644
>> > --- a/Documentation/git-checkout.txt
>> > +++ b/Documentation/git-checkout.txt
>> > @@ -85,9 +85,10 @@ Omitting <branch> detaches HEAD at the tip of the
>> > current branch.
>> > from the index file or from a named <tree-ish> (most often a
>> > commit). In this case, the `-b` and `--track` options are
>> > meaningless and giving either of them results in an error. The
>> > - <tree-ish> argument can be used to specify a specific tree-ish
>> > - (i.e. commit, tag or tree) to update the index for the given
>> Do these lines above actually need reflowing? Their content hasn't
>> changed
>> making it more difficult to spot the significant changes below here.
>
> They're just part of the context of the automatically created patch.

The lines with +/- markings are the actual change lines. It's the lines
without them that are the context lines.

It does sound like an accidental reflow where the "(i.e." moved between
lines, and the "paths" moved for the following lines.

If reflowing is required it's normal to put it in a separate patch so that 
each type of change gets its own commit message.
>
>
>> > + Note that this means
>> > + that the changes this command introduces are staged automatically.
>> Does this actually capture the intent of the user confusion it's meant to
>> cover? I may have missed the original discussions.
>
> There is no original discussion in this mailing list. I got surprised the
> command
> automatically modified my staging area even though I didn't remember to
> have read it in
> the corresponding man page. Upon reading the relevant part of the man page
> again, I
> noticed that it in fact can be inferred from the half sentence "to update
> the index for
> the given paths before updating the working tree." but it isn't pointed
> out explicitly.
> Doing so doesn't take a lot more text and can avoid such surprises. So
> yes, if it's
> pointed out explicitly, the confusion is removed.
>
>
>> For a clean commit checkout my mental model is not one of anything new
>> being
>> actively staged i.e. different from what was in the commit.
>
> Note that this is not about something like `git checkout 925b29` but about
> something like
> `git checkout 925b29 src`.

Yes, that was the part I was getting at.

The commit message (and the patch context) doesn't quite give enough to see 
that is is particular to the -

git checkout [-p|--patch] [<tree-ish>] [--] [<paths>…​]invocation.

>> I can see that
>> if a particular tree is checkout then it's implicit staging could well be 
>> a
>> surprise.

> And it is actually documented. Just not explicitly mentioned / pointed 
> out.

It maybe worth thinking a bit more about how such a mental model may happen.

To those in the know it sounds obvious that the path content replaces the 
old path. It's that this implies a *change* within the commit that may not 
be.

Explaining that it at the heart of the documentation change.

> > --
> Christoph
> >
--
Philip 

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Christoph Michelbach]

On Sun, 2017-04-16 at 19:03 +0100, Philip Oakley wrote:
> From: "Christoph Michelbach" <michelbach94@gmail.com>
> > 
> > On Sun, 2017-04-16 at 00:28 +0100, Philip Oakley wrote:
> > > 
> > > From: "Christoph Michelbach" <michelbach94@gmail.com>
> > > > 
> > > > 
> > > > While technically in the documentation, the fact that changes
> > > > introduced by a checkout <tree-ish> are staged automatically,
> > > > was
> > > > not obvious when reading its documentation. It is now
> > > > specifically
> > > > pointed out.
> > > > 
> > > > Signed-off-by: Christoph Michelbach <michelbach94@gmail.com>
> > > > ---
> > > > Documentation/git-checkout.txt | 7 ++++---
> > > > 1 file changed, 4 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/Documentation/git-checkout.txt
> > > > b/Documentation/git-checkout.txt
> > > > index 8e2c066..cfd7b18 100644
> > > > --- a/Documentation/git-checkout.txt
> > > > +++ b/Documentation/git-checkout.txt
> > > > @@ -85,9 +85,10 @@ Omitting <branch> detaches HEAD at the tip of
> > > > the
> > > > current branch.
> > > > from the index file or from a named <tree-ish> (most often a
> > > > commit). In this case, the `-b` and `--track` options are
> > > > meaningless and giving either of them results in an error. The
> > > > - <tree-ish> argument can be used to specify a specific tree-ish
> > > > - (i.e. commit, tag or tree) to update the index for the given
> > > Do these lines above actually need reflowing? Their content hasn't
> > > changed
> > > making it more difficult to spot the significant changes below
> > > here.
> > They're just part of the context of the automatically created patch.
> The lines with +/- markings are the actual change lines. It's the
> lines
> without them that are the context lines.
> 
> It does sound like an accidental reflow where the "(i.e." moved
> between
> lines, and the "paths" moved for the following lines.
> 
> If reflowing is required it's normal to put it in a separate patch so
> that 
> each type of change gets its own commit message.

Ah, right. I added a missing "to" and changed "to specify a specific".


> > > For a clean commit checkout my mental model is not one of anything
> > > new
> > > being
> > > actively staged i.e. different from what was in the commit.
> > Note that this is not about something like `git checkout 925b29` but
> > about
> > something like
> > `git checkout 925b29 src`.
> Yes, that was the part I was getting at.
> 
> The commit message (and the patch context) doesn't quite give enough
> to see 
> that is is particular to the -
> 
> git checkout [-p|--patch] [<tree-ish>] [--] [<paths>…]invocation.

It's: git checkout [-p|--patch] [<tree-ish>] [--] <pathspec>...

The paths aren't optional. Added it to the commit message:


While technically in the documentation, the fact that changes
introduced by a `git checkout [-p|--patch] [<tree-ish>] [--]
<pathspec>...` are staged automatically, was not obvious when
reading its documentation. It is now specifically pointed out.

Related sentence cleaned up.

Signed-off-by: Christoph Michelbach <michelbach94@gmail.com>
---
 Documentation/git-checkout.txt | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-
checkout.txt
index 8e2c066..cfd7b18 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -85,9 +85,10 @@ Omitting <branch> detaches HEAD at the tip of the
current branch.
        from the index file or from a named <tree-ish> (most often a
        commit).  In this case, the `-b` and `--track` options are
        meaningless and giving either of them results in an error.  The
-       <tree-ish> argument can be used to specify a specific tree-ish
-       (i.e.  commit, tag or tree) to update the index for the given
-       paths before updating the working tree.
+       <tree-ish> argument can be used to specify the tree-ish (i.e.
+       commit, tag, or tree) to update the index to for the given paths
+       before updating the working tree accordingly.  Note that this
means
+       that the changes this command introduces are staged
automatically.
 +
 'git checkout' with <paths> or `--patch` is used to restore modified or
 deleted paths to their original contents from the index or replace
paths
-- 
2.7.4


--
Christoph

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Philip Oakley]

From: "Christoph Michelbach" <michelbach94@gmail.com>
> On Sun, 2017-04-16 at 19:03 +0100, Philip Oakley wrote:
>> From: "Christoph Michelbach" <michelbach94@gmail.com>
>> >
>> > On Sun, 2017-04-16 at 00:28 +0100, Philip Oakley wrote:
>> > >
>> > > From: "Christoph Michelbach" <michelbach94@gmail.com>
>> > > >
>> > > >
>> > > > While technically in the documentation, the fact that changes
>> > > > introduced by a checkout <tree-ish> are staged automatically,
>> > > > was
>> > > > not obvious when reading its documentation. It is now
>> > > > specifically
>> > > > pointed out.
>> > > >
>> > > > Signed-off-by: Christoph Michelbach <michelbach94@gmail.com>
>> > > > ---
>> > > > Documentation/git-checkout.txt | 7 ++++---
>> > > > 1 file changed, 4 insertions(+), 3 deletions(-)
>> > > >
>> > > > diff --git a/Documentation/git-checkout.txt
>> > > > b/Documentation/git-checkout.txt
>> > > > index 8e2c066..cfd7b18 100644
>> > > > --- a/Documentation/git-checkout.txt
>> > > > +++ b/Documentation/git-checkout.txt
>> > > > @@ -85,9 +85,10 @@ Omitting <branch> detaches HEAD at the tip of
>> > > > the
>> > > > current branch.
>> > > > from the index file or from a named <tree-ish> (most often a
>> > > > commit). In this case, the `-b` and `--track` options are
>> > > > meaningless and giving either of them results in an error. The
>> > > > - <tree-ish> argument can be used to specify a specific tree-ish
>> > > > - (i.e. commit, tag or tree) to update the index for the given
>> > > Do these lines above actually need reflowing? Their content hasn't
>> > > changed
>> > > making it more difficult to spot the significant changes below
>> > > here.
>> > They're just part of the context of the automatically created patch.
>> The lines with +/- markings are the actual change lines. It's the
>> lines
>> without them that are the context lines.
>>
>> It does sound like an accidental reflow where the "(i.e." moved
>> between
>> lines, and the "paths" moved for the following lines.
>>
>> If reflowing is required it's normal to put it in a separate patch so
>> that
>> each type of change gets its own commit message.
>
> Ah, right. I added a missing "to" and changed "to specify a specific".
>
>
>> > > For a clean commit checkout my mental model is not one of anything
>> > > new
>> > > being
>> > > actively staged i.e. different from what was in the commit.
>> > Note that this is not about something like `git checkout 925b29` but
>> > about
>> > something like
>> > `git checkout 925b29 src`.
>> Yes, that was the part I was getting at.
>>
>> The commit message (and the patch context) doesn't quite give enough
>> to see
>> that is is particular to the -
>>
>> git checkout [-p|--patch] [<tree-ish>] [--] [<paths>…]
>> invocation.
>
> It's: git checkout [-p|--patch] [<tree-ish>] [--] <pathspec>...

The one I quoted is direct from the Synopsis, which does indicate there are 
potentially more aspects to resolve, such as the influence of using the 
[-p|--patch] options.

It maybe that the paragraph / sentence that needs adjusting is;

'git checkout' with <paths> or `--patch` is used to restore modified or
deleted paths to their original contents from the index or replace paths
with the contents from a named <tree-ish> (most often a commit-ish).

and split it at the "or replace paths" option to pick out your specific 
case.

>
> The paths aren't optional. Added it to the commit message:
>
>
> While technically in the documentation, the fact that changes
> introduced by a `git checkout [-p|--patch] [<tree-ish>] [--]
> <pathspec>...` are staged automatically, was not obvious when
> reading its documentation. It is now specifically pointed out.
>
> Related sentence cleaned up.
>
> Signed-off-by: Christoph Michelbach <michelbach94@gmail.com>
> ---
> Documentation/git-checkout.txt | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-checkout.txt b/Documentation/git-
> checkout.txt
> index 8e2c066..cfd7b18 100644
> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -85,9 +85,10 @@ Omitting <branch> detaches HEAD at the tip of the
> current branch.
> from the index file or from a named <tree-ish> (most often a
> commit). In this case, the `-b` and `--track` options are
> meaningless and giving either of them results in an error. The
> - <tree-ish> argument can be used to specify a specific tree-ish

Ah yes, "specify a specific" does sound a bit tautological.

> - (i.e. commit, tag or tree) to update the index for the given
> - paths before updating the working tree.
> + <tree-ish> argument can be used to specify the tree-ish (i.e.
> + commit, tag, or tree) to update the index to for the given paths
> + before updating the working tree accordingly. Note that this
> means
> + that the changes this command introduces are staged
> automatically.
> +
> 'git checkout' with <paths> or `--patch` is used to restore modified or
> deleted paths to their original contents from the index or replace
> paths
> -- 
> 2.7.4
>
>
> --
> Christoph
> 

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Christoph Michelbach]

On Sun, 2017-04-16 at 22:25 +0100, Philip Oakley wrote:
> From: "Christoph Michelbach" <michelbach94@gmail.com>
> > It's: git checkout [-p|--patch] [<tree-ish>] [--] <pathspec>...
> The one I quoted is direct from the Synopsis, which does indicate
> there are 
> potentially more aspects to resolve, such as the influence of using
> the 
> [-p|--patch] options.

Oh, you are right. I didn't even notice the one in the synopsis doesn't
match the one further down. The one in the synopsis is wrong because
after removing the optional parameters, it's the same as the first one
in the synopsis, yet we expect very different behavior from them.


> It maybe that the paragraph / sentence that needs adjusting is;
> 
> 'git checkout' with <paths> or `--patch` is used to restore modified
> or
> deleted paths to their original contents from the index or replace
> paths
> with the contents from a named <tree-ish> (most often a commit-ish).
> 
> and split it at the "or replace paths" option to pick out your
> specific 
> case.

This one is confusing, too: Paths can lead to folders, yet folders whose
contents have been modified are not restored to their original contents
when executing that command. Only files are.

After reading the documentation and having never used the command
before, one would expect

    #!/bin/bash
    rm -Rf repo
    mkdir repo
    cd repo
    git init &> /dev/null
    mkdir folder
    echo a > folder/a
    git add -A
    git commit -m "Commit 1." &> /dev/null
    echo b > folder/b
    git add -A
    git commit -m "Commit 2." &> /dev/null
    echo c > folder/c
    git add -A
    git commit -m "Commit 3." &> /dev/null
    git checkout `git log --pretty=format:%H | tail -1` folder
    ls folder

to print `a`. However, it prints `a b c` because all of the files inside
`folder` which have been modified or deleted since (here: none) are
reset to their original state after the first commit, but `folder`
itself isn't. Yet, the only path which was passed to the command in
question is `folder`.

In my opinion, this command needs improved documentation (and the
synopsis needs to be fixed).


--
Christoph

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Philip Oakley]

From: "Christoph Michelbach" <michelbach94@gmail.com>
> On Sun, 2017-04-16 at 22:25 +0100, Philip Oakley wrote:
>> From: "Christoph Michelbach" <michelbach94@gmail.com>
>> > It's: git checkout [-p|--patch] [<tree-ish>] [--] <pathspec>...
>> The one I quoted is direct from the Synopsis, which does indicate
>> there are
>> potentially more aspects to resolve, such as the influence of using
>> the
>> [-p|--patch] options.
>
> Oh, you are right. I didn't even notice the one in the synopsis doesn't
> match the one further down. The one in the synopsis is wrong because
> after removing the optional parameters, it's the same as the first one
> in the synopsis, yet we expect very different behavior from them.
>
>
>> It maybe that the paragraph / sentence that needs adjusting is;
>>
>> 'git checkout' with <paths> or `--patch` is used to restore modified
>> or
>> deleted paths to their original contents from the index or replace
>> paths
>> with the contents from a named <tree-ish> (most often a commit-ish).
>>
>> and split it at the "or replace paths" option to pick out your
>> specific
>> case.
>
> This one is confusing, too: Paths can lead to folders, yet folders whose
> contents have been modified are not restored to their original contents
> when executing that command. Only files are.
>
> After reading the documentation and having never used the command
> before, one would expect
>
>    #!/bin/bash
>    rm -Rf repo
>    mkdir repo
>    cd repo
>    git init &> /dev/null
>    mkdir folder
>    echo a > folder/a
>    git add -A
>    git commit -m "Commit 1." &> /dev/null
>    echo b > folder/b
>    git add -A
>    git commit -m "Commit 2." &> /dev/null
>    echo c > folder/c
>    git add -A
>    git commit -m "Commit 3." &> /dev/null
>    git checkout `git log --pretty=format:%H | tail -1` folder
>    ls folder
>
> to print `a`. However, it prints `a b c` because all of the files inside
> `folder` which have been modified or deleted since (here: none) are
> reset to their original state after the first commit, but `folder`
> itself isn't. Yet, the only path which was passed to the command in
> question is `folder`.
>
> In my opinion, this command needs improved documentation (and the
> synopsis needs to be fixed).
>

I think this example is of a different kind of misunderstanding.

We are at commit 3, with a b c in the working directory and index, and then 
we ask to checkout certain specific files from the first commit 1, which 
contains the file a. That old file is extracted and replaces the file from 
commit 3.

As the file a is identical there is no change and we still have the original 
b and c files from commit c.

I'd guess that the misunderstanding is that you maybe thought that the whole 
directory would be reset to it's old state and the files b and c deleted, 
rather than just the named files present in that old commit being extracted. 
If we'd created and added a file d just before the checkout, what should 
have happened to d, and why?

>

Comming back to the mental model issue with the implicit staging of checked 
out paths, I suspect this is a because we have both a snapshot and a diff 
based mental model. Normally the distinction is natural. We have the 
snapshot from the last commit (or branch checkout) in the index, and then we 
have the two steps for additional changes we personally made, and then added 
added, that determine the diff(s).

However in this (git checkout <treeish> -- <paths>) case we don't get that 
two step option. There is IIUC no 'git copyout <treeish> -- <paths>' which 
simply copies older files into the current worktree without adding it to the 
index/staging area.

The confounding of the, both optional, [--patch] and [<paths>] in the same 
description doesn't make it any easier. Splitting their synopses and 
descriptions may be the way forward.

I also haven't used the --patch option directly so there may be more issues 
beneath the surface.

> --
> Christoph
>
>
--
Philip 

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Philip Oakley]

I've added back the list, as it was accidentally dropped.

From: "Christoph Michelbach" <michelbach94@gmail.com>
> On Mon, 2017-04-17 at 17:04 +0100, Philip Oakley wrote:
>> From: "Christoph Michelbach" <michelbach94@gmail.com>
>> >
>> > On Sun, 2017-04-16 at 22:25 +0100, Philip Oakley wrote:
>> > >
>> > > From: "Christoph Michelbach" <michelbach94@gmail.com>
>> > > >
>> > > > It's: git checkout [-p|--patch] [<tree-ish>] [--] <pathspec>...
>> > > The one I quoted is direct from the Synopsis, which does indicate
>> > > there are
>> > > potentially more aspects to resolve, such as the influence of
>> > > using
>> > > the
>> > > [-p|--patch] options.
>> > Oh, you are right. I didn't even notice the one in the synopsis
>> > doesn't
>> > match the one further down. The one in the synopsis is wrong because
>> > after removing the optional parameters, it's the same as the first
>> > one
>> > in the synopsis, yet we expect very different behavior from them.
>> >
>> >
>> > >
>> > > It maybe that the paragraph / sentence that needs adjusting is;
>> > >
>> > > 'git checkout' with <paths> or `--patch` is used to restore
>> > > modified
>> > > or
>> > > deleted paths to their original contents from the index or replace
>> > > paths
>> > > with the contents from a named <tree-ish> (most often a commit-
>> > > ish).
>> > >
>> > > and split it at the "or replace paths" option to pick out your
>> > > specific
>> > > case.
>> > This one is confusing, too: Paths can lead to folders, yet folders
>> > whose
>> > contents have been modified are not restored to their original
>> > contents
>> > when executing that command. Only files are.
>> >
>> > After reading the documentation and having never used the command
>> > before, one would expect
>> >
>> > #!/bin/bash
>> > rm -Rf repo
>> > mkdir repo
>> > cd repo
>> > git init &> /dev/null
>> > mkdir folder
>> > echo a > folder/a
>> > git add -A
>> > git commit -m "Commit 1." &> /dev/null
>> > echo b > folder/b
>> > git add -A
>> > git commit -m "Commit 2." &> /dev/null
>> > echo c > folder/c
>> > git add -A
>> > git commit -m "Commit 3." &> /dev/null
>> > git checkout `git log --pretty=format:%H | tail -1` folder
>> > ls folder
>> >
>> > to print `a`. However, it prints `a b c` because all of the files
>> > inside
>> > `folder` which have been modified or deleted since (here: none) are
>> > reset to their original state after the first commit, but `folder`
>> > itself isn't. Yet, the only path which was passed to the command in
>> > question is `folder`.
>> >
>> > In my opinion, this command needs improved documentation (and the
>> > synopsis needs to be fixed).
>> >
>> I think this example is of a different kind of misunderstanding.
>>
>> We are at commit 3, with a b c in the working directory and index, and
>> then
>> we ask to checkout certain specific files from the first commit 1,
>> which
>> contains the file a. That old file is extracted and replaces the file
>> from
>> commit 3.
>>
>> As the file a is identical there is no change and we still have the
>> original
>> b and c files from commit c.
>>
>> I'd guess that the misunderstanding is that you maybe thought that the
>> whole
>> directory would be reset to it's old state and the files b and c
>> deleted,
>> rather than just the named files present in that old commit being
>> extracted.
>> If we'd created and added a file d just before the checkout, what
>> should
>> have happened to d, and why?
>
> I understand what the command does. It behaves perfectly as I expected
> it to. I did not find this script but wrote it to demonstrate that what
> the documentation says is different from how it behaves after having
> read what the documentation says it should do and noticing that that's
> not how I expected it to work from experience.
>
> What it really does is to copy all files described by the given paths
> from the given tree-ish to the working directory. Or at least that's my
> expectation of what it does.
>
> The documentation, however, says that the given paths are *restored*.
> This is different.

I don't see that difference in the phrase *restored*, compared to your 'copy 
all files described by the given paths'. Could you explain a little more?

>
> To answer your question: Nothing should happen to the file `d`. I stated
> what I genuinely believe to be true above: "What it really does is to
> copy all files described by the given paths from the given tree-ish to
> the working directory." If we take this and apply it, we see that `d` is
> not touched because there is nothing in the given tree-ish that could
> override it because `d` is not in the index.
>
> If we, however, take the existing documentation and apply it, `d` is
> removed if and only if a path leading to `d` (like `d` if `d` is in the
> root of the repo or `folder` or `folder/d` if it is in the folder
> `folder`) is passed as an argument.
>
>
>> Comming back to the mental model issue with the implicit staging of
>> checked
>> out paths, I suspect this is a because we have both a snapshot and a
>> diff
>> based mental model. Normally the distinction is natural. We have the
>> snapshot from the last commit (or branch checkout) in the index, and
>> then we
>> have the two steps for additional changes we personally made, and then
>> added
>> added, that determine the diff(s).
>>
>> However in this (git checkout <treeish> -- <paths>) case we don't get
>> that
>> two step option. There is IIUC no 'git copyout <treeish> -- <paths>'
>> which
>> simply copies older files into the current worktree without adding it
>> to the
>> index/staging area.
>
> Well, technically we can `git reset` to that point, then `git checkout-
> index` where the index is already up-to-date with the state of the
> working tree we want regarding the files we care about, and then `git
> reset` back, but I don't think there is a single command for that,
> either.
>
>
>> The confounding of the, both optional, [--patch] and [<paths>] in the
>> same
>> description doesn't make it any easier. Splitting their synopses and
>> descriptions may be the way forward.
>>
>> I also haven't used the --patch option directly so there may be more
>> issues
>> beneath the surface.
>
> Yeah, definitely. There should be 2 separate entries for this.
>
> I think someone thought it was a good idea to make `<pathspec>...`
> optional in the synopsis because `git checkout` behaves in that special
> way if a patch *or* paths are given. But then, of course, with both `-
> p|--patch` and `<pathspec>...` optional, the command is the same as the
> first variation, just with optional parameters -- but the documentation
> (correctly) says those variations should behave very differently from
> each other.
>
> I don't see how this can be avoided without having 2 separate entries
> for those cases.

true.

--
Philip 

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

>>> If we'd created and added a file d just before the checkout, what
>>> should
>>> have happened to d, and why?
>>
>> I understand what the command does. It behaves perfectly as I expected
>> it to. I did not find this script but wrote it to demonstrate that what
>> the documentation says is different from how it behaves after having
>> read what the documentation says it should do and noticing that that's
>> not how I expected it to work from experience.
>>
>> What it really does is to copy all files described by the given paths
>> from the given tree-ish to the working directory. Or at least that's my
>> expectation of what it does.
>>
>> The documentation, however, says that the given paths are *restored*.
>> This is different.
>
> I don't see that difference in the phrase *restored*, compared to your
> 'copy all files described by the given paths'. Could you explain a
> little more?

I am obviously not Christoph, and I was the one that defined how
"checkout <tree> -- <pathspec>" should work, but when you say
"restore" (which is not what I wrote ;-)) it is fair to expect lack
of 'd' could also be "restored", in addition to path that was in the
directory.

Obviously, "grab all paths that match <pathspec> out of <tree>, add
them to the index and copy them out to the working tree" will never
be able to _restore_ the lack of 'd', even it may match the
<pathspec> being used to do this checkout, by removing it from the
current index and the working tree.

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Christoph Michelbach]

On Mon, 2017-04-17 at 17:31 -0700, Junio C Hamano wrote:
> "Philip Oakley" <philipoakley@iee.org> writes:
> 
> > 
> > > 
> > > > 
> > > > If we'd created and added a file d just before the checkout,
> > > > what
> > > > should
> > > > have happened to d, and why?
> > > I understand what the command does. It behaves perfectly as I
> > > expected
> > > it to. I did not find this script but wrote it to demonstrate that
> > > what
> > > the documentation says is different from how it behaves after
> > > having
> > > read what the documentation says it should do and noticing that
> > > that's
> > > not how I expected it to work from experience.
> > > 
> > > What it really does is to copy all files described by the given
> > > paths
> > > from the given tree-ish to the working directory. Or at least
> > > that's my
> > > expectation of what it does.
> > > 
> > > The documentation, however, says that the given paths are
> > > *restored*.
> > > This is different.
> > I don't see that difference in the phrase *restored*, compared to
> > your
> > 'copy all files described by the given paths'. Could you explain a
> > little more?
> I am obviously not Christoph, and I was the one that defined how
> "checkout <tree> -- <pathspec>" should work, but when you say
> "restore" (which is not what I wrote ;-)) it is fair to expect lack
> of 'd' could also be "restored", in addition to path that was in the
> directory.

Yes, you didn't write "restore" but you wrote: "It updates the named
paths in the working tree from the index file or from a named <tree-ish> 
(most often a commit)."

I suppose that from reading this, some people will assume that `d` is
gone after the command was executed because the folder to which the
given path leads is updated "in the working tree from the index file or
from a named <tree-ish>", and others will not be sure what happens. But
I doubt that a lot of people guess the behavior correctly and are sure
about them being correct.

Note that for the first group of people, it doesn't matter that git
doesn't track folders. You can restore a folder "in the working tree
from the index file or from a named <tree-ish>" without tracking folders
because there only is one property of a folder git cares about, apart
from it itself being able to access files in that folder: Its name. And
it can get that name from the paths of the files in that folder. If
there are no files in that folder, there isn't a contradiction, either,
because then there is no folder in the index or tree-ish which can
possibly be restored.


> Obviously, "grab all paths that match <pathspec> out of <tree>, add
> them to the index and copy them out to the working tree" will never
> be able to _restore_ the lack of 'd', even it may match the
> <pathspec> being used to do this checkout, by removing it from the
> current index and the working tree.

Exactly. "grab all paths that match <pathspec> out of <tree>, add them
to the index and copy them out to the working tree" is a more accurate
description of what the command does (but it might need some rewording
;-) ).

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Junio C Hamano]

Christoph Michelbach <michelbach94@gmail.com> writes:

> On Mon, 2017-04-17 at 17:31 -0700, Junio C Hamano wrote:
>
>> Obviously, "grab all paths that match <pathspec> out of <tree>, add
>> them to the index and copy them out to the working tree" will never
>> be able to _restore_ the lack of 'd', even it may match the
>> <pathspec> being used to do this checkout, by removing it from the
>> current index and the working tree.
>
> Exactly. "grab all paths that match <pathspec> out of <tree>, add them
> to the index and copy them out to the working tree" is a more accurate
> description of what the command does (but it might need some rewording
> ;-) ).

Of course it is accurate, as that is how I would write it, not
"restore", if I were doing the documentation.

Care to send in a patch to update the documentation?

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Christoph Michelbach]

On Tue, 2017-04-18 at 18:40 -0700, Junio C Hamano wrote:
> Christoph Michelbach <michelbach94@gmail.com> writes:
> 
> > 
> > On Mon, 2017-04-17 at 17:31 -0700, Junio C Hamano wrote:
> > 
> > > 
> > > Obviously, "grab all paths that match <pathspec> out of <tree>,
> > > add
> > > them to the index and copy them out to the working tree" will
> > > never
> > > be able to _restore_ the lack of 'd', even it may match the
> > > <pathspec> being used to do this checkout, by removing it from the
> > > current index and the working tree.
> > Exactly. "grab all paths that match <pathspec> out of <tree>, add
> > them
> > to the index and copy them out to the working tree" is a more
> > accurate
> > description of what the command does (but it might need some
> > rewording
> > ;-) ).
> Of course it is accurate, as that is how I would write it, not
> "restore", if I were doing the documentation.
> 
> Care to send in a patch to update the documentation?

From 10c362b0632255f90e0975fb6656feedca5fd407 Mon Sep 17 00:00:00 2001
From: Christoph Michelbach <michelbach94@gmail.com>
Date: Sat, 22 Apr 2017 18:49:57 +0200
Subject: [PATCH] Doc./git-checkout: correct doc. of checkout
<pathspec>...

The previous documentation states that the named paths are
updated both in the index and the working tree. This is not
correct as those paths can point to folders which are not
updated to what they are in the given tree-ish. Rather,
the files pointed to by the pathspecs are copied from the
tree-ish to the index and working tree One difference being
that one can name paths which are not present in the working
tree and another being that only files which are already
present in the given tree-ish are affected.

Furthermore, the changed text is intended to be more
intelligible as it is written in a way which is easy to
follow, rather than describing the technical process in
order of execution.

A hint alerting the users that changes introduced by this
command when naming a tree-ish are automatically staged has
been introduced.

Signed-off-by: Christoph Michelbach <michelbach94@gmail.com>
---
 Documentation/git-checkout.txt | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-
checkout.txt
index 8e2c066..f74f237 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -81,13 +81,15 @@ Omitting <branch> detaches HEAD at the tip of the
current branch.
 'git checkout' [-p|--patch] [<tree-ish>] [--] <pathspec>...::
 
 	When <paths> or `--patch` are given, 'git checkout' does *not*
-	switch branches.  It updates the named paths in the working
tree
-	from the index file or from a named <tree-ish> (most often a
-	commit).  In this case, the `-b` and `--track` options are
-	meaningless and giving either of them results in an error.  The
-	<tree-ish> argument can be used to specify a specific tree-ish
-	(i.e.  commit, tag or tree) to update the index for the given
-	paths before updating the working tree.
+	switch branches.  It copies the files matching the pathspecs in
+	<tree-is> (i.e. a commit, tag, or tree) to the index and
+	subsequently to the working directory, overwriting changes
+	(including deletion of files) in those files.  In this case,
the
+	-b and --track options are meaningless and giving either of
them
+	results in an error.  Note that because the index is updated,
the
+	changes introduced by this command are automatically
staged.  If
+	no tree-ish is provided, the current index is used and remains
+	unchanged, modifying only the working directory.
 +
 'git checkout' with <paths> or `--patch` is used to restore modified or
 deleted paths to their original contents from the index or replace
paths
-- 
2.7.4

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Junio C Hamano]

Christoph Michelbach <michelbach94@gmail.com> writes:

>> Care to send in a patch to update the documentation?

Thanks.  The attached patch text appears linewrapped, but I think
I'll be able to salvage, so forgetting about the formatting, here
are some comments.


> From: Christoph Michelbach <michelbach94@gmail.com>
> Date: Sat, 22 Apr 2017 18:49:57 +0200
> Subject: [PATCH] Doc./git-checkout: correct doc. of checkout
> <pathspec>...
>
> The previous documentation states that the named paths are
> updated both in the index and the working tree. This is not
> correct as those paths can point to folders which are not
> updated to what they are in the given tree-ish. Rather,
> the files pointed to by the pathspecs are copied from the

s/pointed to by/that match/

> tree-ish to the index and working tree One difference being

Missing full-stop after "tree".

> that one can name paths which are not present in the working
> tree ...

That one is not a difference, I would think.  If your current index
and working tree lack F, you give pathspec that match F, and the
tree-ish has that path F, that named path _is_ updated both in the
index and in the working tree.

> ... and another being that only files which are already
> present in the given tree-ish are affected.

This one indeed was missing piece of information.

> Signed-off-by: Christoph Michelbach <michelbach94@gmail.com>
> ---
>  Documentation/git-checkout.txt | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-checkout.txt b/Documentation/git-
> checkout.txt
> index 8e2c066..f74f237 100644
> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -81,13 +81,15 @@ Omitting <branch> detaches HEAD at the tip of the
> current branch.
>  'git checkout' [-p|--patch] [<tree-ish>] [--] <pathspec>...::
>  
>  	When <paths> or `--patch` are given, 'git checkout' does *not*
> +	switch branches.  It copies the files matching the pathspecs in
> +	<tree-is> (i.e. a commit, tag, or tree) to the index and

<tree-ish> is misspelled here.  Drop (i.e. ...) as (1) it is not
correct (a tag may not point at a tree-ish) and (2) "checkout" is
not a place to learn what a tree-ish is (glossary is).

> +	subsequently to the working directory, overwriting changes

Do we need to say "subsequently" when we are aiming to be more
intelligible by not describing the order of execution?  The end
result is that the blobs named by the pathspecs from the tree-ish
are checked out to the index and to the working tree at the same
time.

> +...	Note that because the index is updated, the
> +	changes introduced by this command are automatically staged.

This is redundant and unnecessary, I would say.  If you absolutely
need to say this, at least drop "automatically".  There is nothing
automatic about it.  The user is asking to checkout the named blobs
out of the tree-ish to the index and to the working tree, and Git is
merely doing as it was told.

In addition the description is fuzzy; what are "changes introduced
by this command" relative to?  If you did "checkout HEAD path" after
editing path in the working tree, is "reverting my edit" the
"changes introduced by this command"?  If you did "checkout HEAD
path" after editing path, "git add path" and then editing path
further, what are the "changes introduced by this command"?

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Christoph Michelbach]

On Sun, 2017-04-23 at 18:55 -0700, Junio C Hamano wrote:
> Christoph Michelbach <michelbach94@gmail.com> writes:
> > From: Christoph Michelbach <michelbach94@gmail.com>
> > Date: Sat, 22 Apr 2017 18:49:57 +0200
> > Subject: [PATCH] Doc./git-checkout: correct doc. of checkout
> > <pathspec>...
> > 
> > The previous documentation states that the named paths are
> > updated both in the index and the working tree. This is not
> > correct as those paths can point to folders which are not
> > updated to what they are in the given tree-ish. Rather,
> > the files pointed to by the pathspecs are copied from the
> s/pointed to by/that match/

Ok.


> > 
> > tree-ish to the index and working tree One difference being
> Missing full-stop after "tree".

Oops.


> > 
> > that one can name paths which are not present in the working
> > tree ...
> That one is not a difference, I would think.  If your current index
> and working tree lack F, you give pathspec that match F, and the
> tree-ish has that path F, that named path _is_ updated both in the
> index and in the working tree.

I don't think it's inherently obvious that you can name paths which don't
occur in the working tree if it's stated that those are updated. For
something to be updated, I suppose it has to exist in the first place. But I
don't really want to discuss the commit message as everyone even reading it
already knows how the command works so I just removed it.


From 24a5ca5affbe8a09413b72ceb3cb50ae2fe9fd2b Mon Sep 17 00:00:00 2001
From: Christoph Michelbach <michelbach94@gmail.com>
Date: Sat, 22 Apr 2017 18:49:57 +0200
Subject: [PATCH] Doc./git-checkout: correct doc. of checkout <pathspec>...

The previous documentation states that the named paths are
updated both in the index and the working tree. This is not
correct as those paths can point to folders which are not
updated to what they are in the given tree-ish. Rather,
the files which match the pathspecs are copied from the
tree-ish to the index and working tree. The difference being
that only files which are already present in the given
tree-ish are affected.

Furthermore, the changed text is intended to be more
intelligible as it is written in a way which is easy to
follow, rather than describing the technical process in
order of execution.

A hint alerting the users that changes introduced by this
command when naming a tree-ish are automatically staged has
been introduced.

Signed-off-by: Christoph Michelbach <michelbach94@gmail.com>
---
 Documentation/git-checkout.txt | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 8e2c066..306b9d2 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -81,13 +81,15 @@ Omitting <branch> detaches HEAD at the tip of the
current branch.
 'git checkout' [-p|--patch] [<tree-ish>] [--] <pathspec>...::
 
 	When <paths> or `--patch` are given, 'git checkout' does *not*
-	switch branches.  It updates the named paths in the working tree
-	from the index file or from a named <tree-ish> (most often a
-	commit).  In this case, the `-b` and `--track` options are
-	meaningless and giving either of them results in an error.  The
-	<tree-ish> argument can be used to specify a specific tree-ish
-	(i.e.  commit, tag or tree) to update the index for the given
-	paths before updating the working tree.
+	switch branches.  It copies the files matching the pathspecs in
+	<tree-is> (i.e. a commit, tag, or tree) to the index and
+	subsequently to the working tree, overwriting changes
+	(including deletion of files) in those files.  In this case, the
+	-b and --track options are meaningless and giving either of them
+	results in an error.  Note that because the index is updated, the
+	changes introduced by this command are automatically staged.  If
+	no tree-ish is provided, the current index is used and remains
+	unchanged, modifying only the working tree.
 +
 'git checkout' with <paths> or `--patch` is used to restore modified or
 deleted paths to their original contents from the index or replace paths
-- 
2.7.4

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Christoph Michelbach]

I'm sorry, somehow my email client deleted half of your message when I saved it
when replying. I only noticed this when wanting to delete your email and noticed
that it was pretty long.

--------------------------------------------------------------------------------

On Sun, 2017-04-23 at 18:55 -0700, Junio C Hamano wrote:
> Christoph Michelbach <michelbach94@gmail.com> writes:
> > From: Christoph Michelbach <michelbach94@gmail.com>
> > Date: Sat, 22 Apr 2017 18:49:57 +0200
> > Subject: [PATCH] Doc./git-checkout: correct doc. of checkout
> > <pathspec>...
> > 
> > The previous documentation states that the named paths are
> > updated both in the index and the working tree. This is not
> > correct as those paths can point to folders which are not
> > updated to what they are in the given tree-ish. Rather,
> > the files pointed to by the pathspecs are copied from the
> s/pointed to by/that match/

Ok.


> > 
> > tree-ish to the index and working tree One difference being
> Missing full-stop after "tree".

Oops.


> > 
> > that one can name paths which are not present in the working
> > tree ...
> That one is not a difference, I would think.  If your current index
> and working tree lack F, you give pathspec that match F, and the
> tree-ish has that path F, that named path _is_ updated both in the
> index and in the working tree.

I don't think it's inherently obvious that you can name paths which don't
occur in the working tree if it's stated that those are updated. For
something to be updated, I suppose it has to exist in the first place. But I
don't really want to discuss the commit message as everyone even reading it
already knows how the command works so I just removed it.
> >  	When <paths> or `--patch` are given, 'git checkout' does *not*
> > +	switch branches.  It copies the files matching the pathspecs in
> > +	<tree-is> (i.e. a commit, tag, or tree) to the index and
> <tree-ish> is misspelled here.  Drop (i.e. ...) as (1) it is not
> correct (a tag may not point at a tree-ish) and (2) "checkout" is
> not a place to learn what a tree-ish is (glossary is).

Done.

> 
> > 
> > +	subsequently to the working directory, overwriting changes
> Do we need to say "subsequently" when we are aiming to be more
> intelligible by not describing the order of execution?  The end
> result is that the blobs named by the pathspecs from the tree-ish
> are checked out to the index and to the working tree at the same
> time.

I agree. Done.


> 
> > 
> > +...	Note that because the index is updated, the
> > +	changes introduced by this command are automatically staged.
> This is redundant and unnecessary, I would say.  If you absolutely
> need to say this, at least drop "automatically".  There is nothing
> automatic about it.  The user is asking to checkout the named blobs
> out of the tree-ish to the index and to the working tree, and Git is
> merely doing as it was told.

Done.


> In addition the description is fuzzy; what are "changes introduced
> by this command" relative to?  If you did "checkout HEAD path" after
> editing path in the working tree, is "reverting my edit" the
> "changes introduced by this command"?

All of those no changes which could possibly be staged are immediately staged.
All statements about all elements of the empty set are true.


> If you did "checkout HEAD
> path" after editing path, "git add path" and then editing path
> further, what are the "changes introduced by this command"?

The command can't introduce any further changes just because you edit files
after it has already terminated execution.



From fe0d1298cf4de841af994f4d9f72d49e25edea00 Mon Sep 17 00:00:00 2001
From: Christoph Michelbach <michelbach94@gmail.com>
Date: Sat, 22 Apr 2017 18:49:57 +0200
Subject: [PATCH] Doc./git-checkout: correct doc. of checkout <pathspec>...

The previous documentation states that the named paths are
updated both in the index and the working tree. This is not
correct as those paths can point to folders which are not
updated to what they are in the given tree-ish. Rather,
the files which match the pathspecs are copied from the
tree-ish to the index and working tree. The difference being
that only files which are already present in the given
tree-ish are affected.

Furthermore, the changed text is intended to be more
intelligible as it is written in a way which is easy to
follow, rather than describing the technical process in
order of execution.

A hint alerting the users that changes introduced by this
command when naming a tree-ish are automatically staged has
been introduced.

Signed-off-by: Christoph Michelbach <michelbach94@gmail.com>
---
 Documentation/git-checkout.txt | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 8e2c066..ea3b4df 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -81,13 +81,14 @@ Omitting <branch> detaches HEAD at the tip of the current
branch.
 'git checkout' [-p|--patch] [<tree-ish>] [--] <pathspec>...::
 
 	When <paths> or `--patch` are given, 'git checkout' does *not*
-	switch branches.  It updates the named paths in the working tree
-	from the index file or from a named <tree-ish> (most often a
-	commit).  In this case, the `-b` and `--track` options are
-	meaningless and giving either of them results in an error.  The
-	<tree-ish> argument can be used to specify a specific tree-ish
-	(i.e.  commit, tag or tree) to update the index for the given
-	paths before updating the working tree.
+	switch branches.  It copies the files matching the pathspecs in
+	<tree-ish> to the index and to the working tree, overwriting
+	changes	(including deletion of files) in those files.  In this
+	case, the -b and --track options are meaningless and giving either
+	of them results in an error.  Note that because the index is updated,
+	the changes introduced by this command are staged.  If no tree-ish is
+	provided, the current index is used and remains unchanged, modifying
+	only the working tree.
 +
 'git checkout' with <paths> or `--patch` is used to restore modified or
 deleted paths to their original contents from the index or replace paths
-- 
2.7.4

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Junio C Hamano]

Christoph Michelbach <michelbach94@gmail.com> writes:

> I'm sorry, somehow my email client deleted half of your message when I saved it
> when replying. I only noticed this when wanting to delete your email and noticed
> that it was pretty long.

Please separate your discussion from the patch proper.  Check
Documentation/SubmittingPatches and send a proper patch like other
people do.


> From fe0d1298cf4de841af994f4d9f72d49e25edea00 Mon Sep 17 00:00:00 2001
> From: Christoph Michelbach <michelbach94@gmail.com>
> Date: Sat, 22 Apr 2017 18:49:57 +0200
> Subject: [PATCH] Doc./git-checkout: correct doc. of checkout <pathspec>...

These we take from the e-mail header.  You usually remove them from
the body of the message (and move the Subject: to e-mail subject), hence

> The previous documentation states that the named paths are

this line will become the first line in the body of the message.

> A hint alerting the users that changes introduced by this
> command when naming a tree-ish are automatically staged has
> been introduced.

We are still saying automatically here?

> Signed-off-by: Christoph Michelbach <michelbach94@gmail.com>
> ---
>  Documentation/git-checkout.txt | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)

This is not limited to your message, but from time to time, I see
messages with SP substituted with non-breaking space like the above
two lines (and they appear in the patch text).  I can still read and
comment on the patch, but it is unusuable as an input to "git am" to
be applied.  I wonder where these NBSP come from---perhaps some
commmon MUAs corrupt text messages like this?

>
> diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
> index 8e2c066..ea3b4df 100644
> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -81,13 +81,14 @@ Omitting <branch> detaches HEAD at the tip of the current
> branch.
>  'git checkout' [-p|--patch] [<tree-ish>] [--] <pathspec>...::
>  
>  	When <paths> or `--patch` are given, 'git checkout' does *not*
> -	switch branches.  It updates the named paths in the working tree
> -	from the index file or from a named <tree-ish> (most often a
> -	commit).  In this case, the `-b` and `--track` options are
> -	meaningless and giving either of them results in an error.  The
> -	<tree-ish> argument can be used to specify a specific tree-ish
> -	(i.e.  commit, tag or tree) to update the index for the given
> -	paths before updating the working tree.
> +	switch branches.  It copies the files matching the pathspecs in

I am debating myself if rephrasing "in <tree-ish>" to "from
<tree-ish>" makes the result clearer to understand.  When we say
"Copy files from T to I and W", it would be obvious that what does
not exist in T will not be copied.

I also wonder "If no-tree-ish is provided" at the end of this
paragraph you have is a bit too far from the above primary point of
the description, because you have an unrelated "In this case,
-b/-t...", in between.  

	The blobs matching the pathspecs are checked out from the
	index to the working tree.  Optionally, when <tree-ish> is
	given, the blobs matching the pathspecs from the <tree-ish>
	is copied to the index before that happens.

is what I would want to say, but obviously that does not describe
what happens in the chronological order, so it is the most clear
description for people who understand what is written, but it will
take two reading until the reader gets to that stage X-<.

Perhaps the unrelated "In this case, the -b..." should go first; it
is part of "does *not* switch branches".  Also even with your patch,
it starts with "X is not Y" and does not clearly say "X is Z".

	When <paths> or `--patch` ar given, 'git checkout' does
	*not* switch branches (giving the `-b` and `--track` options
	will cause an error, as they are meaningless).  It checks
	out paths out of the <tree-ish> (if given) and the index to
	the to working tree.  When an optional <tree-ish> is given
	blobs in the <tree-ish> that match <pathspec> are copied to
	the index.  The blobs that match <pathspec> are then copied
	from the index to the working tree, overwriting what is in
	(or missing from) the working tree.

May be an improvement (i.e. say what Z is: checking out paths from
tree-ish and/or index to the working tree).  By explicitly phrasing
that <tree-ish>, from which the index is updated, is optional, it is
clear that without <tree-ish> there is no update to the index.
"missing from" covers two cases: (1) the user removed the file from
the working tree and <tree-ish>, e.g. HEAD, has the file, hence
removed one is resurrected; (2) the user didn't touch the file and
HEAD didn't have it, but by checking out from <tree-ish> that has
the file, the user added that new file to the set of files the user
is working with.

Hmm?

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Christoph Michelbach]

I give up. Bye.
On Mon, 2017-04-24 at 18:35 -0700, Junio C Hamano wrote:
> Christoph Michelbach <michelbach94@gmail.com> writes:
> 
> > 
> > I'm sorry, somehow my email client deleted half of your message when I saved
> > it
> > when replying. I only noticed this when wanting to delete your email and
> > noticed
> > that it was pretty long.
> Please separate your discussion from the patch proper.  Check
> Documentation/SubmittingPatches and send a proper patch like other
> people do.
> 
> 
> > 
> > From fe0d1298cf4de841af994f4d9f72d49e25edea00 Mon Sep 17 00:00:00 2001
> > From: Christoph Michelbach <michelbach94@gmail.com>
> > Date: Sat, 22 Apr 2017 18:49:57 +0200
> > Subject: [PATCH] Doc./git-checkout: correct doc. of checkout <pathspec>...
> These we take from the e-mail header.  You usually remove them from
> the body of the message (and move the Subject: to e-mail subject), hence
> 
> > 
> > The previous documentation states that the named paths are
> this line will become the first line in the body of the message.
> 
> > 
> > A hint alerting the users that changes introduced by this
> > command when naming a tree-ish are automatically staged has
> > been introduced.
> We are still saying automatically here?
> 
> > 
> > Signed-off-by: Christoph Michelbach <michelbach94@gmail.com>
> > ---
> >  Documentation/git-checkout.txt | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> This is not limited to your message, but from time to time, I see
> messages with SP substituted with non-breaking space like the above
> two lines (and they appear in the patch text).  I can still read and
> comment on the patch, but it is unusuable as an input to "git am" to
> be applied.  I wonder where these NBSP come from---perhaps some
> commmon MUAs corrupt text messages like this?
> 
> > 
> > 
> > diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
> > index 8e2c066..ea3b4df 100644
> > --- a/Documentation/git-checkout.txt
> > +++ b/Documentation/git-checkout.txt
> > @@ -81,13 +81,14 @@ Omitting <branch> detaches HEAD at the tip of the
> > current
> > branch.
> >  'git checkout' [-p|--patch] [<tree-ish>] [--] <pathspec>...::
> >  
> >  	When <paths> or `--patch` are given, 'git checkout' does *not*
> > -	switch branches.  It updates the named paths in the working tree
> > -	from the index file or from a named <tree-ish> (most often a
> > -	commit).  In this case, the `-b` and `--track` options are
> > -	meaningless and giving either of them results in an error.  The
> > -	<tree-ish> argument can be used to specify a specific tree-ish
> > -	(i.e.  commit, tag or tree) to update the index for the given
> > -	paths before updating the working tree.
> > +	switch branches.  It copies the files matching the pathspecs in
> I am debating myself if rephrasing "in <tree-ish>" to "from
> <tree-ish>" makes the result clearer to understand.  When we say
> "Copy files from T to I and W", it would be obvious that what does
> not exist in T will not be copied.
> 
> I also wonder "If no-tree-ish is provided" at the end of this
> paragraph you have is a bit too far from the above primary point of
> the description, because you have an unrelated "In this case,
> -b/-t...", in between.  
> 
> 	The blobs matching the pathspecs are checked out from the
> 	index to the working tree.  Optionally, when <tree-ish> is
> 	given, the blobs matching the pathspecs from the <tree-ish>
> 	is copied to the index before that happens.
> 
> is what I would want to say, but obviously that does not describe
> what happens in the chronological order, so it is the most clear
> description for people who understand what is written, but it will
> take two reading until the reader gets to that stage X-<.
> 
> Perhaps the unrelated "In this case, the -b..." should go first; it
> is part of "does *not* switch branches".  Also even with your patch,
> it starts with "X is not Y" and does not clearly say "X is Z".
> 
> 	When <paths> or `--patch` ar given, 'git checkout' does
> 	*not* switch branches (giving the `-b` and `--track` options
> 	will cause an error, as they are meaningless).  It checks
> 	out paths out of the <tree-ish> (if given) and the index to
> 	the to working tree.  When an optional <tree-ish> is given
> 	blobs in the <tree-ish> that match <pathspec> are copied to
> 	the index.  The blobs that match <pathspec> are then copied
> 	from the index to the working tree, overwriting what is in
> 	(or missing from) the working tree.
> 
> May be an improvement (i.e. say what Z is: checking out paths from
> tree-ish and/or index to the working tree).  By explicitly phrasing
> that <tree-ish>, from which the index is updated, is optional, it is
> clear that without <tree-ish> there is no update to the index.
> "missing from" covers two cases: (1) the user removed the file from
> the working tree and <tree-ish>, e.g. HEAD, has the file, hence
> removed one is resurrected; (2) the user didn't touch the file and
> HEAD didn't have it, but by checking out from <tree-ish> that has
> the file, the user added that new file to the set of files the user
> is working with.
> 
> Hmm?
> 

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>    Sent: Tuesday, April 18, 2017 
1:31 AM
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>>>> If we'd created and added a file d just before the checkout, what
>>>> should
>>>> have happened to d, and why?
>>>
>>> I understand what the command does. It behaves perfectly as I expected
>>> it to. I did not find this script but wrote it to demonstrate that what
>>> the documentation says is different from how it behaves after having
>>> read what the documentation says it should do and noticing that that's
>>> not how I expected it to work from experience.
>>>
>>> What it really does is to copy all files described by the given paths
>>> from the given tree-ish to the working directory. Or at least that's my
>>> expectation of what it does.
>>>
>>> The documentation, however, says that the given paths are *restored*.
>>> This is different.
>>
>> I don't see that difference in the phrase *restored*, compared to your
>> 'copy all files described by the given paths'. Could you explain a
>> little more?
>
> I am obviously not Christoph, and I was the one that defined how
> "checkout <tree> -- <pathspec>" should work, but when you say
> "restore" (which is not what I wrote ;-)) it is fair to expect lack
> of 'd' could also be "restored", in addition to path that was in the
> directory.
>
> Obviously, "grab all paths that match <pathspec> out of <tree>, add
> them to the index and copy them out to the working tree" will never
> be able to _restore_ the lack of 'd', even it may match the
> <pathspec> being used to do this checkout, by removing it from the
> current index and the working tree.
>
My attempt at asking about an additional file 'd' ended up being a bit of a 
red herring as it went off at a tangent.

Hopefully Christoph will hang in to help clarify the original issue. I think 
I'm getting a sense of the potential confusion between the different 
messages Git sends out, and which one is the right one here. On the one hand 
Git promotes the two step staging area approach, but on the other hand the 
checkout of a file suggests IIUC that all checkouts will end up as staged 
revisions (one step only).

Philip 

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Christoph Michelbach]

On Mon, 2017-04-17 at 21:59 +0100, Philip Oakley wrote:
> I've added back the list, as it was accidentally dropped.

Thanks. I'm sorry. I apparently pressed the wrong button in my emails
client. What I wrote is visible in your quote.


> From: "Christoph Michelbach" <michelbach94@gmail.com>
> > I understand what the command does. It behaves perfectly as I
> > expected
> > it to. I did not find this script but wrote it to demonstrate that
> > what
> > the documentation says is different from how it behaves after having
> > read what the documentation says it should do and noticing that
> > that's
> > not how I expected it to work from experience.
> > 
> > What it really does is to copy all files described by the given
> > paths
> > from the given tree-ish to the working directory. Or at least that's
> > my
> > expectation of what it does.
> > 
> > The documentation, however, says that the given paths are
> > *restored*.
> > This is different.
> I don't see that difference in the phrase *restored*, compared to your
> 'copy 
> all files described by the given paths'. Could you explain a little
> more?

Suppose you have X. If you say X is restored to what it was half an hour
ago, I expect everything about X to be exactly as it was half an hour
ago. So if X is a file containing the text "Hello, world!", I expect
there to be a file (with the exact same file name) which contains the
text "Hello, world!", even if I changed that text in the mean time or
deleted the file entirely. If X is a folder which contains files, I
expect it to have the exact same contents as before. If there were 5
files in the folder half an hour ago, I expect there to be 5 files with
the same file names and the same content in each file, again, even if I
deleted, renamed, changed the contents of, or added files in the mean
time. This is, however, not the case. Suppose I renamed a file from
"foo" to "bar". Then there are 6 files, not 5. This is not how X was
half an hour ago.

If you, however, say that all files in the path X are copied back from
what they were half an hour ago, if X is a file, I expect the exact same
thing as before for it. And that's actually what happens in reality. But
if X is a folder, there's a difference: Because only the *files X
contains* are copied back – *not X itself* –, all the files in X which
do not occur in the state from half an hour ago, are unaffected. So now
I expect there to be a 6th file because there is no file "bar" in the
state from half an hour ago which could overwrite the file "bar" in the
working tree.


> > Yeah, definitely. There should be 2 separate entries for this.
> > 
> > I think someone thought it was a good idea to make `<pathspec>...`
> > optional in the synopsis because `git checkout` behaves in that
> > special
> > way if a patch *or* paths are given. But then, of course, with both
> > `-
> > p|--patch` and `<pathspec>...` optional, the command is the same as
> > the
> > first variation, just with optional parameters -- but the
> > documentation
> > (correctly) says those variations should behave very differently
> > from
> > each other.
> > 
> > I don't see how this can be avoided without having 2 separate
> > entries
> > for those cases.
> true.

So now we have to find someone who used that command with both a patch
and a tree-ish present a lot because I have no idea how that would
behave and don't even know why you would use it.


--
Christoph

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> I'd guess that the misunderstanding is that you maybe thought that the
> whole directory would be reset to it's old state and the files b and c
> deleted, rather than just the named files present in that old commit
> being extracted. If we'd created and added a file d just before the
> checkout, what should have happened to d, and why?

It probably is a bit unfair to call it "misunderstanding".  I've had
this entry in the "Leftover Bits" list for quite some time:

    git checkout $commit -- somedir may want to remove somedir/file that
    is not in $commit but is in the original index. Anybody who wants to
    do this needs to consider ramifications and devise transition plans.
    Cf. $gmane/234935

In the thread, this message:

https://public-inbox.org/git/xmqqeh8nxltc.fsf@gitster.dls.corp.google.com/

may be a good summary.

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Christoph Michelbach]

On Mon, 2017-04-17 at 17:26 -0700, Junio C Hamano wrote:
> "Philip Oakley" <philipoakley@iee.org> writes:
> 
> > 
> > I'd guess that the misunderstanding is that you maybe thought that
> > the
> > whole directory would be reset to it's old state and the files b and
> > c
> > deleted, rather than just the named files present in that old commit
> > being extracted. If we'd created and added a file d just before the
> > checkout, what should have happened to d, and why?
> It probably is a bit unfair to call it "misunderstanding".  I've had
> this entry in the "Leftover Bits" list for quite some time:
> 
>     git checkout $commit -- somedir may want to remove somedir/file
> that
>     is not in $commit but is in the original index. Anybody who wants
> to
>     do this needs to consider ramifications and devise transition
> plans.
>     Cf. $gmane/234935
> 
> In the thread, this message:
> 
> https://public-inbox.org/git/xmqqeh8nxltc.fsf@gitster.dls.corp.google.
> com/
> 
> may be a good summary.

For clarification: I'm not saying that that's how the command should
behave. I merely want the documentation to describe how the command
actually behaves.

Of course, being able to reset a folder to a previous state would be
great, too. I have needed that several times and what I ended up doing
is to delete the folder and then do the checkout. This doesn't cause
problems if there are no untracked files (as of the state one wants to
restore; what's currently tracked is of course irrelevant) you still
need in that folder. If that's the case, one must make sure to not
delete those files.

I don't know whether there's an easier/better way, though. That's merely
what I did.

Having the *option* to reset an entire folder rather than the files in
it would be great.


--
Christoph

Re: [PATCH] Documentation/git-checkout: make doc. of checkout <tree-ish> clearer

[Philip Oakley]

Thanks,

The link does contatin the ref to the original commit, which is helpful. 
(expands to https://github.com/git/git/commit/0a1283bc3955a97557)

Philip
[I'll be off-line all day]
----- Original Message ----- 
From: "Junio C Hamano" <gitster@pobox.com>
To: "Philip Oakley" <philipoakley@iee.org>
Cc: "Christoph Michelbach" <michelbach94@gmail.com>; "Git Mailing List" 
<git@vger.kernel.org>
Sent: Tuesday, April 18, 2017 1:26 AM
Subject: Re: [PATCH] Documentation/git-checkout: make doc. of checkout 
<tree-ish> clearer


> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> I'd guess that the misunderstanding is that you maybe thought that the
>> whole directory would be reset to it's old state and the files b and c
>> deleted, rather than just the named files present in that old commit
>> being extracted. If we'd created and added a file d just before the
>> checkout, what should have happened to d, and why?
>
> It probably is a bit unfair to call it "misunderstanding".  I've had
> this entry in the "Leftover Bits" list for quite some time:
>
>    git checkout $commit -- somedir may want to remove somedir/file that
>    is not in $commit but is in the original index. Anybody who wants to
>    do this needs to consider ramifications and devise transition plans.
>    Cf. $gmane/234935
>
> In the thread, this message:
>
> https://public-inbox.org/git/xmqqeh8nxltc.fsf@gitster.dls.corp.google.com/
>
> may be a good summary.
>