From: Jacob Keller Date: Wed, 24 Sep 2025 18:23:42 GMT Subject: Re: [PATCH v4 3/3] diff --no-index: support limiting by pathspec Message-ID: In-Reply-To: <5da981d0-0a9c-4840-b413-54ec29d349e6@intel.com> On 9/24/2025 11:19 AM, Jacob Keller wrote: > > > 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