rebase before merge

[Sebastien Binet]

hi there,

with the mono-repo structure and new people (yay!) working on different parts of the mono-repo, I think we should enforce somehow to always rebase before merging.

otherwise, we'll get a mess of a history.

I'd say it is already starting to be a bit of a mess:

```

$> git l

*   5779c127 (HEAD -> master, origin/master, origin/HEAD) Merge pull request #209 from gonum/f32/dot

|\  

| * 9d7baa81 asm/f32: Fixes from PR comments.

| * e0a0508f asm/f32: Asm dot functions.

| * 7f0e3341 asm/f32: Adding tests for dot asm functions.

| * 51fc8a3f asm/f32:  Adding stubs for dot asm functions.

| | * ab2c1e6e (origin/flow) Revert "graph/path: implement sophisticated algorithm"

| | * a9929ba0 graph/path: implement sophisticated algorithm

| | * 09fc4a20 graph/path: add benchmarks for Dominators

| | * bd6dc762 graph/path: replace map and slice index chasing with pointer chasing

| | * c5923049 Revert "graph/path: replace recursive dfs with iterative implementation"

| | * 7a3afb07 graph/path: add dominator tree API

| | * 4c195282 graph/path: replace recursive dfs with iterative implementation

| | * a4553c6e graph/path: replace Dominators with naive Lengauer and Tarjan

| | | * a6e52c49 (origin/kconnected) graph/community: add k-clique community function

| | | * a1136aba graph/topo: add clique graph construction function

| | |/  

| | | * 70474858 (origin/f64/ger) asm/f64: Merging logic changes from #225

| | | * 9dd91876 blas/gonum, asm/f64: Incorporate Ger routine in blas for testing.

| | | * e11ffa2c Build flags for go1.7 testing and guards for oversized test.

| | | * a9c13538 asm/f64: Cleaned up with go1.8-supported instructions.

| | | * 59c56216 asm/f64: Ger asm function added.

| | | * e82f927d asm/f64: Ger asm function framed out for loop construct/flow.

| | | * f8ea1eb8 asm/f64:  Adding stubs for Ger asm implementation.

| |_|/  

|/| |   

* | |   4ac7663e Merge pull request #227 from gonum/generate-error-fix

|\ \ \  

| * | | 1fc51d6a travis: Print git diff when check-generate fails.

* | | | 34774386 blas/gonum:  Fix NaN popagation in Dger.  (#225)

|/ / /  

* | | 49f3edb2 travis: reduce verbosity of test output

* | | f01292bb floats: Fix Argsort documentation.

* | | e01a71d4 mat: make RowView and ColView return Vector type and add RowViewOf and ColViewOf

| |/  

|/|   

| | * 134c8aaf (origin/negative-cycles) graph/path: allow Shortest to traverse negative cycles

| |/  

|/|   

* | d7342e68 travis: add go-1.9.x

* | 40dbcd03 travis: remove go-1.6.x

| | * 767fa7a0 (origin/degeneracy) graph/topo: use degeneracyOrdering instead of VertexOrdering for BronKerbosch

| | * 781be0e4 graph/topo: fix error reporting for VertexOrdering test

| | * 93093475 graph/topo: add function to find k-core of undirected graph

| | * 12013324 graph/{internal,path,topo}: factor reverse function out into internal

| |/  

|/|   

```

whereas, if you compare with Go's stdlib mono-repo:

