Skip to content

Fix stash save bug with fast path index check #4668

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ethomson merged 1 commit into from
Jun 9, 2018
Merged

Fix stash save bug with fast path index check #4668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/diff_generate.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&

@pks-t pks-t Jun 6, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@novalis novalis Jun 6, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@pks-t pks-t Jun 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ethomson ethomson Jun 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Goodness, I don't know what this code is doing, either.

@novalis novalis Jun 7, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

git_oid__cmp(&delta->new_file.id, &item->id) == 0))
return delta;
break;
default:
Expand Down
40 changes: 40 additions & 0 deletions tests/stash/save.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,46 @@ void test_stash_save__can_include_untracked_and_ignored_files(void)
cl_assert(!git_path_exists("stash/just.ignore"));
}

/*
* Note: this test was flaky prior to fixing #4101 -- run it several
* times to get a failure. The issues is that whether the fast
* (stat-only) codepath is used inside stash's diff operation depends
* on whether files are "racily clean", and there doesn't seem to be
* an easy way to force the exact required state.
*/
void test_stash_save__untracked_regression(void)
{
git_checkout_options opts = GIT_CHECKOUT_OPTIONS_INIT;
const char *paths[] = {"what", "where", "how", "why"};
git_reference *head;
git_commit *head_commit;
git_buf untracked_dir;

const char* workdir = git_repository_workdir(repo);

git_buf_init(&untracked_dir, 0);
git_buf_printf(&untracked_dir, "%sz", workdir);

cl_assert(!p_mkdir(untracked_dir.ptr, 0777));

cl_git_pass(git_repository_head(&head, repo));

cl_git_pass(git_reference_peel((git_object **)&head_commit, head, GIT_OBJ_COMMIT));

opts.checkout_strategy = GIT_CHECKOUT_FORCE;

opts.paths.strings = (char **)paths;
opts.paths.count = 4;

cl_git_pass(git_checkout_tree(repo, (git_object*)head_commit, &opts));

cl_git_pass(git_stash_save(&stash_tip_oid, repo, signature, NULL, GIT_STASH_DEFAULT));

assert_commit_message_contains("refs/stash", "WIP on master");

git_buf_free(&untracked_dir);
}

#define MESSAGE "Look Ma! I'm on TV!"
void test_stash_save__can_accept_a_message(void)
{
Expand Down