git commit fails under some circumstances

git commit fails under some circumstances

[Laszlo Papp]

Hi,

There are 2 bugs and 1 wish in this case:
1) It says "Changes to be committed:", but they are actually not
committed, there cannot even be committed since they were not added
"properly". This output is bogus
2) error: Error building trees
---
3) It would be nice to have one command (with no (git) alias for sure)
to see the difference like "git diff" but also the new files.

Best Regards,
Laszlo Papp

========

root /home/djszapi/Projects/kde/attica/build #  git add -N
../lib/achievement.cpp ../lib/achievement.h
../lib/achievementparser.cpp ../lib/achievementparser.h
root /home/djszapi/Projects/kde/attica/build #  git status
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#       new file:   ../lib/achievement.cpp
#       new file:   ../lib/achievement.h
#       new file:   ../lib/achievementparser.cpp
#       new file:   ../lib/achievementparser.h
#
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#       modified:   ../lib/CMakeLists.txt
#       modified:   ../lib/achievement.cpp
#       modified:   ../lib/achievement.h
#       modified:   ../lib/achievementparser.cpp
#       modified:   ../lib/achievementparser.h
#       modified:   ../lib/listjob_inst.cpp
#       modified:   ../lib/provider.cpp
#       modified:   ../lib/provider.h
#       modified:   ../lib/providermanager.cpp
#
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#       ./
root /home/djszapi/Projects/kde/attica/build #  gc
lib/achievement.cpp: not added yet
lib/achievement.h: not added yet
lib/achievementparser.cpp: not added yet
lib/achievementparser.h: not added yet
error: Error building trees
root /home/djszapi/Projects/kde/attica/build #  alias gc
alias gc='git commit --author="Laszlo Papp <djszapi@archlinux.us>"'
root /home/djszapi/Projects/kde/attica/build

Re* git commit fails under some circumstances

[Junio C Hamano]

Laszlo Papp <djszapi@archlinux.us> writes:

> There are 2 bugs and 1 wish in this case:
> 1) It says "Changes to be committed:", but they are actually not
> committed, there cannot even be committed since they were not added
> "properly". This output is bogus
> 2) error: Error building trees

Thanks.

You gave us a concise reproduction recipe and the output you get from it,
which is far better than what an average new user would do.  But that is
still two out of three of what makes a good bug report.

When you utter the word "bug" (or the word "bogus") to an existing
project, please make it a habit to mention what you think should happen as
well.  This is not limited to git but in any open source project you are
not familiar with in general.

It helps tremendously to have a reproduction recipe and its output to
diagnose certain kinds of issues.  If somebody else tries to run it, and
if it gave results different from the output you are getting, you may have
uncovered some unintended environment dependency bug.

But what if your output matches what the original developers expected to
see?  Possibly after spending many hours to design how the thing ought to
behave, it is possible that they still might not have thought about a
better behaviour in some cases, and you may have an idea to present the
result in a more useful way---and your complaint might be that the current
output does not match that idea of yours.  In such a case, a bug report
that does not state what should be shown stops short of being useful.

When running "commit" and "status" with files marked with "intent to add",
I think there are three possible interpretations of what the user wants to
do.

 (1) I earlier said "I'll decide the exact contents to be committed for
     these paths and tell you by running 'git add' later." when I said
     'git add -N'.  But I forgot to do so before running "git commit".
     Thanks for catching this mistake and erroring out.

 (2) I said "I'll decide the exact content to be committed ... later."
     when I said 'git add -N'. I am running "git commit" now, but I still
     don't know what the final contents for this path should be.  I
     changed my mind, and I do not want to include the addition of these
     paths in the commit I am making.  Please do not error out, but just
     ignore the earlier 'add -N' for now.

 (3) I said "I'll decide the exact content to be committed ... later."
     when I said 'git add -N'. I am running "git commit" now, without
     explicitly telling you with 'git add' about the final contents for
     these paths.  Please take it as an implicit hint that I am happy with
     the contents in the working tree and commit them as if I said 'git
     add' on these paths, but do leave modifications to already tracked
     paths that I haven't added with 'git add'.

The current behaviour of "git commit" that refuse to commit without
telling git what the final contents for these pathse is a very deliberate
design and implementation of (1).

The second one is a possible interpretation, but it is a very error prone
one.  People who want to commit without these new files wouldn't have
marked them with "add -N" in the first place, and (2) feels more like
disabling a useful error checking than making the command useful.  Also it
is a silent bug to behave like (2) to people who expect (3), as they will
only later find out that some of the additions were not committed against
their will.

The third one also is a possible interpretation, but it is a very
inconsistent one.  Why should new paths marked with 'git add -N' be
treated differently from paths that were already tracked and you made
changes to the working tree?  In addition, it is a silent bug to behave
like (3) to people who expect (2), as they will only later find out that
some of the additions were committed against their will.

Note that the user who may want to see variants of (2) and (3) can still
name which paths to include (and to exclude by not naming them):

    $ edit foo bar baz
    $ git add baz
    $ git add -N foo bar
    $ git commit baz foo

