Re: [PATCH 7/7] push: document --lockref

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Jul 9, 2013, 20:27 UTC
Message-ID: <51DC723F.2000602@alum.mit.edu>
In-Reply-To: <1373399610-8588-8-git-send-email-gitster@pobox.com>

On 07/09/2013 09:53 PM, Junio C Hamano wrote:

Show 34 quoted lines

> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/git-push.txt | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index f7dfe48..e7c8bd6 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -11,6 +11,7 @@ SYNOPSIS
>  [verse]
>  'git push' [--all | --mirror | --tags] [--follow-tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
>  	   [--repo=<repository>] [-f | --force] [--prune] [-v | --verbose] [-u | --set-upstream]
> +	   [--lockref[=<refname>[:[<expect>]]]]
>  	   [--no-verify] [<repository> [<refspec>...]]
>  
>  DESCRIPTION
> @@ -146,6 +147,31 @@ already exists on the remote side.
>  	to the `master`	branch). See the `<refspec>...` section above
>  	for details.
>  
> +--lockref::
> +--lockref=<refname>::
> +--lockref=<refname>:<expect>::
> +	When updating <refname> at the remote, make sure that the
> +	ref currently points at <expect> (an object name), and else
> +	fail the push, even if `--force` is specified.  If only
> +	<refname> is given, the expected value is taken from the
> +	remote-tracking branch that holds the last-observed value of
> +	the <refname>.  <expect> given as an empty string means the
> +	<refname> should not exist and this push must be creating
> +	it.  If `--lockref` (without any value) is given, make sure
> +	each ref this push is going to update points at the object
> +	our remote-tracking branch for it points at.

I thought that the explanation in your patch 4/7 log message was clearer. In particular, I think that documenting the forms separately, as you did in the log message, makes it unambiguous, whereas for example the distinction in prose between "If only <refname> is given" and "<expect> given as an empty string" is easy to miss.

Does "--lockref" only apply to references that need non-ff updates, or to all references that are being pushed? This is mostly interesting for the zero-argument form (especially if a config option is invented to make this the default), but the question should also be answered for the other forms.

Show 5 quoted lines

> +This is meant to make `--force` safer to use.  Imagine that you have
> +to rebase what you have already published.  You will have to
> +`--force` the push to replace the history you originally published
> +with the rebased history.  If somebody else built on top of your
> +original history while you are rebasing, the tip of the branch at

s/are/were/

> +the remote may advance with her commit, and blindly pushing with

s/advance/have advanced/

> +`--force` will lose her work.  By using this option to specify that
> +you expect the history you are updating is what you rebased and want
> +to replace, you can make sure other people's work will not be losed

s/losed/lost/

> +by a forced push. in such a case.

s/push./push/ or s/in such a case.//

Show 5 quoted lines

> +
>  --repo=<repository>::
>  	This option is only relevant if no <repository> argument is
>  	passed in the invocation. In this case, 'git push' derives the
>

Another minor point: "git update-ref" allows either 40 "0" or the empty string to check that the ref doesn't already exist. For consistency it might be nice to accept 40 "0" here as well.

I still really like the idea of the feature.

<bikeshed> The name "--lockref" is OK, but for me it's less a question of "locking", because as far as the user is concerned the push is an atomic operation so there is no sense of a "lock" that is being held for a finite period of time. For me it is more a question of "checking" or "verifying". I see that the word "verify" already has a meaning for this command, so maybe "--checkref" or "--checkold" or "--checkoldref"? </bikeshed>

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/