RE: Question about fixing windows bug reading graft data

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 27, 2009, 17:55 UTC
Message-ID: <alpine.DEB.1.00.0907271948130.6883@intel-tinevez-2-302>
In-Reply-To: <63BEA5E623E09F4D92233FB12A9F7943033B2744@emailmn.mqsoftware.com>

Hi,

On Mon, 27 Jul 2009, Kelly F. Hickel wrote:

> OK, so the 10th copy of the msysGit Herald post has shamed me out of
> hiding!

;-)

Show 32 quoted lines

> > The bug, is that in in commit.c, the code strips '\n', but not '\r', 
> > so the code says the graft data is bad:
> >
> > struct commit_graft *read_graft_line(char *buf, int len) {
> >         /* The format is just "Commit Parent1 Parent2 ...\n" */
> >         int i;
> >         struct commit_graft *graft = NULL;
> > 
> >         if (buf[len-1] == '\n')
> >                 buf[--len] = 0;
> >         if (buf[0] == '#' || buf[0] == '\0')
> >                 return NULL;
> >         if ((len + 1) % 41) {
> >         bad_graft_data:
> >                 error("bad graft data: %s", buf);
> >                 free(graft);
> >                 return NULL;
> >         }
> > 
> > My first plan was to fix it the way that xdiff-interface.c handles it,
> > assuming that was "the Git way" to deal with CRLF:
> >         /* Exclude terminating newline (and cr) from matching */
> >         if (len > 0 && line[len-1] == '\n') {
> >                 if (len > 1 && line[len-2] == '\r')
> >                         len -= 2;
> >                 else
> >                         len--;
> >         }
> > 
> > But I noticed that there seemed to be several checks for '\n' in 
> > commit.c that didn't check for '\r', and wondered if there was a 
> > reason, or if there'd be a better way to handle it.....

I think that you really only have to handle text files read from the file system. That is not the case for commit object parsers: commit _objects_ are required to have LF line endings.

But a few files come to mind which might have CR/LF line endings and need to be interpreted correctly by Git: "grafts", as you pointed out, but also the refs and of course the config.

It would probably be a good idea to have something like

	static inline fix_line_ending(char *line, int len)
	{
		if (len > 0 && line[len-1] == '\n')
			line[len-1 - (len > 1 && line[len-2] == '\r')] = '\0';
	}

in cache.h, and use it in said places.

Of course, the hassle is to find all those places ;-)

Thanks, Dscho