Respect the force flag on refspecs in git_remote_fetch #5854

alexjg · 2021-05-02T12:43:58Z

This fixes #2957

If the ->force flag on a refspec is not set then we check that the new refs are descendants of the current refs before updating them.

ethomson · 2021-05-06T00:53:32Z

@carlosmn any chance you can spare some 👀 for this? Your knowledge of the networking code is far superior to mine.

carlosmn · 2021-05-26T13:02:46Z

Oops I guess we've been behaving like everything is forced forever. I let the tests run and it looks like there's memory errors here where one of the variables isn't initialized. Running it locally it keeps failing at

remote::fetch::do_update_refs_if_not_descendant_and_force [/home/carlos/git/libgit2/tests/remote/fetch.c:75]

While the overall approach seems like it should work, there do seem to be some issues in the code that can be caught by the tests.

carlosmn · 2021-05-26T13:07:08Z

I suspect the error happens in the case where we first create the remote-tracking branch so we look it up but old is left unwritten-to. Making the graph check conditional on !error should solve this.

alexjg · 2021-05-27T12:56:48Z

I notice that the test I wrote is also failing

 1:   1) Failure:
1: remote::fetch::do_update_refs_if_not_descendant_and_force [../tests/remote/fetch.c:76]
1:   Function call failed: (git_commit_create(commit1id, repo1, REPO1_REFNAME, sig, sig, ((void *)0), "one", empty_tree, 0, ((void *)0)))
1:   error -15 - failed to create commit: current tip is not the first parent

This is strange as it passes on my machine. It looks like it's failing to clean up after the first test and so the test repo is in an unexpected state. Am I missing something on the cleanup?

carlosmn · 2021-05-27T18:53:13Z

It is most likely something to do with cleanup or the interaction between tests. Running just that failing test does pass.

carlosmn · 2021-05-27T18:58:06Z

When you create the new root commit the second time around, the branch has commits, so the empty parent list means a zero-commit as first parent, whereas the existing tip of the branch is whatever actual commit you created.

carlosmn · 2021-05-28T16:07:51Z

This might be the result of #5842 which is somewhat recent and might explain why you don't see it yourself if you branched off of something earlier.

alexjg · 2021-05-28T18:01:36Z

Ah interseting, I've rebased off main and now I get a test failure. Will see what I can do.

alexjg · 2021-05-28T20:28:36Z

I had to expose a cl_fs_rm function to directly remove the test repo in the cleanup code. The tests pass now but this may not be the idiomatic way to remote the repos?

alexjg · 2021-06-10T14:29:00Z

Hey peeps, any chance someone could re-run the tests for this?

carlosmn · 2021-06-11T08:31:21Z

Exposing internal clar functionality is not the way to go. Check out how other tests, e.g. the mkdir tests, deal with custom cleanup

libgit2/tests/core/mkdir.c

Lines 6 to 22 in 868f4bc

    
           static void cleanup_basic_dirs(void *ref) 
        
           { 
        
           	GIT_UNUSED(ref); 
        
           	git_futils_rmdir_r("d0", NULL, GIT_RMDIR_EMPTY_HIERARCHY); 
        
           	git_futils_rmdir_r("d1", NULL, GIT_RMDIR_EMPTY_HIERARCHY); 
        
           	git_futils_rmdir_r("d2", NULL, GIT_RMDIR_EMPTY_HIERARCHY); 
        
           	git_futils_rmdir_r("d3", NULL, GIT_RMDIR_EMPTY_HIERARCHY); 
        
           	git_futils_rmdir_r("d4", NULL, GIT_RMDIR_EMPTY_HIERARCHY); 
        
           } 
        
           void test_core_mkdir__absolute(void) 
        
           { 
        
           	git_buf path = GIT_BUF_INIT; 
        
           	cl_set_cleanup(cleanup_basic_dirs, NULL); 
        
           	git_buf_joinpath(&path, clar_sandbox_path(), "d0");

alexjg · 2021-06-11T17:04:52Z

Thanks for the hint! I've reworked this to use git_futils_rmdir_r

alexjg · 2021-06-19T12:44:12Z

Hey folks, sorry to bother, any chance someone can run the tests again?

tiennou · 2021-06-19T13:52:17Z

Sure, CI has been kicked into action.

alexjg · 2021-06-30T13:01:14Z

I managed to reproduce the memory sanitizer failure in a local version of the CI pipeline and I've fixed the failure there. This should be good to go now.

alexjg · 2021-07-12T18:01:12Z

Could someone nudge CI into action again?

alexjg · 2021-07-15T11:48:06Z

Looks like all the tests are passing, is there anything else to do to merge this?

ethomson · 2021-08-02T15:03:13Z

@carlosmn is this looking good?

carlosmn

The code itself seems fine, mostly just the tests should be closer to the patterns and utilities that we already use elsewhere.

The test functions themselves could be made smaller and the "time-traveling fetch" function could also perform the check but that's minor.

carlosmn · 2021-08-04T19:53:10Z

tests/remote/fetch.c

+		strncpy(result, path, strlen(path));
+		return result;
+	}
+}


This function seems unused.

🤦 removed

carlosmn · 2021-08-04T19:59:45Z

tests/remote/fetch.c

+
+	cl_git_pass(git_buf_join(&repo1_path_buf, '/', sandbox, "fetchtest_repo1"));
+	repo1_path = calloc(repo1_path_buf.size, sizeof(char));
+	git_buf_copy_cstr(repo1_path, repo1_path_buf.size, &repo1_path_buf);


This dance seems like it would be better by using git_buf_detach.

carlosmn · 2021-08-04T20:03:47Z

