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

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

[Alex Riesen]

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

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-cached-to-builtin-commit-operate-only-on-inde.patch --]
[-- Type: text/x-patch; name=0001-Add-cached-to-builtin-commit-operate-only-on-inde.patch, Size: 4642 bytes --]

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

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

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

[Johannes Schindelin]

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:

@@ -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

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

[Alex Riesen]

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

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

> @@ -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 :)

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

[Johannes Sixt]

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

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

[Alex Riesen]

On 27/11/2007, Johannes Sixt <j.sixt@viscovery.net> wrote:
> 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

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

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

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

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

[Alex Riesen]

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?

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

[Alex Riesen]

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

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

[Alex Riesen]

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:
> 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(-)

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

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

[Alex Riesen]

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

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

[Johannes Schindelin]

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.

> +	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

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

[Alex Riesen]

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.

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

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

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

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

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

[Alex Riesen]

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:
> 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(-)

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

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

[Johannes Schindelin]

Hi,

On Wed, 28 Nov 2007, Alex Riesen wrote:

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

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

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

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

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