Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Nate Graham]

So to close the loop on this discussion, it seems like folks are not 
generally in favor of my proposal, for various sensible and 
well-considered reasons. I will beef up our GitLab documentation to add 
more information about how to use a curated commit history approach and 
will drop the idea to squash by default.


We will need to commit to vigilance when merging MRs--our own or someone 
else's--to make sure that merge requests which don't have curated commit 
history are squashed to avoid accidentally polluting the git history.


Note that using the "Apply suggestion" feature in the Gitlab web UI 
creates new commits in the merge request. These must be either manually 
squashed into the commits of a commit-curated MR, or squashed at 
merge-time for an un-commit-curated MR.


Nate



On 10/7/20 10:12 AM, Carson Black wrote:

Am Mi., 7. Okt. 2020 um 11:27 Uhr schrieb Thomas Friedrichsmeier
:


Am Tue, 6 Oct 2020 08:26:02 -0600
schrieb Nate Graham :

GitLab already *kind of* offers this, in the form of the "Squash
commits" checkbox next to the merge button. Apparently it's not
obvious enough though, because I can think of a bunch of cases of
multi-commit MRs with mostly garbage commits accidentally not being
squashed when merging.


Unfortunately the workflow is rather backwards in that the checkbox
needs to be ticked, when creating the merge request, not after review.
However right before merging would be the time to judge whether the
commit history contains valuable information or useless noise.

(IIRC the "squash commits" checkbox can still be changed at that
point, but it's not obviously visible, then).


The checkbox is fairly visible for me, by the merge button like Nate
said: https://imgur.com/a/gzRDYnZ

This can be ticked immediately after review and before merging easily
like you said would be the ideal time to do so.

-- Jan

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[David Hurka]

On Wednesday, October 7, 2020 5:26:05 PM CEST Thomas Friedrichsmeier wrote:
> Probably not something we can easily configure/adjust downstream,
> though?

What we can easily can change on our level, is to provide a default MR 
description in every project. (Like the default bug description in Bugzilla.) 

My theory is that some newcoming contributors already know GitLab, and just 
create the MR as they are used to, without reading our wiki article. The 
consequence is that they ignore these checkboxes.

About default MR descriptions:
https://invent.kde.org/help/user/project/description_templates.md

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Carson Black]

Am Mi., 7. Okt. 2020 um 11:27 Uhr schrieb Thomas Friedrichsmeier
:
>
> Am Tue, 6 Oct 2020 08:26:02 -0600
> schrieb Nate Graham :
> > GitLab already *kind of* offers this, in the form of the "Squash
> > commits" checkbox next to the merge button. Apparently it's not
> > obvious enough though, because I can think of a bunch of cases of
> > multi-commit MRs with mostly garbage commits accidentally not being
> > squashed when merging.
>
> Unfortunately the workflow is rather backwards in that the checkbox
> needs to be ticked, when creating the merge request, not after review.
> However right before merging would be the time to judge whether the
> commit history contains valuable information or useless noise.
>
> (IIRC the "squash commits" checkbox can still be changed at that
> point, but it's not obviously visible, then).

The checkbox is fairly visible for me, by the merge button like Nate
said: https://imgur.com/a/gzRDYnZ

This can be ticked immediately after review and before merging easily
like you said would be the ideal time to do so.

-- Jan

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Thomas Friedrichsmeier]

Am Tue, 6 Oct 2020 08:26:02 -0600
schrieb Nate Graham :
> GitLab already *kind of* offers this, in the form of the "Squash 
> commits" checkbox next to the merge button. Apparently it's not
> obvious enough though, because I can think of a bunch of cases of
> multi-commit MRs with mostly garbage commits accidentally not being
> squashed when merging.

Unfortunately the workflow is rather backwards in that the checkbox
needs to be ticked, when creating the merge request, not after review.
However right before merging would be the time to judge whether the
commit history contains valuable information or useless noise.

(IIRC the "squash commits" checkbox can still be changed at that
point, but it's not obviously visible, then).

Probably not something we can easily configure/adjust downstream,
though?

Thomas


pgp9MX2Jti8Wu.pgp
Description: Digitale Signatur von OpenPGP

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Ben Cooksley]

On Wed, Oct 7, 2020 at 9:33 PM David Hurka  wrote:

