Re: [PATCH] xdiff: improve trimming preprocessing

From: Jun Wu <quark@fb.com>
Date: Mar 6, 2018, 23:05 UTC
Message-ID: <1520370729-sup-3241@x1c>
In-Reply-To: <CAPig+cRYsf161-Xomw7daj8XdEBxZAEQQRdOn4krRTpSAFkPOw@mail.gmail.com>

Excerpts from Eric Sunshine's message of 2018-03-06 14:23:46 -0500:

> On Tue, Mar 6, 2018 at 6:53 AM, Jun Wu <quark@fb.com> wrote:
> > +  printf "x%.0s" {1..934} >>d # pad common suffix to 1024 bytes
> 
> The expression {x..y} is not portable to non-POSIX shells.

Is there a recommended way to generate a repetitive string? Maybe `seq 1000 | sed 's/.*/x/'` ?

Show 5 quoted lines

> > +    fgrep "@@ -1001 +1000,0 @@"
> > +'
> 
> Style: Mix of tabs and spaces for indentation. Please indent only with
> tabs throughout the patch.

Sorry. Will fix in V2.

Show 6 quoted lines

> > +       /* prefix - need line count for xecfg->ptrimmed */
> > +       for (i = 0; ++i < smaller && *ap == *bp;) {
> > +               lines += (*ap == '\n');
> > +               ap++, bp++;
> 
> Is there a good reason for not placing 'ap++, bp++' directly in the 'for'?

"lines += (*ap == '\n');" needs the "ap" before adding. Alternatives are

    for (i = 0; ++i < smaller && *ap == *bp; ) /* 1 */
        lines += (*ap++, *bp++) == '\n';

    for (i = 0; ++i < smaller && *ap == *bp; ap++, bp++) /* 2 */
        lines += (*(ap - 1) == '\n');

Maybe will pick /* 1 */ to make the code shorter.