threads / patch / 11031

patchbuiltin-commit: add --cached to operate only on index

Subject: [PATCH] builtin-commit: add --cached to operate only on index

## tl;dr

14 messages between Nov 27, 2007 and Nov 28, 2007. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

Alex Riesen· Nov 27, 2007, 10:54 UTC · lore

The option is to enable operation exclusively on the index, without touching working tree. Besides speeding up commit process on performance-challenged platforms it also has to ensure that the index is commited exactly how user sees it.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 builtin-commit.c  |   15 +++++++++++++--
 t/t7502-commit.sh |   30 ++++++++++++++++++++++++++++++
 wt-status.c       |    6 ++++--
 wt-status.h       |    1 +
 4 files changed, 48 insertions(+), 4 deletions(-)
Triggered by perpetual Windows/Cygwin problems.
From b7ccb6ef94fe8ed7e9ad232ba9ac772008b2b94f Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Tue, 27 Nov 2007 11:46:39 +0100
Subject: [PATCH] builtin-commit: add --cached to operate only on index

The option is to enable operation exclusively on the index, without touching working tree. Besides speeding up commit process on performance-challenged platforms it also has to ensure that the index is commited exactly how user sees it.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 builtin-commit.c  |   15 +++++++++++++--
 t/t7502-commit.sh |   30 ++++++++++++++++++++++++++++++
 wt-status.c       |    6 ++++--
 wt-status.h       |    1 +
 4 files changed, 48 insertions(+), 4 deletions(-)
Show changes to 4 files +48 −4

builtin-commit.c, t/t7502-commit.sh, wt-status.c, wt-status.h

diff --git a/builtin-commit.c b/builtin-commit.c
index a35881e..6aa459e 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -44,6 +44,7 @@ static int all, edit_flag, also, interactive, only, amend, signoff;
 static int quiet, verbose, untracked_files, no_verify;
 
 static int no_edit, initial_commit, in_merge;
+static int cached_only;
 const char *only_include_assumed;
 struct strbuf message;
 
@@ -82,6 +83,7 @@ static struct option builtin_commit_options[] = {
 	OPT_BOOLEAN('n', "no-verify", &no_verify, "bypass pre-commit hook"),
 	OPT_BOOLEAN(0, "amend", &amend, "amend previous commit"),
 	OPT_BOOLEAN(0, "untracked-files", &untracked_files, "show all untracked files"),
+	OPT_BOOLEAN(0, "cached", &cached_only, "consider only cached files"),
 
 	OPT_END()
 };
@@ -164,6 +166,10 @@ static char *prepare_index(const char **files, const char *prefix)
 	struct path_list partial;
 	const char **pathspec = NULL;
 
+	if (cached_only) {
+		commit_style = COMMIT_AS_IS;
+		return get_index_file();
+	}
 	if (interactive) {
 		interactive_add();
 		commit_style = COMMIT_AS_IS;
@@ -287,6 +293,7 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix)
 	}
 	s.verbose = verbose;
 	s.untracked = untracked_files;
+	s.cached_only = cached_only;
 	s.index_file = index_file;
 	s.fp = fp;
 
@@ -550,6 +557,8 @@ static int parse_and_validate_options(int argc, const char *argv[])
 			free(enc);
 	}
 
+	if (all)
+		cached_only = 0;
 	if (!!also + !!only + !!all + !!interactive > 1)
 		die("Only one of --include/--only/--all/--interactive can be used.");
 	if (argc == 0 && (also || (only && !amend)))
@@ -560,11 +569,12 @@ static int parse_and_validate_options(int argc, const char *argv[])
 		only_include_assumed = "Explicit paths specified without -i nor -o; assuming --only paths...";
 		also = 0;
 	}
-
 	if (all && argc > 0)
 		die("Paths with -a does not make sense.");
 	else if (interactive && argc > 0)
 		die("Paths with --interactive does not make sense.");
+	else if (cached_only && argc > 0)
+		die("Paths with --cached does not make sense.");
 
 	return argc;
 }
