Pull Request HowTo

Post here if you have re-based and finalised code to integrate into master, which was discussed, agreed to and tested in other forums. You can also submit your PR directly on github.
looo
Posts: 2664
Joined: Mon Nov 11, 2013 5:29 pm

Re: Pull Request HowTo

Postby looo » Tue May 02, 2017 10:51 am

keithsloan52 wrote:git rebase master offset2
this will rebase offset2-branch on your master branch. To rebase on FreeCAD/master you can do the following:

1. add FreeCAD as a remote:

Code: Select all

git remote add FreeCAD https://github.com/FreeCAD/FreeCAD
2. update changes from the master:

Code: Select all

git fetch FreeCAD
3. rebase:

Code: Select all

git rebase FreeCAD/master
keithsloan52
Posts: 885
Joined: Mon Feb 27, 2012 5:31 pm

Re: Pull Request HowTo

Postby keithsloan52 » Tue May 02, 2017 12:20 pm

Do I do my changes before or after the rebase?
User avatar
yorik
Site Admin
Posts: 11397
Joined: Tue Feb 17, 2009 9:16 pm
Location: São Paulo, Brazil
Contact:

Re: Pull Request HowTo

Postby yorik » Tue May 02, 2017 5:46 pm

When I screw things too much with git I sometimes do the following to start over:

1. Save your changes in a diff file with git diff mybranch..master > mychanges.patch
2. Delete your dirty branch, make a new clean one from a fresh, up-to-date master
3. Switch to that branch and apply your changes with patch -d1 < mychanges.patch
4. If there are changed files produced by diff that shouldn't be changed, git checkout them

Then you have a rebased branch with only your changes there, ready to be committed.
Jee-Bee
Posts: 1919
Joined: Tue Jun 16, 2015 10:32 am
Location: Netherlands

Re: Pull Request HowTo

Postby Jee-Bee » Wed May 03, 2017 6:21 am

Thanks i didn't know this was possible!!
looo
Posts: 2664
Joined: Mon Nov 11, 2013 5:29 pm

Re: Pull Request HowTo

Postby looo » Wed May 03, 2017 7:24 am

keithsloan52 wrote:Do I do my changes before or after the rebase?
rebase can be done at any point. But normally you rebase before you do a PR.
User avatar
CADennis
Posts: 31
Joined: Tue Apr 18, 2017 10:12 am

Re: Pull Request HowTo

Postby CADennis » Fri May 05, 2017 8:15 pm

yorik wrote:When I screw things too much with git I sometimes do the following to start over:

1. Save your changes in a diff file with git diff mybranch..master > mychanges.patch
2. Delete your dirty branch, make a new clean one from a fresh, up-to-date master
3. Switch to that branch and apply your changes with patch -d1 < mychanges.patch
4. If there are changed files produced by diff that shouldn't be changed, git checkout them

Then you have a rebased branch with only your changes there, ready to be committed.
How would you write an automated "merge officer" scan? It must identify any two developers repetitively skipping the 4th step by lack of time/discipline. I need a solution for my ongoing CI enhancements.

Here is an example of this problem: Two musicians, one from Germany, the other from the USA. They write a song that simply traverses the c major scale. They commit note after note. In Germany we write "cdefgahc". Notice the "h"! In the USA musicians write "cdefgabc". Notice the "b"!

The song might internationally evolve as follows:

Code: Select all

USA    :   (cdef)         (bc)---------(de)-----(fga)------(bc)
                 \       /   \           \         \         \
master :         ( )   ( )   ( )   ( )   ( )  ( )  ( )  ( )  ( )
                   \   /           /          /         /
Germany:            (ga)-------(hcd)------(efg)------(ah)
Both get lazy from (ga) onwards. The master would have the effective states

Code: Select all

(cdef)->(cdefga)->(cdefgabc)->(cdefgahcd)->(cdefgabcde)->(cdefgahcdefg)->(cdefgabcdefga)->(cdefgahcdefgah)->(cdefgabcdefgabc)
Other musicians, who care maybe only about the first octave and try to stay synchronized, might get pretty annoyed and confused about these frequent, repetitive overwrites of

Code: Select all

h->b->h->b->h-> ...
I think we must somehow query git over several commits by the same developer to identify that he repeatedly overwrites the master with his unsynchronized way of writing things. I expect that such identification would usually come at least in pairs of developers running into such a loop, and then the "merge officer" must reject them immediately, maybe even rewind a bit. Any snippets or seen a similar automation (static analysis) already?
User avatar
kkremitzki
Posts: 1650
Joined: Thu Mar 03, 2016 9:52 pm
Location: Texas

Re: Pull Request HowTo

Postby kkremitzki » Sat May 06, 2017 11:38 pm

