[PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically

[PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically

[Andy Parkins]

Raimund Bauer offered this suggestion (paraphrased):

"Maybe we could do git-commit -a  _only_ if the index matches HEAD, and
otherwise keep current behavior?  So people who don't care about the
index won't get tripped up, and when you do have a dirty index, you get
told about it?"

Johannes Schindelin pointed out that this isn't the right thing to do for
an --amend, so that is checked for. Additionally, it's probably not the
right thing to do if any files are specified with "--only" or
"--include", so they turn this behaviour off as well.

Nguyen Thai Ngoc Duy asked that git-commit let you know it's done this
by adding an extra comment to the commit message.

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
This time we also inhibit if "all" mode is already set; if the user
has specified "-a" we don't need to tell them that we've switched "-a"
mode on.


 git-commit.sh |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/git-commit.sh b/git-commit.sh
index 81c3a0c..4552727 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -265,6 +265,16 @@ $1"
 done
 case "$edit_flag" in t) no_edit= ;; esac
 
+# Clever commit - if this commit would do nothing, then make it an "all"
+# commit
+if [ -z "$(git-diff-index --cached --name-only HEAD)" \
+	-a -z "$amend" -a -z "$only" -a -z "$also" -a -z "$all" ]; then
+	echo "# There was nothing to commit but changes were detected in the" > $GIT_DIR/SQUASH_MSG
+	echo "# working tree. 'git commit -a' mode activated." >> $GIT_DIR/SQUASH_MSG
+	echo "#" >> $GIT_DIR/SQUASH_MSG
+	all=t
+fi
+
 ################################################################
 # Sanity check options
 
-- 
1.4.4.1.g3ece-dirty

Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> Raimund Bauer offered this suggestion (paraphrased):
>
> "Maybe we could do git-commit -a  _only_ if the index matches HEAD, and
> otherwise keep current behavior?  So people who don't care about the
> index won't get tripped up, and when you do have a dirty index, you get
> told about it?"
>
> Johannes Schindelin pointed out that this isn't the right thing to do for
> an --amend, so that is checked for. Additionally, it's probably not the
> right thing to do if any files are specified with "--only" or
> "--include", so they turn this behaviour off as well.
>
> Nguyen Thai Ngoc Duy asked that git-commit let you know it's done this
> by adding an extra comment to the commit message.

I think another exception should be needed.  If the index does
not match the working tree, it should not default to "-a".

Otherwise,

I want to fix another thing in pickaxe.

	$ edit builtin-blame.c

My wife calls me.  Away from desk for 20 minutes.  Later I come
back.

	$ git update-index builtin-pickaxe.c

I am so used to that name and did not realize that typo, and I
was not paying too much attention.  My wife calls me again.
Away from desk and back in 20 minutes.

	$ git commit -m 'git-blame: Another fix.'

Oops.

So, please turn this "cleverness" off when the index does not
match the working tree.

Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> I think another exception should be needed.  If the index does
> not match the working tree, it should not default to "-a".
>
> Otherwise,

I think there needs a bit of explanation and additional step
that happened here.  This by the way is not a made-up example.
Everything, including the 20-minute away, were what happened
when I did the latest blame fix you saw a few days ago.

 * I am still futzing with blame from time to time, and have
   this change almost permanently in my working tree.

	$ cat P.diff
        diff --git a/builtin-blame.c b/builtin-blame.c
        index dc3ffea..46ce45c 100644
        --- a/builtin-blame.c
        +++ b/builtin-blame.c
        @@ -38,7 +38,7 @@ static int max_digits;
         static int max_score_digits;

         #ifndef DEBUG
        -#define DEBUG 0
        +#define DEBUG 1
         #endif

 * I also have the GIT-VERSION-GEN change in my working tree.

> I want to fix another thing in pickaxe.
>
> 	$ edit builtin-blame.c
>

 * Of course I did tests here.

> My wife calls me.  Away from desk for 20 minutes.  Later I come
> back.

 * And then reverted the DEBUG back to 0 in preparation for
   "checking into the index"

	$ edit builtin-blame.c

> 	$ git update-index builtin-pickaxe.c

 * And then I reverted it back for later futing.

 	$ git apply P.diff ;# that is a permanent-temporary file.

>
> I am so used to that name and did not realize that typo, and I
> was not paying too much attention.  My wife calls me again.
> Away from desk and back in 20 minutes.
>
> 	$ git commit -m 'git-blame: Another fix.'
>
> Oops.

 * Oops here is not just that builtin-blame.c would have been
   committed; I'd almost never do "commit -a" in this repository,
   because it would take that "DEBUG 1" change _and_
   GIT-VERSION-GEN change into the commit.

> So, please turn this "cleverness" off when the index does not
> match the working tree.

Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically

[Andy Parkins]

On Friday 2006 December 01 11:15, Junio C Hamano wrote:

> I think another exception should be needed.  If the index does
> not match the working tree, it should not default to "-a".

No problem: just don't apply the patch :-)  What you've asked for leaves it as 
a no-op.

This patch activates "-a" when the index equals HEAD.  i.e. git-commit would 
do nothing in this situation.  If it is disabled when the index doesn't match 
the working tree, then we're back to "do nothing".  i.e. HEAD==index==working 
tree.

> So, please turn this "cleverness" off when the index does not
> match the working tree.

How does that help you?  You've updated the index manually, so the 
automatic "-a" is already disabled.  Without this patch you would still have 
committed the wrong thing.

 $ edit builtin-blame.c
 $ git update-index builtin-pickaxe.c
 $ git commit

What is that you would like to have happened at this point?


Andy
-- 
Dr Andy Parkins, M Eng (hons), MIEE

Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> Raimund Bauer offered this suggestion (paraphrased):
>
> "Maybe we could do git-commit -a  _only_ if the index matches HEAD, and
> otherwise keep current behavior?  So people who don't care about the
> index won't get tripped up, and when you do have a dirty index, you get
> told about it?"
>
> Johannes Schindelin pointed out that this isn't the right thing to do for
> an --amend, so that is checked for. Additionally, it's probably not the
> right thing to do if any files are specified with "--only" or
> "--include", so they turn this behaviour off as well.
>
> Nguyen Thai Ngoc Duy asked that git-commit let you know it's done this
> by adding an extra comment to the commit message.
>
> Signed-off-by: Andy Parkins <andyparkins@gmail.com>

Aside from the "working tree not matching index" safety valve I
asked for, there is a more important case that "when the index
matches HEAD" is not a safe enough check for this 'cleverness'.

It's related to this code in git-commit:

        if [ "$?" != "0" -a ! -f "$GIT_DIR/MERGE_HEAD" -a -z "$amend" ]
        then
                rm -f "$GIT_DIR/COMMIT_EDITMSG" "$GIT_DIR/SQUASH_MSG"
                run_status
                exit 1
        fi

This allows you to make a new commit with an identical tree as
the current HEAD, when making a merge.

We originally did not allow a commit that has identical tree
with its first parent.  It was pointed out as a bug in the real
world setting, where two bugs are independently fixed by two
people, textually and also logically differently, and the merge
results in a conflict.  In such a case, after examining the text
(git diff --cc) and motivation (git log --merge) of the changes
that conflicted from both ends, if you decide that you prefer
your own fix over the fix made by the other party, your edit of
the conflicted file would end up to be identical to what you had
originally in the HEAD.

In such a situation, it makes sense to record the fact that the
two development paths came to the same conclusion as a merge.
It would also simplify later merges from the other side, and the
merges the other side attempts by pulling from you.

So after you fix the conflicts, you would mark the conflicted
file with Nico's shiny new "git add".  The index _still_ matches
HEAD.  The 'cleverer' "git commit" would not notice this
situation and commits unrelated changes that the user does not
have any intention to commit in the working tree by mistake.

So that is _another_ exception you must handle.

But I think the problem with this 'cleverer' commit runs
deeper.

Notice that you needed to say "The main idea is this cleverness,
but foo pointed out this special case, and bar pointed out
another, and we fixed all of these known ones and now this is
good, let's apply it." in your proposed commit log message?  You
should smell fishiness in that kind of reasoning.

It's not that you cannot be sure that these suggestions covered
all cases; you should not have to cover everything under the sun
in your initial revision.  New exceptions can certainly be added
incrementally to address breakages.  However, having to have
exception from the beginning is a sign that the general rule you
started from is a broken rule.  To realize that yourself, Think
about how you would describe the behaviour to a new user.

The currently proposed rewording of "git add" and "git commit"
manual pages would define "git commit" to behave in two primary
modes:

 1. With paths, "git commit <paths>" means "forget for a moment
    the changes I staged to be committed, and make a commit that
    includes only these paths (i.e. the new commit and the
    current HEAD are different at exactly these paths and
    nowhere else, and the new commit has contents from the
    working tree for these paths)".

 2. Without paths, "git commit" means "make a commit out of
    everything I have told you to commit (aka 'staged') so far".
    The primary ways to tell git to stage contents are "git
    add/rm/mv".  But as a short-hand, you can say "git commit
    -a" to ask the command to place all the changes in the
    working tree in the changeset to be committed before making
    the new commit.

How would your version be described?

 1. (ditto)

 2. (ditto).  However, there are number of exceptions you should
    be aware of:

 2-a. The above general rule only applies if you have already
      told git to stage something for the next commit.

 2-b. However, even if you had told git to stage new contents,
      if the new contents happen to be the same as HEAD, then
      the command grabs every change in the working tree and
      makes a commit out of it.

 2-c. Contrary to the above exception, even if you staged the
      same contents as in HEAD, if you are in the middle of the
      merge, it does not include the working tree changes in the
      new commit.  This is because....

 2-d. Another exception is that if you are amending an existing
      commit, ...  This is because....

With careful reading, the users may understand it after they
read the above description, but would that understanding really
be committed to memory?  Are you confident that you will
remember your own reasoning for the exceptions and be prepared
to defend this change (and perhaps follow-up additions to the
list of exceptions), if 6 months down the road when an old timer
from the kernel circle says "I updated to recent git, and its
'commit' behaves funnily in the way I did not expect.  Why does
it do that"?

I really think the users would be much better off with
consistent behaviour that is easy and simple to describe than a
complex magic that does the right thing 99.9% of the time,
because you either understand the complex magic or constantly in
fear of the tool that can work against you 0.01% of the time.

Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 3819 bytes --]

