Fix stash save bug with fast path index check #4668
Conversation
|
Of course, this fix is just a point fix, and it's possible that there's a cleaner way to solve this. But it does work. |
tests/stash/save.c
Outdated
| char* untracked_dir = malloc(len + 2); | ||
|
|
||
| strcpy(untracked_dir, workdir); | ||
| strcpy(untracked_dir + len, "z"); |
There was a problem hiding this comment.
I'd prefer to just use git_buf instead of pointer arithmetic. I mean this here is simple enough and unlikely to break, but git_buf_printf(&untracked_dir, "%sz", workdir) is more readable.
tests/stash/save.c
Outdated
|
|
||
| const char* workdir = git_repository_workdir(repo); | ||
| size_t len = strlen(workdir); | ||
| char* untracked_dir = malloc(len + 2); |
There was a problem hiding this comment.
Pointers belong with the variable :)
| @@ -273,7 +273,8 @@ static git_diff_delta *diff_delta__last_for_item( | |||
| break; | |||
| case GIT_DELTA_MODIFIED: | |||
| if (git_oid__cmp(&delta->old_file.id, &item->id) == 0 || | |||
| git_oid__cmp(&delta->new_file.id, &item->id) == 0) | |||
| (delta->new_file.mode == item->mode && | |||
There was a problem hiding this comment.
Do modes have to be completely the same? I get that it doesn't make sense in case one is a directory and the other is not. But what about when e.g. 644 changes to 755?
There was a problem hiding this comment.
I have to admit that I don't really understand this code. It seems completely weird to me that something gets added to the diff list and then later removed. But gdb shows the "mode" for a directory as 16384 -- that is, these are git modes, not filesystem modes. And looking at all three of the callers, they only happen in the case where one side of the comparison is a directory. So I don't think mode changes are relevant here. What do you think?
There was a problem hiding this comment.
16384 equals 0x4000, which indeed is the directory mode of git. But in case it is not a directory but a file, this mode will also mark whether the file is executable or not. And in that case I think that we shouldn't require those two modes to be equal, should we? That being said, I'm not really familiar with the diff generating code, either.
There was a problem hiding this comment.
Goodness, I don't know what this code is doing, either.
There was a problem hiding this comment.
If the question is "should X get removed from the list of diffs because it's actually identical to this other thing", I think a mode change should give the answer "no", because there is still a diff.
(But, again, there's no case where this is called on two files; one arg is always a directory)
If the index contains stat data for a modified file, and the file is not racily dirty, and there exists an untracked working tree directory alphabetically after that file, and there are no other changes to the repo, then git_stash_save would fail. It would confuse the untracked working tree directory for the modified file, because they have the same sha: zero. The wt directory has a sha of zero because it's a directory, and the file would have a zero sha because we wouldn't read the file -- we would just know that it doesn't match the index. To fix this confusion, we simply check mode as well as SHA.
|
Ok, I stepped through here a few times to convince myself that I understood what was going on again. Thanks for digging into this; our diff code is not trivial! :D |
If the index contains stat data for a modified file, and the file is
not racily dirty, and there exists an untracked working tree directory
alphabetically after that file, and there are no other changes to the
repo, then git_stash_save would fail. It would confuse the untracked
working tree directory for the modified file, because they have the
same sha: zero. The wt directory has a sha of zero because it's a
directory, and the file would have a zero sha because we wouldn't read
the file -- we would just know that it doesn't match the index. To
fix this confusion, we simply check mode as well as SHA.
Fixes #4101