or include everything

    $ git commit -a


I thought the above issues through when I wrote "git add -N" and
determined that (1) is the only sane and useful behaviour, and I still
think it is.

Having said all that, we might want to differentiate these paths in the
output from 'status'.  The 'status' command in its current form and
semantics came much later (v1.7.0) than "add -N" (introduced in v1.6.1),
and because "add -N" is such a low-key corner case (adding new paths is
far less frequently done than modifying an existing paths, and showing
only intent to add new paths is even less frequently done), we probably
didn't think about special casing them.

Currently we show these paths as both added for commit (i.e. appears in
the top portion of the verbose output, and in the first column of the -s
output) and having local modification (i.e. appears in the second part or
in the second column, respectively).  For example, here is what I get:

    $ cp COPYING RENAMING
    $ git add -N RENAMING
    $ git status -s
    AM RENAMING
    $ git status
    # ... to be committed ...
    # new file: RENAMING
    # ... have local changes ...
    # modified: RENAMING

We should be able to say something like:

    $ git status -s
    IM RENAMING
    $ git status
    # ... to be committed ...
    # new file: RENAMING (needs 'git add')
    # ... have local changes ...
    # modified: RENAMING

if we don't care about the backward compatibility.

> 3) It would be nice to have one command (with no (git) alias for sure)
> to see the difference like "git diff" but also the new files.

Doesn't "git diff" show the difference for files you told git about via
the "add -N" interface?  After the above "addition" of RENAMING, if I ask
"git diff" (or "git diff HEAD"), I get what I expect to see (addition of
the contents taken from the whole file in the working tree).

Again, please describe what you think should be the right output if it
differs from your expectation.

Also having said that, I notice that "git diff --cached" behaves as if an
empty blob is added in such a case.  I am not sure if we want to special
case this.  After all paths marked with "git add -N" does _not_ have a
concrete contents in the index by definition (as the user told "I'll tell
you later" but hasn't done so), and may want to behave more like unmerged
entries for certain operations (i.e. the path does exist, but comparing
something with it does not have a usual meaning, etc.)

Re: Re* git commit fails under some circumstances

[Will Palmer]

On Sat, 2011-04-02 at 12:16 -0700, Junio C Hamano wrote:
> Laszlo Papp <djszapi@archlinux.us> writes:
> > 3) It would be nice to have one command (with no (git) alias for sure)
> > to see the difference like "git diff" but also the new files.
> 
> Doesn't "git diff" show the difference for files you told git about via
> the "add -N" interface?  After the above "addition" of RENAMING, if I ask
> "git diff" (or "git diff HEAD"), I get what I expect to see (addition of
> the contents taken from the whole file in the working tree).
> 
> Again, please describe what you think should be the right output if it
> differs from your expectation.
> 

To give some context: the problem here is that "git add -N" was being
used as a glorified way of saying "show the content of a file I have
just added in "git diff" output along with unstaged content", as is
described in the manual for "git add" as one of the purposes of -N. All
the other problems are somewhat invented issues stemming from this one.
That is, a result of blindly following instructions to "try git add -N
if you want to see the output in diff", without the implications having
been explained first.
The alternative, "git add everything else, then use git diff --cached" I
believe is unsuitable because the goal is to have "git diff" 'just work'
in future runs, without the need for additional commands being run.
Honestly I do not quite understand the exact use-case.

An option such as --treat-new-as-unstaged might solve this better, but
of course suffers from the problem of having a terrible name. I greatly
suspect that the name being terrible is a sign of the idea not being
fleshed-out enough yet. There are also various cases involving renames
where it would not be clear at all how to handle things.

-- Will

