| Message ID | ae6fc699-6e09-2979-40dc-9cc49f4f8365@kdbg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | diff: don't attempt to strip prefix from absolute Windows paths | expand |
On Thu, Oct 18, 2018 at 11:38 AM Johannes Sixt <j6t@kdbg.org> wrote:
> There is one peculiarity, though: [...]
The explanation makes sense, and the code looks good.
Do we want to have a test for this niche case?
Am 18.10.18 um 20:49 schrieb Stefan Beller: > On Thu, Oct 18, 2018 at 11:38 AM Johannes Sixt <j6t@kdbg.org> wrote: > >> There is one peculiarity, though: [...] > > The explanation makes sense, and the code looks good. > Do we want to have a test for this niche case? > Good point. That would be the following. But give me a day or two to cross-check on Windows and whether it really catches the breakage. diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh index 453e6c35eb..6e0dd6f9e5 100755 --- a/t/t4053-diff-no-index.sh +++ b/t/t4053-diff-no-index.sh @@ -127,4 +127,14 @@ test_expect_success 'diff --no-index from repo subdir respects config (implicit) test_cmp expect actual.head ' +test_expect_success 'diff --no-index from repo subdir with absolute paths' ' + cat <<-EOF >expect && + 1 1 $(pwd)/non/git/{a => b} + EOF + test_expect_code 1 \ + git -C repo/sub diff --numstat \ + "$(pwd)/non/git/a" "$(pwd)/non/git/b" >actual && + test_cmp expect actual +' + test_done
Johannes Sixt <j6t@kdbg.org> writes: > Use is_absolute_path() to detect Windows style absolute paths. When cd676a51 ("diff --relative: output paths as relative to the current subdirectory", 2008-02-12) was done, neither "is_dir_sep()" nor "has_dos_drive_prefix()" existed---the latter had to wait until 25fe217b ("Windows: Treat Windows style path names.", 2008-03-05), but we didn't notice that the above code needs to use the Windows aware is_absolute_path() when we applied the change. > One might wonder whether the check for a directory separator that > is visible in the patch context should be changed from == '/' to > is_dir_sep() or not. It turns out not to be necessary. That code > only ever investigates paths that have undergone pathspec > normalization, after which there are only forward slashes even on > Windows. Thanks for carefully explaining. > static void strip_prefix(int prefix_length, const char **namep, const char **otherp) > { > /* Strip the prefix but do not molest /dev/null and absolute paths */ > - if (*namep && **namep != '/') { > + if (*namep && !is_absolute_path(*namep)) { > *namep += prefix_length; > if (**namep == '/') > ++*namep; > } > - if (*otherp && **otherp != '/') { > + if (*otherp && !is_absolute_path(*otherp)) { > *otherp += prefix_length; > if (**otherp == '/') > ++*otherp; When I read the initial report and guessed the root cause without looking at the code, I didn't expect the problematic area to be this isolated and the solution to be this simple. Nicely done.
diff --git a/diff.c b/diff.c index f0c7557b40..d18eb198f2 100644 --- a/diff.c +++ b/diff.c @@ -4267,12 +4267,12 @@ static void diff_fill_oid_info(struct diff_filespec *one) static void strip_prefix(int prefix_length, const char **namep, const char **otherp) { /* Strip the prefix but do not molest /dev/null and absolute paths */ - if (*namep && **namep != '/') { + if (*namep && !is_absolute_path(*namep)) { *namep += prefix_length; if (**namep == '/') ++*namep; } - if (*otherp && **otherp != '/') { + if (*otherp && !is_absolute_path(*otherp)) { *otherp += prefix_length; if (**otherp == '/') ++*otherp;
git diff can be invoked with absolute paths. Typically, this triggers the --no-index case. Then the absolute paths remain in the file names that are printed in the output. There is one peculiarity, though: When the command is invoked from a a sub-directory in a repository, then it is attempted to strip the sub-directory from the beginning of relative paths. Yet, to detect a relative path the code just checks for an initial forward slash. This mistakes a Windows style path like D:/base as a relative path and the output looks like this, for example: D:\test\one>git -P diff --numstat D:\base D:\diff 1 1 {ase => iff}/1.txt where the correct output should be D:\test\one>git -P diff --numstat D:\base D:\diff 1 1 D:/{base => diff}/1.txt If the sub-directory where 'git diff' is invoked is sufficiently deep that the prefix becomes longer than the path to be printed, then the subsequent code even accesses the paths out of bounds! Use is_absolute_path() to detect Windows style absolute paths. One might wonder whether the check for a directory separator that is visible in the patch context should be changed from == '/' to is_dir_sep() or not. It turns out not to be necessary. That code only ever investigates paths that have undergone pathspec normalization, after which there are only forward slashes even on Windows. Signed-off-by: Johannes Sixt <j6t@kdbg.org> --- diff.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)