On Tue, May 15, 2012 at 5:04 AM, Stefan Sperling <stsp_at_elego.de> wrote:
> On Mon, May 14, 2012 at 11:43:40PM -0400, Greg Stein wrote:
>...
>> My concern is keeping the lower levels free to do what is best for
>> them. The change in this commit is *very* tenuous to its intended
>> purpose. That is going to break at some point. Basically cause it
>> introduces some awful coupling between our subsystems.
>
> Well, what it really does it revert to pre-APR-1.4.6 behaviour.
> I.e. it changes the behaviour back to what it was independent
> of the APR version used.
>
> The intent is to restore a side-effect of this behaviour (stable output).
> We want to keep this side-effect, so we can either keep relying on our
> implicit assumption that a stable hash will produce ordered output,
> or we can sort. I don't care either way. I just want sorted/stable output.
Sorted output is nicer for users.
("stable" just helps to de-confuse)
>> IOW, I'll take a couple milliseconds to perform a sort over the
>> long-term maintenance burden of coupled systems.
>
> You'll have to argue this one with Bert, then :P
If Bert can somehow demonstrate how a qsort() is troublesome, then ...
sure. And if he can somehow show that the cpu clocks are more
important than the long-term ability of our community to maintain svn
against the coupling of "skel prop parsing" and "svn status output"...
then, wow. That would be an amazing demonstration.
> I already told him on IRC I believe sorting dirents in memory won't
> make any noticable difference since we're not going to disk and we're
> not accessing sqlite while sorting, and those expensive operations
> are what 'status' does all the time. But he insisted.
Decisions, discussions, and arguments on list, please.
(all is good; that's what we're doing now)
>> [ to beat this dead horse some more: not a single one of your changes
>> mentioned WHY svn_hash__make() was used instead of apr_hash_make();
>> somebody coming along will have absolutely no idea why that was done;
>> so they might switch it back because there is zero apparent effect at
>> doing so; at a minimum, a comment saying "we sort our skel proplists
>> so that 'svn status' (yah, that thing 15 layers up) can have stable
>> output." ]
>
> The docstring of svn_hash__make() makes the difference quite clear.
That docstring is clear for *that function*. Your change made
absolutely no in-code clarification for *WHY* it was necessary to use
that function. "Go look at the func's docstring" *still* does not make
it clear on *WHY* that hash creator was chosen.
"The grapes are purple" ... yah, great. Facts are stated. Why did you
choose those over the green grapes?
Write a comment for that call to svn_hash__make(). Write a good one.
One that won't make people twitch. One that won't make you squirm
because you're stretching the coupling between skels and the svn
cmdline. I'll buy a round if you can muster that creativity :-D
>...
> Here's my original patch (which won't apply cleanly to HEAD anymore, BTW).
> Note that this still sorts stuff for the benefit of 'svn status' "15
> layers up". I can't find a better place to perform sorting.
Right. As you noted: once you hit a single-node callback, then the
receiver has no choice to sort. But the status walker is much, much
closer to the output than deserialization of some characters.
>...
Cheers,
-g
Received on 2012-05-15 11:39:13 CEST