Re: What's cooking in git.git (Oct 2011, #05; Fri, 14)

Jeff King [off-list ref] writes:

On Fri, Oct 14, 2011 at 04:23:21PM -0700, Junio C Hamano wrote:

quoted

* jk/pull-rebase-with-work-tree (2011-10-13) 1 commit
 - pull,rebase: handle GIT_WORK_TREE better

Looked reasonable.
Will merge to 'next'.

I'm not so sure. Didn't you demonstrate that cd_to_toplevel as-is will
not actually go to the toplevel if we're outside of the work tree?

And changing it is non-trivial, because there may be weird cases that
rely on staying there? See my final note in the thread:

  http://article.gmane.org/gmane.comp.version-control.git/183519

Hmm, I might be mistaken, but my impression was that sane people do not do
so, that the discussion that originated this proposed patch was not such a
use case, and most importantly that fixing unsane ones is just the matter
for them to set GIT_WORKING_TREE correctly. So if anything, wouldn't
getting this in as early as possible to 'master' or at least 'next' help
catching a flaw in the above logic and possible downside in the real
world?

quoted

* jk/daemon-msgs (2011-10-14) 1 commit
 - daemon: give friendlier error messages to clients

Will merge to 'next'.

I'm happy to tweak the "access denied" message if other people want. I
kind of hoped it wouldn't matter, and that most sites would use
--informative-errors.

I've already updated it with the "not exported" bit from Sitaram.

Thanks.

On Sun, Oct 16, 2011 at 10:17:10AM -0700, Junio C Hamano wrote:

quoted

quoted

* jk/pull-rebase-with-work-tree (2011-10-13) 1 commit
 - pull,rebase: handle GIT_WORK_TREE better

Looked reasonable.
Will merge to 'next'.

I'm not so sure. Didn't you demonstrate that cd_to_toplevel as-is will
not actually go to the toplevel if we're outside of the work tree?

And changing it is non-trivial, because there may be weird cases that
rely on staying there? See my final note in the thread:

  http://article.gmane.org/gmane.comp.version-control.git/183519

Hmm, I might be mistaken, but my impression was that sane people do not do
so, that the discussion that originated this proposed patch was not such a
use case, and most importantly that fixing unsane ones is just the matter
for them to set GIT_WORKING_TREE correctly. So if anything, wouldn't
getting this in as early as possible to 'master' or at least 'next' help
catching a flaw in the above logic and possible downside in the real
world?

Hmm. I thought there were two separate problems:

  1. my analysis was wrong, and "git rev-parse --show-toplevel" did not
     actually show the root of the work tree when we were outside it
     (and therefore cd_to_toplevel did not actually go anywhere)

  2. some people might be outside of the work tree, set GIT_DIR
     explicitly, but not bother setting GIT_WORK_TREE

But I was wrong on (1). If GIT_WORK_TREE is set, we _will_ actually go
to its work tree, which is what we want. If it's not set, then we go
nowhere, and assume the cwd is the work tree. Which is compatible with
current behavior, and makes (2) still work.

So I was thinking there was more problem then there is. I agree we
should let it go to 'next' to shake out any bugs.

Sorry for the noise.

-Peff

On Sun, Oct 16, 2011 at 10:17:10AM -0700, Junio C Hamano wrote:

quoted

quoted

* jk/daemon-msgs (2011-10-14) 1 commit
 - daemon: give friendlier error messages to clients

Will merge to 'next'.

I'm happy to tweak the "access denied" message if other people want. I
kind of hoped it wouldn't matter, and that most sites would use
--informative-errors.

I've already updated it with the "not exported" bit from Sitaram.

What you queued in d5570f4 looks sane, but the merge into next is
curious. It's an evil merge that turns on informative errors by default:

  $ git show 415cf53
  commit 415cf53e710b2ff44d485e253bb35a1ca5dff636
  Merge: fbc2ee6 d5570f4
  Author: Junio C Hamano [off-list ref]
  Date:   Sat Oct 15 21:25:41 2011 -0700

      Merge branch 'jk/daemon-msgs' into next

      * jk/daemon-msgs:
        daemon: give friendlier error messages to clients

      Conflicts:
          daemon.c

  diff --cc daemon.c
  index 91c4d9b,72fb53a..95b7df5
  --- a/daemon.c
  +++ b/daemon.c
  @@@ -20,6 -20,7 +20,7 @@@
    static int log_syslog;
    static int verbose;
    static int reuseaddr;
   -static int informative_errors;
  ++static int informative_errors = 1;

    static const char daemon_usage[] =
    "git daemon [--verbose] [--syslog] [--export-all]\n"


The conflicts come from merging with Duy's version, which was already in
next. But the introduced line comes from my second patch, which was
rightly dropped, and should not be there. Was this intentional, or did
you do something clever with "git am" and my original series during
resolution and accidentally pull in that change?

-Peff