{"thread":{"id":"12419","subject":"bug in git-revert","startedAt":"2008-03-02T06:44:49Z","lastAt":"2008-03-03T07:11:02Z","messageCount":7,"participants":["Jeff King","Johannes Schindelin","Alex Riesen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"70587","messageId":"20080302064449.GA6334@coredump.intra.peff.net","threadId":"12419","inReplyTo":null,"subject":"bug in git-revert","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-02T06:44:49Z","receivedAt":"2008-03-02T06:44:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In 9509af6 (Make git-revert & git-cherry-pick a builtin), you introduced\nthe following code:\n\n   252          } else {\n   253                  struct wt_status s;\n   254\n   255                  if (get_sha1(\"HEAD\", head))\n   256                          die (\"You do not have a valid HEAD\");\n   257                  wt_status_prepare(&s);\n   258                  if (s.commitable || s.workdir_dirty)\n   259                          die (\"Dirty index: cannot %s\", me);\n   260                  discard_cache();\n   261          }\n\n(where the die condition was later changed to allow a dirty workdir). Am\nI crazy, or is it impossible to trigger either of those conditions?\nwt_status_prepare is about setting up the object to do the actual diff.\nAt that point it hasn't actually looked at the index or file contents,\nand so commitable is _always_ 0. And if it did do the right thing, it\nwould have spewed a bunch of stuff to stdout.\n\nI think it should be fixable by something like the patch below, but it\nfeels a little hack-ish. Should there perhaps be an equivalent to\n\"wt_status_print_updated\" that just finds the commitable flag?\n\ndiff --git a/builtin-revert.c b/builtin-revert.c\nindex b6dee6a..64f381c 100644\n--- a/builtin-revert.c\n+++ b/builtin-revert.c\n@@ -279,6 +279,8 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \t\tif (get_sha1(\"HEAD\", head))\n \t\t\tdie (\"You do not have a valid HEAD\");\n \t\twt_status_prepare(&s);\n+\t\ts.fp = NULL;\n+\t\twt_status_print_updated(&s);\n \t\tif (s.commitable)\n \t\t\tdie (\"Dirty index: cannot %s\", me);\n \t\tdiscard_cache();\ndiff --git a/color.c b/color.c\nindex 12a6453..6dba762 100644\n--- a/color.c\n+++ b/color.c\n@@ -160,6 +160,8 @@ static int color_vfprintf(FILE *fp, const char *color, const char *fmt,\n {\n \tint r = 0;\n \n+\tif (!fp)\n+\t\treturn 0;\n \tif (*color)\n \t\tr += fprintf(fp, \"%s\", color);\n \tr += vfprintf(fp, fmt, args);\ndiff --git a/wt-status.c b/wt-status.c\nindex 32d780a..bf0934a 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -169,7 +169,8 @@ static void wt_status_print_filepair(struct wt_status *s,\n \tdefault:\n \t\tdie(\"bug: unhandled diff status %c\", p->status);\n \t}\n-\tfprintf(s->fp, \"\\n\");\n+\tif (s->fp)\n+\t\tfprintf(s->fp, \"\\n\");\n \tstrbuf_release(&onebuf);\n \tstrbuf_release(&twobuf);\n }\n@@ -238,7 +239,7 @@ static void wt_status_print_initial(struct wt_status *s)\n \tstrbuf_release(&buf);\n }\n \n-static void wt_status_print_updated(struct wt_status *s)\n+void wt_status_print_updated(struct wt_status *s)\n {\n \tstruct rev_info rev;\n \tinit_revisions(&rev, NULL);\n@@ -369,8 +370,10 @@ void wt_status_print(struct wt_status *s)\n \tif (s->verbose && !s->is_initial)\n \t\twt_status_print_verbose(s);\n \tif (!s->commitable) {\n-\t\tif (s->amend)\n-\t\t\tfprintf(s->fp, \"# No changes\\n\");\n+\t\tif (s->amend) {\n+\t\t\tif (s->fp)\n+\t\t\t\tfprintf(s->fp, \"# No changes\\n\");\n+\t\t}\n \t\telse if (s->nowarn)\n \t\t\t; /* nothing */\n \t\telse if (s->workdir_dirty)\ndiff --git a/wt-status.h b/wt-status.h\nindex 02afaa6..7d40a57 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -32,5 +32,6 @@ int wt_status_use_color;\n int wt_status_relative_paths;\n void wt_status_prepare(struct wt_status *s);\n void wt_status_print(struct wt_status *s);\n+void wt_status_print_updated(struct wt_status *s);\n \n #endif /* STATUS_H */\n"},{"id":"70588","messageId":"alpine.LSU.1.00.0803020659340.22527@racer.site","threadId":"12419","inReplyTo":"20080302064449.GA6334@coredump.intra.peff.net","subject":"Re: bug in git-revert","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-02T07:00:32Z","receivedAt":"2008-03-02T07:00:32Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 2 Mar 2008, Jeff King wrote:\n\n> In 9509af6 (Make git-revert & git-cherry-pick a builtin), you introduced\n> the following code:\n> \n>    252          } else {\n>    253                  struct wt_status s;\n>    254\n>    255                  if (get_sha1(\"HEAD\", head))\n>    256                          die (\"You do not have a valid HEAD\");\n>    257                  wt_status_prepare(&s);\n>    258                  if (s.commitable || s.workdir_dirty)\n>    259                          die (\"Dirty index: cannot %s\", me);\n>    260                  discard_cache();\n>    261          }\n> \n> (where the die condition was later changed to allow a dirty workdir). Am\n> I crazy, or is it impossible to trigger either of those conditions?\n> wt_status_prepare is about setting up the object to do the actual diff.\n> At that point it hasn't actually looked at the index or file contents,\n> and so commitable is _always_ 0.\n\nWell, then this is my mistake.  I haven't looked closely at what \nwt_status_prepare() does (or did).\n\nCiao,\nDscho\n\n"},{"id":"70589","messageId":"20080302070221.GA10629@coredump.intra.peff.net","threadId":"12419","inReplyTo":"20080302064449.GA6334@coredump.intra.peff.net","subject":"Re: bug in git-revert","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-02T07:02:21Z","receivedAt":"2008-03-02T07:02:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 02, 2008 at 01:44:49AM -0500, Jeff King wrote:\n\n> --- a/builtin-revert.c\n> +++ b/builtin-revert.c\n> @@ -279,6 +279,8 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n>  \t\tif (get_sha1(\"HEAD\", head))\n>  \t\t\tdie (\"You do not have a valid HEAD\");\n>  \t\twt_status_prepare(&s);\n> +\t\ts.fp = NULL;\n> +\t\twt_status_print_updated(&s);\n>  \t\tif (s.commitable)\n>  \t\t\tdie (\"Dirty index: cannot %s\", me);\n>  \t\tdiscard_cache();\n\nActually, this isn't right. It needs to call wt_status_print(&s), since\nit does some magic that wt_status_print_updated() requires to run\ncorrectly.\n\n-Peff\n"},{"id":"70592","messageId":"20080302072252.GA14214@coredump.intra.peff.net","threadId":"12419","inReplyTo":"20080302064449.GA6334@coredump.intra.peff.net","subject":"[PATCH] revert: actually check for a dirty index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-02T07:22:52Z","receivedAt":"2008-03-02T07:22:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The previous code mistakenly used wt_status_prepare to check\nwhether the index had anything commitable in it; however,\nthat function is just an init function, and will never\nreport a dirty index.\n\nThe correct way with wt_status_* would be to call\nwt_status_print with the output pointing to /dev/null or\nsimilar. However, that does extra work by both examining the\nworking tree and spewing status information to nowhere.\n\nInstead, let's just implement the useful subset of\nwt_status_print as an \"is_index_dirty\" function.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI said:\n> I think it should be fixable by something like the patch below, but it\n> feels a little hack-ish. Should there perhaps be an equivalent to\n> \"wt_status_print_updated\" that just finds the commitable flag?\n\nThis is much nicer, and more efficient to boot.\n\n builtin-revert.c              |   34 ++++++++++++++++++++++++++++++----\n t/t3501-revert-cherry-pick.sh |    9 +++++++++\n 2 files changed, 39 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-revert.c b/builtin-revert.c\nindex b6dee6a..15ab969 100644\n--- a/builtin-revert.c\n+++ b/builtin-revert.c\n@@ -9,6 +9,9 @@\n #include \"utf8.h\"\n #include \"parse-options.h\"\n #include \"cache-tree.h\"\n+#include \"diff.h\"\n+#include \"diffcore.h\"\n+#include \"revision.h\"\n \n /*\n  * This implements the builtins revert and cherry-pick.\n@@ -246,6 +249,30 @@ static char *help_msg(const unsigned char *sha1)\n \treturn helpbuf;\n }\n \n+static void index_is_dirty_cb(struct diff_queue_struct *q,\n+\t\tstruct diff_options *options,\n+\t\tvoid *data)\n+{\n+\tint *is_dirty = data;\n+\tif (q->nr > 0)\n+\t\t*is_dirty = 1;\n+}\n+\n+static int index_is_dirty(void)\n+{\n+\tstruct rev_info rev;\n+\tint is_dirty = 0;\n+\n+\tinit_revisions(&rev, NULL);\n+\tsetup_revisions(0, NULL, &rev, \"HEAD\");\n+\trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n+\trev.diffopt.format_callback = index_is_dirty_cb;\n+\trev.diffopt.format_callback_data = &is_dirty;\n+\trun_diff_index(&rev, 1);\n+\n+\treturn is_dirty;\n+}\n+\n static int revert_or_cherry_pick(int argc, const char **argv)\n {\n \tunsigned char head[20];\n@@ -274,12 +301,11 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \t\tif (write_cache_as_tree(head, 0, NULL))\n \t\t\tdie (\"Your index file is unmerged.\");\n \t} else {\n-\t\tstruct wt_status s;\n-\n \t\tif (get_sha1(\"HEAD\", head))\n \t\t\tdie (\"You do not have a valid HEAD\");\n-\t\twt_status_prepare(&s);\n-\t\tif (s.commitable)\n+\t\tif(read_cache() < 0)\n+\t\t\tdie(\"could not read the index\");\n+\t\tif (index_is_dirty())\n \t\t\tdie (\"Dirty index: cannot %s\", me);\n \t\tdiscard_cache();\n \t}\ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex 2dbe04f..6da2128 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -59,4 +59,13 @@ test_expect_success 'revert after renaming branch' '\n \n '\n \n+test_expect_success 'revert forbidden on dirty working tree' '\n+\n+\techo content >extra_file &&\n+\tgit add extra_file &&\n+\ttest_must_fail git revert HEAD 2>errors &&\n+\tgrep \"Dirty index\" errors\n+\n+'\n+\n test_done\n-- \n1.5.4.3.365.gc1491.dirty\n"},{"id":"70629","messageId":"20080302103753.GB2973@steel.home","threadId":"12419","inReplyTo":"20080302072252.GA14214@coredump.intra.peff.net","subject":"Re: [PATCH] revert: actually check for a dirty index","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-03-02T10:37:53Z","receivedAt":"2008-03-02T10:37:53Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Jeff King, Sun, Mar 02, 2008 08:22:52 +0100:\n> +static int index_is_dirty(void)\n> +{\n> +\tstruct rev_info rev;\n> +\tint is_dirty = 0;\n> +\n> +\tinit_revisions(&rev, NULL);\n> +\tsetup_revisions(0, NULL, &rev, \"HEAD\");\n> +\trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n> +\trev.diffopt.format_callback = index_is_dirty_cb;\n> +\trev.diffopt.format_callback_data = &is_dirty;\n> +\trun_diff_index(&rev, 1);\n> +\n> +\treturn is_dirty;\n> +}\n\nWouldn't something like what built-commit does more effective?\nIt does:\n\n\tstruct rev_info rev;\n\tunsigned char sha1[20];\n\tconst char *parent = \"HEAD\";\n\n\tif (!active_nr && read_cache() < 0)\n\t\tdie(\"Cannot read index\");\n\n\tif (amend)\n\t\tparent = \"HEAD^1\";\n\n\tif (get_sha1(parent, sha1))\n\t\tcommitable = !!active_nr;\n\telse {\n\t\tinit_revisions(&rev, \"\");\n\t\trev.abbrev = 0;\n\t\tsetup_revisions(0, NULL, &rev, parent);\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\tcommitable = !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);\n\t}\n\nDiff-index in this case will stop as soon as the first difference\nfound, while just using the output method will enforce finding all the\ndifferences, which is just a waste of time, if all you need is just to\nknow if index is different to HEAD.\n\n"},{"id":"70743","messageId":"20080303063055.GA24866@coredump.intra.peff.net","threadId":"12419","inReplyTo":"20080302103753.GB2973@steel.home","subject":"Re: [PATCH] revert: actually check for a dirty index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-03T06:30:56Z","receivedAt":"2008-03-03T06:30:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 02, 2008 at 11:37:53AM +0100, Alex Riesen wrote:\n\n> Wouldn't something like what built-commit does more effective?\n\nI assumed git-commit used wt_status_print, and it does, but only for\nsome code paths. When possible, it uses the more efficient version you\nmentioned.\n\nSo here is my patch respun with the same method. I considered making a\nglobally available \"is_index_dirty\" function, but I think there are a\nlot of assumptions that make such a function more trouble than the 6\nlines it saves. E.g., \"dirty versus what?\", \"which index?\", \"has the\nindex been loaded already?\"\n\nThanks Alex.\n\n-Peff\n\n-- >8 --\nrevert: actually check for a dirty index\n\nThe previous code mistakenly used wt_status_prepare to check\nwhether the index had anything commitable in it; however,\nthat function is just an init function, and will never\nreport a dirty index.\n\nThe correct way with wt_status_* would be to call\nwt_status_print with the output pointing to /dev/null or\nsimilar. However, that does extra work by both examining the\nworking tree and spewing status information to nowhere.\n\nInstead, let's just implement the useful subset of\nwt_status_print as an \"is_index_dirty\" function.\n---\n builtin-revert.c              |   20 ++++++++++++++++----\n t/t3501-revert-cherry-pick.sh |    9 +++++++++\n 2 files changed, 25 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-revert.c b/builtin-revert.c\nindex b6dee6a..1989f96 100644\n--- a/builtin-revert.c\n+++ b/builtin-revert.c\n@@ -9,6 +9,8 @@\n #include \"utf8.h\"\n #include \"parse-options.h\"\n #include \"cache-tree.h\"\n+#include \"diff.h\"\n+#include \"revision.h\"\n \n /*\n  * This implements the builtins revert and cherry-pick.\n@@ -246,6 +248,17 @@ static char *help_msg(const unsigned char *sha1)\n \treturn helpbuf;\n }\n \n+static int index_is_dirty(void)\n+{\n+\tstruct rev_info rev;\n+\tinit_revisions(&rev, NULL);\n+\tsetup_revisions(0, NULL, &rev, \"HEAD\");\n+\tDIFF_OPT_SET(&rev.diffopt, QUIET);\n+\tDIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);\n+\trun_diff_index(&rev, 1);\n+\treturn !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);\n+}\n+\n static int revert_or_cherry_pick(int argc, const char **argv)\n {\n \tunsigned char head[20];\n@@ -274,12 +287,11 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \t\tif (write_cache_as_tree(head, 0, NULL))\n \t\t\tdie (\"Your index file is unmerged.\");\n \t} else {\n-\t\tstruct wt_status s;\n-\n \t\tif (get_sha1(\"HEAD\", head))\n \t\t\tdie (\"You do not have a valid HEAD\");\n-\t\twt_status_prepare(&s);\n-\t\tif (s.commitable)\n+\t\tif(read_cache() < 0)\n+\t\t\tdie(\"could not read the index\");\n+\t\tif (index_is_dirty())\n \t\t\tdie (\"Dirty index: cannot %s\", me);\n \t\tdiscard_cache();\n \t}\ndiff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh\nindex 2dbe04f..6da2128 100755\n--- a/t/t3501-revert-cherry-pick.sh\n+++ b/t/t3501-revert-cherry-pick.sh\n@@ -59,4 +59,13 @@ test_expect_success 'revert after renaming branch' '\n \n '\n \n+test_expect_success 'revert forbidden on dirty working tree' '\n+\n+\techo content >extra_file &&\n+\tgit add extra_file &&\n+\ttest_must_fail git revert HEAD 2>errors &&\n+\tgrep \"Dirty index\" errors\n+\n+'\n+\n test_done\n-- \n1.5.4.3.366.gb96b1.dirty\n\n"},{"id":"70753","messageId":"7vablgjn95.fsf@gitster.siamese.dyndns.org","threadId":"12419","inReplyTo":"20080303063055.GA24866@coredump.intra.peff.net","subject":"Re: [PATCH] revert: actually check for a dirty index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-03T07:11:02Z","receivedAt":"2008-03-03T07:11:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Looks good.  I think we want to have the fix in 1.5.4 maintenance series\nas well, so I'll backport it to 'maint' and merge it up to 'master'.\n"}]}