git-shortlog hangs on bare repo without --bare option

I didn't see this happen with git 1.5.x, but if you do git-shortlog on a bare repo, without specifying --bare, the command will seemingly hang indefinitely.

Here's my output:

[garry@garry-thinkpad] ~/dev (master) $ cd ebay4r/                       <----- Repo with working tree (non-bare)
[garry@garry-thinkpad] ~/dev/ebay4r (master) $ git shortlog -e -s
   130  Garry Dolley <gdolley@arpnetworks.com>
[garry@garry-thinkpad] ~/dev/ebay4r (master) $ cd ..
[garry@garry-thinkpad] ~/dev (master) $ git clone --mirror ebay4r/ ebay4r-bare
Initialized empty Git repository in /home/garry/dev/ebay4r-bare/
[garry@garry-thinkpad] ~/dev (master) $ cd ebay4r-bare/
[garry@garry-thinkpad] ~/dev/ebay4r-bare (master) $ git shortlog -e -s
<hang>                                                                   <----- *** It hangs here ***

[garry@garry-thinkpad] ~/dev/ebay4r-bare (master) $ git --bare shortlog -e -s
   130  Garry Dolley <gdolley@arpnetworks.com>
[garry@garry-thinkpad] ~/dev/ebay4r-bare (master) $ 

If newlines break this output in your reader, you can also view it here: http://pastie.org/261134

I'm using git 1.6.0.1

Is this a bug?

Garry Dolley <gdolley@arpnetworks.com> writes:

Tryp typing ^D here.  I think it is operating in the filter mode.

On Wed, Aug 27, 2008 at 01:32:31PM -0700, Junio C Hamano wrote:

Yeah, ^D just gets me back to the prompt, no output. It does indeed seem to be waiting for something.

Garry Dolley schrieb:

FWIW, I tried git 1.5.0 and it hangs, too. Which exact version did work for you? Could you, based on it, bisect the commit that introduced this behaviour?

As a workaround, you can use "git log | git shortlog".

René

On Wed, Aug 27, 2008 at 11:40:52PM +0200, Ren?? Scharfe wrote:

I just tried it with 1.5.6.2, and it hung. Unfortunately, I had several versions of git inside my ~/src as I upgraded over the months, and just last week I decided to blow away the old ones, so I don't know what version it was that I saw this work. :(

Either way, it's too big a deal, I just wanted to point out what I saw.

If it helps, this is the last few lines of strace before it hangs:

stat(".git", 0x7fff6e44dd90) = -1 ENOENT (No such file or directory) open(".git/config", O_RDONLY) = -1 ENOENT (No such file or directory) pipe([3, 4]) = 0 clone(child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7f7a6642d790) = 12674 dup2(3, 0) = 0 close(3) = 0 close(4) = 0 select(1, [0], NULL, [0], NULL

So looks like it is waiting to be fed some input?  It's hungry :)

Also, 'git --bare shortlog' works too.

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

It is not a workaround. Shortlog can work as a filter to "git log", but if you give revs to work on, e.g. "git shortlog -s -n v1.5.6.5.." or "git shortlog HEAD", it can generate the log and feed to itself.

Somehow it (perhaps incorrectly, I did not look at the command line input) judging that your command line is telling it not to do the log generation itself but instead you will feed the log from its standard input, which happens to be your terminal. So another workaround would be to type the series of log messages ;-)

Junio C Hamano <gitster@pobox.com> writes:

And I think this is related to the complexity that snuck in when worktree feature was added to the setup sequence.

Untested, but I think this would help.

 setup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Junio C Hamano <gitster@pobox.com> writes:

While it might help, I see there are more serious issue here.

If you start "git-shortlog" (or anything that has USE_PAGER but not RUN_SETUP in git.c) inside your project/.git directory (or a bare one, for that matter), the call sequence would become like this:

git.c::main()
 -> handle_internal_command()
   -> run_command()
     -> commit_pager_choice()
       -> setup_pager() (this is in pager.c)
         -> git_config(git_default_config, NULL)
           -> git_path("config")
             -> get_git_dir()
               -> setup_git_env()
                 This sets git_dir to ".git"!!!!

And that was part of the reason why "git-shortlog" started from a bare repository did not even notice that the default "HEAD" that is given internally by the program is not a valid ref (because it tries to read from .git/HEAD without the patch I sent earlier).

