Fix git-rev-parse breakage

Fix git-rev-parse breakage

[Linus Torvalds]

The --flags cleanup caused problems: we used to depend on the fact that 
"revs_only" magically suppressed flags, adn that assumption was broken by 
the recent fixes.

It wasn't a good assumption in the first place, so instead of 
re-introducing it, let's just get rid of it.

This makes "--revs-only" imply "--no-flags".

[ Side note: we might want to get rid of these confusing two-way flags, 
  where some flags say "only print xxx", and others say "don't print yyy". 
  We'd be better off with just three flags that say "print zzz", where zzz
  is one of "flags, revs, norevs" ]

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---
diff --git a/rev-parse.c b/rev-parse.c
--- a/rev-parse.c
+++ b/rev-parse.c
@@ -160,6 +160,7 @@ int main(int argc, char **argv)
 			}
 			if (!strcmp(arg, "--revs-only")) {
 				revs_only = 1;
+				no_flags = 1;
 				continue;
 			}
 			if (!strcmp(arg, "--no-revs")) {

Re: Fix git-rev-parse breakage

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> This makes "--revs-only" imply "--no-flags".
>
> [ Side note: we might want to get rid of these confusing two-way flags, 
>   where some flags say "only print xxx", and others say "don't print yyy". 
>   We'd be better off with just three flags that say "print zzz", where zzz
>   is one of "flags, revs, norevs" ]

I suspect that would not fly too well.

Being able to say "give me all flags meant for rev-list", "give
me all what are meant for rev-list", and "give me all non-flags
that are meant for rev-list" are very handy, so I'd rather want
to see --revs-only to mean "meant for rev-list", --no-revs to
mean "not meant for rev-list", --flags to mean "only ones that
start with a '-'", and --no-flags to mean "only ones that do not
start with a '-'".  And that would give me (rev/no-rev/lack
thereof) x (flag/no-flag/lack thereof) = 9 combinations.

Re: Fix git-rev-parse breakage

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> The --flags cleanup caused problems: we used to depend on the fact that 
> "revs_only" magically suppressed flags, adn that assumption was broken by 
> the recent fixes.
>
> It wasn't a good assumption in the first place, so instead of 
> re-introducing it, let's just get rid of it.
>
> This makes "--revs-only" imply "--no-flags".

I was taking a look at this once again after noticing that I
haven't taking any action on it.  But now I am a bit confused
reading the above.  The first two paragraphs tells me
"--revs-only implied --no-flags and we used to depend on it, but
that is not a right thing so get rid of that assumption" (which
I agree is a good change", and the last sentense says
opposite...

And the code makes --revs-only imply --no-flags.

Here is my thinking, requesting for a sanity check.

* git-whatchanged wants to use it to tell rev-list arguments
  from rev-tree arguments.  You _do_ want to pick --max-count=10
  or --merge-order in this case, and --revs-only implying
  --no-flags would make this impossible.

* git-log-script uses it once to make sure it has one commit to
  start at, and lacks --no-flags by mistake.

* git-bisect uses it to validate the parameter is a valid ref,
  but does not use --verify.

This trivial patch fixes the latter two.

---
diff --git a/git-log-script b/git-log-script
--- a/git-log-script
+++ b/git-log-script
@@ -1,4 +1,4 @@
 #!/bin/sh
-revs=$(git-rev-parse --revs-only --default HEAD "$@") || exit
+revs=$(git-rev-parse --revs-only --no-flags --default HEAD "$@") || exit
 [ "$revs" ] || die "No HEAD ref"
 git-rev-list --pretty $(git-rev-parse --default HEAD "$@") | LESS=-S ${PAGER:-less}

diff --git a/git-bisect-script b/git-bisect-script
--- a/git-bisect-script
+++ b/git-bisect-script
@@ -67,7 +67,7 @@ bisect_good() {
 	bisect_autostart
         case "$#" in
 	0)    revs=$(git-rev-parse --verify HEAD) || exit ;;
-	*)    revs=$(git-rev-parse --revs-only "$@") || exit ;;
+	*)    revs=$(git-rev-parse --revs-only --verify "$@") || exit ;;
 	esac
 	for rev in $revs
 	do

Re: Fix git-rev-parse breakage

[Linus Torvalds]

On Wed, 24 Aug 2005, Junio C Hamano wrote:
> that is not a right thing so get rid of that assumption" (which
> I agree is a good change", and the last sentense says
> opposite...

