On 9/23/2025 7:57 AM, Johannes Schindelin wrote:
> Hi Jacob,
> 
> I know this is about a patch that you contributed four months ago, and
> the usual feedback required sweeping changes, including this one that was
> introduced in v4:
> 

Hello,

(Replying from my work address since its easier today than trying to
load my personal email up).

It's been quite some time since I did this so my memory is a bit hazy on
the logic. It took me a while to get things working, and its very likely
I missed corner cases.

> On Wed, 21 May 2025, Jacob Keller wrote:
> 
>> diff --git a/diff-no-index.c b/diff-no-index.c
>> index 9739b2b268b9..4aeeb98cfa8f 100644
>> --- a/diff-no-index.c
>> +++ b/diff-no-index.c
>> @@ -15,20 +15,45 @@
>>  #include "gettext.h"
>>  #include "revision.h"
>>  #include "parse-options.h"
>> +#include "pathspec.h"
>>  #include "string-list.h"
>>  #include "dir.h"
>>  
>> -static int read_directory_contents(const char *path, struct string_list *list)
>> +static int read_directory_contents(const char *path, struct string_list *list,
>> +				   const struct pathspec *pathspec,
>> +				   int skip)
>>  {
>> +	struct strbuf match = STRBUF_INIT;
>> +	int len;
>>  	DIR *dir;
>>  	struct dirent *e;
>>  
>>  	if (!(dir = opendir(path)))
>>  		return error("Could not open directory %s", path);
>>  
>> -	while ((e = readdir_skip_dot_and_dotdot(dir)))
>> -		string_list_insert(list, e->d_name);
>> +	if (pathspec) {
>> +		strbuf_addstr(&match, path);
>> +		strbuf_complete(&match, '/');
>> +		strbuf_remove(&match, 0, skip);
> 
> Okay, so here the `read_directory_contents()` function learns to
> optionally skip `skip` bytes from the `path` variable, after potentially
> appending a `/`.
> 
>>  [...]
>> @@ -337,7 +369,23 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,
>>  		paths[i] = p;
>>  	}
>>  
>> -	fixup_paths(paths, &replacement);
>> +	if (fixup_paths(paths, &replacement)) {
>> +		parse_pathspec(&pathspec, PATHSPEC_FROMTOP | PATHSPEC_ATTR,
>> +			       PATHSPEC_PREFER_FULL | PATHSPEC_NO_REPOSITORY,
>> +			       NULL, &argv[2]);
>> +		if (pathspec.nr)
>> +			ps = &pathspec;
>> +
>> +		skip1 = strlen(paths[0]);
>> +		skip1 += paths[0][skip1] == '/' ? 0 : 1;
> 
> Since `skip1` is defined as the length of `path[0]`, I would expect
> `paths[0][skip1]` to always evaluate to NUL, and therefore the `== '/'`
> condition to always evaluate to `false`. Did I miss anything?
> 

You're correct that obviously makes no sense.. :D

>> +		skip2 = strlen(paths[1]);
>> +		skip2 += paths[1][skip2] == '/' ? 0 : 1;
> 
> Same here, `paths[1][skip2]` should always return `NUL`.
> 
> This has ramifications where `skip1` and `skip2` are each one larger than
> the length of `paths[0]` and `paths[1]`, respectively, and hence the code
> in `read_directory_contents()` will now try to remove one more than the
> length of the path, after potentially appending a slash.
> 
> But what if there is already a slash? The answer is:
> 
>   $ git diff --no-index -- /tmp/ /tmp/ ':!'
>   fatal: `pos + len' is too far after the end of the buffer
> 
> This has been reported (with Windows paths, don't let that distract) in
> https://github.com/git-for-windows/git/issues/5836.
> 
> I _think_ that what the patch should have done instead was:
> 
> 	if (skip1 > 0 && paths[0][skip1 - 1] == '/')
> 		skip1--;
> 
> and likewise
> 
> 	if (skip2 > 0 && paths[1][skip2 - 1] == '/')
> 		skip2--;
> 
> Focusing on the lines' correctness (which I don't think was the primary
> concern in the review of your patch), that would be what I would suggest.
> 

Perhaps. I need to dig into this code to see what the whole point was. I
think the idea is that I'm trying to skip past the common prefix?

> However, this makes me wonder whether the logic itself is sound? It is not
> immediately obvious to me why the `paths[0]` and `paths[1]` values aren't
> matched against the pathspec yet their entirety is seemingly skipped in
> `read_directory_contents()`?

I recall fiddling a lot to try and get this working. The idea here is
that fixup_paths does some conversions to handle the DWIM logic where a
"diff D F" becomes "diff D/F F". It returns true if both paths are
directories, so we only enter this block when both paths are
directories. (Which is required because we only support pathspec
limiting for directory differences).

We are calculating the skip length which is then passed into the
queue_diff function.

We pass the skip value into the queue_diff function, and this is the
length of the prefix of each path to skip. Essentially, we're skipping
everything from start of the path up to the root of what you pass into
the function.

The queue_diff then descends into each directory, and creates new paths
which are all the files and directories recursively under the target
directory. Each of these needs to have its root skipped (everything the
user supplied) in order to allow path spec matching to work, since we
apply pathspec matches as if you're at the root of the two trees being
diffed.

Crucially, we pass the skip value in to the first call of queue_diff,
but it remains unchanged as we descend further in, so we keep cutting
off only the first part of the path structure as we compare.

You're correct that the logic for calculating this incorrectly, as it
apparently doesn't work right if the paths end in a slash already. I'd
have to dig farther to figure out if this proposed patch is correct. I'm
not certain.

Basically, read_directory_contents is given the path to current
directory + the name of a file under the directory. We convert "path" to
be "dir/", then we remove the prefix of everything that was passed by
the user, so that we only check against parts of the dir path which are
recursively below the original passed directory. But we need to be
careful that we strip out the slash as well as the overall path, so that
we don't end up comparing subdirectories as if they're absolute paths.

Basically, if path does contain a slash, then the strlen alone is
sufficient, but if it doesn't contain a slash, then we need to avoid
adding 1. It happens that we incorrectly checked for this, which results
in us always adding 1, so when a string has a slash we skip too much.

It should be pretty easy to add a test case for this, and I think the
correct check is actually something like:

skip1 = strlen(paths[0])
if (!skip1 || paths[0][skip1 - 1] == '/')
  skip1++

Basically, we actually want one to be the length of the string plus a
trailing slash. If the string doesn't include a trailing slash, we have
to add 1. If it does include one, we don't add one, since the length
already accounts for the slash. Note that if the string is 0 length,
there's no slash so skip1 should be 1, hence the !skip1 part of the check.

Let me see if I can hook up a test case and confirm this.
>
> Ciao,
> Johannes