This may be repeating what Jeff said earlier in another thread, but I think we should rethink the start-up sequence carefully. Ideally (I am thinking aloud)...

 * Have a single function "git_setup()" that notices --bare and --git-dir
   from the command line and GIT_DIR environment, and does the discovery
   of git_dir (if not told with the command line or environment
   explicitly); if we do not find it, do *NOT* barf.  Just record the
   facts it finds somewhere (e.g. the location of git_dir, absense of
   git_dir, if the repository is explicitly bare, etc.).

 * As the next step, still in this single function, if we have git_dir,
   find out where the work_tree is, paying attention to --work-tree from
   the command line and GIT_WORK_TREE environment if exists.  If we do not
   have work tree, do *NOT* barf.  Again, just record the facts it finds.
   If we do not have --work-tree from the command line and if we are not
   told that it is --bare from the command line, then we may need to open
   the config to see where core.worktree points at.

 * Before returning from that single setup function, i think it is Ok to
   make it mimick the current git_setup_directory_gently(), cd up to the
   normal work-tree top if needed, and make it return prefix.

 * After all of the above is done, then we can start futzing with other
   things such as pager.  At that point we know we can read from the
   correct configuratoin.

 * Each individual command may decide to refuse to work in the absense of
   git_dir and/or work_tree, by calling require_work_tree(), etc.

Hmm?

Junio C Hamano <gitster@pobox.com> writes:

While this might help, I see there are more serious issues here.

If you start "git-shortlog" (or anything that has USE_PAGER but not RUN_SETUP in git.c) inside your project/.git directory (or a bare one, for that matter), the call sequence would become like this:

git.c::main()
 -> handle_internal_command()
   -> run_command()
     -> commit_pager_choice()
       -> setup_pager() (this is in pager.c)
         -> git_config(git_default_config, NULL)
           -> git_path("config")
             -> get_git_dir()
               -> setup_git_env()
                 This sets git_dir to ".git"!!!!

And that was part of the reason why "git-shortlog" started from a bare repository did not even notice that the default "HEAD" that is given internally by the program is not a valid ref (because it tries to read from .git/HEAD without the patch I sent earlier). Oh, of course that git_config() won't read from the right config file either.

This may be repeating what Jeff said earlier in another thread, but I think we should rethink the start-up sequence carefully. Ideally (I am thinking aloud)...

 * Have a single function "git_setup()" that notices --bare and --git-dir
   from the command line and GIT_DIR environment, and does the discovery
   of git_dir (if not told with the command line or environment
   explicitly); if we do not find it, do *NOT* barf.  Just record the
   facts it finds somewhere (e.g. the location of git_dir, absense of
   git_dir, if the repository is explicitly bare, etc.).

 * As the next step, still in this single function, if we have git_dir,
   find out where the work_tree is, paying attention to --work-tree from
   the command line and GIT_WORK_TREE environment if exists.  If we do not
   have work tree, do *NOT* barf.  Again, just record the facts it finds.
   If we do not have --work-tree from the command line and if we are not
   told that it is --bare from the command line, then we may need to open
   the config to see where core.worktree points at.

 * Before returning from that single setup function, i think it is Ok to
   make it mimick the current git_setup_directory_gently(), cd up to the
   normal work-tree top if needed, and make it return prefix.

 * After all of the above is done, then we can start futzing with other
   things such as pager.  At that point we know we can read from the
   correct configuratoin.

 * Each individual command may decide to refuse to work in the absense of
   git_dir and/or work_tree, by calling require_work_tree(), etc.

Hmm?

On Wed, Aug 27, 2008 at 04:16:32PM -0700, Junio C Hamano wrote:

Yep. This is the exact same problem I have been complaining about for a while.

Yes, this is roughly what I have been thinking of, as well. IOW, unconditionally find everything right at the beginning, but let us decide whether to barf later, when we know what command we are running.

A few complexities I found were:

  - some things really want no setup, like init and clone. I don't
    recall the exact issues, but trying obvious things caused breakage
    there.

  - the setup needs to not just be part of git.c, but factored out so
    non-builtin C programs can use it. I don't recall if there are
    issues related to git-sh-setup that need to be dealt with.

I think my approach was something like "let's just unconditionally call setup_git_directory() in git.c, and wrap it with a static variable to make sure we don't run it twice." but I ran into a host of issues. Sorry I can't remember specifics.

-Peff

Jeff King <peff@peff.net> writes:

An obvious thing is to start these commands in a subdirectory of an existing git worktree.

Obviously.

On 8/28/08, Junio C Hamano <gitster@pobox.com> wrote:

Part of worktree issues comes from the fact that worktree setup is spread over many functions, starting at setup_git_dir*() and ending at setup_work_tree(). So setting it up in a single function and turning setup_work_tree() into "check and die()" logic sounds great.