Re: [PATCH 24/30] subtree: don't let debug and progress output clash

From: Luke Shumaker <lukeshu@lukeshu.com>
Date: Apr 24, 2021, 00:44 UTC
Message-ID: <87v98ca4uf.wl-lukeshu@lukeshu.com>
In-Reply-To: <CAPig+cRaQnZ7DWYdU+efNpMgxdniXNQdF43TSFiaj6ezLELC_g@mail.gmail.com>

On Fri, 23 Apr 2021 15:07:12 -0600, Eric Sunshine wrote:

Show 6 quoted lines

> 
> 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.

Show 13 quoted lines

> > 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.

Show 21 quoted lines

> > 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.

-- 
Happy hacking,
~ Luke Shumaker