Annotating patches

[Denys Vlasenko]

On Thu, Nov 13, 2008 at 8:40 AM, Michael Abbott <michael at araneidae.co.uk> wrote:
> I notice that the downloads/fixes-$VERSION/ directories quickly accumulate
> interesting fixes, and I'm watching them fly by on this list.  Could I
> make a plea, please, that the patches include at least some context in
> their body?

This adds a bit more work.

> It seems that patch is quite unfussy about what precedes the body of the
> patch, so I think it might be really helpful if the patches included a
> description of what they're addressing -- presumably this could mostly be
> taken directly from the e-mail where the patch was submitted.
>
> Looking at the two 1.12.2 patches:
>
> busybox-1.12.2-lineedit.patch:
>        I don't quite see what this is doing.  Presumably MAX_HISTORY
> won't be negative, so you're eliminating a statement of the form
>        if (value & 0) do_something;
> I thought we could trust the compiler to optimise this sort of thing away.

do_something in this case contains code which would not *compile*.

> Or is the mistake that the original writer was assuming this...?
>
> busybox-1.12.2-getopt.patch:
>        Well, the comment attempts to explain, but it's a little opaque to
> me.  What's the practical impact of this patch, what visible behaviour
> does it address?

libbb/vfork_daemon_rexec.c:

        /* In case getopt() or getopt32() was already called:
         * reset the libc getopt() function, which keeps internal state.
         *
         * BSD-derived getopt() functions require that optind be set to 1 in
         * order to reset getopt() state.  This used to be generally accepted
         * way of resetting getopt().  However, glibc's getopt()
         * has additional getopt() state beyond optind, and requires that
         * optind be set to zero to reset its state.  So the
unfortunate state of
         * affairs is that BSD-derived versions of getopt() misbehave if
         * optind is set to 0 in order to reset getopt(), and glibc's getopt()
         * will core dump if optind is set 1 in order to reset getopt().
         *
         * More modern versions of BSD require that optreset be set to 1 in
         * order to reset getopt().  Sigh.  Standards, anyone?
         */
#ifdef __GLIBC__
        optind = 0;
#else /* BSD style */
        optind = 1;
        /* optreset = 1; */
#endif
        /* optarg = NULL; opterr = 1; optopt = 63; - do we need this too? */
        /* (values above are what they initialized to in glibc and uclibc) */
        /* option_mask32 = 0; - not needed, no applet depends on it being 0 */

--
vda