run_acq_and_save()

[Cody-G]

In the most recent commits to the waveform branch run_acq_and_save_wav() seems to have been merged into run_acq_and_save() in data_acq_thread.cpp. I think that's a good thing, and it's what we have been shooting for. But it seems some things may have been lost in the merge. In particular my most recent patch (see here) is no longer present in run_acq_and_save(). That one is easy to fix, but I'm concerned there may be more differences. It's a little hard to read in the commit history. @kdw503 Is this problem limited to that patch that I referenced? Is there a reason you didn't just delete run_acq_and_save() and rename run_acq_and_save_wav() to run_acq_and_save()?

The commit that made the merge is here: https://github.com/HolyLab/Imagine/commit/5274176fe0dfe3db4b97cbb5be38ab7115dd2f8e

[kdw503]

I did like this.

1. I confirmed your pull request in origin repository.
2. I stashed my local changes, Among these changes, I deleted old run_acq_and_save() and renamed run_acq_and_save_wav() to run_acq_and_save().
3. I pulled original waveform to my local directory.
4. I tried 'git stash apply' --> at this point, merge conflict happened.
5. I executed git gui and chose remote lines where conflict happened.

I think I should have commit and push my local change first before confirm pull request, right?

[Cody-G]

There are at least two correct ways to do it: integrate your changes first and then mine, or integrate my changes first and then yours. Sometimes one of those is easier than the other, but they are both correct. You were trying to integrate my changes first, which is fine. In either case the merge conflict is a bit ugly since you made a lot of changes to that file. My guess is that something went wrong in step 5. The "remote" version of the patched line was probably marked in git as the line in the run_acq_and_save() function, not the line that I changed in the run_acq_and_save_wav() function. This particular case is tricky because there were two nearly-duplicated functions. In general merges will be easier to execute if you commit more often because it's easier to understand the conflicts. It may also be more comfortable for you to merge two branches instead of merging a branch with a stash. Since the stash doesn't have its own branch it feels more fragile and difficult to recover from mistakes. With that in mind I may have done this order instead:

1. I confirmed you pull request in this repository.
2. git checkout -b temp_branch and commit my local changes (preferably multiple small commits), in these changes, I deleted old run_acq_and_save() and renamed run_acq_and_save_wav() to acq_and_save().
3. git checkout waveform. I pulled original waveform to my local directory.
4. git merge temp_branch OR you can checkout temp_branch and run git rebase waveform (I prefer rebase)
5. Now fix merge conflicts either in gui or in text editor

On Mon, Oct 9, 2017 at 10:31 PM, Dae Woo Kim notifications@github.com wrote:

I did like this.

1. I confirmed you pull request in this repository.
2. I stash my local changes, in these changes, I deleted old run_acq_and_save() and renamed run_acq_and_save_wav() to acq_and_save().
3. I pulled original waveform to my local directory.
4. I tried 'git stash apply' --> at this time merge conflict happen.
5. I executed git gui and choose remote lines where conflict happened.

I think I should have commit and push my local change first before confirm pull request, right?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/HolyLab/Imagine/issues/56#issuecomment-335350490, or mute the thread https://github.com/notifications/unsubscribe-auth/AE78hLgi0TBwHYJQK5dF17n2XCZ8hyMJks5squT5gaJpZM4PzJ__ .

[Cody-G]

Oh, and you should rarely need to use git push -f (I don't think you did that this time but just fyi). It ignores merge conflicts and completely replaces the target branch with what you are pushing. I use it sometimes when I'm sure that no one else is working on the same branch as me, but that's the only time it's safe. If anyone tries to make changes to that branch later then it's a big pain because their commit history won't be compatible with the target.

[kdw503]

OK, I learned a lot. Anyway merge conflict happens in this case right? Fortunately, only run_acq_and_save() function has some merging problems in this time. When I checked my part it looks ok. Would you fix your changes and request pull again?

[Cody-G]

Yes a merge conflict happens in any case. Usually it's the committer's responsibility to make sure that the conflict is handled correctly. That is easier to verify if one uses rebase instead of merge, so I think that's one reason some people prefer rebase. If you use rebase, then there will be no "merge commit" in the history and we can easily see whether your changes disrupted previous commit(s).

Are you asking that I make a new commit that has the same changes? If you are totally sure that only those lines are affected then I can do that, but it's not considered the best practice. I think one better practice here would be for you to make a new commit that "reverts" the bad merge commit, then do the merge or rebase again in a way that fixes the conflict correctly. If you want to try that you can do it in a separate branch and submit a pull request so that we can look over it together before merging it into the waveform branch. For example you could do everything I just suggested on a fix_waveform branch before submitting the pull request into the waveform branch. I make new branches liberally when trying new things with git: it's completely safe because if you mess up a branch you can just go back to the original branch, make a new branch, and try again.

[kdw503]

Ok, I will try the better practice one.