I'm fine with that as well, I don't have a strong opinion on this one. And we can also remove it any time later.
Zoltan On Mon, Mar 26, 2018 at 6:43 PM Ryan Blue <rb...@netflix.com.invalid> wrote: > I think we can rely on the "Squash and Merge" default to avoid the commits > that will dirty the commit log. If you have to select "Rebase and Merge" to > use it, I think it will probably be fine. And it is nice to have the > option. > > rb > > On Mon, Mar 26, 2018 at 9:26 AM, Zoltan Ivanfi <z...@cloudera.com> wrote: > > > I think we should, otherwise we may accidentally end up with a commit > > history full of micro-changes like "Initial commit for feature", "Address > > reviewer1's comments", "Address reviewer2's comments". > > > > Zoltan > > > > On Mon, Mar 26, 2018 at 6:06 PM Lars Volker <l...@cloudera.com> wrote: > > > > > I think we only need "Rebase and Merge" for cases where we want to > > submit a > > > single PR as multiple commits, e.g. if a PR fixes two JIRAs that > somehow > > > depend on each other but shouldn't be squashed. Those seem a bit > > > hypothetical to me. However, "Rebase and Merge" does prevent the kind > of > > > merge commit that dirties the git log so I figured we might keep it. > I'm > > > open to disabling it, too, if you feel we should. > > > > > > On Mon, Mar 26, 2018 at 6:01 AM, Zoltan Ivanfi <z...@cloudera.com> > wrote: > > > > > > > To answer my second question: Just tried "Squash and Merge" and I > could > > > > edit the commit message. Looks good. > > > > > > > > Zoltan > > > > > > > > On Mon, Mar 26, 2018 at 2:26 PM Zoltan Ivanfi <z...@cloudera.com> > wrote: > > > > > > > > > +1 and thanks for taking care of this. I would like to have 2 > > questions > > > > > though: > > > > > > > > > > - Do we need "Rebase and Merge"? > > > > > - What will be the commit message of a "Squash and Merge"? > > > > > > > > > > Thanks, > > > > > > > > > > Zoltan > > > > > > > > > > > > > > > On Sun, Mar 25, 2018 at 11:45 AM Uwe L. Korn <uw...@xhochy.com> > > wrote: > > > > > > > > > >> +1 > > > > >> > > > > >> On Fri, Mar 23, 2018, at 11:57 PM, Ryan Blue wrote: > > > > >> > +1 > > > > >> > > > > > >> > On Fri, Mar 23, 2018 at 11:01 AM, Wes McKinney < > > wesmck...@gmail.com > > > > > > > > >> wrote: > > > > >> > > > > > >> > > +1 > > > > >> > > > > > > >> > > On Fri, Mar 23, 2018 at 1:56 PM, Lars Volker <l...@cloudera.com > > > > > > wrote: > > > > >> > > > I checked with the Infra team and they can disable some of > the > > > > >> options > > > > >> > > for > > > > >> > > > us. Before opening a Jira I'd like to get some more feedback > > > here. > > > > >> The > > > > >> > > > options are: > > > > >> > > > > > > > >> > > > - Disable Merge commits > > > > >> > > > - Keep "Squash and Merge" and "Rebase and Merge" > > > > >> > > > - Keep "Squash and Merge" as the default. This is what > our > > > > >> dev/merge > > > > >> > > > script does, too. > > > > >> > > > - Do this for all the parquet-* repos > > > > >> > > > > > > > >> > > > Please leave a +1 if you think that's a good idea. Let me > know > > > if > > > > >> you'd > > > > >> > > > like to have a formal vote. > > > > >> > > > > > > > >> > > > Cheers, Lars > > > > >> > > > > > > > >> > > > On Fri, Mar 23, 2018 at 8:21 AM, Wes McKinney < > > > > wesmck...@gmail.com> > > > > >> > > wrote: > > > > >> > > > > > > > >> > > >> You may be able to ask ASF Infra to disallow merge commits > in > > > the > > > > >> > > >> GitHub UI to prevent this from happening > > > > >> > > >> > > > > >> > > >> On Fri, Mar 23, 2018 at 11:18 AM, Lars Volker < > > l...@cloudera.com > > > > > > > > >> wrote: > > > > >> > > >> > Both of these merges are on me, I wasn't aware aware that > > > they > > > > >> would > > > > >> > > show > > > > >> > > >> > up like this. I personally prefer the format provided by > > the > > > > >> > > dev/merge.py > > > > >> > > >> > script. > > > > >> > > >> > > > > > >> > > >> > Since 2016 Github supports "Rebase and Merge" as a second > > > > option > > > > >> when > > > > >> > > >> > submitting PRs: > > > > >> > > >> > > > > > >> > https://blog.github.com/2016-09-26-rebase-and-merge-pull-requests/ > > > > >> > > >> > > > > > >> > > >> > The option is enabled on the parquet-format repository > but > > I > > > > >> don't > > > > >> > > have > > > > >> > > >> > access to the project configuration to check whether we > can > > > > >> disable > > > > >> > > the > > > > >> > > >> > others and make "Rebase and Merge" the default. > > > > >> > > >> > > > > > >> > > >> > Apologies for messing up the commit log. Please let me > know > > > if > > > > >> you'd > > > > >> > > like > > > > >> > > >> > me to clean them up with a force-push. > > > > >> > > >> > > > > > >> > > >> > Cheers, Lars > > > > >> > > >> > > > > > >> > > >> > On Fri, Mar 23, 2018 at 5:22 AM, Zoltan Ivanfi < > > > > z...@cloudera.com> > > > > >> > > wrote: > > > > >> > > >> > > > > > >> > > >> >> Hi, > > > > >> > > >> >> > > > > >> > > >> >> Parquet was recently migrated to GitBox (thanks, Uwe!). > > The > > > > >> > > dev/merge.py > > > > >> > > >> >> script can be still used by setting > > > > >> PUSH_REMOTE_NAME=apache-github > > > > >> > > >> before > > > > >> > > >> >> invoking it. There is also a new option of merging > changes > > > > >> using the > > > > >> > > >> GitHub > > > > >> > > >> >> web UI. I personally continued using the merge script, > > while > > > > >> others > > > > >> > > >> started > > > > >> > > >> >> using the GitHub web UI. However, I noticed that the > > latter > > > > >> results > > > > >> > > in > > > > >> > > >> less > > > > >> > > >> >> clear commit messages/structure: > > > > >> > > >> >> > > > > >> > > >> >> *git log --oneline* | head > > > > >> > > >> >> 92661a4 Merge pull request #89 from timarmstrong/master > > > > >> > > >> >> 809edf0 Merge pull request #86 from lekv/p323 > > > > >> > > >> >> 31a9ddc Update Encodings.md with RLE_DICTIONARY > > > > >> > > >> >> 4d58831 PARQUET-1236: Align version of slf4j-api > > > > >> > > >> >> 2667e08 PARQUET-323: Mark INT96 as deprecated > > > > >> > > >> >> a64a331 PARQUET-1201: Implement page indexes > > > > >> > > >> >> 9fef1d8 PARQUET-1197: Log rat failures > > > > >> > > >> >> 6e5b78d PARQUET-1065: Deprecate type-defined sort > ordering > > > for > > > > >> INT96 > > > > >> > > >> type > > > > >> > > >> >> 2696f9e PARQUET-1171: Clarify scope of usage for RLE, > > > > BIT_PACKED > > > > >> > > >> encodings > > > > >> > > >> >> c6d306d PARQUET-1064: Deprecate type-defined sort > ordering > > > for > > > > >> > > INTERVAL > > > > >> > > >> >> type. > > > > >> > > >> >> > > > > >> > > >> >> *git log --oneline --graph* | head -n 15 > > > > >> > > >> >> * 92661a4 Merge pull request #89 from > > timarmstrong/master > > > > >> > > >> >> |\ > > > > >> > > >> >> | * 31a9ddc Update Encodings.md with RLE_DICTIONARY > > > > >> > > >> >> * | 809edf0 Merge pull request #86 from lekv/p323 > > > > >> > > >> >> |\ \ > > > > >> > > >> >> | |/ > > > > >> > > >> >> |/| > > > > >> > > >> >> | * 2667e08 PARQUET-323: Mark INT96 as deprecated > > > > >> > > >> >> * | 4d58831 PARQUET-1236: Align version of slf4j-api > > > > >> > > >> >> |/ > > > > >> > > >> >> * a64a331 PARQUET-1201: Implement page indexes > > > > >> > > >> >> * 9fef1d8 PARQUET-1197: Log rat failures > > > > >> > > >> >> * 6e5b78d PARQUET-1065: Deprecate type-defined sort > > ordering > > > > for > > > > >> > > INT96 > > > > >> > > >> type > > > > >> > > >> >> * 2696f9e PARQUET-1171: Clarify scope of usage for RLE, > > > > >> BIT_PACKED > > > > >> > > >> >> encodings > > > > >> > > >> >> * c6d306d PARQUET-1064: Deprecate type-defined sort > > ordering > > > > for > > > > >> > > >> INTERVAL > > > > >> > > >> >> type. > > > > >> > > >> >> > > > > >> > > >> >> I would like to ask the developer community: What do you > > > think > > > > >> about > > > > >> > > the > > > > >> > > >> >> commits resulting from merging using the GitHub web UI? > > Are > > > we > > > > >> fine > > > > >> > > with > > > > >> > > >> >> these or should we come up with some a policy about how > to > > > > >> merge? > > > > >> > > >> >> > > > > >> > > >> >> Thanks, > > > > >> > > >> >> > > > > >> > > >> >> Zoltan > > > > >> > > >> >> > > > > >> > > >> > > > > >> > > > > > > >> > > > > > >> > > > > > >> > > > > > >> > -- > > > > >> > Ryan Blue > > > > >> > Software Engineer > > > > >> > Netflix > > > > >> > > > > > > > > > > > > > > > > > > -- > Ryan Blue > Software Engineer > Netflix >
