Jeff King [off-list ref] writes:
I.e., we trigger the "!o" branch after the parse_object in your example.
Heh, I didn't see this message until now (gmane seems to be lagging
a bit).
I am very tempted to do this.
* Remove unnecessary not_forwardable from "struct ref"; it is only
used inside set_ref_status_for_push();
* "refs/tags/" is the only hierarchy that cannot be replaced
without --force;
* Remove the misguided attempt to force that everything that
updates an existing ref has to be a commit outside "refs/tags/"
hierarchy. This code does not know what kind of objects the user
wants to place in "refs/frotz/" hierarchy it knows nothing about.
I feel moderately strongly about the last point. Defining special
semantics for one hierarchy (e.g. "refs/tags/") and implementing a
policy for enforcement is one thing, but a random policy that
depends on object type that applies globally is simply insane. The
user may want to do "refs/tested/" hierarchy that is meant to hold
references to commit, with one annotated tag "refs/tested/latest"
that points at the "latest tested version" with some commentary, and
maintain the latter by keep pushing to it. If that is the semantics
the user wanted to ahve in the "refs/tested/" hierarchy, it is not
reasonable to require --force for such a workflow. The user knows
better than Git in such a case.
cache.h | 1 -
remote.c | 24 +-----------------------
t/t5516-fetch-push.sh | 21 ---------------------
3 files changed, 1 insertion(+), 45 deletions(-)
diff --git a/cache.h b/cache.h
index a32a0ea..a942bbd 100644
--- a/cache.h
+++ b/cache.h
@@ -1004,7 +1004,6 @@ struct ref {
requires_force:1,
merge:1,
nonfastforward:1,
- not_forwardable:1,
update:1,
deletion:1;
enum {diff --git a/remote.c b/remote.c
index aa6b719..2c747c4 100644
--- a/remote.c
+++ b/remote.c
@@ -1279,26 +1279,6 @@ int match_push_refs(struct ref *src, struct ref **dst,
return 0;
}
-static inline int is_forwardable(struct ref* ref)
-{
- struct object *o;
-
- if (!prefixcmp(ref->name, "refs/tags/"))
- return 0;
-
- /* old object must be a commit */
- o = parse_object(ref->old_sha1);
- if (!o || o->type != OBJ_COMMIT)
- return 0;
-
- /* new object must be commit-ish */
- o = deref_tag(parse_object(ref->new_sha1), NULL, 0);
- if (!o || o->type != OBJ_COMMIT)
- return 0;
-
- return 1;
-}
-
void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
int force_update)
{@@ -1344,8 +1324,6 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
* passing the --force argument
*/
- ref->not_forwardable = !is_forwardable(ref);
-
ref->update =
!ref->deletion &&
!is_null_sha1(ref->old_sha1);
@@ -1355,7 +1333,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
!has_sha1_file(ref->old_sha1)
|| !ref_newer(ref->new_sha1, ref->old_sha1);
- if (ref->not_forwardable) {
+ if (!prefixcmp(ref->name, "refs/tags/")) {
ref->requires_force = 1;
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 6009372..8f024a0 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -950,27 +950,6 @@ test_expect_success 'push requires --force to update lightweight tag' '
)
'
-test_expect_success 'push requires --force to update annotated tag' '
- mk_test heads/master &&
- mk_child child1 &&
- mk_child child2 &&
- (
- cd child1 &&
- git tag -a -m "message 1" Tag &&
- git push ../child2 Tag:refs/tmp/Tag &&
- git push ../child2 Tag:refs/tmp/Tag &&
- >file1 &&
- git add file1 &&
- git commit -m "file1" &&
- git tag -f -a -m "message 2" Tag &&
- test_must_fail git push ../child2 Tag:refs/tmp/Tag &&
- git push --force ../child2 Tag:refs/tmp/Tag &&
- git tag -f -a -m "message 3" Tag HEAD~ &&
- test_must_fail git push ../child2 Tag:refs/tmp/Tag &&
- git push --force ../child2 Tag:refs/tmp/Tag
- )
-'
-
test_expect_success 'push --porcelain' '
mk_empty &&
echo >.git/foo "To testrepo" &&
On Wed, Jan 16, 2013 at 09:10:10AM -0800, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
I.e., we trigger the "!o" branch after the parse_object in your example.
Heh, I didn't see this message until now (gmane seems to be lagging
a bit).
I think it is vger lagging, actually.
I am very tempted to do this.
* Remove unnecessary not_forwardable from "struct ref"; it is only
used inside set_ref_status_for_push();
* "refs/tags/" is the only hierarchy that cannot be replaced
without --force;
Agreed.
* Remove the misguided attempt to force that everything that
updates an existing ref has to be a commit outside "refs/tags/"
hierarchy. This code does not know what kind of objects the user
wants to place in "refs/frotz/" hierarchy it knows nothing about.
I agree with what your patch does, but my thinking is a bit different.
My original suggestion with respect to object types was that the rule
for --force should be "do not ever lose any objects without --force". So
a fast-forward is OK, as the new objects reference the old. A non-fast
forward is not, because objects become unreferenced. Replacing a tag
object is not OK, even if it points to the same commit, as you are
losing the old tag object (replacing an object with a tag that points to
the original object or its descendent is OK in theory, though I doubt it
is common enough to worry about).
I think that is a reasonable rule that could be applied across all parts
of the namespace hierarchy. And it could be applied by the client,
because all you need to know is whether ref->old_sha1 is reachable from
ref->new_sha1.
But it is somewhat orthogonal to the "already exists" idea, and checking
refs/tags/. Those ideas are about enforcing sane rules on the tag
hierarchy. My rule is a safety valve that is meant to extend the idea of
"is fast-forwardable" to non-commit object types. If we do it at all, it
should be part of the fast-forward check (e.g., as part of ref_newer).
The current code conflates the two under the "already exists" condition,
which is just wrong. I think the best thing at this point is to split
the two ideas apart, keep the refs/tags check (and translate it to
"already exists" in the UI, as we do), and table the safety valve. I am
not even sure if it is something that is useful, and it can come later
if we decide it is.
I feel moderately strongly about the last point. Defining special
semantics for one hierarchy (e.g. "refs/tags/") and implementing a
policy for enforcement is one thing, but a random policy that
depends on object type that applies globally is simply insane. The
user may want to do "refs/tested/" hierarchy that is meant to hold
references to commit, with one annotated tag "refs/tested/latest"
that points at the "latest tested version" with some commentary, and
maintain the latter by keep pushing to it. If that is the semantics
the user wanted to ahve in the "refs/tested/" hierarchy, it is not
reasonable to require --force for such a workflow. The user knows
better than Git in such a case.
I see what you are saying, but I think the ship has already sailed to
some degree. We already implement the non-fast-forward check everywhere,
and I cannot have a "refs/tested" hierarchy that pushes arbitrary
commits without regard to their history. If I have such a hierarchy, I
have to use "--force" (or more likely, mark the refspec with "+").
In my mind, the object-type checking is just making that fast-forward
check more thorough (i.e., extending it to non-commit objects).
cache.h | 1 -
remote.c | 24 +-----------------------
t/t5516-fetch-push.sh | 21 ---------------------
3 files changed, 1 insertion(+), 45 deletions(-)
The patch itself looks fine to me. Whether we agree on the fast-forward
object-type checking or not, it is the correct first step to take in
either case.
-Peff
On Wed, Jan 16, 2013 at 11:43 AM, Jeff King [off-list ref] wrote:
I think that is a reasonable rule that could be applied across all parts
of the namespace hierarchy. And it could be applied by the client,
because all you need to know is whether ref->old_sha1 is reachable from
ref->new_sha1.
is_forwardable() did solve a UI issue. Previously all instances where
old is not reachable by new were assumed to be addressable with a
merge. is_forwardable() attempted to determine if the concept of
forwarding made sense given the inputs. For example, if old is a blob
it is useless to suggest merging it.
Chris
On Wed, Jan 16, 2013 at 08:19:28PM -0600, Chris Rorvick wrote:
On Wed, Jan 16, 2013 at 11:43 AM, Jeff King [off-list ref] wrote:
quoted
I think that is a reasonable rule that could be applied across all parts
of the namespace hierarchy. And it could be applied by the client,
because all you need to know is whether ref->old_sha1 is reachable from
ref->new_sha1.
is_forwardable() did solve a UI issue. Previously all instances where
old is not reachable by new were assumed to be addressable with a
merge. is_forwardable() attempted to determine if the concept of
forwarding made sense given the inputs. For example, if old is a blob
it is useless to suggest merging it.
I think it makes sense to mark such a case as different from a regular
non-fast-forward (because "git pull" is not the right advice), but:
1. is_forwardable should assume a missing object is a commit not to
regress the common case; otherwise we do not show the pull advice
when we probably should, and most of the time it is going to be a
commit
2. When we know that we are not working with commits, I am not sure
that "already exists" is the right advice to give for such a case.
It is neither "this tag already exists, so we do not update it",
nor is it strictly "cannot fast forward this commit", but rather
something else.
The expanded definition of "what is a fast forward" that I
suggested would let this fall naturally between the two.
-Peff
On Wed, Jan 16, 2013 at 9:11 PM, Jeff King [off-list ref] wrote:
quoted
is_forwardable() did solve a UI issue. Previously all instances where
old is not reachable by new were assumed to be addressable with a
merge. is_forwardable() attempted to determine if the concept of
forwarding made sense given the inputs. For example, if old is a blob
it is useless to suggest merging it.
I think it makes sense to mark such a case as different from a regular
non-fast-forward (because "git pull" is not the right advice), but:
1. is_forwardable should assume a missing object is a commit not to
regress the common case; otherwise we do not show the pull advice
when we probably should, and most of the time it is going to be a
commit
Yes, obviously this was a bug, thus the use of "attempted" above. It
would have been better to assume a missing 'old' was potentially
forwardable to present the user with the most helpful advice.
2. When we know that we are not working with commits, I am not sure
that "already exists" is the right advice to give for such a case.
It is neither "this tag already exists, so we do not update it",
nor is it strictly "cannot fast forward this commit", but rather
something else.
But the reference already existing in the remote is a substantial
reason for not allowing the push in all of these cases. You can break
this out further if you like to explain why the specific reference
shouldn't be moved on the remote, but this is even more complicated a
simple "is old reachable from new?" test.
Chris