@@ -678,7 +688,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	}
 
 	if (!prepare_log_message(index_file, prefix) && !in_merge) {
-		run_status(stdout, index_file, prefix);
+		if (!cached_only)
+			run_status(stdout, index_file, prefix);
 		rollback_index_files();
 		unlink(commit_editmsg);
 		return 1;
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 21ac785..bcfadd7 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -89,4 +89,34 @@ test_expect_success 'verbose' '
 
 '
 
+test_expect_success 'cached only' '
+
+	git reset --hard &&
+	echo >file1 &&
+	echo >file2 &&
+	git add file1 file2 &&
+	git commit --cached -mcached1
+
+'
+
+test_expect_success 'cached only (ignore uncached)' '
+
+	git reset --hard &&
+	echo file1 >file1 &&
+	echo file2 >file2 &&
+	git add file1 &&
+	git status --cached | grep file2
+	test $? != 0
+
+'
+
+test_expect_success 'do not commit if index unchanged' '
+
+	git reset --hard &&
+	echo change >file1 &&
+	git commit --cached -mcached2
+	test $? != 0
+
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index 0e0439f..9306491 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -367,8 +367,10 @@ void wt_status_print(struct wt_status *s)
 		wt_status_print_updated(s);
 	}
 
-	wt_status_print_changed(s);
-	wt_status_print_untracked(s);
+	if (!s->cached_only) {
+		wt_status_print_changed(s);
+		wt_status_print_untracked(s);
+	}
 
 	if (s->verbose && !s->is_initial)
 		wt_status_print_verbose(s);
diff --git a/wt-status.h b/wt-status.h
index 225fb4d..f50e780 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -17,6 +17,7 @@ struct wt_status {
 	int verbose;
 	int amend;
 	int untracked;
+	int cached_only;
 	/* These are computed during processing of the individual sections */
 	int commitable;
 	int workdir_dirty;
-- 
1.5.3.6.1008.gbaccf
Johannes Schindelin· Nov 27, 2007, 11:18 UTC · re: Alex Riesen · lore

Re: [PATCH] builtin-commit: add --cached to operate only on index

Hi,

[commenting on the patch is a little cumbersome when the mailer does not quote it for you...]

On Tue, 27 Nov 2007, Alex Riesen wrote:
Show changes to diff +4 −1
@@ -550,6 +557,8 @@ static int parse_and_validate_options(int argc, const char *argv[])
                        free(enc);
        }

+       if (all)
+               cached_only = 0;
        if (!!also + !!only + !!all + !!interactive > 1)
                die("Only one of --include/--only/--all/--interactive can be used.");
        if (argc == 0 && (also || (only && !amend)))



My reply> I would rather add another !!cached_only to the existing if().



@@ -678,7 +688,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
        }

        if (!prepare_log_message(index_file, prefix) && !in_merge) {
-               run_status(stdout, index_file, prefix);
+               if (!cached_only)
+                       run_status(stdout, index_file, prefix);
                rollback_index_files();
                unlink(commit_editmsg);
                return 1;


My reply> Would it not make more sense to avoid run_status() when no_edit?

My reply> Ciao,
My reply> Dscho
Alex Riesen· Nov 27, 2007, 12:57 UTC · re: Johannes Schindelin · lore

Re: [PATCH] builtin-commit: add --cached to operate only on index

On 27/11/2007, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> [commenting on the patch is a little cumbersome when the mailer does not
> quote it for you...]

Yes, I'm sorry. But being at work I'm extremely lucky to have access to even gmail (it was blocked for almost a month as a virus source. Of course it is a virus source! It is a damn mailbox!)

Show 10 quoted lines
> On Tue, 27 Nov 2007, Alex Riesen wrote:
>
> @@ -550,6 +557,8 @@ static int parse_and_validate_options(int argc, const char *argv[])
>                         free(enc);
>         }
>
> +       if (all)
> +               cached_only = 0;
>
> My reply> I would rather add another !!cached_only to the existing if().
I'm... Not sure. I'm using it like this:
   $ alias gci='git commit --cached'
   $ gci

So sometimes I actually want to override --cached, and just so happens it is always --all case, which seem to be in line with proposals to make --all default commit behavior.

