| Message ID | 20190203083545.5877-1-martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | doc-diff: don't `cd_to_toplevel` before calling `usage` | expand |
On Sun, Feb 3, 2019 at 3:37 AM Martin Ågren <martin.agren@gmail.com> wrote: > `usage` tries to call $0, which might very well be "./doc-diff", so if > we `cd_to_toplevel` before calling `usage`, we'll end with an error to > the effect of "./doc-diff: not found" rather than a friendly `doc-diff > -h` output. Granted, all of these `usage` calls are in error paths, so > we're about to exit anyway, but the user experience of something like > `(cd Documentation && ./doc-diff)` could be a bit better than > "./doc-diff: not found". > > This regressed in ad51743007 ("doc-diff: add --clean mode to remove > temporary working gunk", 2018-08-31) where we moved the call to > `cd_to_toplevel` to much earlier. Move it back to where it was, and > teach the "--clean" code to cd on its own. This way, we only cd once > we've verified the arguments. Thanks for spotting this; I wasn't aware of it when crafting ad51743007. I wonder if a more fruitful, longer-term fix which would save us from having to worry about this in the future, would be to make git-sh-setup.sh remember the original $0 before cd_to_toplevel() and then employ the original value when usage() re-execs with the -h option. That would also avoid the slightly ugly repeated cd_to_top_level() and 'tmp' assignment in this patch. > Signed-off-by: Martin Ågren <martin.agren@gmail.com>
On Sun, Feb 3, 2019 at 4:08 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > I wonder if a more fruitful, longer-term fix which would save us from > having to worry about this in the future, would be to make > git-sh-setup.sh remember the original $0 before cd_to_toplevel() and > then employ the original value when usage() re-execs with the -h > option. That would also avoid the slightly ugly repeated > cd_to_top_level() and 'tmp' assignment in this patch. By "original $0", I meant a path which would be suitable for re-exec'ing (which wouldn't be the literal original $0). Sorry for the confusion.
On Sun, 3 Feb 2019 at 10:12, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Sun, Feb 3, 2019 at 4:08 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > I wonder if a more fruitful, longer-term fix which would save us from > > having to worry about this in the future, would be to make > > git-sh-setup.sh remember the original $0 before cd_to_toplevel() and > > then employ the original value when usage() re-execs with the -h > > option. That would also avoid the slightly ugly repeated > > cd_to_top_level() and 'tmp' assignment in this patch. > > By "original $0", I meant a path which would be suitable for > re-exec'ing (which wouldn't be the literal original $0). Sorry for the > confusion. I thought about this, and I probably should have said something about it in the commit message. My uneducated guess was that "all" other users are in $PATH and aren't being called like `./foobar`, but just `foobar`. Or, they're internal helpers where the caller has already done the grunt setup work, so their cd-ing is a no-op. I could be very wrong. To be honest, I wasn't very tempted to risk breaking git-sh-setup only(?) to help a development helper script like this. But let's see if I can spend some more time on this... The only way I'd know how to do something like this would be with readlink or realpath. Judging by d2addc3b96 ("t7800: readlink may not be available", 2016-05-31), we can't count on readlink. And I'd expect that commit to have switched to realpath if THAT were available everywhere. That commit instead goes for "ls | sed" and notes that "[t]his is no universal readlink replacement but works in the controlled test environment well enough." Ok, so I am not too eager to try and tackle this with fallback strategies and what-not. What would you say if I punted on this? I could add something like this to the commit message: A more general fix would be to teach git-sh-setup to save away the absolute path for $0 and then use that, instead. I'm not aware of any portable way of doing that, see, e.g., d2addc3b96 ("t7800: readlink may not be available", 2016-05-31), so let's just fix this user instead. What do you think? Thanks for your comments. Martin
On Sun, Feb 3, 2019 at 5:37 AM Martin Ågren <martin.agren@gmail.com> wrote: > On Sun, 3 Feb 2019 at 10:12, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Sun, Feb 3, 2019 at 4:08 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > > I wonder if a more fruitful, longer-term fix which would save us from > > > having to worry about this in the future, would be to make > > > git-sh-setup.sh remember the original $0 before cd_to_toplevel() and > > > then employ the original value when usage() re-execs with the -h > > > option. That would also avoid the slightly ugly repeated > > > cd_to_top_level() and 'tmp' assignment in this patch. > > > > By "original $0", I meant a path which would be suitable for > > re-exec'ing (which wouldn't be the literal original $0). Sorry for the > > confusion. > > Ok, so I am not too eager to try and tackle this with fallback > strategies and what-not. What would you say if I punted on this? I could > add something like this to the commit message: > > A more general fix would be to teach git-sh-setup to save away the > absolute path for $0 and then use that, instead. I'm not aware of any > portable way of doing that, see, e.g., d2addc3b96 ("t7800: readlink > may not be available", 2016-05-31), so let's just fix this user > instead. > > What do you think? Thanks for your comments. Punting and extending the commit message like that sounds reasonable.
diff --git a/Documentation/doc-diff b/Documentation/doc-diff index dfd9418778..f820febf8f 100755 --- a/Documentation/doc-diff +++ b/Documentation/doc-diff @@ -39,12 +39,11 @@ do shift done -cd_to_toplevel -tmp=Documentation/tmp-doc-diff - if test -n "$clean" then test $# -eq 0 || usage + cd_to_toplevel + tmp=Documentation/tmp-doc-diff git worktree remove --force "$tmp/worktree" 2>/dev/null rm -rf "$tmp" exit 0 @@ -66,6 +65,9 @@ to=$1; shift from_oid=$(git rev-parse --verify "$from") || exit 1 to_oid=$(git rev-parse --verify "$to") || exit 1 +cd_to_toplevel +tmp=Documentation/tmp-doc-diff + if test -n "$force" then rm -rf "$tmp"
`usage` tries to call $0, which might very well be "./doc-diff", so if we `cd_to_toplevel` before calling `usage`, we'll end with an error to the effect of "./doc-diff: not found" rather than a friendly `doc-diff -h` output. Granted, all of these `usage` calls are in error paths, so we're about to exit anyway, but the user experience of something like `(cd Documentation && ./doc-diff)` could be a bit better than "./doc-diff: not found". This regressed in ad51743007 ("doc-diff: add --clean mode to remove temporary working gunk", 2018-08-31) where we moved the call to `cd_to_toplevel` to much earlier. Move it back to where it was, and teach the "--clean" code to cd on its own. This way, we only cd once we've verified the arguments. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- Documentation/doc-diff | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)