Discussing two changesets
Manuzhai
I'd like to discuss two recent changesets with which I don't necessarily agree.
The first is r4769 [1], which fixes issue #3421 [2]. This patch
visually merges two or more consecutive changesets when the author and
commit messages are the same, but I'm not sure this is a good idea. In
my opinion, the timeline is important in that it serves as a good
overview of what's been happening to a project, but with this patch,
information on changesets can is conflated. Of course, having many
commits with a similar/the same message is in most cases just a Bad
Idea, and I don't like how Trac codifies that this is okay. Also, with
the current code this is not a preference, so I can't easily turn this
behavior off.
I would suggest that the changeset be entirely reverted, so the
timeline just displays the changesets as-is (my preference), or that
this is turned into a preference. Either way, I feel this feature is
somewhat orthogonal to Trac's KISS principles.
The second is r4786 [3], which fixes issue #1925 [4]. Now, I'm
actually the reporter on this bug, so obviously I care about this
little thing (I have maintained my working copy with a patch that
fixes this for 2 years). I think it's an unobtrusive feature that
makes the reports easier to use at the expense of an optional extra
preference. I recently posted my patch to the comments since a
committer (cboos) finally showed some interest in the ticket (it was
rejected by cmlenz initially because he figured that if I just wanted
the open tickets list I could get it by disabling the reports module
and use the query module and its default config).
In r4786, cboos fixed the ticket by making his own version of my
patch. Instead of changing the navcontributor in the reports module to
check for, then link to, the default_report option value, he chose to
have a default -1 value for the option, then make /report/-1 link to
the list (instead of /report, as before) and make /report link to the
default report. I think this is a bad idea for two reasons: (a) -1 is
kind of an ugly magic value that suddenly does something different,
which should not be exposed to the URL interface (and preferably not
to .ini file either, I'd prefer having something None-like or just
leaving the default_report option out if it's not wanted). The other
reason (b) is that this changes links to the report list and the
default report from before:
http://trac.xavamedia.nl/nts/report becomes
http://trac.xavamedia.nl/nts/report/-1
http://trac.xavamedia.nl/nts/report/1 becomes
http://trac.xavamedia.nl/nts/report
(Provided 1 is the default_report.)
So I'd like to ask the list to consider these sentiments. If this is
just whining, I'll just shut up (about these issues) and maintain my
own patches. If you agree, however, may be speak up and discuss how
minor improvements should be implemented.
Regards,
Manuzhai
[1] http://trac.edgewall.org/changeset/4769
[2] http://trac.edgewall.org/ticket/3421
[3] http://trac.edgewall.org/changeset/4786
[4] http://trac.edgewall.org/ticket/1925
Christian Boos
Manuzhai wrote:
> So I'd like to ask the list to consider these sentiments. ...
>
Well, there's no point to jumping on the guns, as changes can always be
discussed/amended.
It's not because those changes are going into trunk that they're carved
in stone.
Quite to the contrary, I think that sometimes giving more exposure to a
change enables more people to discuss about them...
> The first is r4769 [1], which fixes issue #3421 [2]. This patch
> visually merges two or more consecutive changesets ...
> Of course, having many
> commits with a similar/the same message is in most cases just a Bad
> Idea, and I don't like how Trac codifies that this is okay.
Well, with this change, Trac doesn't encourage that behavior, it just
better copes with those admittedly bad things when they happen. For
example, we frequently have a sequence of "Copied/Renamed remotely"
changesets, due to a sloppy use of TortoiseSVN... Compressing such
changesets into one entry improves a bit the clarity of the timeline.
One could argue that I could have saved the time coding the feature by
educating the users, oh well...
> Also, with
> the current code this is not a preference, so I can't easily turn this
> behavior off.
>
That could be a way.
> I would suggest that the changeset be entirely reverted, so the
> timeline just displays the changesets as-is (my preference), or that
> this is turned into a preference. Either way, I feel this feature is
> somewhat orthogonal to Trac's KISS principles.
>
Well, nobody said anything /against/ the proposed feature and
furthermore it's quite similar to #38, which aims at doing the same for
Wiki edits, so it's not really out-of-line of Trac's principles.
But I'm OK with making that optional.
> The second is r4786 [3], which fixes issue #1925 [4].
> ...
> In r4786, cboos fixed the ticket by making his own version of my
> patch. Instead of changing the navcontributor in the reports module to
> check for, then link to, the default_report option value, he chose to
> have a default -1 value for the option, then make /report/-1 link to
> the list (instead of /report, as before) and make /report link to the
> default report.
I thought it was nicer to have ''View Tickets'' stay as /report and have
that linking to the default report.
It's true that sometimes we get a list of resources when pointing to the
"realm" (e.g. milestone/ -> the list of milestones, aka. the roadmap),
but sometimes we also get the default resource for it (e.g. wiki/ -> the
WikiStart page).
> I think this is a bad idea for two reasons: (a) -1 is
> kind of an ugly magic value that suddenly does something different,
> which should not be exposed to the URL interface (and preferably not
> to .ini file either, I'd prefer having something None-like or just
> leaving the default_report option out if it's not wanted). The other
> reason (b) is that this changes links to the report list and the
> default report from before:
>
> http://trac.xavamedia.nl/nts/report becomes
> http://trac.xavamedia.nl/nts/report/-1
> http://trac.xavamedia.nl/nts/report/1 becomes
> http://trac.xavamedia.nl/nts/report
>
> (Provided 1 is the default_report.)
>
The problem I see with specifying the default report number in the path
is that you can't really "export" a link to the default report, as the
way you did it, you have to give the report number in the URL.
Perhaps by using a /report/default link one could achieve both goals?
-- Christian
Waldemar Kornewald
On 2/19/07, Christian Boos <cb...@neuf.fr> wrote:
> Well, nobody said anything /against/ the proposed feature and
> furthermore it's quite similar to #38, which aims at doing the same for
> Wiki edits, so it's not really out-of-line of Trac's principles.
> But I'm OK with making that optional.
Maybe we can find a better solution than making it optional. What
about allowing to expand the list in-place?
What is the exact problem with the new behavior?
Bye,
Waldemar Kornewald
Eli Carter
>
> Hello all,
>
> I'd like to discuss two recent changesets with which I don't necessarily
agree.
>
> The first is r4769 [1], which fixes issue #3421 [2]. This patch
> visually merges two or more consecutive changesets when the author and
> commit messages are the same, but I'm not sure this is a good idea. In
> my opinion, the timeline is important in that it serves as a good
> overview of what's been happening to a project, but with this patch,
> information on changesets can is conflated. Of course, having many
> commits with a similar/the same message is in most cases just a Bad
> Idea, and I don't like how Trac codifies that this is okay. Also, with
> the current code this is not a preference, so I can't easily turn this
> behavior off.
I'll second this. I don't want Trac coalescing changesets.
Eli
Eli Carter
>
> Hello Manuzhai,
>
> Manuzhai wrote:
> > The second is r4786 [3], which fixes issue #1925 [4].
> > ...
> > In r4786, cboos fixed the ticket by making his own version of my
> > patch. Instead of changing the navcontributor in the reports module to
> > check for, then link to, the default_report option value, he chose to
> > have a default -1 value for the option, then make /report/-1 link to
> > the list (instead of /report, as before) and make /report link to the
> > default report.
>
> I thought it was nicer to have ''View Tickets'' stay as /report and have
> that linking to the default report.
> It's true that sometimes we get a list of resources when pointing to the
> "realm" (e.g. milestone/ -> the list of milestones, aka. the roadmap),
> but sometimes we also get the default resource for it (e.g. wiki/ -> the
> WikiStart page).
I think in this case, "View Tickets" should point to the default, and /report
should stay as a list of available reports. Reasons in the next section.
> > I think this is a bad idea for two reasons: (a) -1 is
> > kind of an ugly magic value that suddenly does something different,
> > which should not be exposed to the URL interface (and preferably not
> > to .ini file either, I'd prefer having something None-like or just
> > leaving the default_report option out if it's not wanted). The other
> > reason (b) is that this changes links to the report list and the
> > default report from before:
> >
> > http://trac.xavamedia.nl/nts/report becomes
> > http://trac.xavamedia.nl/nts/report/-1
> > http://trac.xavamedia.nl/nts/report/1 becomes
> > http://trac.xavamedia.nl/nts/report
> >
> > (Provided 1 is the default_report.)
> >
>
> The problem I see with specifying the default report number in the path
> is that you can't really "export" a link to the default report, as the
> way you did it, you have to give the report number in the URL.
> Perhaps by using a /report/default link one could achieve both goals?
Something else to think about here: making the query module the default while
keeping the report module enabled. If you make 'view tickets' point to the
default report explicitly, it's a simple step from there to make it also
allow the default to point to the query module.
Examples:
default_report = report (The default value)
default_report = report/4
default_report = query
default_report = query?status=new&status=reopened
I think that approach would be more flexible, wouldn't introduce the "-1"
magic, and wouldn't change urls. (Further, if someone wrote a completely
different report module as a plugin, they could set that as the default
with "default_report = advancedquery" or the like.)
Yes, this could be "abused" with an entry like "default_report =
wiki/CommonQueries", but maybe that would be useful too, come to think of
it...
Thoughts?
Eli
Emmanuel Blot
So do I.
I'm not sure whether this feature can be useful, but if it is
implemented and kept, it should be optional and *not* be enabled in
the default settings.
Knowing that there is a proliferation of configuration options that
will eventually make Trac harder to administrate, I'd rather see this
behaviour implemented as an external plugin rather than to be
implemented in Trac core.
Cheers,
Manu
Christian Boos
> ...
> I think in this case, "View Tickets" should point to the default, and /report
> should stay as a list of available reports. Reasons in the next section.
>
Ok, I think there's overwhelming support for this ;-)
> Something else to think about here: making the query module the default while
> keeping the report module enabled. If you make 'view tickets' point to the
> default report explicitly, it's a simple step from there to make it also
> allow the default to point to the query module.
>
> Examples:
> default_report = report (The default value)
> default_report = report/4
> default_report = query
> default_report = query?status=new&status=reopened
>
> I think that approach would be more flexible, wouldn't introduce the "-1"
> magic, and wouldn't change urls. (Further, if someone wrote a completely
> different report module as a plugin, they could set that as the default
> with "default_report = advancedquery" or the like.)
> Yes, this could be "abused" with an entry like "default_report =
> wiki/CommonQueries", but maybe that would be useful too, come to think of
> it...
>
> Thoughts?
That's interesting, before I jump on implementing that, if anyone
disagrees, please speak now ...
-- Christian
Christian Boos
as I didn't see the mail go through, I'm reposting)
Emmanuel Blot wrote:
>> I'll second this. I don't want Trac coalescing changesets.
>>
>
> So do I.
> I'm not sure whether this feature can be useful, but if it is
> implemented and kept, it should be optional and *not* be enabled in
> the default settings.
Well, have you checked what the feature does precisely?
I don't see what harm it can do: only strictly consecutive changesets,
having the same author and same message are replaced by a "Changesets
[from:to]" entry, which saves space, but doesn't loose any information
(except you get only the time for the last changeset in the sequence).
You can get to the full information ("expand" the list, as Waldemar
suggested) simply by clicking on the link, which takes you to the
corresponding log view for that revision range. From there, you have a
quick way to grasp the whole span of changes at once, by using the view
differences button, or you can go to the individual changes.
I'm OK to make it optional for those not wanting that feature, but I
don't see any good reason to turn it /off/ by default.
-- Christian
John Hampton
> Eli Carter wrote:
>> Something else to think about here: making the query module the default while
>> keeping the report module enabled. If you make 'view tickets' point to the
>> default report explicitly, it's a simple step from there to make it also
>> allow the default to point to the query module.
>>
>> Examples:
>> default_report = report (The default value)
>> default_report = report/4
>> default_report = query
>> default_report = query?status=new&status=reopened
>>
>> I think that approach would be more flexible, wouldn't introduce the "-1"
>> magic, and wouldn't change urls. (Further, if someone wrote a completely
>> different report module as a plugin, they could set that as the default
>> with "default_report = advancedquery" or the like.)
>> Yes, this could be "abused" with an entry like "default_report =
>> wiki/CommonQueries", but maybe that would be useful too, come to think of
>> it...
>>
>> Thoughts?
>
> That's interesting, before I jump on implementing that, if anyone
> disagrees, please speak now ...
I actually think that this is a good idea. It gives the flexibility
needed at a very low cost. So +1 from me.
-John
Emmanuel Blot
> don't see any good reason to turn it /off/ by default.
Ok, let's make it on by default.
What I don't like here is that Trac will not show the same number of
entries than another SVN client, which may lead to confusion. If it
can be switched off, I'm ok with this feature.
Cheers,
Manu
Matt Good
Well, the problem I have with this is that it's bad for the RSS feed.
Collapsing the entries means that the old entry is now gone from the
feed and replaced with an "overlapping" entry, which I don't like.
However, I don't think there's a way to handle this one way for the
HTML version and another way for RSS. IMO repeating commit messages
is a Very Bad Thing(TM), so while this isn't bad from the web view I
certainly wouldn't miss it if it wasn't there.
-- Matt Good
Christian Boos
Well, I think we just have a little difference in the perspective about
how the Timeline should behave.
In my view, the Timeline should present a /readable summary/ of the
activity of the day, not necessarily a rigorous listing of the events.
In that perspective, having one entry "Changesets [100-105]:
retructuring the library structure" is better than 6 consecutive
entries sharing the same message (not defending this practice,
wholeheartedly agreeing with Matt on that point, but merely
acknowledging that it happens). In that perspective also, collapsing
multiple consecutive edits done on a Wiki page by the same author would
be an improvement as well, and this happens even more frequently than
with changesets (#38).
Now, the exact information, changeset by changeset, is available as
well, using the Revision Log, which is also able to provide an RSS feed
with those events listed individually.
However, I think there's a way to make everybody happy on this point,
it's simply to have a 'collapse similar events' check box in the
advanced preference panel. Based on this boolean, event providers (the
changeset one, the wiki one) would collapse or not their events. That
would be a simple "collapse=1" in the links, so it would make easy to
get RSS feeds without the collapsing.
The "advanced" preference panel is something I'd like to introduce for
adding more options: filtering by author - #1198, filtering by paths -
#1135 (and there was another one for filtering on wiki page names), show
abbreviated content - #2360, etc.
-- Christian
Marc-Antoine Zizka
>
> Well, have you checked what the feature does precisely?
> I don't see what harm it can do: only strictly consecutive changesets,
> having the same author and same message are replaced by a "Changesets
> [from:to]" entry, which saves space, but doesn't loose any information
> (except you get only the time for the last changeset in the sequence).
Just wondering here... will this collapse changesets from the same author
with an empty message? Some developers are pretty stubborn and do not
comment their changes. I don't think empty messages should be "merged" in
this way.
Waldemar Kornewald
> Just wondering here... will this collapse changesets from the same author
> with an empty message? Some developers are pretty stubborn and do not
> comment their changes. I don't think empty messages should be "merged" in
> this way.
In what way is this different from merging changesets which have a comment?
Bye,
Waldemar Kornewald
Marc-Antoine Zizka
>
>> Just wondering here... will this collapse changesets from the same
>> author with an empty message? Some developers are pretty stubborn and do
>> not comment their changes. I don't think empty messages should be
>> "merged" in this way.
>
> In what way is this different from merging changesets which have a
> comment?
I guess it's the same, but my issue is that some developers simply do not
comment their changes. Therefore if they make X consecutive changes, they
will always be merged in the timeline. This behaviour should definitely be
optional.
Alec Thomas
1. As Matt mentioned, it breaks the expected behaviour of the RSS feed.
2. It doesn't make sense if other timline events should have occurred in
between the coalesced events. eg. a wiki page is changed in between
two changesets that are coalesced.
3. It is inconsistent with other timeline providers, such as the Wiki,
where identical changes are not coalesced.
That being said, I think the ability to collapse timeline events is useful, but
I think it should be occurring in the timeline module itself, rather than
individual event providers.
Two options for representing this that I can think of off the top of my head,
with both requiring the timline module to detect identical sequential events and
mark them as being from the same "group" of coalesced events (ie. a grouping_id):
1. Draw every event and have JS collapse/expand them dynamically. This
might clutter the interface though, if not done right.
2. Have an option in the INI file for coalescing the events server-side.
Possibly with a toggle in the UI for overriding it.
--
Evolution: Taking care of those too stupid to take care of themselves.
Christian Boos
Just a small follow-up to mention I finally got around fixing this: the
problematic changeset that started this all was reverted and a much more
general solution has been implemented in
http://trac.edgewall.org/changeset/5251, enjoy!
-- Christian
Eli Carter
Very cool. I like the ability to rename things there too. :)
Eli