Well, the patch makes it an _explicit_ assumption, instead of a very 
subtly hidden one from the code-flow. It was the non-obvious hidden 
assumption that caused the bug.

> Here is my thinking, requesting for a sanity check.
> 
> * git-whatchanged wants to use it to tell rev-list arguments
>   from rev-tree arguments.  You _do_ want to pick --max-count=10
>   or --merge-order in this case, and --revs-only implying
>   --no-flags would make this impossible.

Fair enough. However, there are two kinds of flags: the "revision flags", 
and the "-p" kind of flags.

And the problem was that "git-whatchanged -p" didn't work any more,
because we passed "-p" along to "git-rev-list". Gaah.

			Linus

[PATCH] Audit rev-parse users again.

[Junio C Hamano]

Some callers to rev-parse were using the output selection flags
inconsistently.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 git-bisect-script       |    4 ++--
 git-branch-script       |    2 +-
 git-log-script          |    2 +-
 git-request-pull-script |    4 ++--
 git-revert-script       |    2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

ff84d327dfb8a9aa0634b0aaaca1c018cdc5117a
diff --git a/git-bisect-script b/git-bisect-script
--- a/git-bisect-script
+++ b/git-bisect-script
@@ -58,7 +58,7 @@ bisect_start() {
 bisect_bad() {
 	bisect_autostart
         case "$#" in 0 | 1) ;; *) usage ;; esac
-	rev=$(git-rev-parse --revs-only --verify --default HEAD "$@") || exit
+	rev=$(git-rev-parse --verify --default HEAD "$@") || exit
 	echo "$rev" > "$GIT_DIR/refs/bisect/bad"
 	bisect_auto_next
 }
@@ -67,7 +67,7 @@ bisect_good() {
 	bisect_autostart
         case "$#" in
 	0)    revs=$(git-rev-parse --verify HEAD) || exit ;;
-	*)    revs=$(git-rev-parse --revs-only "$@") || exit ;;
+	*)    revs=$(git-rev-parse --revs-only --no-flags "$@") || exit ;;
 	esac
 	for rev in $revs
 	do
diff --git a/git-branch-script b/git-branch-script
--- a/git-branch-script
+++ b/git-branch-script
@@ -25,7 +25,7 @@ case "$#" in
 	head="$2^0" ;;
 esac
 branchname="$1"
-rev=$(git-rev-parse --revs-only --verify "$head") || exit
+rev=$(git-rev-parse --verify "$head") || exit
 
 [ -e "$GIT_DIR/refs/heads/$branchname" ] && die "$branchname already exists"
 
diff --git a/git-log-script b/git-log-script
--- a/git-log-script
+++ b/git-log-script
@@ -1,4 +1,4 @@
 #!/bin/sh
-revs=$(git-rev-parse --revs-only --default HEAD "$@") || exit
+revs=$(git-rev-parse --revs-only --no-flags --default HEAD "$@") || exit
 [ "$revs" ] || die "No HEAD ref"
 git-rev-list --pretty $(git-rev-parse --default HEAD "$@") | LESS=-S ${PAGER:-less}
diff --git a/git-request-pull-script b/git-request-pull-script
--- a/git-request-pull-script
+++ b/git-request-pull-script
@@ -19,8 +19,8 @@ head=${3-HEAD}
 [ "$revision" ] || usage
 [ "$url" ] || usage
 
-baserev=`git-rev-parse --verify $revision^0` &&
-headrev=`git-rev-parse --verify $head^0` || exit
+baserev=`git-rev-parse --verify "$revision"^0` &&
+headrev=`git-rev-parse --verify "$head"^0` || exit
 
 echo "The following changes since commit $baserev:"
 git log --max-count=1 --pretty=short "$baserev" |
diff --git a/git-revert-script b/git-revert-script
--- a/git-revert-script
+++ b/git-revert-script
@@ -10,7 +10,7 @@ case "$status" in
 	die "Your working tree is dirty; cannot revert a previous patch." ;;
 esac
 
-rev=$(git-rev-parse --no-flags --verify --revs-only "$@") &&
+rev=$(git-rev-parse --verify "$@") &&
 commit=$(git-rev-parse --verify "$rev^0") || exit
 if git-diff-tree -R -M -p $commit | git-apply --index &&
    msg=$(git-rev-list --pretty=oneline --max-count=1 $commit)

