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. 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. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> > --- > unpack-trees.c | 45 ++++++++++++++++++++++++++------------------- > 1 file changed, 26 insertions(+), 19 deletions(-) > > diff --git a/unpack-trees.c b/unpack-trees.c > index 3a85a02a77..114496cfc2 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -18,6 +18,9 @@ > #include "fsmonitor.h" > #include "fetch-object.h" > > +/* Do not use the_index here, you probably want o->src_index */ > +#define the_index the_index_should_not_be_used here Good call.
