Re: first contribution: smart rename for the mv command

[Stéphane Archer]

Thanks a lot for your feedback ☺️
I will try my best to improve my patch
I have to admit that understanding the code base and the idioms is
currently difficult for me. I imagine it comes with practice. Is there any
resources or books that have helped you in this area or is it just pure
practice?

On Sat, Jan 14, 2023, 04:57 Kaz Kylheku <962-396-1872@kylheku.com> wrote:

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