git commit -p with file arguments

git commit -p with file arguments

[Christian Neukirchen]

Hi,

I noticed the following suprising behavior:

% git --version
git version 2.10.0

% git add bar
% git status -s 
A  bar
 M foo

% git commit -p foo
[stage a hunk]
...
# Explicit paths specified without -i or -o; assuming --only paths...
# On branch master
# Changes to be committed:
#       new file:   bar
#       modified:   foo
#

So why does it want to commit bar too, when I explicitly wanted to
commit foo only?

This is not how "git commit files..." works, and the man page says

            3.by listing files as arguments to the commit command, in which
           case the commit will ignore changes staged in the index, and
           instead record the current content of the listed files (which must
           already be known to Git);

I'd expect "git commit -p files..." to work like
"git add -p files... && git commit files...".

Thanks,
-- 
Christian Neukirchen  <chneukirchen@gmail.com>  http://chneukirchen.org

Re: git commit -p with file arguments

[Duy Nguyen]

On Tue, Sep 6, 2016 at 4:08 AM, Christian Neukirchen
<chneukirchen@gmail.com> wrote:
> Hi,
>
> I noticed the following suprising behavior:
>
> % git --version
> git version 2.10.0
>
> % git add bar
> % git status -s
> A  bar
>  M foo
>
> % git commit -p foo
> [stage a hunk]
> ...
> # Explicit paths specified without -i or -o; assuming --only paths...
> # On branch master
> # Changes to be committed:
> #       new file:   bar
> #       modified:   foo
> #
>
> So why does it want to commit bar too, when I explicitly wanted to
> commit foo only?
>
> This is not how "git commit files..." works, and the man page says
>
>             3.by listing files as arguments to the commit command, in which
>            case the commit will ignore changes staged in the index, and
>            instead record the current content of the listed files (which must
>            already be known to Git);
>
> I'd expect "git commit -p files..." to work like
> "git add -p files... && git commit files...".

The paths after '-p' could mean two things, either as a filter (e.g.
like in "git add -p") to help save your time going through all changed
files, or as "git commit files...". I think the paths were meant to be
filter when '-p' was added. There's a separate bullet point git-commit
man page, number 5, in about --patch, so that paragraph you quoted is
probably _not_ about --patch. Either way changing its behavior now
might surprise users used to it.

At the least I think we should clarify this in the document. Maybe we
could add --patch-only as well, which commits just what you select in
--patch mode, ignoring anything in existing index.
-- 
Duy

Re: git commit -p with file arguments

[Jacob Keller]

On Mon, Sep 5, 2016 at 2:08 PM, Christian Neukirchen
<chneukirchen@gmail.com> wrote:
> Hi,
>
> I noticed the following suprising behavior:
>
> % git --version
> git version 2.10.0
>
> % git add bar
> % git status -s
> A  bar
>  M foo
>
> % git commit -p foo
> [stage a hunk]
> ...
> # Explicit paths specified without -i or -o; assuming --only paths...
> # On branch master
> # Changes to be committed:
> #       new file:   bar
> #       modified:   foo
> #
>
> So why does it want to commit bar too, when I explicitly wanted to
> commit foo only?

It wants to commit bar too because you already added bar before. It works like:

"git add bar && git add -p foo && git commit" does it not?

I fail to see why "git commit -p <path>" would unstage the bar you
already added? Or am I missing some assumption here?

Thanks,
Jake

>
> This is not how "git commit files..." works, and the man page says
>
>             3.by listing files as arguments to the commit command, in which
>            case the commit will ignore changes staged in the index, and
>            instead record the current content of the listed files (which must
>            already be known to Git);
>
> I'd expect "git commit -p files..." to work like
> "git add -p files... && git commit files...".
>

I guess the part about "git commit files" is different from "git
commit -p files", which is confusing.

> Thanks,
> --
> Christian Neukirchen  <chneukirchen@gmail.com>  http://chneukirchen.org
>

Re: git commit -p with file arguments

[Christian Neukirchen]

Jacob Keller <jacob.keller@gmail.com> writes:

> It wants to commit bar too because you already added bar before. It works like:
>
> "git add bar && git add -p foo && git commit" does it not?
>
> I fail to see why "git commit -p <path>" would unstage the bar you
> already added? Or am I missing some assumption here?

Yet the commit message comment says:
# Explicit paths specified without -i or -o; assuming --only paths...

But files are committed which were not given on the command line.

