[PATCH/RFC 0/3] teaching log's --glob=<glob> and friends to git shortlog

[PATCH/RFC 0/3] teaching log's --glob=<glob> and friends to git shortlog

[Jonathan Nieder]

Hi,

While looking over the setup_revisions code to work on an interesting
request from Josh Triplett[1] (that happens to coincide with a
longstanding wish of my own), I got distracted by a few things.  The
nominal subject of this series is to teach "git shortlog" to handle
commands like

	git shortlog --glob=remotes/charon/notes/*

Patch 1 makes the code clearer about something I had been confused
by for a long time --- the --no-walk option invented for "git show"'s
benefit has global effect.  So tricks like[2]

	git rev-list ^HEAD --no-walk commit1 commit2 ...

to list branches not contained in HEAD will not work, alas.

Patch 2 unindents the code a little.

Patch 3 is the title piece --- it adds --glob=, --tag=, and so forth
to the whitelist in handle_revision_opt so commands like "git
shortlog" that parse revision listing options in two stages will queue
them correctly.

While working on this I was struck by how close the setup_revisions
code has been to parseoptification all these years.  Maybe some
weekend.

Thoughts and improvements welcome, of course.

Jonathan Nieder (3):
  revisions: clarify handling of --no-walk and --do-walk
  revisions: split out handle_revision_pseudo_opt function
  revisions: allow --glob and friends in parse_options-enabled commands

 revision.c               |  138 ++++++++++++++++++++++++----------------------
 t/t6018-rev-list-glob.sh |   50 +++++++++++++++++
 2 files changed, 122 insertions(+), 66 deletions(-)

[1] http://bugs.debian.org/621601
[2] http://thread.gmane.org/gmane.comp.version-control.git/138109/focus=138124

[WRONG/PATCH 1/3] revisions: clarify handling of --no-walk and --do-walk

[Jonathan Nieder]

As v1.6.0-rc2~42 (Allow "non-option" revision options in
parse_option-enabled commands, 2008-07-31) explains, commands which
use parse_options() but also call setup_revisions() do their parsing
in two stages:

 1. first, they parse all options. Anything unknown goes to
    parse_revision_opt() (which calls handle_revision_opt), which
    may claim the option or say "I don't recognize this"

 2. the non-option remainder goes to setup_revisions() to
    actually get turned into revisions

Some revision options, like --all and --not, are "non-options" in that
they must be parsed in order with their revision counterparts in
setup_revisions().  It would be nice if --no-walk and --do-walk fell
in this category and set a flag only for revs coming after them on the
command line, but they do not, so move parsing of --no-walk and
--do-walk to the first "global options" stage for clarity.

---
Wait, the above is not actually the full story.  If I do

	git show maint..master

then this turns on walking automatically, to give the commit range
meaning.  Likewise

	git log --no-walk maint..master

will, in fact walk, but

	git log maint..master --no-walk

will not.  Which I should have understood from v1.6.0-rc2~42
(2008-07-31) already.  Will think more; sorry for the nonsense.

 revision.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/revision.c b/revision.c
index 0f38364..7b87bd0 100644
--- a/revision.c
+++ b/revision.c
@@ -1177,7 +1177,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	if (!strcmp(arg, "--all") || !strcmp(arg, "--branches") ||
 	    !strcmp(arg, "--tags") || !strcmp(arg, "--remotes") ||
 	    !strcmp(arg, "--reflog") || !strcmp(arg, "--not") ||
-	    !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk") ||
 	    !strcmp(arg, "--bisect"))
 	{
 		unkv[(*unkc)++] = arg;
@@ -1334,6 +1333,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->unpacked = 1;
 	} else if (!prefixcmp(arg, "--unpacked=")) {
 		die("--unpacked=<packfile> no longer supported.");
+	} else if (!strcmp(arg, "--no-walk")) {
+		revs->no_walk = 1;
+	} else if (!strcmp(arg, "--do-walk")) {
+		revs->no_walk = 0;
 	} else if (!strcmp(arg, "-r")) {
 		revs->diff = 1;
 		DIFF_OPT_SET(&revs->diffopt, RECURSIVE);
@@ -1622,14 +1625,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 				flags ^= UNINTERESTING;
 				continue;
 			}
-			if (!strcmp(arg, "--no-walk")) {
-				revs->no_walk = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--do-walk")) {
-				revs->no_walk = 0;
-				continue;
-			}
 			if (!strcmp(arg, "--stdin")) {
 				if (revs->disable_stdin) {
 					argv[left++] = arg;
-- 
1.7.5.rc3

[PATCH 2/3] revisions: split out handle_revision_pseudo_opt function

[Jonathan Nieder]

As v1.6.0-rc2~42 (Allow "non-option" revision options in
parse_option-enabled commands, 2008-07-31) explains, options handled
by setup_revisions fall into two categories:

 1. global options like --topo-order handled by parse_revision_opt,
    which can take detached arguments and can be parsed in advance;

 2. pseudo-options that must be parsed in order with their revision
    counterparts, like --not and --all.

The global options are taken care of by handle_revision_opt; the
pseudo-options are currently in a deeply indented portion of
setup_revisions.  Give them their own function for easier reading.

The only goal is to make setup_revisions easier to read straight
through.  No functional change intended.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Continuing without patch 1.

 revision.c |  123 +++++++++++++++++++++++++++++-------------------------------
 1 files changed, 59 insertions(+), 64 deletions(-)

diff --git a/revision.c b/revision.c
index 0f38364..c9b1e32 100644
--- a/revision.c
+++ b/revision.c
@@ -1526,6 +1526,59 @@ static void append_prune_data(const char ***prune_data, const char **av)
 	*prune_data = prune;
 }
 
+static int handle_revision_pseudo_opt(const char *submodule,
+				struct rev_info *revs,
+				int argc, const char **argv, int *flags)
+{
+	const char *arg = argv[0];
+	const char *optarg;
+	int argcount;
+
+	if (!strcmp(arg, "--all")) {
+		handle_refs(submodule, revs, *flags, for_each_ref_submodule);
+		handle_refs(submodule, revs, *flags, head_ref_submodule);
+	} else if (!strcmp(arg, "--branches")) {
+		handle_refs(submodule, revs, *flags, for_each_branch_ref_submodule);
+	} else if (!strcmp(arg, "--bisect")) {
+		handle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);
+		handle_refs(submodule, revs, *flags ^ UNINTERESTING, for_each_good_bisect_ref);
+		revs->bisect = 1;
+	} else if (!strcmp(arg, "--tags")) {
+		handle_refs(submodule, revs, *flags, for_each_tag_ref_submodule);
+	} else if (!strcmp(arg, "--remotes")) {
+		handle_refs(submodule, revs, *flags, for_each_remote_ref_submodule);
+	} else if ((argcount = parse_long_opt("glob", argv, &optarg))) {
+		struct all_refs_cb cb;
+		init_all_refs_cb(&cb, revs, *flags);
+		for_each_glob_ref(handle_one_ref, optarg, &cb);
+		return argcount;
+	} else if (!prefixcmp(arg, "--branches=")) {
+		struct all_refs_cb cb;
+		init_all_refs_cb(&cb, revs, *flags);
+		for_each_glob_ref_in(handle_one_ref, arg + 11, "refs/heads/", &cb);
+	} else if (!prefixcmp(arg, "--tags=")) {
+		struct all_refs_cb cb;
+		init_all_refs_cb(&cb, revs, *flags);
+		for_each_glob_ref_in(handle_one_ref, arg + 7, "refs/tags/", &cb);
+	} else if (!prefixcmp(arg, "--remotes=")) {
+		struct all_refs_cb cb;
+		init_all_refs_cb(&cb, revs, *flags);
+		for_each_glob_ref_in(handle_one_ref, arg + 10, "refs/remotes/", &cb);
+	} else if (!strcmp(arg, "--reflog")) {
+		handle_reflog(revs, *flags);
+	} else if (!strcmp(arg, "--not")) {
+		*flags ^= UNINTERESTING;
+	} else if (!strcmp(arg, "--no-walk")) {
+		revs->no_walk = 1;
+	} else if (!strcmp(arg, "--do-walk")) {
+		revs->no_walk = 0;
+	} else {
+		return 0;
+	}
+
+	return 1;
+}
+
 /*
  * Parse revision information, filling in the "rev_info" structure,
  * and removing the used arguments from the argument list.
@@ -1538,8 +1591,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	int i, flags, left, seen_dashdash, read_from_stdin, got_rev_arg = 0;
 	const char **prune_data = NULL;
 	const char *submodule = NULL;
-	const char *optarg;
-	int argcount;
 
 	if (opt)
 		submodule = opt->submodule;
@@ -1566,70 +1617,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		if (*arg == '-') {
 			int opts;
 
-			if (!strcmp(arg, "--all")) {
-				handle_refs(submodule, revs, flags, for_each_ref_submodule);
-				handle_refs(submodule, revs, flags, head_ref_submodule);
-				continue;
-			}
-			if (!strcmp(arg, "--branches")) {
-				handle_refs(submodule, revs, flags, for_each_branch_ref_submodule);
-				continue;
-			}
-			if (!strcmp(arg, "--bisect")) {
-				handle_refs(submodule, revs, flags, for_each_bad_bisect_ref);
-				handle_refs(submodule, revs, flags ^ UNINTERESTING, for_each_good_bisect_ref);
-				revs->bisect = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--tags")) {
-				handle_refs(submodule, revs, flags, for_each_tag_ref_submodule);
-				continue;
-			}
-			if (!strcmp(arg, "--remotes")) {
-				handle_refs(submodule, revs, flags, for_each_remote_ref_submodule);
-				continue;
-			}
-			if ((argcount = parse_long_opt("glob", argv + i, &optarg))) {
-				struct all_refs_cb cb;
-				i += argcount - 1;
-				init_all_refs_cb(&cb, revs, flags);
-				for_each_glob_ref(handle_one_ref, optarg, &cb);
-				continue;
-			}
-			if (!prefixcmp(arg, "--branches=")) {
-				struct all_refs_cb cb;
-				init_all_refs_cb(&cb, revs, flags);
-				for_each_glob_ref_in(handle_one_ref, arg + 11, "refs/heads/", &cb);
-				continue;
-			}
-			if (!prefixcmp(arg, "--tags=")) {
-				struct all_refs_cb cb;
-				init_all_refs_cb(&cb, revs, flags);
-				for_each_glob_ref_in(handle_one_ref, arg + 7, "refs/tags/", &cb);
-				continue;
-			}
-			if (!prefixcmp(arg, "--remotes=")) {
-				struct all_refs_cb cb;
-				init_all_refs_cb(&cb, revs, flags);
-				for_each_glob_ref_in(handle_one_ref, arg + 10, "refs/remotes/", &cb);
-				continue;
-			}
-			if (!strcmp(arg, "--reflog")) {
-				handle_reflog(revs, flags);
-				continue;
-			}
-			if (!strcmp(arg, "--not")) {
-				flags ^= UNINTERESTING;
-				continue;
-			}
-			if (!strcmp(arg, "--no-walk")) {
-				revs->no_walk = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--do-walk")) {
-				revs->no_walk = 0;
+			opts = handle_revision_pseudo_opt(submodule,
+						revs, argc - i, argv + i,
+						&flags);
+			if (opts > 0) {
+				i += opts - 1;
 				continue;
 			}
+
 			if (!strcmp(arg, "--stdin")) {
 				if (revs->disable_stdin) {
 					argv[left++] = arg;
-- 
1.7.5.rc3

[PATCH 3/3] revisions: allow --glob and friends in parse_options-enabled commands

[Jonathan Nieder]

As v1.6.0-rc2~42 (2008-07-31) explains, even pseudo-options like --not
and --glob that need to be parsed in order with revisions should be
marked handled by handle_revision_opt to avoid an error when
parse_revision_opt callers like "git shortlog" encounter them.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thanks for reading.  I hope despite the mess it was not too
unpleasant.

 revision.c               |   14 ++++++++++++-
 t/t6018-rev-list-glob.sh |   50 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 1 deletions(-)

diff --git a/revision.c b/revision.c
index c9b1e32..2389764 100644
--- a/revision.c
+++ b/revision.c
@@ -1178,7 +1178,9 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	    !strcmp(arg, "--tags") || !strcmp(arg, "--remotes") ||
 	    !strcmp(arg, "--reflog") || !strcmp(arg, "--not") ||
 	    !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk") ||
-	    !strcmp(arg, "--bisect"))
+	    !strcmp(arg, "--bisect") || !prefixcmp(arg, "--glob=") ||
+	    !prefixcmp(arg, "--branches=") || !prefixcmp(arg, "--tags=") ||
+	    !prefixcmp(arg, "--remotes="))
 	{
 		unkv[(*unkc)++] = arg;
 		return 1;
@@ -1534,6 +1536,16 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	const char *optarg;
 	int argcount;
 
+	/*
+	 * NOTE!
+	 *
+	 * Commands like "git shortlog" will not accept the options below
+	 * unless parse_revision_opt queues them (as opposed to erroring
+	 * out).
+	 *
+	 * When implementing your new pseudo-option, remember to
+	 * register it in the list at the top of handle_revision_opt.
+	 */
 	if (!strcmp(arg, "--all")) {
 		handle_refs(submodule, revs, *flags, for_each_ref_submodule);
 		handle_refs(submodule, revs, *flags, head_ref_submodule);
diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index fb8291c..f00cebf 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -69,6 +69,18 @@ test_expect_success 'rev-parse --glob=heads/subspace' '
 
 '
 
+test_expect_failure 'rev-parse accepts --glob as detached option' '
+
+	compare rev-parse "subspace/one subspace/two" "--glob heads/subspace"
+
+'
+
+test_expect_failure 'rev-parse is not confused by option-like glob' '
+
+	compare rev-parse "master" "--glob --symbolic master"
+
+'
+
 test_expect_success 'rev-parse --branches=subspace/*' '
 
 	compare rev-parse "subspace/one subspace/two" "--branches=subspace/*"
@@ -129,6 +141,12 @@ test_expect_success 'rev-list --glob refs/heads/subspace/*' '
 
 '
 
+test_expect_success 'rev-list not confused by option-like --glob arg' '
+
+	compare rev-list "master" "--glob -0 master"
+
+'
+
 test_expect_success 'rev-list --glob=heads/subspace/*' '
 
 	compare rev-list "subspace/one subspace/two" "--glob=heads/subspace/*"
@@ -213,4 +231,36 @@ test_expect_success 'rev-list --remotes=foo' '
 
 '
 
+test_expect_success 'shortlog accepts --glob/--tags/--remotes' '
+
+	compare shortlog "subspace/one subspace/two" --branches=subspace &&
+	compare shortlog \
+	  "master subspace-x someref other/three subspace/one subspace/two" \
+	  --branches &&
+	compare shortlog master "--glob=heads/someref/* master" &&
+	compare shortlog "subspace/one subspace/two other/three" \
+	  "--glob=heads/subspace/* --glob=heads/other/*" &&
+	compare shortlog \
+	  "master other/three someref subspace-x subspace/one subspace/two" \
+	  "--glob=heads/*" &&
+	compare shortlog foo/bar --tags=foo &&
+	compare shortlog foo/bar --tags &&
+	compare shortlog foo/baz --remotes=foo
+
+'
+
+test_expect_failure 'shortlog accepts --glob as detached option' '
+
+	compare shortlog \
+	  "master other/three someref subspace-x subspace/one subspace/two" \
+	  "--glob heads/*"
+
+'
+
+test_expect_failure 'shortlog --glob is not confused by option-like argument' '
+
+	compare shortlog master "--glob -e master"
+
+'
+
 test_done
-- 
1.7.5.rc3

Re: [WRONG/PATCH 1/3] revisions: clarify handling of --no-walk and --do-walk

[Michael J Gruber]

Jonathan Nieder venit, vidit, dixit 21.04.2011 12:39:
> As v1.6.0-rc2~42 (Allow "non-option" revision options in
> parse_option-enabled commands, 2008-07-31) explains, commands which
> use parse_options() but also call setup_revisions() do their parsing
> in two stages:
> 
>  1. first, they parse all options. Anything unknown goes to
>     parse_revision_opt() (which calls handle_revision_opt), which
>     may claim the option or say "I don't recognize this"
> 
>  2. the non-option remainder goes to setup_revisions() to
>     actually get turned into revisions
> 
> Some revision options, like --all and --not, are "non-options" in that
> they must be parsed in order with their revision counterparts in
> setup_revisions().  It would be nice if --no-walk and --do-walk fell
> in this category and set a flag only for revs coming after them on the
> command line, but they do not, so move parsing of --no-walk and
> --do-walk to the first "global options" stage for clarity.
> 
> ---
> Wait, the above is not actually the full story.  If I do
> 
> 	git show maint..master
> 
> then this turns on walking automatically, to give the commit range
> meaning.  Likewise
> 
> 	git log --no-walk maint..master
> 
> will, in fact walk, but
> 
> 	git log maint..master --no-walk
> 
> will not.  Which I should have understood from v1.6.0-rc2~42
> (2008-07-31) already.  Will think more; sorry for the nonsense.

This is not unrelated to the tip of gitster/mg/show-without-prune, i.e.

0c738b6 (builtin/show: do not prune by pathspec, 2011-04-01)

See http://permalink.gmane.org/gmane.comp.version-control.git/170461

We should rethink the ui balance between deviating from the usual log
option processing and the usefulness here.

Michael

Re: [PATCH/RFC 0/3] teaching log's --glob=<glob> and friends to git shortlog

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 	git rev-list ^HEAD --no-walk commit1 commit2 ...

That should be "git rev-list --no-walk ^HEAD commit1 commit2 ..." from the
syntactical point of view to begin with, but more importantly, what does
it mean to ask excluding commits reachable from HEAD without walking?

As ^HEAD is inherently about walking the history to paint the part of DAG
that are not interesting because the nodes in there are reachable from
there, the request that command line tries to make does not make much
sense.

As I said in

 http://thread.gmane.org/gmane.comp.version-control.git/170457/focus=170629

the "--no-walk" hack happens to work by accident, and we would need to
rethink the way it operates and revamp it.

Re: [PATCH/RFC 0/3] teaching log's --glob=<glob> and friends to git shortlog

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> 	git rev-list ^HEAD --no-walk commit1 commit2 ...
>
> That should be "git rev-list --no-walk ^HEAD commit1 commit2 ..." from the
> syntactical point of view to begin with, but more importantly, what does
> it mean to ask excluding commits reachable from HEAD without walking?

This framing of the question is based on --no-walk being a global
flag.  It is possible to imagine it instead being a positional
argument like --not, without invasive changes to the code structure,
as in the patch below.

> As ^HEAD is inherently about walking the history to paint the part of DAG
> that are not interesting because the nodes in there are reachable from
> there, the request that command line tries to make does not make much
> sense.

A separate question is whether paying attention to the position of
--no-walk in the command line was a good idea.  Let's see what it
would take to reconcile my belief that "no, it isn't" with current
practice.

The idea would be to introduce a new option --behave-like-show with
the following semantics:

. "git log --behave-like-show foo..bar" is a synonym for "git log foo..bar"
. "git log --behave-like-show -5 foo" is a synonym for "git log -5 foo"
. "git log --behave-like-show --do-walk foo" is a synonym for "git log foo"
. "git log --behave-like-show foo" is a synonym for "git log --no-walk foo"

Then we could remove the existing magic that turns "git log --no-walk foo..bar"
to "git log foo..bar" and error out since the operator has requested
something undoable ("--no-walk is incompatible with negative revisions").

More precisely, it could work like so:

 - --no-walk/--do-walk becomes a tristate (walk, don't walk, default)
 - --behave-like-show state is another tristate (seen ^foo or -5,
   using --behave-like-show but not seen ^foo or -5, neither)

The rules:

 - if we saw --no-walk (and it was not cancelled by a later --do-walk)
   along with ^foo, error out.
 - otherwise, if we saw --do-walk (and it was not cancelled by a later
   --no-walk), walk.
 - otherwise, if we saw --no-walk, don't walk.  If we saw ^foo, walk.
 - by default, walk if and only if we didn't see --behave-like-show.

So the usual "git show" usage patterns would work, and the position of
--no-walk (except in that it can be cancelled by --do-walk) wouldn't
matter.

> As I said in
>
>  http://thread.gmane.org/gmane.comp.version-control.git/170457/focus=170629
>
> the "--no-walk" hack happens to work by accident, and we would need to
> rethink the way it operates and revamp it.

Yes, [1] is interesting (though a bit broader in scope).

The promised nonsense patch implementing a NO_WALK commit flag (i.e.,
the approach the patch upthread was meant to clarify we are not using)
follows.

[1] http://thread.gmane.org/gmane.comp.version-control.git/170457/focus=170839

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Please feel free to play around with it but do not consider
it for inclusion. :)
---
 revision.c |   37 ++++++++++++++++++++++++++++---------
 revision.h |    4 +++-
 2 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/revision.c b/revision.c
index 0f38364..dae16da 100644
--- a/revision.c
+++ b/revision.c
@@ -133,8 +133,10 @@ void mark_parents_uninteresting(struct commit *commit)
 
 static void add_pending_object_with_mode(struct rev_info *revs, struct object *obj, const char *name, unsigned mode)
 {
-	if (revs->no_walk && (obj->flags & UNINTERESTING))
+	if (obj->flags & UNINTERESTING) {
+		obj->flags &= ~NO_WALK;
 		revs->no_walk = 0;
+	}
 	if (revs->reflog_info && obj->type == OBJ_COMMIT) {
 		struct strbuf buf = STRBUF_INIT;
 		int len = interpret_branch_name(name, &buf);
@@ -166,6 +168,10 @@ void add_head_to_pending(struct rev_info *revs)
 	obj = parse_object(sha1);
 	if (!obj)
 		return;
+	if (revs->no_walk)
+		obj->flags |= NO_WALK;
+	else
+		obj->flags &= ~NO_WALK;
 	add_pending_object(revs, obj, "HEAD");
 }
 
@@ -506,7 +512,7 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,
 	 */
 	try_to_simplify_commit(revs, commit);
 
-	if (revs->no_walk)
+	if ((commit->object.flags & NO_WALK) && !revs->force_walk)
 		return 0;
 
 	left_flag = (commit->object.flags & SYMMETRIC_LEFT);
@@ -1062,6 +1068,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
 			} else
 				a->object.flags |= flags_exclude;
 			b->object.flags |= flags;
+			b->object.flags &= ~NO_WALK;
 			add_pending_object(revs, &a->object, this);
 			add_pending_object(revs, &b->object, next);
 			return 0;
@@ -1128,6 +1135,7 @@ static void read_revisions_from_stdin(struct rev_info *revs, const char ***prune
 {
 	struct strbuf sb;
 	int seen_dashdash = 0;
+	int flags = 0;
 
 	strbuf_init(&sb, 1000);
 	while (strbuf_getwholeline(&sb, stdin, '\n') != EOF) {
@@ -1143,7 +1151,11 @@ static void read_revisions_from_stdin(struct rev_info *revs, const char ***prune
 			}
 			die("options not supported in --stdin mode");
 		}
-		if (handle_revision_arg(sb.buf, revs, 0, 1))
+		if (revs->no_walk)
+			flags |= NO_WALK;
+		else
+			flags &= ~NO_WALK;
+		if (handle_revision_arg(sb.buf, revs, flags, 1))
 			die("bad revision '%s'", sb.buf);
 	}
 	if (seen_dashdash)
@@ -1186,7 +1198,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 
 	if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
 		revs->max_count = atoi(optarg);
-		revs->no_walk = 0;
+		revs->force_walk = 1;
 		return argcount;
 	} else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
 		revs->skip_count = atoi(optarg);
@@ -1194,16 +1206,16 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if ((*arg == '-') && isdigit(arg[1])) {
 	/* accept -<digit>, like traditional "head" */
 		revs->max_count = atoi(arg + 1);
-		revs->no_walk = 0;
+		revs->force_walk = 1;
 	} else if (!strcmp(arg, "-n")) {
 		if (argc <= 1)
 			return error("-n requires an argument");
 		revs->max_count = atoi(argv[1]);
-		revs->no_walk = 0;
+		revs->force_walk = 1;
 		return 2;
 	} else if (!prefixcmp(arg, "-n")) {
 		revs->max_count = atoi(arg + 2);
-		revs->no_walk = 0;
+		revs->force_walk = 1;
 	} else if ((argcount = parse_long_opt("max-age", argv, &optarg))) {
 		revs->max_age = atoi(optarg);
 		return argcount;
@@ -1563,6 +1575,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	read_from_stdin = 0;
 	for (left = i = 1; i < argc; i++) {
 		const char *arg = argv[i];
+		if (revs->no_walk)
+			flags |= NO_WALK;
+		else
+			flags &= ~NO_WALK;
+
 		if (*arg == '-') {
 			int opts;
 
@@ -1688,6 +1705,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		if (get_sha1_with_mode(revs->def, sha1, &mode))
 			die("bad default revision '%s'", revs->def);
 		object = get_reference(revs, revs->def, sha1, 0);
+		if (revs->no_walk)
+			object->flags |= NO_WALK;
+		else
+			object->flags &= ~NO_WALK;
 		add_pending_object_with_mode(revs, object, revs->def, mode);
 	}
 
@@ -1951,8 +1972,6 @@ int prepare_revision_walk(struct rev_info *revs)
 	}
 	free(list);
 
-	if (revs->no_walk)
-		return 0;
 	if (revs->limited)
 		if (limit_list(revs) < 0)
 			return -1;
diff --git a/revision.h b/revision.h
index 9fd8f30..60f00ff 100644
--- a/revision.h
+++ b/revision.h
@@ -15,7 +15,8 @@
 #define ADDED		(1u<<7)	/* Parents already parsed and added? */
 #define SYMMETRIC_LEFT	(1u<<8)
 #define PATCHSAME	(1u<<9)
-#define ALL_REV_FLAGS	((1u<<10)-1)
+#define NO_WALK		(1u<<10)
+#define ALL_REV_FLAGS	((1u<<11)-1)
 
 #define DECORATE_SHORT_REFS	1
 #define DECORATE_FULL_REFS	2
@@ -42,6 +43,7 @@ struct rev_info {
 	unsigned int	dense:1,
 			prune:1,
 			no_walk:1,
+			force_walk:1,
 			show_all:1,
 			remove_empty_trees:1,
 			simplify_history:1,
-- 
1.7.5.rc3

Re: [WRONG/PATCH 1/3] revisions: clarify handling of --no-walk and --do-walk

[Jonathan Nieder]

Michael J Gruber wrote:

> This is not unrelated to the tip of gitster/mg/show-without-prune, i.e.
>
> 0c738b6 (builtin/show: do not prune by pathspec, 2011-04-01)

True.

> We should rethink the ui balance between deviating from the usual log
> option processing and the usefulness here.

Could you expand on that?  I think --no-walk being a positional
argument is just an ugly consequence of the code that permits

	git show -5 master
	git show maint..
	git show --do-walk master

while making sure

	git show master
	git show --do-walk --no-walk master

still show only one commit.  I'm not convinced it's meant to be useful
beyond that.

(Maybe "git log --no-walk maint..HEAD" should be an error because
meaningless, while "git rev-list --no-walk maint..HEAD" should be
accepted so people can continue write their own "git show"-like
commands.  Unfortunately the popular idiom of using git log
--format=... instead of

	git rev-list ... |
	git diff-tree -s --format=... --stdin

in scripts makes this porcelain-plumbing distinction more fuzzy than I
would like.)

Re: [WRONG/PATCH 1/3] revisions: clarify handling of --no-walk and --do-walk

[Michael J Gruber]

Jonathan Nieder venit, vidit, dixit 21.04.2011 23:30:
> Michael J Gruber wrote:
> 
>> This is not unrelated to the tip of gitster/mg/show-without-prune, i.e.
>>
>> 0c738b6 (builtin/show: do not prune by pathspec, 2011-04-01)
> 
> True.
> 
>> We should rethink the ui balance between deviating from the usual log
>> option processing and the usefulness here.
> 
> Could you expand on that?  I think --no-walk being a positional
> argument is just an ugly consequence of the code that permits
> 

Sorry for being fuzzy. What I meant was: There is the systematic
approach which gives the user exactly what he asks for, i.e.: log,
rev-list default to walking, show to not walking (and rev-parse of
course), and when a user feeds a range it may or may may not make sense.
Similarly, "git show commit -- path" returns nothing at all if path is
not touched by commit because the commit gets pruned.

We don't do the systematic approach now. In some situations, some
commands switch on the walker automatically (I think "show A..B") to
make things more useful (to most users) but less systematic, even less
predictable if you don't know these deviations/exceptions. I've
suggested such a "usefulness exception" myself (don't prune commits by
path for "show").

The strict, systematic approach produces some command/argument
combinations which are not useful or rarely useful.

The "helpful" approach produces special casing which may make things
confusing if there's too much of it.

Finding the right balance is probably more difficult than being
radically systematic...

Michael

Re: [WRONG/PATCH 1/3] revisions: clarify handling of --no-walk and --do-walk

[Jonathan Nieder]

Michael J Gruber wrote:

> We don't do the systematic approach now. In some situations, some
> commands switch on the walker automatically (I think "show A..B") to
> make things more useful (to most users) but less systematic, even less
> predictable if you don't know these deviations/exceptions. I've
> suggested such a "usefulness exception" myself (don't prune commits by
> path for "show").

Ah.  To be clearer about the present state:

 - cmd_show reimplements much of get_revision, to work around the
   revision walking machinery's lack of callbacks to print tags,
   blobs, and so on.

 - ^A means "--do-walk ^A", and A..B means "--do-walk ^A B".  This
   holds in rev-list, log, show, etc --- they all share the code that
   does this.

 - Similarly, -5 means "--do-walk -5".

 - rev-parse shares a revision parser (get_sha1) with rev-list, but it
   doesn't share an option parser (alas).

I personally kind of like the "don't prune commits by path with
--no-walk" idea.  Not sure what happened to that.

> The strict, systematic approach produces some command/argument
> combinations which are not useful or rarely useful.

Well, --no-walk foo..bar would be nonsense regardless of whether "git
show" did the equivalent of "git rev-list --no-walk ...".  And when
the user supplies nonsense, why should she expect us not to give
nonsense back?  So how about something along these lines?

Needs documentation and tests.

-- >8 --
Subject: revisions: treat "log A..B --no-walk" as a synonym for "--no-walk A..B"

When git learned to treat "git show A..B" as "git show --do-walk
A..B", it was only an accident that the implied --do-walk had that
position.

Sure, it meant that one could write "git show A..B --no-walk" to get
the ancient behavior of showing "B" unless the two commits are the
same, but that is not a convenience --- on the contrary, it's a
confusing wart that has required effort to preserve ever since (see
v1.6.0-rc2~42, 2008-07-31, for example).  So let's simplify the
semantics.  After this patch:

Attempts to show A..B or show -5 _always_ walk.  "git log A..B
--no-walk" simply doesn't make sense, so we assume the A..B came from
the end-user who wanted to override a script's suggestion not to walk.

The effect of --no-walk is the same everywhere, except that later
--do-walk overrides early --no-walk.

There is no longer a way to limit the number of commits listed to 5
without walking or to provide a negative revision without walking.
If someone wants that, we can introduce a new option instead of
tacking it onto the hack that supports "git show".

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 revision.c |   15 +++++++++------
 revision.h |    1 +
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/revision.c b/revision.c
index 0f38364..0996c6f 100644
--- a/revision.c
+++ b/revision.c
@@ -133,8 +133,8 @@ void mark_parents_uninteresting(struct commit *commit)
 
 static void add_pending_object_with_mode(struct rev_info *revs, struct object *obj, const char *name, unsigned mode)
 {
-	if (revs->no_walk && (obj->flags & UNINTERESTING))
-		revs->no_walk = 0;
+	if (obj->flags & UNINTERESTING)
+		revs->implied_walk = 1;
 	if (revs->reflog_info && obj->type == OBJ_COMMIT) {
 		struct strbuf buf = STRBUF_INIT;
 		int len = interpret_branch_name(name, &buf);
@@ -1186,7 +1186,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 
 	if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
 		revs->max_count = atoi(optarg);
-		revs->no_walk = 0;
+		revs->implied_walk = 1;
 		return argcount;
 	} else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
 		revs->skip_count = atoi(optarg);
@@ -1194,16 +1194,16 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if ((*arg == '-') && isdigit(arg[1])) {
 	/* accept -<digit>, like traditional "head" */
 		revs->max_count = atoi(arg + 1);
-		revs->no_walk = 0;
+		revs->implied_walk = 1;
 	} else if (!strcmp(arg, "-n")) {
 		if (argc <= 1)
 			return error("-n requires an argument");
 		revs->max_count = atoi(argv[1]);
-		revs->no_walk = 0;
+		revs->implied_walk = 1;
 		return 2;
 	} else if (!prefixcmp(arg, "-n")) {
 		revs->max_count = atoi(arg + 2);
-		revs->no_walk = 0;
+		revs->implied_walk = 1;
 	} else if ((argcount = parse_long_opt("max-age", argv, &optarg))) {
 		revs->max_age = atoi(optarg);
 		return argcount;
@@ -1691,6 +1691,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		add_pending_object_with_mode(revs, object, revs->def, mode);
 	}
 
+	if (revs->implied_walk)
+		revs->no_walk = 0;
+
 	/* Did the user ask for any diff output? Run the diff! */
 	if (revs->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT)
 		revs->diff = 1;
diff --git a/revision.h b/revision.h
index 9fd8f30..c18d2c1 100644
--- a/revision.h
+++ b/revision.h
@@ -42,6 +42,7 @@ struct rev_info {
 	unsigned int	dense:1,
 			prune:1,
 			no_walk:1,
+			implied_walk:1,
 			show_all:1,
 			remove_empty_trees:1,
 			simplify_history:1,
-- 
1.7.5.rc3

Re: [WRONG/PATCH 1/3] revisions: clarify handling of --no-walk and --do-walk

[Michael J Gruber]

Jonathan Nieder venit, vidit, dixit 25.04.2011 02:21:
> Michael J Gruber wrote:
> 
>> We don't do the systematic approach now. In some situations, some
>> commands switch on the walker automatically (I think "show A..B") to
>> make things more useful (to most users) but less systematic, even less
>> predictable if you don't know these deviations/exceptions. I've
>> suggested such a "usefulness exception" myself (don't prune commits by
>> path for "show").
> 
> Ah.  To be clearer about the present state:
> 
>  - cmd_show reimplements much of get_revision, to work around the
>    revision walking machinery's lack of callbacks to print tags,
>    blobs, and so on.
> 
>  - ^A means "--do-walk ^A", and A..B means "--do-walk ^A B".  This
>    holds in rev-list, log, show, etc --- they all share the code that
>    does this.
> 
>  - Similarly, -5 means "--do-walk -5".
> 
>  - rev-parse shares a revision parser (get_sha1) with rev-list, but it
>    doesn't share an option parser (alas).
> 
> I personally kind of like the "don't prune commits by path with
> --no-walk" idea.  Not sure what happened to that.

It got dropped after the switch to the new cycle. The question/problem
was whether some people used "git show A B C -- path" as a kind of
commit filter, selecting only commits which touch path (i.e. prune
commits by path). I would claim it's the wrong command to do that and
was never documented anyways...

So, my choice would be "do not prune commits unless --do-walk was given"
(where "was given" may include "was switched on by our range=>walk logic").

Another choice would be "do not prune if there is only one rev argument
and no --do-walk".

> Ah.  To be clearer about the present state:
> 
>  - cmd_show reimplements much of get_revision, to work around the
>    revision walking machinery's lack of callbacks to print tags,
>    blobs, and so on.
> 

You mean, other than pretty formats... I think our code does a couple of
things by hand which could be done with a pretty format. Though a
callback may be more efficient, unless our formats are pre-parsed.

Michael