threads / bug / 19746

Question about fixing windows bug reading graft data

Subject: Question about fixing windows bug reading graft data

## tl;dr

4 messages between Jun 8, 2009 and Jul 27, 2009.

replies: 3people: 2as markdown or json

Kelly F. Hickel· Jun 8, 2009, 17:19 UTC · lore

Hi All,
	Ran into a bug trying to use grafts on windows with cygwin git
version 1.6.1.2.  I've verified that the bug is still there in the
latest source, and was going to submit a patch, but then I noticed that
there seem to be more occurrences in commit.c, and wondered if there was
a better way to fix it than what I had first come up with.

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

--
 
Kelly F. Hickel
Senior Product Architect
MQSoftware, Inc.
952-345-8677 Office
952-345-8721 Fax
kfh@mqsoftware.com
www.mqsoftware.com
Certified IBM SOA Specialty
Your Full Service Provider for IBM WebSphere Learn more at
www.mqsoftware.com

Kelly F. Hickel· Jul 27, 2009, 17:33 UTC · re: Kelly F. Hickel · lore

RE: Question about fixing windows bug reading graft data

OK, so the 10th copy of the msysGit Herald post has shamed me out of hiding! I posted the below awhile back, and since I volunteered to fix something (if given a few pointers), I felt I had "Done My Duty" to the Git world!

But now Dscho has made me rip the blinders from my eyes, to try once again to offer to fix this bug (even though I found it in Cygwin Git and don't use msysgit, but hey, Git is Git, right!?!?!)....

So, here I am, gonna put myself out there, willing to suffer ridicule, etc!

Any guidance on "the Git way" to properly deal with \r in a meta file???? Show me the light!

--

Kelly F. Hickel Senior Product Architect MQSoftware, Inc. 952-345-8677 Office 952-345-8721 Fax kfh@mqsoftware.com www.mqsoftware.com Certified IBM SOA Specialty Your Full Service Provider for IBM WebSphere Learn more at www.mqsoftware.com

Show 11 quoted lines

> -----Original Message-----
> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On
> Behalf Of Kelly F. Hickel
> Sent: Monday, June 08, 2009 12:20 PM
> To: git@vger.kernel.org
> Subject: Question about fixing windows bug reading graft data
> 
> Hi All,
> 	Ran into a bug trying to use grafts on windows with cygwin git
> version 1.6.1.2.  I've verified that the bug is still there in the
> latest source, and was going to submit a patch, but then I noticed

that

Show 5 quoted lines

> there seem to be more occurrences in commit.c, and wondered if there
> was
> a better way to fix it than what I had first come up with.
> 
> The bug, is that in in commit.c, the code strips '\n', but not '\r',

so

Show 51 quoted lines

> 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.....
> 
> 
> 
> --
> 
> Kelly F. Hickel
> Senior Product Architect
> MQSoftware, Inc.
> 952-345-8677 Office
> 952-345-8721 Fax
> kfh@mqsoftware.com
> www.mqsoftware.com
> Certified IBM SOA Specialty
> Your Full Service Provider for IBM WebSphere Learn more at
> www.mqsoftware.com
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Johannes Schindelin· Jul 27, 2009, 17:55 UTC · re: Kelly F. Hickel · lore

RE: Question about fixing windows bug reading graft data

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

Johannes Schindelin· Jul 27, 2009, 19:31 UTC · re: Johannes Schindelin · lore

RE: Question about fixing windows bug reading graft data

Hi,

On Mon, 27 Jul 2009, Johannes Schindelin wrote:

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

Forgot at least one: objects/info/alternates.

And this one was a very real issue on my side, when I installed an alternate on somebody's Windows computer, and it did not work. Should have used vi to begin with... :-)

Ciao, Dscho