Re: [PATCH 4/9] Make setup_work_tree() return new prefix

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Feb 28, 2008, 13:02 UTC
Message-ID: <fcaeb9bf0802280502m2cfc967bt792be98d81a2996@mail.gmail.com>
In-Reply-To: <alpine.LSU.1.00.0802281130140.22527@racer.site>

On Thu, Feb 28, 2008 at 6:30 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 44 quoted lines

> Hi,
>
>
>
>  On Wed, 27 Feb 2008, Nguyễn Thái Ngọc Duy wrote:
>
>  > @@ -336,24 +321,21 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  >                       static char buffer[1024 + 1];
>  >                       const char *retval;
>  >
>  > +                     /*
>  > +                      * The old behaviour (which we retain here) is to set
>  > +                      * the work tree root to the cwd, unless overridden by
>  > +                      * the config, the command line, or GIT_WORK_TREE.
>  > +                      */
>  >                       if (!work_tree_env) {
>  > -                             retval = set_work_tree(gitdirenv);
>  > -                             /* config may override worktree */
>  > -                             if (check_repository_format_gently(nongit_ok))
>  > -                                     return NULL;
>  > -                             return retval;
>  > +                             char buffer[PATH_MAX + 1];
>  > +
>  > +                             if (!getcwd(buffer, sizeof(buffer)))
>  > +                                     die ("Could not get the current working directory");
>  > +                             git_work_tree_cfg = xstrdup(buffer);
>  > +                             inside_work_tree = 1;
>  >                       }
>  > -                     if (check_repository_format_gently(nongit_ok))
>  > -                             return NULL;
>  > -                     retval = get_relative_cwd(buffer, sizeof(buffer) - 1,
>  > -                                     get_git_work_tree());
>  > -                     if (!retval || !*retval)
>  > -                             return NULL;
>  > -                     set_git_dir(make_absolute_path(gitdirenv));
>  > -                     if (chdir(work_tree_env) < 0)
>  > -                             die ("Could not chdir to %s", work_tree_env);
>  > -                     strcat(buffer, "/");
>  > -                     return retval;
>  > +                     check_repository_format_gently(nongit_ok);
>  > +                     return NULL;
>
>  What about the situation where you are in a subdirectory of core.worktree?
>  You return prefix NULL?  That's wrong.

This is an intermediate patch. At the end of the series, you will see that _gently() does not return prefix. I split it for easier review.

>  I am sorry, but with all these intrusive changes, I get a very uneasy
>  feeling.  As uneasy as with the original series, which I tried to fix up,
>  not really succeeding.

In constrast I feel good :) To me setup_git_directory*() at the end look really nice. No chdir() all over the place (temporary chdir() for detecting .git does not count). It also gives a chance to get rid of worktree to commands that do not care about worktree at all.

>  Ciao,
>  Dscho
>

-- 
Duy