Re: [RFC PATCH 0/4] deny push to current branch of non-bare repo

Jeff King [off-list ref] writes:

The FAQ even says "don't do this until you know what you are doing." So
the safety valve is configurable, so that those who know what they are
doing can switch it off.

"We are breaking your existing working setup but you can add a new
configuration to unbreak it" should not be done lightly.  I think as the
end result it is a reasonable thing to aim for for this particular
feature, but we do need a transition plan patch in between that introduces
a step that warns but not forbids.  We can ship 1.6.1 with it and then
switch the default to forbid in 1.6.3, for example.

Patch 4/4 is the interesting one. 1/4 is a cleanup I saw while fixing
tests. 2/4 is a cleanup to prepare for 3/4. And 3/4 fixes a bunch of
tests which were inadvertently doing such a push (but didn't care
because they didn't look at the working directory).

I wonder if you can use the tests 3/4 touches as the test for your "keep
existing setup" configuration variable, pretending that they are old
timer's repositories?

On Fri, Nov 07, 2008 at 03:16:53PM -0800, Junio C Hamano wrote:

quoted

The FAQ even says "don't do this until you know what you are doing." So
the safety valve is configurable, so that those who know what they are
doing can switch it off.

"We are breaking your existing working setup but you can add a new
configuration to unbreak it" should not be done lightly.  I think as the
end result it is a reasonable thing to aim for for this particular
feature, but we do need a transition plan patch in between that introduces
a step that warns but not forbids.  We can ship 1.6.1 with it and then
switch the default to forbid in 1.6.3, for example.

Yeah, I was kind of hoping we could assume that anybody relying on this
behavior was somewhat insane, and wouldn't be too upset when it broke.
But you're probably right that we should be more conservative. I'll
rework it with a "yes/no/warn" option for the config, and we can set it
to "warn" (and those who really do want it can shut off the warning with
"no"). Or we can even start with just leaving it on "no", but I think
the deprecation period should begin when we switch it to "warn".

quoted

Patch 4/4 is the interesting one. 1/4 is a cleanup I saw while fixing
tests. 2/4 is a cleanup to prepare for 3/4. And 3/4 fixes a bunch of
tests which were inadvertently doing such a push (but didn't care
because they didn't look at the working directory).

I wonder if you can use the tests 3/4 touches as the test for your "keep
existing setup" configuration variable, pretending that they are old
timer's repositories?

Yes, they do break with 4/4 applied without 3/4 (that was how I found
them, but "git rebase -i" let me pretend I had the proper foresight. ;)
). We can keep 3/4 back until the switch from "warn" to "yes", if that's
what you are suggesting.

-Peff

Hi,

On Sat, 8 Nov 2008, Jeff King wrote:

On Fri, Nov 07, 2008 at 03:16:53PM -0800, Junio C Hamano wrote:

quoted

quoted

The FAQ even says "don't do this until you know what you are doing." 
So the safety valve is configurable, so that those who know what 
they are doing can switch it off.

"We are breaking your existing working setup but you can add a new 
configuration to unbreak it" should not be done lightly.  I think as 
the end result it is a reasonable thing to aim for for this particular 
feature, but we do need a transition plan patch in between that 
introduces a step that warns but not forbids.  We can ship 1.6.1 with 
it and then switch the default to forbid in 1.6.3, for example.

Yeah, I was kind of hoping we could assume that anybody relying on this
behavior was somewhat insane, and wouldn't be too upset when it broke.

I think I have a repository with "git read-tree -u -m HEAD" as update hook 
for that kind of behavior.

But I will not be the person responsible to keep that behavior, if I am 
the only one relying on it.

I very much like the approach of defaulting to "warn" for quite some time 
(but setting the variable to "refuse" in git-init) and then adapt the 
default after some time.

Ciao,
Dscho

On 11/8/08, Jeff King [off-list ref] wrote:

On Fri, Nov 07, 2008 at 03:16:53PM -0800, Junio C Hamano wrote:

 > > The FAQ even says "don't do this until you know what you are doing." So
 > > the safety valve is configurable, so that those who know what they are
 > > doing can switch it off.
 >
 > "We are breaking your existing working setup but you can add a new
 > configuration to unbreak it" should not be done lightly.  I think as the
 > end result it is a reasonable thing to aim for for this particular
 > feature, but we do need a transition plan patch in between that introduces
 > a step that warns but not forbids.  We can ship 1.6.1 with it and then
 > switch the default to forbid in 1.6.3, for example.


Yeah, I was kind of hoping we could assume that anybody relying on this
 behavior was somewhat insane, and wouldn't be too upset when it broke.

I do not think that having a work-flow different from yours deserves a
"somewhat insane" label. But let us consider the consequences of
banning push into a (current branch) non-bare repo. To propagate
changes to such a non-bare repo there are two remaining alternatives
neither of which is fully satisfactory:

(1) Switch target's current branch to something else (prevent a
conflict) before pushing and then restore it back after the push

