| Message ID | 20210423194230.1388945-25-lukeshu@lukeshu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | subtree: clean up, improve UX | expand |
On Fri, Apr 23, 2021 at 3:43 PM Luke Shumaker <lukeshu@lukeshu.com> wrote: > Currently, debug output (triggered by passing '-d') and progress output > stomp on eachother. The debug output is just streamed as lines to s/eachother/each other/ > stderr, and the progress output is sent to stderr as '%s\r'. It is > difficult to distinguish between the debug output and a progress line. > When writing to a terminal the debug lines hide progress lines. > > So, when '-d' has been passed, spit out progress as 'progress: %s\n', > instead of as '%s\r', so that it can be detected, and so that the debug > lines don't overwrite the progress when written to a terminal. Makes perfect sense when output is to a terminal, though might be annoying for the person who redirects stderr to a file. Just idly wondering if it makes sense to take that case into consideration... (but maybe it doesn't matter much when someone is working at debugging a problem). > Signed-off-by: Luke Shumaker <lukeshu@datawire.io> > --- > diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh > @@ -53,7 +53,12 @@ debug () { > progress () { > if test -z "$GIT_QUIET" > then > - printf "%s\r" "$*" >&2 > + if test -n "$arg_debug" > + then > + printf "progress: %s\n" "$*" >&2 > + else > + printf "%s\r" "$*" >&2 > + fi > fi > } Subjective (not necessarily worth changing): An `echo` would suffice in place of `printf "...\n"`: echo "progress: $*" >&2 It _might_ be worthwhile to have an in-code comment here explaining why progress() behaves differently in debug mode, especially if the reader is confused about why one case explicitly emits "progress:" and the other doesn't.
On Fri, 23 Apr 2021 15:07:12 -0600, Eric Sunshine wrote: > > On Fri, Apr 23, 2021 at 3:43 PM Luke Shumaker <lukeshu@lukeshu.com> wrote: > > Currently, debug output (triggered by passing '-d') and progress output > > stomp on eachother. The debug output is just streamed as lines to > > s/eachother/each other/ Ack. > > stderr, and the progress output is sent to stderr as '%s\r'. It is > > difficult to distinguish between the debug output and a progress line. > > When writing to a terminal the debug lines hide progress lines. > > > > So, when '-d' has been passed, spit out progress as 'progress: %s\n', > > instead of as '%s\r', so that it can be detected, and so that the debug > > lines don't overwrite the progress when written to a terminal. > > Makes perfect sense when output is to a terminal, though might be > annoying for the person who redirects stderr to a file. Just idly > wondering if it makes sense to take that case into consideration... > (but maybe it doesn't matter much when someone is working at debugging > a problem). The '%s\r' isn't really useful when written to a file, so this change is useful in the file case too. I'll add a comment and update the commit-message. > > Signed-off-by: Luke Shumaker <lukeshu@datawire.io> > > --- > > diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh > > @@ -53,7 +53,12 @@ debug () { > > progress () { > > if test -z "$GIT_QUIET" > > then > > - printf "%s\r" "$*" >&2 > > + if test -n "$arg_debug" > > + then > > + printf "progress: %s\n" "$*" >&2 > > + else > > + printf "%s\r" "$*" >&2 > > + fi > > fi > > } > > Subjective (not necessarily worth changing): An `echo` would suffice > in place of `printf "...\n"`: > > echo "progress: $*" >&2 echo would suffice, but I prefer the printf. > It _might_ be worthwhile to have an in-code comment here explaining > why progress() behaves differently in debug mode, especially if the > reader is confused about why one case explicitly emits "progress:" and > the other doesn't. Adding a comment is a good idea, I'll do that.
On Fri, Apr 23, 2021 at 8:44 PM Luke Shumaker <lukeshu@lukeshu.com> wrote: > On Fri, 23 Apr 2021 15:07:12 -0600, Eric Sunshine wrote: > > Makes perfect sense when output is to a terminal, though might be > > annoying for the person who redirects stderr to a file. Just idly > > wondering if it makes sense to take that case into consideration... > > (but maybe it doesn't matter much when someone is working at debugging > > a problem). > > The '%s\r' isn't really useful when written to a file, so this change > is useful in the file case too. I'll add a comment and update the > commit-message. Ah, right. I didn't pay close enough attention to really notice that the "\r"-terminated line was going to stderr, so ignore that bit in my review comment.
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index ddfa74c3bf..62cf54928e 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -53,7 +53,12 @@ debug () { progress () { if test -z "$GIT_QUIET" then - printf "%s\r" "$*" >&2 + if test -n "$arg_debug" + then + printf "progress: %s\n" "$*" >&2 + else + printf "%s\r" "$*" >&2 + fi fi }