Re: [PATCH 1/5] Add platform-independent .git "symlink"

["Lars Hjemli" <hjemli@gmail.com>]

On Feb 18, 2008 6:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Lars Hjemli <hjemli@gmail.com> writes:
>
> > This patch allows .git to be a regular textfile containing the path of
> > the real git directory (formatted like "gitdir: <path>\n"), which is
> > useful on platforms lacking support for real symlinks.
>
> I think a sane simplification is to allow the file to have
> any number of optional \r or \n at the end.

Agreed.

> > +     len = read_in_full(fd, buf, sizeof(buf));
> > +     close(fd);
> > +     if (len != st.st_size)
> > +             return NULL;
> > +     if (!len || buf[len - 1] != '\n')
> > +             return NULL;
> > +     buf[len - 1] = '\0';
> > +     if (prefixcmp(buf, "gitdir: "))
> > +             return NULL;
>
> But I am not sure about this part.  We found what claims to be
> the ".git" fake symlink but it is ill-formed.  Don't we want to
> diagnose the possible breakage for the user?

Yes, I think I got to eager in my 'gentleness'. It's probably better
to die() with an appropriate errormessage.

>
> > +/*
> > +     if (!is_git_directory(buf + 8))
> > +             return NULL;
> > +*/
>
> Likewise.

True, I'll uncomment and die().

Thanks for the review.

--
larsh