On Mon, Jul 22, 2024 at 11:18:00AM +0100, Phillip Wood wrote: > > That's why I thought, aligned with what we are already doing in > > `wait_for_pager_at_exit()`, that this is a sensible approach: > > That extra information is important. When I said [1] > > > Isn't it a bug to call this with old_fd1 == -1 or have I missed > > something? > > What I'd missed was that we can return early without executing anything. Yep, missing that old optimization is easy ;) > We cannot do > > if (!git_pager(asatty(1)) > return > > at the beginning of wait_for_pager() because if we're running a pager > isatty(1) will return false so I think the old_fd as you suggested is the > easiest fix. Now that we agree, I'll do it :) > The existing callers do not need to know if setup_pager() > applied the "cat" optimization because they only setup the pager once. For > "add -p" this no-longer applies so we should think about returning a flag to > say "there was an error"/"there is no pager or the pager is 'cat'"/"the > pager has been started" I'm not sure it would be valuable for us to make the caller aware that "there is no pager or the pager is 'cat' ... just use stdout". However, I do agree that probably in the future, if we finally add the "|[cmd]" command, we'll need to return some kind of error instead of `die()`, in setup_pager().
