Skip to content

Addressing issue (https://github.com/scijava/scijava-common/issues/128) ...#129

Merged
dscho merged 1 commit intoscijava:masterfrom
4Quant:master
Oct 11, 2014
Merged

Addressing issue (https://github.com/scijava/scijava-common/issues/128) ...#129
dscho merged 1 commit intoscijava:masterfrom
4Quant:master

Conversation

@kmader
Copy link
Contributor

@kmader kmader commented Oct 8, 2014

...by having the IndexReader ignore new line characters introduced by Maven Shade AppendingTransformer.

@kmader
Copy link
Contributor Author

kmader commented Oct 8, 2014

It passes all of the unit tests and I have tested on my project and it works perfectly. I cannot really think of any new issues it might cause, but I do not have much insight into what the entire scijava framework looks like.

@dscho
Copy link
Contributor

dscho commented Oct 8, 2014

That looks already pretty nice! I would like to ask for one more thing: in this project, we like to rewrite topic branches to look pretty before merging (actually, the real reason is that we do not want to merge commits fixing problems introduced in the same topic branch, that just makes bisecting, following the history, and understanding the changes hard). I wrote up some helpful comments in another OSS project how to rewrite topic branches.

@kmader
Copy link
Contributor Author

kmader commented Oct 8, 2014

Sorry I am really not experienced in git, can you tell me exactly what I need to do? My bash history is

git
git log
git commit --fixup 0aba408d2e1f159b4e4435fd81c77e1ca83138f2
git commit --fixup 779586657c39e9e1aca215f1fec27a09aa1a81a5
git rebase --autosquash -i origin/master
git rebase --continue
git push origin +HEAD
git log
git rebase -i HEAD~2
git commit --amend
git rebase --continue
git log
git commit --amend
git commit --fixup 779586657c39e9e1aca215f1fec27a09aa1a81a5
git commit --amend
git rebase --continue
git rebase --continue
git rebase --autosquash -i origin/master
git rebase --edit-todo
git rebase --continue
git rebase --continue
git add src/main/java/org/scijava/annotations/AbstractIndexWriter.java
git add src/main/java/org/scijava/annotations/IndexReader.java
git commit --amend
git rebase --continue
git push origin +HEAD

WIth a number of pick and squash commands but I don't appear to have fixed the issue.

@dscho
Copy link
Contributor

dscho commented Oct 8, 2014

Your bash history looks pretty good, there is only one problem: the previous commits were not created using --fixup and hence the commits were not squashed into one.

So maybe the best idea would be to do this: git rebase -i origin/master, in the edit list remove the line starting with pick 585dbec Addressing... (this is an incorrect commit adding some unwanted confict markers) and then replacing the pick of the second and third commit by fixup. Then quit the editor and let Git do its magic.

Explanation: three commits needed to be squashed into a single one, a fourth commit dropped. The latter was achieved by deleting the corresponding line, and the former by calling fixup instead of pick on the fixup commits.

@dscho
Copy link
Contributor

dscho commented Oct 8, 2014

I am sorry, my advice was not very helpful. Let me try to be more helpful by providing the cleaned up branch.

When creating uber jars via Maven Shade's AppendingTransformer, the
annotation indexes are concatenated, separated by line feed characters.

So let's just ingore whitespace between indexed annotations, handling the
illustrated problem gracefully.

This fixes scijava#128.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho
Copy link
Contributor

dscho commented Oct 8, 2014

Okay, so I used a combination of git rebase -i and git checkout -p to modify the changes. I also suggested a new commit message: 9b82d16

If you agree, could you please execute the following steps to update this pull request?

git fetch https://github.com/scijava/scijava-common concatenated-annotations
git reset --hard 9b82d16c49738e1ec6cf848c80e78e8a7ce4a76c
git push origin +HEAD

@dscho
Copy link
Contributor

dscho commented Oct 10, 2014

@kmader did you have a chance to look over the suggested final version of the commit?

@kmader
Copy link
Contributor Author

kmader commented Oct 10, 2014

Yes they look fine, I think I have now pushed everything correctly so it should be good to go

dscho added a commit that referenced this pull request Oct 11, 2014
@dscho dscho merged commit d0c26b5 into scijava:master Oct 11, 2014
@dscho
Copy link
Contributor

dscho commented Oct 11, 2014

Thanks so much!

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.

2 participants