Re: [RFC PATCH 2/3] grep: prepare grep for submodules

From: Chris Packham <judge.packham@gmail.com>
Date: Oct 1, 2010, 16:26 UTC
Message-ID: <4CA60BB5.4080202@gmail.com>
In-Reply-To: <AANLkTikH+sd2kPAraGSTB-ik-Toz3s2nTLoLVOj86oSm@mail.gmail.com>

On 01/10/10 07:37, Nguyen Thai Ngoc Duy wrote:

Show 20 quoted lines

> On 10/1/10, Chris Packham <judge.packham@gmail.com> wrote:
>>  > You can make setup_explicit_git_dir() realize that situation (current
>>  > working directory outside $GIT_WORK_TREE), then calculate and save the
>>  > submodule prefix in startup_info struct. Then "git grep" or any
>>  > commands can just read startup_info to find out the submodule prefix.
>>
>>
>> Here's my first naive attempt at implementing what you describe. Needs
>>  tests, more comments, sign-off etc.
> 
> Thanks.
> 
>>  One situation that could be handled better is when the cwd is a
>>  subdirectory of the specified worktree. At the moment this ends up
>>  giving the full path to the worktree, the output would look much nicer
>>  if it gave the relative path (e.g. ../../).
> 
> Hmm.. if cwd is inside a worktree, prefix (the 3rd parameter in
> cmd_grep) should be correctly set and "git grep" should also show
> relative path. Or are you talking about another command?

I was testing this with grep but also with my submodule changes. I should probably move this to its own topic branch and get it working then rebase my grep changes on top of it.

Show 29 quoted lines

>>
>>  ---8<---
>>  From: Chris Packham <judge.packham@gmail.com>
>>  Date: Thu, 30 Sep 2010 11:19:29 -0700
>>  Subject: [RFC PATCH] save the work tree prefix in startup_info
>>
>>  This is the relative path between the cwd and the worktree or the
>>  absolute path of the worktree if the worktree is not a subdirectory
>>  of the worktree.
>>  ---
>>   cache.h |    1 +
>>   dir.c   |   26 ++++++++++++++++++++++++++
>>   dir.h   |    1 +
>>   setup.c |    4 ++++
>>   4 files changed, 32 insertions(+), 0 deletions(-)
>>
>>  diff --git a/cache.h b/cache.h
>>  index e1d3ffd..f320e78 100644
>>  --- a/cache.h
>>  +++ b/cache.h
>>  @@ -1111,6 +1111,7 @@ const char *split_cmdline_strerror(int cmdline_errno);
>>   /* git.c */
>>   struct startup_info {
>>         int have_repository;
>>  +       const char *prefix;
> 
> You should use another name here to avoid confusion with the current
> prefix, relative to worktree toplevel directory. I'm thinking of
> outer_prefix or cwd_prefix, but I'm usually bad at naming.

I couldn't come up with a better name either, that’s why I used "prefix". "cwd_prefix" seems sensible enough to me.

Show 25 quoted lines

>>   };
>>   extern struct startup_info *startup_info;
>>
>>  diff --git a/dir.c b/dir.c
>>  index 58ec1a1..2148730 100644
>>  --- a/dir.c
>>  +++ b/dir.c
>>  @@ -1036,6 +1036,32 @@ char *get_relative_cwd(char *buffer, int size,
>>  const char *dir)
>>         }
>>   }
>>
>>  +char *get_relative_wt(char *buffer, int size, const char *dir)
>>  +{
>>  +       char *cwd = buffer;
>>  +
>>  +       if (!dir)
>>  +               return NULL;
>>  +       if (!getcwd(buffer, size))
>>  +               die_errno("can't find the current directory");
>>  +       if (!is_absolute_path(dir))
>>  +               dir = make_absolute_path(dir);
>>  +       if (strstr(dir, cwd)) {
> 
> Why not strncmp?

I actually tried strcmp first, expecting to get a number that I can increment dir by. What I actually got was -1, maybe I just screwed up the order of dir and cwd. I'll look into it.

Show 15 quoted lines

> 
>>  +               dir += strlen(cwd);
>>  +               switch(*dir){
>>  +               case '\0':
>>  +                       return NULL;
>>  +               case '/':
>>  +                       dir++;
>>  +                       break;
> 
> Yeah.
> 
>>  +               default:
>>  +                       break;
> 
> Should we properly handle relative path that includes ".." here too?

By now dir and cwd are both absolute paths. So I dn't think there is anything to handle

Show 6 quoted lines

>>  +               }
>>  +       }
>>  +       strncpy(buffer, dir, size);
> 
> So if "cwd" is inside "dir", an absolute "dir" is returned? That does
> not look like a prefix to me.

That is a problem. Maybe I should be returning NULL in that case and let the existing code handle the cwd inside dir case. I think if I wrote some tests first I could see the various permutations better.

Show 50 quoted lines

>>  +       return buffer;
>>  +}
>>  +
>>   int is_inside_dir(const char *dir)
>>   {
>>         char buffer[PATH_MAX];
>>  diff --git a/dir.h b/dir.h
>>  index b3e2104..d3c161f 100644
>>  --- a/dir.h
>>  +++ b/dir.h
>>  @@ -81,6 +81,7 @@ extern void add_exclude(const char *string, const char
>>  *base,
>>   extern int file_exists(const char *);
>>
>>   extern char *get_relative_cwd(char *buffer, int size, const char *dir);
>>  +extern char *get_relative_wt(char *buffer, int size, const char *dir);
>>   extern int is_inside_dir(const char *dir);
>>
>>   static inline int is_dot_or_dotdot(const char *name)
>>  diff --git a/setup.c b/setup.c
>>  index a3b76de..bd9d9fd 100644
>>  --- a/setup.c
>>  +++ b/setup.c
>>  @@ -317,6 +317,7 @@ static const char *setup_explicit_git_dir(const char
>>  *gitdirenv,
>>                                 const char *work_tree_env, int *nongit_ok)
>>   {
>>         static char buffer[1024 + 1];
>>  +       static char wtbuf[1024 + 1];
>>         const char *retval;
>>
>>         if (PATH_MAX - 40 < strlen(gitdirenv))
>>  @@ -337,6 +338,9 @@ static const char *setup_explicit_git_dir(const char
>>  *gitdirenv,
>>         }
>>         if (check_repository_format_gently(nongit_ok))
>>                 return NULL;
>>  +
>>  +       startup_info->prefix=get_relative_wt(wtbuf, sizeof(wtbuf) - 1,
>>  +                       get_git_work_tree());
>>         retval = get_relative_cwd(buffer, sizeof(buffer) - 1,
>>                         get_git_work_tree());
>>         if (!retval || !*retval)
>>
>> --
>>  1.7.3.1
>>
>>
> 
>