Hi Holger,

On Jun 20, 2016, at 1:24 AM, Holger Freyther holger@freyther.de wrote:

...

On 20 Jun 2016, at 09:53, Norbert Hartl norbert@hartl.name wrote:

A commit is associated with a comment. So commits should be rather small and describing the change. Most of the time there are some changes/commits that only make sense when being together. This would be the view from the build server or from the process of having buildable artifacts all the time. So you can see it as freedom to have a commit that breaks the software which can be build after 5 more commits. So no problem for the world if you push 5 commits at once.

This approach breaks tools like git bisect. If a user has something easy/quick to reproduce and a "good" version and "bad" one, he/she can identify the first broken commit almost automatically. If commits are small then it is quite easy for the maintainer to identify the issue and resolve it.

Projects like Linux have the policy that each single commit needs to compile and for my GPRS/EDGE PCU[1] we have the same policy and I can point to a recent example of git bisect used by a user[2]. As a maintainer this reduces the work I have to do.

In the context of spur there is a significant performance regression of our TCAP benchmark from one spur version to another and with git bisect I could do:

git bisect start git bisect good <SHA1 of better VM> git bisect bad <SHA1 of first bad VM>

and then commits from good to bad will be picked and all I need to do is:

make bench

and then decide to mark this commit as:

git bisect bad git bisect good

and eventually end with the first bad commit (or none if I track noise)

It is obviously up to the maintainers if they want to support git bisect but thanks to git it is not really a burden. One can experiment with temporary branches and then rebase to a final and working patchset. With later git versions can be easily automated as well.

I think this is a very sensible policy and that we should support bisect. Do you have energy to work with Tim F to update CONTRIBUTING.md to include this policy? Are there ways of enforcing or advising the policy when using git tools or is it simply something we have to tell users? This latter approach is terribly fragile.

git rebase -i --execute "./compile_and_test_all.sh" origin/master close the editor..and git starts compiling all your intermediate commits and stops if they fail

kind regards holger

[1] A GSM component that converts a LLC frame to smaller pieces. Decides on the coding scheme (error correction bits vs. data), schedules resources across several handsets. [2] http://lists.osmocom.org/pipermail/openbsc/2016-May/009139.html

On Mon, Jun 20, 2016 at 5:55 PM, Tim Felgentreff timfelgentreff@gmail.com wrote:

I've pushed a CONTRIBUTING.md to the repository that we can iterate over and discuss.

If you go to https://github.com/OpenSmalltalk/vm/commit/5052e307fda0369f04550966fcfdf0a37... you can leave comments inline or at the end.

Thanks Tim, thats a really good starting point. Usually github would be the place to comment in-line, but this influences community workflow a a broader audience is useful.

The thing I want to understand most is the strong discouragement of rebasing rather than using it to maintain a tidy history.

First there is...

This also means that you should avoid rebasing or squashing your work. Just keep the history.

I happened to look at Laura's PR that adds some good info to CONTRIBUTING, so I use that as an example... https://github.com/OpenSmalltalk/vm/pull/14 Do you really want all four commits to appear in the master history? I hunted up a dozen articles around this point, and even though a little long winded, the best was this one...

http://blog.izs.me/post/37650663670/git-rebase

I particularly liked how the two camps of rebase or not were characterised into platform or service, as well as the explanation of bisect.

Then there is...

Every so many commits it is a good idea to push your work. Please refrain from rebasing, unless your history looks like this:

So the need for history cleanup is alluded to, but no guidance is given on *how* to do it safely. The critical point to make is to distinguish between cleaning private branches and leaving public branches alone. This is a good article providing some examples...

https://sandofsky.com/blog/git-workflow.html

Thirdly there is...

However, if you're unsure what rebasing is, just forget about it and push the history as-is.

Again this seems to instil fear of a very useful tool for cleaning up work in progress commits before submitting a pull request. But first the community needs to discuss the degree to which a clean linear history is beneficial or desired. Then we might define some recipes to achieve that.

Finally it says...

