[Toybox] [PATCH] Fix mv on overwrite.

Wed Sep 9 19:11:30 PDT 2015

On Wed, Sep 9, 2015 at 2:18 AM, Rob Landley <rob at landley.net> wrote:
> 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 a fine goal, but the whole reason facebook were poking around
this particular corner is because they were trying to find something
that worked on original flavor toolbox mv, BSD mv (which is what we'd
been shipping for a while), and toybox mv. so you don't just have to
reach a good point, and we don't just need to have shipped it --- we
need to wait until N or O or whichever release it is is so old that no
one cares about anything younger. so even if you're done tomorrow,
"use your own known binary" is going to be the best answer for these
kinds of folks.

but, yeah, one day toybox will be good enough and N will be
sufficiently far in the past that everyone's happy enough.

the only thing GNU has given me lately is to make grep more annoying
by turning "Is a directory" warnings on by default. other than that,
afaik no GNU tool has changed at all in the last decade. and i'd be
better off if i was still running an older grep.

> 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.

yeah, i don't think i meant the GNU ones. i think i was pointing out
the choice between consistency and behave like the GNU tools.

personally i think without a strong argument to hide these prompts, it
makes more sense to show them. (though of course if stdin has been
closed, the chances are probably slim that stdout and stderr are going
anywhere useful. but at least we tried.)

> 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.

me too. that's why the coreutils rm behavior was surprising and seems
like a bug.

>> 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

-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.

 1441851090.0