I will take a look at doing this over the weekend. This was my first time using both git and github so I'm not surprised that I didn't work in a very git-like way.
-Brian On Sep 8, 12:33 pm, Christian Boos <cb...@neuf.fr> wrote: > Hello Brian, > > On 9/7/2010 2:34 AM, Brian Meeker wrote: > > > > 134135 > > > Most of the work on #525 [1] is now done and I would like to start > > getting some feedback. There is also a proposal [2]. I added a pull > > request on GitHub just to pull everything together in one place [2]. > > Here is what I have done: > > > * New batch modify module. > > * Batch modify UI integrated into query page. > > * New permission (TICKET_BATCH_MODIFY) > > * Added a new page to the TracGuide and updated the TOC macro. > > * Wrote tests against new module. > > > I still have a couple of outstanding issues as well. Any suggestions > > would be appreciated. > > > * One of the new batch tests fails. Right now I think the problem is > > with the test itself. I cannot reproduce the error through a running > > Trac instance. I'm probably doing something wrong with the > > EnvironmentStub. > > * I don't have any tests on my query page integration. Are there any > > examples of permission testing I can look at? I want to test that > > batch data is only added when the user has the appropriate permission. > > * The documentation page I added to the wiki feels off. I don't feel > > like I'm explaining the configuration options in a clean way. > > Suggestions welcome. > > > I moved the ticket into 0.13 since I am mostly done. > > Thanks! I'll start to review this soon... > > > I would like to > > make a patch to add to the ticket, but I need to figure out the best > > way to do that with git. If anybody knows how to easily make a diff > > from all of my commits let me know. This is my first time using Git > > and GitHub. > > > Thanks, > > Brian > > > [1]136http://trac.edgewall.org/ticket/525 > > [2]137http://trac.edgewall.org/wiki/TracDev/Proposals/BatchModification > > [3]138http://github.com/edgewall/trac/pull/1 > > But before that, some notes about the development process itself... > > First, it's great that you've started to use git! You're one of the > first to do so, and there were no precise instructions about the > workflow to use, so not surprisingly there are some rough edges ... > As it stands, your changes are a bit difficult to review: you've mixed > them with changes taken from upstream, not to mention those from jomae > that you later reverted ;-). I suppose this happened because you used > the "Fork Queue" feature from github. That tool was rather designed for > "upstream" owners who'd like to integrate contributor changes, so the > reverse situation than yours actually. Even without this tool, you could > have done it wrong by repeatedly merging from upstream. This also would > not have been optimal. > > Ideally, you should have made only "Batch Modifications" related > changes, in your fork. > It is however possible to fix this, let me try to guide you through. > > Your first commit was df77576c9d1da2f78523ea25d4994a4e819fb9c1. > After some filtering, I came down to a reduced list of genuine > changesets of yours, which we can cherry-pick to a new topic branch. > That branch can simply be restarted from the above changeset. > > i.e. > > $ git checkout -b ticket525-batchmodify > df77576c9d1da2f78523ea25d4994a4e819fb9c1 > > Then we re-apply the genuine changes only: > > $ git cherry-pick c96bace617182075008cea62435447ef22affee4 > ... > $ git cherry-pick bb9a87373c746bc052370579b44f1eea0cf0c2bc > > I think I got the list right, so instead of redoing the filtering and > cherry pick sequence by yourself, you can directly pull the branch from > me and verify I picked the appropriate changes ;-) > > From inside the git working copy of your repository, first verify > you're on the right branch: > > $ git branch > ticket525-batchmodify > > (if not, you missed the `git checkout -b ...` step above) > > $ git pull139http://github.com/cboos/trac.gitticket525-batchmodify > > You can now push that branch to your own github repository: > > $ git push origin ticket525-batchmodify > > And you can now resume work on that branch. Having such a "linear" > branch is good enough for a review, and github even makes it easily > possible to review the changes as a whole > (e.g.140http://github.com/cboos/trac/compare/ticket525-batchmodify). > > If we manage to make branches reasonably short-lived, your changes can > be later integrated back in Subversion without requiring you to first > merge more changes from upstream. In fact, there are only two reasons I > see for merging changes from upstream in a topic branch: > - you want to take advantage from some new code (making your job easier) > - you want to check for conflicts, and solve them if any (making our > job easier, thanks!) > > In either case, from our perspective it's better if you'd *rebase* your > changes, in order to keep your commits in a linear sequence and > therefore easier to review. To that end, you can do something like the > following: > > $ git remote add edgewall141http://github.com/edgewall/trac.git# do that once > $ git pull --rebase edgewall trunk > > Once the rebase is complete, remember that you have to "force" (-f) when > pushing. This won't play well with any derived branches of your branch, > but for a topic branch like this one, there are usually no derived > branches, so that shouldn't be a problem. > > Note also that when you were just checking for conflicts and the rebase > went through entirely automatically, there's no value in pushing the > changes to the outside and you can always "undo" this rebase by > resetting the branch to its previous state (see for > reference142http://stackoverflow.com/questions/134882/undoing-a-git-rebasefor > details, which saves me the caveats about reset ;-) ). > > In summary: > - always create a topic branch when working on something, never commit > directly on a branch mirroring a remote (i.e. 'trunk' in the case of > Trac, which corresponds to 'master' in most other projects) > - try your best to keep the changes linear (favor rebase over merge) > - don't (ab)use github's "Fork Queue" ;-) > > We can discuss and amend the above guidelines in more details, I'll then > summarize the results in the wiki. > > -- Christian -- You received this message because you are subscribed to the Google Groups "Trac Development" group. To post to this group, send email to trac-...@googlegroups.com. To unsubscribe from this group, send email to trac-dev+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/trac-dev?hl=en.
