Commits that implicitly close PRs do not remember doing so

[Mojca Miklavec]

On 12 December 2016 at 10:35, Joshua Root wrote:
> On 2016-12-12 20:21 , Rainer Müller wrote:
>> On 2016-12-12 02:29, Lawrence Velázquez wrote:
>>>> On Dec 6, 2016, at 10:33 AM, Rainer Müller wrote:
>>>>
>>>> (e) add "Closes: #XYZ" to the commit message
>>>
>>> When commits close PRs implicitly (by merging the PR branch) instead of
>>> explicitly (by using "closes #XYZ" in the message), the PR is remembered
>>> internally by GitHub and displayed on the website but is not recorded in our
>>> Git repository. If we ever migrate off GitHub, we would presumably lose this
>>> information.
>>>
>>> Should we consider this a problem?
>>
>>
>> I don't think there is any value in preserving all patch iterations of a
>> pull request. I would compare this to an initial patch submission in the
>> issue tracker, which are often merged in a different form later. Nobody
>> will look at the initial patch again or even remember.
>
> I think the information Larry is referring to is not the actual PR contents,

I don't think the initial patch is relevant either. But the discussion might be.
Generally I never open any old tickets, but sometimes when working on
a particular port I want to understand why there's a specific patch or
some specific line of code, then go through commit history and often
find references to trac tickets with an extensive discussions that
explains a lot. Having an explicit link to a GitHub PR would certainly
help.

> but rather the links from commits to PRs.

I also think that this information might be useful. When opening
tickets on Trac (or if we had issues), we would always provide a link.
On GitHub this is only implicit and even now people with local git
clients (either command line or gui ones) don't get a link to the PR
in case they are interested.

The stupid bit is that getting this right is sometimes a bit annoying.
I recently opened a pull request with 18 commits. (I wanted to know if
anyone had any concerns before merging; eventually I just committed
everything and there was zero discussion, but that's not the point.)

One doesn't know the PR number in advance, so putting in any number
would be pure speculation. On the other hand putting that information
in after opening the pull request would require editing all the 18
commit messages. It's not impossible, just a bit annoying.

> I don't think this is a big
> problem,

I don't think it is a big problem either.

But it's a good point and I would be in favour of it (but not insisting).

> but it's one more reason to open a trac ticket for any change that
> involves significant discussion that should be archived.

I would argue that you never know in advance how much discussion a
pull request would attract. On top of that you would often want to
comment on a specific line of code. Opening a ticket on Trac just for
"extensive discussion" about pull request will just spread the
information to more places and lead to more confusion (I would never
know whether to comment on Trac or on GitHub).


It is not unrealistic that we will want to or have to migrate one day
in the future, there's already quite a bit of the desired
functionality missing, but it's the best option we have at the moment
(like sourceforge used to be the place-to-go for opensource projects
long long time ago). In that case we could still leave the account
open and old PRs could remain there (or be migrated and links somehow
fixed).

This actually reminded me of another potential "problem". In the past
we always used the
    (closes #1234567)
syntax which we changed during migration to GIT to absolute links. Do
we want to use absolute links for
    Closes: https://github.com/macports/macports-ports/pull/1234567
as well?

Mojca