Show 9 quoted lines
> @@ -678,7 +688,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>         }
>
>         if (!prepare_log_message(index_file, prefix) && !in_merge) {
> -               run_status(stdout, index_file, prefix);
> +               if (!cached_only)
> +                       run_status(stdout, index_file, prefix);
>
> My reply> Would it not make more sense to avoid run_status() when no_edit?
Will try it out this evening at home. I suspect you're right :)
Johannes Sixt· Nov 27, 2007, 11:35 UTC · re: Alex Riesen · lore

Re: [PATCH] builtin-commit: add --cached to operate only on index

Alex Riesen schrieb:
> The option is to enable operation exclusively on the index, without touching
> working tree. Besides speeding up commit process on performance-challenged
> platforms it also has to ensure that the index is commited exactly how
> user sees it.
Huh?

Doesn't git-commit operate only on the index, unless you pass it extra arguments?

What am I missing?
-- Hannes
Alex Riesen· Nov 27, 2007, 12:48 UTC · re: Johannes Sixt · lore

Re: [PATCH] builtin-commit: add --cached to operate only on index

On 27/11/2007, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 10 quoted lines
> Alex Riesen schrieb:
> > The option is to enable operation exclusively on the index, without touching
> > working tree. Besides speeding up commit process on performance-challenged
> > platforms it also has to ensure that the index is commited exactly how
> > user sees it.
>
> Huh?
>
> Doesn't git-commit operate only on the index, unless you pass it extra
> arguments?
It doesn't
> What am I missing?
run_status and check for changed files
Junio C Hamano· Nov 27, 2007, 17:16 UTC · re: Alex Riesen · lore

Re: [PATCH] builtin-commit: add --cached to operate only on index

"Alex Riesen" <raa.lkml@gmail.com> writes:
Show 8 quoted lines
>> Doesn't git-commit operate only on the index, unless you pass it extra
>> arguments?
>
> It doesn't
>
>> What am I missing?
>
> run_status and check for changed files

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.
Alex Riesen· Nov 27, 2007, 18:12 UTC · re: Junio C Hamano · lore

Re: [PATCH] builtin-commit: add --cached to operate only on index

On 27/11/2007, Junio C Hamano <gitster@pobox.com> wrote:
> 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)".

I don't just mean to avoid lstat. I'm trying to avoid _any_ interaction with the thing unless absolutely needed.

>  * 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";

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 no different from HEAD) and I want commit it now.

>  * Skip run_status() and replace with "diff-index --cached HEAD" (or "is
>    the index empty?") when the user instructs so;
Right. Is it not what happens with the patch?
Alex Riesen· Nov 27, 2007, 18:18 UTC · re: Junio C Hamano · lore

Re: [PATCH] builtin-commit: add --cached to operate only on index

On 27/11/2007, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> 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.

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.

Alex Riesen· Nov 27, 2007, 21:44 UTC · re: Alex Riesen · lore

[PATCH] Do not generate full commit log message if it not going to be used

Like when it is already specified through -C, -F or -m to git-commit.
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Alex Riesen, Tue, Nov 27, 2007 19:18:56 +0100:
Show 9 quoted lines
> On 27/11/2007, Junio C Hamano <gitster@pobox.com> wrote:
> > ... 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.
> 
> This is of course very useful optimization, and will speed up things
> everywhere (and especially here).

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(-)
Show changes to builtin-commit.c +24 −0
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
Alex Riesen· Nov 27, 2007, 21:47 UTC · re: Alex Riesen · lore

Re: [PATCH] Do not generate full commit log message if it not going to be used

Alex Riesen, Tue, Nov 27, 2007 22:44:25 +0100:
Show 11 quoted lines
> --- 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;

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

Johannes Schindelin· Nov 28, 2007, 12:18 UTC · re: Alex Riesen · lore

Re: [PATCH] Do not generate full commit log message if it not going to be used

Hi,
On Tue, 27 Nov 2007, Alex Riesen wrote:
> Could not stop myself. Hopefully didn't beat anyone to it :)
> Almost all code shamelessly stolen from builtin-diff-index.c.

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 able to reuse it ;-)

> 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.
How does it break Vim word completion?  And why should something like
		DIFF_OPT_SET(&rev.diffopt, QUIET | EXIT_WITH_STATUS);
look ugly?  I find it highly readable.
Show 13 quoted lines
> +	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;

Don't want to be anal here, but are there possibly reasons (read "possible errors") other than an empty repo where this triggers?

> +
> +		init_revisions(&rev, "");
> +		rev.abbrev = 0;
> +		(void)setup_revisions(2, argv, &rev, NULL);
(void)?
Besides, would this not be more elegant as
		setup_revisions(0, NULL, &rev, "HEAD");
Hmm?
> +		(void)run_diff_index(&rev, 1 /* cached */);
(void)?

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

Thanks, Dscho

Alex Riesen· Nov 28, 2007, 21:10 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Do not generate full commit log message if it not going to be used

Johannes Schindelin, Wed, Nov 28, 2007 13:18:10 +0100:
Show 8 quoted lines
> On Tue, 27 Nov 2007, Alex Riesen wrote:
> 
> > Could not stop myself. Hopefully didn't beat anyone to it :)
> > Almost all code shamelessly stolen from builtin-diff-index.c.
> 
> 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 
> able to reuse it ;-)
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 Git's code that well...