```

$> git l

* 0202aa8ec7 (origin/master, origin/HEAD) doc: update DWARF version

* 13772edbbd cmd/compile: simplify exporting universal 'error' type

* 261a8d9abd testing: use time.Since instead of time.Now().Sub

* 6367c19f26 cmd/internal/obj/x86: add some more AVX2 instructions

* 31ddd8a39d cmd/compile: more compact DWARF location for locals and arguments

* 84188296e9 cmd/compile: more compact representation of DW_AT_high_pc

* 50f1f639a4 cmd/asm: add most SSE4 missing instructions

* b15e8babc8 cmd/asm: add amd64 PALIGNR instruction

* 4074e4e5be cmd/asm: add amd64 CLFLUSH instruction

* 26dadbe32c cmd/asm: add amd64 PAB{SB,SD,SW}, PMADDUBSW, PMULHRSW, PSIG{NB,ND,NW}

* 16edf0b1f7 crypto/cipher: panic when IV length does not equal block size in NewOFB

* 6c102e141c cmd/compile: avoid stack allocation of a map bucket for large constant hints

* 034d825ea3 runtime: avoid redundant zeroing of hiter

* 9c3f268558 cmd/compile: set hiter type for map iterator in order pass

* bb31217579 runtime: move map ismapkey check to the compiler

* 2d362f7a49 flag: simplify arg logic in parseOne

* 6675fadfb8 runtime: cleanup amd64p32 memmove and memclr file organization

* 0fb82fbcce cmd/compile: remove global bout variable

* 34db5f0c4d cmd/compile: fix evaluation order for OASOP

* d349fa25df cmd/compile: fix and improve struct field reflect information

* 812b34efae cmd/compile: ignore non-code nodes when inlining

* b2e8630f2f cmd/compile: simplify "missing function body" error message

* 53d24f76fb doc: Fixed missing dot in effective_go.html

* aed1c119fd cmd/compile: fix assembly test

* fb165eaffd cmd/compile: combine x*n - y*n into (x-y)*n

* 03c3bb5f84 math: Add Round function (ties away from zero)

* dbe3522c7f runtime: fix hashmap load factor computation

* a6a92b1867 net/url: add examples to URL methods

* a4e1a72f0a cmd/compile/internal/amd64: add MOVLloadidx8 and MOVLstoreidx8

* 605331f43e cmd/go: pass plugin package name to compile -p

* 9690d245d5 spec: clarify context type for certain non-constant shifts

* ec359643a1 archive/tar: populate Devmajor and Devminor in FileInfoHeader on Darwin

* cecba84a0d go/types: fix Info.Implicits entries

* e3f3adedcd cmd/go: document the build flags properly for vet

* f74b52cf50 cmd/cgo: support large unsigned macro again

* 51e92d7261 cmd/go: fix clang option handling

* 44e86bef06 crypto/cipher: extend the docs of BlockMode and Stream

* e236b6721c doc/articles/wiki: fix final-test.patch

* f316e1a88f go/types: escape +build sequence to silence vet warning

* 37dbfc51b0 cmd/link: emit DW_AT_data_member_location as a constant

* 1d07ed1579 io/ioutil: don't cap buffer size in ReadFile

* bf90da97c1 cmd/fix: rewrite x/net/context by default

* e3e09e3d5f doc: add error handling on http.ListenAndServe

```

a much more linear history.

much more readable too.

is there a github magic configuration thing that would always rebase before merging?

-s

[Dan Kortschak]

I too prefer a linear history, however occasionally I think there is
merit in retaining some internal structure to a PR that would be lost
by enforcing a rebase onto master approach. This is a result of
deficits in the github PR handling approach that is not missing the the
gerrit approach (which IMO has its own failings - though is probably
still better than the github approach).

For example in a PR that is currently open by me I want to retain some
changes that are there for historical reasons, but I also want to make
sure that the PR is seen at an atomic unit. This means that I need to
do a merge commit.

In short, I don't see a good technological fix for this.

[Dan Kortschak]

To add a little to this. I would like to have less merging from master
and other branches into PR branches. That particular approach leads to
a pretty significant component of the spaghetti that we see.

[Sebastien Binet]

On Thu, Sep 7, 2017 at 2:39 AM, Dan Kortschak <dan.ko...@adelaide.edu.au> wrote:

To add a little to this. I would like to have less merging from master
and other branches into PR branches. That particular approach leads to
a pretty significant component of the spaghetti that we see.

On Thu, 2017-09-07 at 08:18 +0930, Dan Kortschak wrote:
> I too prefer a linear history, however occasionally I think there is
> merit in retaining some internal structure to a PR that would be lost
> by enforcing a rebase onto master approach. This is a result of
> deficits in the github PR handling approach that is not missing the
> the
> gerrit approach (which IMO has its own failings - though is probably
> still better than the github approach).
>
> For example in a PR that is currently open by me I want to retain
> some
> changes that are there for historical reasons, but I also want to
> make
> sure that the PR is seen at an atomic unit. This means that I need to
> do a merge commit.

