Re: [PATCH v3 10/13] map/print ranges along traversing the history topologically
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:49:07
Bo Yang [off-list ref] writes:
Since ranges may change in different branches, we should make sure we do not pass range to parent until all the ranges get 'combined' at the commit which is a split commit. So, topological traversing is necessary.
Without explaining what a "split commit" is, "So, ... is necessary" doesn't really justify this.
quoted hunk ↗ jump to hunk
Signed-off-by: Bo Yang <redacted> --- builtin/log.c | 5 ++++- line.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ line.h | 2 ++ 3 files changed, 60 insertions(+), 1 deletions(-)diff --git a/builtin/log.c b/builtin/log.c index 1e90b03..47b386d 100644 --- a/builtin/log.c +++ b/builtin/log.c@@ -615,7 +615,10 @@ int cmd_log(int argc, const char **argv, const char *prefix) memset(&opt, 0, sizeof(opt)); opt.def = "HEAD"; cmd_log_init(argc, argv, prefix, &rev, &opt); - return cmd_log_walk(&rev); + if (rev.line) + return cmd_line_log_walk(&rev); + else + return cmd_log_walk(&rev); } /* format-patch */diff --git a/line.c b/line.c index 8813376..5c8f77a 100644 --- a/line.c +++ b/line.c@@ -1196,3 +1196,57 @@ static void line_log_flush(struct rev_info *rev, struct commit *c) } } +int cmd_line_log_walk(struct rev_info *rev) +{ + struct commit *commit; + struct commit_list *list = NULL; + struct diff_line_range *r = NULL; + + if (prepare_revision_walk(rev)) + die("revision walk prepare failed"); + + list = rev->commits; + if (list) { + list->item->object.flags |= RANGE_UPDATE; + list = list->next; + } + /* Clear the flags */ + while (list) { + list->item->object.flags &= 0x0;
Just assign "= 0" instead, please. But more importantly, why is it safe to clear all the flags? Even if your traversal is limited (i.e. prepare_revision_walk() already walked everything), at least wouldn't you need to keep flags like SYMMETRIC_LEFT, UNINTERESTING, BOUNDARY etc. depending on what the end user asked from the command line? If you ask prepare_revison_walk()->limit_list() callchain to run a limited and topo-sorted traversal, wouldn't the resulting revs->list have commits in the order you need already? Why do you need to have a custom walker here? I am not suggesting to roll this logic into prepare_revision_walk() and limit_list(); there are a lot more than "we need topological order" going on in this function, and the log message needs to explain what they are.