Re: [PATCH 1/2] doc: git-push: create PUSH RULES section

[Junio C Hamano <gitster@pobox.com>]

"Julia Evans via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  --force::
> -	Usually, the command refuses to update a remote ref that is
> -	not an ancestor of the local ref used to overwrite it.
> -	Also, when `--force-with-lease` option is used, the command refuses
> -	to update a remote ref whose current value does not match
> -	what is expected.
> +	Usually, `git push` will refuse to update a branch that is not an
> +	ancestor of the local branch or commit being pushed.

I read this as "there are two conditions, and satisifying only one of
them is sufficient for the push to be allowed.  (1) the local branch
is a decendant of the remote branch being updated, or (2) the commit
we push to update the remote branch is a descendant of the remote
branch being updated".

But of course that is not what you wanted to say.  (1) would mean

    $ git reset origin/foo && git push origin anything:foo

would allow us to push literally anything to overwrite origin's foo
branch.

I think

    "... not an ancestor of the commit being pushed to update it."

would be a way to avoid such confusion.

The problem the original description has is the phrase "the local
ref used to overwrite it".  It wasn't as commonly done to push a
specific commit that may not necessarily at the tip of the branch
back when this paragraph was written.

> -This flag disables these checks, and can cause the remote repository
> -to lose commits; use it with care.
> +This flag disables that check, the other safety checks in PUSH RULES
> +below, and the checks in --force-with-lease. It can cause the remote
> +repository to lose commits; use it with care.

OK.

> +PUSH RULES
> +----------
> +
> +As a safety feature, the `git push` command only allows certain kinds of
> +updates to prevent you from accidentally losing data on the remote.
> +
> +Because branches and tags are intended to be used differently, the
> +safety rules for pushing to a branch are different from the rules
> +for pushing to a tag. In the following rules "update" means any
> +modifications except deletes. Deletions are always allowed, except when
> +forbidden by configuration or hooks.

One important operation is omitted.  "update" does not include
"create" in the following, no?  Obviously since refs/tags/ would
never take any "update" (unless forced), if "create" were thrown
into the same category as "update", you cannot push a new tag out.

So, next to "Deletions are always allowed", shouldn't we describe
what rules apply to creations?  I presume that they are also always
allowed?

> +1. If the push destination is a **branch** (`refs/heads/*`): only
> +   fast-forward updates are allowed: the destination must be an ancestor
> +   of the source commit. The source must be a commit.

Reads very well and also correct.

The first colon might be acceptable (I find it a bit odd, though).
The second colon is very weird.  ": only" -> ", only" & "allowed:
the" -> "allowed. The", perhaps?

> +2. If the push destination is a **tag** (`refs/tags/*`): all updates will
> +   be rejected. The source can be any object
> +   (since commits, trees and blobs can be tagged).

Again, I might prefer ":" -> ",".  I cannot decide which I prefer
between "all updates will be rejected" and "by default no updates
are allowed".  Either should be OK, so let's take what has already
been written.

The second sentence is not wrong per-se, and I can see that this was
inherited from the original, but gives me a strange aftertaste. When
you list object types in the context of "tag" and have only commit,
tree, and blob, a little voice in the back of my head asks "oh, what
happend to tags?".  It is made a bit worse with the phrase "can be
tagged", as it typically means either (1) to create an annotated or
signed tag object, or (2) to create a ref in refs/tags/ hierarchy
locally, but usually you do not think of pushing to refs/tags/
hierarchy as "tagging that object remotely".

I think the untold assumption here is that refs/tags/foo at the
remote is being updated most of the time from refs/tags/foo we have
locally, and "any kind of object can be tagged" is trying to say
that refs/tags/foo we have locally can be an object of any type, as
the act of creating a ref "refs/tags/foo" and pointing it directly
at an object is "to create a light-weight tag" for the object.
Since we can have not just tags but any kind of object locally
(because any object "can be tagged"), a push can ask object of any
kind to be pushed to refs/tags/* hierarchy.  But it is an awkward
concept to explain.

Would side-stepping what exactly "tagging a thing" means, and
phrasing it like this

    The source is not limited to an annotated or signed tag object,
    but can be a commit, a tree or even a blob.

work better, I wonder?

> +3. If the push destination is not a branch or tag:

Here, I do understand and support the colon, so I'd equally support
the first colon of the previous 2 sections for consistency.

> +   * If the source is a tree or blob object, any updates will be rejected

OK, so this is the same rule as the refs/tags/ hierarchy.

> +   * If the source is a tag or commit object, any fast-forward update
> +     is allowed, even in cases where what's being fast-forwarded is not a
> +     commit, but a tag object which happens to point to a new commit which
> +     is a fast-forward of the commit the last tag (or commit) it's
> +     replacing. Replacing a tag with an entirely different tag is also
> +     allowed, if it points to the same commit, as well as pushing a peeled
> +     tag, i.e. pushing the commit that existing tag object points to, or a
> +     new tag object which an existing commit points to.
> +
> +You can override these rules by passing `--force` or by adding the
> +optional leading `+` to a refspec. The only exception to this is that no
> +amount of forcing will make a branch accept a non-commit object.
> +
> +Hooks and configuration can also override or amend these rules,
> +see e.g. `receive.denyNonFastForwards` and `receive.denyDeletes`
> +in linkgit:git-config[1] and `pre-receive` and `update` in
> +linkgit:githooks[5].

Very well written.  refs/heads/ not taking any non-commit, and
receiver side hooks that may reject a push, are not something
"--force" can override.  The mention to "the only exception" above
sounded as if it forgot to mention the latter.

Looking mostly good.  Thanks.