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.
> 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.
> I'll push in your name if you're ok with the above changes.
It's really in Nishan's name, but OK :)
