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 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 author to do merging with the extra
burden as a committer to keep track of another PR and its state (waiting
for clean-up) is taxing. I really liked the idea of the mergebot (even
though it didn't work out in practice) because it could do all the
policy work on my behalf.
Anything that reduces my overhead as a committer is useful as for the
100s of PRs that I have merged, I've only had to rollback a couple so
I'm for Charle's suggestion which makes the rollback flow slightly more
complicated for a significantly easier PR merge workflow.
On Wed, Sep 19, 2018 at 1:13 PM Charles Chen <c...@google.com
<mailto:c...@google.com>> wrote:
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 <m...@apache.org
<mailto:m...@apache.org>> wrote:
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
difficult because you have to check whether you missed a "fixup".
> Ideally every commit should compile and pass tests though, right?
That is definitely what we should strive for when doing a merge
against
master.
> Perhaps the bigger issue is that we need better documentation
and a playbook on how to do this these common tasks in git.
We do actually have basic documentation about this but most
people don't
read it. For example, the commit message of a Merge commit
should be:
Merge pull request #xxxx: [BEAM-yyyy] Issue title
But most merge commits don't comply with this rule :) See
https://beam.apache.org/contribute/committer-guide/#merging-it
On 19.09.18 21:34, Reuven Lax wrote:
> Ideally every commit should compile and pass tests though, right?
>
> On Wed, Sep 19, 2018 at 12:15 PM Ankur Goenka
<goe...@google.com <mailto:goe...@google.com>
> <mailto:goe...@google.com <mailto:goe...@google.com>>> 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 are
not very
> useful. Its a good idea to squash them.
> However, I think its ok to keep separate commits for
different
> logical pieces of the code which make reviewing and
revisiting code
> easier.
> Example PR: Support X in the pipeline
> Commit 1: Restructuring a bunch of code without any
logical change.
> Commit 2: Changing validation logic for pipeline.
> Commit 3: Supporting new field "X" for pipeline.
>
> On Wed, Sep 19, 2018 at 11:27 AM Charles Chen
<c...@google.com <mailto:c...@google.com>
> <mailto:c...@google.com <mailto:c...@google.com>>> wrote:
>
> 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 <commitid>" to revert the
commit; this
> selects the first parent as the base for a merge
commit (in
> the case where a single commit needs to be
reverted, just
> use "git revert <commitid>" without the "-m 1" flag).
>
> In any case, as a general good engineering practice,
I do agree
> that it is highly desirable to have small independent PRs
> instead of large jumbo PRs whenever possible.
>
> On Wed, Sep 19, 2018 at 11:20 AM Charles Chen
<c...@google.com <mailto:c...@google.com>
> <mailto:c...@google.com <mailto:c...@google.com>>> wrote:
>
> 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 preserved even if intermediate
commits are
> kept. Perhaps the bigger issue is that we need
better
> documentation and a playbook on how to do this
these common
> tasks in git.
>
> On Wed, Sep 19, 2018 at 9:27 AM Thomas Weise
<t...@apache.org <mailto:t...@apache.org>
> <mailto:t...@apache.org <mailto:t...@apache.org>>>
wrote:
>
> 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:
>
>
https://beam.apache.org/contribute/committer-guide/#finishing-touches
>
> PRs are worked on over time and may
accumulate many
> commits. Sometimes because scope expands,
sometimes just
> to separate independent changes but most of
the time the
> commits are just fixups that are added as
review progresses.
> It is important that the latter get squashed
prior to PR
> merge, as otherwise we lost the ability to
roll back
> changes by reverting a single commit and also
generally
> cause a lot of noise in the commit history
that does not
> help other contributors. To be clear, I refer
to the
> "Fixup!", "Address comments", "Address even more
> comments" type of entries :)
>
> I would also propose that every commit gets
tagged with
> a JIRA (except those fixups that will be
squashed).
> Having the JIRA and possibly other tags makes
it easier
> for others not involved in the PR to identify
changes
> after they were merged, for example when
looking at the
> revision history or annotated source.
>
> As for other scenarios of jumbo PRs with many
commits,
> there are probably situations where work
needs to be
> broken down into smaller units, making life
better for
> both, contributor and reviewer(s). Ideally,
every PR
> would have only one commit, but that may be a
bit much
> to mandate? Is the general expectation
something we need
> to document more clearly?
>
> Thanks,
> Thomas
>
