Skip to content

Autosave tse#50

Closed
bhill-slac wants to merge 10 commits into
areaDetector:masterfrom
bhill-slac:autosave-TSE
Closed

Autosave tse#50
bhill-slac wants to merge 10 commits into
areaDetector:masterfrom
bhill-slac:autosave-TSE

Conversation

@bhill-slac
Copy link
Copy Markdown
Contributor

Adds support for initialization and autosave of TSE field for records that update w/ each new image.

@ulrikpedersen
Copy link
Copy Markdown
Member

Hi Bruce,

Thanks for the Pull Request. I have a few practical comments and suggestions that will make it easier to review and eventually merge:

It seems that there are 10 commits in the Pull Request at the moment - but most of them are some merges, perhaps of the upstream "master". The real changes seems to me are all happening in the last commit: f8b0bd0 - is this correct?

In order to maintain a clean/tidy history of the main ADCore repository it would be helpful if you could provide a simpler Pull Request with only the real changes. This can be done in a couple of ways:

  • Clean the history of this branch (autosave-TSE) on your fork into a single commit - this can be achieved with the git squash command and it will modify the history of the branch, so you should only do this if no-one else has clones of your fork (or notify them. I suspect the Pull Request will automatically update to reflect this change - if not it will have to be recreated.
  • Create yet another branch on your fork, and cherry pick the specific commit from your original branch. Finally create a new Pull Request from the new branch.

Cheers,
Ulrik

@ulrikpedersen ulrikpedersen mentioned this pull request Jan 8, 2015
@bhill-slac
Copy link
Copy Markdown
Contributor Author

Hi Ulrik
I'm having trouble squashing the extra commits. I'm using git 1.8.1.4
which doesn't have a squash command, so I ran
git rebase -i master
which has a squash option. It seems to work on my local branch, but
when I push to github it just adds more commits to my history,
including one to expand the tabs. I tried closing and recreating the
pull request but it now shows 12 commits.

I'm not confident that cherry pick will work either, as I'd still have at least
2 commits, the bitsPerPixel commit and the expand tabs commit.

Let me know if you have other suggestions. My other option might
be to just start over with a clean branch and apply a patch, which
is how I've been moving changes over from my svn repo.

Thanks,

  • Bruce

On 01/08/2015 03:38 AM, Ulrik Kofoed Pedersen wrote:

Hi Bruce,

Thanks for the Pull Request. I have a few practical comments and suggestions that will make it easier to review and
eventually merge:

It seems that there are 10 commits in the Pull Request at the moment - but most of them are some merges, perhaps of
the upstream "master". The real changes seems to me are all happening in the last commit: f8b0bd0
f8b0bd0 - is this correct?

In order to maintain a clean/tidy history of the main ADCore repository it would be helpful if you could provide a
simpler Pull Request with only the real changes. This can be done in a couple of ways:

  • Clean the history of this branch (autosave-TSE) on your fork into a single commit - this can be achieved with the
    git squash command and it will modify the history of the branch, so you should only do this if no-one else has
    clones of your fork (or notify them. I suspect the Pull Request will automatically update to reflect this change -
    if not it will have to be recreated.
  • Create yet another branch on your fork, and cherry pick
    https://ariejan.net/2010/06/10/cherry-picking-specific-commits-from-another-branch/ the specific commit from
    your original branch. Finally create a new Pull Request from the new branch.

Cheers,
Ulrik


Reply to this email directly or view it on GitHub #50 (comment).

Bruce Hill
Member Technical Staff
SLAC National Accelerator Lab
2575 Sand Hill Road M/S 10
Menlo Park, CA 94025

@ulrikpedersen
Copy link
Copy Markdown
Member

Hi Bruce,

I have just had a go at it. I agree the rebase/squash is a bit difficult to get working - and potentially a bit dangerous to rewrite history anyway. I gave up on that after playing around with it a bit.

Instead I had some success with cherry-pick - the trick is just to identify the exact commits where the real changes happen. I think I figured that out (including one which has spaces instead of tabs). What you will have to do is create a new clean branch off master and then cherry pick the exact useful commits one-by-one from your old branches into this new one. I have done that as an example with these commands:

git checkout master      # Assuming that master is up to date with areaDetector/ADCore
git checkout -b cherries

# Now cherry-picking your specific commits
git cherry-pick f8b0bd07bdef4f6905e844223fb1aa0e553a4ba6
Finished one cherry-pick.
[cherries 317ee57] Add support for initialization and autosave of TSE fields for records that derive from image updates
 Author: Bruce Hill <bhill@slac.stanford.edu>
 2 files changed, 18 insertions(+), 0 deletions(-)

git cherry-pick 1bdc71470086e73f6ebc7a332eaff6d963865bc4
Finished one cherry-pick.
[cherries a67ee81] Adding support for BitsPerPixel and BytesPerPixel PV's in ADBase
 Author: Bruce Hill <bhill@slac.stanford.edu>
 11 files changed, 94 insertions(+), 26 deletions(-)

# Finally I push it up to my own fork
git push ukp cherries 

You can see the result here: https://github.com/ulrikpedersen/ADCore/tree/cherries and review the comparison from there. I think it's even possible for you to create a Pull Request straight off my fork if you wish!

Cheers,
Ulrik

@bhill-slac
Copy link
Copy Markdown
Contributor Author

Hi Ulrik
I've been trying to duplicate this from my bhill-slac/ADCore master branch, but
the problem there is that I don't have a clean master branch.
I made the rookie mistake of committing changes to that branch soon
after I created it back in Oct, so now anything I do on that branch reflects
those changes plus a commit to undo them.

I've tried rebase but haven't been able to find a way to get a clean master branch.
Particularly on github. I can do some cleanup from my local repo using rebase
with the --hard and --onto options, but github complains when I try to push those to origin.
I used -f to force it, but I'm missing lots of Mark's commits now, and still can't get
rid of all of mine.

I tried renaming the branch locally, thinking I could create a new one, but can't
make github remove master, so now it has master and old-master.

I ended up pushing the bitsPerPixel change from your cherries branch, which worked.

I'd like to move to a workflow where my bhill-slac/ADCore master branch only reflects
commits done on areaDetector/ADCore. Then I can always use git pull -ff
to keep it uptodate without creating any unneeded commits, and create new topic
branches for each of my pull requests.

I'm thinking maybe my best option is just to delete the bhill-slac/ADCore fork on github
and create a new clean fork from areaDetector/ADCore.

Any recommendations?

Thanks,

  • Bruce

On 01/09/2015 02:40 AM, Ulrik Kofoed Pedersen wrote:

Hi Bruce,

I have just had a go at it. I agree the rebase/squash is a bit difficult to get working - and potentially a bit
dangerous to rewrite history anyway. I gave up on that after playing around with it a bit.

Instead I had some success with cherry-pick - the trick is just to identify the exact commits where the real changes
happen. I think I figured that out (including one which has spaces instead of tabs). What you will have to do is
create a new clean branch off master and then cherry pick the exact useful commits one-by-one from your old branches
into this new one. I have done that as an example with these commands:

|git checkout master # Assuming that master is up to date with areaDetector/ADCore
git checkout -b cherries

Now cherry-picking your specific commits

git cherry-pick f8b0bd0
Finished one cherry-pick.
[cherries 317ee57] Add support for initialization and autosave of TSE fields for records that derive from image updates
Author: Bruce Hill bhill@slac.stanford.edu
2 files changed, 18 insertions(+), 0 deletions(-)

git cherry-pick 1bdc714
Finished one cherry-pick.
[cherries a67ee81] Adding support for BitsPerPixel and BytesPerPixel PV's in ADBase
Author: Bruce Hill bhill@slac.stanford.edu
11 files changed, 94 insertions(+), 26 deletions(-)

Finally I push it up to my own fork

git push ukp cherries
|

You can see the result here: https://github.com/ulrikpedersen/ADCore/tree/cherries and review the comparison from
there. I think it's even possible for you to create a Pull Request straight off my fork if you wish!

Cheers,
Ulrik


Reply to this email directly or view it on GitHub #50 (comment).

Bruce Hill
Member Technical Staff
SLAC National Accelerator Lab
2575 Sand Hill Road M/S 10
Menlo Park, CA 94025

@ulrikpedersen
Copy link
Copy Markdown
Member

Hi Bruce,

Yes, I see you have got your master branch into a bit of a mess over several months with many merges and I think you are probably right, the best (certainly easier) way may be to simply delete you entire fork, and then create a new one. Your workflow on your new fork is also good: only use the master branch for synchronising with the upstream (i.e. original) ADCore. That is also how I work at the moment. It is a bit of a nuisance to start over, but consider it a hard earned git lesson - I've done it a couple of times before as well :-)

Just make sure that you don't accidentally lose the changes in your Pull Request: i.e. hold on with the delete until we have accepted and merged (or rejected) your Pull Requests.

Cheers,
Ulrik

@MarkRivers
Copy link
Copy Markdown
Member

It looks to me that this breaks backwards compatibility. The default value of TSE is 0, so all existing IOCs will have TSE=0. But this pull request sets the default value of TSE to -2 for some records, which will change the timestamps on these records to be the time the image was collected, rather than the time the record processed. I think we need to preserve backwards compatibility.

@MarkRivers MarkRivers closed this Jan 16, 2015
@MarkRivers
Copy link
Copy Markdown
Member

Whoops, this is the one I accidentally closed, not #51. I am reopening this one and closing #51.

@MarkRivers MarkRivers reopened this Jan 16, 2015
@ulrikpedersen
Copy link
Copy Markdown
Member

Hi @MarkRivers
Actually the TSE field addition is also included in #53 - which really include only 2 commits (2 new features) just because we were messing about with git...

I think we can close this specific Pull Request - but your comment about TSE=0 vs TSE=-2 should apply to #53.

Cheers,
Ulrik

@bhill-slac
Copy link
Copy Markdown
Contributor Author

I'm closing this request as it's a mess and the changes are also in #53.

@bhill-slac bhill-slac closed this Jan 16, 2015
@bhill-slac bhill-slac deleted the autosave-TSE branch January 16, 2015 23:21
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.

3 participants