Re: [PATCH/RFC/BUG] unpack-trees.c: do not use "the_index"

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jun 1, 2018 at 8:34 PM, Elijah Newren <newren@xxxxxxxxx> wrote:
> Hi,
>
> On Fri, Jun 1, 2018 at 9:11 AM, Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> wrote:
>> unpack-trees code works on multiple indexes specified in
>> unpack_trees_options. Although they normally all refer to the_index at
>> the call site, that is the caller's business. unpack-trees.c should
>> not make any assumption about that and should use the correct index
>> field in unpack_trees_options.
>>
>> This patch is actually confusing because sometimes the function
>> parameter is also named "the_index" while some other times "the_index"
>> is the global variable because the function just does not have a
>> parameter of the same name! The only subtle difference is that the
>> function parameter is a pointer while the global one is not.
>>
>> This is more of a bug report than an actual fix because I'm not sure
>> if "o->src_index" is always the correct answer instead of "the_index"
>> here. But this is very similar to 7db118303a (unpack_trees: fix
>> breakage when o->src_index != o->dst_index - 2018-04-23) and could
>> potentially break things again...
>
> Actually, I don't think the patch will break anything in the current
> code.  Currently, all callers of unpack_trees() (even merge recursive
> which uses different src_index and dst_index now) set src_index =
> &the_index.  So, these changes should continue to work as before (with
> a minor possible exception of merge-recursive calling into other
> functions from unpack-trees.c after unpack_trees() has finished..).
> That's not to say that your patch is bug free, just that I think any
> bugs shouldn't be triggerable from the current codebase.

Ah.. I thought merge-recursive would do fancier things and used some
temporary index. Good to know.

> Also, if any of the changes you made are wrong, what was there before
> was also clearly wrong.  So I think we're at least no worse off.
>
> But, I agree, it's not easy to verify what the correct thing should be
> in all cases.  I'll try to take a closer look in the next couple days.

Thanks. I will also stare at this code some more in the next couple
days trying to remember what these functions do.
-- 
Duy




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux