PR for this: https://github.com/apache/beam/pull/7129
On Tue, Oct 16, 2018 at 11:40 AM Robert Bradshaw
wrote:
> Thanks for bringing this to a conclusion.
>
> On Mon, Oct 15, 2018 at 6:18 PM Thomas Weise wrote:
> >
> > Here is my attempt to summarize the discussion, please see the TBDs.
> >
>
Thanks for bringing this to a conclusion.
On Mon, Oct 15, 2018 at 6:18 PM Thomas Weise wrote:
>
> Here is my attempt to summarize the discussion, please see the TBDs.
>
> I would work on a PR with respective contributor and committer guideline
> updates next.
>
> Thanks,
> Thomas
>
>
> Goals:
>
+1 in general.
However, I'd suggest to use "rebase and merge" more often.
Otherwise reading history is really inconvenient. Like in attached
screenshot. You can never easily understand what is included in any of
previous commits of remotes/upstream/master, unless you do explicit git log
on those
Here is my attempt to summarize the discussion, please see the TBDs.
I would work on a PR with respective contributor and committer guideline
updates next.
Thanks,
Thomas
Goals:
- Clear history with purpose and origin of changes
- Ability to perform a granular rollback, if necessary
-
+1 to add JIRA issue as the part of commit message. it requires less effort
but will help a lot on our commit history. I used to not do that but I will
start to add JIRA info to my future commits.
-Rui
On Mon, Oct 1, 2018 at 2:59 AM Alexey Romanenko
wrote:
> +1 to what Anton said, I’m fully
+1 to what Anton said, I’m fully agree with this.
Looking on the current commit history we can notice that there are many commits
like “fix”, “add/edit/remove”, “clean up”, etc, that, seems, were introduced
after the review process iterations. I think this makes commit history too much
SGTM. I generally favor "trust the committer's judgment" aka (2)(d) when it
is obvious. We had some small communication problem about this before so I
just wanted to be careful to ask the author when it is not obvious.
Kenn
On Sat, Sep 29, 2018 at 9:06 AM Robert Bradshaw wrote:
> There's two
There's two separate topics of discussion going on here.
(1) Is it acceptable for PRs to have more than one commit. We've discussed
this before, and the consensus seems to be that this is both allowed and
intentional.
(2) What to do about PRs that are clearly "[BEAM-] Implement feature" +
On Fri, Sep 28, 2018 at 10:29 AM Thomas Weise wrote:
> +1 for stating the goal of clear provenance and granular rollback.
>
>
>
Also of course efficiency and quality of review (we don't to review tiny or
out-of-context changes or huge mega-changes), efficiency of authoring
(don't want to wait
+1 for stating the goal of clear provenance and granular rollback.
I think this discussion helps to remind/identify best practices how to get
there. Where appropriate we should augment guidelines for both, contributor
and committer.
Kenn: would you elaborate on the "1 commit = 1 review (and
Anton makes a good point. We have been talking about policy for what we do,
but the real issue is what we want to come out of it: a clear history for
seeing where code came from and granular rollback. I think in both cases
the key thing is that each commit is a single clear change. How they get
+1 to Anton's points. It looks like the main concern with unsquashed
commits is aesthetic, in that having "!fixup" commits produces noise and
clutters the code tree. I would like to point out again for those unaware,
that "git log --first-parent" filters the commit history so that each PR
Is there an actual problem caused by squashing or not squashing the commits
that we face in the project? I personally have never needed to revert
something complicated that would be problematic either way (and don't have
a strong opinion about which way we should do it). From what I see so far
in
Thanks for linking the previous discussion.
I have also seen a few cases where the intention was to make individual
changes that can be applied independently. But why not create separate PRs
for those, so they can also be reviewed and merged independently?
Also, if the intention is to make
Fully agree, if there is a logical commit history, we should keep it. I
think this is speaking to the large number of PRs that have a single "real"
commit, a bunch of fixups, and specifically authors who haven't gone
through and cleaned up their history.
(Now if the commits could logically be
I brought up this discussion a few months ago from the other side: I don't
like my commits being squashed. I try to create logical commits that each
passes tests and could be broken up into multiple PRs. Keeping those
changes intact is useful from a history perspective and squashing may break
On Thu, Sep 27, 2018 at 9:51 AM Robert Bradshaw wrote:
> I agree that we should create a good pointer for cleaning up PRs, and
> request (though not require) that authors do it. It's unfortunate though
> that squashing during a review makes things difficult to follow, so adds
> one more round
I agree that we should create a good pointer for cleaning up PRs, and
request (though not require) that authors do it. It's unfortunate though
that squashing during a review makes things difficult to follow, so adds
one more round trip.
We could consider for those PRs that make sense as a single
As a non-committer I think some automated squashing of commits sounds best
since it lightens the load of regular contributors, by not having to always
remember to squash, and lightens the load of committers so it doesn't take
as long to have your PR approved by one.
But for now I think the second
I tend to agree with you Lukasz. Of course we should try to follow the
guide lines as much as possible but if it requires an extra back and
forth with the PR author for a cosmetic change, it may not be worth the
time.
On 19.09.18 22:17, Lukasz Cwik wrote:
I have to say I'm guilty of not
I was also thinking about the possibility of wanting to revert
individual commits from a merge commit. The solution you propose works,
but only if you want to revert everything.
On 19.09.18 22:12, Charles Chen wrote:
What I mean is that if you get the first-parent commit using "git log
I have to say I'm guilty of not following the merge guidelines, sometimes
doing merges without rebasing/flatten commits.
I find that it is a few extra mins of my time to fix someones PR history if
they have more then one logical commit they want to be separate and it
usually takes days for the PR
What I mean is that if you get the first-parent commit using "git log
--first-parent", it will incorporate any and all fix up commits so we don't
need to worry about missing any.
On Wed, Sep 19, 2018, 1:07 PM Maximilian Michels wrote:
> Generally, +1 for isolated commits which are easy to
Generally, +1 for isolated commits which are easy to revert.
I don't think it's actually harder to roll back a set of commits that are merged together.
I think Thomas was mainly concerned about "fixup" commits to land in
master (as part of a merge). These indeed make reverting commits more
Ideally every commit should compile and pass tests though, right?
On Wed, Sep 19, 2018 at 12:15 PM Ankur Goenka wrote:
> I agree with the cleanliness of the Commit history.
> "Fixup!", "Address comments", "Address even more comments" type of
> comments does not convey meaningful information and
I agree with the cleanliness of the Commit history.
"Fixup!", "Address comments", "Address even more comments" type of comments
does not convey meaningful information and are not very useful. Its a good
idea to squash them.
However, I think its ok to keep separate commits for different logical
To be concrete, it is very easy to revert a commit in any case:
1. First, use "git log --first-parent" to find the first-parent commit
corresponding to a PR merge (this is a one-to-one correspondence).
2. Use "git revert -m 1 " to revert the commit; this selects
the first parent as
I don't think it's actually harder to roll back a set of commits that are
merged together. Git has the notion of first-parent commits (you can see,
for example, "git log --first-parent", which filters out the intermediate
commits). In this sense, PRs still get merged as one unit and this is
Wanted to bring this up as reminder as well as opportunity to discuss
potential changes to our committer guide. It has been a while since last
related discussion and we welcomed several new committers since then.
Finishing up pull requests pre-merge:
29 matches
Mail list logo
