{"thread":{"id":"11031","subject":"[PATCH] builtin-commit: add --cached to operate only on index","startedAt":"2007-11-27T10:54:52Z","lastAt":"2007-11-28T21:43:12Z","messageCount":14,"participants":["Alex Riesen","Johannes Schindelin","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"61045","messageId":"81b0412b0711270254i58be4d2fi5021767d99fcb753@mail.gmail.com","threadId":"11031","inReplyTo":null,"subject":"[PATCH] builtin-commit: add --cached to operate only on index","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-11-27T10:54:52Z","receivedAt":"2007-11-27T10:54:52Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"The option is to enable operation exclusively on the index, without touching\nworking tree. Besides speeding up commit process on performance-challenged\nplatforms it also has to ensure that the index is commited exactly how\nuser sees it.\n\nSigned-off-by: Alex Riesen \n---\n builtin-commit.c | 15 +++++++++++++--\n t/t7502-commit.sh | 30 ++++++++++++++++++++++++++++++\n wt-status.c | 6 ++++--\n wt-status.h | 1 +\n 4 files changed, 48 insertions(+), 4 deletions(-)\n\nTriggered by perpetual Windows/Cygwin problems.\n\n\nFrom b7ccb6ef94fe8ed7e9ad232ba9ac772008b2b94f Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen \nDate: Tue, 27 Nov 2007 11:46:39 +0100\nSubject: [PATCH] builtin-commit: add --cached to operate only on index\n\nThe option is to enable operation exclusively on the index, without touching\nworking tree. Besides speeding up commit process on performance-challenged\nplatforms it also has to ensure that the index is commited exactly how\nuser sees it.\n\nSigned-off-by: Alex Riesen \n---\n builtin-commit.c | 15 +++++++++++++--\n t/t7502-commit.sh | 30 ++++++++++++++++++++++++++++++\n wt-status.c | 6 ++++--\n wt-status.h | 1 +\n 4 files changed, 48 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex a35881e..6aa459e 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -44,6 +44,7 @@ static int all, edit_flag, also, interactive, only, amend, signoff;\n static int quiet, verbose, untracked_files, no_verify;\n \n static int no_edit, initial_commit, in_merge;\n+static int cached_only;\n const char *only_include_assumed;\n struct strbuf message;\n \n@@ -82,6 +83,7 @@ static struct option builtin_commit_options[] = {\n \tOPT_BOOLEAN('n', \"no-verify\", &no_verify, \"bypass pre-commit hook\"),\n \tOPT_BOOLEAN(0, \"amend\", &amend, \"amend previous commit\"),\n \tOPT_BOOLEAN(0, \"untracked-files\", &untracked_files, \"show all untracked files\"),\n+\tOPT_BOOLEAN(0, \"cached\", &cached_only, \"consider only cached files\"),\n \n \tOPT_END()\n };\n@@ -164,6 +166,10 @@ static char *prepare_index(const char **files, const char *prefix)\n \tstruct path_list partial;\n \tconst char **pathspec = NULL;\n \n+\tif (cached_only) {\n+\t\tcommit_style = COMMIT_AS_IS;\n+\t\treturn get_index_file();\n+\t}\n \tif (interactive) {\n \t\tinteractive_add();\n \t\tcommit_style = COMMIT_AS_IS;\n@@ -287,6 +293,7 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix)\n \t}\n \ts.verbose = verbose;\n \ts.untracked = untracked_files;\n+\ts.cached_only = cached_only;\n \ts.index_file = index_file;\n \ts.fp = fp;\n \n@@ -550,6 +557,8 @@ static int parse_and_validate_options(int argc, const char *argv[])\n \t\t\tfree(enc);\n \t}\n \n+\tif (all)\n+\t\tcached_only = 0;\n \tif (!!also + !!only + !!all + !!interactive > 1)\n \t\tdie(\"Only one of --include/--only/--all/--interactive can be used.\");\n \tif (argc == 0 && (also || (only && !amend)))\n@@ -560,11 +569,12 @@ static int parse_and_validate_options(int argc, const char *argv[])\n \t\tonly_include_assumed = \"Explicit paths specified without -i nor -o; assuming --only paths...\";\n \t\talso = 0;\n \t}\n-\n \tif (all && argc > 0)\n \t\tdie(\"Paths with -a does not make sense.\");\n \telse if (interactive && argc > 0)\n \t\tdie(\"Paths with --interactive does not make sense.\");\n+\telse if (cached_only && argc > 0)\n+\t\tdie(\"Paths with --cached does not make sense.\");\n \n \treturn argc;\n }\n@@ -678,7 +688,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (!prepare_log_message(index_file, prefix) && !in_merge) {\n-\t\trun_status(stdout, index_file, prefix);\n+\t\tif (!cached_only)\n+\t\t\trun_status(stdout, index_file, prefix);\n \t\trollback_index_files();\n \t\tunlink(commit_editmsg);\n \t\treturn 1;\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex 21ac785..bcfadd7 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -89,4 +89,34 @@ test_expect_success 'verbose' '\n \n '\n \n+test_expect_success 'cached only' '\n+\n+\tgit reset --hard &&\n+\techo >file1 &&\n+\techo >file2 &&\n+\tgit add file1 file2 &&\n+\tgit commit --cached -mcached1\n+\n+'\n+\n+test_expect_success 'cached only (ignore uncached)' '\n+\n+\tgit reset --hard &&\n+\techo file1 >file1 &&\n+\techo file2 >file2 &&\n+\tgit add file1 &&\n+\tgit status --cached | grep file2\n+\ttest $? != 0\n+\n+'\n+\n+test_expect_success 'do not commit if index unchanged' '\n+\n+\tgit reset --hard &&\n+\techo change >file1 &&\n+\tgit commit --cached -mcached2\n+\ttest $? != 0\n+\n+'\n+\n test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 0e0439f..9306491 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -367,8 +367,10 @@ void wt_status_print(struct wt_status *s)\n \t\twt_status_print_updated(s);\n \t}\n \n-\twt_status_print_changed(s);\n-\twt_status_print_untracked(s);\n+\tif (!s->cached_only) {\n+\t\twt_status_print_changed(s);\n+\t\twt_status_print_untracked(s);\n+\t}\n \n \tif (s->verbose && !s->is_initial)\n \t\twt_status_print_verbose(s);\ndiff --git a/wt-status.h b/wt-status.h\nindex 225fb4d..f50e780 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -17,6 +17,7 @@ struct wt_status {\n \tint verbose;\n \tint amend;\n \tint untracked;\n+\tint cached_only;\n \t/* These are computed during processing of the individual sections */\n \tint commitable;\n \tint workdir_dirty;\n-- \n1.5.3.6.1008.gbaccf\n\n"},{"id":"61049","messageId":"Pine.LNX.4.64.0711271115300.27959@racer.site","threadId":"11031","inReplyTo":"81b0412b0711270254i58be4d2fi5021767d99fcb753@mail.gmail.com","subject":"Re: [PATCH] builtin-commit: add --cached to operate only on index","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-27T11:18:38Z","receivedAt":"2007-11-27T11:18:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\n[commenting on the patch is a little cumbersome when the mailer does not \nquote it for you...]\n\nOn Tue, 27 Nov 2007, Alex Riesen wrote:\n\n@@ -550,6 +557,8 @@ static int parse_and_validate_options(int argc, const char *argv[])\n free(enc);\n }\n\n+ if (all)\n+ cached_only = 0;\n if (!!also + !!only + !!all + !!interactive > 1)\n die(\"Only one of --include/--only/--all/--interactive can be used.\");\n if (argc == 0 && (also || (only && !amend)))\n\n\n\nMy reply> I would rather add another !!cached_only to the existing if().\n\n\n\n@@ -678,7 +688,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n }\n\n if (!prepare_log_message(index_file, prefix) && !in_merge) {\n- run_status(stdout, index_file, prefix);\n+ if (!cached_only)\n+ run_status(stdout, index_file, prefix);\n rollback_index_files();\n unlink(commit_editmsg);\n return 1;\n\n\nMy reply> Would it not make more sense to avoid run_status() when no_edit?\n\nMy reply> Ciao,\nMy reply> Dscho\n"},{"id":"61055","messageId":"474C0105.3010908@viscovery.net","threadId":"11031","inReplyTo":"81b0412b0711270254i58be4d2fi5021767d99fcb753@mail.gmail.com","subject":"Re: [PATCH] builtin-commit: add --cached to operate only on index","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-11-27T11:35:33Z","receivedAt":"2007-11-27T11:35:33Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Alex Riesen schrieb:\n> The option is to enable operation exclusively on the index, without touching\n> working tree. Besides speeding up commit process on performance-challenged\n> platforms it also has to ensure that the index is commited exactly how\n> user sees it.\n\nHuh?\n\nDoesn't git-commit operate only on the index, unless you pass it extra \narguments?\n\nWhat am I missing?\n\n-- Hannes\n"},{"id":"61063","messageId":"81b0412b0711270448s6534a849u86bcb161d4d7b3fe@mail.gmail.com","threadId":"11031","inReplyTo":"474C0105.3010908@viscovery.net","subject":"Re: [PATCH] builtin-commit: add --cached to operate only on index","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-11-27T12:48:02Z","receivedAt":"2007-11-27T12:48:02Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 27/11/2007, Johannes Sixt wrote:\n> Alex Riesen schrieb:\n> > The option is to enable operation exclusively on the index, without touching\n> > working tree. Besides speeding up commit process on performance-challenged\n> > platforms it also has to ensure that the index is commited exactly how\n> > user sees it.\n>\n> Huh?\n>\n> Doesn't git-commit operate only on the index, unless you pass it extra\n> arguments?\n\nIt doesn't\n\n> What am I missing?\n\nrun_status and check for changed files\n"},{"id":"61065","messageId":"81b0412b0711270457i1d0a05a2nc2715f1918f26f31@mail.gmail.com","threadId":"11031","inReplyTo":"Pine.LNX.4.64.0711271115300.27959@racer.site","subject":"Re: [PATCH] builtin-commit: add --cached to operate only on index","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-11-27T12:57:34Z","receivedAt":"2007-11-27T12:57:34Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 27/11/2007, Johannes Schindelin wrote:\n> [commenting on the patch is a little cumbersome when the mailer does not\n> quote it for you...]\n\nYes, I'm sorry. But being at work I'm extremely lucky to have access\nto even gmail (it was blocked for almost a month as a virus source. Of\ncourse it is a virus source! It is a damn mailbox!)\n\n> On Tue, 27 Nov 2007, Alex Riesen wrote:\n>\n> @@ -550,6 +557,8 @@ static int parse_and_validate_options(int argc, const char *argv[])\n> free(enc);\n> }\n>\n> + if (all)\n> + cached_only = 0;\n>\n> My reply> I would rather add another !!cached_only to the existing if().\n\nI'm... Not sure. I'm using it like this:\n\n $ alias gci='git commit --cached'\n $ gci\n\nSo sometimes I actually want to override --cached, and just so happens\nit is always --all case, which seem to be in line with proposals to make\n--all default commit behavior.\n\n> @@ -678,7 +688,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> }\n>\n> if (!prepare_log_message(index_file, prefix) && !in_merge) {\n> - run_status(stdout, index_file, prefix);\n> + if (!cached_only)\n> + run_status(stdout, index_file, prefix);\n>\n> My reply> Would it not make more sense to avoid run_status() when no_edit?\n\nWill try it out this evening at home. I suspect you're right :)\n"},{"id":"61105","messageId":"7vfxyrd2x2.fsf@gitster.siamese.dyndns.org","threadId":"11031","inReplyTo":"81b0412b0711270448s6534a849u86bcb161d4d7b3fe@mail.gmail.com","subject":"Re: [PATCH] builtin-commit: add --cached to operate only on index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-27T17:16:41Z","receivedAt":"2007-11-27T17:16:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" writes:\n\n>> Doesn't git-commit operate only on the index, unless you pass it extra\n>> arguments?\n>\n> It doesn't\n>\n>> What am I missing?\n>\n> run_status and check for changed files\n\nI am sympathetic to the _cause_, but I do not think the option --cached\nis a good match for this change. As Hannes points out, as-is commit is\nthe default, and --cached to other commands mean \"work only with index\nnot work tree\", not \"short-circuit for systems with slow lstat(3)\".\n\nObviously we cannot short-circuit checking for modified or removed paths\nwhen \"git-commit -a\" is run, but it is plausible that people may still\nwant to trade run_status output with interactive speed even when doing\n\"git-commit -a\".\n\nOn the other hand, when we know we do not have to _show_ the list of\nstaged/modified/untracked files (i.e. we already have the commit log\nmessage via -m, -F, or -C and we were told not to invoke editor), we do\nnot have to call run_status(), only to discard its output. In such a\ncase, we are calling it only to see if we have something committable,\nand we should be able to optimize THAT without being told by the user\nwith this new option. Incidentally I just checked the scripted version;\nit does not do this optimization (git-commit.sh, ll. 514-517). The C\nrewrite in 'next' does not have it in either (builtin-commit.c,\nll. 387-390). When no_edit is in effect, I think these two places can\nbe replaced with an equivalent of \"diff-index --cached HEAD --\" (which\nshould not hit the work tree at all) to see if there is anything to be\ncommitted. For initial commit the check would obviously be \"is the\nindex empty?\" instead.\n\nSo in short:\n\n * The option \"--cached\" is a wrong thing to have the user say and is\n not what you want anyway. You want \"no status list in the commit log\n template\";\n\n * Skip run_status() and replace with \"diff-index --cached HEAD\" (or \"is\n the index empty?\") when the user instructs so;\n\n * In addition, the same optimization should apply when we know we do\n not use the exact run_status() output.\n"},{"id":"61112","messageId":"81b0412b0711271012v67f1058fj8c0de8e511543d4e@mail.gmail.com","threadId":"11031","inReplyTo":"7vfxyrd2x2.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-commit: add --cached to operate only on index","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-11-27T18:12:30Z","receivedAt":"2007-11-27T18:12:30Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 27/11/2007, Junio C Hamano wrote:\n> I am sympathetic to the _cause_, but I do not think the option --cached\n> is a good match for this change. As Hannes points out, as-is commit is\n> the default, and --cached to other commands mean \"work only with index\n> not work tree\", not \"short-circuit for systems with slow lstat(3)\".\n\nI don't just mean to avoid lstat. I'm trying to avoid _any_\ninteraction with the thing unless absolutely needed.\n\n> * The option \"--cached\" is a wrong thing to have the user say and is\n> not what you want anyway. You want \"no status list in the commit log\n> template\";\n\nThat is not what I meant to say. I do want status list in commit message.\nI don't want anything except git-diff-index --name-status --cached HEAD in it.\nNo untracked files whatsoever.\n\nAnd, as I said, I also just want to commit the index _exactly_ as it is.\nNo checking for files changed after they were updated in the index\nintended. I think that whatever the index holds is perfect (except it's\nno different from HEAD) and I want commit it now.\n\n> * Skip run_status() and replace with \"diff-index --cached HEAD\" (or \"is\n> the index empty?\") when the user instructs so;\n\nRight. Is it not what happens with the patch?\n"},{"id":"61113","messageId":"81b0412b0711271018m6419b076n269a0175494fac84@mail.gmail.com","threadId":"11031","inReplyTo":"7vfxyrd2x2.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-commit: add --cached to operate only on index","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-11-27T18:18:56Z","receivedAt":"2007-11-27T18:18:56Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 27/11/2007, Junio C Hamano wrote:\n> On the other hand, when we know we do not have to _show_ the list of\n> staged/modified/untracked files (i.e. we already have the commit log\n> message via -m, -F, or -C and we were told not to invoke editor), we do\n> not have to call run_status(), only to discard its output. In such a\n> case, we are calling it only to see if we have something committable,\n> and we should be able to optimize THAT without being told by the user\n> with this new option. Incidentally I just checked the scripted version;\n> it does not do this optimization (git-commit.sh, ll. 514-517). The C\n> rewrite in 'next' does not have it in either (builtin-commit.c,\n> ll. 387-390). When no_edit is in effect, I think these two places can\n> be replaced with an equivalent of \"diff-index --cached HEAD --\" (which\n> should not hit the work tree at all) to see if there is anything to be\n> committed. For initial commit the check would obviously be \"is the\n> index empty?\" instead.\n\nThis is of course very useful optimization, and will speed up things\neverywhere (and especially here).\n\nBut still: I didn't mean it. I really meant the interactive case, with\neditor and prepared commit message which has status list.\nSometimes this list is the reason why a commit is aborted:\nI notice that I'm committing something I didn't really intend to.\n"},{"id":"61126","messageId":"20071127214425.GA3156@steel.home","threadId":"11031","inReplyTo":"81b0412b0711271018m6419b076n269a0175494fac84@mail.gmail.com","subject":"[PATCH] Do not generate full commit log message if it not going to be used","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-11-27T21:44:25Z","receivedAt":"2007-11-27T21:44:25Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Like when it is already specified through -C, -F or -m to git-commit.\n\nSigned-off-by: Alex Riesen \n---\n\nAlex Riesen, Tue, Nov 27, 2007 19:18:56 +0100:\n> On 27/11/2007, Junio C Hamano wrote:\n> > ... When no_edit is in effect, I think these two places can\n> > be replaced with an equivalent of \"diff-index --cached HEAD --\" (which\n> > should not hit the work tree at all) to see if there is anything to be\n> > committed. For initial commit the check would obviously be \"is the\n> > index empty?\" instead.\n> \n> This is of course very useful optimization, and will speed up things\n> everywhere (and especially here).\n\nCould not stop myself. Hopefully didn't beat anyone to it :)\nAlmost all code shamelessly stolen from builtin-diff-index.c.\nBuilds, runs, passes all the tests. That !active_nr is\nmicro-optimization, but a nice one: clearly don't reread.\n\nPreprocessor trickery in DIFF_OPT_* macros is disgusting, it breaks\nVim word completion and trying to use many flags in one expression\nlooks just ugly. What was wrong is inline functions?\n\n builtin-commit.c | 24 ++++++++++++++++++++++++\n 1 files changed, 24 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex a35881e..8167ce4 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -367,6 +367,30 @@ static int prepare_log_message(const char *index_file, const char *prefix)\n \n \tstrbuf_release(&sb);\n \n+\tif (no_edit) {\n+\t\tstatic const char *argv[] = { NULL, \"HEAD\", NULL };\n+\t\tstruct rev_info rev;\n+\t\tunsigned char sha1[40];\n+\t\tint is_initial;\n+\n+\t\tfclose(fp);\n+\n+\t\tif (!active_nr && read_cache() < 0)\n+\t\t\tdie(\"Cannot read index\");\n+\n+\t\tif (get_sha1(\"HEAD\", sha1) != 0)\n+\t\t\treturn !!active_nr;\n+\n+\t\tinit_revisions(&rev, \"\");\n+\t\trev.abbrev = 0;\n+\t\t(void)setup_revisions(2, argv, &rev, NULL);\n+\t\tDIFF_OPT_SET(&rev.diffopt, QUIET);\n+\t\tDIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);\n+\t\t(void)run_diff_index(&rev, 1 /* cached */);\n+\n+\t\treturn !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);\n+\t}\n+\n \tif (in_merge && !no_edit)\n \t\tfprintf(fp,\n \t\t\t\"#\\n\"\n-- \n1.5.3.6.1006.g08f2\n"},{"id":"61127","messageId":"20071127214713.GB3156@steel.home","threadId":"11031","inReplyTo":"20071127214425.GA3156@steel.home","subject":"Re: [PATCH] Do not generate full commit log message if it not going to be used","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-11-27T21:47:13Z","receivedAt":"2007-11-27T21:47:13Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Alex Riesen, Tue, Nov 27, 2007 22:44:25 +0100:\n> --- a/builtin-commit.c\n> +++ b/builtin-commit.c\n> @@ -367,6 +367,30 @@ static int prepare_log_message(const char *index_file, const char *prefix)\n> \n> \tstrbuf_release(&sb);\n> \n> +\tif (no_edit) {\n> +\t\tstatic const char *argv[] = { NULL, \"HEAD\", NULL };\n> +\t\tstruct rev_info rev;\n> +\t\tunsigned char sha1[40];\n> +\t\tint is_initial;\n\nis_initial is left over from \"development process\". Well, from\nstealing out of wt-status.c\n"},{"id":"61160","messageId":"Pine.LNX.4.64.0711281211130.27959@racer.site","threadId":"11031","inReplyTo":"20071127214425.GA3156@steel.home","subject":"Re: [PATCH] Do not generate full commit log message if it not going to be used","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-28T12:18:10Z","receivedAt":"2007-11-28T12:18:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 27 Nov 2007, Alex Riesen wrote:\n\n> Could not stop myself. Hopefully didn't beat anyone to it :)\n> Almost all code shamelessly stolen from builtin-diff-index.c.\n\nThen I have to wonder if it would not be a better idea to refactor the \ncode, so that other people do not have to steal the code again, but are \nable to reuse it ;-)\n\n> Preprocessor trickery in DIFF_OPT_* macros is disgusting, it breaks Vim \n> word completion and trying to use many flags in one expression looks \n> just ugly.\n\nHow does it break Vim word completion? And why should something like\n\n\t\tDIFF_OPT_SET(&rev.diffopt, QUIET | EXIT_WITH_STATUS);\n\nlook ugly? I find it highly readable.\n\n> +\tif (no_edit) {\n> +\t\tstatic const char *argv[] = { NULL, \"HEAD\", NULL };\n> +\t\tstruct rev_info rev;\n> +\t\tunsigned char sha1[40];\n> +\t\tint is_initial;\n> +\n> +\t\tfclose(fp);\n> +\n> +\t\tif (!active_nr && read_cache() < 0)\n> +\t\t\tdie(\"Cannot read index\");\n> +\n> +\t\tif (get_sha1(\"HEAD\", sha1) != 0)\n> +\t\t\treturn !!active_nr;\n\nDon't want to be anal here, but are there possibly reasons (read \"possible \nerrors\") other than an empty repo where this triggers?\n\n> +\n> +\t\tinit_revisions(&rev, \"\");\n> +\t\trev.abbrev = 0;\n> +\t\t(void)setup_revisions(2, argv, &rev, NULL);\n\n(void)?\n\nBesides, would this not be more elegant as\n\n\t\tsetup_revisions(0, NULL, &rev, \"HEAD\");\n\nHmm?\n\n> +\t\t(void)run_diff_index(&rev, 1 /* cached */);\n\n(void)?\n\nOther than that (including my remark about refactoring that piece of \ncode), I like it.\n\nThanks,\nDscho\n"},{"id":"61289","messageId":"20071128211059.GA3173@steel.home","threadId":"11031","inReplyTo":"Pine.LNX.4.64.0711281211130.27959@racer.site","subject":"Re: [PATCH] Do not generate full commit log message if it not going to be used","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-11-28T21:10:59Z","receivedAt":"2007-11-28T21:10:59Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Johannes Schindelin, Wed, Nov 28, 2007 13:18:10 +0100:\n> On Tue, 27 Nov 2007, Alex Riesen wrote:\n> \n> > Could not stop myself. Hopefully didn't beat anyone to it :)\n> > Almost all code shamelessly stolen from builtin-diff-index.c.\n> \n> Then I have to wonder if it would not be a better idea to refactor the \n> code, so that other people do not have to steal the code again, but are \n> able to reuse it ;-)\n\nNot sure it will be worth the effort. It is really short.\n\nOTOH, I missed a diff-index interface where I could pass a resolved\nsha1 (the u8 array returned by get_sha1) and index state. Something\nlike \"int diff_tree_index(const unsigned char *sha1, struct rev_info *)\".\n\nTempting, but I have only one use case for it. I don't actually know\nGit's code that well...\n\n> > Preprocessor trickery in DIFF_OPT_* macros is disgusting, it breaks Vim \n> > word completion and trying to use many flags in one expression looks \n> > just ugly.\n> \n> How does it break Vim word completion? And why should something like\n> \n> \t\tDIFF_OPT_SET(&rev.diffopt, QUIET | EXIT_WITH_STATUS);\n> \n> look ugly? I find it highly readable.\n\nOh, this would look ok. It just wont compile: DIFF_OPT_SET prepends\nsecond argument with DIFF_OPT_:\n\n#define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag)\n#define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag)\n#define DIFF_OPT_CLR(opts, flag) ((opts)->flags &= ~DIFF_OPT_##flag)\n\n> > +\tif (no_edit) {\n> > +\t\tstatic const char *argv[] = { NULL, \"HEAD\", NULL };\n> > +\t\tstruct rev_info rev;\n> > +\t\tunsigned char sha1[40];\n> > +\t\tint is_initial;\n> > +\n> > +\t\tfclose(fp);\n> > +\n> > +\t\tif (!active_nr && read_cache() < 0)\n> > +\t\t\tdie(\"Cannot read index\");\n> > +\n> > +\t\tif (get_sha1(\"HEAD\", sha1) != 0)\n> > +\t\t\treturn !!active_nr;\n> \n> Don't want to be anal here, but are there possibly reasons (read \"possible \n> errors\") other than an empty repo where this triggers?\n\nDefinitely. I just don't know. OTOH, I can only return \"committable\"\nor \"not committable\". If I return \"commitable\" for a broken repo,\nit should fail elsewhere. If I return \"not commitable\" git-commit\nshall finish telling user there is nothing to commit, which is just\nwrong.\n\n> > +\n> > +\t\tinit_revisions(&rev, \"\");\n> > +\t\trev.abbrev = 0;\n> > +\t\t(void)setup_revisions(2, argv, &rev, NULL);\n> \n> (void)?\n\nYep. Sorry, I just got carried away by recent tendency (here at work)\nto shut up PC-Lint (but please don't ask).\n\n> Besides, would this not be more elegant as\n> \n> \t\tsetup_revisions(0, NULL, &rev, \"HEAD\");\n\nHmm... And I was so puzzled as to what that \"def\" argument could\npossibly mean... Still am, in fact. But it works.\n\n> > +\t\t(void)run_diff_index(&rev, 1 /* cached */);\n> \n> (void)?\n\nI'll remove them and resubmit. Stupid custom.\n\n> Other than that (including my remark about refactoring that piece of \n> code), I like it.\n\nMe too: I have *extensively* tested it today and a commit on the\n2.6GHz/2Gb/SATA windows machine is almost as fast as on my linux\nlaptop now (Centrino/1.2GHz downclocked to 800MHz/384Mb/IDE).\n"},{"id":"61290","messageId":"20071128211308.GB3173@steel.home","threadId":"11031","inReplyTo":"20071128211059.GA3173@steel.home","subject":"[PATCH] Do not generate full commit log message if it is not going to be used","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-11-28T21:13:08Z","receivedAt":"2007-11-28T21:13:08Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Like when it is already specified through -C, -F or -m to git-commit.\n\nSigned-off-by: Alex Riesen \n---\nAlex Riesen, Wed, Nov 28, 2007 22:10:59 +0100:\n> Johannes Schindelin, Wed, Nov 28, 2007 13:18:10 +0100:\n> > Besides, would this not be more elegant as\n> > \n> > \t\tsetup_revisions(0, NULL, &rev, \"HEAD\");\n> \n> Hmm... And I was so puzzled as to what that \"def\" argument could\n> possibly mean... Still am, in fact. But it works.\n> \n> > > +\t\t(void)run_diff_index(&rev, 1 /* cached */);\n> > \n> > (void)?\n> \n> I'll remove them and resubmit. Stupid custom.\n> \n\nHere it goes.\n\n builtin-commit.c | 22 ++++++++++++++++++++++\n 1 files changed, 22 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex a35881e..1a9a256 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -367,6 +367,28 @@ static int prepare_log_message(const char *index_file, const char *prefix)\n \n \tstrbuf_release(&sb);\n \n+\tif (no_edit) {\n+\t\tstruct rev_info rev;\n+\t\tunsigned char sha1[40];\n+\n+\t\tfclose(fp);\n+\n+\t\tif (!active_nr && read_cache() < 0)\n+\t\t\tdie(\"Cannot read index\");\n+\n+\t\tif (get_sha1(\"HEAD\", sha1) != 0)\n+\t\t\treturn !!active_nr;\n+\n+\t\tinit_revisions(&rev, \"\");\n+\t\trev.abbrev = 0;\n+\t\tsetup_revisions(0, NULL, &rev, \"HEAD\");\n+\t\tDIFF_OPT_SET(&rev.diffopt, QUIET);\n+\t\tDIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);\n+\t\trun_diff_index(&rev, 1 /* cached */);\n+\n+\t\treturn !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);\n+\t}\n+\n \tif (in_merge && !no_edit)\n \t\tfprintf(fp,\n \t\t\t\"#\\n\"\n-- \n1.5.3.6.1014.g3543\n"},{"id":"61291","messageId":"Pine.LNX.4.64.0711282140470.27959@racer.site","threadId":"11031","inReplyTo":"20071128211059.GA3173@steel.home","subject":"Re: [PATCH] Do not generate full commit log message if it not going to be used","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-28T21:43:12Z","receivedAt":"2007-11-28T21:43:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 28 Nov 2007, Alex Riesen wrote:\n\n> Johannes Schindelin, Wed, Nov 28, 2007 13:18:10 +0100:\n> > On Tue, 27 Nov 2007, Alex Riesen wrote:\n> > \n> > > Could not stop myself. Hopefully didn't beat anyone to it :)\n> > > Almost all code shamelessly stolen from builtin-diff-index.c.\n> > \n> > Then I have to wonder if it would not be a better idea to refactor the \n> > code, so that other people do not have to steal the code again, but are \n> > able to reuse it ;-)\n> \n> Not sure it will be worth the effort. It is really short.\n\nOkay.\n\n> > > Preprocessor trickery in DIFF_OPT_* macros is disgusting, it breaks Vim \n> > > word completion and trying to use many flags in one expression looks \n> > > just ugly.\n> > \n> > How does it break Vim word completion? And why should something like\n> > \n> > \t\tDIFF_OPT_SET(&rev.diffopt, QUIET | EXIT_WITH_STATUS);\n> > \n> > look ugly? I find it highly readable.\n> \n> Oh, this would look ok. It just wont compile: DIFF_OPT_SET prepends\n> second argument with DIFF_OPT_:\n> \n> #define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag)\n> #define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag)\n> #define DIFF_OPT_CLR(opts, flag) ((opts)->flags &= ~DIFF_OPT_##flag)\n\nOuch. Sorry for suggesting it when I clearly had no clue.\n\n> > > +\tif (no_edit) {\n> > > +\t\tstatic const char *argv[] = { NULL, \"HEAD\", NULL };\n> > > +\t\tstruct rev_info rev;\n> > > +\t\tunsigned char sha1[40];\n> > > +\t\tint is_initial;\n> > > +\n> > > +\t\tfclose(fp);\n> > > +\n> > > +\t\tif (!active_nr && read_cache() < 0)\n> > > +\t\t\tdie(\"Cannot read index\");\n> > > +\n> > > +\t\tif (get_sha1(\"HEAD\", sha1) != 0)\n> > > +\t\t\treturn !!active_nr;\n> > \n> > Don't want to be anal here, but are there possibly reasons (read \"possible \n> > errors\") other than an empty repo where this triggers?\n> \n> Definitely. I just don't know. OTOH, I can only return \"committable\" or \n> \"not committable\".\n\nI guess it is good enough. Just wanted to point out that this can fail if \n.git/HEAD is not readable.\n\n> > Besides, would this not be more elegant as\n> > \n> > \t\tsetup_revisions(0, NULL, &rev, \"HEAD\");\n> \n> Hmm... And I was so puzzled as to what that \"def\" argument could\n> possibly mean... Still am, in fact. But it works.\n\nIt is the mechanism which makes \"git log\" default to \"git log HEAD\".\n\n> > Other than that (including my remark about refactoring that piece of \n> > code), I like it.\n> \n> Me too: I have *extensively* tested it today and a commit on the\n> 2.6GHz/2Gb/SATA windows machine is almost as fast as on my linux\n> laptop now (Centrino/1.2GHz downclocked to 800MHz/384Mb/IDE).\n\nYes, I suspected something like this would happen.\n\nCiao,\nDscho\n"}]}