My confusion is that I use "git commit" with explicit files, yet other
files are committed.  AFAICS, this only happens with -p.

-- 
Christian Neukirchen  <chneukirchen@gmail.com>  http://chneukirchen.org

Re: git commit -p with file arguments

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

> It wants to commit bar too because you already added bar before. It works like:
>
> "git add bar && git add -p foo && git commit" does it not?
>
> I fail to see why "git commit -p <path>" would unstage the bar you
> already added? Or am I missing some assumption here?

Yes.

"git commit -p <pathspec>" were added originally for lazy people who
do not want to type "git add -p <pathspec> && git commit", which
matches your expectation.  If you already added "bar" that is
outside of the <pathspec> given to "add -p", the final "git commit"
step would record the latest contents of "bar" in it.

For obvious reasons, "git commit -p <pathspec>" cannot be a
short-hand to "git add -p <pathspec> && git commit <pathspec>", so
the current behaviour was the best they could do for those who aded
"commit -p", I guess.

Re: git commit -p with file arguments

[Jakub Narębski]

W dniu 09.09.2016 o 20:03, Junio C Hamano pisze:
> Jacob Keller <jacob.keller@gmail.com> writes:
> 
>> It wants to commit bar too because you already added bar before. It works like:
>>
>> "git add bar && git add -p foo && git commit" does it not?
>>
>> I fail to see why "git commit -p <path>" would unstage the bar you
>> already added? Or am I missing some assumption here?
> 
> Yes.
> 
> "git commit -p <pathspec>" were added originally for lazy people who
> do not want to type "git add -p <pathspec> && git commit", which
> matches your expectation.  If you already added "bar" that is
> outside of the <pathspec> given to "add -p", the final "git commit"
> step would record the latest contents of "bar" in it.
> 
> For obvious reasons, "git commit -p <pathspec>" cannot be a
> short-hand to "git add -p <pathspec> && git commit <pathspec>", so
> the current behaviour was the best they could do for those who aded
> "commit -p", I guess.

The 'obvious reasons' are that 

  $ git add -p <pathspec> && git commit <pathspec>

would not work as intended, that is it wouldn't create commit out of
HEAD and changes to <pathspec> created interactively in the index.
"git commit <pathspec>" is a shortcut to "git commit --only <pathspec>";
the git-commit(1) manpage explains (emphasis mine):

 -o
 --only
    Make a commit by taking the updated *working tree contents* of
    the paths specified on the command line, disregarding any contents
    that have been staged for other paths. [...]

