librelist archives

Security Merge (hi ltucker and groldan!)

From:

David Winslow

Date:

2010-07-20 @ 23:48

Hey all.  Luke showed me yesterday some weird behavior during the merge 
of the security branch with his master.  (Some changes that were almost 
definitely not intended were being made as part of the merge; in 
particular the templates for some of the csw transactions were being 
removed.)  I spent more time than I should have trying to track down 
where this change was coming from, but the revision history around the 
security stuff is quite complicated.

So, I just spent an hour or so rebasing both Luke's master branch (where 
the django side of the security integration is) and Gabriel's security 
branch (where the GeoServer side is) onto master.  I did a test merge as 
well and it looks like the spurious commits are gone, so hurrah.  I'm 
not publishing a merged commit though because I was kind of guessing 
during the conflicts (and there were a number of merge conflicts even 
during the rebase).  Luke and Gabriel, you should check things out.

My results are on my github fork at:
http://github.com/dwins/geonode/tree/gs_security_rebase
http://github.com/dwins/geondoe/tree/dj_acl_publisher


So, why a rebase?  git rebase can do a lot of stuff, but in its simplest 
invocation it simply picks up the current branch and rewrites it as 
though it were based off another one.  (you can look at the pretty ASCII 
art in the man page for git rebase here: 
http://kernel.org/pub/software/scm/git/docs/git-rebase.html) Along the 
way it:
   * drops commits that are already on the new base (!)
   * folds everything up so it looks like a straight development line 
(no branching within the branch, even if multiple developers have been 
hacking on it)

I never did figure out what revision was erasing the CSW templates, but 
after the rebase they were there again on groldan's security branch.

If you would rather redo the changes yourself (took me about 20-30 
minutes each for geoserver and django stuff, would probably be quicker 
for someone who's been looking at the code recently) here are the 
commands I used:

git checkout groldan/security -b gs_security_rebase
git rebase origin/master
while [git reports conflicts]
do
   git mergetool
   git rebase --continue
done

git checkout ltucker/master -b dj_acl_publisher
git rebase origin/master
while [git reports conflicts]
do
   git mergetool
   git rebase --continue
done

--
David Winslow
OpenGeo - http://opengeo.org/

I forgot to mention yesterday that "git rebase" is a fairly destructive 
operation; it actually modifies history, unlike most of git's operations 
which add new commits without affecting pre-existing ones.

In particular, the checksum for a commit (which git log and other tools 
use to determine what commit are "unmerged" between branches) depends on 
that commit's predecessors (recursively, all the way to the root of the 
history tree).  Rebasing a commit, even if it doesn't require 
modifications due to merge conflicts, changes that lineage and removes 
git's ability to determine that branch A and branch B have commits in 
common.  For example, if I rebase a branch, and Gabriel merges the 
result onto his copy of the original branch, we'll have an even more 
tangled history than before.  (Fortunately, Gabriel is likely to get a 
clue that something is wrong when he has a huge set of merge conflicts 
to deal with.)

What this means then, is that when a branch is rebased, the "proper" 
thing to do is to abandon the previous version of the branch (and it's 
really important to get everyone who's using it to do likewise).  The 
rebase command does this for you by default, but it can't affect remote 
repositories (which is a really good reason to avoid rebasing commits 
that have already been pushed to a public repository).  When I sent out 
the mail earlier I really should have added "Luke and Gabriel, please 
use these rebased branches as alternatives to the old versions, and 
whatever you do, don't merge them back onto the originals."

So, if you guys decide to use Gabriel's version as the base and merge 
the Django-side work onto it, I'd recommend something like:

$ git remote add dwins git://github.com/dwins/geonode.git
$ git fetch dwins gs_security_rebase
$ git fetch dwins dj_acl_service
$ git checkout dwins/gs_security_rebase -b gs_security_rebase
$ git merge dwins/dj_acl_service

Throw in some conflict resolution, and this should produce a revision 
with the Django stuff AND the GeoServer stuff that is based on a 
relatively recent version of our "trunk" development line.  If there is 
work left to do, I'd suggest doing that work on this merged branch.

--
David Winslow
OpenGeo - http://opengeo.org/

On 07/21/2010 11:07 AM, David Winslow wrote:
> $ git remote add dwins git://github.com/dwins/geonode.git
> $ git fetch dwins gs_security_rebase
> $ git fetch dwins dj_acl_service
> $ git checkout dwins/gs_security_rebase -b gs_security_rebase
> $ git merge dwins/dj_acl_service
>    
Oops, the fetch commands should have looked like:

$ git fetch dwins gs_security_rebase:gs_security_rebase
$ git fetch dwins dj_acl_service:dj_acl_service

Otherwise git will just assign them the temporary name "FETCH_HEAD" 
which is subject to reassignment the next time you fetch.

--
David Winslow
OpenGeo - http://opengeo.org/