Skip to content

Remote revamp (refspecless, _ls)#1914

Merged
vmg merged 9 commits intodevelopmentfrom
cmn/fetch-default-head
Nov 11, 2013
Merged

Remote revamp (refspecless, _ls)#1914
vmg merged 9 commits intodevelopmentfrom
cmn/fetch-default-head

Conversation

@carlosmn
Copy link
Member

[This turned into a bit of a larger change than expected]

When we fetch from a refspecless remote, we need to download its HEAD, and we need to write out a FETCH_HEAD for it.

Let's keep a copy of the refspecs we're going to use, which is usually what the user already has, but may have an additional "HEAD" if there are none.

While here, I noticed that the behaviour of git_remote_ls() is stupid, as there's no reason to use callbacks when we could simply return the array we already have (with typical caveats about lifetime), so I'm changing it to do just that, which ends up removing a lot of silly code in callbacks.

@carlosmn
Copy link
Member Author

In order to avoid changing the user's object, I was wondering if it might make sense to clone the vectors we're interested in so we can do whatever we want with them. Or maybe that's not worth the effort to avoid this extra handling?

@vmg
Copy link
Member

vmg commented Nov 1, 2013

Needs rebase please. :)

The correct behaviour when a remote has no refspecs (e.g. a URL from the
command-line) is to download the remote's HEAD. Let's do that.

This fixes #1261.
This avoids sending our whole history bit by bit to the remote in cases
where there is no common history, just to give up in the end.

The number comes from the canonical implementation.
When downloading the default branch due to lack of refspecs, we still
need to write out FETCH_HEAD with the tip we downloaded, unfortunately
with a format that doesn't match what we already have.
This allows us to add e.g. "HEAD" as a refspec when none are given
without overwriting the user's data.
@carlosmn
Copy link
Member Author

carlosmn commented Nov 2, 2013

Alright, this should be ready for review now.

Copy link
Member

Choose a reason for hiding this comment

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

How are binding authors going to free the allocated remote head array?

Copy link
Member Author

Choose a reason for hiding this comment

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

They don't. The memory belongs to the transport and it's guaranteed to exist until the next function call. It works much the same as git_index_entry except this gives out the array instead of a _byindex() accessor.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Can you document that then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. This is documented in git_remote_ls which is what you'd use, but it can't hurt to repeat it here.

Removing arbitrary refspecs makes things more complex to reason
about. Instead, let the user set the fetch and push refspec list to
whatever they want it to be.
The callback-based method of listing remote references dates back to the
beginning of the network code's lifetime, when we didn't know any
better.

We need to keep the list around for update_tips() after disconnect() so
let's make use of this to simply give the user a pointer to the array so
they can write straightforward code instead of having to go through a
callback.
Copy the pointers into temporary vectors instead of assigning them tot
he same array so we don't mess up with someone else's memory by
accident (e.g. by sorting).
These tests were forgotten when modifying git_remote_ls().
vmg pushed a commit that referenced this pull request Nov 11, 2013
@vmg vmg merged commit 91223c5 into development Nov 11, 2013
@vmg
Copy link
Member

vmg commented Nov 11, 2013

OOOKKK

@vmg
Copy link
Member

vmg commented Nov 11, 2013

This has been reverted. Please re-open a PR once the segfault is fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants