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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 11, 2009, 22:19 UTC
Message-ID: <7vljthpjsi.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <4aea85caafd38a058145c5769fe8a30ffdbd4d13.1231669012.git.trast@student.ethz.ch>

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

Show 8 quoted lines

> +/* 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.

Show 7 quoted lines

>  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.

Show 7 quoted lines

>  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.

Show 7 quoted lines

> -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).

Show 19 quoted lines

> +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...

Show 6 quoted lines

> +	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.