I am sympathetic to the _cause_, but I do not think the option --cached
is a good match for this change.  As Hannes points out, as-is commit is
the default, and --cached to other commands mean "work only with index
not work tree", not "short-circuit for systems with slow lstat(3)".

Obviously we cannot short-circuit checking for modified or removed paths
when "git-commit -a" is run, but it is plausible that people may still
want to trade run_status output with interactive speed even when doing
"git-commit -a".

On the other hand, when we know we do not have to _show_ the list of
staged/modified/untracked files (i.e. we already have the commit log
message via -m, -F, or -C and we were told not to invoke editor), we do
not have to call run_status(), only to discard its output.  In such a
case, we are calling it only to see if we have something committable,
and we should be able to optimize THAT without being told by the user
with this new option.  Incidentally I just checked the scripted version;
it does not do this optimization (git-commit.sh, ll. 514-517).  The C
rewrite in 'next' does not have it in either (builtin-commit.c,
ll. 387-390).  When no_edit is in effect, I think these two places can
be replaced with an equivalent of "diff-index --cached HEAD --" (which
should not hit the work tree at all) to see if there is anything to be
committed.  For initial commit the check would obviously be "is the
index empty?" instead.

So in short:

 * The option "--cached" is a wrong thing to have the user say and is
   not what you want anyway. You want "no status list in the commit log
   template";

 * Skip run_status() and replace with "diff-index --cached HEAD" (or "is
   the index empty?") when the user instructs so;

 * In addition, the same optimization should apply when we know we do
   not use the exact run_status() output.

-

[ message continues ]

I don't just mean to avoid lstat. I'm trying to avoid _any_

That is not what I meant to say. I do want status list in commit message.
I don't want anything except git-diff-index --name-status --cached HEAD in it.
No untracked files whatsoever.

And, as I said, I also just want to commit the index _exactly_ as it is.
No checking for files changed after they were updated in the index
intended. I think that whatever the index holds is perfect (except it's

Right. Is it not what happens with the patch?
-

[ message continues ]

This is of course very useful optimization, and will speed up things
everywhere (and especially here).

But still: I didn't mean it. I really meant the interactive case, with
editor and prepared commit message which has status list.
Sometimes this list is the reason why a commit is aborted:
I notice that I'm committing something I didn't really intend to.
-

[ message continues ]

Like when it is already specified through -C, -F or -m to git-commit.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---


Could not stop myself. Hopefully didn't beat anyone to it :)
Almost all code shamelessly stolen from builtin-diff-index.c.
Builds, runs, passes all the tests. That !active_nr is
micro-optimization, but a nice one: clearly don't reread.

Preprocessor trickery in DIFF_OPT_* macros is disgusting, it breaks
Vim word completion and trying to use many flags in one expression
looks just ugly. What was wrong is inline functions?

 builtin-commit.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index a35881e..8167ce4 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -367,6 +367,30 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 
 	strbuf_release(&sb);
 
+	if (no_edit) {
+		static const char *argv[] = { NULL, "HEAD", NULL };
+		struct rev_info rev;
+		unsigned char sha1[40];
+		int is_initial;
+
+		fclose(fp);
+
+		if (!active_nr && read_cache() < 0)
+			die("Cannot read index");
+
+		if (get_sha1("HEAD", sha1) != 0)
+			return !!active_nr;
+
+		init_revisions(&rev, "");
+		rev.abbrev = 0;
+		(void)setup_revisions(2, argv, &rev, NULL);
+		DIFF_OPT_SET(&rev.diffopt, QUIET);
+		DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
+		(void)run_diff_index(&rev, 1 /* cached */);
+
+		return !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
+	}
+
 	if (in_merge && !no_edit)
 		fprintf(fp,
 			"#\n"
-- 
1.5.3.6.1006.g08f2

-

[ message continues ]

is_initial is left over from "development process". Well, from
stealing out of wt-status.c

-

[ message continues ]

Hi,


Then I have to wonder if it would not be a better idea to refactor the 
code, so that other people do not have to steal the code again, but are 

How does it break Vim word completion?  And why should something like

		DIFF_OPT_SET(&rev.diffopt, QUIET | EXIT_WITH_STATUS);


Don't want to be anal here, but are there possibly reasons (read "possible 

(void)?

Besides, would this not be more elegant as

		setup_revisions(0, NULL, &rev, "HEAD");


(void)?

Other than that (including my remark about refactoring that piece of 
code), I like it.

Thanks,
Dscho

-

[ message continues ]

Not sure it will be worth the effort. It is really short.

OTOH, I missed a diff-index interface where I could pass a resolved
sha1 (the u8 array returned by get_sha1) and index state. Something
like "int diff_tree_index(const unsigned char *sha1, struct rev_info *)".

Tempting, but I have only one use case for it. I don't actually know

Oh, this would look ok. It just wont compile: DIFF_OPT_SET prepends
second argument with DIFF_OPT_:

#define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
#define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)

Definitely. I just don't know. OTOH, I can only return "committable"
or "not committable". If I return "commitable" for a broken repo,
it should fail elsewhere. If I return "not commitable" git-commit
shall finish telling user there is nothing to commit, which is just

Yep. Sorry, I just got carried away by recent tendency (here at work)

Hmm... And I was so puzzled as to what that "def" argument could


Me too: I have *extensively* tested it today and a commit on the
2.6GHz/2Gb/SATA windows machine is almost as fast as on my linux
laptop now (Centrino/1.2GHz downclocked to 800MHz/384Mb/IDE).

-

[ message continues ]

Like when it is already specified through -C, -F or -m to git-commit.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Here it goes.

 builtin-commit.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index a35881e..1a9a256 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -367,6 +367,28 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 
 	strbuf_release(&sb);
 
+	if (no_edit) {
+		struct rev_info rev;
+		unsigned char sha1[40];
+
+		fclose(fp);
+
+		if (!active_nr && read_cache() < 0)
+			die("Cannot read index");
+
+		if (get_sha1("HEAD", sha1) != 0)
+			return !!active_nr;
+
+		init_revisions(&rev, "");
+		rev.abbrev = 0;
+		setup_revisions(0, NULL, &rev, "HEAD");
+		DIFF_OPT_SET(&rev.diffopt, QUIET);
+		DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
+		run_diff_index(&rev, 1 /* cached */);
+
+		return !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
+	}
+
 	if (in_merge && !no_edit)
 		fprintf(fp,
 			"#\n"
-- 
1.5.3.6.1014.g3543

-

[ message continues ]

Hi,




I guess it is good enough.  Just wanted to point out that this can fail if 


Yes, I suspected something like this would happen.

Ciao,
Dscho

-

[ message continues ]