On Sat, 02 Dec 2006 00:09:09 -0800, Junio C Hamano wrote:
> But I think the problem with this 'cleverer' commit runs
> deeper.

I agree. Being too clever is a problem.

It's very helpful to estimate usability and learnability by the length
of prose needed to describe a command.

>  1. With paths, "git commit <paths>" means "forget for a moment
>     the changes I staged to be committed, and make a commit that
>     includes only these paths (i.e. the new commit and the
>     current HEAD are different at exactly these paths and
>     nowhere else, and the new commit has contents from the
>     working tree for these paths)".
>
>  2. Without paths, "git commit" means "make a commit out of
>     everything I have told you to commit (aka 'staged') so far".
>     The primary ways to tell git to stage contents are "git
>     add/rm/mv".  But as a short-hand, you can say "git commit
>     -a" to ask the command to place all the changes in the
>     working tree in the changeset to be committed before making
>     the new commit.

Junio, thanks so much for these descriptions. They help ground the
discussion quite nicely, (and will also contribute to improved
documentation).

Here's pseudo-code for the above descriptions:

	if (command-line has paths) {
		ignore staging area, commit named files
	else {
		if (commit -a)
			update all files into staging area
		commit staging area
	}

One problem I see in that is that the primary distinction is made
based on what appears on the command-line, rather than what job the
user is trying to perform. Also, "commit -a" is define in terms of the
staging area, even though the staging area is basically irrelevant to
this operation, (just as it is in the case of a commit with paths).

So I would re-factor that in a way that focuses on what the user is
trying to do:

	if (! doing a staged commit) {
		if (file list is empty)
			file list = all tracked files
		commit file list
	} else {
		commit staging area
	}

This brings the description of "commit -a" and "commit files..."
together, (which I think are conceptually more related than "commit
-a" is to a commit of the staging area, (and yes, this ignores the
history of the implementation). What we're talking about is how to
document what the user wants to do, not how the implementation does
it.

Notice also that "staging area" never appears in the description of
the else clause, (which is good since the conceptual use of these
commands does not involve staging).

So translating my re-factored version back into prose, we might get:

   commit <paths>
   commit -a

	Commit the working-tree contents of the named <paths>, (or all
	tracked paths for -a). Files which no longer exist in the
	working tree will be removed. New files to be tracked must be
	added with "git add".

   commit

	Commit the content that exists in the staging area. The
	staging area initially consists of the contents of the most
	recent commit, but can be modified with the "git add",
	"git rm", and "git mv".

So that's shorter. I think it's also more clear and focused on what
the user wants to do without being any less accurate.

It doesn't make it obvious that "commit -a" is the most common form
and what users should look at first. So what I'd like to see is the
semantic changes that would allow us to document this as:

   commit
   commit <paths>

	Commit the working-tree contents of all tracked paths, (or
	just the specific paths listed). Files which no longer exist
	in the working tree will be removed. New files to be tracked
	must be added with "git add".

   commit --staged

	Commit the content that exists in the staging area. The
	staging area initially consists of the contents of the most
	recent commit, but updated content from the working tree can
	be placed into it with "git stage <paths>".

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically

[Andy Parkins]

On Saturday 2006, December 02 08:09, Junio C Hamano wrote:

> Aside from the "working tree not matching index" safety valve I
> asked for, there is a more important case that "when the index
> matches HEAD" is not a safe enough check for this 'cleverness'.

As I pointed out, that safety valve made the whole thing a no-op.

> So that is _another_ exception you must handle.
>
> But I think the problem with this 'cleverer' commit runs
> deeper.
>
> Notice that you needed to say "The main idea is this cleverness,
> but foo pointed out this special case, and bar pointed out
> another, and we fixed all of these known ones and now this is
> good, let's apply it." in your proposed commit log message?  You
> should smell fishiness in that kind of reasoning.

I don't believe so.  In deep parts of a program cleverness is always a bad 
idea.  It just obfuscates functionality.  However, this "cleverness" is about 
making things easier for a human.  Humans have a notoriously illogical set of 
requirements for doing the right thing.  As an example I'd hold up git's 
clever date specification code; a computer would be perfectly happy to accept 
epoch time, but instead humans like to be able to say "three weeks ago this 
Thursday at five to four".

This patch is merely to make git-commit do something sensible when it would 
normally do nothing.  I was sure there would be more exceptions to when it 
should activate; as git-commit is already a mess of "useful" extra switches.  
To blame this patch for the fact that git-commit does a lot of things in 
slightly different ways hardly seems fair.

As usual though: I don't mind, I'm not some huge proponent of this 
functionality - /I/ get along fine with git-commit

> I really think the users would be much better off with
> consistent behaviour that is easy and simple to describe than a
> complex magic that does the right thing 99.9% of the time,
> because you either understand the complex magic or constantly in
> fear of the tool that can work against you 0.01% of the time.

I suspect that this patch is the least of git-commit's problems in that 
department.  "Easy to describe" is certainly not the case already, otherwise 
none of this discussion (or patch) would ever have started.


Andy
-- 
Dr Andrew Parkins, M Eng (Hons), AMIEE

Re: [PATCH/RFC] Make git-commit cleverer - have it figure out whether it needs -a automatically

[Jakub Narebski]

Andy Parkins wrote:

> Raimund Bauer offered this suggestion (paraphrased):
> 
> "Maybe we could do git-commit -a  _only_ if the index matches HEAD, and
> otherwise keep current behavior?  So people who don't care about the
> index won't get tripped up, and when you do have a dirty index, you get
> told about it?"
> 
> Johannes Schindelin pointed out that this isn't the right thing to do for
> an --amend, so that is checked for.
> 
> Additionally, it's probably not the right thing to do if any files are
> specified with "--only" or "--include", so they turn this behaviour off
> as well.

Could we add suggestion by Andreas Ericsson to print in the "smart commit"
case:

  Nothing to commit but changes in working tree. Assuming 'git commit -a'

or something like that?
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

[PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically

[Andy Parkins]

Raimund Bauer offered this suggestion (paraphrased):

"Maybe we could do git-commit -a  _only_ if the index matches HEAD, and
otherwise keep current behavior?  So people who don't care about the
index won't get tripped up, and when you do have a dirty index, you get
told about it?"

Johannes Schindelin pointed out that this isn't the right thing to do for
an --amend, so that is checked for.

Additionally, it's probably not the right thing to do if any files are
specified with "--only" or "--include", so they turn this behaviour off
as well.

I've also output a message as suggested by Andreas Ericsson.

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
 git-commit.sh |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/git-commit.sh b/git-commit.sh
index 81c3a0c..fabfeae 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -265,6 +265,14 @@ $1"
 done
 case "$edit_flag" in t) no_edit= ;; esac
 
+# Clever commit - if this commit would do nothing, then make it an "all"
+# commit
+if [ -z "$(git-diff-index --cached --name-only HEAD)" \
+	-a -z "$amend" -a -z "$only" -a -z "$also" ]; then
+	echo "Nothing to commit but changes in working tree. Assuming 'git commit -a'"
+	all=t
+fi
+
 ################################################################
 # Sanity check options
 
-- 
1.4.4.1.g3ece-dirty

Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically

[Nguyen Thai Ngoc Duy]

> diff --git a/git-commit.sh b/git-commit.sh
> index 81c3a0c..fabfeae 100755
> --- a/git-commit.sh
> +++ b/git-commit.sh
> @@ -265,6 +265,14 @@ $1"
>  done
>  case "$edit_flag" in t) no_edit= ;; esac
>
> +# Clever commit - if this commit would do nothing, then make it an "all"
> +# commit
> +if [ -z "$(git-diff-index --cached --name-only HEAD)" \
> +       -a -z "$amend" -a -z "$only" -a -z "$also" ]; then
> +       echo "Nothing to commit but changes in working tree. Assuming 'git commit -a'"

