@matheusbsbs / matheus-bagliano / commits / c0bec06cfe

diff --no-index: fix logic for paths ending in '/'

If one of the two provided paths for git diff --no-index ends in a '/', a failure similar to the following occurs: $ git diff --no-index -- /tmp/ /tmp/ ':!' fatal: `pos + len' is too far after the end of the buffer This occurs because of an incorrect calculation of the skip lengths in diff_no_index(). The code wants to calculate the length of the string, but add one in case the string doesn't end with a slash. The method it uses is incorrect, as it always checks the trailing NUL character of the string. This will never be a '/', so we always add one. In the event that we *do* have a trailing slash, this will create an off-by-one length error later when using the skip value. The most straightforward fix would be to correct the skip1 and skip2 lengths by using ends_with(). However, Johannes made a good point that the existing logic is wasting a lot of computation. We generate the match string by copying the path in and then skipping almost all of it immediately with a potentially expensive memmove() from the strbuf_remove() call. We also re-initialize the match stringbuf each time we call read_directory_contents. The read_directory_contents really wants a path that is rooted at the start of the directory scan. We're currently building this by taking the full path and stripping out the start portion. Instead, replace this logic by building up the portion of the match as we go. Start by initializing two strbuf in diff_no_index containing the empty string. Pass these into queue_diff, which in turn passes the appropriate left or right side into read_directory_contents. As before, we build up the matches by appending elements to the match path and then clearing them using strbuf_setlen. In the recursive portion of the queue_diff algorithm, we build up new match paths the same way that we build up new buffer paths, by appending the elements and then clearing them with strbuf_setlen after each iteration. This is cheaper as it avoids repeated allocations, and is a bit simpler to track what is going on. Add a couple of test cases that pass in paths already ending in '/', to ensure the tests cover this regression. Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Closes: https://lore.kernel.org/git/c75ec5f9-407a-6555-d4fb-bb629d54ec61@gmx.de/ Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> [jc: small leakfixes at the end of diff_no_index()] Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jacob Keller committed Sep 24, 2025 at 13:57 UTC c0bec06cfedb930ecb3cd8c8cdb0f3e14e5e9308

2 files changed +51 -28

diff-no-index.c

+35 -28

@@ -21,30 +21,21 @@

22 static int read_directory_contents(const char *path, struct string_list *list,

23 const struct pathspec *pathspec,

24 - int skip)

24 + struct strbuf *match)

25 {

26 - struct strbuf match = STRBUF_INIT;

27 - int len;

26 + int len = match->len;

27 DIR *dir;

28 struct dirent *e;

30 if (!(dir = opendir(path)))

31 return error("Could not open directory %s", path);

34 - if (pathspec) {

35 - strbuf_addstr(&match, path);

36 - strbuf_complete(&match, '/');

37 - strbuf_remove(&match, 0, skip);

38 -

39 - len = match.len;

40 - }

41 -

33 while ((e = readdir_skip_dot_and_dotdot(dir))) {

34 if (pathspec) {

35 int is_dir = 0;

46 - strbuf_setlen(&match, len);

47 - strbuf_addstr(&match, e->d_name);

37 + strbuf_setlen(match, len);

38 + strbuf_addstr(match, e->d_name);

39 if (NOT_CONSTANT(DTYPE(e)) != DT_UNKNOWN) {

40 is_dir = (DTYPE(e) == DT_DIR);

41 } else {

@@ -57,7 +48,7 @@ static int read_directory_contents(const char *path, struct string_list *list,

48 }

50 if (!match_leading_pathspec(NULL, pathspec,

60 - match.buf, match.len,

51 + match->buf, match->len,

52 0, NULL, is_dir))

53 continue;

54 }

@@ -65,7 +56,7 @@ static int read_directory_contents(const char *path, struct string_list *list,

56 string_list_insert(list, e->d_name);

57 }

68 - strbuf_release(&match);

59 + strbuf_setlen(match, len);

60 closedir(dir);

61 return 0;

62 }

@@ -169,7 +160,8 @@ static struct diff_filespec *noindex_filespec(const struct git_hash_algo *algop,

160

161 static int queue_diff(struct diff_options *o, const struct git_hash_algo *algop,

162 const char *name1, const char *name2, int recursing,

172 - const struct pathspec *ps, int skip1, int skip2)

163 + const struct pathspec *ps,

164 + struct strbuf *ps_match1, struct strbuf *ps_match2)

165 {

166 int mode1 = 0, mode2 = 0;

167 enum special special1 = SPECIAL_NONE, special2 = SPECIAL_NONE;

@@ -208,10 +200,12 @@ static int queue_diff(struct diff_options *o, const struct git_hash_algo *algop,

200 struct string_list p2 = STRING_LIST_INIT_DUP;

201 int i1, i2, ret = 0;

202 size_t len1 = 0, len2 = 0;

203 + size_t match1_len = ps_match1->len;

204 + size_t match2_len = ps_match2->len;

205

212 - if (name1 && read_directory_contents(name1, &p1, ps, skip1))

206 + if (name1 && read_directory_contents(name1, &p1, ps, ps_match1))

207 return -1;

214 - if (name2 && read_directory_contents(name2, &p2, ps, skip2)) {

208 + if (name2 && read_directory_contents(name2, &p2, ps, ps_match2)) {

209 string_list_clear(&p1, 0);

210 return -1;

211 }

@@ -235,6 +229,11 @@ static int queue_diff(struct diff_options *o, const struct git_hash_algo *algop,

229 strbuf_setlen(&buffer1, len1);

230 strbuf_setlen(&buffer2, len2);

231

232 + if (ps) {

233 + strbuf_setlen(ps_match1, match1_len);

234 + strbuf_setlen(ps_match2, match2_len);

235 + }

236 +

237 if (i1 == p1.nr)

238 comp = 1;

239 else if (i2 == p2.nr)

@@ -245,18 +244,28 @@ static int queue_diff(struct diff_options *o, const struct git_hash_algo *algop,

244 if (comp > 0)

245 n1 = NULL;

246 else {

248 - strbuf_addstr(&buffer1, p1.items[i1++].string);

247 + strbuf_addstr(&buffer1, p1.items[i1].string);

248 + if (ps) {

249 + strbuf_addstr(ps_match1, p1.items[i1].string);

250 + strbuf_complete(ps_match1, '/');

251 + }

252 n1 = buffer1.buf;

253 + i1++;

254 }

255

256 if (comp < 0)

257 n2 = NULL;

258 else {

255 - strbuf_addstr(&buffer2, p2.items[i2++].string);

259 + strbuf_addstr(&buffer2, p2.items[i2].string);

260 + if (ps) {

261 + strbuf_addstr(ps_match2, p2.items[i2].string);

262 + strbuf_complete(ps_match2, '/');

263 + }

264 n2 = buffer2.buf;

265 + i2++;

266 }

267

259 - ret = queue_diff(o, algop, n1, n2, 1, ps, skip1, skip2);

268 + ret = queue_diff(o, algop, n1, n2, 1, ps, ps_match1, ps_match2);

269 }

270 string_list_clear(&p1, 0);

271 string_list_clear(&p2, 0);

@@ -346,7 +355,8 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,

355 int implicit_no_index, int argc, const char **argv)

356 {

357 struct pathspec pathspec, *ps = NULL;

349 - int i, no_index, skip1 = 0, skip2 = 0;

358 + struct strbuf ps_match1 = STRBUF_INIT, ps_match2 = STRBUF_INIT;

359 + int i, no_index;

360 int ret = 1;

361 const char *paths[2];

362 char *to_free[ARRAY_SIZE(paths)] = { 0 };

@@ -387,11 +397,6 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,

397 NULL, &argv[2]);

398 if (pathspec.nr)

399 ps = &pathspec;

390 -

391 - skip1 = strlen(paths[0]);

392 - skip1 += paths[0][skip1] == '/' ? 0 : 1;

393 - skip2 = strlen(paths[1]);

394 - skip2 += paths[1][skip2] == '/' ? 0 : 1;

400 } else if (argc > 2) {

401 warning(_("Limiting comparison with pathspecs is only "

402 "supported if both paths are directories."));

@@ -415,7 +420,7 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,

420 revs->diffopt.flags.exit_with_status = 1;

421

422 if (queue_diff(&revs->diffopt, algop, paths[0], paths[1], 0, ps,

418 - skip1, skip2))

423 + &ps_match1, &ps_match2))

424 goto out;

425 diff_set_mnemonic_prefix(&revs->diffopt, "1/", "2/");

426 diffcore_std(&revs->diffopt);

@@ -431,6 +436,8 @@ out:

436 for (i = 0; i < ARRAY_SIZE(to_free); i++)

437 free(to_free[i]);

438 strbuf_release(&replacement);

439 + strbuf_release(&ps_match1);

440 + strbuf_release(&ps_match2);

441 if (ps)

442 clear_pathspec(ps);

443 return ret;

t/t4053-diff-no-index.sh

+16

@@ -322,6 +322,22 @@ test_expect_success 'diff --no-index with pathspec' '

322 test_cmp expect actual

323 '

324

325 +test_expect_success 'diff --no-index first path ending in slash with pathspec' '

326 + test_expect_code 1 git diff --name-status --no-index a/ b 1 >actual &&

327 + cat >expect <<-EOF &&

328 + D a/1

329 + EOF

330 + test_cmp expect actual

331 +'

332 +

333 +test_expect_success 'diff --no-index second path ending in slash with pathspec' '

334 + test_expect_code 1 git diff --name-status --no-index a b/ 1 >actual &&

335 + cat >expect <<-EOF &&

336 + D a/1

337 + EOF

338 + test_cmp expect actual

339 +'

340 +

341 test_expect_success 'diff --no-index with pathspec no matches' '

342 test_expect_code 0 git diff --name-status --no-index a b missing

343 '