{"thread":{"id":"1633","subject":"Fix git-rev-parse breakage","startedAt":"2005-08-24T02:17:37Z","lastAt":"2005-08-24T21:40:24Z","messageCount":6,"participants":["Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"7694","messageId":"Pine.LNX.4.58.0508231908170.3317@g5.osdl.org","threadId":"1633","inReplyTo":null,"subject":"Fix git-rev-parse breakage","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-08-24T02:17:37Z","receivedAt":"2005-08-24T02:17:37Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nThe --flags cleanup caused problems: we used to depend on the fact that \n\"revs_only\" magically suppressed flags, adn that assumption was broken by \nthe recent fixes.\n\nIt wasn't a good assumption in the first place, so instead of \nre-introducing it, let's just get rid of it.\n\nThis makes \"--revs-only\" imply \"--no-flags\".\n\n[ Side note: we might want to get rid of these confusing two-way flags, \n  where some flags say \"only print xxx\", and others say \"don't print yyy\". \n  We'd be better off with just three flags that say \"print zzz\", where zzz\n  is one of \"flags, revs, norevs\" ]\n\nSigned-off-by: Linus Torvalds <torvalds@osdl.org>\n---\ndiff --git a/rev-parse.c b/rev-parse.c\n--- a/rev-parse.c\n+++ b/rev-parse.c\n@@ -160,6 +160,7 @@ int main(int argc, char **argv)\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--revs-only\")) {\n \t\t\t\trevs_only = 1;\n+\t\t\t\tno_flags = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--no-revs\")) {\n"},{"id":"7697","messageId":"7vy86skoib.fsf@assigned-by-dhcp.cox.net","threadId":"1633","inReplyTo":"Pine.LNX.4.58.0508231908170.3317@g5.osdl.org","subject":"Re: Fix git-rev-parse breakage","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-08-24T03:22:20Z","receivedAt":"2005-08-24T03:22:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> This makes \"--revs-only\" imply \"--no-flags\".\n>\n> [ Side note: we might want to get rid of these confusing two-way flags, \n>   where some flags say \"only print xxx\", and others say \"don't print yyy\". \n>   We'd be better off with just three flags that say \"print zzz\", where zzz\n>   is one of \"flags, revs, norevs\" ]\n\nI suspect that would not fly too well.\n\nBeing able to say \"give me all flags meant for rev-list\", \"give\nme all what are meant for rev-list\", and \"give me all non-flags\nthat are meant for rev-list\" are very handy, so I'd rather want\nto see --revs-only to mean \"meant for rev-list\", --no-revs to\nmean \"not meant for rev-list\", --flags to mean \"only ones that\nstart with a '-'\", and --no-flags to mean \"only ones that do not\nstart with a '-'\".  And that would give me (rev/no-rev/lack\nthereof) x (flag/no-flag/lack thereof) = 9 combinations.\n"},{"id":"7716","messageId":"7v4q9fdv5w.fsf@assigned-by-dhcp.cox.net","threadId":"1633","inReplyTo":"Pine.LNX.4.58.0508231908170.3317@g5.osdl.org","subject":"Re: Fix git-rev-parse breakage","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-08-24T18:52:43Z","receivedAt":"2005-08-24T18:52:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> The --flags cleanup caused problems: we used to depend on the fact that \n> \"revs_only\" magically suppressed flags, adn that assumption was broken by \n> the recent fixes.\n>\n> It wasn't a good assumption in the first place, so instead of \n> re-introducing it, let's just get rid of it.\n>\n> This makes \"--revs-only\" imply \"--no-flags\".\n\nI was taking a look at this once again after noticing that I\nhaven't taking any action on it.  But now I am a bit confused\nreading the above.  The first two paragraphs tells me\n\"--revs-only implied --no-flags and we used to depend on it, but\nthat is not a right thing so get rid of that assumption\" (which\nI agree is a good change\", and the last sentense says\nopposite...\n\nAnd the code makes --revs-only imply --no-flags.\n\nHere is my thinking, requesting for a sanity check.\n\n* git-whatchanged wants to use it to tell rev-list arguments\n  from rev-tree arguments.  You _do_ want to pick --max-count=10\n  or --merge-order in this case, and --revs-only implying\n  --no-flags would make this impossible.\n\n* git-log-script uses it once to make sure it has one commit to\n  start at, and lacks --no-flags by mistake.\n\n* git-bisect uses it to validate the parameter is a valid ref,\n  but does not use --verify.\n\nThis trivial patch fixes the latter two.\n\n---\ndiff --git a/git-log-script b/git-log-script\n--- a/git-log-script\n+++ b/git-log-script\n@@ -1,4 +1,4 @@\n #!/bin/sh\n-revs=$(git-rev-parse --revs-only --default HEAD \"$@\") || exit\n+revs=$(git-rev-parse --revs-only --no-flags --default HEAD \"$@\") || exit\n [ \"$revs\" ] || die \"No HEAD ref\"\n git-rev-list --pretty $(git-rev-parse --default HEAD \"$@\") | LESS=-S ${PAGER:-less}\n\ndiff --git a/git-bisect-script b/git-bisect-script\n--- a/git-bisect-script\n+++ b/git-bisect-script\n@@ -67,7 +67,7 @@ bisect_good() {\n \tbisect_autostart\n         case \"$#\" in\n \t0)    revs=$(git-rev-parse --verify HEAD) || exit ;;\n-\t*)    revs=$(git-rev-parse --revs-only \"$@\") || exit ;;\n+\t*)    revs=$(git-rev-parse --revs-only --verify \"$@\") || exit ;;\n \tesac\n \tfor rev in $revs\n \tdo\n"},{"id":"7719","messageId":"Pine.LNX.4.58.0508241200500.3317@g5.osdl.org","threadId":"1633","inReplyTo":"7v4q9fdv5w.fsf@assigned-by-dhcp.cox.net","subject":"Re: Fix git-rev-parse breakage","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-08-24T19:03:45Z","receivedAt":"2005-08-24T19:03:45Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 24 Aug 2005, Junio C Hamano wrote:\n> that is not a right thing so get rid of that assumption\" (which\n> I agree is a good change\", and the last sentense says\n> opposite...\n\nWell, the patch makes it an _explicit_ assumption, instead of a very \nsubtly hidden one from the code-flow. It was the non-obvious hidden \nassumption that caused the bug.\n\n> Here is my thinking, requesting for a sanity check.\n> \n> * git-whatchanged wants to use it to tell rev-list arguments\n>   from rev-tree arguments.  You _do_ want to pick --max-count=10\n>   or --merge-order in this case, and --revs-only implying\n>   --no-flags would make this impossible.\n\nFair enough. However, there are two kinds of flags: the \"revision flags\", \nand the \"-p\" kind of flags.\n\nAnd the problem was that \"git-whatchanged -p\" didn't work any more,\nbecause we passed \"-p\" along to \"git-rev-list\". Gaah.\n\n\t\t\tLinus\n"},{"id":"7727","messageId":"7vll2rc93o.fsf_-_@assigned-by-dhcp.cox.net","threadId":"1633","inReplyTo":"Pine.LNX.4.58.0508241200500.3317@g5.osdl.org","subject":"[PATCH] Audit rev-parse users again.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-08-24T21:34:35Z","receivedAt":"2005-08-24T21:34:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Some callers to rev-parse were using the output selection flags\ninconsistently.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n\n---\n\n git-bisect-script       |    4 ++--\n git-branch-script       |    2 +-\n git-log-script          |    2 +-\n git-request-pull-script |    4 ++--\n git-revert-script       |    2 +-\n 5 files changed, 7 insertions(+), 7 deletions(-)\n\nff84d327dfb8a9aa0634b0aaaca1c018cdc5117a\ndiff --git a/git-bisect-script b/git-bisect-script\n--- a/git-bisect-script\n+++ b/git-bisect-script\n@@ -58,7 +58,7 @@ bisect_start() {\n bisect_bad() {\n \tbisect_autostart\n         case \"$#\" in 0 | 1) ;; *) usage ;; esac\n-\trev=$(git-rev-parse --revs-only --verify --default HEAD \"$@\") || exit\n+\trev=$(git-rev-parse --verify --default HEAD \"$@\") || exit\n \techo \"$rev\" > \"$GIT_DIR/refs/bisect/bad\"\n \tbisect_auto_next\n }\n@@ -67,7 +67,7 @@ bisect_good() {\n \tbisect_autostart\n         case \"$#\" in\n \t0)    revs=$(git-rev-parse --verify HEAD) || exit ;;\n-\t*)    revs=$(git-rev-parse --revs-only \"$@\") || exit ;;\n+\t*)    revs=$(git-rev-parse --revs-only --no-flags \"$@\") || exit ;;\n \tesac\n \tfor rev in $revs\n \tdo\ndiff --git a/git-branch-script b/git-branch-script\n--- a/git-branch-script\n+++ b/git-branch-script\n@@ -25,7 +25,7 @@ case \"$#\" in\n \thead=\"$2^0\" ;;\n esac\n branchname=\"$1\"\n-rev=$(git-rev-parse --revs-only --verify \"$head\") || exit\n+rev=$(git-rev-parse --verify \"$head\") || exit\n \n [ -e \"$GIT_DIR/refs/heads/$branchname\" ] && die \"$branchname already exists\"\n \ndiff --git a/git-log-script b/git-log-script\n--- a/git-log-script\n+++ b/git-log-script\n@@ -1,4 +1,4 @@\n #!/bin/sh\n-revs=$(git-rev-parse --revs-only --default HEAD \"$@\") || exit\n+revs=$(git-rev-parse --revs-only --no-flags --default HEAD \"$@\") || exit\n [ \"$revs\" ] || die \"No HEAD ref\"\n git-rev-list --pretty $(git-rev-parse --default HEAD \"$@\") | LESS=-S ${PAGER:-less}\ndiff --git a/git-request-pull-script b/git-request-pull-script\n--- a/git-request-pull-script\n+++ b/git-request-pull-script\n@@ -19,8 +19,8 @@ head=${3-HEAD}\n [ \"$revision\" ] || usage\n [ \"$url\" ] || usage\n \n-baserev=`git-rev-parse --verify $revision^0` &&\n-headrev=`git-rev-parse --verify $head^0` || exit\n+baserev=`git-rev-parse --verify \"$revision\"^0` &&\n+headrev=`git-rev-parse --verify \"$head\"^0` || exit\n \n echo \"The following changes since commit $baserev:\"\n git log --max-count=1 --pretty=short \"$baserev\" |\ndiff --git a/git-revert-script b/git-revert-script\n--- a/git-revert-script\n+++ b/git-revert-script\n@@ -10,7 +10,7 @@ case \"$status\" in\n \tdie \"Your working tree is dirty; cannot revert a previous patch.\" ;;\n esac\n \n-rev=$(git-rev-parse --no-flags --verify --revs-only \"$@\") &&\n+rev=$(git-rev-parse --verify \"$@\") &&\n commit=$(git-rev-parse --verify \"$rev^0\") || exit\n if git-diff-tree -R -M -p $commit | git-apply --index &&\n    msg=$(git-rev-list --pretty=oneline --max-count=1 $commit)\n"},{"id":"7728","messageId":"7vek8jc8tz.fsf@assigned-by-dhcp.cox.net","threadId":"1633","inReplyTo":"Pine.LNX.4.58.0508241200500.3317@g5.osdl.org","subject":"[PATCH] Rationalize output selection in rev-parse.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-08-24T21:40:24Z","receivedAt":"2005-08-24T21:40:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Earlier rounds broke 'whatchanged -p'.  In attempting to fix this,\nmake two axis of output selection in rev-parse orthogonal:\n\n  --revs-only\ttells it not to output things that are not revisions nor\n\t\tflags that rev-list would take.\n  --no-revs\ttells it not to output things that are revisions or\n\t\tflags that rev-list would take.\n  --flags\ttells it not to output parameters that do not start with\n\t\ta '-'.\n  --no-flags\ttells it not to output parameters that starts with a '-'.\n\nSo for example 'rev-parse --no-revs -p arch/i386' would yield '-p arch/i386',\nwhile 'rev-parse --no-revs --flags -p arch/i386' would give just '-p'.\n\nAlso the meaning of --verify has been made stronger.  It now rejects\nanything but a single valid rev argument.  Earlier it passed some flags\nthrough without complaining.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n\n---\n\n  Linus Torvalds <torvalds@osdl.org> writes:\n\n  > Fair enough. However, there are two kinds of flags: the\n  > \"revision flags\", and the \"-p\" kind of flags.\n  >\n  > And the problem was that \"git-whatchanged -p\" didn't work any more,\n  > because we passed \"-p\" along to \"git-rev-list\". Gaah.\n\n  Thanks for noticing.\n\n rev-parse.c |  121 ++++++++++++++++++++++++++---------------------------------\n 1 files changed, 54 insertions(+), 67 deletions(-)\n\n4866ccf0f434db118c4dcdeeab840eb4844d50a4\ndiff --git a/rev-parse.c b/rev-parse.c\n--- a/rev-parse.c\n+++ b/rev-parse.c\n@@ -7,20 +7,21 @@\n #include \"commit.h\"\n #include \"refs.h\"\n \n+#define DO_REVS\t\t1\n+#define DO_NOREV\t2\n+#define DO_FLAGS\t4\n+#define DO_NONFLAGS\t8\n+static int filter = ~0;\n+\n static char *def = NULL;\n-static int no_revs = 0;\n-static int single_rev = 0;\n-static int revs_only = 0;\n-static int do_rev_argument = 1;\n-static int output_revs = 0;\n-static int flags_only = 0;\n-static int no_flags = 0;\n-static int output_sq = 0;\n-static int symbolic = 0;\n \n #define NORMAL 0\n #define REVERSED 1\n static int show_type = NORMAL;\n+static int symbolic = 0;\n+static int output_sq = 0;\n+\n+static int revs_count = 0;\n \n /*\n  * Some arguments are relevant \"revision\" arguments,\n@@ -30,13 +31,19 @@ static int show_type = NORMAL;\n static int is_rev_argument(const char *arg)\n {\n \tstatic const char *rev_args[] = {\n-\t\t\"--max-count=\",\n+\t\t\"--bisect\",\n+\t\t\"--header\",\n \t\t\"--max-age=\",\n-\t\t\"--min-age=\",\n+\t\t\"--max-count=\",\n \t\t\"--merge-order\",\n-\t\t\"--topo-order\",\n-\t\t\"--bisect\",\n+\t\t\"--min-age=\",\n \t\t\"--no-merges\",\n+\t\t\"--objects\",\n+\t\t\"--parents\",\n+\t\t\"--pretty\",\n+\t\t\"--show-breaks\",\n+\t\t\"--topo-order\",\n+\t\t\"--unpacked\",\n \t\tNULL\n \t};\n \tconst char **p = rev_args;\n@@ -47,11 +54,13 @@ static int is_rev_argument(const char *a\n \t\tif (!str)\n \t\t\treturn 0;\n \t\tlen = strlen(str);\n-\t\tif (!strncmp(arg, str, len))\n+\t\tif (!strcmp(arg, str) ||\n+\t\t    (str[len-1] == '=' && !strncmp(arg, str, len)))\n \t\t\treturn 1;\n \t}\n }\n \n+/* Output argument as a string, either SQ or normal */\n static void show(const char *arg)\n {\n \tif (output_sq) {\n@@ -70,11 +79,13 @@ static void show(const char *arg)\n \t\tputs(arg);\n }\n \n+/* Output a revision, only if filter allows it */\n static void show_rev(int type, const unsigned char *sha1, const char *name)\n {\n-\tif (no_revs)\n+\tif (!(filter & DO_REVS))\n \t\treturn;\n-\toutput_revs++;\n+\tdef = NULL;\n+\trevs_count++;\n \n \tif (type != show_type)\n \t\tputchar('^');\n@@ -84,29 +95,12 @@ static void show_rev(int type, const uns\n \t\tshow(sha1_to_hex(sha1));\n }\n \n-static void show_rev_arg(char *rev)\n+/* Output a flag, only if filter allows it. */\n+static void show_flag(char *arg)\n {\n-\tif (no_revs)\n+\tif (!(filter & DO_FLAGS))\n \t\treturn;\n-\tshow(rev);\n-}\n-\n-static void show_norev(char *norev)\n-{\n-\tif (flags_only)\n-\t\treturn;\n-\tif (revs_only)\n-\t\treturn;\n-\tshow(norev);\n-}\n-\n-static void show_arg(char *arg)\n-{\n-\tif (no_flags)\n-\t\treturn;\n-\tif (do_rev_argument && is_rev_argument(arg))\n-\t\tshow_rev_arg(arg);\n-\telse\n+\tif (filter & (is_rev_argument(arg) ? DO_REVS : DO_NOREV))\n \t\tshow(arg);\n }\n \n@@ -122,7 +116,6 @@ static void show_default(void)\n \t\t\tshow_rev(NORMAL, sha1, s);\n \t\t\treturn;\n \t\t}\n-\t\tshow_norev(s);\n \t}\n }\n \n@@ -134,7 +127,7 @@ static int show_reference(const char *re\n \n int main(int argc, char **argv)\n {\n-\tint i, as_is = 0;\n+\tint i, as_is = 0, verify = 0;\n \tunsigned char sha1[20];\n \tconst char *prefix = setup_git_directory();\n \t\n@@ -143,15 +136,13 @@ int main(int argc, char **argv)\n \t\tchar *dotdot;\n \t\n \t\tif (as_is) {\n-\t\t\tshow_norev(arg);\n+\t\t\tshow(arg);\n \t\t\tcontinue;\n \t\t}\n \t\tif (*arg == '-') {\n \t\t\tif (!strcmp(arg, \"--\")) {\n-\t\t\t\tshow_default();\n-\t\t\t\tif (revs_only || flags_only)\n-\t\t\t\t\tbreak;\n \t\t\t\tas_is = 1;\n+\t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--default\")) {\n \t\t\t\tdef = argv[i+1];\n@@ -159,25 +150,24 @@ int main(int argc, char **argv)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--revs-only\")) {\n-\t\t\t\trevs_only = 1;\n+\t\t\t\tfilter &= ~DO_NOREV;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--no-revs\")) {\n-\t\t\t\tno_revs = 1;\n+\t\t\t\tfilter &= ~DO_REVS;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--flags\")) {\n-\t\t\t\tflags_only = 1;\n+\t\t\t\tfilter &= ~DO_NONFLAGS;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--no-flags\")) {\n-\t\t\t\tno_flags = 1;\n+\t\t\t\tfilter &= ~DO_FLAGS;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--verify\")) {\n-\t\t\t\trevs_only = 1;\n-\t\t\t\tdo_rev_argument = 0;\n-\t\t\t\tsingle_rev = 1;\n+\t\t\t\tfilter &= ~(DO_FLAGS|DO_NOREV);\n+\t\t\t\tverify = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--sq\")) {\n@@ -197,12 +187,17 @@ int main(int argc, char **argv)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--show-prefix\")) {\n-\t\t\t\tputs(prefix);\n+\t\t\t\tif (prefix)\n+\t\t\t\t\tputs(prefix);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tshow_arg(arg);\n+\t\t\tif (verify)\n+\t\t\t\tdie(\"Needed a single revision\");\n+\t\t\tshow_flag(arg);\n \t\t\tcontinue;\n \t\t}\n+\n+\t\t/* Not a flag argument */\n \t\tdotdot = strstr(arg, \"..\");\n \t\tif (dotdot) {\n \t\t\tunsigned char end[20];\n@@ -212,9 +207,6 @@ int main(int argc, char **argv)\n \t\t\t\tif (!*n)\n \t\t\t\t\tn = \"HEAD\";\n \t\t\t\tif (!get_sha1(n, end)) {\n-\t\t\t\t\tif (no_revs)\n-\t\t\t\t\t\tcontinue;\n-\t\t\t\t\tdef = NULL;\n \t\t\t\t\tshow_rev(NORMAL, end, n);\n \t\t\t\t\tshow_rev(REVERSED, sha1, arg);\n \t\t\t\t\tcontinue;\n@@ -223,26 +215,21 @@ int main(int argc, char **argv)\n \t\t\t*dotdot = '.';\n \t\t}\n \t\tif (!get_sha1(arg, sha1)) {\n-\t\t\tif (no_revs)\n-\t\t\t\tcontinue;\n-\t\t\tdef = NULL;\n \t\t\tshow_rev(NORMAL, sha1, arg);\n \t\t\tcontinue;\n \t\t}\n \t\tif (*arg == '^' && !get_sha1(arg+1, sha1)) {\n-\t\t\tif (no_revs)\n-\t\t\t\tcontinue;\n-\t\t\tdef = NULL;\n \t\t\tshow_rev(REVERSED, sha1, arg+1);\n \t\t\tcontinue;\n \t\t}\n-\t\tshow_default();\n-\t\tshow_norev(arg);\n+\t\tif (verify)\n+\t\t\tdie(\"Needed a single revision\");\n+\t\tif ((filter & (DO_NONFLAGS|DO_NOREV)) ==\n+\t\t    (DO_NONFLAGS|DO_NOREV))\n+\t\t\tshow(arg);\n \t}\n \tshow_default();\n-\tif (single_rev && output_revs != 1) {\n-\t\tfprintf(stderr, \"Needed a single revision\\n\");\n-\t\texit(1);\n-\t}\n+\tif (verify && revs_count != 1)\n+\t\tdie(\"Needed a single revision\");\n \treturn 0;\n }\n"}]}