Hi,
Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@xxxxxxxxx> 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@xxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html