This is hardly seen as the editor will immediately pop up. Better
pause a second or put it in commit template (I'd prefer the latter).

> +       all=t
> +fi
> +
>  ################################################################
>  # Sanity check options
>
> --
> 1.4.4.1.g3ece-dirty
-- 

Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically

[Jakub Narebski]

Nguyen Thai Ngoc Duy wrote:

>> +# Clever commit - if this commit would do nothing, then make it an "all"
>> +# commit
>> +if [ -z "$(git-diff-index --cached --name-only HEAD)" \
>> +       -a -z "$amend" -a -z "$only" -a -z "$also" ]; then
>> +       echo "Nothing to commit but changes in working tree. Assuming 'git commit -a'"
> 
> This is hardly seen as the editor will immediately pop up. Better
> pause a second or put it in commit template (I'd prefer the latter).

Well, if it is VISUAL editor, you would see this. But adding this
to template is certainly good idea.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

[PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically

[Andy Parkins]

Raimund Bauer offered this suggestion (paraphrased):

"Maybe we could do git-commit -a  _only_ if the index matches HEAD, and
otherwise keep current behavior?  So people who don't care about the
index won't get tripped up, and when you do have a dirty index, you get
told about it?"

Johannes Schindelin pointed out that this isn't the right thing to do for
an --amend, so that is checked for. Additionally, it's probably not the
right thing to do if any files are specified with "--only" or
"--include", so they turn this behaviour off as well.

Nguyen Thai Ngoc Duy asked that git-commit let you know it's done this
by adding an extra comment to the commit message.

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
 git-commit.sh |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/git-commit.sh b/git-commit.sh
index 81c3a0c..b391257 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -265,6 +265,16 @@ $1"
 done
 case "$edit_flag" in t) no_edit= ;; esac
 
+# Clever commit - if this commit would do nothing, then make it an "all"
+# commit
+if [ -z "$(git-diff-index --cached --name-only HEAD)" \
+	-a -z "$amend" -a -z "$only" -a -z "$also" ]; then
+	echo "# There was nothing to commit but changes were detected in the" > $GIT_DIR/SQUASH_MSG
+	echo "# working tree. 'git commit -a' mode activated." >> $GIT_DIR/SQUASH_MSG
+	echo "#" >> $GIT_DIR/SQUASH_MSG
+	all=t
+fi
+
 ################################################################
 # Sanity check options
 
-- 
1.4.4.1.g3ece-dirty

Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically

[Salikh Zakirov]

Andy Parkins wrote:
> Raimund Bauer offered this suggestion (paraphrased):
> 
> "Maybe we could do git-commit -a  _only_ if the index matches HEAD, and
> otherwise keep current behavior?  So people who don't care about the
> index won't get tripped up, and when you do have a dirty index, you get
> told about it?"

Brilliant solution!

Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically

[Jakub Narebski]

Andy Parkins wrote:

> Raimund Bauer offered this suggestion (paraphrased):
> 
> "Maybe we could do git-commit -a  _only_ if the index matches HEAD, and
> otherwise keep current behavior?  So people who don't care about the
> index won't get tripped up, and when you do have a dirty index, you get
> told about it?"
> 
> Johannes Schindelin pointed out that this isn't the right thing to do for
> an --amend, so that is checked for. Additionally, it's probably not the
> right thing to do if any files are specified with "--only" or
> "--include", so they turn this behaviour off as well.
> 
> Nguyen Thai Ngoc Duy asked that git-commit let you know it's done this
> by adding an extra comment to the commit message.

Insount on #git pointed out fragility of this solution with respect
to adding/removing/moving files, which dirties index (which might not be
understood by newbie user: "git commit" used to work, but doesn't work the
same when I added some files).
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] Make git-commit cleverer - have it figure out whether it needs -a automatically

[Andy Parkins]

On Thursday 2006 November 30 16:28, Jakub Narebski wrote:

> Insount on #git pointed out fragility of this solution with respect
> to adding/removing/moving files, which dirties index (which might not be
> understood by newbie user: "git commit" used to work, but doesn't work the
> same when I added some files).

It does, provided they only added files and didn't change anything else.  If 
they did then we're out of it for this patch anyway.

Anyway, this is only meant to help ease people into the index.  As discussed 
elsewhere, hiding the index is a silly policy.

This patch isn't /just/ for the newbies by the way (who are already confused, 
so that hasn't changed), I know about the index, but I still like it.  
Remember, all it's really saying is "when commit would do nothing, do 
something".  So it only takes away an option that you can't have been using 
anyway because it didn't do anything.


Andy
-- 
Dr Andy Parkins, M Eng (hons), MIEE