> On Wednesday, October 7, 2020 9:52:41 AM CEST Ben Cooksley wrote:
> > > Isn’t it true that “Allow contributions” must be checked before the
> > > “Squash
> > > commits” checkbox is available? (I already wrote that, but I feel
> people
> > > don’t
> > > care, so I make it a question now.)
> >
> > The allow contributions box should always be ticked - we have
> > infrastructure components that automatically change this in the
> background
> > whenever a merge request is created.
>
> Oh, nice! This is from a MR from June 2020, which couldn’t be rebased:
>
> > You need to force push since you're rewriting history.
> >
> > If that doesn't work it means the branch author manually unchecked
> > the checkbox that enables collaboration and indeed there's not much
> > we can do other than creating a new branch/MR somewhere else.
>
> (https://invent.kde.org/graphics/okular/-/merge_requests/195#note_103948)
>
> Is the mentioned infrastructure component newer than that? Then it
> probably
> works fine. :)
>


>

I've taken a look and it appears that the review in question does have the
"Allow Collaboration" box ticked, so you should have been able to rebase it.
Unfortunately it has since been closed, so we can't investigate any further
unfortunately...

Cheers,
Ben

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[David Hurka]

On Wednesday, October 7, 2020 9:52:41 AM CEST Ben Cooksley wrote:
> > Isn’t it true that “Allow contributions” must be checked before the
> > “Squash
> > commits” checkbox is available? (I already wrote that, but I feel people
> > don’t
> > care, so I make it a question now.)
> 
> The allow contributions box should always be ticked - we have
> infrastructure components that automatically change this in the background
> whenever a merge request is created.

Oh, nice! This is from a MR from June 2020, which couldn’t be rebased:

> You need to force push since you're rewriting history.
> 
> If that doesn't work it means the branch author manually unchecked
> the checkbox that enables collaboration and indeed there's not much
> we can do other than creating a new branch/MR somewhere else.

(https://invent.kde.org/graphics/okular/-/merge_requests/195#note_103948)

Is the mentioned infrastructure component newer than that? Then it probably 
works fine. :)

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Ben Cooksley]

On Wed, Oct 7, 2020 at 8:08 AM David Hurka  wrote:

> On Tuesday, October 6, 2020 4:26:02 PM CEST Nate Graham wrote:
> > Taking stock of the responses so far, it doesn't seem like there's much
> > enthusiasm for the original proposal. That's fine, and I can understand
> > the desire to push people to improve their git skills.
>
> Yes, I interpret this thread the same.
>
> > It seems like
> > there is some agreement on an alternative, which various people have
> > proposed:
> >
> > GitLab already *kind of* offers this, in the form of the "Squash
> > commits" checkbox next to the merge button.
>
> Isn’t it true that “Allow contributions” must be checked before the
> “Squash
> commits” checkbox is available? (I already wrote that, but I feel people
> don’t
> care, so I make it a question now.)
>

The allow contributions box should always be ticked - we have
infrastructure components that automatically change this in the background
whenever a merge request is created.

Cheers,
Ben

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[David Hurka]

On Tuesday, October 6, 2020 4:26:02 PM CEST Nate Graham wrote:
> Taking stock of the responses so far, it doesn't seem like there's much
> enthusiasm for the original proposal. That's fine, and I can understand
> the desire to push people to improve their git skills.

Yes, I interpret this thread the same.

> It seems like
> there is some agreement on an alternative, which various people have
> proposed:
> 
> GitLab already *kind of* offers this, in the form of the "Squash
> commits" checkbox next to the merge button.

Isn’t it true that “Allow contributions” must be checked before the “Squash 
commits” checkbox is available? (I already wrote that, but I feel people don’t 
care, so I make it a question now.)

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Nate Graham]

Taking stock of the responses so far, it doesn't seem like there's much 
enthusiasm for the original proposal. That's fine, and I can understand 
the desire to push people to improve their git skills. It seems like 
there is some agreement on an alternative, which various people have 
proposed:



On 10/3/20 6:10 AM, David Edmundson wrote:
> We don't want a default for a merge option, we want an exposed action
> like the existing rebase button to squash things within the local
> branch. That would mean reviewers can review commits (and therefore
> review commit messages properly) and you still provide an easy path for
> people who can't squash locally. If we only approve when commits
> themselves are sound, it'll be easy to manage. Win-win.

