Hello Brian,
On 9/7/2010 2:34 AM, Brian Meeker wrote:
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] http://trac.edgewall.org/ticket/525
[2] http://trac.edgewall.org/wiki/TracDev/Proposals/BatchModification
[3] http://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 pull http://github.com/cboos/trac.git ticket525-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.
http://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 edgewall http://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 reference
http://stackoverflow.com/questions/134882/undoing-a-git-rebase for
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.
