Re: [PATCH] rev-parse: Fix shell scripts whose cwd is a symlink into a git work-dir

From: Marcel M. Cary <marcel@oak.homeunix.org>
Date: Nov 25, 2008, 05:17 UTC
Message-ID: <492B8A64.7070309@oak.homeunix.org>
In-Reply-To: <m31vx3l94x.fsf@localhost.localdomain>

Show 10 quoted lines

> But that is contrary to the _name_ of option. It is --show-cdup, as
> in "show cd up". And I think your change will break a few scripts.
> 
> I think you should use "git rev-parse --work-tree" for full path
> to working directory:
> 
>     --show-cdup
>         When the command is invoked from a subdirectory, show the path
>         of the top-level directory relative to the current directory
>         (typically a sequence of "../", or an empty string).

Jakub,

Yes, I agree, there is a risk in breaking scripts that rely on the "../" format. That's the "substantial change" I was alluding. Here's how I arrived at that choice. I considered these fixes:

(a) add some shell code to cd_to_toplevel to find the canonical pwd and
    interpret --show-cdup output from there

(b) make a new option (--work-tree would be a good name) to print the
    canonical work tree path, and leave --show-cdup as it is.  Then
    change cd_to_toplevel and/or git pull to use the new option

(c) change --show-cdup to print the canonical work tree path, even
    though it's not entirely consistent with the name of the option

The main reason I avoided (a), even though that "cd" is what violated my expecations, is because I didn't want to have to re-implement code to check whether each path component is a symlink. (Now I see that "cd `/bin/pwd` might be a more concise fix.)

The reason I avoided (b) is because, to make all of git work for me, I expected to have to change several calling scripts. (Now that I look, I see only three calls to --show-cdup in the git codebase to change.) Even so, third-party scripts that I might want to use in the future would not immediately be changed.

Option (c) keeps the change small and isolated and makes it effective everywhere. The documentation, while perhaps in need of update given my patch, doesn't promise to always return zero or more "../". Also, there's a branch branch of the --show-cdup code (that I can't seem to exercise) that the result of get_git_work_tree(), which might not be zero or more "../".

Would you suggest pursuing option (a)? I wonder whether there are languages other than shell that might suffer from the same problem of keeping an internal PWD variable of some sort, or perhaps there are shell scripts out there that call --show-cdup directly instead of calling cd_to_toplevel.

Do you think it would be less likely to break existing scripts if I restrict the (c) behavior to when getenv("PWD") doesn't match the starting getcwd()? (I'm not sure yet whether that's a reliable way to detect the symlink scenario, but it seems to work with bash.)

Marcel