Re: [PATCH v4 3/3] diff --no-index: support limiting by pathspec
- From
Jacob Keller <jacob.e.keller@intel.com>
- Date
- Sep 24, 2025, 18:23 UTC
- Message-ID
- <c919ef8a-cf37-4ee7-9713-775246bc7243@intel.com>
- In-Reply-To
- <5da981d0-0a9c-4840-b413-54ec29d349e6@intel.com>
On 9/24/2025 11:19 AM, Jacob Keller wrote:
Show 28 quoted lines
> > > On 9/24/2025 4:19 AM, Johannes Schindelin wrote: >> Hi Jacob, >> >> On Tue, 23 Sep 2025, Jacob Keller wrote: >> >>> On 9/23/2025 7:57 AM, Johannes Schindelin wrote: >>> >>>> 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). >> >> I do wonder, after seeing that `read_directory_contents()` has to >> (re-)construct a complete `strbuf` in every single invocation whether it >> would make more sense to construct two `strbuf`s in `diff_no_index()` and >> pass those along to `queue_diff()` _instead_ of `skip1`/`skip2`. The >> `queue_diff()` function would then have to extend these >> `strbuf`s as it already does with `buffer1`/`buffer2`. >>
Ah, but I see what you meant better after some more thought. Instead of bothering with skip1 and skip2 at all, we just generate the sub part of the path along with buffer1/buffer2, but we start these new pathspec bits empty, so that we can construct the proper C/D/E path in read_directory_contents directly without needing to do a skip or memmove etc.
Makes sense. Good suggestion!
I'll try to work on this today and make a test case to confirm this.
Thanks, Jake