Re: [PATCH v3 1/4] word diff: comments, preparations for regex customization

[Junio C Hamano <gitster@pobox.com>]

Thomas Rast <trast@student.ethz.ch> writes:

> +/* unused right now */
> +enum diff_word_boundaries {
> +	DIFF_WORD_UNDEF,
> +	DIFF_WORD_BODY,
> +	DIFF_WORD_END,
> +	DIFF_WORD_SPACE,
> +	DIFF_WORD_SKIP
> +};

Don't do this.  Please introduce them when you start using them.

>  struct diff_words_buffer {
>  	mmfile_t text;
>  	long alloc;
>  	long current; /* output pointer */
>  	int suppressed_newline;
> +	enum diff_word_boundaries *boundaries;
>  };

Likewise.  Especially because this raises eyebrows "Huh, a pointer to an
enum, or perhaps he allocates an array of enums?" without allowing the
reader to figure it out much later when the field is actually used.

>  static void diff_words_append(char *line, unsigned long len,
> @@ -339,16 +349,23 @@ static void diff_words_append(char *line, unsigned long len,
>  struct diff_words_data {
>  	struct diff_words_buffer minus, plus;
>  	FILE *file;
> +	regex_t *word_regex; /* currently unused */
>  };

I see having this here and keeping it NULL in this patch makes the later
patch to diff_words_show() more readable, so this probably should stay
here.

> -static void print_word(FILE *file, struct diff_words_buffer *buffer, int len, int color,
> +/*
> + * Print 'len' characters from the "real" diff data in 'buffer'.  Also
> + * returns how many characters were printed (currently always 'len').
> + * With 'suppress_newline', we remember a final newline instead of
> + * printing it.
> + */

"... Even in such a case, 'len' that is returned counts the suppressed
newline", or something like that?  If you can concisely describe why the
caller wants the returned count not match the number of actually printed
chars (i.e. it includes the suppressed newline), it would help the reader
understand the logic.  I am _guessing_ it is because this is called to
print matching words from the preimage buffer, and the return value is
used to skip over the same part in the postimage buffer, and by definition
they have to be of the same length (otherwise they won't be matching).

> +static int print_word(FILE *file, struct diff_words_buffer *buffer, int len, int color,
>  		int suppress_newline)
>  {
>  	const char *ptr;
>  	int eol = 0;
>  
>  	if (len == 0)
> -		return;
> +		return len;
>  
>  	ptr  = buffer->text.ptr + buffer->current;
>  	buffer->current += len;
> @@ -368,18 +385,30 @@ static void print_word(FILE *file, struct diff_words_buffer *buffer, int len, in
>  		else
>  			putc('\n', file);
>  	}
> +
> +	/* we need to return how many chars to skip on the other side,
> +	 * so account for the (held off) \n */

Multi-line comment style?  I won't repeat this but you have many...

> +	return len+eol;
>  }
>  
> +/*
> + * Callback for word diff output
> + */

Without saying "to do what", the comment adds more noise than signal.
"Called to parse diff between pre- and post- image files converted into
one-word-per-line format and concatenate them to into lines by dropping
some of the end-of-lines but keeping some others", or something like that?

Thanks.