> Also having said that, I notice that "git diff --cached" behaves as if an
> empty blob is added in such a case.  I am not sure if we want to special
> case this.  After all paths marked with "git add -N" does _not_ have a
> concrete contents in the index by definition (as the user told "I'll tell
> you later" but hasn't done so), and may want to behave more like unmerged
> entries for certain operations (i.e. the path does exist, but comparing
> something with it does not have a usual meaning, etc.)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re* git commit fails under some circumstances

[Junio C Hamano]

Will Palmer <wmpalmer@gmail.com> writes:

> The alternative, "git add everything else, then use git diff --cached" I
> believe is unsuitable

It sounds as if you are looking for "git diff HEAD" to me...

Re: Re* git commit fails under some circumstances

[Will Palmer]

On Tue, 2011-04-05 at 01:18 -0700, Junio C Hamano wrote:
> Will Palmer <wmpalmer@gmail.com> writes:
> 
> > The alternative, "git add everything else, then use git diff --cached" I
> > believe is unsuitable
> 
> It sounds as if you are looking for "git diff HEAD" to me...
> 

Quite possible another case of "the mindset of someone used to using the
index, and so always wanting to reference it, clashing with the mindset
of someone who never uses the index, and so doesn't even consider it".
Quite possible this is indeed the solution that was needed.

Re: Re* git commit fails under some circumstances

[Jeff King]

On Sat, Apr 02, 2011 at 12:16:23PM -0700, Junio C Hamano wrote:

> When running "commit" and "status" with files marked with "intent to add",
> I think there are three possible interpretations of what the user wants to
> do.
> 
>  (1) I earlier said "I'll decide the exact contents to be committed for
>      these paths and tell you by running 'git add' later." when I said
>      'git add -N'.  But I forgot to do so before running "git commit".
>      Thanks for catching this mistake and erroring out.
> 
>  (2) I said "I'll decide the exact content to be committed ... later."
>      when I said 'git add -N'. I am running "git commit" now, but I still
>      don't know what the final contents for this path should be.  I
>      changed my mind, and I do not want to include the addition of these
>      paths in the commit I am making.  Please do not error out, but just
>      ignore the earlier 'add -N' for now.
> 
>  (3) I said "I'll decide the exact content to be committed ... later."
>      when I said 'git add -N'. I am running "git commit" now, without
>      explicitly telling you with 'git add' about the final contents for
>      these paths.  Please take it as an implicit hint that I am happy with
>      the contents in the working tree and commit them as if I said 'git
>      add' on these paths, but do leave modifications to already tracked
>      paths that I haven't added with 'git add'.
> 
> The current behaviour of "git commit" that refuse to commit without
> telling git what the final contents for these pathse is a very deliberate
> design and implementation of (1).

I think that all three of those are reasonable things to want to do, and
that the current behavior of (1) is a nice, conservative default. But
where we could do better is in helping the user understand what happened
and what their options are.

In Laszlo's example, he saw:

  $ git commit
  lib/achievement.cpp: not added yet
  lib/achievement.h: not added yet
  lib/achievementparser.cpp: not added yet
  lib/achievementparser.h: not added yet
  error: Error building trees

I see a couple of places to improve:

  1. We say "not added yet", which is a good start. We at least point
     out the problematic paths. But it's perhaps a little confusing.
     Those paths _have_ been added. It's just that no content was added
     at them yet.

  2. The "error: Error building trees" is unnecessarily scary, as it
     looks like git ran into some error while trying to do what you
     wanted (and indeed, that is the same message you get for something
     like "oops, we couldn't write to the object db"). It's not even
     clear from that output that the "not added yet" lines are the cause
     of the error, unless you understand how "git commit" is
     implemented.

  3. There is no advice given on how to proceed.

So I think a much nicer output would be something like:

  $ git commit
  error: some paths could not be committed
  The following paths were marked with intent-to-add, but have
  not had any content added:

    lib/achievement.cpp
    lib/achievement.h
    lib/achievementparser.cpp
    lib/achievementparser.h

  If you want to commit them with their current content, use "git add".
  If you want git to forget about them, use "git rm --cached".

We could also provide some options to git commit to make those
operations easier. Especially "ignore these for this commit but leave
them in the index" is a little awkward to do.

> Having said all that, we might want to differentiate these paths in the
> output from 'status'.

Yes, that was my first thought on reading Laszlo's message. We are
somewhat lying to say "changes to be committed". Your suggestion:

>     # ... to be committed ...
>     # new file: RENAMING (needs 'git add')
>     # ... have local changes ...
>     # modified: RENAMING

You could also give them a separate stanza, like:

  # Changes to be committed:
  #   (use "git reset HEAD <file>..." to unsage)
  #
  #     modified: some-other-file
  #
  # Paths marked for addition, but needing content:
  #   (use "git add <file>..." to update what will be committed)
  #
  #     new file: RENAMING

> if we don't care about the backward compatibility.

I don't know how much we need to care about "git status" output of this
form. It is parsed mainly be editors' syntax highlighters. Adding this
new information might not get highlighted as nicely, but it's probably
not a big deal.

I am much more concerned with whether and how this information would be
represented in the "git status --porcelain" format.

-Peff

Re: Re* git commit fails under some circumstances

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I am much more concerned with whether and how this information would be
> represented in the "git status --porcelain" format.

I earlier suggested using 'I'; a safer alternative would be to change
nothing and let the callers figure it out.  I slightly favor the former;
while there is a definite risk of breaking scripts' expectations, they can
be tentatively marked "this script does not work until you 'git add' paths
you used 'add -N'" and I don't think it would be such a big deal.

Re: Re* git commit fails under some circumstances

[Jeff King]

On Wed, Apr 06, 2011 at 10:24:17AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I am much more concerned with whether and how this information would be
> > represented in the "git status --porcelain" format.
> 
> I earlier suggested using 'I'; a safer alternative would be to change
> nothing and let the callers figure it out.

Ah, sorry, I missed that.

> I slightly favor the former; while there is a definite risk of
> breaking scripts' expectations, they can be tentatively marked "this
> script does not work until you 'git add' paths you used 'add -N'" and
> I don't think it would be such a big deal.

I think that is reasonable. Maintaining backwards compatibility doesn't
mean we can _never_ expand to cover new situations or give more
information. In this case we would be maintaining syntactic
compatibility, and have identical output when the "add -N" feature is
not used.

-Peff