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

["Philip Oakley" <philipoakley@iee.org>]

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