[PATCH] Rationalize output selection in rev-parse.

[Junio C Hamano]

Earlier rounds broke 'whatchanged -p'.  In attempting to fix this,
make two axis of output selection in rev-parse orthogonal:

  --revs-only	tells it not to output things that are not revisions nor
		flags that rev-list would take.
  --no-revs	tells it not to output things that are revisions or
		flags that rev-list would take.
  --flags	tells it not to output parameters that do not start with
		a '-'.
  --no-flags	tells it not to output parameters that starts with a '-'.

So for example 'rev-parse --no-revs -p arch/i386' would yield '-p arch/i386',
while 'rev-parse --no-revs --flags -p arch/i386' would give just '-p'.

Also the meaning of --verify has been made stronger.  It now rejects
anything but a single valid rev argument.  Earlier it passed some flags
through without complaining.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

  Linus Torvalds <torvalds@osdl.org> writes:

  > Fair enough. However, there are two kinds of flags: the
  > "revision flags", and the "-p" kind of flags.
  >
  > And the problem was that "git-whatchanged -p" didn't work any more,
  > because we passed "-p" along to "git-rev-list". Gaah.

  Thanks for noticing.

 rev-parse.c |  121 ++++++++++++++++++++++++++---------------------------------
 1 files changed, 54 insertions(+), 67 deletions(-)

4866ccf0f434db118c4dcdeeab840eb4844d50a4
diff --git a/rev-parse.c b/rev-parse.c
--- a/rev-parse.c
+++ b/rev-parse.c
@@ -7,20 +7,21 @@
 #include "commit.h"
 #include "refs.h"
 
+#define DO_REVS		1
+#define DO_NOREV	2
+#define DO_FLAGS	4
+#define DO_NONFLAGS	8
+static int filter = ~0;
+
 static char *def = NULL;
-static int no_revs = 0;
-static int single_rev = 0;
-static int revs_only = 0;
-static int do_rev_argument = 1;
-static int output_revs = 0;
-static int flags_only = 0;
-static int no_flags = 0;
-static int output_sq = 0;
-static int symbolic = 0;
 
 #define NORMAL 0
 #define REVERSED 1
 static int show_type = NORMAL;
+static int symbolic = 0;
+static int output_sq = 0;
+
+static int revs_count = 0;
 
 /*
  * Some arguments are relevant "revision" arguments,
@@ -30,13 +31,19 @@ static int show_type = NORMAL;
 static int is_rev_argument(const char *arg)
 {
 	static const char *rev_args[] = {
-		"--max-count=",
+		"--bisect",
+		"--header",
 		"--max-age=",
-		"--min-age=",
+		"--max-count=",
 		"--merge-order",
-		"--topo-order",
-		"--bisect",
+		"--min-age=",
 		"--no-merges",
+		"--objects",
+		"--parents",
+		"--pretty",
+		"--show-breaks",
+		"--topo-order",
+		"--unpacked",
 		NULL
 	};
 	const char **p = rev_args;
@@ -47,11 +54,13 @@ static int is_rev_argument(const char *a
 		if (!str)
 			return 0;
 		len = strlen(str);
-		if (!strncmp(arg, str, len))
+		if (!strcmp(arg, str) ||
+		    (str[len-1] == '=' && !strncmp(arg, str, len)))
 			return 1;
 	}
 }
 
+/* Output argument as a string, either SQ or normal */
 static void show(const char *arg)
 {
 	if (output_sq) {
@@ -70,11 +79,13 @@ static void show(const char *arg)
 		puts(arg);
 }
 
+/* Output a revision, only if filter allows it */
 static void show_rev(int type, const unsigned char *sha1, const char *name)
 {
-	if (no_revs)
+	if (!(filter & DO_REVS))
 		return;
-	output_revs++;
+	def = NULL;
+	revs_count++;
 
 	if (type != show_type)
 		putchar('^');
@@ -84,29 +95,12 @@ static void show_rev(int type, const uns
 		show(sha1_to_hex(sha1));
 }
 
-static void show_rev_arg(char *rev)
+/* Output a flag, only if filter allows it. */
+static void show_flag(char *arg)
 {
-	if (no_revs)
+	if (!(filter & DO_FLAGS))
 		return;
-	show(rev);
-}
-
-static void show_norev(char *norev)
-{
-	if (flags_only)
-		return;
-	if (revs_only)
-		return;
-	show(norev);
-}
-
-static void show_arg(char *arg)
-{
-	if (no_flags)
-		return;
-	if (do_rev_argument && is_rev_argument(arg))
-		show_rev_arg(arg);
-	else
+	if (filter & (is_rev_argument(arg) ? DO_REVS : DO_NOREV))
 		show(arg);
 }
 
@@ -122,7 +116,6 @@ static void show_default(void)
 			show_rev(NORMAL, sha1, s);
 			return;
 		}
-		show_norev(s);
 	}
 }
 