I personnally prefer the gerrit+go-stdlib approach (of always rebasing+squashing) but ok.

note that what I was advocating was to always rebase.

so, instead of having:

```

*    4ac7663e Merge pull request #227 from gonum/generate-error-fix

|\   

| *  1fc51d6a travis: Print git diff when check-generate fails.

* |  34774386 blas/gonum:  Fix NaN popagation in Dger.  (#225)

|/   

*  49f3edb2 travis: reduce verbosity of test output

*  f01292bb floats: Fix Argsort documentation.

*  e01a71d4 mat: make RowView and ColView return Vector type and add RowViewOf and ColViewOf

```

you'd have:

```

*    4ac7663e Merge pull request #227 from gonum/generate-error-fix

|\   

| *  1fc51d6a travis: Print git diff when check-generate fails.

|/   

*  34774386 blas/gonum:  Fix NaN popagation in Dger.  (#225)

*  49f3edb2 travis: reduce verbosity of test output

*  f01292bb floats: Fix Argsort documentation.

*  e01a71d4 mat: make RowView and ColView return Vector type and add RowViewOf and ColViewOf

```

for 1-commit PRs, I would argue that one should hit the "rebase and merge" button instead of "merge pull request", so it would look like:

```

*  1fc51d6a travis: Print git diff when check-generate fails.

*  34774386 blas/gonum:  Fix NaN popagation in Dger.  (#225)

*  49f3edb2 travis: reduce verbosity of test output

*  f01292bb floats: Fix Argsort documentation.

*  e01a71d4 mat: make RowView and ColView return Vector type and add RowViewOf and ColViewOf

```

but that's my personnal preference.

AFAICT, it's not possible to enforce rebasing while still retaining the merge commit:

 https://help.github.com/articles/configuring-commit-rebasing-for-pull-requests/

there is perhaps some hope here (but my ruby is rusty):

 https://stackoverflow.com/questions/2039773/have-remote-git-repository-refuse-merge-commits-on-push

-s

[Dan Kortschak]

I think we agree with an epsilon related to the underlying technology.
The problem with always doing a squashed cherrypick or the github
equivalent which is a squash/rebase is that some history is lost in the
github system that is not lost in the gerrit system, since gerrit keeps
all the revisions in the change list.

I agree, for one commit PRs it should always be a rebase commit.

On Thu, 2017-09-07 at 09:24 +0200, Sebastien Binet wrote:
> On Thu, Sep 7, 2017 at 2:39 AM, Dan Kortschak <dan.kortschak@adelaide
> .edu.au

[Vladimír Chalupecký]

I'm also not very happy with what GitHub does - it does not offer to rebase and keep the merge commit, which is in my opinion the best option to have linear history with related commits grouped in a branch/merge bypass as in:

*   007ce2c Merge branch 'fix-dlartg'

|\  

| * 386170c (fix-dlartg) testlapack: add huge and tiny input values to Dlartg test

| * 54dfdb6 native: fix computation of safe minimum in Dlartg

|/  

* 95501d6 native: make constants constant where possible

*   c4b79bc Merge branch 'add-dlagge'

|\  

| * 24cc479 (add-dlagge) testlapack: add test for Dlagsy and Dlagge

| * b97eda8 testlapack: fix checking of k,kl,ku in Dlagsy and Dlagge

| * 7d76e70 testlapack: add Dlagge

|/  

*   bca0fec Merge branch 'dlagsy-k'

Previously this was possible via command line, now that master is protected only GitHub UI can be used. I kind of miss that freedom.

I definitely do not like the spaghetti from the original post so if the plain "Merge" button were disabled, it would be a good thing.

[Kunde21]

I completely missed this post, but there's now an option to rebase and merge: https://github.com/blog/2243-rebase-and-merge-pull-requests

The tradeoff there is the loss of merge commits, though.