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
