The short of it is that it's dangerous, we see people confused by it

(there was another one just yesterday), and it's a FAQ:
  http://git.or.cz/gitwiki/GitFaq#head-b96f48bc9c925074be9f95c0fce69bcece5...
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.
And it's even on Sam's "UI improvements" list. :)
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).
-Peff

--

"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
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?

--

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

--

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

--

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
(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.
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--

--

(3) set the config in the target repository to allow such a push

    regardless of the git version.
Remember, I am in the third camp in this topic myself.
--

Junio,

thanks for supporting the "third way". I am not sure whether I

interpret it correctly but in the same thread several message earlier

you wrote "We can ship 1.6.1 with it and then switch the default to

forbid in 1.6.3, for example". With the default set to "deny" it would

be useful if the git-push error message will indicate what config

variable to set in order to reverse the denial.
--Leo--

--

I meant to suggest that change contained in 3/4 can instead be "set the

configuration to allow such a dangerous push upfront, and make sure the

pushes the current tests perform actually are still allowed", _if_ you are

changing the default to forbid.
I think the default should be to warn for two release cycles during which

we will give deprecation notice, and then switch the default to forbid

(and we do not touch "git init/git clone" at all --- changing the default

to forbid in newly created repositories earlier than existing repositories

would be changing the behaviour of the command between old and new

repositories, which is madness).  If we are going this route, I think we

can modify the tests 3/4 touches to set the configuration to allow such a

push and make sure that such a push is still allowed.
--

Ah, I see. I did think about using the config variable for those tests,

but it felt too much like testing two things at once. That is, it is

nicer to debug if each test breaks only when the thing it is testing for

is broken, not some other random unrelated feature. Obviously that isn't

always possible, but it seemed kind of clumsy to me.
Anyway, with a default of "warn" the tests don't need any update at all

(and do serve as a test that we still haven't broken people), and I can

pass the decision off to whoever changes it to "refuse" after the
OK, the patch is below, replacing 4/4. 3/4 can simply be dropped at this

point (and I think 1/4 is a no-brainer to apply, and 2/4 is probably

worth it as cleanup).
I worded the warning to explain what happened so that the Frequently

Asking users might have a clue that something bad has happened. But

maybe it should also:
  - suggest "git reset --hard"; of course, then we need to explain that

    you would be losing your work, so we have to warn about that, too.
  - more explicitly warn that the behavior is deprecated.
Also, we could potentially note the deprecation in the documentation for
I agree. I suggested that for another config option recently, and I now

think I was wrong. It really doesn't dodge the "things are changing"

bullet. It just makes them change at a slightly different time, which

can be even more confusing (i.e., "this breaks in my repo, but when I

make a test repo it works" or vice versa).
I do feel like we made a config change like that at some point long ago,

but I can't recall for what, or the reasoning. Maybe
Again, I am not sure that is best, as above. But I tried to cover all

cases explicitly with my tests, so I think we should get good coverage

either way (and my tests don't depend on any particular default config

setting).
-- >8 --

receive-pack: detect push to current branch of non-bare repo
Pushing into the currently checked out branch of a non-bare

repository can be dangerous; the HEAD th...
[ message continues ]

Hmm, I wonder if it would be possible to also add a "detach" variant;

which would create a detached-HEAD at the current commit when

automatically receiving a push to the working branch.  I have a

post-receive script that does so right now on a couple repositories.

It's still a little confusing to someone actively working in the

repository being pushed to, but it's much easier to explain than the

current default behavior.
Cheers,

Kyle Moffett

--

A neat idea, but I'm not sure what workflow that is meant to support.
Before you had:
  1. git push non-bare-remote theirHEAD

  2a. echo Oops, I've just screwed myself.

    3a. ssh remote 'git reset --soft HEAD@{1}'

  2b. echo Oops, I just screwed somebody else.

    3b. echo sorry | mail somebody.else
With "refuse" you have:
  1. git push non-bare-remote theirHEAD

  2. echo Oops, rejected.

  3. git push non-bare-remote theirHEAD:elsewhere

  4a. ssh remote 'git merge elsewhere'

  4b. echo 'please merge elsewhere' | mail somebody.else
which is an improvement. With "detach" you have:
  1. git push non-bare-remote theirHEAD

  2. echo Oh, now we've detached on the remote.

  3a. ssh remote 'git checkout theirHEAD'

  3b. echo 'please merge theirHEAD. BTW, you have been detached without

            realizing it, so make sure you didn't lose any commits.' |

            mail somebody.else
So I think in the case that you are working by yourself, you haven't

really saved much effort (you didn't have to repeat your push, but you

still have to go to the remote and checkout instead of merge). But if

you are pushing into somebody _else_'s repo, you have just mightily

confused them as they start to make commits on top of the detached HEAD.
Still, there may be some instances where moving to the detached HEAD is

preferable. But, like the "try to merge if we can" strategy, I think it

is better implemented by setting denyCurrentBranch to ignore and using a

hook for those instances. And if either hook becomes ubiquitous, maybe

it will be worth implementing within git itself (but I doubt it for

either, as the desired behavior is highly dependent on your personal

workflow).
-Peff

--

Basically, I have a remote tree on a fast multicore box used for runs

of a test suite on various peoples different branches.  When I want

somebody to push something for me to test, they push directly to that

repo, and when I'm done playing with a previous run I just do:
$ git checkout new/branch/to/test

$ make clean

$ ./configure

$ make

$ make check
Occasionally I notice a bug which I want to temporarily fix to let the

build continue, even though I will need to have the author merge that

fix as a part of his original buggy patch.  If nobody pushes the

branch I'm currently testing again, I can "git diff" just fine to see

what I had to fix.  If somebody pushes to a different branch than the

one I'm testing, it's also fine.  The inconsistency is pushing to the

branch I'm on.
So it would be handy to be able to mark that repository as

"detach-HEAD-on-push-of-current-branch", which would let me remember

where I was, even if that's not where that branch is anymore.
There are other ways I could probably do something very similar, but

since the config option was being added it seemed it would probably be

easy to extend.  If nobody else is interested in that behavior, I will

just keep maintaining my own hook, but I thought I'd mention it.
Cheers,

Kyle Moffett

--

OK, I see how using a detached HEAD makes sense. But I think just going

straight to a detached HEAD might make even more sense. With your

proposed behavior, you need to be prepared to unexpectedly and

asynchronously move to a detached HEAD at any time, so why not just

start there in the first place?
And then the "push to current branch" problem is neatly solved: you have

no current branch.
So:
  $ git checkout new/branch/to/test^0

  $ make, configure, etc
-Peff

--

Exactly.
I keep a handful pseudo worktrees around (created with git-new-workdir on

top of a single repository) for quick patch test and build purposes.  I do

not push into them but pushing into a non-bare repository and checking out

the same branch twice in such a setup share exactly the same issue, and I

keep their HEADs all detached for exactly the same reason.

--

I guess the issue comes down to a UI complication.  It would very easy

for me to tell somebody how to check out and test their branch in my

testbed if I'm not around, except for that little bit of arcane

syntax.  Moreover, the consequences if they forget are really

frustrating and hard to figure out.  It's also very easy with a GUI to

do the simple *rightclick branch, click "Checkout"*, but would be much

harder to do the detached HEAD checkout correctly.
If it didn't involve reconfiguring a lot of other people's

repositories, I might consider having them push to "refs/remotes/*".

In theory that's actually much closer to what I'm doing anyways.  That

would force any checkouts to be bare, but it would require lots of

git-foo on the pushing side.  Perhaps some way to "git push" which

asks the remote repository where it wants the stuff?
Alternatively, it might be possible to add ref attributes or a config

option to force detached HEAD checkouts.
Cheers,

Kyle Moffett

--

If the problem is merely the syntax, then perhaps that argues for "git
And again, perhaps this argues for a "Detach" option in the GUI.
But I have to admit, this is a pretty infrequently-used use-case. I

detach all the time when looking at non-branches, but I can't think of
Or git-receive could even just silently munge the incoming refs when

writing them out (i.e., it exposes "refs/test/*" as "refs/heads/*", and

when you ask to write "refs/heads/foo" it writes "refs/test/foo"

instead).
Though that sort of lying feels a little wrong to me, since the pushing

side will incorrectly update its tracking branches. It wouldn't so bad

if the "fetch" side respected the munging, too.
But again, this seems uncommon enough that it is not worth trying to
I think that is a more sensible solution. Your workflow is not about

"sometimes I want to detach the HEAD" but rather "in this particular

repo, we should _always_ detach the HEAD." Which a config option

represents very nicely.
-Peff

--

Hi,
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

--

Pushing into the currently checked out branch of a non-bare

repository can be dangerous; the HEAD then loses sync with

the index and working tree, and it looks in the receiving

repo as if the pushed changes have been reverted in the

index (since they were never there in the first place).
This patch adds a safety valve that checks for this

condition and denies the push. We trigger the check only on

a non-bare repository, since a bare does not have a working

tree (and in fact, pushing to the HEAD branch is a common

workflow for publishing repositories).
This behavior is still configurable, though, since some very

specific setups may want to allow such a push if they know

they will take action to reconcile the working tree and HEAD

afterwards (e.g., a post-receive hook that does "git reset

--hard").
Signed-off-by: Jeff King <peff@peff.net>

---

My feeling is that this is dangerous behavior that we see new users

confused by, so it is worth addressing. The other obvious route is to

at least _try_ the merge, and if it comes out cleanly, to allow it.
But it looks like Sam is promoting that as a hook, which makes a lot

more sense to me. And we can still support that, but the user of the

hook must now not only install the hook, but also set the config value.
I am open to comments on the name of the config value. Somebody at the

GitTogether suggested (possibly under the influence of beer) that it be

receive.PEBKAC (since you should only turn it off if you really know

what you're doing, you would set PEBKAC to "false"), but I didn't want

to give the impression that git wasn't user-friendly. ;)
One final issue: do we need to make a special exception for "branch yet

to be born"? I believe we do so for the analagous "fetch" situation.
 Documentation/config.txt |    8 ++++++++

 builtin-receive-pack.c   |   16 ++++++++++++++++

 t/t5516-fetch-push.sh    |   21 +++++++++++++++++++++

 3 files changed, 45 insertions(+), 0 deletions(-)
diff --git a/Documentation/config.txt b/Docu...
[ message continues ]

Many tests create a new repo, and then push into the master

branch (sometimes after making some commits on that branch).

After such a push the index and working tree of the

receiving repo are out of sync with the HEAD. This isn't a

problem for most tests, since they don't bother looking at

the working tree after such a push.  But this is generally a

dangerous behavior, and the tests would break if we later

decided to put in a safety valve.
Depending on the situation, this patch takes one of two

approaches:
  - creates the pushed-to repo as a bare repository. This

    works if we don't actually want to create our own

    commits in the repo.
  - switches the pushed-to repo to another branch before

    pushing. Since we never look at the working tree after

    the push anyway, this doesn't impact the test results.
Signed-off-by: Jeff King <peff@peff.net>

---

This is not the _most_ minimal patch, since when changing a non-bare

repo to a bare one, the name of the git dir changed (e.g.,

s{victim/.git}{victim}), causing a lot of textual changes. We could

technically call the bare clone "victim/.git", but I think this is less

confusing (if a bit harder to read the diff).
 t/t5400-send-pack.sh        |   30 ++++++++++++----------

 t/t5401-update-hooks.sh     |   58 +++++++++++++++++++++---------------------

 t/t5405-send-pack-rewind.sh |    3 +-

 t/t5516-fetch-push.sh       |    3 +-

 t/t5517-push-mirror.sh      |    2 +-

 5 files changed, 50 insertions(+), 46 deletions(-)
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh

index da69f08..6bcb4df 100755

--- a/t/t5400-send-pack.sh

+++ b/t/t5400-send-pack.sh

@@ -31,7 +31,7 @@ test_expect_success setup '

 	    parent=$commit || return 1

 	done &&

 	git update-ref HEAD "$commit" &&

-	git clone ./. victim &&

+	git clone --bare ./. victim &&

 	cd victim &&

 	git log &&

 	cd .. &&

@@ -68,7 +68,7 @@ test_expect_success 'pack the destination repository' '

 tes...
[ message continues ]

t5516 sets up some utility functions for starting each test

with a clean slate. However, there were a few tests added

that do not use these functions, but instead make their own

repositories.
Let's bring these in line with the rest of the tests. Not

only do we reduce the number of lines, but these tests will

benefit from any further enhancements to the utility

scripts.
The conversion is pretty straightforward. Most of the tests

created a parent/child clone relationship, for which we now

use 'testrepo' as the parent.  One test looked in testrepo,

but relied on previous tests to have set it up; it now sets

up testrepo explicitly, which makes it a bit more robust to

changes in the script, as well.
Signed-off-by: Jeff King <peff@peff.net>

---

This is on top of 'next' to pick up the recent test from Clemens

Buchacher.
A few oddities here while I was digging in the history:
  - I actually introduced the first of these tests for local tracking

    refs in 09fba7a59 (and the others, being related, copied the style).

    But then I reverted them in 0673c96, because Alex had added other

    similar tests in t5404.  However, these tests ended up being

    re-added by Dscho in 28391a80, which adds a totally unrelated test.

    I think it's the result of a bad patch application (IIRC, he marked

    up my tests to avoid having them chdir for the whole test script.

    During application, Junio would see them going from tweaks to whole

    creation, and presumably just resolved the conflict that way).
    So my initial thought was to simply delete these tests. But since

    then, other related tests have been added to this script, and we do

    want to keep those. So I decided to keep them all, as they form a

    logical progression related to tracking refs. So while there is some

    duplication with t5404, I don't think it is a problem.
  - One of the tests called 'pwd', and I can't see that it would do

    anything useful. I assume it was just leftover debugging cruft

  ...
[ message continues ]

Commit a240de11 introduced this test and the code to make it

successful.
Signed-off-by: Jeff King <peff@peff.net>

---

Reading over the mailing list postings which led to a240de11, I think it

is simply a case that Jan didn't fully understand what expect_failure

meant (it means "this is a test that is currently broken, but we hope to

fix in the future", and not anything to do with the test_must_fail in

the test itself).
 t/t5400-send-pack.sh |    2 +-

 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh

index 6fe2f87..da69f08 100755

--- a/t/t5400-send-pack.sh

+++ b/t/t5400-send-pack.sh

@@ -103,7 +103,7 @@ unset GIT_CONFIG GIT_CONFIG_LOCAL

 HOME=`pwd`/no-such-directory

 export HOME ;# this way we force the victim/.git/config to be used.
-test_expect_failure \

+test_expect_success \

 	'pushing a delete should be denied with denyDeletes' '

 	cd victim &&

 	git config receive.denyDeletes true &&

--

1.6.0.3.866.gc189b
--

Hi,
Yes, that's exactly what happened, and it won't likely happen again.

Thanks for fixing and for the Cc.
-Jan

--