Junio C Hamano <junkio@xxxxxxx> wrote:
> Shawn Pearce <spearce@xxxxxxxxxxx> writes:
>
> > diff --git a/refs.c b/refs.c
> > index 31cf276..d3ddc82 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -142,6 +142,8 @@ static int do_for_each_ref(const char *b
> > namelen = strlen(de->d_name);
> > if (namelen > 255)
> > continue;
> > + if (namelen>5 && !strcmp(de->d_name+namelen-5,".lock"))
> > + continue;
> > memcpy(path + baselen, de->d_name, namelen+1);
> > if (stat(git_path("%s", path), &st) < 0)
> > continue;
>
> Now this got me worried. Until now I did not realize that we
> are clobbering refnames that ends with ".lock" if another ref
> with the name without ".locK" is updated. Because we do not
> forbid a name that ends with ".lock" to be used as a refname,
> this is an accident waiting to happen.
>
> Not your fault, though. It was like this ever since the initial
> version of refs.c was accepted by Linus.
>
> We could do one of two things: officially forbid any refname
> that ends with ".lock", or fix the lockfile naming convention.
>
> Nobody should be relying on what the actual lockfile-to-be-
> renamed-to-become-the-real-file is called. I suspect we would
> want to fix refs.c::ref_lock_file_name() to use a name that
> would never be used as a refname.
>
> We could make it begin with ".", so the lock file for the master
> ".git/refs/heads/master" would become ".git/refs/heads/.master",
> for example. That way, we cannot clobber a valid unrelated ref
> (".master" is not a valid ref name), and as an added bonus, you
> do not even have to have the above hunk.
Not a bad idea. I found this bug because fetch.c was trying to
work on the bob.lock while it was fetching "heads/bob". Which
didn't make sense to me, so I put this skip thing in...
I'd want to get all of the "lockers" move to using the common
lock code before changing the lock name to ".master" (for example)
but yes that's a good idea.
--
Shawn.
-
: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html