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

From: Jeff King <peff@peff.net>
Date: Jan 24, 2013, 06:58 UTC
Message-ID: <20130124065842.GC610@sigill.intra.peff.net>
In-Reply-To: <1358978130-12216-4-git-send-email-gitster@pobox.com>

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

Show 14 quoted lines

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

Show 6 quoted lines

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