On 10/5/20 8:21 AM, Volker Krause wrote:
> Even better might be to force an explicit decision by not having a 
default for
> this at all, e.g. by offering "Rebase" and "Squash + Rebase" actions 
next to

> each other.

On 10/5/20 10:38 AM, Ömer Fadıl USTA wrote:
my suggestion is not making squash default but implement a way that  
will pops up a question if there are more then 1 commits in mr so user 
can select on that time.



GitLab already *kind of* offers this, in the form of the "Squash 
commits" checkbox next to the merge button. Apparently it's not obvious 
enough though, because I can think of a bunch of cases of multi-commit 
MRs with mostly garbage commits accidentally not being squashed when 
merging.


Maybe this is just a teething issue that we'll overcome with more 
experience?



Nate

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Ömer Fadıl USTA]

my suggestion is not making squash default but implement a way that  will
pops up a question if there are more then 1 commits in mr so user can
select on that time.

On Mon, Oct 5, 2020, 19:15 David Hurka  wrote:

> > Even better might be to force an explicit decision by not having a
> default
> > for this at all, e.g. by offering "Rebase" and "Squash + Rebase" actions
> > next to each other.
>
> The squash checkbox is available directly next to the Rebase/Merge button;
> provided the “Allow Contributions” checkbox was checked when creating the
> MR.
>
>
>

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[David Hurka]

> Even better might be to force an explicit decision by not having a default
> for this at all, e.g. by offering "Rebase" and "Squash + Rebase" actions
> next to each other.

The squash checkbox is available directly next to the Rebase/Merge button; 
provided the “Allow Contributions” checkbox was checked when creating the MR.

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Volker Krause]

On Freitag, 2. Oktober 2020 23:38:20 CEST Albert Astals Cid wrote:
> El divendres, 2 d’octubre de 2020, a les 21:19:02 CEST, Konstantin Kharlamov 
va escriure:
> > On Fri, 2020-10-02 at 11:39 -0600, Nate Graham wrote:
> > > Accordingly, I think squash-merging makes sense as the default value to
> > > avoid this. People who are comfortable with the "curated MR commit
> > > history" workflow will of course still be able to turn it off. IMO it
> > > makes more sense to ask experts to turn it off than to ask newcomers and
> > > novices to turn it on.
> > 
> > Please don't. This will result in having many MRs with a valid history to
> > get squashed just because people would keep forgetting to uncheck it.
> > Given there are much more people who aware of how to work with git
> > history, I think this would hurt more than occasionally merging a
> > non-squashed MR from a newcomer.
> +1, squash by default is bound to break more things than fix.

I agree, the damage of an accidentally squashed set of properly done commits 
is bigger than an accidentally integrated fixup commit.

Even better might be to force an explicit decision by not having a default for 
this at all, e.g. by offering "Rebase" and "Squash + Rebase" actions next to 
each other.

Regards,
Volker

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Elvis Angelaccio]

On 02/10/20 19:39, Nate Graham wrote:
> Hello folks,

Hi

> I've been told that our Sysadmins have developed some tooling capable of
> checking the "Squash when merging" checkbox by default for new Merge
> Requests. This would be a downstream solution to
> https://gitlab.com/gitlab-org/gitlab/-/issues/222175.
> 
> I'd like to propose that this be done as a sane default for new Merge
> Requests. Now, personally I have gotten used to the alternative "curate
> your MR's git history" approach and have written documentation for it at
> https://community.kde.org/Infrastructure/GitLab#Curating_your_merge_request_commit_history.
> 
> 
> However, it remains a fairly advanced workflow which is challenging for
> newcomers, drive-by-developers, and people not as familiar with git. For
> these people, squash-merging makes much more sense, and when they forget
> to check that checkbox and someone merges their work, the result is tons
> of garbage commits in the git history.

People should not blindly merge a MR without checking the commit history
first:
- if the submitter pushed one or more well-written atomic commits, those
can be merged without squashing
- otherwise, whoever merges the MR should squash the history and write a
proper commit message. Or they should ask the submitter to fix its history.

> 
> Accordingly, I think squash-merging makes sense as the default value to
> avoid this. People who are comfortable with the "curated MR commit
> history" workflow will of course still be able to turn it off. IMO it
> makes more sense to ask experts to turn it off than to ask newcomers and
> novices to turn it on.

