Re: [RFD] Making "git push [--force/--delete]" safer?
Johannes Sixt writes: > Am 7/3/2013 21:53, schrieb Junio C Hamano: >> Johannes Sixt writes: >> >>> I don't think that is necessary. We already have *two* options to >>> force-push a ref: the + in front of refspec, and --force. >> >> They mean exactly the same thing; the only difference being that "+" >> prefix is per target ref, while "--force" covers everything, acting >> as a mere short-hand to add "+" to everything you push. > > I know, and I'm saying that we do not have to keep this duplicity. Of course we do. If you change + prefix to "push but always require tracking ref in the opposite direction", you will break existing setup by (1) making it impossible to loosen the restriction per ref, and (2) forcing people to have reverse tracking ref. It is OK to introduce another prefix that mean a new thing. It is absolutely not OK to change the semantics of only one and not the other. -- 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: [RFD] Making "git push [--force/--delete]" safer?
Am 7/3/2013 21:53, schrieb Junio C Hamano: > Johannes Sixt writes: > >> I don't think that is necessary. We already have *two* options to >> force-push a ref: the + in front of refspec, and --force. > > They mean exactly the same thing; the only difference being that "+" > prefix is per target ref, while "--force" covers everything, acting > as a mere short-hand to add "+" to everything you push. I know, and I'm saying that we do not have to keep this duplicity. > If the "--lockref/--update-only-if-ref-is-still-there" option > defeats "--force", it should defeat "+src:dst" exactly the same way. This logic is backwards. If anything, then "--force" must defeat the safety that "--lockref" gives. -- Hannes -- 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: [RFD] Making "git push [--force/--delete]" safer?
Junio C Hamano writes: > Jonathan del Strother writes: > >> I'm struggling to think of instances where I wouldn't want this >> CAS-like behaviour. Wouldn't it be better to make it the default when >> pushing, and allowing the current behaviour with "git push >> --blind-force" or something? > > Not until we run this in the wild for a while and the mechanism > proves to be useful without being too cumbersome to some population. > > Then at a major version bump, we can start talking about enabling it > by default, allowing people to selectively disable it. If we enable this by default, we would need to be a lot more careful designing what should happen when there is no remote-tracking branch the corresponds to what we are updating/deleting. The proposed behaviour so far is to fail, and that is justifiable because "the user asked us to check, but did not say what to check against, and we tried to check with a remote-tracking branch and found none. We cannot satisfy the user's request to check, hence we fail". Enabling the check by default will change the picture somewhat; that justification no longer holds. If you are pushing to more than one publishing branches of your own, there is no reason to have remote-tracking branches for the secondary locations, because you always push to all your publishing repositories at the same time, and you only need to keep remote tracking for one of them to remember what you pushed out. Making a push to secondaries fail in such a case is bad, and forcing the user to disable the feature for each secondary remotes is unnice, too. -- 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: [RFD] Making "git push [--force/--delete]" safer?
Johannes Sixt writes: > I don't think that is necessary. We already have *two* options to > force-push a ref: the + in front of refspec, and --force. They mean exactly the same thing; the only difference being that "+" prefix is per target ref, while "--force" covers everything, acting as a mere short-hand to add "+" to everything you push. If the "--lockref/--update-only-if-ref-is-still-there" option defeats "--force", it should defeat "+src:dst" exactly the same way. -- 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: [RFD] Making "git push [--force/--delete]" safer?
Jonathan del Strother writes: > I'm struggling to think of instances where I wouldn't want this > CAS-like behaviour. Wouldn't it be better to make it the default when > pushing, and allowing the current behaviour with "git push > --blind-force" or something? Not until we run this in the wild for a while and the mechanism proves to be useful without being too cumbersome to some population. Then at a major version bump, we can start talking about enabling it by default, allowing people to selectively disable it. -- 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: [RFD] Making "git push [--force/--delete]" safer?
Johan Herland writes: >> I'll leave the name open but tentatively use this name in the >> following, primarily to see how well it sits on the command line >> examples. > > I agree that neither --expect nor --validate are very good. I also > don't like --lockref, mostly because there is no locking involved, and Yes and no. This is not compare-and-swap but is "store-conditional" step in ll/sc. It is letting other people's activities to break your lock to prevent you from making an undesirable update. So in that sense, this mechanism is very much a lock. > Some other suggestions: > > a) --update-if. I think this reads quite nicely in the fully specified > variant: --update-if=theirRefName:expectedValue, but it becomes more > cryptic when defaults are assumed (i.e. --update-if without any > arguments). This name is in line with the "store conditional" aspect of the operation, but it, together with your --precond and --pre-verify, share the same problem as your --validate. This is only to check one specific precondition "The remote ref being updated must point at this object", but all the names you suggested are too broad. If we were to go in the direction (3) I suggested in the original message to let you specify an arbitrary script that reads the list of proposed updates and decide to allow them, --update-if=script.sh would be the ideal name for that option to specify the script to be run, though. That mechanism is broad enough to deserve such a broad name, if we were to go in that direction. > b) --precond. This makes it clear that we're specifying a precondition > on the push. Again, I think the fully specified version reads nicely, > but it might seem a little cryptic when no arguments are given. See above. > c) --pre-verify, --pre-check are merely variations on (b), other > variations include --pre-verify-ref or --pre-check-ref, making things > more explicit at the cost of option name length. See above. > So, how do we deal with the various corner cases? I thought I spelled out everything, but apparently I didn't. Here is what I had in mind. (1) A bare "--lockref" exists on the command line. E.g. $ git push --lockref [remote [refspec]...] ;# nothing else about lockref This will apply to updates of _all_ refs to be updated (e.g. with "remote.origin.push = +refs/heads/pu:refs/heads/pu", the update of 'pu' at the origin will be rejected if 'pu' fails to pass the test) with this push. We make sure - we have remote-tracking branch for the updated ref; if we do not have any, we *fail* the update. - the value of that remote-tracking branch is the same as what the remote advertises to "git push"; if they do not match, we *fail* the update. This includes the case where there is no such ref at the remote (may have deleted while we are looking the other way). (2) Remote ref specified on one of the --lockref option(s). E.g. $ git push --lockref=theirRef[:value] [remote [refspec]...] This will apply to updates of _only_ the refs given. refs not covered by --lockref will follow the usual rule (i.e. with --force, anything goes, without --force, only fast-forward is allowed). If ":value" is given, we will use it, otherwise we will try to find the remote tracking branch for the updated ref, just like a non-specific case as above. A --lockref=theirRef[:value] that specifies theirRef that is not being pushed will be _ignored_ and not checked, so that you could say [alias] safepush = push --lockref=next [remote "origin"] push = refs/heads/maint:refs/heads/maint push = refs/heads/master:refs/heads/master push = refs/heads/next:refs/heads/next push = +refs/heads/pu:refs/heads/pu and then do $ git safepush origin +next after a major version bump to rewind 'next' but still do so with safety, while still allowing you to say $ git safepush origin maint to push out 'maint' without having to worry about --lockref=next getting in the way. (3) Mixing --lockref and --lockref=theirRef[:value]. Apply (2) for refs we do have remote ref specified on --lockref, and apply (1) for other refs we are going to update. In any case, this check happens after we learn the current value of remote refs but before we propose what the updated values would be, so we can afford to fail the entire push atomically. We could only fail the ones that do not pass the check and let others go, but I do not think it is a good idea. So in short, I think I agree with you on the semantics. -- 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: [RFD] Making "git push [--force/--delete]" safer?
Am 7/3/2013 12:50, schrieb Michael Haggerty: > On 07/03/2013 12:11 PM, Johan Herland wrote: >> On Wed, Jul 3, 2013 at 12:06 PM, Jonathan del Strother >> wrote: >>> I'm struggling to think of instances where I wouldn't want this >>> CAS-like behaviour. Wouldn't it be better to make it the default when >>> pushing, and allowing the current behaviour with "git push >>> --blind-force" or something? >> >> I believe I agree with you. I guess the reason this hasn't come up >> before is that by far most of the pushes we do are either >> fast-forwarding, or pushing into a non-shared repo (e.g. my own public >> repo), and this safety only really applies when we're forcing a >> non-fast-forward push into a shared repo... > > I didn't see Jonathan's original email but I was having exactly the same > though as him (and was even going to propose the same option name). > > Non-ff pushing without knowing what you are going to overwrite is > irresponsible in most scenarios, and (if backwards-compatibility > concerns can be overcome) I think it would be quite prudent to forbid a > non-ff push if there is no local remote-tracking branch that is > up-to-date at the time of the push. Circumventing that check should > require some extra-super-force option. I don't think that is necessary. We already have *two* options to force-push a ref: the + in front of refspec, and --force. IMO, the meaning of + should be changed to "force-push with safety", and --force can then be used to override if the safety triggers (i.e., --force is your extra-super-force option). -- Hannes -- 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: [RFD] Making "git push [--force/--delete]" safer?
On 3 July 2013 11:00, Johan Herland wrote:
> On Wed, Jul 3, 2013 at 10:49 AM, Junio C Hamano wrote:
>> Johan Herland writes:
>>> Overnight, it occured to me that --force-if-expected could be
>>> simplified by leveraging the existing --force option; for the above
>>> two examples, respectively:
>>>
>>> $ git push --force --expect
>>> # validate foo @ origin == @{upstream} before pushing
>>>
>>> and
>>>
>>> $ git push --force --expect=refs/original/foo my_remote HEAD:foo
>>> # validate foo @ my_remote == refs/original/foo before pushing
>>
>> First, on the name.
>>
>> I do not think either "--validate" or "--expect" is particularly a
>> good one. The former lets this feature squat on a good name that
>> covers a much broader spectrum, forbidding people from adding other
>> kinds of validation later. "--expect" is slightly less bad in that
>> sense; saying "we expect this" does imply "otherwise it is an
>> unexpected situation and we would fail", but the name still does not
>> feel ideal.
>>
>> What is the essense of compare-and-swap? Perhaps we can find a good
>> word by thinking that question through.
>>
>> To me, it is a way to implement a "lock" on the remote ref without
>> actually taking a lock (which would leave us open for a stale lock),
>> and this "lock"-ness is what we want in order to guarantee safety.
>>
>> So we could perhaps call it "--lockref"?
>>
>> I'll leave the name open but tentatively use this name in the
>> following, primarily to see how well it sits on the command line
>> examples.
>
> I agree that neither --expect nor --validate are very good. I also
> don't like --lockref, mostly because there is no locking involved, and
> I think most users will jump to an incorrect conclusion about what
> this option does, unless they read the documentation.
>
> Some other suggestions:
>
> a) --update-if. I think this reads quite nicely in the fully specified
> variant: --update-if=theirRefName:expectedValue, but it becomes more
> cryptic when defaults are assumed (i.e. --update-if without any
> arguments).
>
> b) --precond. This makes it clear that we're specifying a precondition
> on the push. Again, I think the fully specified version reads nicely,
> but it might seem a little cryptic when no arguments are given.
>
> c) --pre-verify, --pre-check are merely variations on (b), other
> variations include --pre-verify-ref or --pre-check-ref, making things
> more explicit at the cost of option name length.
I'm struggling to think of instances where I wouldn't want this
CAS-like behaviour. Wouldn't it be better to make it the default when
pushing, and allowing the current behaviour with "git push
--blind-force" or something?
--
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: [RFD] Making "git push [--force/--delete]" safer?
On 07/03/2013 12:11 PM, Johan Herland wrote: > On Wed, Jul 3, 2013 at 12:06 PM, Jonathan del Strother > wrote: >> I'm struggling to think of instances where I wouldn't want this >> CAS-like behaviour. Wouldn't it be better to make it the default when >> pushing, and allowing the current behaviour with "git push >> --blind-force" or something? > > I believe I agree with you. I guess the reason this hasn't come up > before is that by far most of the pushes we do are either > fast-forwarding, or pushing into a non-shared repo (e.g. my own public > repo), and this safety only really applies when we're forcing a > non-fast-forward push into a shared repo... I didn't see Jonathan's original email but I was having exactly the same though as him (and was even going to propose the same option name). Non-ff pushing without knowing what you are going to overwrite is irresponsible in most scenarios, and (if backwards-compatibility concerns can be overcome) I think it would be quite prudent to forbid a non-ff push if there is no local remote-tracking branch that is up-to-date at the time of the push. Circumventing that check should require some extra-super-force option. So yes, I very much like the general idea of the RFD and personally would lean towards making it stronger and default, at the 2.0 transition if necessary. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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: [RFD] Making "git push [--force/--delete]" safer?
On Wed, Jul 3, 2013 at 12:06 PM, Jonathan del Strother wrote: > I'm struggling to think of instances where I wouldn't want this > CAS-like behaviour. Wouldn't it be better to make it the default when > pushing, and allowing the current behaviour with "git push > --blind-force" or something? I believe I agree with you. I guess the reason this hasn't come up before is that by far most of the pushes we do are either fast-forwarding, or pushing into a non-shared repo (e.g. my own public repo), and this safety only really applies when we're forcing a non-fast-forward push into a shared repo... ...Johan -- Johan Herland, www.herland.net -- 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: [RFD] Making "git push [--force/--delete]" safer?
On Wed, Jul 3, 2013 at 10:49 AM, Junio C Hamano wrote:
> Johan Herland writes:
>> Overnight, it occured to me that --force-if-expected could be
>> simplified by leveraging the existing --force option; for the above
>> two examples, respectively:
>>
>> $ git push --force --expect
>> # validate foo @ origin == @{upstream} before pushing
>>
>> and
>>
>> $ git push --force --expect=refs/original/foo my_remote HEAD:foo
>> # validate foo @ my_remote == refs/original/foo before pushing
>
> First, on the name.
>
> I do not think either "--validate" or "--expect" is particularly a
> good one. The former lets this feature squat on a good name that
> covers a much broader spectrum, forbidding people from adding other
> kinds of validation later. "--expect" is slightly less bad in that
> sense; saying "we expect this" does imply "otherwise it is an
> unexpected situation and we would fail", but the name still does not
> feel ideal.
>
> What is the essense of compare-and-swap? Perhaps we can find a good
> word by thinking that question through.
>
> To me, it is a way to implement a "lock" on the remote ref without
> actually taking a lock (which would leave us open for a stale lock),
> and this "lock"-ness is what we want in order to guarantee safety.
>
> So we could perhaps call it "--lockref"?
>
> I'll leave the name open but tentatively use this name in the
> following, primarily to see how well it sits on the command line
> examples.
I agree that neither --expect nor --validate are very good. I also
don't like --lockref, mostly because there is no locking involved, and
I think most users will jump to an incorrect conclusion about what
this option does, unless they read the documentation.
Some other suggestions:
a) --update-if. I think this reads quite nicely in the fully specified
variant: --update-if=theirRefName:expectedValue, but it becomes more
cryptic when defaults are assumed (i.e. --update-if without any
arguments).
b) --precond. This makes it clear that we're specifying a precondition
on the push. Again, I think the fully specified version reads nicely,
but it might seem a little cryptic when no arguments are given.
c) --pre-verify, --pre-check are merely variations on (b), other
variations include --pre-verify-ref or --pre-check-ref, making things
more explicit at the cost of option name length.
> Then on the semantics/substance.
>
> I had quite a similar thought as you had while reading your initial
> response. In the most generic form, we would want to be able to
> pass necessary information fully via the option, i.e.
>
> --lockref=theirRefName:expectedValue
>
> but when the option is spelled without details, we could fill in the
> default values by making a reasonable guess of what the user could
> have meant. If we only have --lockref without refname nor value,
> then we will enable the safety for _all_ refs that we are going to
> update during this push. If we have --lockref=theirRefName without
> the expected value for that ref, we will enable the safety only for
> the ref (you can give more than one --lockref=theirRefName), and
> guess what value we should expect. If we have a fully specified
> option, we do not have to guess the value.
>
> And for the expected value, when we have a tracking branch for the
> branch at the remote we are trying to update, its value is a very
> good guess of what the user meant.
>
> Note, however, that this is very different from @{upstream}.
>
> You could be pushing a branch "frotz", that is configured to
> integrate with "master" taken from "origin", but
>
> (1) to a branch different from "master" of "origin", e.g.
>
> $ git push --lockref origin frotz:nitfol
> $ git push --lockref origin :nitfol ;# deleting
>
> (2) even to a branch of a remote that is different from "origin",
> e.g.
>
> $ git push --lockref xyzzy frotz:nitfol
> $ git push --lockref xyzzy :nitfol ;# deleting
>
> Even in these case, if you have a remote tracking branch for the
> destination (i.e. you have refs/remotes/origin/nitfol in case (1) or
> refs/remotes/xyzzy/nitfol in case (2) to be updated by fetching from
> origin or xyzzy), we can and should use that value as the default.
>
> There is no room for frotz@{upstream} (or @{upstream} of the current
> branch) to get in the picture.
>
> Except when you happen to be pushing with "push.default = upstream",
> that is. But that is a natural consequence of the more generic
> check with "our remote tracking branch of the branch we are updating
> at the remote" rule.
Fully agree with all of the above. I've been living under the
"push.default = upstream" rock for too long... ;)
So, how do we deal with the various corner cases?
If we don't have a tracking branch for the branch at the remote we are
trying to update (and an expected value is not specified on the
command line) we should obviously fail the push as we were asked to
verify, but don't know what to verify against.
Re: [RFD] Making "git push [--force/--delete]" safer?
Johan Herland writes:
> Overnight, it occured to me that --force-if-expected could be
> simplified by leveraging the existing --force option; for the above
> two examples, respectively:
>
> $ git push --force --expect
> # validate foo @ origin == @{upstream} before pushing
>
> and
>
> $ git push --force --expect=refs/original/foo my_remote HEAD:foo
> # validate foo @ my_remote == refs/original/foo before pushing
First, on the name.
I do not think either "--validate" or "--expect" is particularly a
good one. The former lets this feature squat on a good name that
covers a much broader spectrum, forbidding people from adding other
kinds of validation later. "--expect" is slightly less bad in that
sense; saying "we expect this" does imply "otherwise it is an
unexpected situation and we would fail", but the name still does not
feel ideal.
What is the essense of compare-and-swap? Perhaps we can find a good
word by thinking that question through.
To me, it is a way to implement a "lock" on the remote ref without
actually taking a lock (which would leave us open for a stale lock),
and this "lock"-ness is what we want in order to guarantee safety.
So we could perhaps call it "--lockref"?
I'll leave the name open but tentatively use this name in the
following, primarily to see how well it sits on the command line
examples.
Then on the semantics/substance.
I had quite a similar thought as you had while reading your initial
response. In the most generic form, we would want to be able to
pass necessary information fully via the option, i.e.
--lockref=theirRefName:expectedValue
but when the option is spelled without details, we could fill in the
default values by making a reasonable guess of what the user could
have meant. If we only have --lockref without refname nor value,
then we will enable the safety for _all_ refs that we are going to
update during this push. If we have --lockref=theirRefName without
the expected value for that ref, we will enable the safety only for
the ref (you can give more than one --lockref=theirRefName), and
guess what value we should expect. If we have a fully specified
option, we do not have to guess the value.
And for the expected value, when we have a tracking branch for the
branch at the remote we are trying to update, its value is a very
good guess of what the user meant.
Note, however, that this is very different from @{upstream}.
You could be pushing a branch "frotz", that is configured to
integrate with "master" taken from "origin", but
(1) to a branch different from "master" of "origin", e.g.
$ git push --lockref origin frotz:nitfol
$ git push --lockref origin :nitfol ;# deleting
(2) even to a branch of a remote that is different from "origin",
e.g.
$ git push --lockref xyzzy frotz:nitfol
$ git push --lockref xyzzy :nitfol ;# deleting
Even in these case, if you have a remote tracking branch for the
destination (i.e. you have refs/remotes/origin/nitfol in case (1) or
refs/remotes/xyzzy/nitfol in case (2) to be updated by fetching from
origin or xyzzy), we can and should use that value as the default.
There is no room for frotz@{upstream} (or @{upstream} of the current
branch) to get in the picture.
Except when you happen to be pushing with "push.default = upstream",
that is. But that is a natural consequence of the more generic
check with "our remote tracking branch of the branch we are updating
at the remote" rule.
--
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: [RFD] Making "git push [--force/--delete]" safer?
On Wed, Jul 3, 2013 at 12:55 AM, Johan Herland wrote:
> I assume that in most cases the expected value of the remote ref would
> equal the current value of the corresponding remote-tracking ref in
> the user's repo, so why not use that as the default expected value?
> E.g.:
>
> $ git config push.default simple
> $ git checkout -b foo -t origin/foo
> # prepare non-ff update
> $ git push --force-if-expected
> # the above validates foo @ origin != origin/foo before pushing
Oops, typo: s/!=/==/
>
> And if the users expects a different value, (s)he can pass that to the
> same option:
>
> $ git push --force-if-expected=refs/original/foo my_remote HEAD:foo
> # the above fails if foo @ origin != refs/original/foo
>
> The option name probably needs a little work, but as long as it
> properly communicates the user's _intent_ I'm fine with whatever we
> call it.
Overnight, it occured to me that --force-if-expected could be
simplified by leveraging the existing --force option; for the above
two examples, respectively:
$ git push --force --expect
# validate foo @ origin == @{upstream} before pushing
and
$ git push --force --expect=refs/original/foo my_remote HEAD:foo
# validate foo @ my_remote == refs/original/foo before pushing
In other words, the --expect option becomes a modifier on the --force
behaviour: If --expect is given, and the remote ref is not as
expected, then the push will still fail, even when --force is given.
Furthermore, this could be fleshed out by allowing the user to
configure push.expect = True, in which case --expect will be assumed
whenever --force is used, and the user can override with --no-expect.
If push.expect == True (or if --expect is given on command-line
without a parameter), we default to using @{upstream} as the expected
value, and we complain to the user if the current branch has no
upstream. This way, you can still enable push.expect even when you do
not configure @{upstream}, but it compels you to always supply
--expect=$something (or --no-expect) when you use --force.
...Johan
PS: I'm still unsure about the option naming. Maybe --validate would
be better than --expect, but I feel it should convey more strongly
that we're doing _pre_-validation, as opposed to (post-)validating the
_result_ of the push, whatever that would look like.
--
Johan Herland,
www.herland.net
--
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: [RFD] Making "git push [--force/--delete]" safer?
On Tue, Jul 2, 2013 at 10:57 PM, Junio C Hamano wrote: [...] > (2) Add --compare-and-swap=dst:expect parameters, e.g. > > $ git push --cas=master:deadbabecafe --cas=next:cafebabe ":" > > This removes the "reservation" I expressed against (1) above > (i.e. we are doing a "matching" push in this example, but we > will fail if 'master' and 'next' are not pointing at the > expected objects). I still think this is too long/verbose for the average user to remember, and type out. Also, I don't like the name, as it is too 'technical', and describes the nature of the implementation (i.e. the "how") rather than the purpose of using it (i.e. the "why" or "what"). > (3) Add a mechanism to call a custom validation script after "git > push" reads the list of tuples, > but before responding with the proposed update. The script > would be fed a list ofname, refname> tuples (i.e. what the sender _would_ tell the > receiving end if there weren't this mechanism), and can tell > "git push" to fail with its exit status. > > This would be the most flexible in that the validation does > not have to be limited to "the ref must be still pointing at > the object we expect" (aka compare-and-swap); the script could > implement other semantics (e.g. "the ref must be pointing at > the object or its ancestor"). With this, I guess --dry-run could be reformulated as a trivial validation script that always returns a non-zero exit code (although it should still cause 'push' to return zero). [...] > I am inclined to say, if we were to do this, we should do (2) among > the above three. > > But of course, others may have better ideas ;-). I assume that in most cases the expected value of the remote ref would equal the current value of the corresponding remote-tracking ref in the user's repo, so why not use that as the default expected value? E.g.: $ git config push.default simple $ git checkout -b foo -t origin/foo # prepare non-ff update $ git push --force-if-expected # the above validates foo @ origin != origin/foo before pushing And if the users expects a different value, (s)he can pass that to the same option: $ git push --force-if-expected=refs/original/foo my_remote HEAD:foo # the above fails if foo @ origin != refs/original/foo The option name probably needs a little work, but as long as it properly communicates the user's _intent_ I'm fine with whatever we call it. ...Johan -- Johan Herland, www.herland.net -- 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
[RFD] Making "git push [--force/--delete]" safer?
Consider these two scenarios. 1. If you are collaborating with others and you have arranged with the participants to rewind a shared branch, you would do something like this: $ git fetch origin branch ... fetch everything so that we won't lose anything ... $ git checkout -b rebase-work FETCH_HEAD $ git rebase -i $ git push origin +HEAD:branch The last step has to be "--force", as you are deliberately pushing a history that does not fast-forward. 2. If you know a branch you pushed over there has now been fully merged to the "trunk" and want to remove it, you would do this: $ git fetch origin branch:tmp-branch trunk:tmp-trunk ... double check to make sure branch is fully merged ... $ git merge-base --is-ancestor tmp-branch tmp-trunk; echo $? 0 ... good, branch is part of trunk ... $ git push origin --delete branch The last step would delete the branch, but you made sure it has been merged to the trunk, not to lose anybody's work. But in either of these cases, if something happens at 'origin' to the branch you are forcing or deleting since you fetched to inspect it, you may end up losing other people's work. - In the first scenario, somebody who is unaware of the decision to rewind and rebuild the branch may attempt to push to the branch between the time you fetched to rebase it and the time you pushed to replace it with the result of the rebasing. - In the second scenario, somebody may have pushed a new change to the branch since you fetched to inspect. We can make these pushes safer by optionally allowing the user to tell "git push" this: I am forcing/deleting, based on the assumption that the value of 'branch' is still at this object. If that assumption no longer holds, i.e. if something happened to the branch since I started preparing for this push, please do not proceed and fail this push. With such a mechanism, the first example would say "'branch' must be at $(git rev-parse --verify FETCH_HEAD)", and the second example would say "'branch' must be at $(git rev-parse --verify tmp-branch)". The network protocol of "git push" conveys enough information to make this possible. An early part of the exchange goes like this: receiver -> sender list of sender -> receiver list of sender -> receiver packfile payload ... When the "git push" at the last step of the above two examples contact the other end, we would immediately know what the current value of 'branch' is. We can locally fail the command if the value is different from what we expect. Now at the syntax level, I see three possibilities to let the user express this new constraints: (1) Extend "refspec" syntax to "src:dst:expect", e.g. $ git push there HEAD:branch:deadbabecafe would say "update 'branch' with the object at my HEAD, only if the current value of 'branch' is deadbabecafe". An reservation I have against this syntax is that it does not mesh well with the "update the upstream of the currrent branch" and other modes, and instead you have to always spell three components out. But perhaps requiring the precondition is rare enough that it may be acceptable. (2) Add --compare-and-swap=dst:expect parameters, e.g. $ git push --cas=master:deadbabecafe --cas=next:cafebabe ":" This removes the "reservation" I expressed against (1) above (i.e. we are doing a "matching" push in this example, but we will fail if 'master' and 'next' are not pointing at the expected objects). (3) Add a mechanism to call a custom validation script after "git push" reads the list of tuples, but before responding with the proposed update. The script would be fed a list of tuples (i.e. what the sender _would_ tell the receiving end if there weren't this mechanism), and can tell "git push" to fail with its exit status. This would be the most flexible in that the validation does not have to be limited to "the ref must be still pointing at the object we expect" (aka compare-and-swap); the script could implement other semantics (e.g. "the ref must be pointing at the object or its ancestor"). But it may be cumbersome to use and the added flexibility may not be worth it. - The way to specify the validation script could be an in-repository hook but then there will need a way to pass additional per-invocation parameters (in the earlier sample scenarios, values of FETCH_HEAD and tmp-branch). - Or it could be a "--validate-script=check.sh" option, and it is up to the caller how to tailor that check.sh script customized for this particular invocation (i.e. embedding the values of FETCH_HEAD and tmp