@@ -134,7 +127,7 @@ static int show_reference(const char *re
 
 int main(int argc, char **argv)
 {
-	int i, as_is = 0;
+	int i, as_is = 0, verify = 0;
 	unsigned char sha1[20];
 	const char *prefix = setup_git_directory();
 	
@@ -143,15 +136,13 @@ int main(int argc, char **argv)
 		char *dotdot;
 	
 		if (as_is) {
-			show_norev(arg);
+			show(arg);
 			continue;
 		}
 		if (*arg == '-') {
 			if (!strcmp(arg, "--")) {
-				show_default();
-				if (revs_only || flags_only)
-					break;
 				as_is = 1;
+				continue;
 			}
 			if (!strcmp(arg, "--default")) {
 				def = argv[i+1];
@@ -159,25 +150,24 @@ int main(int argc, char **argv)
 				continue;
 			}
 			if (!strcmp(arg, "--revs-only")) {
-				revs_only = 1;
+				filter &= ~DO_NOREV;
 				continue;
 			}
 			if (!strcmp(arg, "--no-revs")) {
-				no_revs = 1;
+				filter &= ~DO_REVS;
 				continue;
 			}
 			if (!strcmp(arg, "--flags")) {
-				flags_only = 1;
+				filter &= ~DO_NONFLAGS;
 				continue;
 			}
 			if (!strcmp(arg, "--no-flags")) {
-				no_flags = 1;
+				filter &= ~DO_FLAGS;
 				continue;
 			}
 			if (!strcmp(arg, "--verify")) {
-				revs_only = 1;
-				do_rev_argument = 0;
-				single_rev = 1;
+				filter &= ~(DO_FLAGS|DO_NOREV);
+				verify = 1;
 				continue;
 			}
 			if (!strcmp(arg, "--sq")) {
@@ -197,12 +187,17 @@ int main(int argc, char **argv)
 				continue;
 			}
 			if (!strcmp(arg, "--show-prefix")) {
-				puts(prefix);
+				if (prefix)
+					puts(prefix);
 				continue;
 			}
-			show_arg(arg);
+			if (verify)
+				die("Needed a single revision");
+			show_flag(arg);
 			continue;
 		}
+
+		/* Not a flag argument */
 		dotdot = strstr(arg, "..");
 		if (dotdot) {
 			unsigned char end[20];
@@ -212,9 +207,6 @@ int main(int argc, char **argv)
 				if (!*n)
 					n = "HEAD";
 				if (!get_sha1(n, end)) {
-					if (no_revs)
-						continue;
-					def = NULL;
 					show_rev(NORMAL, end, n);
 					show_rev(REVERSED, sha1, arg);
 					continue;
@@ -223,26 +215,21 @@ int main(int argc, char **argv)
 			*dotdot = '.';
 		}
 		if (!get_sha1(arg, sha1)) {
-			if (no_revs)
-				continue;
-			def = NULL;
 			show_rev(NORMAL, sha1, arg);
 			continue;
 		}
 		if (*arg == '^' && !get_sha1(arg+1, sha1)) {
-			if (no_revs)
-				continue;
-			def = NULL;
 			show_rev(REVERSED, sha1, arg+1);
 			continue;
 		}
-		show_default();
-		show_norev(arg);
+		if (verify)
+			die("Needed a single revision");
+		if ((filter & (DO_NONFLAGS|DO_NOREV)) ==
+		    (DO_NONFLAGS|DO_NOREV))
+			show(arg);
 	}
 	show_default();
-	if (single_rev && output_revs != 1) {
-		fprintf(stderr, "Needed a single revision\n");
-		exit(1);
-	}
+	if (verify && revs_count != 1)
+		die("Needed a single revision");
 	return 0;
 }