On Saturday 01 November 2003 1:47 pm, Philip Martin wrote:
> > I was just following what revert-cmd.c does. What's the benefit
> > of clearing at the start of the loop?
>
> See the HACKING file. The project's knowledge about the use of pools
> has developed over time, not all of the existing code follows the best
> practice.
I hadn't read that bit of HACKING (mea culpa), but it still
doesn't explain what the benefit is. Brian observes that if
one uses the "continue" statement then clearing at the
start of the loop body will make sure everything gets freed
each iteration, which seems plausible.
> >> You don't appear to have deleted the subpool variable.
> >
> > Are we reading the same code? :-) The subpool is destroyed
> > immediately on exit from the loop.
>
> You don't appear to have deleted the declaration.
Ohhhhh. So I haven't. My apologies for misinterpreting
your comment.
> > 4. The name "out" for a clean-everything-up-and-return label
> > is taken from the original revert.c code, but I don't like it. I'd
> > prefer "finish" or something. Oh, and I'd like it painted dark
> > green.
>
> I dislike the way svn_client_revert uses labels irrespective of the
> name, it's too easy to introduce a line "SVN_ERR (some_func())" that
> isn't "label aware", witness my first comment above. When there is
> some essential post-processing that must always be done I prefer to
> use a wrapper function, see svn_repos_finish_report in
> libsvn_repos/reporter.c.
Sounds reasonable.
Yet another version of the patch is attached. Changes:
- Delete superfluous subpool variable in rename-cmd.c.
- Factor svn_revert_list into a "core" and a "wrapper"
for more robust handling of errors and subpool.
- The warn_on_nonexistent flag was still there, even
though it wasn't being used. Feh. Now removed.
- I now check for nullity of ctx->cancel_func. At present
the only caller always makes it non-null, but I shouldn't
have been relying on that.
- We now always check for cancellation, even after an
"entry not found" notification. If someone manages to
feed an enormous list of nonexistent paths to "svn revert"
then they'll thank me for this. And it makes the code
a little easier to understand.
- Better commenting.
There are still two public functions (svn_client_revert
and svn_client_revert_list). I'm waiting to see if there are
dissenting opinions before eliminating the former and
renaming the latter.
The function now called revert_1 still uses the "if (err=...) goto out;"
strategy (as it did before I touched the file). It would be possible to
wrap this too, but I'm not sure that doing so really belongs in the
same patch as this one.
--
g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Nov 1 16:33:12 2003