From: Elijah Newren <newren@gmail.com>
Date: Wed, 25 Aug 2010 23:50:39 GMT
Subject: Re: [PATCH 2/7] Fix ignoring of pathspecs with rev-list --objects
Message-ID: <AANLkTimCv-fn-h21pM8L9NiGg3w7OXBJ-kfiC+HJLsTP@mail.gmail.com>
In-Reply-To: <AANLkTimguNHXSFyO1EvS+dHbHiXU-dXTNwkWfEsVUbVC@mail.gmail.com>

On Wed, Aug 25, 2010 at 4:11 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> On Wed, Aug 25, 2010 at 12:53 PM, Elijah Newren <newren@gmail.com> wrote:
>> @@ -84,7 +85,24 @@ static void process_tree(struct rev_info *revs,
>>
>>        init_tree_desc(&desc, tree->buffer, tree->size);
>>
>> -       while (tree_entry(&desc, &entry)) {
>> +       for (; desc.size; update_tree_entry(&desc)) {
>> +               entry = desc.entry;
>> +
>> +               if (!all_interesting) {
>> +                       char *full_path = path_name(path, name);
>> +                       int full_path_len = strlen(full_path);
>> +                       int showit = tree_entry_interesting(&desc,
>> full_path, full_path_len,
>> +                                                           &revs->diffopt);
>> +                       free(full_path);
>
> I wonder if we can avoid xmalloc/free so many times here. If full_path
> is unchanged, how about moving it outside the loop?

Good point.

>> diff --git a/tree-diff.c b/tree-diff.c
>> index cd659c6..2fb670b 100644
>> --- a/tree-diff.c
>> +++ b/tree-diff.c
>> @@ -91,7 +91,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct
>> tree_desc *t2, const
>>  *  - zero for no
>>  *  - negative for "no, and no subsequent entries will be either"
>>  */
>> -static int tree_entry_interesting(struct tree_desc *desc, const char *base,
>> int baselen, struct diff_options *opt)
>> +int tree_entry_interesting(struct tree_desc *desc, const char *base, int
>> baselen, struct diff_options *opt)
>>  {
>>        const char *path;
>>        const unsigned char *sha1;
>
> While at it, can you please also fix its comments? The comments say
> pathspec while what it uses is actually path prefix. Maybe something
> like this
>
> diff --git a/tree-diff.c b/tree-diff.c
> index 3c34c77..514dbca 100644
> --- a/tree-diff.c
> +++ b/tree-diff.c
> @@ -83,7 +83,7 @@ static int compare_tree_entry(struct tree_desc *t1,
> struct tree_desc *t2, const
>  }
>
>  /*
> - * Is a tree entry interesting given the pathspec we have?
> + * Is a tree entry interesting given the path prefix we have?

I believe the comment is parsed thus: "tree entry" == combination of
desc, base, and baselen.  "pathspec" == paths and pathlens fields of
opt (which do provide a pathspec).  So I believe the original was
correct, though I can see how it's confusing at first.

>  *
>  * Return:
>  *  - 2 for "yes, and all subsequent entries will be"
>
>
> This patch is good stuff. Please be informed I will steal this patch
> for my narrow use.

Thanks, but I just found a bug with it; I'm investigating.