+0, I don't have a preference for this default value. As explained, it's
orthogonal to the problem of a bad commit history.

> 
> Thoughts?
> 
> Nate

Cheers,
Elvis

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Johan Ouwerkerk]

On Sat, Oct 3, 2020 at 12:19 PM David Hurka  wrote:
>
> Why should colleagues navigate through any commits, when the MR is intended to
> be squashed? Wouldn’t squash merge make it easier to review an MR?
>

No because squashing happens only when merging, i.e. *after*
reviewing. So if you review commit-by-commit you have more work and
you're more likely to comment on things which turn out to be fixed by
a later commit.

>
> I think this can not be expected from new contributors.
>
> If we reject a patch submitted a new user with “Thanks, your patch finally
> fixes this annoying bug, but we can not accept it, because I don’t like your
> git history. Please learn git first.”, that would IMHO be the first step to
> make KDE a closed community.
>

I think that the other position is more accurately stated as follows:
if a patch needs work on the git commit level but the code is fine,
perhaps the maintainer should sit down and take time to checkout that
branch locally, do their git magic until they are satisfied and then
push that result to master.
Then just leave a comment on the MR thanking people for their work,
explaining their changes got merged into master via a different route
and invite the contributor to test the new shiny to validate that
their issue is fully fixed.

A similar thing is also needed w.r.t. rebasing if for example you want
to preserve a linear history in master: you can't expect drive by
contributors to have infinite patience to check back and rebase, but
it's not unreasonable for a maintainer who insists on linear history
to shoulder that burden when they get round to merging things in.

In general both approaches would point towards contributors not
merging things on their own by default until they're familiar with the
project and made a co-maintainer. It is also more closely aligned with
the PR/MR workflow itself: note the R stands for "request" after all
:) With active maintainers, that should not be hostile to new
contributors, since you mostly care that your issue gets scratched and
not who does the scratching...

But it does require very active maintainers and it may not be a good
fit for all projects. :)

Regards,

- Johan Ouwerkerk

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[David Edmundson]

>
> your_merge_request_commit_history
> 
> .
>
> However, it remains a fairly advanced workflow which is challenging for
> newcomers, drive-by-developers, and people not as familiar with git. For
> these people, squash-merging makes much more sense, and when they forget
> to check that checkbox and someone merges their work, the result is tons
> of garbage commits in the git history.
>

I do agree that state is a problem. Because we know that box exists we
press approve when the commits themselves are garbage and then we get this
mess.

Accordingly, I think squash-merging makes sense as the default value to
> avoid this. People who are comfortable with the "curated MR commit
> history" workflow will of course still be able to turn it off. IMO it
> makes more sense to ask experts to turn it off than to ask newcomers and
> novices to turn it on.
>
> Thoughts?
>

-1
New to kde doesn't mean new to git. We have a lot of skilled people, and
one of the biggest gains of adopting gitlab is that we expose git more
directly.

Imho that feature request to gitlab to set a default isn't actually what
we're after. It's requesting a bodge that replaces one problem with another.

We don't want a default for a merge option, we want an exposed action like
the existing rebase button to squash things within the local branch. That
would mean reviewers can review commits (and therefore review commit
messages properly) and you still provide an easy path for people who can't
squash locally. If we only approve when commits themselves are sound, it'll
be easy to manage. Win-win.

David

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Johan Ouwerkerk]

On Sat, Oct 3, 2020 at 10:15 AM Boudewijn Rempt  wrote:
>
> On Friday, 2 October 2020 19:39:37 CEST Nate Graham wrote:
>
> > Thoughts?
>
> Like others have said, please, no. Squashed commits are the worst things to 
> have in a git history. They make it hard to use git blame, they make it hard 
> to read the history... And the whole argument about making life easier for 
> drive-by contributors and newbies and people not familiar with git isn't to 
> the point anyway: those people will make a fork, do their work, make a merge 
> request, get that reviewed, and then the maintainer does the merge.
>

This last bit is worth emphasising strongly. The whole thing is only
at issue because in KDE we assume every developer can land their
changes anywhere and rely on an honour system to enforce that it is
properly reviewed (with the exception of sysadmin stuff). But many
drive-by contributors and newbies or even seasoned contributors won't
be deterred at all from not having commit access to the main repo.
Additionally I suspect not many people will object to a maintainer
taking their changes but landing them in different commits or a
slightly different form either.