Show 9 quoted lines
> > 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.
> 
> How does it break Vim word completion?  And why should something like
> 
> 		DIFF_OPT_SET(&rev.diffopt, QUIET | EXIT_WITH_STATUS);
> 
> look ugly?  I find it highly readable.

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) #define DIFF_OPT_CLR(opts, flag) ((opts)->flags &= ~DIFF_OPT_##flag)

Show 16 quoted lines
> > +	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;
> 
> Don't want to be anal here, but are there possibly reasons (read "possible 
> errors") other than an empty repo where this triggers?

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 wrong.

Show 6 quoted lines
> > +
> > +		init_revisions(&rev, "");
> > +		rev.abbrev = 0;
> > +		(void)setup_revisions(2, argv, &rev, NULL);
> 
> (void)?

Yep. Sorry, I just got carried away by recent tendency (here at work) to shut up PC-Lint (but please don't ask).

> Besides, would this not be more elegant as
> 
> 		setup_revisions(0, NULL, &rev, "HEAD");

Hmm... And I was so puzzled as to what that "def" argument could possibly mean... Still am, in fact. But it works.

> > +		(void)run_diff_index(&rev, 1 /* cached */);
> 
> (void)?
I'll remove them and resubmit. Stupid custom.
> Other than that (including my remark about refactoring that piece of 
> code), I like it.

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).

Alex Riesen· Nov 28, 2007, 21:13 UTC · re: Alex Riesen · lore

[PATCH] Do not generate full commit log message if it is not going to be used

Like when it is already specified through -C, -F or -m to git-commit.
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Alex Riesen, Wed, Nov 28, 2007 22:10:59 +0100:
Show 14 quoted lines
> Johannes Schindelin, Wed, Nov 28, 2007 13:18:10 +0100:
> > Besides, would this not be more elegant as
> > 
> > 		setup_revisions(0, NULL, &rev, "HEAD");
> 
> Hmm... And I was so puzzled as to what that "def" argument could
> possibly mean... Still am, in fact. But it works.
> 
> > > +		(void)run_diff_index(&rev, 1 /* cached */);
> > 
> > (void)?
> 
> I'll remove them and resubmit. Stupid custom.
> 
Here it goes.
 builtin-commit.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)
Show changes to builtin-commit.c +22 −0
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
Johannes Schindelin· Nov 28, 2007, 21:43 UTC · re: Alex Riesen · lore

Re: [PATCH] Do not generate full commit log message if it not going to be used

Hi,
On Wed, 28 Nov 2007, Alex Riesen wrote:
Show 11 quoted lines
> Johannes Schindelin, Wed, Nov 28, 2007 13:18:10 +0100:
> > On Tue, 27 Nov 2007, Alex Riesen wrote:
> > 
> > > Could not stop myself. Hopefully didn't beat anyone to it :)
> > > Almost all code shamelessly stolen from builtin-diff-index.c.
> > 
> > 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 
> > able to reuse it ;-)
> 
> Not sure it will be worth the effort. It is really short.
Okay.
Show 16 quoted lines
> > > 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.
> > 
> > How does it break Vim word completion?  And why should something like
> > 
> > 		DIFF_OPT_SET(&rev.diffopt, QUIET | EXIT_WITH_STATUS);
> > 
> > look ugly?  I find it highly readable.
> 
> 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)
> #define DIFF_OPT_CLR(opts, flag)    ((opts)->flags &= ~DIFF_OPT_##flag)
Ouch.  Sorry for suggesting it when I clearly had no clue.
Show 19 quoted lines
> > > +	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;
> > 
> > Don't want to be anal here, but are there possibly reasons (read "possible 
> > errors") other than an empty repo where this triggers?
> 
> Definitely. I just don't know. OTOH, I can only return "committable" or 
> "not committable".

I guess it is good enough. Just wanted to point out that this can fail if .git/HEAD is not readable.

Show 6 quoted lines
> > Besides, would this not be more elegant as
> > 
> > 		setup_revisions(0, NULL, &rev, "HEAD");
> 
> Hmm... And I was so puzzled as to what that "def" argument could
> possibly mean... Still am, in fact. But it works.
It is the mechanism which makes "git log" default to "git log HEAD".
Show 6 quoted lines
> > Other than that (including my remark about refactoring that piece of 
> > code), I like it.
> 
> 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).
Yes, I suspected something like this would happen.

Ciao, Dscho

← back to recent threads