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

[Jonathan Nieder <jrnieder@gmail.com>]

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