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