[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