Re: first contribution: smart rename for the mv command

[Kaz Kylheku]

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