Re: [PATCH v3 10/13] map/print ranges along traversing the history topologically
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 12, 2010, 21:52 UTC
- Message-ID
- <7vmxtw8j1k.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1278829141-11900-10-git-send-email-struggleyb.nku@gmail.com>
Bo Yang <struggleyb.nku@gmail.com> 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.
Show 49 quoted lines
>
> Signed-off-by: Bo Yang <struggleyb.nku@gmail.com>
> ---
> 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.