tests/remote/fetch.c

+	git_buf repo2_path_buf = GIT_BUF_INIT;
+	const char *sandbox = clar_sandbox_path();
+
+	cl_git_pass(git_buf_join(&repo1_path_buf, '/', sandbox, "fetchtest_repo1"));


git_buf_joinpath would be nicer here; it's what I was expecting and made me think you were trying to prepend slash :/

carlosmn · 2021-08-04T20:35:12Z

One bigger change and consequence of this is that the update_tips callback should also be called when it doesn't get updated so it's able to report that to the user, as I believe git also does.

alexjg · 2021-08-05T11:45:08Z

@carlosmn Thanks for the review, I believe I've addressed your comments. Regarding the larger change to update_tips, that would imply an extension of the callback interface which could be done in a subsequent PR right?

carlosmn · 2021-08-10T12:33:16Z

Regarding the larger change to update_tips, that would imply an extension of the callback interface which could be done in a subsequent PR right?

Yes, that would require increasing the version of the remote callback structure and adding some sort of update_tips_ext to include whether the update succeeded or not and why.

ethomson · 2021-08-10T14:06:38Z

Thanks for the change - and the great test @alexjg - and for the code review @carlosmn.

Fix fetching git repos after a force push. Users have been reporting that the index has not been updating for them. This was caused by the update to libgit2 1.3 (from 1.1) which has changed some behavior around force pushes. The index was squashed on 2021-09-24, and if a user had the index fetched from before that point, and they used nightly-2021-10-14 or newer, then the fetch would succeed, but the `refs/remotes/origin/HEAD` would not get updated. Cargo uses the `origin/HEAD` ref to know what to look at, and thus was looking at old data. The solution here is to use `+` on the refspec to force libgit2 to do a forced update (a fast-forward). I think this may have been introduced in libgit2 1.2 via libgit2/libgit2#5854, though that is just a guess. Fixes #9976

Fix fetching git repos after a force push. Users have been reporting that the index has not been updating for them. This was caused by the update to libgit2 1.3 (from 1.1) which has changed some behavior around force pushes. The index was squashed on 2021-09-24, and if a user had the index fetched from before that point, and they used nightly-2021-10-14 or newer, then the fetch would succeed, but the `refs/remotes/origin/HEAD` would not get updated. Cargo uses the `origin/HEAD` ref to know what to look at, and thus was looking at old data. The solution here is to use `+` on the refspec to force libgit2 to do a forced update (a fast-forward). I think this may have been introduced in libgit2 1.2 via libgit2/libgit2#5854, though that is just a guess. Fixes rust-lang#9976

xla mentioned this pull request May 6, 2021

net: run rere after serving the fetch radicle-dev/radicle-link#652

Merged

alexjg force-pushed the respect-force-flag-in-remote-fetch branch from 4e7d891 to 68aec6e Compare May 28, 2021 18:31

Respect the force flag on refspecs in git_remote_fetch

1713ab4

alexjg force-pushed the respect-force-flag-in-remote-fetch branch from 68aec6e to 1713ab4 Compare June 11, 2021 16:59

Merge branch 'libgit2:main' into respect-force-flag-in-remote-fetch

6c035b5

alexjg added 2 commits June 30, 2021 12:59

Fix memory sanitizer failures in tests

8ad5998

Merge branch 'main' into respect-force-flag-in-remote-fetch

39fd764

carlosmn previously approved these changes Aug 4, 2021

View reviewed changes

skip descendant check if remote ref not found

a569670

alexjg dismissed carlosmn’s stale review via a569670 August 5, 2021 11:43

alexjg force-pushed the respect-force-flag-in-remote-fetch branch from 4226929 to a569670 Compare August 5, 2021 11:43

carlosmn approved these changes Aug 10, 2021

View reviewed changes

ethomson merged commit 5fd4423 into libgit2:main Aug 10, 2021

ehuss mentioned this pull request Oct 18, 2021

Fix fetching git repos after a force push. rust-lang/cargo#9979

Merged

Respect the force flag on refspecs in git_remote_fetch #5854

Respect the force flag on refspecs in git_remote_fetch #5854

Uh oh!

Conversation

alexjg commented May 2, 2021

Uh oh!

ethomson commented May 6, 2021

Uh oh!

carlosmn commented May 26, 2021

Uh oh!

carlosmn commented May 26, 2021

Uh oh!

alexjg commented May 27, 2021

Uh oh!

carlosmn commented May 27, 2021

Uh oh!

carlosmn commented May 27, 2021

Uh oh!

carlosmn commented May 28, 2021

Uh oh!

alexjg commented May 28, 2021

Uh oh!

alexjg commented May 28, 2021

Uh oh!

alexjg commented Jun 10, 2021

Uh oh!

carlosmn commented Jun 11, 2021

Uh oh!

alexjg commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexjg commented Jun 19, 2021

Uh oh!

tiennou commented Jun 19, 2021

Uh oh!

alexjg commented Jun 30, 2021

Uh oh!

alexjg commented Jul 12, 2021

Uh oh!

alexjg commented Jul 15, 2021

Uh oh!

ethomson commented Aug 2, 2021

Uh oh!

carlosmn left a comment

Choose a reason for hiding this comment

Uh oh!

carlosmn Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

alexjg Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

carlosmn Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

alexjg Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

carlosmn Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

alexjg Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

carlosmn commented Aug 4, 2021

Uh oh!

alexjg commented Aug 5, 2021

Uh oh!

carlosmn commented Aug 10, 2021

Uh oh!

ethomson commented Aug 10, 2021

Uh oh!

Uh oh!

alexjg commented Jun 11, 2021 •

edited

Loading