Re: [PATCH 03/12] add the basic data structure for line level history

[Bo Yang <struggleyb.nku@gmail.com>]

On Mon, Jun 28, 2010 at 2:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> +/*
>> + * copied from blame.c, indeed, we can even to use this to test
>
> A bit of refactoring would certainly help, before this series graduates
> the WIP/RFC stage.

I have put the 'parse_loc' into the line.c and make blame.c use it.
Please see the commit log. :-)

>> diff --git a/revision.c b/revision.c
>> index 7847921..3186e0e 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -1397,18 +1397,19 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>>       return 1;
>>  }
>>
>> -void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
>> +int parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
>>                       const struct option *options,
>>                       const char * const usagestr[])
>>  {
>>       int n = handle_revision_opt(revs, ctx->argc, ctx->argv,
>>                                   &ctx->cpidx, ctx->out);
>>       if (n <= 0) {
>> -             error("unknown option `%s'", ctx->argv[0]);
>> -             usage_with_options(usagestr, options);
>> +             return -1;
>>       }
>>       ctx->argv += n;
>>       ctx->argc -= n;
>> +
>> +     return 0;
>>  }
>
> The function has existing callers and they expect it to fail with
> usage_with_options(), never to return.  Doesn't this change break them?
>
> This change is not described nor justified in the commit log message.

Yes, this will affect other callers badly. But, with the new one pass
parsing method, we don't need this change anymore, so please forget
it.

>>  static int for_each_bad_bisect_ref(each_ref_fn fn, void *cb_data)
>> @@ -1631,6 +1632,12 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>>       if (revs->combine_merges)
>>               revs->ignore_merges = 0;
>>       revs->diffopt.abbrev = revs->abbrev;
>> +
>> +     if (revs->line) {
>> +             revs->limited = 1;
>> +             revs->topo_order = 1;
>> +     }
>> +
>>       if (diff_setup_done(&revs->diffopt) < 0)
>>               die("diff_setup_done failed");
>>
>> diff --git a/revision.h b/revision.h
>> index 855464f..26c2ff5 100644
>> --- a/revision.h
>> +++ b/revision.h
>> @@ -14,6 +14,7 @@
>>  #define CHILD_SHOWN  (1u<<6)
>>  #define ADDED                (1u<<7) /* Parents already parsed and added? */
>>  #define SYMMETRIC_LEFT       (1u<<8)
>> +#define RANGE_UPDATE (1u<<9) /* for line level traverse */
>>  #define ALL_REV_FLAGS        ((1u<<9)-1)
>
> It doesn't make sense to add a global flag here and keep ALL_REV_FLAGS the
> same value.  Have you audited the existing code and made sure that they
> either do use 1<<9 for their own or their uses of such a custom flag are
> compatible with this?
>

Hmm, I really mean the ALL_REV_FLAGS would be ((1u<<10)-1). :-)

And all the other comments from you are very helpful and I have
changed the according it. Thanks a lot!

-- 
Regards!
Bo
----------------------------
My blog: http://blog.morebits.org
Why Git: http://www.whygitisbetterthanx.com/