[PATCH] Add a new option '-k --keepdirs' to the 'remove' command, which, if specified on command line, does not delete empty directories recursively.

Peter Arrenbrecht peter.arrenbrecht at gmail.com
Fri Aug 28 08:27:08 CDT 2009


On Fri, Aug 28, 2009 at 3:04 PM, Greg Ward<greg-hg at gerg.ca> wrote:
>> # HG changeset patch
>> # User Jonny Dee <jonny.dee at gmx.net>
>> # Date 1251414081 -7200
>> # Node ID be9b3b933595ba0c88f01cdb9f2ccb3cc32b690d
>> # Parent  7345fa5e572e029362bc6f2f96af452607ebb1ff
>> Add a new option '-k --keepdirs' to the 'remove' command, which, if specified on command line, does not delete empty directories recursively.
>
> Please aim for a concise one-sentence description that fits in ~70
> chars with context as a prefix.  More detail, if needed, on subsequent
> lines.  E.g.:
>
>  remove: add -k/--keep-dirs option to disable removing empty directories.
>
> Writing checkin messages is an art; writing one-line checkin messages
> for Mercurial is a particularly demanding art.  They are picky around
> here.  ;-)
>
>> @@ -3456,6 +3457,8 @@
>>           [('A', 'after', None, _('record delete for missing files')),
>>            ('f', 'force', None,
>>             _('remove (and delete) file even if added or modified')),
>> +          ('k', 'keepdirs', None,
>> +           _('do not recursively delete empty directories')),
>>           ] + walkopts,
>>           _('[OPTION]... FILE...')),
>>      "rename|mv":
>
> The usual convention for multi-word options is to use a hyphen, so
> that should be 'keep-dirs'.  E.g. 'log --follow-first', 'commit
> --close-branch'.
>
> Also, it's debatable whether this option deserves a one-letter short
> form, since it's a fairly arcane use case.

I'd drop the short form, too.
-parren

>> @@ -445,14 +445,15 @@
>>          os.unlink(temp)
>>          os.rename(src, dst)
>>
>> -def unlink(f):
>> +def unlink(f, keepdirs=None):
>>      """unlink and remove the directory if it is empty"""
>>      os.unlink(f)
>>      # try removing directories that might now be empty
>> -    try:
>> -        os.removedirs(os.path.dirname(f))
>> -    except OSError:
>> -        pass
>> +    if not keepdirs:
>> +      try:
>> +       os.removedirs(os.path.dirname(f))
>> +      except OSError:
>> +       pass
>
> Please update the docstring to describe 'keepdirs'.
>
> Four-space indentation, please.
>
> Other than those details, it's looking pretty good!
>
> Greg
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>



More information about the Mercurial-devel mailing list