tavianator/bfs

Improve bfs -help with LESS already set

tavianator opened this issue · 2 comments

tavianator commented

RE: @markus-oberhumer's #68

I think it makes sense to not override LESS, in case it's necessary for less to work properly in the user's environment. But I can think of a couple improvements that would make sense:

  • Override LESS if it's empty. Similar to the new PAGER handling. I think git ought to do the same thing.
  • If LESS is nonempty, don't colorize the help. This should at least make it readable for you. And if you want the colors, you could do LESS= bfs -help.

But I just thought of a complication: if $PAGER != less, it's weird for $LESS to affect the behaviour of bfs help. Thoughts?

markus-oberhumer commented

I think it makes sense to not override LESS, in case it's necessary for less to work properly in the user's environment. But I can think of a couple improvements that would make sense:

* Override `LESS` if it's empty.  Similar to the new `PAGER` handling.  I think `git` ought to do the same thing.

Yes, that makes sense. BTW, my code from #68 is buggy as mixes putenv with the explicit envp parameter - I can submit a new version if you want.

* If `LESS` is nonempty, don't colorize the help.  This should at least make it readable for you.  And if you want the colors, you could do `LESS= bfs -help`.

But I just thought of a complication: if $PAGER != less, it's weird for $LESS to affect the behaviour of bfs help. Thoughts?

After some more googling around I wouldn't touch that code - more works out of the box, and it's probably time for me to change my defaults to export LESS="-R -MM -i".

Thanks!

tavianator commented

Yes, that makes sense. BTW, my code from #68 is buggy as mixes putenv with the explicit envp parameter - I can submit a new version if you want.

Oh true. Better to just change how parse.c constructs the new environment I think. bfs_spawn_addputenv() has some other semantic issues like how it interacts with PATH (come to think of it, I think my spawn.c is already wrong about this on some platforms), and it would make #47 even more challenging.

So if you feel like submitting a patch that just fixes the empty LESS case, I'd take it and consider this issue closed.