[PATCH] git: add references for Git pull request heads
Andrew Shadura
andrew at shadura.me
Fri Jan 6 20:55:02 UTC 2017
On 06/01/17 21:17, Mads Kiilerich wrote:
> On 01/06/2017 08:07 PM, Andrew Shadura wrote:
>> # HG changeset patch
>> # User Andrew Shadura <andrew at shadura.me>
>> # Date 1474617925 -7200
>> # Fri Sep 23 10:05:25 2016 +0200
>> # Node ID d55296180cd477d84545d97d2abf1073ca8f0711
>> # Parent 1341be63734a5b90253bdd1589caf1a01e9266d1
>> git: add references for Git pull request heads
>>
>> When a pull request is created, stick a reference to it in the refs/pull
>> namespace, similarly to what GitHub does, and remove it when the pull
>> request is deleted.
>
> How likely is it that our use of refs/pull will conflict with other uses
> of it? Are these names local to the repo managed by Kallithea and not
> shared with clients?
refs/pull namespace isn't shared with clients by default unless they
explicitly ask for it. The Git default for remotes is:
fetch = +refs/heads/*:refs/remotes/origin/*
>> That reference guarantees the commits stay around
>> and don't get garbage-collected when a PR is rebased or revised.
>> Also, that makes it possible to access head of the historis
>
> (typo)
Ack.
>> pull
>> requests using Git by fetching a ref from the source repository.
>
> What does that mean?
>
> Is it so that the pull command shown on a git PR also introduce a local
> name for it so it is possible to check the revision out without getting
> into detached head mode?
Users can do git fetch origin pull/ID/head:BRANCHNAME and have the ref
fetched into the local repository as BRANCHNAME. Then you can check out
that branch, yes.
>> Unlike Git though,
>
> But this is Git. Do you mean github?
Sure, it's a typo, I meant GitHub.
>> we don't put the ref into the destination repository
>> and don't copy commits there to prevent undesired repository growth.
>> Kallithea uses global pull request identifiers, so there should not be
>> any confusion as to what pull request the ref corresponds to.
>
> That would be because the PR id is a part of the ref? Perhaps clarify
> the exact refs naming scheme first.
What I meant was that ref/pull/23423/head can refer to PR 23423, and we
can have one and only one PR with that number. GH, on the other side,
gives PRs numbers per-repository, so ref/pull/23423/head doesn't specify
what repository the PR is related to.
>> We may later provide a shim to redirect users to the source repository
>> if that deems needed.
>>
>> diff --git a/kallithea/controllers/pullrequests.py
>> b/kallithea/controllers/pullrequests.py
>> --- a/kallithea/controllers/pullrequests.py
>> +++ b/kallithea/controllers/pullrequests.py
>> @@ -330,6 +330,8 @@ class PullrequestsController(BaseRepoCon
>> if org_ref_type == 'rev':
>> cs = org_repo.scm_instance.get_changeset(org_rev)
>> org_ref = 'branch:%s:%s' % (cs.branch, cs.raw_id)
>> + else:
>> + cs = org_rev
>
> In one branch cs is a changeset, in the other it is just the hash of it?
> I suggest making it more consistent and just make the "cs =
> org_repo.scm_instance.get_changeset(org_rev)" unconditional.
>
> Or perhaps even better: just use org_rev instead of cs?
Will think about it.
>> other_repo_name = _form['other_repo']
>> other_ref = _form['other_ref'] # will have symbolic name and
>> head revision
>> @@ -385,6 +387,11 @@ class PullrequestsController(BaseRepoCon
>> created_by, org_repo, org_ref, other_repo,
>> other_ref, revisions,
>> title, description, reviewer_ids)
>> Session().commit()
>> +
>> + if org_repo.scm_instance.alias == 'git':
>> + # create a ref under refs/pull/ so that commits don't
>> get garbage-collected
>> + org_repo.scm_instance._repo["refs/pull/%d/head" %
>> pull_request.pull_request_id] = safe_str(cs)
>> +
>> h.flash(_('Successfully opened new pull request'),
>> category='success')
>> except UserInvalidException as u:
>> @@ -579,6 +586,14 @@ class PullrequestsController(BaseRepoCon
>> if pull_request.owner_id == c.authuser.user_id:
>> PullRequestModel().delete(pull_request)
>> Session().commit()
>> +
>> + if org_repo.scm_instance.alias == 'git':
>> + # remove a ref under refs/pull/ so that commits can
>> be garbage-collected
>> + try:
>> + del
>> org_repo.scm_instance._repo["refs/pull/%s/head" %
>> safe_str(pull_request_id)]
>
> The safe_str is a bit odd when we know it is a number. Perhaps use
> pull_request.pull_request_id instead? It is perhaps a bit dirty to use
> pull_request outside the transaction ... but the id must already be
> fetched and I think it is better than safe_str.
>
> I wonder if it is better to do this deletion inside or outside the
> transaction ... but I can see how it is safer to do it outside.
>
>> + except KeyError:
>> + pass
>> +
>> h.flash(_('Successfully deleted pull request'),
>> category='success')
>> raise HTTPFound(location=url('my_pullrequests'))
>> @@ -803,8 +818,17 @@ class PullrequestsController(BaseRepoCon
>>
>> h.HasRepoPermissionAny('repository.admin')(pull_request.org_repo.repo_name)
>> or
>>
>> h.HasRepoPermissionAny('repository.admin')(pull_request.other_repo.repo_name)
>>
>> ) and not pull_request.is_closed():
>> + org_repo =
>> RepoModel()._get_repo(pull_request.org_repo.repo_name)
>> PullRequestModel().delete(pull_request)
>> Session().commit()
>> +
>> + if org_repo.scm_instance.alias == 'git':
>> + # remove a ref under refs/pull/ so that commits
>> can be garbage-collected
>> + try:
>> + del
>> org_repo.scm_instance._repo["refs/pull/%s/head" %
>> safe_str(pull_request_id)]
>> + except KeyError:
>> + pass
>
> This looks a bit like there should be a function for PR deletion so we
> don't have to repeat this.
Probably.
--
Cheers,
Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: OpenPGP digital signature
URL: <http://lists.sfconservancy.org/pipermail/kallithea-general/attachments/20170106/02aed483/attachment.sig>
More information about the kallithea-general
mailing list