non-empty index with git commit -a

non-empty index with git commit -a

[Sverre Rabbelier]

Heya,

Can we please fix this?

$ hack hack hack
$ git add -p # carefully add my changes
$ git commit -am "Add feature foo"
$ DAAAMN YOUUU FINGERS!!

I remember bringing this up before, but I can't remember what the
discussion ended up coming to.

-- 
Cheers,

Sverre Rabbelier

Re: non-empty index with git commit -a

[Jeff King]

On Tue, Feb 15, 2011 at 10:43:53PM +0000, Sverre Rabbelier wrote:

> Can we please fix this?
> 
> $ hack hack hack
> $ git add -p # carefully add my changes
> $ git commit -am "Add feature foo"
> $ DAAAMN YOUUU FINGERS!!
> 
> I remember bringing this up before, but I can't remember what the
> discussion ended up coming to.

How do you differentiate this from:

  $ hack hack hack
  $ git add -p ;# carefully add my changes
  $ hack hack hack
  $ git diff ;# ok, everything looks good
  $ git commit -am "Add feature foo"

? IOW, I don't think git can know that you are making a mistake in your
case, since from its perspectives those same actions might be
reasonable. The real problem is that your intermediate index state is
never saved. Do we need an index reflog? :)

-Peff

Re: non-empty index with git commit -a

[Jonathan Nieder]

Jeff King wrote:
> On Tue, Feb 15, 2011 at 10:43:53PM +0000, Sverre Rabbelier wrote:

>> Can we please fix this?
>>
>> $ hack hack hack
>> $ git add -p # carefully add my changes
>> $ git commit -am "Add feature foo"
>> $ DAAAMN YOUUU FINGERS!!
>>
>> I remember bringing this up before, but I can't remember what the
>> discussion ended up coming to.
>
> How do you differentiate this from:
>
>   $ hack hack hack
>   $ git add -p ;# carefully add my changes
>   $ hack hack hack
>   $ git diff ;# ok, everything looks good
>   $ git commit -am "Add feature foo"
>
> ?

If I remember the discussion[1] correctly, by requiring a separate
add -u step (or commit -f -a) if the "I don't trust my fingers"
configuration is enabled.

[1] http://thread.gmane.org/gmane.linux.debian.devel.bugs.general/698001/focus=145662

Re: non-empty index with git commit -a

[Jeff King]

On Tue, Feb 15, 2011 at 09:20:47PM -0600, Jonathan Nieder wrote:

> Jeff King wrote:
> > On Tue, Feb 15, 2011 at 10:43:53PM +0000, Sverre Rabbelier wrote:
> 
> >> Can we please fix this?
> >>
> >> $ hack hack hack
> >> $ git add -p # carefully add my changes
> >> $ git commit -am "Add feature foo"
> >> $ DAAAMN YOUUU FINGERS!!
> >>
> >> I remember bringing this up before, but I can't remember what the
> >> discussion ended up coming to.
> >
> > How do you differentiate this from:
> >
> >   $ hack hack hack
> >   $ git add -p ;# carefully add my changes
> >   $ hack hack hack
> >   $ git diff ;# ok, everything looks good
> >   $ git commit -am "Add feature foo"
> >
> > ?
> 
> If I remember the discussion[1] correctly, by requiring a separate
> add -u step (or commit -f -a) if the "I don't trust my fingers"
> configuration is enabled.
> 
> [1] http://thread.gmane.org/gmane.linux.debian.devel.bugs.general/698001/focus=145662

Thanks for the link, I didn't recall that discussion. I have no problem
with a config option that requires an extra step, but such a safety
valve should definitely be optional, since the current behavior is part
of a perfectly reasonable workflow.

-Peff

Re: non-empty index with git commit -a

[Sverre Rabbelier]

Heya,

On Wed, Feb 16, 2011 at 03:20, Jonathan Nieder <jrnieder@gmail.com> wrote:
> If I remember the discussion[1] correctly, by requiring a separate
> add -u step (or commit -f -a) if the "I don't trust my fingers"
> configuration is enabled.

That would definitely work for me. The question then is, should git
consider the index precious by default, or not? That is, should that
value default to true in 1.8?

-- 
Cheers,

Sverre Rabbelier

Re: non-empty index with git commit -a

[Nguyen Thai Ngoc Duy]

On Wed, Feb 16, 2011 at 3:18 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Heya,
>
> On Wed, Feb 16, 2011 at 03:20, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> If I remember the discussion[1] correctly, by requiring a separate
>> add -u step (or commit -f -a) if the "I don't trust my fingers"
>> configuration is enabled.
>
> That would definitely work for me. The question then is, should git
> consider the index precious by default, or not? That is, should that
> value default to true in 1.8?

Is it insane that "git commit -a" still commits everything, but then

1. If old index is different from old HEAD, keeps index as is and warn users
2. If old index is the same as old HEAD, update index with new HEAD

?
-- 
Duy

Re: non-empty index with git commit -a

[Jonathan Nieder]

Nguyen Thai Ngoc Duy wrote:
> On Wed, Feb 16, 2011 at 3:18 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
>> On Wed, Feb 16, 2011 at 03:20, Jonathan Nieder <jrnieder@gmail.com> wrote:

>>> If I remember the discussion[1] correctly, by requiring a separate
>>> add -u step (or commit -f -a) if the "I don't trust my fingers"
>>> configuration is enabled.
>>
>> That would definitely work for me. The question then is, should git
>> consider the index precious by default, or not? That is, should that
>> value default to true in 1.8?
>
> Is it insane that "git commit -a" still commits everything, but then
>
> 1. If old index is different from old HEAD, keeps index as is and warn users
> 2. If old index is the same as old HEAD, update index with new HEAD
>
> ?

Yes, I think that would be very confusing. :)

My take on Sverre's question: this doesn't seem to fit the "bad choice
made long ago and finally we have a chance to fix it" mold.  More like
"nice new feature that could break muscle memory".  So I don't think
it makes sense to tie it to 1.8.

If I were writing the patch, I'd provide the configuration and hope
that the experience of using it could help with deciding on a good
default behavior.

Re: non-empty index with git commit -a

[Jeff King]

On Wed, Feb 16, 2011 at 08:18:18AM +0000, Sverre Rabbelier wrote:

> On Wed, Feb 16, 2011 at 03:20, Jonathan Nieder <jrnieder@gmail.com> wrote:
> > If I remember the discussion[1] correctly, by requiring a separate
> > add -u step (or commit -f -a) if the "I don't trust my fingers"
> > configuration is enabled.
> 
> That would definitely work for me. The question then is, should git
> consider the index precious by default, or not? That is, should that
> value default to true in 1.8?

If we do not consider the index precious, then no, it should not be the
default. If we do consider it precious, then isn't this option fixing
one small case and leaving all of the others untouched?

I can just as easily run "git add -u", or "git add -p" on top of some
changes and then realize afterwards that I actually wanted the previous
index state.

The solution for that would be an index reflog.

-Peff

Re: non-empty index with git commit -a

[Matthieu Moy]

Jonathan Nieder <jrnieder@gmail.com> writes:

> If I remember the discussion[1] correctly, by requiring a separate
> add -u step (or commit -f -a) if the "I don't trust my fingers"
> configuration is enabled.
>
> [1] http://thread.gmane.org/gmane.linux.debian.devel.bugs.general/698001/focus=145662

You may want to look at what eg (easy git) does too:

http://people.gnome.org/~newren/eg/documentation/commit.html

essentially, it considers commiting with dirty index the uncommon
case, and requires an extra --staged flag to allow it.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: non-empty index with git commit -a

[Sverre Rabbelier]

Heya,

On Wed, Feb 16, 2011 at 08:51, Jeff King <peff@peff.net> wrote:
> I can just as easily run "git add -u", or "git add -p" on top of some
> changes and then realize afterwards that I actually wanted the previous
> index state.

I disagree, those are all index manipulation tools, `git commit -a` is not.

-- 
Cheers,

Sverre Rabbelier

Re: non-empty index with git commit -a

[Jeff King]

On Wed, Feb 16, 2011 at 09:52:23AM +0000, Sverre Rabbelier wrote:

> On Wed, Feb 16, 2011 at 08:51, Jeff King <peff@peff.net> wrote:
> > I can just as easily run "git add -u", or "git add -p" on top of some
> > changes and then realize afterwards that I actually wanted the previous
> > index state.
> 
> I disagree, those are all index manipulation tools, `git commit -a` is not.

So? Your question was whether index state is precious. If it's precious,
shouldn't we be keeping a history of it?

-Peff

Re: non-empty index with git commit -a

[Sverre Rabbelier]

Heya,

On Wed, Feb 16, 2011 at 09:54, Jeff King <peff@peff.net> wrote:
> So? Your question was whether index state is precious. If it's precious,
> shouldn't we be keeping a history of it?

I don't think it's quite _that_ precious, but the only operation that
I regularly use that can blow away my carefully constructed index as
side effect of doing something else is `git commit -a`.

-- 
Cheers,

Sverre Rabbelier

Re: non-empty index with git commit -a

[Jeff King]

On Wed, Feb 16, 2011 at 09:58:52AM +0000, Sverre Rabbelier wrote:

> On Wed, Feb 16, 2011 at 09:54, Jeff King <peff@peff.net> wrote:
> > So? Your question was whether index state is precious. If it's precious,
> > shouldn't we be keeping a history of it?
> 
> I don't think it's quite _that_ precious, but the only operation that
> I regularly use that can blow away my carefully constructed index as
> side effect of doing something else is `git commit -a`.

OK, so how precious is it? :)

If you want to have an option that specifically prevents the "git commit
-a" muscle memory thing, then go for it. I'm guessing it is the most
common "oops" one. Even with an index reflog, you might want it on top.

But it just seems silly to me to not protect at the same time against
the other ways you can lose state from the index.

-Peff

Re: non-empty index with git commit -a

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> So? Your question was whether index state is precious. If it's precious,
> shouldn't we be keeping a history of it?

If it's really precious, it probably means you should be commit
--amend instead. You'd get the reflog, and essentially the same
functionalities.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: non-empty index with git commit -a

[Michael J Gruber]

Jeff King venit, vidit, dixit 16.02.2011 11:06:
> On Wed, Feb 16, 2011 at 09:58:52AM +0000, Sverre Rabbelier wrote:
> 
>> On Wed, Feb 16, 2011 at 09:54, Jeff King <peff@peff.net> wrote:
>>> So? Your question was whether index state is precious. If it's precious,
>>> shouldn't we be keeping a history of it?
>>
>> I don't think it's quite _that_ precious, but the only operation that
>> I regularly use that can blow away my carefully constructed index as
>> side effect of doing something else is `git commit -a`.
> 
> OK, so how precious is it? :)

Maybe it's a bit precious, but not overly...

> 
> If you want to have an option that specifically prevents the "git commit
> -a" muscle memory thing, then go for it. I'm guessing it is the most
> common "oops" one. Even with an index reflog, you might want it on top.
> 
> But it just seems silly to me to not protect at the same time against
> the other ways you can lose state from the index.
> 
> -Peff

so that keeping one backup would be enough? I.e. an automated way of
doing "cp index index.bak" before an index update and some "reset"
incarnation to revive the copy?

Michael

Re: non-empty index with git commit -a

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> On Wed, Feb 16, 2011 at 08:51, Jeff King <peff@peff.net> wrote:
>> I can just as easily run "git add -u", or "git add -p" on top of some
>> changes and then realize afterwards that I actually wanted the previous
>> index state.
>
> I disagree, those are all index manipulation tools, `git commit -a` is not.

"git commmit" is not an index-manipulation tool.  Giving "-a" or pathspec
to the command however is.

Having said that, exactly because "commit" _feels_ more like a
point-of-no-return than a mistaken second "add" to the end user (even
though it is not technically different), I think it is Ok to add a
configuration that makes the command bail out when "-a" is given and there
are changes to the index.  I don't think it is sane to trigger the same
safety when a pathspec is given, though.