(2) Use git-fetch from the target.

Method (1) is no better than what is available today with "git reset
--hard" to sync working directory.
Method (2) is even worse, because git-fetch provides no control of
what branches/tags to fetch, it sucks everything in from all branches.
"git-push", OTOH, can be instructed to be very selective.

Here is an example of such a work-flow

Foo.git -- main bare repo of the project
Foo.wip -- everyday "work in progress" repo. Cloned from Foo.git.
Pushes to Foo.git
Foo.wip.insane -- experimental "crazy" stuff cloned from Foo.wip.
Pushed to Foo.wip

Proposed patch makes this work flow impossible (cannot push from
Foo.wip.insane to Foo.wip)

--Leo--

On Mon, Dec 01, 2008 at 09:22:43PM -0500, Leo Razoumov wrote:

I do not think that having a work-flow different from yours deserves a
"somewhat insane" label. But let us consider the consequences of

  a) you are responding to a nearly month-old message. Please read the
     rest of the thread where we decide that it is not so insane, and
     that the behavior should be configurable with a default of "warn"
     at least for now.

  b) My comment was not that it is insane simply because it is different
     from mine. It is because it creates a dangerous situation (where
     dangerous implies changes might be silently lost) which requires
     manual intervention to fix, and which the user was given no warning
     whatsoever about. It is a direct response to frequent complaints on
     the list about users getting bit by this.

(1) Switch target's current branch to something else (prevent a
conflict) before pushing and then restore it back after the push

(2) Use git-fetch from the target.

(3) Use git-reset --hard, but set a config variable that says "I know
what I'm doing." You don't even have to do it per-repo, you can do it
per-user.

(4) Push into a non-current branch and merge from the target.

Method (2) is even worse, because git-fetch provides no control of
what branches/tags to fetch, it sucks everything in from all branches.
"git-push", OTOH, can be instructed to be very selective.

Er, what? git-fetch takes a refspec very similar to the ones used by
git-push. The real reason that (2) is not an acceptable solution is that
you can't necessarily connect to the source repo (e.g., it is on your
workstation with no ssh or git server running).

-Peff

On 12/1/08, Jeff King [off-list ref] wrote:

[..snip..]
 >
Er, what? git-fetch takes a refspec very similar to the ones used by
 git-push. The real reason that (2) is not an acceptable solution is that
 you can't necessarily connect to the source repo (e.g., it is on your
 workstation with no ssh or git server running).

 -Peff

I am sorry, I had to be more accurate in my wording. "git fetch" with
no explicit refspecs fetches everything in. It is quite cumbersome to
form a refspec for git-fetch operation if you are  not logged in into
the "source repo" machine. git-fetch does not have a --dry-run option
to help discover all the branch/tag names on the source side needed
for a meaningful refspec. "git-push -v --dry-run" allows one to
experiment and see what branches/tags exist at the destination and
form refspecs selectively. To the best of my knowledge, git-fetch does
not provide such discovery tools.

--Leo--