merge: reload index before git_merge by Etiene · Pull Request #4407 · libgit2/libgit2

Etiene · 2017-11-11T17:50:44Z

When the index in memory diverges from the index in disk git_merge
aborts with GIT_ECONFLICT. More details about this are found in the
issue #4203, which this PR attempts to fix.

A further discussion is needed wether this behaviour is actually desired.
(Suppose someone actually is writing something to the index in memory
and a concurrent task is merging. A possible argument is that the index
should be written to disk then.)

Closes #4203

pks-t

Some stylistic nits. I won't comment about that regarding whats the right thing to do here before having a look at it with fresh eyes tomorrow.

pks-t · 2017-11-11T17:54:06Z


 	assert(repo && their_heads);

+	if((error = git_repository_index(&index, repo) < 0) ||


Please put a whitespace between if and (.

Furthermore, it would be nice to actually have a giterr_set() here describing why we failed. Furthermore, you'd have to call git_index_free on index.

Actually, I think that the error message set by git_repository_index itself would be superior to changing the error message. It will have the OS error details, like permission denied, which we would not want to overwrite with a less descriptive error.

pks-t · 2017-11-11T17:59:00Z

-	git_repository_index(&index, repo);
-	git_index_read(index, 0);
+	if((error = git_repository_index(&index, repo) < 0) ||
+		(error = git_index_read(index, 0) < 0))


I think we usually indent continuation lines with four spaces from the originial start of the conditional, such that we can easily distinguish it from the first statement inside of that condition.

pks-t · 2017-11-11T18:00:08Z

+	git_annotated_commit *their_heads[1];
+	git_oid their_oid;
+
+	git_index_entry entry = {{0}};


We usually don't have newlines in the list of variable declarations.

ethomson · 2017-11-11T18:14:59Z

One note: @pks-t and I have been discussing correct behavior here and whether we should always reload the index from disk. (Like checkout does.)

It's worth noting that merge's behavior is to create a new index (that represents the merge results) and check that out, setting it as the new index.

During discussion, we were concerned that somebody may want to git_index_add an entry and expect it to be preserved even though they had not done a git_index_write. This change might remove that functionality.

After further thoughts, it does not. AFAICT, there is no case where somebody could git_index_add something, and not write the changes to the index, and have those preserved. That's because the on-disk index is overwritten with the merge results index. So unsaved index changes are lost (today) and this does nothing to change that behavior one way or another.

ethomson · 2017-11-11T18:17:35Z


 	assert(repo && their_heads);

+	if((error = git_repository_index(&index, repo) < 0) ||


This actually needs to move down in the code a bit, write after git_indexwriter_init_for_operation.

git_indexwriter will take set up the lockfile on the index. We should re-read the index under that lock - if we do not, there's still a race condition where we read the index, and somebody makes changes before we take the lock.

pks-t · 2017-11-11T19:05:05Z

@ethomson Unfortunately, I don't think that's actually true. First, there's the GIT_CHECKOUT_DONT_WRITE_INDEX flag, which gets evaluated by the index writer. Second, it's not really about the writing but about the reading of the index from disk. If we have a modified index, the indexwriter will simply use that, and necessarily so. If there are changes which are not written to disk, then currently the index writer will not re-read that from the index but will simply do a git_repository_index__weakptr and get the currently modified one.

So re-reading the index from disk does change behaviour in some cases. Whether those cases are cases are sane or not is an entirely different topic, though.

If the index in memory is different from the index on the disk, previously merge would abort with GIT_ECONFLICT. Reload the index before merging to fix this. Fixes libgit2#4203

Adds a test case for the issue libgit2#4203, when diverging indexes on memory and disk cause git merge to abort with GIT_ECONFLICT

Cleans up should git_repository_index or git_index_read fail

Etiene · 2017-11-11T19:45:32Z

@pks-t please re-review the code style? :)

@ethomson is this where you wanted it moved? :)

Concerning the desired behaviour, let me know when the council has made a decision ;)

ethomson · 2017-11-11T20:06:30Z

So re-reading the index from disk does change behaviour in some cases. Whether those cases are cases are sane or not is an entirely different topic, though.

Correct. I was totally mistaken about that.

I'm going to introduce a test during index refresh paths that ensures that we do not have unwritten index entries (ie someone has called git_index_add but not git_index_write) to ensure that we do not lose their changes and can refresh safely.

Etiene · 2017-11-11T20:18:45Z

Summary of future improvements, then, based on IRL discussion:

erring on dirty index
still reloading on stale

