On 2022-12-22 07:59, Stéphane Archer wrote:
> Dear Gnu Coreutils community,
>
> I would like to contribute to the project a feature I miss a lot in the
> `mv` command.
> I currently have a working patch but it's clearly not perfect. I would like
> to have feedback on it and would like to know if you are willing to accept
> this change.
> Do you think it's possible?
I have only technical feedback about code.
1. access function: you probably don't want to use this.
The purpose of the access function is for setuid programs to determine
whether the real user ID would have a certain access, without having
to drop privileges to that ID.
This is almost certainly not what you want in a utility like this;
mv just should wield whatever effective user ID powers it has, and
not hold back.
Even if the operation is F_OK for simple existence, the function is not
appropriate, because POSIX says:
The checks for accessibility (including directory permissions
checked during pathname resolution) shall be performed using
the real user ID in place of the effective user ID and the real
group ID in place of the effective group ID.
So if you do access("/home/bob/private/xyzzy.txt", F_OK), as EUID = root,
but RUID = alice, access will pretend that it's running as alice and
not allow the existence check through bob's private directory.
2. Don't invent new functions from scratch.
find_dot(str) -> strcspn(str, ".") // mostly
This strcspn will return the length of the prefix of str
before the first dot. If there is not dot, it returns strlen(str).
3. string functions like strlen return size_t, not unsigned int;
don't mix types. It's not a huge deal because you probably
won't get a 64 bit size_t value that is truncated when converted
to a 32 bit unsigned int. It's untidy though.
4. I see a malloc call, but most GNU programs don't use malloc directly
and the Coreutils are no exception: they use xmalloc, xzalloc
and such, which are wrappers. Always look around in the code base
for examples of the kind of thing you're trying to do, to see what
functions it's using; reuse its idioms so your patch blends in.
5. I think your create_smart_name function can be condensed with some
creative use of the snprintf function. Possibly one call to that,
with some conditional in the argument list, could do the whole job
of combining the pieces.
6. Re:
+ unsigned int i = 0;
+ char is[100]; // What len should I put?
+ sprintf(is, "%d", i);
Because i is unsigned int, use "%u" and not "%d",
which will interpret it as int, possibly negative.
Your loop should test for i hitting UINT_MAX;
you don't want to increment from there to zero.
One fine day someone will have that many files;
and filesystems can be virtual (e.g. we could use
FUSE in some way to make a test case for this without
making billions of files.)
Since the index is unsigned int with a UINT_MAX
maximum value, we can conservatively estimate
the number of digits needed by dividing
(UINT_MAX * CHAR_BIT) / 3. (The idea being every 3
binary digits encodes one decimal digit's worth of
entropy). Then add 1 for the null terminator.
For 32 bits we get 32 / 3 + 1 = 11. (exact fit for UINT_MAX)
For 63 bits we get 64 / 3 + 1 = 22. (one byte wasted)
7. Pay attention to coding style. GNU uses funny indentation for braces:
if (smart_rename)
{ // + 2
dest = // + 2 again
}
Do not even think about doing this outside of GNU, though. :)