Re: non-empty index with git commit -a

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Feb 16, 2011 at 09:58:52AM +0000, Sverre Rabbelier wrote:
>
>> On Wed, Feb 16, 2011 at 09:54, Jeff King <peff@peff.net> wrote:
>> > So? Your question was whether index state is precious. If it's precious,
>> > shouldn't we be keeping a history of it?
>> 
>> I don't think it's quite _that_ precious, but the only operation that
>> I regularly use that can blow away my carefully constructed index as
>> side effect of doing something else is `git commit -a`.
>
> OK, so how precious is it? :)

The world is not that black-and-white, but is full of different shades of
gray.

If you made mistakes with a second "git add", you can "reset --mixed"
everything away and restart from scratch.  The same thing can be said for
a mistaken "git commit -a" that can be "reset HEAD^" (or --amend).  So
there is not much difference at the technical level.

I don't think this is primarily about "protecting the index".  It is more
about making the user feel better.  Compared to a mistaken second "add", a
mistaken "commit -a" feels like a lot heavier point-of-no-return.

Re: non-empty index with git commit -a

[Jeff King]

On Wed, Feb 16, 2011 at 03:41:35PM +0100, Michael J Gruber wrote:

> > If you want to have an option that specifically prevents the "git commit
> > -a" muscle memory thing, then go for it. I'm guessing it is the most
> > common "oops" one. Even with an index reflog, you might want it on top.
> > 
> > But it just seems silly to me to not protect at the same time against
> > the other ways you can lose state from the index.
> > 
> so that keeping one backup would be enough? I.e. an automated way of
> doing "cp index index.bak" before an index update and some "reset"
> incarnation to revive the copy?

No, I am not proposing one backup. I am proposing a reflog with multiple
entries that would be expired on the regular reflog schedule.

-Peff

Re: non-empty index with git commit -a

[Jeff King]

On Wed, Feb 16, 2011 at 10:51:31AM -0800, Junio C Hamano wrote:

> > OK, so how precious is it? :)
> 
> The world is not that black-and-white, but is full of different shades of
> gray.
> 
> If you made mistakes with a second "git add", you can "reset --mixed"
> everything away and restart from scratch.  The same thing can be said for
> a mistaken "git commit -a" that can be "reset HEAD^" (or --amend).  So
> there is not much difference at the technical level.

Sure. The problem there is that "scratch" may involve losing some work
you did picking apart changes.

> I don't think this is primarily about "protecting the index".  It is more
> about making the user feel better.  Compared to a mistaken second "add", a
> mistaken "commit -a" feels like a lot heavier point-of-no-return.

I guess. I have very occasionally run into the second-add problem and
wanted to be able to return to an earlier index state, but I admit it
doesn't come up that much. I just see them as the same problem.

I think I am also a little turned off by the config option solution
because it seems very un-git to me. Our usual approach is to give the
user a lot of flexibility, let them shoot their own foot off, and then
let them know that their intact foot is waiting in the reflog, ready to
be sewn back on.

Yes, we try to limit uselessly destructive actions before they happen.
But this is not one of those cases. The exact command set that is being
listed as dangerous is also part of a very reasonable workflow. It just
depends on what the user's intention was.

But as I said, I am not against a config option if it is such a common
problem. I certainly would not turn it on. And I don't think it should
be on by default.

-Peff

Re: non-empty index with git commit -a

[Jeff King]

On Wed, Feb 16, 2011 at 11:28:17AM +0100, Matthieu Moy wrote:

> Jeff King <peff@peff.net> writes:
> 
> > So? Your question was whether index state is precious. If it's precious,
> > shouldn't we be keeping a history of it?
> 
> If it's really precious, it probably means you should be commit
> --amend instead. You'd get the reflog, and essentially the same
> functionalities.

Yeah, that is good advice, and I do actually end up doing a lot of
intermediate commits and then either amending or sorting them out at the
end. And that helps with sorting out big changes.

But it does sort of feel like saying "we don't need a safety valve. You
just need to back up your work before doing anything dangerous". I have
to remember to do it. Which is fine if my workflow is
add-amend-add-amend. But maybe I have some half-saved index state and I
am about to try doing a merge checkout or stash apply.

Anyway, I'm not convinced it is all that precious (at least not enough
to merit working on the reflog). Yes, there have been times I've wished
I could go back to the index state of a minute ago, and had to re-sort
changes. But it usually doesn't take all that much time (thanks to "git
add -p"). My main objection is that I just don't see those cases as
different from "git commit -a". Either it is worth protecting, or it
isn't.

