www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - "Squash and merge" on GitHub

reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
GitHub rolled out a new feature:

https://github.com/blog/2141-squash-your-commits

Essentially it gives people who merge a PR the option that 
instead of creating a merge commit with one parent being "master" 
(or the target branch) and the other being a PR branch, create a 
single commit with all the commits from the PR squashed into one, 
with no merge commit.

In the context of our repos, I'm not sure it's very useful, and I 
think it should be turned off. My arguments are:

1. It makes bisecting more difficult because you can no longer 
tell which PR a commit belongs to just from the DAG.

It is still sort-of possible if you take the commit hash, paste 
it into your browser's address bar (e.g. 
https://github.com/D-Programming-Language/phobos/commit/8dc29bff62fc7c2f806e0
cccc01be55923fa83b) - GitHub will add links to the pull request this commit was
a part of at the top. I'm not aware of a way to do it programmatically (other
than scraping the website).

2. It makes bisecting more difficult because all commits are 
squashed into one.

Clearly this option should not be used for large PRs where the 
commits are already separate self-contained changes in the first 
place. This doesn't apply when the PR author sends off a bunch of 
fixup PRs without amending/rebasing, in which case it can be 
useful.

3. It makes bisecting more difficult because it breaks Digger :) 
Although I could probably add support for such merges, I'd rather 
not :)

4. We should use the autotester's auto-merge feature anyway.

In theory the autotester could be improved to allow choosing 
which merge style to use... in theory.

5. It will create merge conflicts if the PR branch is used 
elsewhere.

Not sure if this applies to just me, but when I submit a PR to 
scratch a personal itch, I will probably use the branch locally 
too... when the PR is merged into master, updating my personal 
branch is normally painless, however this this will put a stick 
to this wheel.

As such, I propose to disable the feature completely for the core 
D-P-L repositories, so that it's unavailable to committers when 
merging PRs.
Apr 01 2016
next sibling parent Jack Stouffer <jack jackstouffer.com> writes:
On Friday, 1 April 2016 at 20:28:23 UTC, Vladimir Panteleev wrote:

 3. It makes bisecting more difficult because it breaks Digger 
 :) Although I could probably add support for such merges, I'd 
 rather not :)


This alone is evidence enough that it should be disabled.
Apr 01 2016
prev sibling parent reply Daniel Murphy <yebbliesnospam gmail.com> writes:
On 2/04/2016 7:28 AM, Vladimir Panteleev wrote:

 4. We should use the autotester's auto-merge feature anyway.


Can we disable both and force everyone to use the autotester?
Apr 01 2016
next sibling parent reply Jack Stouffer <jack jackstouffer.com> writes:
On Saturday, 2 April 2016 at 03:58:27 UTC, Daniel Murphy wrote:

 On 2/04/2016 7:28 AM, Vladimir Panteleev wrote:

 4. We should use the autotester's auto-merge feature anyway.


 Can we disable both and force everyone to use the autotester?


Yes, you can take away everyone but the auto-tester's merge 
rights.
Apr 01 2016
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 4/2/16 12:36 AM, Jack Stouffer wrote:

 On Saturday, 2 April 2016 at 03:58:27 UTC, Daniel Murphy wrote:

 On 2/04/2016 7:28 AM, Vladimir Panteleev wrote:

 4. We should use the autotester's auto-merge feature anyway.


 Can we disable both and force everyone to use the autotester?


 Yes, you can take away everyone but the auto-tester's merge rights.


Actually that might be a good setup. We'd need an escape hatch for a 
small circle, and control over the autotester. Also we'd need to make 
the autotester work with all repos. -- Andrei
Apr 01 2016
next sibling parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Saturday, 2 April 2016 at 05:18:47 UTC, Andrei Alexandrescu 
wrote:

 On 4/2/16 12:36 AM, Jack Stouffer wrote:

 On Saturday, 2 April 2016 at 03:58:27 UTC, Daniel Murphy wrote:

 On 2/04/2016 7:28 AM, Vladimir Panteleev wrote:

 4. We should use the autotester's auto-merge feature anyway.


 Can we disable both and force everyone to use the autotester?


 Yes, you can take away everyone but the auto-tester's merge 
 rights.


 Actually that might be a good setup. We'd need an escape hatch 
 for a small circle, and control over the autotester. Also we'd 
 need to make the autotester work with all repos. -- Andrei


Currently it seems that the autotester uses the committer's 
account to perform the merge (probably as an additional security 
measure that only committers can merge via the autotester). It 
would need some changes.
Apr 01 2016
parent Seb <seb wilzba.ch> writes:
On Saturday, 2 April 2016 at 06:02:26 UTC, Vladimir Panteleev 
wrote:

 On Saturday, 2 April 2016 at 05:18:47 UTC, Andrei Alexandrescu 
 wrote:

 On 4/2/16 12:36 AM, Jack Stouffer wrote:

 On Saturday, 2 April 2016 at 03:58:27 UTC, Daniel Murphy 
 wrote:

 On 2/04/2016 7:28 AM, Vladimir Panteleev wrote:

 4. We should use the autotester's auto-merge feature anyway.


 Can we disable both and force everyone to use the autotester?


 Yes, you can take away everyone but the auto-tester's merge 
 rights.


 Actually that might be a good setup. We'd need an escape hatch 
 for a small circle, and control over the autotester. Also we'd 
 need to make the autotester work with all repos. -- Andrei


 Currently it seems that the autotester uses the committer's 
 account to perform the merge (probably as an additional 
 security measure that only committers can merge via the 
 autotester). It would need some changes.


