Skip to content

Conversation

@harveyj1953
Copy link
Contributor

fixed copy/paste error in stressLinear
fixed resource leak in ruleVisHists

@phsft-bot
Copy link

Can one of the admins verify this patch?

@martinmine
Copy link
Contributor

@phsft-bot build!

@phsft-bot
Copy link

Starting build on gcc49/centos7, native/mac1011, gcc49/slc6, gcc62/slc6, native/ubuntu14 with CMake flags -Dvc=OFF -Dimt=OFF -Dccache=ON

@pcanal
Copy link
Member

pcanal commented Apr 25, 2017

Something seems odd with this pull requests. It lists 31 commits but only 4 modified files ... maybe the rebase against master did not work properly?

@harveyj1953
Copy link
Contributor Author

harveyj1953 commented Apr 25, 2017 via email

@vgvassilev
Copy link
Member

Maybe sync your fork or create every new PR from a separate branch.

@pcanal
Copy link
Member

pcanal commented Apr 25, 2017

2 of the commits were fixing merge conflicts, the last 2 are real bugs fixed today.

This is actually the problem (it creates a 'parallel' history where all your commits are there twice (up to the merge commit). I recommend you start from 'scratch' a new branch and cherry-pick your new commits.

@harveyj1953
Copy link
Contributor Author

harveyj1953 commented Apr 25, 2017 via email

@pcanal
Copy link
Member

pcanal commented Apr 25, 2017

Hi John,

The merge conflict was just because Pere intervened to correct a fix I had made (just a formatting > issue) but he did it directly in the main repository and in a way that left my ‘origin’ out of sync.

Yes and the way it was resolved (presumedly by the web interface) is sub-optimal. The history of master looks like:

* 203703f - (71 minutes ago) coverity 61577: fix resource leak delete [] c — John Harvey
* e815eaa - (2 hours ago) coverity 61279: fix copy/paste error — John Harvey
*   ed09395 - (6 days ago) resolving merge conflicts — John Harvey
|\  
| * 6c63896 - (6 days ago) Fix for failing test (too strict tolerances for floating point values) — Guilherme Amadio
| * 9d6ac44 - (7 days ago) Data members used as array size must be static. — Philippe Canal
| * 27d58ca - (7 days ago) - Allows any number of options - Small mods in the online help - Spell check — Olivier Couet
....
| * | | | | 7db5e8c - (8 weeks ago) Coverity 94053: fixed uninitialised class members — John Harvey
....
* | | | | | 96fa411 - (20 hours ago) Downgrade to C++11 — Philippe Canal (origin/master, origin/HEAD)
....
* | a18c480 - (8 weeks ago) Coverity 94053: fixed uninitialised class members — John Harvey

So for example the commit "Coverity 94053: fixed uninitialised class members" is twice in your master's history (that is because the merge/resolve conflict rather than having done a rebase).

At this point the best is to do some manual intervention on your repository to 'clean-up' your master.

This would include:

  • make a branch out of your master to make sure to not lose anything:
    git branch masterApril2017 203703f
  • do a hard reset of master to match origin/master
    git checkout master; git reset --hard origin/master
  • Re-apply the commit with a merge conflict.
    git diff 6c63896..ed09395 math/mathmore/test/testInterpolation.cxx | patch -p1; git commit -m "Redo: removed duplicate increment in for loop (c627ce92488abd79)"
  • cherry-pick the two new commits
    git cherry-pick e815eaa 203703f

In the future, I recommend that rather than working directly in the master, you consider working on a "feature" branch. See for example: http://geant.cern.ch/content/suggested-work-flow-distributed-projects-nosy

For more detail on rebase vs merge, see https://medium.com/@porteneuve/getting-solid-at-git-rebase-vs-merge-4fa1a48c53aa#.1t4khiyjg

Thanks,
Philippe.

@vgvassilev
Copy link
Member

Alternatively, one can rebase with git rebase -s recursive -X ours upstream/master. This would give preference to what's already in the master.

@vgvassilev
Copy link
Member

Closing this one now. The relevant items are in #526.

@vgvassilev vgvassilev closed this Apr 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants