From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Tue, 23 Sep 2025 14:57:18 GMT
Subject: Re: [PATCH v4 3/3] diff --no-index: support limiting by pathspec
Message-ID: <c75ec5f9-407a-6555-d4fb-bb629d54ec61@gmx.de>
In-Reply-To: <20250521232917.2333291-4-jacob.e.keller@intel.com>

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:

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?

> +		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.

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()`?

Ciao,
Johannes