You will be thankful later when you need to use `git bisect` to find where a bug comes from.

But IIUC, bisect works better with a clean and linear history. Also from the the repository network graph...

https://github.com/OpenSmalltalk/vm/network

before the 25 May the SVN history is *very* linear (or is this just an artefact of the migration?) And after the 19 June (considering only within the OpenSmalltalk swim lane, i.e. ignoring the personal repo forks) the history is less linear. Now branching is one of git's greatest strengths to encourage participation, but that doesn't mean every branch needs a permanent record in the trunk history.

So considering one particular workflow case, an afternoons work on documentation or a quick bug fix, which yet may have several commits but is only pushed once upon completion -- I believe a reasonable workflow here would be for `rebase -i` to squash down to a single commit prior to issuing the pull request.

Any subsequent commits to the PR from feedback would remain separate until it is ready to integrate, when the integrator could squash them as described here...

"Merge pull request” [Big Green Button] Considered Harmful https://blog.spreedly.com/2014/06/24/merge-pull-request-considered-harmful

However it sounds like you've some personal war stories that might be useful for us to hear. To me rebase seems straightforward for certain defined situations, but maybe there is more about the risks I need to learn...

Seriously, skipping bad commits a bisect is easier than fixing someones git tree once they have lost commits to the depths of reflog. Especially if their recovery attempts have already triggered a Git GC.

cheers -ben

On Sun, Jun 26, 2016 at 4:51 AM, Tim Felgentreff timfelgentreff@gmail.com wrote:

Hi Ben,

One of your arguments is about history cleaning. Mostly my discouraging of rebase -i comes from experience with people who made a mistake (e.g. as simple as deleting a line and then getting "lucky" and having git not complain about conflicts during the rebase) to lose a commit entirely. And sometimes they continued working for a while, eventually a git gc was triggered, and then that commit was cleaned up and was lost for good by the time they noticed their work was gone. This happened to people around me more than just once, and then I wasn't able to help because git gc had run. Eventually I gave up and encouraged people who do not want to spend the time to really put in the work and understand how git works and what the internal structures are to not use rebase.

Ah, it never cross my mind to delete a line. So I never hit the problem.

At https://git-scm.com/docs/git-rebase I found rebase.missingCommitsCheck - If set to "warn", print warnings about removed commits in interactive mode. If set to "error", print the warnings and stop the rebase. If set to "ignore", no checking is done. "ignore" by default.

which you can see in action here... https://github.com/git/git/blob/cf4c2cfe52be5bd973a4838f73a35d3959ce2f43/git... warn "Warning: some commits may have been dropped" \ "accidentally." warn "Dropped commits (newer to older):"

but that didn't work for me and I had to ask on #git IRC to discover this was introduced in 2.6.0. I'm only 2.1.4 and current release is 2.9.0.

((Also btw I learnt a new trick, at the top of that last link, click <Blame>, then search for "missingCommitsCheck" and scroll back up to click on the commit, and at the bottom of the commit it says "master v2.9.0…v2.6.0-rc0" ))

Your other argument is about bisecting. Another experience I had was with people rebasing to "clean up" history by grouping related changes together, but then not checking that each commit still compiles. So the bisect scripts we used in these projects still had to include the logic to skip non-compiling commits.

Good point. Although I do like Holger's git rebase -i --execute "./compile_and_test_all.sh" origin/master but this is probably hard to police in an open source environment.

As would any script used on a repository where no one used rebase to remove such 'bad' commits. Secondly, if people tried a bunch of different approaches and finally refactored their code and then squashed it all together, any bisects will only find one large commit as the culprit without explanatory history around it, making debugging harder.

I wouldn't expect every commit to be squashed, just the minuscule things. Logical groups of work should remain.

All that being said, if you all think it's safe and we all agree that no one has the right to complain about git if they loose their commits, I won't strongly oppose rebasing.

