You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@daffodil.apache.org by "Beckerle, Mike" <mb...@tresys.com> on 2019/12/11 13:02:22 UTC
merge without squash - fix or leave it?
I neglected to squash the two commits together before merging the daffodil-2242-tunable branch, which is the standard for our workflow.
Should I fix and force push, or just leave it? I.e., which is the greater sin, to not squash and litter the history, or force push to master?
________________________________________
From: mbeckerle@apache.org <mb...@apache.org>
Sent: Wednesday, December 11, 2019 7:54 AM
To: commits@daffodil.apache.org
Subject: [incubator-daffodil] 02/02: Fix 2.11 scala compile issue.
This is an automated email from the ASF dual-hosted git repository.
mbeckerle pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-daffodil.git
commit 7aaabca399367b59f1eb36503d8510ed71d1e11b
Author: Michael Beckerle <mb...@tresys.com>
AuthorDate: Tue Dec 10 13:19:37 2019 -0500
Fix 2.11 scala compile issue.
Recursive definition needed type in 2.11. Somehow 2.12 does without
this.
DAFFODIL-2242
---
.../src/main/scala/org/apache/daffodil/dpath/Expression.scala | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala b/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
index eb135fd..fcf666f 100644
--- a/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
+++ b/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
@@ -39,6 +39,7 @@ import org.apache.daffodil.BasicComponent
import org.apache.daffodil.api.DaffodilTunables
import org.apache.daffodil.oolag.OOLAG.OOLAGHostImpl
import org.apache.daffodil.oolag.OOLAG.OOLAGHost
+import org.apache.daffodil.api.UnqualifiedPathStepPolicy
/**
* Root class of the type hierarchy for the AST nodes used when we
@@ -62,8 +63,8 @@ abstract class Expression extends OOLAGHostImpl()
requiredEvaluations(isTypeCorrect)
requiredEvaluations(compiledDPath_)
- override lazy val tunable = parent.tunable
- override lazy val unqualifiedPathStepPolicy = parent.unqualifiedPathStepPolicy
+ override lazy val tunable: DaffodilTunables = parent.tunable
+ override lazy val unqualifiedPathStepPolicy: UnqualifiedPathStepPolicy = parent.unqualifiedPathStepPolicy
/**
* Override where we traverse/access elements.
*/
@@ -575,8 +576,9 @@ case class WholeExpression(
host: BasicComponent)
extends Expression {
- final override lazy val tunable = host.tunable
- final override lazy val unqualifiedPathStepPolicy = host.unqualifiedPathStepPolicy
+ final override lazy val tunable: DaffodilTunables = host.tunable
+ final override lazy val unqualifiedPathStepPolicy : UnqualifiedPathStepPolicy
+ = host.unqualifiedPathStepPolicy
def init() {
this.setOOLAGContext(host) // we are the root of expression, but we propagate diagnostics further.
Re: AW: merge without squash - fix or leave it?
Posted by Steve Lawrence <sl...@apache.org>.
There a few reasons why I personally prefer squashing:
1) Single commits make it easier to see what was done to add a
feature/fix a bug. This can be useful when added a feature that's
similar to another feature. For example, say someone added feature A
spread out across multiple commits, and then someone else wanted to add
feature B that was very similar. It would be useful to be able to look
at the diff for feature A to figure out exactly what changes were
needed. But if that was spread out across multiple commits that can be
difficult. You would need to figure out which commits were part of
feature A and view those commits as one diff. Not impossible, but just
more difficult.
2) Single commits make it easier to undo/revert changes. This hopefully
doesn't happen often, but if something goes wrong and you realize a pull
request completely broke everything after being merged, sometimes the
best solution is to just revert it. If it's split across multiple
commits, you need to revert all of them. Just more work to figure out
which commits need to be reverted if it's possible there's more than one.
3) Single commits make it easier to bisect history. Sometimes we realize
there's a bug or performance degradation well after a pull request is
merged, but have no idea which commit added the bug. Using git bisect
can be a good way to figure out which commit created the issue, and we
can look at the diff to try to narrow down specifically what caused it.
But it's not uncommon for the first commit of a pull requests to fail to
compile or tests to fail and a later commit in the PR fixes it. These
broken commits make bisecting a bit more difficult since certain commits
just don't work. By squashing, we have a better chance that every commit
compiles and tests pass, which makes git bisect easier to use.
4) Maybe most importantly to me personaly, it just makes the commit
history easier to look at and reason about. Without squashing, we'd
probably see a history like:
Add feature A
Add missing semicolon so it compiles
Fix test I accidentally broke
Reword comment to make more sense
None of that is really useful to have in the history and just gets in
the way if you're trying to look at what was recently changed. All we
really want in the history is the fact that you added a new feature A.
That fact that you forgot a semicolon or fixed a comment isn't really
important to point out as a single change--it's just all part of adding
the new feature. Squashing these kinds of changes (which is probably the
most common case of extra commits we see in pull requests) just makes
the history more useful.
5) I think it actually makes git blame more useful. Using the example
above, git blame might say a certain line was changed by the "Add
missing semicolon so it compiles" commit. That's really just not that
useful. We really wanted to know that the line was added as part of
added feature A. By squashing, we hiding those kinds of things that
don't matter.
I'll add that there are definitely reasons not to squash sometimes. For
example, maybe we're making a giant change, and it's easier to reason
about if it's split up into multiple steps. In that case, perhaps we
would want to review it as a single pull request, but we might want to
keep the story that multiple commits tells of how the giant change was
made. I don't think this is a hard rule that we must always squash, but
in the majority of cases, it makes our lives easier down the road.
On 12/13/19 3:18 AM, Christofer Dutz wrote:
> Hi all,
>
> May I ask why you want to squash anyway?
> I usually try to commit often and write commit comments on what I did. This way when using git blame you sort of get an explanation why a line is the way it is. If you sqasch everything this information gets lost.
>
> So what's the benefit of squashing anyway?
>
> Chris
> ________________________________
> Von: Julian Feinauer <j....@pragmaticminds.de>
> Gesendet: Freitag, 13. Dezember 2019 07:39:43
> An: dev@daffodil.apache.org <de...@daffodil.apache.org>
> Betreff: Re: merge without squash - fix or leave it?
>
> Hi all,
>
> I suggest to not rely on features e.g. from github as this is just a "mirror" for us as ASF and we should only rely on our own infra for our processes.
> I think a force push to develop / master branch should not be a regular process but if it happens once or twice WITH proper discussion on the list its fine.
> We also had that several times in the beginning of the PLC4X project when things were new and some changes rather breaking.
>
> But, I also think its not to bad to have some more commits in the history. This is something you will also observe is harder to keep as soon as the number of comitters (hopefully) grows. So its probably best to already start getting used to it.
>
> Julian
>
> Am 12.12.19, 16:00 schrieb "Beckerle, Mike" <mb...@tresys.com>:
>
> Remembering to push new changes with "autosquash" is one more thing to forget to do, just like forgetting to squash them was in the first place.
>
> So I'm not sure this improves the situation unless we can require commits to a PR to be pushed in this way. (which would be yet another hook?)
>
> We want a hook that just says PRs with more than 1 commit cannot be merged. That prevents the error.
>
>
>
>
> ________________________________
> From: Steve Lawrence <sl...@apache.org>
> Sent: Thursday, December 12, 2019 7:17 AM
> To: dev@daffodil.apache.org <de...@daffodil.apache.org>
> Subject: Re: merge without squash - fix or leave it?
>
> Looking into the GitHub actions solution a bit, I found this GitHub action:
>
> https://github.com/xt0rted/block-autosquash-commits-action
>
> It's not exactly what I described, but when added to our config it will
> show a build failure (similar to a test failing) if a pull request has
> any *autosquash* commits.
>
> So one option would be to enable this and then adopt a workflow where
> commits added to a pull request should be created with either "git
> commit --fixup HEAD" or "git commit --squash HEAD". This will mark the
> new commit as an autosquash commit and it will be more visible when it's
> time to merge.
>
> Thoughts?
>
> On 12/11/19 8:08 AM, Steve Lawrence wrote:
> > Considering our user base is still fairly small and you just committed
> > it, I don't think it's unreasonable to force push to have a clean git
> > commit history.
> >
> > But as the project grows, this is something we'll likely want to avoid.
> >
> > I think there's a way to use the new github actions feature to add
> > custom checks before allowing a merge, similar to the CI stuff we do
> > now. To prevent this in the future, I imagine it wouldn't be hard to add
> > a check that won't allow merging if there are more than one commit in
> > the PR.
> >
> > On 12/11/19 8:02 AM, Beckerle, Mike wrote:
> >> I neglected to squash the two commits together before merging the daffodil-2242-tunable branch, which is the standard for our workflow.
> >>
> >> Should I fix and force push, or just leave it? I.e., which is the greater sin, to not squash and litter the history, or force push to master?
> >>
> >> ________________________________________
> >> From: mbeckerle@apache.org <mb...@apache.org>
> >> Sent: Wednesday, December 11, 2019 7:54 AM
> >> To: commits@daffodil.apache.org
> >> Subject: [incubator-daffodil] 02/02: Fix 2.11 scala compile issue.
> >>
> >> This is an automated email from the ASF dual-hosted git repository.
> >>
> >> mbeckerle pushed a commit to branch master
> >> in repository https://gitbox.apache.org/repos/asf/incubator-daffodil.git
> >>
> >> commit 7aaabca399367b59f1eb36503d8510ed71d1e11b
> >> Author: Michael Beckerle <mb...@tresys.com>
> >> AuthorDate: Tue Dec 10 13:19:37 2019 -0500
> >>
> >> Fix 2.11 scala compile issue.
> >>
> >> Recursive definition needed type in 2.11. Somehow 2.12 does without
> >> this.
> >>
> >> DAFFODIL-2242
> >> ---
> >> .../src/main/scala/org/apache/daffodil/dpath/Expression.scala | 10 ++++++----
> >> 1 file changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala b/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
> >> index eb135fd..fcf666f 100644
> >> --- a/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
> >> +++ b/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
> >> @@ -39,6 +39,7 @@ import org.apache.daffodil.BasicComponent
> >> import org.apache.daffodil.api.DaffodilTunables
> >> import org.apache.daffodil.oolag.OOLAG.OOLAGHostImpl
> >> import org.apache.daffodil.oolag.OOLAG.OOLAGHost
> >> +import org.apache.daffodil.api.UnqualifiedPathStepPolicy
> >>
> >> /**
> >> * Root class of the type hierarchy for the AST nodes used when we
> >> @@ -62,8 +63,8 @@ abstract class Expression extends OOLAGHostImpl()
> >> requiredEvaluations(isTypeCorrect)
> >> requiredEvaluations(compiledDPath_)
> >>
> >> - override lazy val tunable = parent.tunable
> >> - override lazy val unqualifiedPathStepPolicy = parent.unqualifiedPathStepPolicy
> >> + override lazy val tunable: DaffodilTunables = parent.tunable
> >> + override lazy val unqualifiedPathStepPolicy: UnqualifiedPathStepPolicy = parent.unqualifiedPathStepPolicy
> >> /**
> >> * Override where we traverse/access elements.
> >> */
> >> @@ -575,8 +576,9 @@ case class WholeExpression(
> >> host: BasicComponent)
> >> extends Expression {
> >>
> >> - final override lazy val tunable = host.tunable
> >> - final override lazy val unqualifiedPathStepPolicy = host.unqualifiedPathStepPolicy
> >> + final override lazy val tunable: DaffodilTunables = host.tunable
> >> + final override lazy val unqualifiedPathStepPolicy : UnqualifiedPathStepPolicy
> >> + = host.unqualifiedPathStepPolicy
> >>
> >> def init() {
> >> this.setOOLAGContext(host) // we are the root of expression, but we propagate diagnostics further.
> >>
> >>
> >
>
>
>
AW: merge without squash - fix or leave it?
Posted by Christofer Dutz <ch...@c-ware.de>.
Hi all,
May I ask why you want to squash anyway?
I usually try to commit often and write commit comments on what I did. This way when using git blame you sort of get an explanation why a line is the way it is. If you sqasch everything this information gets lost.
So what's the benefit of squashing anyway?
Chris
________________________________
Von: Julian Feinauer <j....@pragmaticminds.de>
Gesendet: Freitag, 13. Dezember 2019 07:39:43
An: dev@daffodil.apache.org <de...@daffodil.apache.org>
Betreff: Re: merge without squash - fix or leave it?
Hi all,
I suggest to not rely on features e.g. from github as this is just a "mirror" for us as ASF and we should only rely on our own infra for our processes.
I think a force push to develop / master branch should not be a regular process but if it happens once or twice WITH proper discussion on the list its fine.
We also had that several times in the beginning of the PLC4X project when things were new and some changes rather breaking.
But, I also think its not to bad to have some more commits in the history. This is something you will also observe is harder to keep as soon as the number of comitters (hopefully) grows. So its probably best to already start getting used to it.
Julian
Am 12.12.19, 16:00 schrieb "Beckerle, Mike" <mb...@tresys.com>:
Remembering to push new changes with "autosquash" is one more thing to forget to do, just like forgetting to squash them was in the first place.
So I'm not sure this improves the situation unless we can require commits to a PR to be pushed in this way. (which would be yet another hook?)
We want a hook that just says PRs with more than 1 commit cannot be merged. That prevents the error.
________________________________
From: Steve Lawrence <sl...@apache.org>
Sent: Thursday, December 12, 2019 7:17 AM
To: dev@daffodil.apache.org <de...@daffodil.apache.org>
Subject: Re: merge without squash - fix or leave it?
Looking into the GitHub actions solution a bit, I found this GitHub action:
https://github.com/xt0rted/block-autosquash-commits-action
It's not exactly what I described, but when added to our config it will
show a build failure (similar to a test failing) if a pull request has
any *autosquash* commits.
So one option would be to enable this and then adopt a workflow where
commits added to a pull request should be created with either "git
commit --fixup HEAD" or "git commit --squash HEAD". This will mark the
new commit as an autosquash commit and it will be more visible when it's
time to merge.
Thoughts?
On 12/11/19 8:08 AM, Steve Lawrence wrote:
> Considering our user base is still fairly small and you just committed
> it, I don't think it's unreasonable to force push to have a clean git
> commit history.
>
> But as the project grows, this is something we'll likely want to avoid.
>
> I think there's a way to use the new github actions feature to add
> custom checks before allowing a merge, similar to the CI stuff we do
> now. To prevent this in the future, I imagine it wouldn't be hard to add
> a check that won't allow merging if there are more than one commit in
> the PR.
>
> On 12/11/19 8:02 AM, Beckerle, Mike wrote:
>> I neglected to squash the two commits together before merging the daffodil-2242-tunable branch, which is the standard for our workflow.
>>
>> Should I fix and force push, or just leave it? I.e., which is the greater sin, to not squash and litter the history, or force push to master?
>>
>> ________________________________________
>> From: mbeckerle@apache.org <mb...@apache.org>
>> Sent: Wednesday, December 11, 2019 7:54 AM
>> To: commits@daffodil.apache.org
>> Subject: [incubator-daffodil] 02/02: Fix 2.11 scala compile issue.
>>
>> This is an automated email from the ASF dual-hosted git repository.
>>
>> mbeckerle pushed a commit to branch master
>> in repository https://gitbox.apache.org/repos/asf/incubator-daffodil.git
>>
>> commit 7aaabca399367b59f1eb36503d8510ed71d1e11b
>> Author: Michael Beckerle <mb...@tresys.com>
>> AuthorDate: Tue Dec 10 13:19:37 2019 -0500
>>
>> Fix 2.11 scala compile issue.
>>
>> Recursive definition needed type in 2.11. Somehow 2.12 does without
>> this.
>>
>> DAFFODIL-2242
>> ---
>> .../src/main/scala/org/apache/daffodil/dpath/Expression.scala | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala b/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
>> index eb135fd..fcf666f 100644
>> --- a/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
>> +++ b/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
>> @@ -39,6 +39,7 @@ import org.apache.daffodil.BasicComponent
>> import org.apache.daffodil.api.DaffodilTunables
>> import org.apache.daffodil.oolag.OOLAG.OOLAGHostImpl
>> import org.apache.daffodil.oolag.OOLAG.OOLAGHost
>> +import org.apache.daffodil.api.UnqualifiedPathStepPolicy
>>
>> /**
>> * Root class of the type hierarchy for the AST nodes used when we
>> @@ -62,8 +63,8 @@ abstract class Expression extends OOLAGHostImpl()
>> requiredEvaluations(isTypeCorrect)
>> requiredEvaluations(compiledDPath_)
>>
>> - override lazy val tunable = parent.tunable
>> - override lazy val unqualifiedPathStepPolicy = parent.unqualifiedPathStepPolicy
>> + override lazy val tunable: DaffodilTunables = parent.tunable
>> + override lazy val unqualifiedPathStepPolicy: UnqualifiedPathStepPolicy = parent.unqualifiedPathStepPolicy
>> /**
>> * Override where we traverse/access elements.
>> */
>> @@ -575,8 +576,9 @@ case class WholeExpression(
>> host: BasicComponent)
>> extends Expression {
>>
>> - final override lazy val tunable = host.tunable
>> - final override lazy val unqualifiedPathStepPolicy = host.unqualifiedPathStepPolicy
>> + final override lazy val tunable: DaffodilTunables = host.tunable
>> + final override lazy val unqualifiedPathStepPolicy : UnqualifiedPathStepPolicy
>> + = host.unqualifiedPathStepPolicy
>>
>> def init() {
>> this.setOOLAGContext(host) // we are the root of expression, but we propagate diagnostics further.
>>
>>
>
Re: merge without squash - fix or leave it?
Posted by Julian Feinauer <j....@pragmaticminds.de>.
Hi all,
I suggest to not rely on features e.g. from github as this is just a "mirror" for us as ASF and we should only rely on our own infra for our processes.
I think a force push to develop / master branch should not be a regular process but if it happens once or twice WITH proper discussion on the list its fine.
We also had that several times in the beginning of the PLC4X project when things were new and some changes rather breaking.
But, I also think its not to bad to have some more commits in the history. This is something you will also observe is harder to keep as soon as the number of comitters (hopefully) grows. So its probably best to already start getting used to it.
Julian
Am 12.12.19, 16:00 schrieb "Beckerle, Mike" <mb...@tresys.com>:
Remembering to push new changes with "autosquash" is one more thing to forget to do, just like forgetting to squash them was in the first place.
So I'm not sure this improves the situation unless we can require commits to a PR to be pushed in this way. (which would be yet another hook?)
We want a hook that just says PRs with more than 1 commit cannot be merged. That prevents the error.
________________________________
From: Steve Lawrence <sl...@apache.org>
Sent: Thursday, December 12, 2019 7:17 AM
To: dev@daffodil.apache.org <de...@daffodil.apache.org>
Subject: Re: merge without squash - fix or leave it?
Looking into the GitHub actions solution a bit, I found this GitHub action:
https://github.com/xt0rted/block-autosquash-commits-action
It's not exactly what I described, but when added to our config it will
show a build failure (similar to a test failing) if a pull request has
any *autosquash* commits.
So one option would be to enable this and then adopt a workflow where
commits added to a pull request should be created with either "git
commit --fixup HEAD" or "git commit --squash HEAD". This will mark the
new commit as an autosquash commit and it will be more visible when it's
time to merge.
Thoughts?
On 12/11/19 8:08 AM, Steve Lawrence wrote:
> Considering our user base is still fairly small and you just committed
> it, I don't think it's unreasonable to force push to have a clean git
> commit history.
>
> But as the project grows, this is something we'll likely want to avoid.
>
> I think there's a way to use the new github actions feature to add
> custom checks before allowing a merge, similar to the CI stuff we do
> now. To prevent this in the future, I imagine it wouldn't be hard to add
> a check that won't allow merging if there are more than one commit in
> the PR.
>
> On 12/11/19 8:02 AM, Beckerle, Mike wrote:
>> I neglected to squash the two commits together before merging the daffodil-2242-tunable branch, which is the standard for our workflow.
>>
>> Should I fix and force push, or just leave it? I.e., which is the greater sin, to not squash and litter the history, or force push to master?
>>
>> ________________________________________
>> From: mbeckerle@apache.org <mb...@apache.org>
>> Sent: Wednesday, December 11, 2019 7:54 AM
>> To: commits@daffodil.apache.org
>> Subject: [incubator-daffodil] 02/02: Fix 2.11 scala compile issue.
>>
>> This is an automated email from the ASF dual-hosted git repository.
>>
>> mbeckerle pushed a commit to branch master
>> in repository https://gitbox.apache.org/repos/asf/incubator-daffodil.git
>>
>> commit 7aaabca399367b59f1eb36503d8510ed71d1e11b
>> Author: Michael Beckerle <mb...@tresys.com>
>> AuthorDate: Tue Dec 10 13:19:37 2019 -0500
>>
>> Fix 2.11 scala compile issue.
>>
>> Recursive definition needed type in 2.11. Somehow 2.12 does without
>> this.
>>
>> DAFFODIL-2242
>> ---
>> .../src/main/scala/org/apache/daffodil/dpath/Expression.scala | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala b/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
>> index eb135fd..fcf666f 100644
>> --- a/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
>> +++ b/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
>> @@ -39,6 +39,7 @@ import org.apache.daffodil.BasicComponent
>> import org.apache.daffodil.api.DaffodilTunables
>> import org.apache.daffodil.oolag.OOLAG.OOLAGHostImpl
>> import org.apache.daffodil.oolag.OOLAG.OOLAGHost
>> +import org.apache.daffodil.api.UnqualifiedPathStepPolicy
>>
>> /**
>> * Root class of the type hierarchy for the AST nodes used when we
>> @@ -62,8 +63,8 @@ abstract class Expression extends OOLAGHostImpl()
>> requiredEvaluations(isTypeCorrect)
>> requiredEvaluations(compiledDPath_)
>>
>> - override lazy val tunable = parent.tunable
>> - override lazy val unqualifiedPathStepPolicy = parent.unqualifiedPathStepPolicy
>> + override lazy val tunable: DaffodilTunables = parent.tunable
>> + override lazy val unqualifiedPathStepPolicy: UnqualifiedPathStepPolicy = parent.unqualifiedPathStepPolicy
>> /**
>> * Override where we traverse/access elements.
>> */
>> @@ -575,8 +576,9 @@ case class WholeExpression(
>> host: BasicComponent)
>> extends Expression {
>>
>> - final override lazy val tunable = host.tunable
>> - final override lazy val unqualifiedPathStepPolicy = host.unqualifiedPathStepPolicy
>> + final override lazy val tunable: DaffodilTunables = host.tunable
>> + final override lazy val unqualifiedPathStepPolicy : UnqualifiedPathStepPolicy
>> + = host.unqualifiedPathStepPolicy
>>
>> def init() {
>> this.setOOLAGContext(host) // we are the root of expression, but we propagate diagnostics further.
>>
>>
>
Re: merge without squash - fix or leave it?
Posted by "Beckerle, Mike" <mb...@tresys.com>.
Remembering to push new changes with "autosquash" is one more thing to forget to do, just like forgetting to squash them was in the first place.
So I'm not sure this improves the situation unless we can require commits to a PR to be pushed in this way. (which would be yet another hook?)
We want a hook that just says PRs with more than 1 commit cannot be merged. That prevents the error.
________________________________
From: Steve Lawrence <sl...@apache.org>
Sent: Thursday, December 12, 2019 7:17 AM
To: dev@daffodil.apache.org <de...@daffodil.apache.org>
Subject: Re: merge without squash - fix or leave it?
Looking into the GitHub actions solution a bit, I found this GitHub action:
https://github.com/xt0rted/block-autosquash-commits-action
It's not exactly what I described, but when added to our config it will
show a build failure (similar to a test failing) if a pull request has
any *autosquash* commits.
So one option would be to enable this and then adopt a workflow where
commits added to a pull request should be created with either "git
commit --fixup HEAD" or "git commit --squash HEAD". This will mark the
new commit as an autosquash commit and it will be more visible when it's
time to merge.
Thoughts?
On 12/11/19 8:08 AM, Steve Lawrence wrote:
> Considering our user base is still fairly small and you just committed
> it, I don't think it's unreasonable to force push to have a clean git
> commit history.
>
> But as the project grows, this is something we'll likely want to avoid.
>
> I think there's a way to use the new github actions feature to add
> custom checks before allowing a merge, similar to the CI stuff we do
> now. To prevent this in the future, I imagine it wouldn't be hard to add
> a check that won't allow merging if there are more than one commit in
> the PR.
>
> On 12/11/19 8:02 AM, Beckerle, Mike wrote:
>> I neglected to squash the two commits together before merging the daffodil-2242-tunable branch, which is the standard for our workflow.
>>
>> Should I fix and force push, or just leave it? I.e., which is the greater sin, to not squash and litter the history, or force push to master?
>>
>> ________________________________________
>> From: mbeckerle@apache.org <mb...@apache.org>
>> Sent: Wednesday, December 11, 2019 7:54 AM
>> To: commits@daffodil.apache.org
>> Subject: [incubator-daffodil] 02/02: Fix 2.11 scala compile issue.
>>
>> This is an automated email from the ASF dual-hosted git repository.
>>
>> mbeckerle pushed a commit to branch master
>> in repository https://gitbox.apache.org/repos/asf/incubator-daffodil.git
>>
>> commit 7aaabca399367b59f1eb36503d8510ed71d1e11b
>> Author: Michael Beckerle <mb...@tresys.com>
>> AuthorDate: Tue Dec 10 13:19:37 2019 -0500
>>
>> Fix 2.11 scala compile issue.
>>
>> Recursive definition needed type in 2.11. Somehow 2.12 does without
>> this.
>>
>> DAFFODIL-2242
>> ---
>> .../src/main/scala/org/apache/daffodil/dpath/Expression.scala | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala b/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
>> index eb135fd..fcf666f 100644
>> --- a/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
>> +++ b/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
>> @@ -39,6 +39,7 @@ import org.apache.daffodil.BasicComponent
>> import org.apache.daffodil.api.DaffodilTunables
>> import org.apache.daffodil.oolag.OOLAG.OOLAGHostImpl
>> import org.apache.daffodil.oolag.OOLAG.OOLAGHost
>> +import org.apache.daffodil.api.UnqualifiedPathStepPolicy
>>
>> /**
>> * Root class of the type hierarchy for the AST nodes used when we
>> @@ -62,8 +63,8 @@ abstract class Expression extends OOLAGHostImpl()
>> requiredEvaluations(isTypeCorrect)
>> requiredEvaluations(compiledDPath_)
>>
>> - override lazy val tunable = parent.tunable
>> - override lazy val unqualifiedPathStepPolicy = parent.unqualifiedPathStepPolicy
>> + override lazy val tunable: DaffodilTunables = parent.tunable
>> + override lazy val unqualifiedPathStepPolicy: UnqualifiedPathStepPolicy = parent.unqualifiedPathStepPolicy
>> /**
>> * Override where we traverse/access elements.
>> */
>> @@ -575,8 +576,9 @@ case class WholeExpression(
>> host: BasicComponent)
>> extends Expression {
>>
>> - final override lazy val tunable = host.tunable
>> - final override lazy val unqualifiedPathStepPolicy = host.unqualifiedPathStepPolicy
>> + final override lazy val tunable: DaffodilTunables = host.tunable
>> + final override lazy val unqualifiedPathStepPolicy : UnqualifiedPathStepPolicy
>> + = host.unqualifiedPathStepPolicy
>>
>> def init() {
>> this.setOOLAGContext(host) // we are the root of expression, but we propagate diagnostics further.
>>
>>
>
Re: merge without squash - fix or leave it?
Posted by Steve Lawrence <sl...@apache.org>.
Looking into the GitHub actions solution a bit, I found this GitHub action:
https://github.com/xt0rted/block-autosquash-commits-action
It's not exactly what I described, but when added to our config it will
show a build failure (similar to a test failing) if a pull request has
any *autosquash* commits.
So one option would be to enable this and then adopt a workflow where
commits added to a pull request should be created with either "git
commit --fixup HEAD" or "git commit --squash HEAD". This will mark the
new commit as an autosquash commit and it will be more visible when it's
time to merge.
Thoughts?
On 12/11/19 8:08 AM, Steve Lawrence wrote:
> Considering our user base is still fairly small and you just committed
> it, I don't think it's unreasonable to force push to have a clean git
> commit history.
>
> But as the project grows, this is something we'll likely want to avoid.
>
> I think there's a way to use the new github actions feature to add
> custom checks before allowing a merge, similar to the CI stuff we do
> now. To prevent this in the future, I imagine it wouldn't be hard to add
> a check that won't allow merging if there are more than one commit in
> the PR.
>
> On 12/11/19 8:02 AM, Beckerle, Mike wrote:
>> I neglected to squash the two commits together before merging the daffodil-2242-tunable branch, which is the standard for our workflow.
>>
>> Should I fix and force push, or just leave it? I.e., which is the greater sin, to not squash and litter the history, or force push to master?
>>
>> ________________________________________
>> From: mbeckerle@apache.org <mb...@apache.org>
>> Sent: Wednesday, December 11, 2019 7:54 AM
>> To: commits@daffodil.apache.org
>> Subject: [incubator-daffodil] 02/02: Fix 2.11 scala compile issue.
>>
>> This is an automated email from the ASF dual-hosted git repository.
>>
>> mbeckerle pushed a commit to branch master
>> in repository https://gitbox.apache.org/repos/asf/incubator-daffodil.git
>>
>> commit 7aaabca399367b59f1eb36503d8510ed71d1e11b
>> Author: Michael Beckerle <mb...@tresys.com>
>> AuthorDate: Tue Dec 10 13:19:37 2019 -0500
>>
>> Fix 2.11 scala compile issue.
>>
>> Recursive definition needed type in 2.11. Somehow 2.12 does without
>> this.
>>
>> DAFFODIL-2242
>> ---
>> .../src/main/scala/org/apache/daffodil/dpath/Expression.scala | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala b/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
>> index eb135fd..fcf666f 100644
>> --- a/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
>> +++ b/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
>> @@ -39,6 +39,7 @@ import org.apache.daffodil.BasicComponent
>> import org.apache.daffodil.api.DaffodilTunables
>> import org.apache.daffodil.oolag.OOLAG.OOLAGHostImpl
>> import org.apache.daffodil.oolag.OOLAG.OOLAGHost
>> +import org.apache.daffodil.api.UnqualifiedPathStepPolicy
>>
>> /**
>> * Root class of the type hierarchy for the AST nodes used when we
>> @@ -62,8 +63,8 @@ abstract class Expression extends OOLAGHostImpl()
>> requiredEvaluations(isTypeCorrect)
>> requiredEvaluations(compiledDPath_)
>>
>> - override lazy val tunable = parent.tunable
>> - override lazy val unqualifiedPathStepPolicy = parent.unqualifiedPathStepPolicy
>> + override lazy val tunable: DaffodilTunables = parent.tunable
>> + override lazy val unqualifiedPathStepPolicy: UnqualifiedPathStepPolicy = parent.unqualifiedPathStepPolicy
>> /**
>> * Override where we traverse/access elements.
>> */
>> @@ -575,8 +576,9 @@ case class WholeExpression(
>> host: BasicComponent)
>> extends Expression {
>>
>> - final override lazy val tunable = host.tunable
>> - final override lazy val unqualifiedPathStepPolicy = host.unqualifiedPathStepPolicy
>> + final override lazy val tunable: DaffodilTunables = host.tunable
>> + final override lazy val unqualifiedPathStepPolicy : UnqualifiedPathStepPolicy
>> + = host.unqualifiedPathStepPolicy
>>
>> def init() {
>> this.setOOLAGContext(host) // we are the root of expression, but we propagate diagnostics further.
>>
>>
>
Re: merge without squash - fix or leave it?
Posted by Steve Lawrence <sl...@apache.org>.
Considering our user base is still fairly small and you just committed
it, I don't think it's unreasonable to force push to have a clean git
commit history.
But as the project grows, this is something we'll likely want to avoid.
I think there's a way to use the new github actions feature to add
custom checks before allowing a merge, similar to the CI stuff we do
now. To prevent this in the future, I imagine it wouldn't be hard to add
a check that won't allow merging if there are more than one commit in
the PR.
On 12/11/19 8:02 AM, Beckerle, Mike wrote:
> I neglected to squash the two commits together before merging the daffodil-2242-tunable branch, which is the standard for our workflow.
>
> Should I fix and force push, or just leave it? I.e., which is the greater sin, to not squash and litter the history, or force push to master?
>
> ________________________________________
> From: mbeckerle@apache.org <mb...@apache.org>
> Sent: Wednesday, December 11, 2019 7:54 AM
> To: commits@daffodil.apache.org
> Subject: [incubator-daffodil] 02/02: Fix 2.11 scala compile issue.
>
> This is an automated email from the ASF dual-hosted git repository.
>
> mbeckerle pushed a commit to branch master
> in repository https://gitbox.apache.org/repos/asf/incubator-daffodil.git
>
> commit 7aaabca399367b59f1eb36503d8510ed71d1e11b
> Author: Michael Beckerle <mb...@tresys.com>
> AuthorDate: Tue Dec 10 13:19:37 2019 -0500
>
> Fix 2.11 scala compile issue.
>
> Recursive definition needed type in 2.11. Somehow 2.12 does without
> this.
>
> DAFFODIL-2242
> ---
> .../src/main/scala/org/apache/daffodil/dpath/Expression.scala | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala b/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
> index eb135fd..fcf666f 100644
> --- a/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
> +++ b/daffodil-core/src/main/scala/org/apache/daffodil/dpath/Expression.scala
> @@ -39,6 +39,7 @@ import org.apache.daffodil.BasicComponent
> import org.apache.daffodil.api.DaffodilTunables
> import org.apache.daffodil.oolag.OOLAG.OOLAGHostImpl
> import org.apache.daffodil.oolag.OOLAG.OOLAGHost
> +import org.apache.daffodil.api.UnqualifiedPathStepPolicy
>
> /**
> * Root class of the type hierarchy for the AST nodes used when we
> @@ -62,8 +63,8 @@ abstract class Expression extends OOLAGHostImpl()
> requiredEvaluations(isTypeCorrect)
> requiredEvaluations(compiledDPath_)
>
> - override lazy val tunable = parent.tunable
> - override lazy val unqualifiedPathStepPolicy = parent.unqualifiedPathStepPolicy
> + override lazy val tunable: DaffodilTunables = parent.tunable
> + override lazy val unqualifiedPathStepPolicy: UnqualifiedPathStepPolicy = parent.unqualifiedPathStepPolicy
> /**
> * Override where we traverse/access elements.
> */
> @@ -575,8 +576,9 @@ case class WholeExpression(
> host: BasicComponent)
> extends Expression {
>
> - final override lazy val tunable = host.tunable
> - final override lazy val unqualifiedPathStepPolicy = host.unqualifiedPathStepPolicy
> + final override lazy val tunable: DaffodilTunables = host.tunable
> + final override lazy val unqualifiedPathStepPolicy : UnqualifiedPathStepPolicy
> + = host.unqualifiedPathStepPolicy
>
> def init() {
> this.setOOLAGContext(host) // we are the root of expression, but we propagate diagnostics further.
>
>