[Toybox] [PATCH] Fix mv on overwrite.

[Rob Landley]

On 08/31/2015 02:16 PM, enh wrote:
> On Sun, Aug 30, 2015 at 4:39 AM, Rob Landley <rob at landley.net> wrote:
> at some point every release cycle, AOSP master no longer gets merged
> into the release branch. it's still possible to cherrypick, but that
> gets increasingly difficult as time passes. those of us on the lower
> levels try to stabilize early. (this is also why i'm keen to get stuff
> like uptime and lsof switched over now --- not because they have any
> hope of being in M, but to maximize the testing they can get in master
> before N ever opens for business.)
> 
> folks relying on toybox for automated testing should be shipping their
> own known binary rather than relying on whatever's on the system, and
> one day i'd like to get that into the NDK so it's easy for random
> developers too.

I hope that after the toybox 1.0 release the android base version is
generally usable for everybody.

It's... still a ways off. (Although I'm strongly tempted to start the
"vi" implementation since I know how to do it now, but... busy recanning
worms at present.)

>>> Fix mv on overwrite.
>>>
>>> We need to remove the destination, not the source, to be able to overwrite.
>>>
>>> diff --git a/tests/mv.test b/tests/mv.test
>>> index 53fc999..030e9cc 100755
>>> --- a/tests/mv.test
>>> +++ b/tests/mv.test
>>> @@ -96,3 +96,10 @@ touch file1 file2
>>>  testing "Move -n file new_file (exist)" "mv -n file1 file2 &&
>>>     [ -e file1 -a -e file2 ] && echo 'yes'" "yes\n" "" ""
>>>  rm -f file*
>>> +
>>> +touch file1 file2
>>> +chmod 400 file1 file2
>>> +testing "Move file over unwritable file with no stdin" \
>>> +   "</dev/null mv file2 file1 && [ -e file -a ! -e file2 ] && echo 'yes'" \
>>> +   "yes\n" "" ""
>>> +rm -f file*
>>> diff --git a/toys/posix/cp.c b/toys/posix/cp.c
>>> index d5e92f2..5a55f40 100644
>>> --- a/toys/posix/cp.c
>>> +++ b/toys/posix/cp.c
>>> @@ -382,7 +382,7 @@ void cp_main(void)
>>>          {
>>>            fprintf(stderr, "%s: overwrite '%s'", toys.which->name, TT.destname);
>>>            if (!yesno("", 1)) rc = 0;
>>> -          else unlink(src);
>>> +          else unlink(TT.destname);
>>>          }
>>>        }
>>
>> Applied.
>>
>> I need to look at the second one more closely after dinner.
>>
>> (I'm currently implementing the darn aboriginal linux toybox-test build
>> control image so I can run tests as root in a known environment, and
>> then I need to tackle the horrible state of the testsuite which is
>> failing all sorts of stuff and needs fixing. Then I need to finish the
>> nommu support because it's got my tree screwed up and not building
>> easily until that works through. And THEN I need to finish the cp
>> --preserve xargs support. And then I need to flush the other half-dozen
>> pending commits out of my tree...)

Oh hey, I got distracted from that. I should get back to it and finish it...

Ok, wrong email for the if (!isatty(0)) rationale, where was... Ah:

> Fix mv on overwrite and its prompt.
>
> We need to remove the destination, not the source, to be able to
> overwrite.
>
> Also, coreutils mv doesn't prompt if it's not talking to a tty.

But _why_?

> This
> change also affects killall, crontab, find, and rm.

I tested rm, killall, and find and it didn't for me. They all prompted
and tried to read from stdin when < /dev/null.

The few times I've edited crontab I did so with vi because actual
multiuser systems are few and far between these days, although I suppose
the cloud might be bringing it back a little...

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/crontab.html

Does not mention -i option.

  $ crontab -ir
  no crontab for landley

Hmmm. Typing crontab by itself didn't give me my prompt back... Let's
see what the man page says:

 The  -l  option  causes the current crontab to be displayed on standard
 output. See the note under DEBIAN SPECIFIC below.

No. Nope. Not going there. So very much not going DEBIAN SPECIFIC...

Sigh. What does busybox crontab --help say... no -i support. Of course not.

> The rm case at
> least is interesting --- coreutils silently *does* do the removal,
> whereas this patch would make toybox silently *not* do the removal.
> This despite the fact that coreutils rm on a tty defaults to 'n'. So
> do we want two different defaults for yesno? Or is this a coreutils
> bug? (It certainly seems surprising to me.)

The default is an argument to yesno, it's context sensitive. That said,
what the right behavior is: open question. I lean towards not damaging
things when something goes wrong.

> diff --git a/lib/lib.c b/lib/lib.c
> index c16cffe..85ea4e1 100644
> --- a/lib/lib.c
> +++ b/lib/lib.c
> @@ -634,6 +634,8 @@ int yesno(char *prompt, int def)
>  {
>    char buf;
>
> +  if (!isatty(0)) return def;
> +

The change I made instead yanked the prompt behavior from yesno() and
had the caller do it, meaning the !isatty(0) stuff would be the caller's
responsibility as well. (And there are times "yes | thingy-that-prompts"
is expected to fill in the y's, that's why "yes" is called that.)

Rob

 1441790294.0