pull requests: variable names

[Mads Kiilerich]

On 02/19/2015 01:32 PM, Thomas De Schampheleire wrote:
> On Thu, Feb 19, 2015 at 11:07 AM, Nick Coghlan <ncoghlan at gmail.com> wrote:
>> On 17 February 2015 at 13:13, Mads Kiilerich <mads at kiilerich.com> wrote:
>>> On 02/14/2015 09:36 PM, Thomas De Schampheleire wrote:
>>>> Hi,
>>>>
>>>> In the pull request code (both controllers and templates) I find the
>>>> variable names non-intuitive and it is therefore harder to make
>>>> changes.
>>>
>>> I have tried to make names that was better than before ... but it seems like
>>> there is some way to go ;-)
>>>
>>>> With this mail I would like to get an agreement on the terminology we
>>>> can change to.
>>>>
>>>> - org vs other --> source/src and dest
>>>
>>> Yes, that is a bit messy and could use some cleanup.
>>>
>>> I think the main challenge is that a pull request and compare in some ways
>>> are looking at the same thing from different point of views and reuse code
>>> and templates. The names source and dest are not so obvious in the context
>>> of compare. But give it a try and see how it works out. (Consider doing the
>>> two renaming in two different changesets so it is easier to review.)
>> Ah, that does indeed make a difference - source/dest isn't right for pure diffs.
>>
>>>> - c.cs_repo: what does 'cs' stand for here? This could become src_repo
>>>
>>> 'cs' is for changesets, referring to the side where the changes happened.
>>>
>>> I think the advantage of the current naming is that it refers to what it
>>> _is_ not to what we might happen to use it for or how we interpret it.
>> Would "changed_repo" be OK?
>>
>>>> - c.a_repo: what does 'a' stand for here?? This could become dest_repo.
>>> The 'a' was meant to refer to the ancestor used for comparing.
>> And expanding this out to "ancestor_repo"?
>>
>> Now you've explained them, I think changed & ancestor make sense as
>> terminology, but the "cs" and "a" abbreviations are arguably a bit
>> *too* abbreviated for clarity.
>>
> Agreed.
>
> Note that 'ancestor_repo' is not necessarily true: when comparing two
> repos they could both be peers, not ancestor-child, right?

Yes, not ancestor as in ancestor/child but as in 'common ancestor' ... 
which usually(?) is what you want when comparing branches. (This is 
where I think a "pull request" pretty much is a "named branch compare")

> Also, in case of compare, change may neither be accurate.

It is when doing an asymmetric "merge" compare where you by definition 
only look at changes from one side.

There might be cases where you just want to compare unrelated changesets 
(even though it is hard to imagine when that should be relevant) - in 
that case you are right that both sides might have changesets.

> I think it's hard to find one combination of terms that matches both
> compare _and_ pullrequests.

Well ... 'cs' and 'a' was my attempt of finding something that applies 
both to 'merge compare' and pull requests.

> Which code is actually shared between both?

I don't think there is a good answer to that. Try it out and see how far 
you can come.

> If the terminology there is neutral, then in both compare and
> pullrequests the appropriate derived terminology could be mapped on
> it, for example left/right for compare, and src/dest for pullrequest.

Note also that for a pull request, you really are previewing the merge 
with the 'target head' but in doing so you look at the diff from the 
'target branch ancestor'. Target (or dest) is thus not unambiguous spot on.

[Bitbucket do not show a diff from the ancestor. They try to do an in 
memory merge and show the diff from the target/destination head, often 
with unreadable conflict markers. I don't like that.]

/Mads