Regards,

- Johan Ouwerkerk

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Johan Ouwerkerk]

On Sat, Oct 3, 2020 at 12:26 AM David Hurka  wrote:
>
> > > However, it remains a fairly advanced workflow which is challenging for
> > > newcomers, drive-by-developers, and people not as familiar with git. For
> > > these people, squash-merging makes much more sense, [...]
>
> This workflow is too advanced for me. My commits are usually garbage like “fix
> pipeline”, and often contain notes on what I was doing and what I still need
> to do. If I was maintaining a clean history, that wouldn’t be possible.
>

Arguing from theory on maintaining a good commit history, this
actually means you should not be squashing your commits -- you should
be using the `fixup` operation in an interactive rebase

But I understand where you are coming from. At work we used to have a
policy on squash commits for much the same reason, even professional
developers don't always bother to learn their scm tools. (I've also
worked with someone who did not appreciate the idea of commit history
and commit messages at all, as in a "why would I ever look at that?"
level of don't care).

Still others have raised a valid point that for long term maintenance
it is better to have a curated history if you can get it. That is why
we abandoned it eventually, especially for non-functional refactoring
commits or for a bunch of accumulated bug fixes squashing did more
harm than good. Essentially you get punished for doing things
'correctly' (for opinionated values of correct).

>
> Is it possible to make the default configurable for each user? Then I could
> simply check “Squash by default” and all my MRs are how I want them.
>
>

I'm not sure about that, but at least it *is* possible to do this per
repo, there's no need why projects should stick to a global default.
So if you're the maintainer you could nudge the default towards what
makes sense for your project and its dominant workflow. Go to Settings
(side bar) > General > Squashing and merging. Apparently that has been
a feature of Gitlab core since version 11.

Regards,

- Johan Ouwerkerk

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[David Hurka]

> That doesn't prevent me from having a clean history when I finally git-push
> to an opened MR, so my colleagues could easily review my code. I know that
> if I'd push some "dirty" commits to my "merge request", my colleagues would
> unnecessarily spend time navigating these commits, and reviewing code-lines
> that were fixed in the further commit. And why? Just because I didn't care
> to squash last commit?

Why should colleagues navigate through any commits, when the MR is intended to 
be squashed? Wouldn’t squash merge make it easier to review an MR?

> I should note though that while I do a lot of stuff with git as I work
> (e.g. swapping commits, amending a commit, going back to a commit in history
> then changing it, etc.), I spend minuscule amount of time on it.

I think this can not be expected from new contributors.

If the default for this checkbox is configurable per user, I would like it 
when new MRs from new contributors are squash merged by default.

New users usually submit easier patches, right? Such patches usually can not 
be separated into individual, clean commits, unless we want to push broken 
code to production branches.

If we reject a patch submitted a new user with “Thanks, your patch finally 
fixes this annoying bug, but we can not accept it, because I don’t like your 
git history. Please learn git first.”, that would IMHO be the first step to 
make KDE a closed community.

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Boudewijn Rempt]

On Friday, 2 October 2020 19:39:37 CEST Nate Graham wrote:

> Thoughts?

Like others have said, please, no. Squashed commits are the worst things to 
have in a git history. They make it hard to use git blame, they make it hard to 
read the history... And the whole argument about making life easier for 
drive-by contributors and newbies and people not familiar with git isn't to the 
point anyway: those people will make a fork, do their work, make a merge 
request, get that reviewed, and then the maintainer does the merge.

-- 
https://www.krita.org

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Konstantin Kharlamov]

On Sat, 2020-10-03 at 00:26 +0200, David Hurka wrote:
> > > However, it remains a fairly advanced workflow which is challenging for
> > > newcomers, drive-by-developers, and people not as familiar with git. For
> > > these people, squash-merging makes much more sense, [...]
> 
> This workflow is too advanced for me. My commits are usually garbage like
> “fix pipeline”, and often contain notes on what I was doing and what I still 
> need 
> to do. If I was maintaining a clean history, that wouldn’t be possible.

