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

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

 



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.




[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