Its probably too early for anyone new to git to form an experiential opinion on whether rebase is safe. Still the main contributors coudl choose to be conservative. I think the CONTRIBUTION page could say something like... "git rebase is a dangerous but useful tool for a narrow range of well defined tasks. Receipies for these tasks are presented below. For project consistency, you should adhere strongly to the recipe and if something unexpected occurs, do... git rebase --abort. Please discuss any deviations on the mail list."

and if that sounds reasonable, I'll follow up with some recipes, one of which might be check "git --version" and upgrade to latest git to make rebase a bit safer.

cheers -ben

P.S. while on the topic of disasters we can learn from, this "Disaster 2" story [1] might be a useful read for anyone adapting from SVN. [1] https://randyfay.com/content/avoiding-git-disasters-gory-story

On Wed, Jun 29, 2016 at 1:38 AM, Ben Coman btc@openinworld.com wrote:

On Tue, Jun 28, 2016 at 11:41 PM, Ben Coman btc@openinworld.com wrote:

...

On Sun, Jun 26, 2016 at 4:51 AM, Tim Felgentreff timfelgentreff@gmail.com wrote:

...

Hi Ben,

One of your arguments is about history cleaning. Mostly my discouraging of rebase -i comes from experience with people who made a mistake (e.g. as simple as deleting a line and then getting "lucky" and having git not complain about conflicts during the rebase) to lose a commit entirely. And sometimes they continued working for a while, eventually a git gc was triggered, and then that commit was cleaned up and was lost for good by the time they noticed their work was gone. This happened to people around me more than just once, and then I wasn't able to help because git gc had run. Eventually I gave up and encouraged people who do not want to spend the time to really put in the work and understand how git works and what the internal structures are to not use rebase.

Ah, it never cross my mind to delete a line. So I never hit the problem.

So of course I had to experiment to understand properly. The log is a bit long, but maybe a handy reference... Each experiment delineated by "#===="

TL;DR - check out the last two examples.

#====SETUP MASTER

$ mkdir testrebase && cd testrebase $ git init $ echo M >> blah ; git add blah ; git commit -m M

#====NORMAL SQUASH ONLY, NO DELETES

$ git checkout -b testSquashOnly master $ for N in 1 2 3 4 ; do (echo $N >> blah ; git commit -am $N) ; done $ cat blah ; git log --oneline M 1 2 3 4

b9784f0 4 97df945 3 c57a0e6 2 77366e2 1 9b9b515 M

$ git rebase -i master # in editor, change commit 2 to squash, save, exit # in second editor, change commit message to "1&2", save, exit

$ cat blah ; git log --oneline M 1 2 3 4

15cbb61 4 6416fd2 3 3b00e2a 1&2 4bba92f M

No changes lost. History cleaned okay.

#====DELETE,ERROR,ABORT

$ git checkout -b testDeleteAbort master $ for N in 1 2 3 4 ; do (echo $N >> blah ; git commit -am $N) ; done $ cat blah ; git log --oneline # as above

$ git rebase -i master # in editor, delete commit 2 line, save, exit error: could not apply 97df945... 3 When you have resolved this problem, run "git rebase --continue". If you prefer to skip this patch, run "git rebase --skip" instead. To check out the original branch and stop rebasing, run "git rebase --abort". Could not apply 97df94529f50a597489a53bf21f25a9c270226a0... 3

# error??!! ==> panic!!! ==> abort $ cd rebase --abort $ cat blah ; git log --oneline master 1 2 3 4

b9784f0 4 97df945 3 c57a0e6 2 77366e2 1 9b9b515 master

Abort on all errors is okay.

#====DELETE,ERROR,EDIT,CONTINUE

$ git checkout -b testDeleteMerge master $ for N in 1 2 3 4 ; do (echo $N >> blah ; git commit -am $N) ; done $ cat blah ; git log --oneline # as above

$ git rebase -i master # in editor, delete commit 2 line, save, exit error: could not apply 97df945... 3

# open editor on file blah # remove line <<<< ==== >>>> inserted by rebase, to end up with... master 1 2 3

$ git add blah; git rebase --continue [detached HEAD 1f5ed82] 3 1 file changed, 2 insertions(+) Successfully rebased and updated refs/heads/test.