Everyone has such commits on more or less non-trivial changes. As a matter of
fact, ATM at dayjob I'm working on a branch, where either because of some
complexities, or because I wanted to make stuff work, and clean up it later, I
have 3 "dirty" commits on top of my branch. Two of them will eventually be
removed, and another one will be cleaned up, and squashed into the previous
commit.

That doesn't prevent me from having a clean history when I finally git-push to 
an
opened MR, so my colleagues could easily review my code. I know that if I'd push
some "dirty" commits to my "merge request", my colleagues would unnecessarily
spend time navigating these commits, and reviewing code-lines that were fixed in
the further commit. And why? Just because I didn't care to squash last commit?

I should note though that while I do a lot of stuff with git as I work
(e.g. swapping commits, amending a commit, going back to a commit in history 
then
changing it, etc.), I spend minuscule amount of time on it. To work with git
quickly you simply need to establish some workflow. From my experience the
following things help:

1. Shell autocompletion. E.g. for zsh I'm using `zsh-autosuggestions`. This
   allows you to rarely type longer commands, such as `git rebase
   upstream/master`. Since I rarely rebase on something else, once I typed `git
   r`, I have the completion I can use.
2. Having aliases to commands you use most often. For examples ones I have
   regarding git in my zshrc:
   
   alias rc="git add -u && git rebase --continue"
   alias ca="git add -u && git commit --amend -v"
   alias cax="git add -u && git commit --amend -v --no-edit"
   alias c="git add -u && git commit -v"
   alias po="git push origin HEAD"
   alias pu="git push upstream HEAD"
   alias or="git pull origin   HEAD --rebase"
   alias ur="git pull upstream HEAD --rebase"
   alias co="git checkout"

   The most often used alias here is `cax`. It often happens that you already
   have a commit on top, and you also did some newer changes that should part of
   the commit. So instead of typing out `git commit --amend`, etc, you execute
   `cax`, which is instantaneous.

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[David Hurka]

> > However, it remains a fairly advanced workflow which is challenging for
> > newcomers, drive-by-developers, and people not as familiar with git. For
> > these people, squash-merging makes much more sense, [...]

This workflow is too advanced for me. My commits are usually garbage like “fix 
pipeline”, and often contain notes on what I was doing and what I still need 
to do. If I was maintaining a clean history, that wouldn’t be possible.

Others of you give me the impression that we are looking for perfect 
developers who never make mistakes. That’s not me, so there seem to be 
different opinions.

Is it possible to make the default configurable for each user? Then I could 
simply check “Squash by default” and all my MRs are how I want them.

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Albert Astals Cid]

El divendres, 2 d’octubre de 2020, a les 21:19:02 CEST, Konstantin Kharlamov va 
escriure:
> On Fri, 2020-10-02 at 11:39 -0600, Nate Graham wrote:
> > Accordingly, I think squash-merging makes sense as the default value to
> > avoid this. People who are comfortable with the "curated MR commit
> > history" workflow will of course still be able to turn it off. IMO it
> > makes more sense to ask experts to turn it off than to ask newcomers and
> > novices to turn it on.
> 
> Please don't. This will result in having many MRs with a valid history to get
> squashed just because people would keep forgetting to uncheck it. Given there
> are much more people who aware of how to work with git history, I think this 
> would
> hurt more than occasionally merging a non-squashed MR from a newcomer.
 
+1, squash by default is bound to break more things than fix.

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Konstantin Kharlamov]

On Fri, 2020-10-02 at 11:39 -0600, Nate Graham wrote:
> Hello folks,
> I've been told that our Sysadmins have developed some tooling capable of
> checking the "Squash when merging" checkbox by default for new Merge
> Requests. This would be a downstream solution to
> https://gitlab.com/gitlab-org/gitlab/-/issues/222175.
>
> I'd like to propose that this be done as a sane default for new Merge
> Requests. Now, personally I have gotten used to the alternative "curate
> your MR's git history" approach and have written documentation for it at
>
https://community.kde.org/Infrastructure/GitLab#Curating_your_merge_request_commit_history
.
>
> However, it remains a fairly advanced workflow which is challenging for
> newcomers, drive-by-developers, and people not as familiar with git. For
> these people, squash-merging makes much more sense, and when they forget
> to check that checkbox and someone merges their work, the result is tons
> of garbage commits in the git history.