Which means that with "git add -p <pathspec> && git commit <pathspec>",
the "git add -p <pathspec>" would carefully craft the <pathspec> state
in the index... and "git commit <pathspec>" would take worktree version
of <pathspec> for commit, ignoring what was in the index :-(

Currently there is no way to create commit out of subset of the index,
e.g. with "git commit :0:<path>"

Best,
-- 
Jakub Narębski

Re: git commit -p with file arguments

[Christian Neukirchen]

Jakub Narębski <jnareb@gmail.com> writes:

> Which means that with "git add -p <pathspec> && git commit <pathspec>",
> the "git add -p <pathspec>" would carefully craft the <pathspec> state
> in the index... and "git commit <pathspec>" would take worktree version
> of <pathspec> for commit, ignoring what was in the index :-(
>
> Currently there is no way to create commit out of subset of the index,
> e.g. with "git commit :0:<path>"

I played around with creating a new index just for "add -p" and then
committing that one.  Seems to have worked...

Perhaps I'll just wrap git-commit myself then.

cu,
-- 
Christian Neukirchen  <chneukirchen@gmail.com>  http://chneukirchen.org

Re: git commit -p with file arguments

[Jakub Narębski]

W dniu 09.09.2016 o 22:52, Christian Neukirchen napisał:
> Jakub Narębski <jnareb@gmail.com> writes:
> 
>> Which means that with "git add -p <pathspec> && git commit <pathspec>",
>> the "git add -p <pathspec>" would carefully craft the <pathspec> state
>> in the index... and "git commit <pathspec>" would take worktree version
>> of <pathspec> for commit, ignoring what was in the index :-(
>>
>> Currently there is no way to create commit out of subset of the index,
>> e.g. with "git commit :0:<path>"
> 
> I played around with creating a new index just for "add -p" and then
> committing that one.  Seems to have worked...
> 
> Perhaps I'll just wrap git-commit myself then.

What I wanted to say is that there is no built-in way to create commit
out of subset of the index.  You can of course do what "git commit <file>"
does, that is use temporary index file (GIT_INDEX_FILE etc.; see
contrib/examples/git-commit.sh).

I wonder, if git-commit is to acquire such feature, what would be the
best interface.  "git commit :0:./<path>"?  "git commit -o -p <path>"
(that is, "git commit --only --patch <pathspec>")?

-- 
Jakub Narębski

Re: git commit -p with file arguments

[Junio C Hamano]

Jakub Narębski <jnareb@gmail.com> writes:

> I wonder, if git-commit is to acquire such feature, what would be the
> best interface.  "git commit :0:./<path>"?  "git commit -o -p <path>"
> (that is, "git commit --only --patch <pathspec>")?

Just do "git reset && git commit -p <pathspec>", I would say.
Anything more elaborate would just confuse the end users.

Re: git commit -p with file arguments

[Jacob Keller]

On Sun, Sep 11, 2016 at 2:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jakub Narębski <jnareb@gmail.com> writes:
>
>> I wonder, if git-commit is to acquire such feature, what would be the
>> best interface.  "git commit :0:./<path>"?  "git commit -o -p <path>"
>> (that is, "git commit --only --patch <pathspec>")?
>
> Just do "git reset && git commit -p <pathspec>", I would say.
> Anything more elaborate would just confuse the end users.
>

Yes, I'm actually confused by "git commit <files>" *not* usinng what's
in the index already, so I think that isn't intuitive as is.

Thanks,
Jake

Re: git commit -p with file arguments

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

> Yes, I'm actually confused by "git commit <files>" *not* usinng what's
> in the index already, so I think that isn't intuitive as is.

You are excused ;-)

In ancient days, "git commit <pathspec>" was to add the contents
from working tree files that match <pathspec> to what is already in
the index and create a commit from that state.  This ran against the
intuition of many users who knew older systems (e.g. cvs) and we had
to migrate it to the current behaviour by breaking backward
compatibility.

Re: git commit -p with file arguments

[Jacob Keller]

On Sun, Sep 11, 2016 at 6:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> You are excused ;-)
>
> In ancient days, "git commit <pathspec>" was to add the contents
> from working tree files that match <pathspec> to what is already in
> the index and create a commit from that state.  This ran against the
> intuition of many users who knew older systems (e.g. cvs) and we had
> to migrate it to the current behaviour by breaking backward
> compatibility.

I see.

Thanks,
Jake

Re: git commit -p with file arguments

[Jakub Narębski]

W dniu 12.09.2016 o 03:57, Junio C Hamano pisze:
> Jacob Keller <jacob.keller@gmail.com> writes:
> 
>> Yes, I'm actually confused by "git commit <files>" *not* usinng what's
>> in the index already, so I think that isn't intuitive as is.
> 
> You are excused ;-)
> 
> In ancient days, "git commit <pathspec>" was to add the contents
> from working tree files that match <pathspec> to what is already in
> the index and create a commit from that state.

That is, "git commit <pathspec>" meant --include, being equivalent to
"git commit --include <pathspec>".

>                                               This ran against the
> intuition of many users who knew older systems (e.g. cvs) and we had
> to migrate it to the current behaviour by breaking backward
> compatibility.

That is, "git commit <pathspec>" means --only, being equivalent to
"git commit --only <pathspec>".

But it was always about working tree version of <pathspec>.

And of course older version control systems didn't have the index...
-- 
Jakub Narębski

Re: git commit -p with file arguments

[Duy Nguyen]

On Fri, Sep 09, 2016 at 05:54:30PM +0700, Duy Nguyen wrote:
> On Tue, Sep 6, 2016 at 4:08 AM, Christian Neukirchen
> <chneukirchen@gmail.com> wrote:
> > Hi,
> >
> > I noticed the following suprising behavior:
> >
> > % git --version
> > git version 2.10.0
> >
> > % git add bar
> > % git status -s
> > A  bar
> >  M foo
> >
> > % git commit -p foo
> > [stage a hunk]
> > ...
> > # Explicit paths specified without -i or -o; assuming --only paths...
> > # On branch master
> > # Changes to be committed:
> > #       new file:   bar
> > #       modified:   foo
> > #
> >
> > So why does it want to commit bar too, when I explicitly wanted to
> > commit foo only?
> >
> > This is not how "git commit files..." works, and the man page says
> >
> >             3.by listing files as arguments to the commit command, in which
> >            case the commit will ignore changes staged in the index, and
> >            instead record the current content of the listed files (which must
> >            already be known to Git);
> >
> > I'd expect "git commit -p files..." to work like
> > "git add -p files... && git commit files...".
> 
> ...
> 
> At the least I think we should clarify this in the document.