$ cat blah ; git log --oneline master 1 2 3 4

ef57ae0 4 1f5ed82 3 77366e2 1 9b9b515 master

All changes survived, but there is a risk of losing changes if wrong lines merged in editor.

#====DELETE,ERROR,CONTINUE(WITHOUT EDIT)

$ git checkout -b testDeleteMerge2 master $ for N in 1 2 3 4 ; do (echo $N >> blah ; git commit -am $N) ; done $ cat blah ; git log --oneline # as above

$ git rebase -i master # in editor, delete commit 2 line, save, exit error: could not apply 97df945... 3

# note, no edit of blah $ git add blah; git rebase --continue # in editor, save, exit [detached HEAD bf742d8] 3 1 file changed, 5 insertions(+) error: could not apply 83209d3... 4

$ git add blah; git rebase --continue # in editor, save, exit [detached HEAD 42ab0c6] 4 1 file changed, 4 insertions(+) Successfully rebased and updated refs/heads/testDeleteMerge2.

$ cat blah ; git log --oneline M 1 <<<<<<< HEAD ======= 2 3 <<<<<<< HEAD >>>>>>> 7176be1... 3 ======= 4 >>>>>>> 83209d3... 4 42ab0c6 4 bf742d8 3 a13429f 1 4bba92f M

Urgh! No lines actually lost, but an awful mess to clean up.

#====DELETE,ERROR,SKIP

$ git checkout -b testDeleteSkip master $ for N in 1 2 3 4 ; do (echo $N >> blah ; git commit -am $N) ; done $ cat blah ; git log --oneline # as above

$ git rebase -i master # in editor, delete commit 2 line, save, exit error: could not apply 97df945... 3

$ git rebase --skip error: could not apply b873786... 4

$ git rebase --skip Successfully rebased and updated refs/heads/testDeleteSkip.

$ cat blah ; git log --oneline M 1 fc67d5e 1 4bba92f M

OUCH!! CHANGES LOST!!!! AAARRRGGHHH!!

#====THE FOLLOWING REQUIRES git --version > 2.6.0

#====DELETE, missingCommitsCheck WARN

$ git checkout -b testMissingCommitsCheckWarn master $ for N in 1 2 3 4 ; do (echo $N >> blah ; git commit -am $N) ; done $ cat blah ; git log --oneline # as above

$ git config rebase.missingCommitsCheck warn $ git rebase -i master # in editor, delete commit 2 line, save, exit Warning: some commits may have been dropped accidentally. Dropped commits (newer to older): - 304610b 2 To avoid this message, use "drop" to explicitly remove a commit.

Use 'git config rebase.missingCommitsCheck' to change the level of warnings. The possible behaviours are: ignore, warn, error.

error: could not apply a427113... 3

When you have resolved this problem, run "git rebase --continue". If you prefer to skip this patch, run "git rebase --skip" instead. To check out the original branch and stop rebasing, run "git rebase --abort". Could not apply a427113b3137a1e585fcd04dcdd64c899fa4f163... 3

SO WITH WARN WE ARE STILL PRESENTED WITH THE CHOICE TO SKIP, ETC...

#====DELETE, missingCommitsCheck ERROR

$ git checkout -b testMissingCommitsCheckError master $ for N in 1 2 3 4 ; do (echo $N >> blah ; git commit -am $N) ; done $ cat blah ; git log --oneline # as above

$ git config rebase.missingCommitsCheck error $ git rebase -i master # in editor, delete commit 2 line, save, exit Warning: some commits may have been dropped accidentally. Dropped commits (newer to older): - 08d9c7b 2 To avoid this message, use "drop" to explicitly remove a commit.

Use 'git config rebase.missingCommitsCheck' to change the level of warnings. The possible behaviours are: ignore, warn, error.

You can fix this with 'git rebase --edit-todo'. Or you can abort the rebase with 'git rebase --abort'.

SO WITH ERROR WE ARE PRESENTED WITH SAFER OPTIONS

cheers -ben.