Well, what you say makes sense. But I don't think squashing commits would help
too much with having clean git history in these cases. You see, when there's an
MR with tons garbage commits, this likely also means the contributor is doing
something complicated enough that should be separated to more than one commit.
In
which case it's easy to see a problem with this squash-checkbox: a maintainer
asks the author to broke their MR to three commits per functional changed,
contributors does that, and then everyone happily forget to remove that pesky
"squash-checkbox", so upon merging all the work in separating commits gets
wasted.

It may be a question of whether that newcomer needs to learn to work with git
just to contribute. Well, I think that the fact they try to contribute means
they're a developer, whether a beginner or not. And as a developer, they have to
learn to work with the git-history. I think it's a nice moment to mention this
great blog-post on the matter by Peter Hutterer, a libinput maintainer:
http://who-t.blogspot.com/2009/12/on-commit-messages.html It may be old, but
still relevant.


> Accordingly, I think squash-merging makes sense as the default value to
> avoid this. People who are comfortable with the "curated MR commit
> history" workflow will of course still be able to turn it off. IMO it
> makes more sense to ask experts to turn it off than to ask newcomers and
> novices to turn it on.

Please don't. This will result in having many MRs with a valid history to get
squashed just because people would keep forgetting to uncheck it. Given there
are
much more people who aware of how to work with git history, I think this would
hurt more than occasionally merging a non-squashed MR from a newcomer.

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[Francis Herne]

On Friday, 2 October 2020 18:39:37 BST Nate Graham wrote:
> Hello folks,
> I've been told that our Sysadmins have developed some tooling capable of 
> checking the "Squash when merging" checkbox by default for new Merge 
> Requests. This would be a downstream solution to 
> https://gitlab.com/gitlab-org/gitlab/-/issues/222175.
> 
> I'd like to propose that this be done as a sane default for new Merge 
> Requests. Now, personally I have gotten used to the alternative "curate 
> your MR's git history" approach and have written documentation for it at 
> https://community.kde.org/Infrastructure/GitLab#Curating_your_merge_request_commit_history.
> 
> However, it remains a fairly advanced workflow which is challenging for 
> newcomers, drive-by-developers, and people not as familiar with git. For 
> these people, squash-merging makes much more sense, and when they forget 
> to check that checkbox and someone merges their work, the result is tons 
> of garbage commits in the git history.
> 
> Accordingly, I think squash-merging makes sense as the default value to 
> avoid this. People who are comfortable with the "curated MR commit 
> history" workflow will of course still be able to turn it off. IMO it 
> makes more sense to ask experts to turn it off than to ask newcomers and 
> novices to turn it on.
> 
> Thoughts?
> 
> Nate
> 

I think this would be a mistake.

KDE projects are typically long-lived, with many contributors and
considerable turnover in those contributors over the years. Many of them are
also quite large.

Maintaining a clear history with good commit messages is crucial to keeping
our codebases maintainable. We already have many problems with legacy
components that no current maintainer really understands.

Small, logically-separated and labelled commits are a great help in many
situations, either bisecting to find recent bugs or when using `git blame` to
find out why some obscure line of code was added ten years ago.

We should consider "maintaining a sane commit history" as a fundamental skill
required of contributors -- of course, one that existing developers can help 
with.

Just as we wouldn't accept a change as-is that copy-pasted lots of code to add
some new feature, but give review feedback on how to generalize the original,
we shouldn't accept merges with 'tons of garbage commits' and instead explain
how to do things properly.

Squash-merging *hides* the problem of uncared-for commit messages,
by creating large and poorly-described commits that will cause technical debt
going forward.
It doesn't *solve* it, and it actively *discourages* the kind of merges
we should actually be encouraging.

My thoughts only, of course.

-Francis H

Re: Proposal: make squash-merging the default behavior for gitlab MRs

[David Hurka]

> [...]
> I've been told that our Sysadmins have developed some tooling capable of
> checking the "Squash when merging" checkbox by default for new Merge
> Requests. [...]
> 
> I'd like to propose [squash-merge] as a sane default for new Merge
> Requests.
> 
> [...]
> Thoughts?

Yes, I would like that, it saves me one or two clicks per month. :)

Probably it will save additional one or two MRs per month that just repeat 
another MR that just misses this option enabled. Related to the this, is it 
possible to check the "Allow contributions" checkbox by default?