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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 07/09/2013 09:53 PM, Junio C Hamano wrote:
> 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.

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

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