Hmm, that seems like a lot more complication than should be necessary. What problem in FreeCAD development necessitates this?
Like my FreeCAD work? I'd appreciate any level of support via Patreon, Liberapay, or PayPal! Read more about what I do at my blog.
User avatar
CADennis
Posts: 31
Joined: Tue Apr 18, 2017 10:12 am

Re: Pull Request HowTo

Postby CADennis » Mon May 08, 2017 8:38 pm

c'mon, that's not complicated! That's just two guys writing down the C Major scale :D :D
Maybe your mobile browser didn't render the monospaced font nicely?
monospaced_desktop.png
monospaced_desktop.png (3.62 KiB) Viewed 2338 times
Here is a shell script to demo and have something to implement the scanner against:

Code: Select all

#!/bin/bash
# Copyright 2017, CADennis, FreeCAD Community, 
# Licensed under Creative Commons BY
# ============================================================================
# The song might internationally evolve as follows:
#
# USA    :   (cdef)         (bc)---------(de)-------(fga)--------(bc)
#                  \       /   \           \           \           \
# master :         ( )   ( )   ( )   ( )   ( )   ( )   ( )   ( )   ( )
#                    \   /           /           /           /
# Germany:            (ga)-------(hcd)-------(efg)--------(ah)
# 
# Both get lazy from (ga) onwards. The master would have the effective states
#
# (cdef)->(cdefga)->(cdefgabc)->(cdefgahcd)->(cdefgabcde)->(cdefgahcdefg)->
# -> (cdefgabcdefga)->(cdefgahcdefgah)->(cdefgabcdefgabc)
#
# Other musicians, who care maybe only about the first octave and 
# try to stay synchronized, might get pretty annoyed and confused about 
# these frequent, repetitive overwrites of 
#
# h->b->h->b->h-> ...
# ============================================================================
mkdir torwaltz
cd torwaltz
mkdir protocol
mkdir torwaltz_git
cd torwaltz_git
git init
echo "" > song.txt
git add song.txt
git commit -m "let's make music"
# ====
git checkout -b usa
echo "cdef" >> song.txt
git add song.txt 
git commit -m "cdef - with love from USA"
# ====
git checkout master
git merge usa
NOW=$(date +%Y_%m_%d-%H%M--%S.%3N)
SNAPSHOT=`git rev-parse --short HEAD`
cp song.txt ../protocol/master_snapshot-${NOW}_[${SNAPSHOT}]
# ====
git checkout -b germany
echo "ga" >> song.txt
git add song.txt 
git commit -m "ga - with love from Germany"
# ====
git checkout master
git merge germany
NOW=$(date +%Y_%m_%d-%H%M--%S.%3N)
SNAPSHOT=`git rev-parse --short HEAD`
cp song.txt ../protocol/master_snapshot-${NOW}_[${SNAPSHOT}]
# ====
git checkout usa
echo "bc" >> song.txt
git add song.txt 
git commit -m "bc - with love from USA"
git rebase master 
sed -i '/<<<<<<</d' song.txt
sed -i '/>>>>>>>/d' song.txt
sed -i '/=======/d' song.txt
git add song.txt
git rebase --continue
# ====
git checkout master
git merge usa
NOW=$(date +%Y_%m_%d-%H%M--%S.%3N)
SNAPSHOT=`git rev-parse --short HEAD`
cp song.txt ../protocol/master_snapshot-${NOW}_[${SNAPSHOT}]
# ====
echo "Both get lazy now"
# ====
git checkout germany
echo "hcd" >> song.txt
git add song.txt 
git commit -m "hcd - with love from Germany"
git diff germany..master > mychanges.patch
ls -lsa
git checkout master
git branch -D germany
git checkout -b germany
ls -lsa
patch -R -d . < mychanges.patch
rm mychanges.patch
git add song.txt
git commit -m "hcd - with love from Germany"
# ====
git checkout master
git merge germany
NOW=$(date +%Y_%m_%d-%H%M--%S.%3N)
SNAPSHOT=`git rev-parse --short HEAD`
cp song.txt ../protocol/master_snapshot-${NOW}_[${SNAPSHOT}]
# ====
git checkout usa
echo "de" >> song.txt
git add song.txt 
git commit -m "de - with love from USA"
git diff usa..master > mychanges.patch
ls -lsa
git checkout master
git branch -D usa
git checkout -b usa
ls -lsa
patch -R -d . < mychanges.patch
rm mychanges.patch
git add song.txt
git commit -m "de - with love from USA"
# ====
git checkout master
git merge usa
NOW=$(date +%Y_%m_%d-%H%M--%S.%3N)
SNAPSHOT=`git rev-parse --short HEAD`
cp song.txt ../protocol/master_snapshot-${NOW}_[${SNAPSHOT}]
# ====
git checkout germany
echo "efg" >> song.txt
git add song.txt 
git commit -m "efg - with love from Germany"
git diff germany..master > mychanges.patch
ls -lsa
git checkout master
git branch -D germany
git checkout -b germany
ls -lsa
patch -R -d . < mychanges.patch
rm mychanges.patch
git add song.txt
git commit -m "efg - with love from Germany"
# ====
git checkout master
git merge germany
NOW=$(date +%Y_%m_%d-%H%M--%S.%3N)
SNAPSHOT=`git rev-parse --short HEAD`
cp song.txt ../protocol/master_snapshot-${NOW}_[${SNAPSHOT}]
# ====
git checkout usa
echo "fga" >> song.txt
git add song.txt 
git commit -m "fga - with love from USA"
git diff usa..master > mychanges.patch
ls -lsa
git checkout master
git branch -D usa
git checkout -b usa
ls -lsa
patch -R -d . < mychanges.patch
rm mychanges.patch
git add song.txt
git commit -m "fga - with love from USA"
# ====
git checkout master
git merge usa
NOW=$(date +%Y_%m_%d-%H%M--%S.%3N)
SNAPSHOT=`git rev-parse --short HEAD`
cp song.txt ../protocol/master_snapshot-${NOW}_[${SNAPSHOT}]
# ====
git checkout germany
echo "ah" >> song.txt
git add song.txt 
git commit -m "ah - with love from Germany"
git diff germany..master > mychanges.patch
ls -lsa
git checkout master
git branch -D germany
git checkout -b germany
ls -lsa
patch -R -d . < mychanges.patch
rm mychanges.patch
git add song.txt
git commit -m "ah - with love from Germany"
# ====
git checkout master
git merge germany
NOW=$(date +%Y_%m_%d-%H%M--%S.%3N)
SNAPSHOT=`git rev-parse --short HEAD`
cp song.txt ../protocol/master_snapshot-${NOW}_[${SNAPSHOT}]
# ====
git checkout usa
echo "bc" >> song.txt
git add song.txt 
git commit -m "bc - with love from USA"
git diff usa..master > mychanges.patch
ls -lsa
git checkout master
git branch -D usa
git checkout -b usa
ls -lsa
patch -R -d . < mychanges.patch
rm mychanges.patch
git add song.txt
git commit -m "bc - with love from USA"
# ====
git checkout master
git merge usa
NOW=$(date +%Y_%m_%d-%H%M--%S.%3N)
SNAPSHOT=`git rev-parse --short HEAD`
cp song.txt ../protocol/master_snapshot-${NOW}_[${SNAPSHOT}]
# ====
git log --graph --all --oneline
git status
echo "=========="
echo "let's see HEAD (all 'b' - you wouldn't notice anything)"
cat song.txt
echo "=========="
echo "let's see HEAD~1 (all 'h' - oops, how is that possible?!)"
git checkout -q HEAD~1
cat song.txt
echo "=========="
echo "let's see HEAD~2 (all 'h' - why that, why no 'fga' from USA?!)"
git checkout -q HEAD~2
cat song.txt
echo "=========="
echo "let's see germany (all 'h' - consistent in himself, but not synced)"
git checkout germany
cat song.txt
echo "=========="
echo "If you dare, have a look at the toggling h->b->h->b->... in the protocoled effective master snapshots:"
cd ../protocol
pwd
ls -l
echo "=========="
echo "Any 3rd developer trying to sync with these two bad guys would have given up ;-)"
Attachments
torwaltz.zip
torwaltz.sh - a shell script to demo and have something to implement the scanner against
(1.32 KiB) Downloaded 34 times
User avatar
yorik
Site Admin
Posts: 11397
Joined: Tue Feb 17, 2009 9:16 pm
Location: São Paulo, Brazil
Contact:

Re: Pull Request HowTo

Postby yorik » Mon May 08, 2017 9:03 pm

Oh we have "merge officiers" already. They are mostly Werner, and a bit myself when my limited knowledge allows it... They work quite well :D

There is a part you cannot totally solve I think... It would be unrealistic to imagine that you will find an automatic system that removes all errors that a coder can commit. If it's not an error like you described, it will be another... It seems to me that the best defenses you can build against errors are a good test suite. This works wonderfully with pull requests, each pull request must pass the whole test suite, that's already a fairly good guarantee that it didn't screw something serious.

Then add a couple of simple rules, like, don't make gigantic pull requests but divide things in smaller parts that are easier to review, etc.
User avatar
sgrogan
Posts: 5207
Joined: Wed Oct 22, 2014 5:02 pm

Re: Pull Request HowTo

Postby sgrogan » Mon May 08, 2017 9:31 pm

CADennis wrote:Any snippets or seen a similar automation (static analysis) already?
Yes, it's called "git rebase"
Please see here: https://www.atlassian.com/git/tutorials ... s-rebasing and wmayer's most recent previous post in this thread. wmayer and yorik do a great job helping to merge when a contributor has trouble with git, but I think the tools are there, if not easy to use.