FYI: One can and should enable the CI pass protection for merging 
(at least the auto-tester itself should be included). I have used 
this for a couple of repos (libmir, dlang-tour, ...) and it makes 
one sleep "safer" ;-)
(admins still have the right to merge PRs directly if really 
needed).

[1] 
https://help.github.com/articles/enabling-required-status-checks/


 Essentially it gives people who merge a PR the option that 
 instead of creating a merge commit with one parent being 
 "master" (or the target branch) and the other being a PR 
 branch, create a single commit with all the commits from the PR 
 squashed into one, with no merge commit.


I would like to restart the discussion as over the last months I 
realized how much pain it's if (a) you are managing a PR and have 
to squash your commits from time to time and thus destroy the 
review history instead of just appending the commits  or (b) if 
you are a reviewer you always have the back-and-forth between the 
submitter to explain him how to squash his PR. Especially for 
newbies or people coming from other projects this can be quite 
confusing.

I think this back-and-forth squash cycle is pretty annoying for 
everybody, a famous example for this is the "Nazi" commit at 
Phobos:

https://github.com/dlang/phobos/pull/4662

Hence I argue that we should support squashing as an option to 
make our Github PR flow easier in many cases. An alternative 
would be to add an option to the auto-tester to squash all 
commits before merging, which would retain the additional merge 
commit, but afaict this wouldn't work with CI protection enabled.


 1. It makes bisecting more difficult because you can no longer 
 tell which PR a commit belongs to just from the DAG.


The PR id is part of the commit message


 2. It makes bisecting more difficult because all commits are 
 squashed into one.


I see this as an advantage, see above.


 4. We should use the autotester's auto-merge feature anyway.


Yep, but maybe autotester can support squashed merges as an 
option.


 5. It will create merge conflicts if the PR branch is used 
 elsewhere.


That's seldom and I am not arguing that we should use squashing 
all the time.
Aug 21 2016
prev sibling parent reply Jonathan M Davis via Digitalmars-d <digitalmars-d puremagic.com> writes:
On Saturday, April 02, 2016 01:18:47 Andrei Alexandrescu via Digitalmars-d 
wrote:

 On 4/2/16 12:36 AM, Jack Stouffer wrote:

 On Saturday, 2 April 2016 at 03:58:27 UTC, Daniel Murphy wrote:

 On 2/04/2016 7:28 AM, Vladimir Panteleev wrote:

 4. We should use the autotester's auto-merge feature anyway.


 Can we disable both and force everyone to use the autotester?


 Yes, you can take away everyone but the auto-tester's merge rights.


 Actually that might be a good setup. We'd need an escape hatch for a
 small circle, and control over the autotester. Also we'd need to make
 the autotester work with all repos. -- Andrei


While it sounds like a good idea in theory, I question that it's worth it.
Almost always, committers are already merging using the autotester, so I
don't think that we actually have a problem here. And there are definitely
cases where you _need_ to merge stuff without the autotester (primarily
because you end up with commits in different repos that depend on each other
to avoid breaking the build). The "small circle" with full commit access
would presumably solve that problem, but then you'd be stuck waiting for
them whenever there was a set of PRs in that boat, which risks creating
another bottleneck. In practice, in may not be a problem (especially since
PRs like that are fairly rare, and they usually involve folks who would
probably be in the small circle anyway), but since we don't really have a
problem with folks skipping the autotester, I'm not sure that it's worth
going through the trouble.

Another thing to consider is that - as Vladimir points out - currently the
autotester commits with the commiter's account and not with a separate one,
and that's not just an implementation issue. It's also an accountability
issue. Right now, because of how the autotester does the merge, you can see
who did it in the commit log. But if the autotester does the commit with its
own account, how do you then figure out who did the merge? Would we have to
have a separate log in the autotester to tell us? Do you have to know which
PR it went with to see the comment in github? Right now, if a committer
starts misbehaving, you can easily see that in the commit logs, and it's
easy to see their name in the messages that github sends when a merge is
done. We lose that if the autotester merges with its own account.

So, I'm inclined to think that while forcing the restriction on merging via
the autotester might be a good idea on the surface, it's actually a bad idea
when you get down to the details. And enforcing it via social policy has
worked well overall thus far.

- Jonathan M Davis
Apr 02 2016
parent Kagamin <spam here.lot> writes:
Wasn't there the "Committed of behalf" feature?
Apr 02 2016
prev sibling parent Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Saturday, 2 April 2016 at 03:58:27 UTC, Daniel Murphy wrote:

 On 2/04/2016 7:28 AM, Vladimir Panteleev wrote:

 4. We should use the autotester's auto-merge feature anyway.


 Can we disable both and force everyone to use the autotester?


Not an answer to your question, but after accidentally bypassing 
the autotester a few times (lack of habit due to working on other 
GitHub projects), I wrote this for myself:

http://dump.thecybershadow.net/6265c3ae18e8bf7a176d00e5e22ff395/NoMergeButton.user.js
Apr 02 2016