Re: [PATCH] git-push: update remote tags only with force

2012-10-31 Thread Chris Rorvick
(oops, now my email was rejected)

On Wed, Oct 31, 2012 at 12:55 AM, Felipe Contreras
felipe.contre...@gmail.com wrote:
 Hi,

 (again because the mailing list rejected it) (Gmal switched interface
 and HTML is the default)

 On Wed, Oct 31, 2012 at 6:37 AM, Chris Rorvick ch...@rorvick.com wrote:

 References are allowed to update from one commit-ish to another if the
 former is a ancestor of the latter.  This behavior is oriented to
 branches which are expected to move with commits.  Tag references are
 expected to be static in a repository, though, thus an update to a
 tag (lightweight and annotated) should be rejected unless the update is
 forced.

 To enable this functionality, the following checks have been added to
 set_ref_status_for_push() for updating refs (i.e, not new or deletion)
 to restrict fast-forwarding in pushes:

   1) The old and new references must be commits.  If this fails,
  it is not a valid update for a branch.

   2) The reference name cannot start with refs/tags/.  This
  catches lightweight tags which (usually) point to commits
  and therefore would not be caught by (1).

 If either of these checks fails, then it is flagged (by default) with a
 status indicating the update is being rejected due to the reference
 already existing in the remote.  This can be overridden by passing
 --force to git push.

 The new status has the added benefit of being able to provide accurate
 feedback as to why the ref update failed and what can be done.
 Currently all ref update rejections are assumed to be for branches.

 Makes sense to me. I've believe I've been hit by this a couple of
 times when tags were updated, and a colleague did 'git push' and they
 went all back, or something like that. To handle that case properly
 probably more changes are needed, but this is a change in the right
 direction.

 +test_expect_success 'push tag requires --force to update remote tag' '
 +   mk_test heads/master 
 +   mk_child child1 
 +   mk_child child2 
 +   (
 +   cd child1 
 +   git tag lw_tag 
 +   git tag -a -m message 1 ann_tag 
 +   git push ../child2 lw_tag 
 +   git push ../child2 ann_tag 
 +   file1 
 +   git add file1 
 +   git commit -m file1 
 +   git tag -f lw_tag 
 +   git tag -f -a -m message 2 ann_tag 
 +   ! git push ../child2 lw_tag 

 You probably should use test_must_fail.

Thanks, will fix.

 I don't see anything wrong with the patch, but I wonder if it might be
 possible to split it to ease the review.

I initially thought I'd split it into two: 1) to improve the feedback
and 2) to change the behavior.  But (1) was shaping up to be similar
in size to the sum so I scrapped that idea.  I will see what I can do.

Thanks,

Chris
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] git-push: update remote tags only with force

2012-10-30 Thread Felipe Contreras
Hi,

(again because the mailing list rejected it) (Gmal switched interface
and HTML is the default)

On Wed, Oct 31, 2012 at 6:37 AM, Chris Rorvick ch...@rorvick.com wrote:

 References are allowed to update from one commit-ish to another if the
 former is a ancestor of the latter.  This behavior is oriented to
 branches which are expected to move with commits.  Tag references are
 expected to be static in a repository, though, thus an update to a
 tag (lightweight and annotated) should be rejected unless the update is
 forced.

 To enable this functionality, the following checks have been added to
 set_ref_status_for_push() for updating refs (i.e, not new or deletion)
 to restrict fast-forwarding in pushes:

   1) The old and new references must be commits.  If this fails,
  it is not a valid update for a branch.

   2) The reference name cannot start with refs/tags/.  This
  catches lightweight tags which (usually) point to commits
  and therefore would not be caught by (1).

 If either of these checks fails, then it is flagged (by default) with a
 status indicating the update is being rejected due to the reference
 already existing in the remote.  This can be overridden by passing
 --force to git push.

 The new status has the added benefit of being able to provide accurate
 feedback as to why the ref update failed and what can be done.
 Currently all ref update rejections are assumed to be for branches.

Makes sense to me. I've believe I've been hit by this a couple of
times when tags were updated, and a colleague did 'git push' and they
went all back, or something like that. To handle that case properly
probably more changes are needed, but this is a change in the right
direction.

 +test_expect_success 'push tag requires --force to update remote tag' '
 +   mk_test heads/master 
 +   mk_child child1 
 +   mk_child child2 
 +   (
 +   cd child1 
 +   git tag lw_tag 
 +   git tag -a -m message 1 ann_tag 
 +   git push ../child2 lw_tag 
 +   git push ../child2 ann_tag 
 +   file1 
 +   git add file1 
 +   git commit -m file1 
 +   git tag -f lw_tag 
 +   git tag -f -a -m message 2 ann_tag 
 +   ! git push ../child2 lw_tag 

You probably should use test_must_fail.

I don't see anything wrong with the patch, but I wonder if it might be
possible to split it to ease the review.

Cheers.

--
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html