I just created a new branch as you described above. Everything is now at http://github.com/CuriousCurmudgeon/trac-batch-modify/tree/ticket525-batchmodify. Have you had a chance to review the code itself?
As far as the wiki goes, I think the points in your summary are a good start. What messed me up was that I was just using a git command reference to learn, but didn't read anything about what is considered a proper git workflow. I think pointing to some resources on proper git use in the wiki would be helpful. Thanks for the help. I didn't know anything about cherry-pick or how to properly rebase. I've been learning as I go, but it sounds like it's time to grab a book to learn git in some depth. On Sep 8, 6:42 pm, Brian Meeker <meeker.br...@gmail.com> wrote: > 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 <cb168...@neuf.fr> wrote: > > 169170 > > > 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]136171http://trac.edgewall.org/ticket/525 > > > [2]137172http://trac.edgewall.org/wiki/TracDev/Proposals/BatchModification > > > [3]138173http://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 pull139174http://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.140175http://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 edgewall141176http://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 > > reference142177http://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.
