[PATCH] git: add references for Git pull request heads

[Mads Kiilerich]

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?

> 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)

> 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?

> Unlike Git though,

But this is Git. Do you mean 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.

> 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?

>           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.

/Mads

> +
>                   h.flash(_('Successfully deleted pull request %s') % pull_request_id,
>                           category='success')
>                   return {
> _______________________________________________
> kallithea-general mailing list
> kallithea-general at sfconservancy.org
> https://lists.sfconservancy.org/mailman/listinfo/kallithea-general