From: Nguyen Thai Ngoc Duy Date: Thu, 22 Jul 2010 22:36:29 GMT Subject: Re: [PATCH] t7002: test git grep --no-index from a bare repository Message-ID: In-Reply-To: <20100722070219.GA6577@burratino> 2010/7/22 Jonathan Nieder : > Hi Duy, > > Nguyễn Thái Ngọc Duy wrote: > >> Subject: [PATCH] t7002: test git grep --no-index from a bare repository > > It’s t7810 now (to keep t7811 company). > >>  There is an interesting thing about this command. Back in tp/setup >>  series, there is a patch that changes the current behavior, >>  "calculate prefix even if no worktree is found". grep is interesting >>  because it depends on the current behavior, i.e. prefix being NULL >>  in bare repo, while it still needs true prefix to do chdir() >>  stuff in run_pager(). > > Yes, sorry to let this hang for so long.  I liked your setup series > for many reasons and am happy to see pieces of it coming back to life. > >> +++ b/Documentation/git-grep.txt >> @@ -28,8 +28,9 @@ SYNOPSIS >>  DESCRIPTION >>  ----------- >>  Look for specified patterns in the tracked files in the work tree, blobs >> -registered in the index file, or blobs in given tree objects. >> - >> +registered in the index file, or blobs in given tree objects. By default >> +it will only search tracked files within the current directory (or full >> +tree if in bare repository). > > Probably deserves more detail. > >        Searches for lines matching the specified patterns in the >        work tree, the index, or a specified tree. > >        By default, 'git grep' only examines tracked files in the >        subtree of the work tree rooted at the current working >        directory.  Output consists of matching lines preceded with >        the corresponding filename and a colon. > >        With `--cached`, 'git grep' does the same search in the >        version scheduled for the next commit in the index. > >        With `--no-index`, 'git grep' pays no mind to the index >        file and reports *all* matching files under the working >        directory. > >        Given a commit name, 'git grep' does the same search in that >        historical version.  More generally, given a tree name, 'git >        grep' searches the subtree of that tree object corresponding >        to the path to the current directory from the root of the work >        tree (or the entire tree if there is no work tree, as in a >        bare repository). > Yeah.. grep looks more complex than I thought :) Documentation patch should go separately then. Please go ahead and make a patch of this text. Oh you did. >> >>  OPTIONS >>  ------- >> diff --git a/builtin/grep.c b/builtin/grep.c >> index 597f76b..e8abdc7 100644 >> --- a/builtin/grep.c >> +++ b/builtin/grep.c >> @@ -1109,6 +1109,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix) >> >>       if (use_threads) >>               hit |= wait_all(); >> + >> +     /* FIXME prefix is NULL in bare repo, no matter where cwd is */ >>       if (hit && show_in_pager) >>               run_pager(&opt, prefix); > > This comment seems kind of unhelpful.  Maybe something like > >        /* >         * NOTE NOTE NOTE: Even in a bare repository, the user >         * probably expected the command specified with -O to run in >         * the current directory, but when --no-index is supplied, we >         * are passing it paths relative to the .git directory. >         * Until that changes, this needs not to chdir() in that case. >         */ > > Do I understand correctly? Hmm.. on second thought, the fault is probably not grep's. These things must be consistent together: - new cwd after setup is at worktree top-level directory (or undefined if no worktree is found?) - prefix must be aligned with new cwd. That is, chdir(prefix) must give the original cwd (**1**) - gitdir is relative to new cwd - worktree is relative to new cwd So probably best to adjust cwd in setup_git_directory_gently() in this case to align with the NULL prefix, not the other way around. Something like this (still incorrect): diff --git a/setup.c b/setup.c index 87c21f0..b67b3aa 100644 --- a/setup.c +++ b/setup.c @@ -412,6 +412,15 @@ const char *setup_git_directory_gently(int *nongit_ok) inside_git_dir = 1; if (!work_tree_env) inside_work_tree = 0; + + /* + * The old behavior is return NULL here. + * Follow it and cwd back to because + * NULL prefix means cwd does not move + */ + if (chdir(cwd)) + die_errno("Cannot come back to cwd"); + if (offset != len) { root_len = offset_1st_component(cwd); cwd[offset > root_len ? offset : root_len] = '\0'; (**1**) Not entirely correct, if original cwd is outside worktree, then prefix is NULL anyway because prefix is not designed to be "../../../" -- Duy