Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

[Michael Haggerty <mhagger@alum.mit.edu>]

On 08/04/2016 12:29 AM, Jacob Keller wrote:
> On Wed, Aug 3, 2016 at 3:00 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> +/*
>> + * If a line is indented more than this, get_indent() just returns this value.
>> + * This avoids having to do absurd amounts of work for data that are not
>> + * human-readable text, and also ensures that the output of get_indent fits within
>> + * an int.
>> + */
>> +#define MAX_INDENT 200
>> +
>> +/*
>> + * Return the amount of indentation of the specified line, treating TAB as 8
>> + * columns. Return -1 if line is empty or contains only whitespace. Clamp the
>> + * output value at MAX_INDENT.
>> + */
>> +static int get_indent(xrecord_t *rec)
>> +{
>> +       long i;
>> +       int ret = 0;
>> +
>> +       for (i = 0; i < rec->size; i++) {
>> +               char c = rec->ptr[i];
>> +
>> +               if (!XDL_ISSPACE(c))
>> +                       return ret;
>> +               else if (c == ' ')
>> +                       ret += 1;
>> +               else if (c == '\t')
>> +                       ret += 8 - ret % 8;
>> +               /* ignore other whitespace characters */
>> +
>> +               if (ret >= MAX_INDENT)
>> +                       return MAX_INDENT;
> 
> Should we return -1 here?
> 
>> +       }
>> +       /*
>> +        * We have reached the end of the line without finding any non-space
>> +        * characters; i.e., the whole line consists of trailing spaces, which we
>> +        * are not interested in.
>> +        */
>> +       return -1;
> 
> It seems odd to be that a line with "199" spaces and nothing else will
> return "-1" but a line with 200 spaces and nothing else will return
> 200..? Would it be safe to just return -1 in both cases (if a line is
> all spaces or starts with more than 200 spaces just return -1)?
> 
>> +}
>> +

Thanks for your feedback.

I was implicitly assuming that such lines would have text somewhere
after those 200 spaces (or 25 TABs or whatever). But you're right, the
line could consist only of whitespace. Unfortunately, the only way to
distinguish these two cases is to read the rest of the line, which is
exactly what we *don't* want to do.

But I think it doesn't matter anyway. Such "text" will likely never be
read by a human, so it's not a big deal if the slider position is not
picked perfectly. And remember, this whole saga is just to improve the
aesthetics of the diff. The diff is *correct* (e.g., in the sense of
applicable) regardless of where we position the sliders.

Michael