Question about fixing windows bug reading graft data

Question about fixing windows bug reading graft data

[Kelly F. Hickel]

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 

RE: Question about fixing windows bug reading graft data

[Kelly F. Hickel]

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 


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

RE: Question about fixing windows bug reading graft data

[Johannes Schindelin]

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!

;-)

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

RE: Question about fixing windows bug reading graft data

[Johannes Schindelin]

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