On Fri, 2016-04-29 at 09:38 +0200, Michael Haggerty wrote:
> On 04/27/2016 08:55 PM, Junio C Hamano wrote:
> > Michael Haggerty <mhagger@xxxxxxxxxxxx> writes:
> >
> > > @@ -2380,8 +2381,8 @@ int rename_ref(const char *oldrefname,
> > > const char *newrefname, const char *logms
> > > goto rollback;
> > > }
> > >
> > > - if (!read_ref_full(newrefname, RESOLVE_REF_READING,
> > > sha1, NULL) &&
> > > - delete_ref(newrefname, sha1, REF_NODEREF)) {
> > > + if (!read_ref_full(newrefname, resolve_flags, sha1,
> > > NULL) &&
> > > + delete_ref(newrefname, NULL, REF_NODEREF)) {
> >
> > Could you explain s/sha1/NULL/ here in the proposed log message?
>
> Good question.
>
> Passing sha1 to delete_ref() doesn't add any safety, because the same
> sha1 was just read a moment before, and it is not used for anything
> else. So the check only protects us from a concurrent update to
> newrefname between the call to read_ref_full() and the call to
> delete_ref(). But such a race is indistinguishable from the case that
> a
> modification to newrefname happens just before the call to
> read_ref_full(), which would have the same outcome as the new code.
> So
> the "sha1" check only adds ways for the rename() to fail in
> situations
> where nothing harmful would have happened anyway.
>
> That being said, this is a very unlikely race, and I don't think it
> matters much either way. In any case, the change s/sha1/NULL/ here
> seems
> orthogonal to the rest of the patch.
>
> David, you wrote the original version of this patch. Am I overlooking
> something?
I think I might have been handling some weird case related to symbolic
refs, but I don't recall the details. Your argument seems right to me.
--
To unsubscribe from this list: 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