Re: [PATCH v4.1 5/5] push: update remote tags only with force

[Chris Rorvick <chris@rorvick.com>]

On Mon, Nov 19, 2012 at 2:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Chris Rorvick <chris@rorvick.com> writes:
>>  The object referenced by <src> is used to update the <dst> reference
>> -on the remote side, but by default this is only allowed if the
>> -update can fast-forward <dst>.  By having the optional leading `+`,
>> -you can tell git to update the <dst> ref even when the update is not a
>> -fast-forward.  This does *not* attempt to merge <src> into <dst>.  See
>> -EXAMPLES below for details.
>> +on the remote side.  By default this is only allowed if the update is
>> +a branch, and then only if it can fast-forward <dst>.  By having the
>
> I can already see the next person asking "I can update refs/notes
> the same way.  The doc says it applies only to the branches.  Is
> this a bug?".

Sure, will fix.

>> diff --git a/cache.h b/cache.h
>> index f410d94..127e504 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1004,13 +1004,14 @@ struct ref {
>>               requires_force:1,
>>               merge:1,
>>               nonfastforward:1,
>> -             is_a_tag:1,
>> +             forwardable:1,
>
> This is somewhat an unfortunate churn.  Perhaps is_a_tag could have
> started its life under its final name in the series?
>
> I am assuming that the struct members will be initialized to 0 (false),
> so everything by default is now not forwardable if somebody forgets
> to set this flag?

Yeah, this was one of a few stupid mistakes.  Previously I used
'forwardable' throughout, but that is awkward except in the last
commit since until then everything is allowed to fast-forward and the
flag is only used to output tag-specific advice.  But inverting the
meaning of the flag is dumb, and I didn't even do it right.

But, as I think you're suggesting, it probably makes more sense to use
a flag that prevents fast-forwarding when set.  So maybe
"not_forwardable", or "is_a_tag" => "not_forwardable" if you think the
renaming is a good idea.  Or maybe just "is_a_tag"?  I think the
rename is good because its meaning is more general in the last commit.

>> diff --git a/remote.c b/remote.c
>> index 44e72d6..a723f7a 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -1311,14 +1311,24 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
>>                *     to overwrite it; you would not know what you are losing
>>                *     otherwise.
>>                *
>> -              * (3) if both new and old are commit-ish, and new is a
>> -              *     descendant of old, it is OK.
>> +              * (3) if new is commit-ish and old is a commit, new is a
>> +              *     descendant of old, and the reference is not of the
>> +              *     refs/tags/ hierarchy it is OK.
>>                *
>>                * (4) regardless of all of the above, removing :B is
>>                *     always allowed.
>>                */
>
> I think this is unreadable.  Isn't this more like
>
>     (1.5) if the destination is inside refs/tags/ and already
>           exists, you are not allowed to overwrite it without
>           --force or +A:B notation.
>
> early in the rule set?

Yes, something like that is much better.

>> -             ref->is_a_tag = !prefixcmp(ref->name, "refs/tags/");
>> +             if (prefixcmp(ref->name, "refs/tags/")) {
>> +                     /* Additionally, disallow fast-forwarding if
>> +                      * the old object is not a commit.  This kicks
>> +                      * out annotated tags that might pass the
>> +                      * is_newer() test but dangle if the reference
>> +                      * is updated.
>> +                      */
>
> Huh?  prefixcmp() excludes refs/tags/ hierarchy. What is an
> annotated tag doing there?  Is this comment outdated???

I think the comment is accurate, although inverting the meaning of the
flag probably doesn't help the clarity of this change.

What do you mean by "there"?  Annotated tags normally are referenced
via something under refs/tags/, but they could be elsewhere, right?
So what I meant to say was: if the reference is not under refs/tags/
then the starting point has to be a commit for it to be forwardable.

I interpreted parse_object(ref->old_sha1) == NULL as the old thing not
existing (rule 1) but I'm pretty sure that was a mistake.  I should
first test it with is_null_sha1, if true then it doesn't exist.
Otherwise parse_object() returning NULL means we just don't have the
object and therefore should follow rule 2.

> Regarding the other arm of this "if (not in refs/tags/ hierarchy)",
> what happens when refs/tags/foo is a reference to a blob or a tree?
> This series declares that the point of tag is not to let people to
> move it willy-nilly, and I think it is in line with its spirit if
> you just rejected non-creation events.

Nothing under refs/tags/ is updateable and only commits are updateable
outside of that due to the new logic.  The ref_newer() call will
reject updating to blobs and trees, although that probably isn't the
right thing to do.  Previously I was ensuring both old and new objects
were commits if the ref was not under refs/tags/, but it occurred to
me that fast-forwarding from a commit to a tag might be a reasonable
thing to want to do.  But a rejected update to a blob should get the
already-exists advice, not a message suggesting a merge (which is what
you get by relying on ref_newer() to fail.)

> Also, I suspect that you do not even need to have old object locally
> when looking at refs/tags/ hierarchy.  "Do not overwrite tags" can
> be enforced by only noticing that they already have something; you
> do not know what that something actually is to prevent yourself from
> overwriting it.  You may have to update the rule (2) in remote.c
> around l.1311 for this.

Isn't it already doing this?  I think I've either confounded things
with my mistakes or I'm missing this point.

>> +test_expect_success 'push requires --force to update lightweight tag' '
>> +     mk_test heads/master &&
>> +     mk_child child1 &&
>> +     mk_child child2 &&
>> +     (
>> +             cd child1 &&
>> +             git tag Tag &&
>> +             git push ../child2 Tag &&
>
> Don't you want to make sure that another "git push ../child2 Tag" at
> this point, i.e. attempt to update Tag with the same, should succeed
> without --force?

Yes, that is clearly a good addition.

Thanks for all the feedback.  Sorry I mucked up this series a bit,
tried to get an update out too quickly I guess.

Chris