Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [mylyn-dev] development process

Steffen Pingel wrote:
 I would like to ask how do you plan to review review patches that
already been committed?
By looking at the patch file in a text editor.
 I see...

Anyways, I just wanted to share my concerns, but if others feel like they need patches, I will just comply.

To my knowledge, in order to take a really good look at the patch you
would need the appropriate workspace setup with no changes included
into the patch. But then you can simply run synchronize and see the
same changes grouped by the change sets and use the much more
convenient comparisons editor from there. More over, the Eclipse
Team/CVS integration allow to run comparison for the branch head with
any other branch to get the same change sets and use same comparison
editor without even having the branched code in your workspace.
I haven't used that feature much and don't know if it is pratical. Head is likely to have many changes so it could be difficult to find a specific changeset particularly when other changes are made to the same file.
I do use merge and comparison tools quite often and in my understanding the whole idea wasn't about the head, but about the maintenance branch which won't have big number of changes.

 So, unless I am missing something (which I very well may), the whole
the patch idea is just an unpractical and unnecessary overhead, because
the version control system provides the same information in a form more
usable for review.
It's certainly true that the same information is preserved in CVS but it can difficult to extract a change set from CVS.

Particularly when patches overlap, i.e. multiple changes affect the same file, it becomes harder to track which change is related to specific bug. Patches also make it fairly easy to revert a particular change in case it turns out that it introduces a regression.
In my personal experience overlapping patches are harder to deal with then merging changes. I haven't been able to apply or revert parch on the code that had other changes, but I haven't been applying as many patches as you have. on the other hand I do recall number of requests to contributors to do the merge and recreate patch after other changes went to the code before their patch been applied.

If attaching patches imposes significant overhead it certainly doesn't make any sense. With Mylyn I find that it's very easy to save a patch to the clipboard before committing and attach it to a bug. What is unpractical about it?
You have complained yourself in the past that my patches include unrelated changes. Well, maybe it won't be applicable here (especially when working in the clean workspace with no other changes), but I saw it several times that patch copied into clipboard include unrelated stuff and I had to verify it if it been created correctly. That require extra steps on top of verifying changes before committing (which I do for every commit). There also some limitations in the task editor which doesn't make it any easier (e.g. you can't attach anything to the task that has unsubmitted changes).

Assuming that it doesn't impose much overhead I still think there is a benefit to explictly track the few changes made to the maintenance branch. That said I am fine with trying out the Eclipse synchronize view and change sets. It seems likely that changes will not overlap and it could work well enough to track it that way.

Eugene, how easy is it to revert a change from the synchronize perspective?
For a single incoming change, you just mark that change as merged and commit after that.

To rollback the older changes, then you could do run merge and select a start tag from the past. Then you can do the work in comparison editor and commit changes after that. Another option would be to get specific revision from the history view and commit. The second option would require to do it for each changed file (that is one of the reasons many projects moved to Subversion).

 regards,
 Eugene




Back to the top