How about something like this? Would it help?

-- 8< --
Subject: [PATCH] git-commit.txt: clarify --patch mode with pathspec

How pathspec is used, with and without --interactive/--patch, is
different. But this is not clear from the document. These changes hint
the user to keep reading (to option #5) instead of stopping at #2 and
assuming --patch/--interactive behaves the same way.

And since all the options listed here always mention how the index is
involved (or not) in the final commit, add that bit for #5 as well. This
"on top of the index" is implied when you head over git-add(1), but if
you just go straight to the "Interactive mode" and not read what git-add
is for, you may miss it.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-commit.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index b0a294d..f2ab0ee 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -29,7 +29,8 @@ The content to be added can be specified in several ways:
 2. by using 'git rm' to remove files from the working tree
    and the index, again before using the 'commit' command;
 
-3. by listing files as arguments to the 'commit' command, in which
+3. by listing files as arguments to the 'commit' command
+   (without --interactive or --patch switch), in which
    case the commit will ignore changes staged in the index, and instead
    record the current content of the listed files (which must already
    be known to Git);
@@ -41,7 +42,8 @@ The content to be added can be specified in several ways:
    actual commit;
 
 5. by using the --interactive or --patch switches with the 'commit' command
-   to decide one by one which files or hunks should be part of the commit,
+   to decide one by one which files or hunks should be part of the commit
+   in addition to contents in the index,
    before finalizing the operation. See the ``Interactive Mode'' section of
    linkgit:git-add[1] to learn how to operate these modes.
 
-- 
2.8.2.524.g6ff3d78

-- 8< --
Duy

Re: git commit -p with file arguments

[Christian Neukirchen]

Duy Nguyen <pclouds@gmail.com> writes:

>> At the least I think we should clarify this in the document.
>
> How about something like this? Would it help?

I think it captures the current behavior well.

-- 
Christian Neukirchen  <chneukirchen@gmail.com>  http://chneukirchen.org

Re: git commit -p with file arguments

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

>> At the least I think we should clarify this in the document.
>
> How about something like this? Would it help?
>
> -- 8< --
> Subject: [PATCH] git-commit.txt: clarify --patch mode with pathspec
>
> How pathspec is used, with and without --interactive/--patch, is
> different. But this is not clear from the document. These changes hint
> the user to keep reading (to option #5) instead of stopping at #2 and
> assuming --patch/--interactive behaves the same way.
>
> And since all the options listed here always mention how the index is
> involved (or not) in the final commit, add that bit for #5 as well. This
> "on top of the index" is implied when you head over git-add(1), but if
> you just go straight to the "Interactive mode" and not read what git-add
> is for, you may miss it.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Excellent.  

>  Documentation/git-commit.txt | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
> index b0a294d..f2ab0ee 100644
> --- a/Documentation/git-commit.txt
> +++ b/Documentation/git-commit.txt
> @@ -29,7 +29,8 @@ The content to be added can be specified in several ways:
>  2. by using 'git rm' to remove files from the working tree
>     and the index, again before using the 'commit' command;
>  
> -3. by listing files as arguments to the 'commit' command, in which
> +3. by listing files as arguments to the 'commit' command
> +   (without --interactive or --patch switch), in which
>     case the commit will ignore changes staged in the index, and instead
>     record the current content of the listed files (which must already
>     be known to Git);
> @@ -41,7 +42,8 @@ The content to be added can be specified in several ways:
>     actual commit;
>  
>  5. by using the --interactive or --patch switches with the 'commit' command
> -   to decide one by one which files or hunks should be part of the commit,
> +   to decide one by one which files or hunks should be part of the commit
> +   in addition to contents in the index,
>     before finalizing the operation. See the ``Interactive Mode'' section of
>     linkgit:git-add[1] to learn how to operate these modes.

When re-reading these 5 bullet points, I feel there may be some
unrelated updates needed to clarify (e.g. it is unclear when reading
the description pretending to be a newbie, if it is the "changes"
that is recorded in the index, or if it is the "state"; the answer
is the latter but if the reader's world model is still the former,
the description can mislead to expect a different behaviour).

But regardless of such remaining potential issues, this is a clearly
good change.  Thanks.