ethomson · 2017-11-17T00:33:41Z

I've been working on adding a guard to the index so that you can't run a command that reloads the index when the index is "dirty" - which is to say that you've made changes via git_index_add and friends but you haven't saved those changes (with git_index_write) or aborted them (with git_index_read).

Regrettably, we have a bunch of tests that believe that they can work in the old way and not flush the index to disk. Sensibly, because why should they? That would just slow our test runs down.

I've mostly got everything fixed here, but there are still a few tests failing. I'm chipping away at these, and once that's done, I think we'll be good to merge this.

Etiene · 2017-11-17T12:24:12Z

@ethomson wonderful! Do you need help in that?

ethomson · 2018-06-30T15:11:59Z

Now that #4536 has landed, I think we can move forward with this. I have one code review point. And we should add some data to the changelog, something like:

Merge now reloads the index before proceeding, to ensure that it has the latest changes that were made by other clients. In the unlikely event that you have made changes to the index and not written them before calling git_merge, those changes may now be lost. You should ensure that your changes are saved by calling git_index_write. If you would like an error in the case where your index entries would be overwritten, you can set the GIT_OPT_ENABLE_UNSAVED_INDEX_SAFETY option.

ethomson · 2018-06-30T15:13:37Z

 		goto done;

+	if ((error = git_repository_index(&index, repo) < 0) ||
+	    (error = git_index_read(index, 0) < 0))


We're leaking this index now. I would encourage us to use a separate variable here, eg:

if ((error = git_repository_index(&repo_index, repo) < 0) || (error = git_index_read(repo_index, 0) < 0))

And be sure to git_index_free this repo_index.

ethomson · 2018-10-19T10:34:28Z

@Etiene do you have interest in finishing this up? Or would you like me to fix those two last little bits?

ethomson · 2018-10-20T19:20:17Z

I added a commit to fix the memory leak and merged this manually. Thanks again for doing this, @Etiene!

pks-t requested changes Nov 11, 2017

View reviewed changes

ethomson reviewed Nov 11, 2017

View reviewed changes

GC8 and others added 2 commits November 11, 2017 19:31

merge: reload index before git_merge

bb9e379

If the index in memory is different from the index on the disk, previously merge would abort with GIT_ECONFLICT. Reload the index before merging to fix this. Fixes libgit2#4203

tests: add test case for index reloads on merge

5248a1a

Adds a test case for the issue libgit2#4203, when diverging indexes on memory and disk cause git merge to abort with GIT_ECONFLICT

Etiene force-pushed the issue-4203 branch from b2040de to 650d51d Compare November 11, 2017 19:32

merge: add error handling for index reload

e8d373c

Cleans up should git_repository_index or git_index_read fail

Etiene force-pushed the issue-4203 branch from 650d51d to e8d373c Compare November 11, 2017 19:44

pks-t added the bloomberg label Nov 12, 2017

ethomson mentioned this pull request Feb 19, 2018

Add a "dirty" state to the index when it has unsaved changes #4536

Merged

ethomson reviewed Jun 30, 2018

View reviewed changes

ethomson merged commit e8d373c into libgit2:master Oct 20, 2018


		assert(repo && their_heads);

		if((error = git_repository_index(&index, repo) < 0) \|\|

Uh oh!

Conversation

Etiene commented Nov 11, 2017

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

pks-t Nov 11, 2017

Choose a reason for hiding this comment

Uh oh!

pks-t Nov 11, 2017

Choose a reason for hiding this comment

Uh oh!

ethomson Nov 11, 2017

Choose a reason for hiding this comment

Uh oh!

pks-t Nov 11, 2017

Choose a reason for hiding this comment

Uh oh!

pks-t Nov 11, 2017

Choose a reason for hiding this comment

Uh oh!

ethomson commented Nov 11, 2017

Uh oh!

ethomson Nov 11, 2017

Choose a reason for hiding this comment

Uh oh!

pks-t commented Nov 11, 2017

Uh oh!

Etiene commented Nov 11, 2017

Uh oh!

ethomson commented Nov 11, 2017

Uh oh!

Etiene commented Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ethomson commented Nov 17, 2017

Uh oh!

Etiene commented Nov 17, 2017

Uh oh!

ethomson commented Jun 30, 2018

Uh oh!

ethomson Jun 30, 2018

Choose a reason for hiding this comment

Uh oh!

ethomson commented Oct 19, 2018

Uh oh!

ethomson commented Oct 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Etiene commented Nov 11, 2017 •

edited

Loading