Skip to content

Conversation

@friederbluemle
Copy link
Contributor

This PR contains general project maintenance related commits:

  • Add Gradle build and wrapper files
  • Make this project buildable through Travis continuous integration
  • Minor code changes (mostly cleanup and some consolidation of code duplication)

@friederbluemle
Copy link
Contributor Author

Hi @koush have you had a chance to look at this PR? Thanks.

@deprogram
Copy link
Contributor

Be nice if these were broken out into related pieces:

  • Gradle/Travis build files
  • Cosmetic changes
  • Refactors/deduplication

As it is, it's a bit unwieldy.

@friederbluemle
Copy link
Contributor Author

Thanks for your comment, @deprogram
I already broke up the changes into small, targeted commits with meaningful and specific commit messages. You can click each individual commit to see the diff. Not sure what you're asking for.. Do you want me to send separate pull requests for each commit? Seems overkill.

I just rebased my commits and this PR should be ready to merge. Travis CI build passes as well.
@ctso Can you check this please?

There were several tab/spaces mixups in the commits that went into the repo after I initially submitted the PR. During the rebase I fixed these as well, and added it into 944b285 Fix whitespace and formatting (instead of adding a new commit).

@deprogram
Copy link
Contributor

I just like cosmetic changes to be treated separately from changes to program logic. I don't mind cosmetic changes, I just don't want to be forced to accept them along with functional changes that I actually want.

I also have to have QA run a pass on such an extensive set of changes. Not a big deal, but it's another reason it hasn't simply been merged. :)

@friederbluemle
Copy link
Contributor Author

I rebased my commits onto the latest master, and noticed that someone force pushed to master, effectively erasing commits and screwing up my (and everybody else's) local branches (if the commits were based on one in the rewritten history). The merge conflicts were nasty. This really should never happen.

Regarding your comments: What would you suggest? Do you want me to split up this PR into two or three separate pull requests?
Thanks.

@ctso
Copy link

ctso commented Jul 3, 2014

I'm entirely unsure who force pushed, but this displeases me as much as it displeases you.

I'll review this change over the long weekend in its entirety. I believe there was a desire to split this out into smaller more manageable change requests so we can cherry pick the changes we want. However, just a quick glance at what is changing, I don't really see a need to split it up right now.

@friederbluemle
Copy link
Contributor Author

Great, thanks for your comments and review @ctso.

I replied to your comments and rebased my commits.

@friederbluemle
Copy link
Contributor Author

There was another force push that caused rewritten history on the master branch (@ctso's commit 985f101 was deleted). This time it was pretty clear what caused it: The Automatic translation import commit. I assume there is a script running somewhere that automatically force pushes without rebasing or merging upstream changes first.. @mikeNG Do you know how this happened?

@rmcc
Copy link

rmcc commented Jul 19, 2014

That sounds like gerrit doing its job. any changes that it doesn't know about will be clobbered, by design.

@ctso , this is under gerrit; please be sure you push any changes through there

@friederbluemle
Copy link
Contributor Author

Hmm, that sounds like a questionable design, or an unclear collaboration workflow to me. @rmcc when you say "this is under gerrit", are you implying that this is not the authoritative repository? If so, please advise how to properly contribute, in case GitHub pull requests are not the right method. Relying on someone to manually sync any changes done here to some other repo sounds like a flawed process.
Thanks.

@rmcc
Copy link

rmcc commented Jul 19, 2014

Yeah, that's what I mean. gerrit's copy (http://r.cyanogenmod.org) is the authoritative repository and all submissions should go through there (http://wiki.cyanogenmod.org/w/Doc:_using_gerrit). In case of conflicts, gerrit will forcepush its own copy on top of github's, which appears to be what's happening here.

The wiki instructions assume the repository is part of the android manifest (and mentions the "repo" tool) which doesn't quite apply in this case. Pushing to ":refs/for/branchname" (i.e., "git push ssh://r.cyanogenmod.org:29418/cyngn/OneClickAndroid HEAD:refs/for/master") will do the same as the "repo upload" calls mentioned in that document.

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.

4 participants