Re: [PATCH v4 0/3] Finishing touches to push advises

2013-01-24 Thread Chris Rorvick
On Wed, Jan 23, 2013 at 3:55 PM, Junio C Hamano gits...@pobox.com wrote:
 This builds on Chris Rorvick's earlier effort to forbid unforced
 updates to refs/tags/ hierarchy and giving sensible error and advise
 messages for that case (we are not rejecting such a push due to fast
 forwardness, and suggesting to fetch and integrate before pushing
 again does not make sense).

FWIW, these changes look good to me.  The logic in
set_ref_status_for_push() is easier to follow and the additional error
statuses (and associated advice) make things much clearer.

Had I written the the already exists advice in the context of these
additional statuses I would have said the destination *tag* reference
already exists, or maybe even just the destination *tag* already
exists.  It's probably fine the way it is, but I only avoided using
tag in the advice because I was abusing it.

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 v4 0/3] Finishing touches to push advises

2013-01-24 Thread Junio C Hamano
Chris Rorvick ch...@rorvick.com writes:

 Had I written the the already exists advice in the context of these
 additional statuses I would have said the destination *tag* reference
 already exists, or maybe even just the destination *tag* already
 exists.

Yeah, now we do not use already exists for anything other than
refs/tags/, right?  Your rewording sounds like the right thing to
make it even clearer.

Thanks for bringing it up.  

Would it be sufficient to do this?  I think the tag already exists
in the remote is already clear that we are talking about the
destination.

diff --git a/builtin/push.c b/builtin/push.c
index a2b3fbe..78789be 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -228,7 +228,7 @@ static const char message_advice_ref_fetch_first[] =
   See the 'Note about fast-forwards' in 'git push --help' for 
details.);
 
 static const char message_advice_ref_already_exists[] =
-   N_(Updates were rejected because the destination reference already 
exists\n
+   N_(Updates were rejected because the tag already exists\n
   in the remote.);
 
 static const char message_advice_ref_needs_force[] =
--
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 v4 0/3] Finishing touches to push advises

2013-01-24 Thread Chris Rorvick
On Thu, Jan 24, 2013 at 11:04 PM, Junio C Hamano gits...@pobox.com wrote:
 Would it be sufficient to do this?  I think the tag already exists
 in the remote is already clear that we are talking about the
 destination.

Good point.

 diff --git a/builtin/push.c b/builtin/push.c
 index a2b3fbe..78789be 100644
 --- a/builtin/push.c
 +++ b/builtin/push.c
 @@ -228,7 +228,7 @@ static const char message_advice_ref_fetch_first[] =
See the 'Note about fast-forwards' in 'git push --help' for 
 details.);

  static const char message_advice_ref_already_exists[] =
 -   N_(Updates were rejected because the destination reference already 
 exists\n
 +   N_(Updates were rejected because the tag already exists\n
in the remote.);

  static const char message_advice_ref_needs_force[] =

Looks like the new-line is now unnecessary, but that looks good to me.

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


[PATCH v4 0/3] Finishing touches to push advises

2013-01-23 Thread Junio C Hamano
This builds on Chris Rorvick's earlier effort to forbid unforced
updates to refs/tags/ hierarchy and giving sensible error and advise
messages for that case (we are not rejecting such a push due to fast
forwardness, and suggesting to fetch and integrate before pushing
again does not make sense).

The series applies on top of 256b9d7 (push: fix refs/tags/
hierarchy cannot be updated without --force, 2013-01-16).

This fourth round swaps the order of clean-up patches and now the
bottom two are clean-up patches.  The main change is in the third
one.

When the object at the tip of the remote is not a committish, or the
object we are pushing is not a committish, the existing code already
rejects such a push on the client end, but we used the same error
and advice messages as the ones used when rejecting a push that does
not fast-forward, i.e. pull from there and integrate before pushing
again.  Introduce a new rejection reason NEEDS_FORCE and explain why
the push was rejected, stressing the fact that --force is required
when non committish objects are involved, so that the user can (1)
notice a possibly mistyped source object name or destination ref
name, when the user is trying to push an ordinary commit, or (2)
learn that --force is an appropriate thing to use when the user is
sure that s/he wants to push a non-committish (which is unusual).

Unlike the third round, we do not say fetch first, inspect the
situation to decide what to do, when we do not have the object
sitting at the tip of the remote.  Most likely, it is a commit
somebody who has been working on the same branch pushed that we
haven't fetched yet, so suggesting to pull is often sufficient and
appropriate, and in a more uncommon case in which the unknown object
is not a committish, the suggested pull will fail without making
permanent damage anywhere.  Next atttempt to push without changing
anything (e.g. reset --hard) will then trigger the NEEDS_FORCE
Your push involves non-commit objects case.


Junio C Hamano (3):
  push: further clean up fields of struct ref
  push: further simplify the logic to assign rejection reason
  push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE

 Documentation/config.txt | 12 +++-
 advice.c |  4 
 advice.h |  2 ++
 builtin/push.c   | 29 +
 builtin/send-pack.c  | 10 ++
 cache.h  |  6 +++---
 remote.c | 42 +++---
 send-pack.c  |  2 ++
 transport-helper.c   | 10 ++
 transport.c  | 14 +-
 transport.h  |  2 ++
 11 files changed, 105 insertions(+), 28 deletions(-)

-- 
1.8.1.1.517.g0318d2b

--
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