-Peff

Re: non-empty index with git commit -a

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But as I said, I am not against a config option if it is such a common
> problem. I certainly would not turn it on. And I don't think it should
> be on by default.

I think we are pretty much on the same page, except that I am not very
interested in the index-log either.

Re: non-empty index with git commit -a

[Jeff King]

On Wed, Feb 16, 2011 at 11:55:54AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > But as I said, I am not against a config option if it is such a common
> > problem. I certainly would not turn it on. And I don't think it should
> > be on by default.
> 
> I think we are pretty much on the same page, except that I am not very
> interested in the index-log either.

To be fair, I am not sure I am that interested in the index-log either.
I just see it as a superior solution to the same problem, but I don't
consider it a very important problem.

So I think we are really on the same page. :)

-Peff

Re: non-empty index with git commit -a

[Jakub Narebski]

Dnia środa 16. lutego 2011 20:59, Jeff King napisał:
> On Wed, Feb 16, 2011 at 11:55:54AM -0800, Junio C Hamano wrote:
> 
> > Jeff King <peff@peff.net> writes:
> > 
> > > But as I said, I am not against a config option if it is such a common
> > > problem. I certainly would not turn it on. And I don't think it should
> > > be on by default.
> > 
> > I think we are pretty much on the same page, except that I am not very
> > interested in the index-log either.
> 
> To be fair, I am not sure I am that interested in the index-log either.
> I just see it as a superior solution to the same problem, but I don't
> consider it a very important problem.
> 
> So I think we are really on the same page. :)

Also it would be not as easy as reflogs - you would have either to save
copy of index, or create a tree out of it and save reference in reflog.

-- 
Jakub Narebski
Poland

Re: non-empty index with git commit -a

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

>> So I think we are really on the same page. :)
>
> Also it would be not as easy as reflogs - you would have either to save
> copy of index, or create a tree out of it and save reference in reflog.

Also note that "Create a tree out of it" is not always a useful way to
save the state of the index.  The "stash" shares the same issue, but you
would need an equivalent of a tarball that consists of the index file
(with conflicted state) and the files in the working tree that corresponds
to index entries with conflicted states if you want to be useful for
storing a state away in case you botch the conflict resolution you are
attempting.

Re: non-empty index with git commit -a

[Jeff King]

On Wed, Feb 16, 2011 at 01:46:01PM -0800, Junio C Hamano wrote:

> > Also it would be not as easy as reflogs - you would have either to save
> > copy of index, or create a tree out of it and save reference in reflog.
> 
> Also note that "Create a tree out of it" is not always a useful way to
> save the state of the index.  The "stash" shares the same issue, but you
> would need an equivalent of a tarball that consists of the index file
> (with conflicted state) and the files in the working tree that corresponds
> to index entries with conflicted states if you want to be useful for
> storing a state away in case you botch the conflict resolution you are
> attempting.

I had assumed it would be a reflog of trees (or more likely parentless
commits), just keeping track of useful states. But the usage there isn't
very intuitive. You can do "git show index@{5.minutes.ago}:foo.c >bar.c"
to recover the state of a file, or even "git read-tree index@{1}" to
get a whole state. And those would be enough for the basic "oops, I just
want to go back to where I was".

But probably you should be able to do a three-way merge with where you
are now, with the HEAD at the time of save as a pseudo-parent. IOW,
treat the index state as if you had committed it and put it on a reflog.
And in the case that you are at the same HEAD, then a merge collapses
into a "fast-forward" to the index state you were at (I put
"fast-forward" in quotes because we are obviously not interested in
moving HEAD to the pseudo-commit at all, but just what ends up in the
index).

I dunno. I haven't thought all that hard about it, so maybe there is a
more clever way of doing it. Certainly the porcelain support for "reset
to this index, but don't move my HEAD" is not good, nor for "merge this
index, but nevermind about recording anything about the commit".  They
are both conceptually easy to implement in terms of plumbing, but the
commands to run them are a little unintuitive.

-Peff