Re: [PATCH 5/5] diff: handle NULs in get_string_hash()

From: Jeff King <peff@peff.net>
Date: Oct 19, 2017, 21:39 UTC
Message-ID: <20171019213958.d6rp2upiga5tsvys@sigill.intra.peff.net>
In-Reply-To: <CAGZ79kY3jVgBRCKkswHV7xbjiiNgvsbVsgq_9PpVwa-zTt4q0Q@mail.gmail.com>

On Thu, Oct 19, 2017 at 02:31:20PM -0700, Stefan Beller wrote:

Show 27 quoted lines

> > This is unlikely to ever come up in practice since our line
> > boundaries generally come from calling strlen() in the first
> > place.
> 
> get_string_hash is called from
>  prepare_entry, which in turn is called from
>   add_lines_to_move_detection or mark_color_as_moved
>    diff_flush_patch_all_file_pairs
> 
> that constructs the arguments in
> diff_flush_patch
>  run_diff
>   run_diff_cmd
>    builtin_diff (part "/* Crazy xdl interfaces.. */")
>     xdi_diff_outf( fn_out_consume as arg!)
>      xdi_diff
>       xdl_diff
>        xdl_call_hunk_func
>         -> fn_out_consume(cb, line, len)
> 
> xdl_call_hunk_func however uses pointer arithmetic instead
> of strlen. So I think this sentence is not a good idea to put in
> the commit message.
> 
> It may not occur in practice, due to binary files detection using
> NUL as a signal, but conceptually our move-colored(!) diffs
> should be compatible with NULs with this patch now.

Good catch. I just skimmed over all the diff_emit_* functions, which use strlen(). But at the bottom is emit_add_line(), which has a real length marker from xdiff.

I agree we wouldn't see NULs in general, but this is maybe fixing "diff --color-moved -a". I dunno. It's probably not worth digging too much, since it seems like the right thing to do regardless.

-Peff