GNU bug report logs - #44889
[PATCH] rm: do not skip extra files when removal of empty directories fails

Previous Next

Full log

Message #22 received at 44889-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Nick Alcock <nick.alcock <at> oracle.com>
Cc: elena.zannoni <at> oracle.com, nishant.nayan <at> oracle.com, bert.barbe <at> oracle.com,
 44889-done <at> debbugs.gnu.org, jemarch <at> gnu.org
Subject: Re: bug#44889: [PATCH] rm: do not skip extra files when removal of
 empty directories fails
Date: Thu, 26 Nov 2020 19:39:54 +0000

On 26/11/2020 18:49, Nick Alcock wrote:
> On 26 Nov 2020, Pádraig Brady verbalised:
> 
>> On 26/11/2020 14:35, Nick Alcock wrote:
>>> diff --git a/src/remove.c b/src/remove.c
>>> index 2d40c55cd..1150cf179 100644
>>> --- a/src/remove.c
>>> +++ b/src/remove.c
>>> @@ -508,8 +508,7 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const *x)
>>>                s = excise (fts, ent, x, true);
>>>                fts_skip_tree (fts, ent);
>>>              }
>>> -
>>> -        if (s != RM_OK)
>>> +        else if (s != RM_OK)
>>>              {
>>>                mark_ancestor_dirs (ent);
>>>                fts_skip_tree (fts, ent);
>>
>> I think we'd still like to mark ancestors when failing to remove,
>> so that we don't prompt unnecessarily.
> 
> If this doesn't make the test fail, I'm fine with it.

Cool, the test still passes :)

>> I think it would be better to do:
>>
>> diff --git a/src/remove.c b/src/remove.c
>> index 2d40c55cd..adf948935 100644
>> --- a/src/remove.c
>> +++ b/src/remove.c
>> @@ -506,7 +506,8 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const *x)
>>               /* When we know (from prompt when in interactive mode)
>>                  that this is an empty directory, don't prompt twice.  */
>>               s = excise (fts, ent, x, true);
>> -            fts_skip_tree (fts, ent);
>> +            if (s == RM_OK)
>> +              fts_skip_tree (fts, ent);
>>             }
> 
> I don't really understand what mark_ancestor_dirs is doing, but as long
> as it's not making the file disappear and the new test still passes I'm
> fine with this (though honestly the s= stuff is incredibly confusing and
> really should be using two distinct variables for result-of-prompt and
> result-of-excision to make it obvious what the flaming dingbats is going
> on).
> 
> Another worry of mine is that I don't understand why fts_skip_tree is
> skipping an entry *other* than ent the second time it's called. Naively
> I'd have assumed fts_skip_tree (fts, ent) would be idempotent: calling
> it repeatedly with the same ent should do the same as calling it only
> once. But clearly this is not the case, so I'm misreading the code in
> gnulib and/or glibc somehow.

There is an fts_read() in fts_skip_tree() that's consuming the entry.

Pushed at:
https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=6bf108358

cheers,
Pádraig

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.