Re: [PATCH-v2/RFC 2/6] xutils: fix hash with whitespace on incomplete line

[Thell Fowler <git@tbfowler.name>]

Junio C Hamano (gitster@pobox.com) wrote on Aug 23, 2009:
> Thell Fowler <git@tbfowler.name> writes:
> 
> >   - Make xdl_hash_record_with_whitespace stop hashing before the
> >     eof when ignoring space change or space at eol on an incomplete
> >     line.
> >
> >   Resolves issue with a final trailing space being included in the
> >   hash on an incomplete line by treating the eof in the same fashion
> >   as a newline.
> 
> Please study the style of existing commit messages and imitate them.
> 

I'll keep trying.

> > Signed-off-by: Thell Fowler <git@tbfowler.name>
> > ---
> >  xdiff/xutils.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> > index 04ad468..c6512a5 100644
> > --- a/xdiff/xutils.c
> > +++ b/xdiff/xutils.c
> > @@ -248,12 +248,12 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,
> >  			if (flags & XDF_IGNORE_WHITESPACE)
> >  				; /* already handled */
> >  			else if (flags & XDF_IGNORE_WHITESPACE_CHANGE
> > -					&& ptr[1] != '\n') {
> > +					&& ptr[1] != '\n' && ptr + 1 < top) {
> >  				ha += (ha << 5);
> >  				ha ^= (unsigned long) ' ';
> >  			}
> >  			else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL
> > -					&& ptr[1] != '\n') {
> > +					&& ptr[1] != '\n' && ptr + 1 < top) {
> >  				while (ptr2 != ptr + 1) {
> >  					ha += (ha << 5);
> >  					ha ^= (unsigned long) *ptr2;
> 
> Thanks.
> 
> The issue you identified and tried to fix is a worthy one.  But before the
> pre-context of this hunk, I notice these lines:
> 
> 		if (isspace(*ptr)) {
> 			const char *ptr2 = ptr;
> 			while (ptr + 1 < top && isspace(ptr[1])
> 					&& ptr[1] != '\n')
> 				ptr++;
> 
> If you have trailing whitespaces on an incomplete line, ptr initially
> points at the first such whitespace, ptr2 points at the same location, and
> then the while() loop advances ptr to point at the last byte on the line,
> which in turn will be the last byte of the file.  And the codepath with
> your updates still try to access ptr[1] that is beyond that last byte.
> 
> I would write it like this patch instead.
> 
> The intent is the same as your patch, but it avoids accessing ptr[1] when
> that is beyond the end of the buffer, and the logic is easier to follow as
> well.
> 

I appreciate your taking the time to look at the issue and explaining the 
reasons for your change.

> -- >8 --
> Subject: xutils: fix hashing an incomplete line with whitespaces at the end
> 
> Upon seeing a whitespace, xdl_hash_record_with_whitespace() first skipped
> the run of whitespaces (excluding LF) that begins there, ensuring that the
> pointer points the last whitespace character in the run, and assumed that
> the next character must be LF at the end of the line.  This does not work
> when hashing an incomplete line, that lacks the LF at the end.
> 
> Introduce "at_eol" variable that is true when either we are at the end of
> line (looking at LF) or at the end of an incomplete line, and use that
> instead throughout the code.
> 
> Noticed by Thell Fowler.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Yeah... comparing this commit message to the original shows a pretty stark 
difference.  I'll get it 'the git way' eventually.

-- 
Thell