Re: [PATCH v4 3/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE

2013-01-24 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 I wonder if we can reword it to explain more about why we do not have
 the object, without getting too inaccurate. Something like:

   Updates were rejected because the remote contains objects that you do
   not have locally. This is usually caused by another repository pushing
   to the same ref. You may want to first merge the remote changes (e.g.,
   'git pull') before pushing again.

 I was also tempted to s/objects/work/, which is more vague, but is less
 jargon-y for new users who do not know how git works.

After all this is hint, and there is a value in being more
approachable at the cost of being less accurate, over being
impenetrable to achieve perfect correctness.

 Also, how should this interact with the checkout-then-pull-then-push
 advice? We make a distinction for the non-fastforward case between HEAD
 and other refs. Should we be making the same distinction here?

Possibly, but I am not among the people who cared most about the
distinction there; with the default behaviour switching to 'simple',
that distinction will start mattering even less, I suspect.
--
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 3/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE

2013-01-23 Thread Jeff King
On Wed, Jan 23, 2013 at 01:55:30PM -0800, Junio C Hamano wrote:

 If we do not have the current object at the tip of the remote, we do
 not even know that object, when fetched, is something that can be
 merged.  In such a case, suggesting to pull first just like
 non-fast-forward case may not be technically correct, but in
 practice, most such failures are seen when you try to push your work
 to a branch without knowing that somebody else already pushed to
 update the same branch since you forked, so pull first would work
 as a suggestion most of the time.
 
 In these cases, the current 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 non-fast-forward push, i.e. pull from
 there and integrate before pushing again.  Introduce new
 rejection reasons and reword the messages appropriately.

So obviously from our previous discussion, I agree with the general
behavior of this patch. Let me get nit-picky on the message itself,
though:

 +static const char message_advice_ref_fetch_first[] =
 + N_(Updates were rejected because you do not have the object at the 
 tip\n
 +of the remote. You may want to first merge the remote changes 
 (e.g.\n
 + 'git pull') before pushing again.\n
 +See the 'Note about fast-forwards' in 'git push --help' for 
 details.);
 +

The condition that triggers this message is going to come up fairly
often for new git users (e.g., anyone using a central repo model), which
I think is why the original message_advice_pull_before_push has gotten
so much attention.  And in most cases, users will be seeing this message
now instead of pull before push, because the common triggering cause
is somebody else pushing unrelated work.

The existing message says:

  Updates were rejected because a pushed branch tip is behind its remote
  counterpart. Check out this branch and merge the remote changes
  (e.g. 'git pull') before pushing again.

I wonder: will the new message be as comprehensible to a new user as the
old?

They are quite similar, but something about the presence of the word
behind in the latter makes me think it helps explain what is going on
a bit more. When I read the new one, my first question is why don't I
have that object?. Of course, saying behind in this case would not be
strictly accurate, because we do not even know the remote has a commit.

I wonder if we can reword it to explain more about why we do not have
the object, without getting too inaccurate. Something like:

  Updates were rejected because the remote contains objects that you do
  not have locally. This is usually caused by another repository pushing
  to the same ref. You may want to first merge the remote changes (e.g.,
  'git pull') before pushing again.

I was also tempted to s/objects/work/, which is more vague, but is less
jargon-y for new users who do not know how git works.

Also, how should this interact with the checkout-then-pull-then-push
advice? We make a distinction for the non-fastforward case between HEAD
and other refs. Should we be making the same distinction here?

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