[darcs-devel] [patch468] add a --no-record option to rollback (and 1 more)

Petr Rockai me at mornfall.net
Sat Jan 15 00:28:56 UTC 2011


Hey,

> -               (rollItBackNow opts repository ps)
> +               if (rollbackInWorkingDir opts)
> +               then (undoItNow opts repository)
> +               else (rollItBackNow opts repository ps)

I haven't noticed anything else wrong with the patch, but the name
undoItNow (a new one) is, IMHO, pretty bogus. I know that rollItBackNow
has been there before, but I don't think it's a good practice to follow
such precedents. It may be better to rename the old one instead.

What about undoByPatch and undoInWorking? Or something else, but I don't
think "undo" v. "rollback" is the right way to express the difference
between recording a patch and changing the working copy (they are more
or less synonymous to me). Moreover, "ItNow" carries 0 information: it's
quite customary that an action will do stuff when it's called, not a
week later.

Yours,
   Petr

PS: It may be worth double-checking the tentativelyAddToPending call. It
may also be worth doing this just a bit cleaner. Surely applying a patch
to working (95 % of undoItNow) is something that should be abstracted
properly and available as a canned API? Other parts of code do something
very similar, don't they?


More information about the darcs-devel mailing list