Re: [PATCH] diff --no-index: fix logic for paths ending in '/'

[Junio C Hamano <gitster@pobox.com>]

Jacob Keller <jacob.e.keller@intel.com> writes:

> If one of the two provided paths for git diff --no-index ends in a '/',
> a failure similar to the following occurs:
>
>   $ git diff --no-index -- /tmp/ /tmp/ ':!'
>   fatal: `pos + len' is too far after the end of the buffer
>
> This occurs because of an incorrect calculation of the skip lengths in
> diff_no_index(). The code wants to calculate the length of the string,
> but add one in case the string doesn't end with a slash.
>
> The method it uses is incorrect, as it always checks the trailing NUL
> character of the string. This will never be a '/', so we always add one.
> In the event that we *do* have a trailing slash, this will create an
> off-by-one length error later when using the skip value.
>
> The most straightforward fix would be to correct the skip1 and skip2
> lengths by using ends_with().

Meaning, "ah, this one ends with '/' so let's trim it and do
everything else the same as before"?

It certainly is how we usually handle regression fixes.

There were two topics that touched "git diff --no-index" in Git 2.51
timeframe, and the pathspec support was a new feature added by them,
so this is not exactly a regression.

> This fix might feel overly complex. We can drop this and just go with a
> simple ends_with() fix, but that leaves the needless strbuf_remove() in the
> read_directory_contents.

We can do it as a two-step patch series, which would allow us to
revert the more complex part relatively cleanly if it turns out to
break the cases that the current code handles fine.  But as this is
a new feature in 2.51, it is OK to declare that the code never
worked correctly and we are making it right this time with a single
more ambitious patch.

Will queue.  Thanks, both of you.