smart: use push_glob instead of manual filtering #5195

tiennou · 2019-07-31T06:42:53Z

The code worked under the assumption that anything under refs/tags are tag objects, and all the rest would be peelable to a commit. As it is completely valid to have tags to blobs under a non refs/tags ref, this would cause failures when trying to peel a tag to a commit (#3595).

Fix the broken filtering by switching to git_revwalk_push_glob, which already handles this case.

Fixes #3595, supersedes #4383, #5193.

ethomson · 2019-07-31T09:00:14Z

src/transports/smart_protocol.c

@@ -358,7 +314,10 @@ int git_smart__negotiate_fetch(git_transport *transport, git_repository *repo, c
 	if ((error = git_pkt_buffer_wants(wants, count, &t->caps, &data)) < 0)
 		return error;

-	if ((error = fetch_setup_walk(&walk, repo)) < 0)
+	if ((error = git_revwalk_new(&walk, repo)) < 0)
+		return error;


goto on_error to clean up data?

If you felt like it, making data the first param to git_pkt_buffer_wants might help in understanding that it's an out param...

Fixed.

This is in the vicinity of #5105, hence I didn't want more extensive changes here. If that's fine with you I'll tackle that out there.

pks-t · 2019-08-01T09:26:13Z

tests/refs/foreachglob.c

-	/* 12 heads (including one packed head) + 1 note + 2 remotes + 7 tags */
-	assert_retrieval("*", 22);
+	/* 12 heads (including one packed head) + 1 note + 2 remotes + 7 tags + 1 blob */
+	assert_retrieval("*", 23);


Please excuse my ignorance, but I really don't get why there's any changes to either refs::foreachglob or refs::iterator. As far as I can see, both these test suites act on a repository that has been opened via git_repository_open. No cloning took place and thus no negotiation via smart protocol. But the code changes you do is only in the smart protocol, so I'm wondering why values in either of those test suites should have changed at all?

Good question - I agree. I think it was a cherry-pick from that other PR, and that introduced a new ref? I think that this doesn't fit here anymore.

Note that I got a green build locally, and was surprised to have it come back red on CI (looking into it). AFAICS this test only adds one of those weird tags-to-non-commits, thus ensuring we correctly ignore it in whatever place we do not (😉) ; hence I'd vote to keep it. I can try adding a "clone this repo" in another testcase to exercise the actual fix to the smart transport.

But there is no file added to any test repo, so the reference counts couldn't have changed.

Right. Since git looks for the presence of file named HEAD to determine whether a directory is a repository or not, our test resources always appear as actual repositories. So you've probably still got a dirty test directory (with those refs) but you don't know it until you actually look in there.

This has bothered me for some time.

Maybe we should treat HEADs like .git files/folders and rename them during test resource copying, to prevent this problem.

That was it, sorry about that. GitX sometimes eats add entries when building amend indexes, and it slipped out as an untracked file. This should be good now.

Nothing to be sorry about! :)

tiennou · 2019-08-01T10:19:16Z

/rebuild

libgit2-azure-pipelines · 2019-08-01T10:19:33Z

Okay, @tiennou, I started to rebuild this pull request as build #2483.

pks-t · 2019-08-01T12:02:13Z

src/transports/smart_protocol.c

-	if ((error = git_revwalk_new(&walk, repo)) < 0)
-		return error;
-
-	git_revwalk_sorting(walk, GIT_SORT_TIME);


By the way, one more question: previously, we've been sorting by time, now we aren't. While obvious that this will greatly improve performance as we now don't have to go through all commits, it's not immediately obvious to me why this is fine and didn't find anything in the commit message.

True, I took @carlosmn's comment about blanket-replacing at face-value 😉. Checking around, pack-protocol seems to imply there's no expected order so this seems fine — though a second opinion would be welcome.

Diving into the code a bit more. Previously, we've sorted all commits by date, which would cause us to compare the newest 256 commits present in our own repos with the HAVEs of the remote. Now with push_glob, we're pushing the commits in an unspecified order that is completely independent of the date. If one has a repo with more than 256 refs, then it may happen that we now compare 256 old commits with the HAVEs. Heuristically, it's obvious that the first variant which adds commits by date may be a lot more efficient in the common case.

This analysis neglects that previously we had to load all commits of the repo to properly sort them by time, which was quite inefficient, too. I think we can do better than that, though: if push_glob were to use git_commit_list_insert_by_date, then we'd avoid inserting commits in a suboptimal order but we still wouldn't suck in all the repo's commit objects.

So I'd propose to modify push_glob to accept another parameter that determines how to sort pushed commits. push_glob will pass than on to push_commit, which will either use git_commit_list_insert or git_commit_list_insert_by_date.

Thanks for the in-depth analysis @pks-t. I've amended the PR by API-fying a bit more (DRYing all the functions), so that the revwalk could be tweaked accordingly. Not sure if it's worth making this API public instead of the current one — no idea how useful it would be to "outsiders" — so it's a little rough around the edges (a single flags might have been sufficient).

(though re-reading I'm now unsure if the insertion was to happen from an "initially sorted revwalk", as previously, or if it's okay with just the insertion being done by commit date. Right now the latter is done).

biodranik · 2019-08-07T22:16:52Z

Tested this change, it works fine. LGTM.

The code worked under the assumption that anything under `refs/tags` are tag objects, and all the rest would be peelable to a commit. As it is completely valid to have tags to blobs under a non `refs/tags` ref, this would cause failures when trying to peel a tag to a commit. Fix the broken filtering by switching to `git_revwalk_push_glob`, which already handles this case.

src/revwalk.h

src/revwalk.c

Before we can tweak the revwalk to be more efficent when negotiating, we need to add an "insertion mode" option. Since there's already an implicit set of those, make it visible, at least privately.

pks-t

Thanks a lot, @tiennou! One more thing about default options, otherwise I'm happy!

pks-t · 2019-08-27T06:27:42Z

src/revwalk.c

@@ -38,7 +38,7 @@ git_commit_list_node *git_revwalk__commit_lookup(
 	return commit;
 }

-static int push_commit(git_revwalk *walk, const git_oid *oid, int uninteresting, int from_glob)
+int git_revwalk__push_commit(git_revwalk *walk, const git_oid *oid, const git_revwalk__push_options *opts)


We should probably fall back to default options if opts == NULL?

I wanted the argument to be mandatory, mostly because the "mode" kinda is (OID vs glob, else it's ambiguous what should happen w.r.t to peeling). But I forgot to assert that though 😉.

Fair enough, it's internal anyway so all callers can be checked trivially. Let's just go ahead

pks-t · 2019-09-13T08:32:02Z

Thanks a lot, @tiennou!

tiennou mentioned this pull request Jul 31, 2019

transports: Avoid walking over non-commit objects #5193

Closed

ethomson reviewed Jul 31, 2019

View reviewed changes

ethomson mentioned this pull request Jul 31, 2019

transports: Avoid walking over non-commit objects #4383

Closed

tiennou force-pushed the fix/commitish-smart-push branch from 045b7b6 to 13dabf3 Compare July 31, 2019 09:07

pks-t reviewed Aug 1, 2019

View reviewed changes

tiennou force-pushed the fix/commitish-smart-push branch from 13dabf3 to e0f3c95 Compare August 1, 2019 11:13

pks-t reviewed Aug 1, 2019

View reviewed changes

tiennou force-pushed the fix/commitish-smart-push branch from e0f3c95 to 74512a1 Compare August 21, 2019 17:49

pks-t requested changes Aug 23, 2019

View reviewed changes

src/revwalk.h Outdated Show resolved Hide resolved

src/revwalk.c Outdated Show resolved Hide resolved

src/revwalk.c Outdated Show resolved Hide resolved

tiennou force-pushed the fix/commitish-smart-push branch from 74512a1 to a71000d Compare August 23, 2019 13:01

revwalk: expose more ways of scheduling commits

4b91f05

Before we can tweak the revwalk to be more efficent when negotiating, we need to add an "insertion mode" option. Since there's already an implicit set of those, make it visible, at least privately.

tiennou force-pushed the fix/commitish-smart-push branch from a71000d to 6542eac Compare August 23, 2019 13:17

smart: implement by-date insertion when revwalking

53f51c6

tiennou force-pushed the fix/commitish-smart-push branch from 6542eac to 53f51c6 Compare August 23, 2019 13:22

pks-t requested changes Aug 27, 2019

View reviewed changes

pks-t approved these changes Sep 13, 2019

View reviewed changes

pks-t merged commit 5d8a465 into libgit2:master Sep 13, 2019

tiennou deleted the fix/commitish-smart-push branch December 6, 2019 20:59

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

snyk-bot mentioned this pull request Mar 12, 2020

[Snyk] Upgrade nodegit from 0.26.0 to 0.26.4 thisconnect/nodegit-kit#95

Merged

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open

smart: use push_glob instead of manual filtering #5195

smart: use push_glob instead of manual filtering #5195

Uh oh!

Conversation

tiennou commented Jul 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiennou commented Aug 1, 2019

Uh oh!

libgit2-azure-pipelines bot commented Aug 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pks-t Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

biodranik commented Aug 7, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pks-t commented Sep 13, 2019

Uh oh!

Uh oh!

pks-t Aug 9, 2019 •

edited

Loading