patchprogress: use \r as EOL only if isatty(stderr) is true

So far progress always uses \r to produce one-line output on stderr. This only produces useful and easy parsable output if stderr is opened on a file which does interpret CR as a real carriage return operation. This patch changes EOL to the plain newline \n control if isatty() is false instead.

Signed-off-by: Steffen Daode Nurpmeso <sdaoden@gmail.com>
---
 progress.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

Steffen Daode Nurpmeso <sdaoden@googlemail.com> writes:

I kind of like this patch, in the sense that if there is a sane scenario to emit progress to non-tty, we should do just LF not CRLF, but I would like to know the real motivation behind this proposal.

I thought that we try to disable the progress pretty much everywhere when we are not talking to a tty, so ugliness coming from many CRLF appearing in the cron e-mail shouldn't be the issue.

@ Junio C Hamano <gitster@pobox.com> wrote (2011-06-28 20:33+0200):

If Tay Ray Chuan would have Cc'd the list, too, i would have thought he tried to draw attention onto the fact that this is questionable design, if you allow me this comment.

It would indeed be more logical to simply "peek" onto progress and let that decide on it's own wether it is useful or possible to print any progress indication or not.

-- Ciao, Steffen sdaoden(*)(gmail.com) () ascii ribbon campaign - against html e-mail /\ www.asciiribbon.org - against proprietary attachments

@ Junio C Hamano <gitster@pobox.com> wrote (2011-06-28 20:33+0200):

That is to say, at least some kind of progress_maybe_applicable() or the like, instead of having things like

    builtin/pack-objects.c: progress = isatty(2);

spread out in the code, and just see what 'git grep -F isatty' reports ...

-- Ciao, Steffen sdaoden(*)(gmail.com) () ascii ribbon campaign - against html e-mail /\ www.asciiribbon.org - against proprietary attachments

On Tue, Jun 28, 2011 at 11:33:48AM -0700, Junio C Hamano wrote:

We certainly do try to turn off progress reporting when stderr isn't a tty. So unless "--progress" is being given explicitly, seeing it is a bug that should be fixed.

I'm not sure dropping the CR is a good thing, though. One of the uses for forcing output to a non-terminal via "--progress", is that something _else_ is going to parse the output. And that other thing gets useful information from the carriage returns.

For example, you may be piping into a file or a fifo and running 'tail' to a terminal on the other end. You want the CR because we are ultimately going to a terminal.

Another example: you write a GUI wrapper around git that captures and parses stderr. You show progress and informative messages in a running dialog. The difference between CR and LF is important. The former means "clear the progress line and show this new one instead"; the latter means "keep this on the screen and show more lines".

I'm willing to accept that there are use cases where you don't want the CRs, but just want a list of lines[1]. But it seems like this change hurts some existing use cases.

-Peff

[1] Actually, I would be curious to see such a use case. If you are planning on saving the output, is it really useful to have a hundred lines saying:

  Compressing objects 1% (100/10000)
  Compressing objects 2% (200/10000)

and so forth?

Jeff King <peff@peff.net> writes:

All valid arguments against the change, I think.  Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Yeah, I think better to leave it alone.

I sometimes pipe the output of such progress-meter-using programs through a filter that both sends the output to the terminal (where the \rs are clearly desired) and logs it to a file -- and the \rs are even useful for the latter purpose, and the filter can use them to delete all but the last copy of each line before logging.

-Miles

I desperately tried to reproduce the 'xy  ^Mxy  ^M' case, but i
can't.  In fact i cannot even find a piece of code which would do
it.  :-/  Such a shame.
Still: i insist on having that seen in my arena-manager.log.

While searching around i stumbled over fetch-pack output with an ANSI escape sequence after a '(xy) 2>&1 | tee LOG'. -- Ciao, Steffen sdaoden(*)(gmail.com) () ascii ribbon campaign - against html e-mail /\ www.asciiribbon.org - against proprietary attachments

-- >8 --
Subject: [PATCH/RFC] sideband: remove line padding

For formatting purposes recv_sideband() sofar appended a suffix to band #2 (informative) messages: dependent on getenv("TERM") lines may have been space-filled ("dumb") or padded with an ANSI escape sequence (ANSI EL, mode 0: clear from cursor to end of line). This patch removes handling of terminal specifics and any pad suffixes.

It also fixes two 'signed/unsigned comparison' compiler warnings.

Signed-off-by: Steffen Daode Nurpmeso <sdaoden@gmail.com>
---
I believe that users of fetch-pack, send-pack and archive, the three
affected operations, will ensure that lines are refreshed as approbiate,
even if 'brk-1' == CR.  But even if not: i would move handling of
"TERM" and dumb/non-dumb terminals out of this packet handler and
into some terminal encapsulator, which can only be color.* as far
as i see.  The question would be how this should be done.  Maybe
term_fprintf_padln(?, desired-line-length-or--1)?  Such a thing would
surely find other users in the codebase.  A second approach would
be to only use the "dumb" pad, as in

    fprintf(stderr, "%.*s        %c", brk - 1, b, b[brk - 1]);

What do you think of that?

 sideband.c |   35 +++++------------------------------
 1 files changed, 5 insertions(+), 30 deletions(-)

On Wed, 29 Jun 2011, Steffen Daode Nurpmeso wrote:

Why?

Nicolas

@ Nicolas Pitre <nico@fluxnic.net> wrote (2011-06-29 20:15+0200):

Ok, they don't. (Your initial patch was from 2008, and since git(1) is evolving very fast, it might have been that padding is no longer of any use at all.) So, then, i'm the right person to turn the big wheel:

- Move color.* and even progress.* stuff into new visual.[hc].
- Add a vis[ual]_init() which does check isatty() for [012] and
  does the TERM check (i.e. "dumb" or otherwise).
  There could also be flags which could be used to restrict what
  is checked ...
- Replace calls to isatty(xy) by std{in,out,err}_is_tty(), defined
  in visual.h.
  Calling vis[ual]_init() would not be needed for these.
  (But they could be inline if it would be.)
- Add a series of vis_print functions in equal spirit to the
  color series which use detected terminal capabilities to ensure
  that a line consists only of the printed data.

I (almost) had that state hour ago, but it's ... (The problem with that print series is that in the dumb case the data is to be injected before a possibly contained NL/CR sequence, so that already inspected data is to be reinspected. Or data has to be copied around. Or writes have to be splitted, but here i'm talking about stderr, and that's unbuffered, and so each invocation goes through the OS.)

So what else could be done to remove TERM and ANSI escape sequence knowledge from a nice multiband stream splitter, and to avoid that it writes the escape sequence if output is stupidly redirected to a file? Well, visual.h could consist of a single function only:

    const char *ansi_el_if_tty_and_termok_else_null(void);
--
Ciao, Steffen
sdaoden(*)(gmail.com)
() ascii ribbon campaign - against html e-mail
/\ www.asciiribbon.org - against proprietary attachments

On Thu, 30 Jun 2011, Steffen Daode Nurpmeso wrote:

But they absolutely still are. What makes you think they aren't anymore?

I'm afraid you'll have to understand _why_ that padding is still needed first. Luckily the various commit messages for sideband.c are well detailed on the reasons for the current code.

Nicolas

(Original subject was:
Re: [PATCH] progress: use \r as EOL only if isatty(stderr) is true)

@ Junio C Hamano <gitster@pobox.com> wrote (2011-06-28 20:33+0200):

Today i got this by accident:

    ================================================================
    openbsd-src.git-null: performing reduce
    Checking out files:  17% (11410/65508)   ^MChecking out files:
    18% (11792/65508)   ^M [..]

This is the output of my dumb arena-manager, which i append after the first scissor for reference. (Maybe someone finds it useful. It's a dumb sh(1) thing, i don't do shell scripting that often.) I haven't actually tried it yet, but setting .quiet for unpack_trees may stop this output. (Compilation succeeds, though.) I still and *truly* have no idea of the git(1) internals, so i may oversee .. just *any* thinkable side-effect. Hope this helps a bit. (Actual patch as response. Have a nice weekend.)

--Steffen
Ciao, sdaoden(*)(gmail.com)
ASCII ribbon campaign           ( ) More nuclear fission plants
  against HTML e-mail            X    can serve more coloured
    and proprietary attachments / \     and sounding animations
-- >8 --
#!/bin/bash
# NOTE: this acts according to extensions, e.g.:
#   docutils.svn-git-null: git svn
#   groff.cvs-git-null   : git cvsimport
#   openbsd-src.git-null : git
#   vim.hg-null          : Mercurial
# If a '-null' is in the suffix one may use 'reduce' and 'expand'
# modes; for git(1) these modes require an empty NULL branch:
#   $ git co --orphan NULL && git rm -rf '*' &&
#     echo DEFAULT-BRANCH >NULL && git add NULL && git ci -m NULL
# Mercurial has builtin support for 'null'.  (Nice for backups.)

# Top dir where everything happens ARENA="$HOME/arena/code.extern.repos"

##

GIT=git HG=hg SVN=svn CVS='cvs -fz 9 update -ACRPd' ESTAT=0 LOGFILE=lastlog.txt CVSROOT=/nonexistent CURR=/nonexistent

log() {
    echo "$*"
    echo "$*" >> $LOGFILE
}
logerr() {
    echo >&2 "ERROR: $*"
    echo "ERROR: $*" >> $LOGFILE
    ESTAT=1
}

SEP='================================================================'
intro() {
    log ''
    log $SEP
    log $1
}
final() {
    local es=$?
    if test $es -eq 0; then
        log "... ok: $1"
    else
        logerr "$1"
        ESTAT=$es
    fi
    log $SEP
    log ''
}

if test "$BASH" = x""; then
    echo >&2 "This script needs the GNU bash shell interpreter"
    exit 1
fi
cd $ARENA || {
    echo >&2 "Failed to chdir to $ARENA"
    exit 1
}
rm -rf $LOGFILE || {
    echo >&2 "Failed to remove stale $LOGFILE"
    exit 1
}

MODE="$1" shift PARAMS="$@" set -u test $# -ne 0 || PARAMS=$(echo *.*)

# Perform basename cleanup and move over to $params[]
log "$0: script startup, mode $MODE"
declare -a params
for rd in $PARAMS; do
    rd=$(echo "$rd" | sed -Ee 's/(.+)\/+$/\1/' -e 's/.*\/([^/]+)$/\1/')
    params[${#params[*]}]="$rd"
done

GITDID=
case "$MODE" in
reduce|expand)
    if test "$MODE" == 'reduce'; then
        git_branch='NULL'
        hg_branch='null'
    else
        git_branch=
        hg_branch=''
    fi

    for rd in ${params[@]}; do
        if test "$rd" == "${rd/null/}"; then
            log "[$rd: $MODE does not apply]"
            continue
        fi

        intro "$rd: performing $MODE"
        set -o pipefail
        (   cd "$rd" || exit 1
            if test "$rd" != "${rd/git-null}"; then
                # On branch NULL file NULL contains master branch's name
                if test -z "$git_branch"; then
                    if test -f NULL; then
                        git_branch=$(<NULL)
                    else
                        echo >&2 "No file NULL in $rd"
                        git_branch=master
                    fi
                fi
                $GIT checkout $git_branch
            else
                $HG up $hg_branch
            fi
            exit $?
        ) 2>&1 | tee -a "$LOGFILE"
        final "$rd"
        set +o pipefail
    done
    ;;
update)
    for rd in ${params[@]}; do
        intro "$rd: performing $MODE"
        set -o pipefail
        (   cd "$rd" || exit 1
            if test "$rd" != "${rd/.git}"; then
                #$GIT pull -v --ff-only --stat --prune
                $GIT fetch --verbose --prune
                GITDID=1
            elif test "$rd" != "${rd/.svn-git}"; then
                $GIT svn rebase
                GITDID=1
            elif test "$rd" != "${rd/.cvs-git}"; then
                ldir='.git/.cvsps'
                tar xjf "$ldir.tbz" || {
                    echo >&2 "$rd: bail: tar xjf $ldir.tbz"
                    exit 1
                }
                hdir="$HOME/.cvsps"
                test -d "$hdir" || mkdir "$hdir" || {
                    echo >&2 "$rd: failed to create $hdir directory"
                    exit 2
                }
                root="$ldir/CVSROOT"
                repo="$ldir/MODULE"
                cache=$(<$ldir/CVSPS_FILE)
                mv -f "$ldir/cvs-revisions" .git/
                mv -f "$ldir/$cache" "$hdir/$cache"
                $GIT cvsimport -aR -r origin -p '-u,--cvs-direct' \
                     -d $(<$root) $(<$repo)
                es=$?
                GITDID=1
                mv -f .git/cvs-revisions "$ldir/"
                mv -f "$hdir/$cache" "$ldir/$cache"
                tar cjf "$ldir.tbz" "$ldir" || {
                    echo >&2 "$rd: bail: tar cjf $ldir.tbz $ldir"
                    exit 3
                }
                rm -rf $ldir
                exit $es
            elif test "$rd" != "${rd/.hg}"; then
                $HG -v pull #-u
            elif test "$rd" != "${rd/.svn}"; then
                $SVN update
            elif test "$rd" != "${rd/.cvs}"; then
                $CVS
            else
                echo "Unknown revision-control-system: $rd"
                exit 1
            fi
        ) 2>&1 | tee -a "$LOGFILE"
        final "$rd"
        set +o pipefail
    done
    ;;
fullgc|gc)
    gct=
    test "$MODE" == fullgc && gct=--aggressive
    for rd in ${params[@]}; do
        intro "$rd: performing $MODE"
        set -o pipefail
        (   cd "$rd" || exit 1
            test "$rd" != "${rd/.git}" && git gc $gct
        ) 2>&1 | tee -a "$LOGFILE"
        final "$rd"
        set +o pipefail
    done
    ;;
*)
    echo 'USAGE: manager reduce|expand|update|gc|fullgc LIST-OF-DIRECTORIES'
    exit 1
    ;;
esac
test x"$GITDID" != x && log 'git(1) fetched data - do arena-manager [full]gc ..'

test $ESTAT -ne 0 && log 'Errors occurred!' exit $ESTAT # vim:set fenc=utf-8 filetype=sh syntax=sh ts=4 sts=4 sw=4 et tw=79:

Signed-off-by: Steffen Daode Nurpmeso <sdaoden@gmail.com>
---
 builtin/checkout.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

Steffen Daode Nurpmeso <sdaoden@googlemail.com> writes:

Justification is necessary why this is a good change.

Perhaps you meant something like this:

	In general, the progress output should not be given unless the
        output is sent to a tty (i.e. an interactive session).

But this patch may be squelching the output a bit too much. The opts.quiet field is used not just to set verbose_update in the unpack_trees_options used in reset_tree() and merge_working_tree(), but also used to report the local changes at the end of merge_working_tree(), report tracking information, and report where the detached HEAD is at, among other things.

Independently, it might make sense to squelch advice messages in a non-interactive session, but I think that should probably be done by flipping advice_* variables in advice.c, I think.

also sprach Junio C Hamano <gitster@pobox.com> [2011.08.28.0822 +0200]:

Just as a note — around Unix, it's generally "output should not be given unless there was an unexpected condition, or --verbose was passed. If a tool did successfully what it was asked to do, it should just be quiet about it."

This commit adds support for the -v/--verbose pair of options, and thus offers the possibility to be more specific in deciding which purely informational feedback message is displayed or not.

Without any of --verbose and --quiet involved, the progress reporting is now restricted to interactive sessions, i.e. only shown if the output is send to a terminal.

Analyzed-by: Junio C Hamano <gitster@pobox.com>
Inspired-by: martin f krafft <madduck@madduck.net>
Signed-off-by: Steffen Daode Nurpmeso <sdaoden@gmail.com>
---

Well i was stepping down from my hill, actually singing my sunday's song (was it Elvis..), but i didn't dare to implement the behaviour Martin suggested. But isn't he right? This thing here was also tested a bit.

 Documentation/git-checkout.txt |   13 +++++++++----
 builtin/checkout.c             |   12 +++++++++---
 2 files changed, 18 insertions(+), 7 deletions(-)

From: Steffen Daode Nurpmeso <sdaoden@gmail.com>

Whereas only syntactic sugar, i think it's a bit odd that during a checkout which mostly removes files the git-checkout progress displays "Checking out files" all of the time.

There are two solutions to this: either simply change that string to "Updating working tree", which is acceptable to anybody at anytime. Or divert what progress reports to the actual action which is currently performed. This series tries to achieve the latter.

But maybe progress should instead be extended that it could handle a situation like the following directly. I.e., offer an additional start_parted_progress() series, or extend the current one with an additional "signed parted" argument. That update_progress() could then instead be named progress_change_part() or whatever. ?

    Updating work tree: removing files (x/y [xy%]) total%
       Main-Title        Action-title  Action-cnt 

Steffen Daode Nurpmeso (2):
  progress: add update_progress()
  unpack-trees: divert check_updates() output via update_progress()

 progress.c     |   15 +++++++++++++++
 progress.h     |    2 ++
 unpack-trees.c |   30 +++++++++++++++++++++++-------
 3 files changed, 40 insertions(+), 7 deletions(-)

From: Steffen Daode Nurpmeso <sdaoden@gmail.com>

Sometimes the task which is tracked via progress is splitted into two parts, e.g. check_updates() in unpack_trees.c updates the working tree by first removing files, followed by checking out files. Whereas it is possible to simply recreate a progress reporter, it's easier to simply call in to update the state of the yet existing one.

Inspired-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Steffen Daode Nurpmeso <sdaoden@gmail.com>
---
 progress.c |   15 +++++++++++++++
 progress.h |    2 ++
 2 files changed, 17 insertions(+), 0 deletions(-)

From: Steffen Daode Nurpmeso <sdaoden@gmail.com>

The progress shown by check_updates() yet always printed "Checking out files", even if basically files were only unlinked.

This commit diverts that into "Updating working tree:" plus the actual action which currently is performed (i.e. "removing files" or "checking out files").

Inspired-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Steffen Daode Nurpmeso <sdaoden@gmail.com>
---
 unpack-trees.c |   30 +++++++++++++++++++++++-------
 1 files changed, 23 insertions(+), 7 deletions(-)

@ Tay Ray Chuan <rctay89@gmail.com> wrote (2011-06-28 18:27+0200):

I dunno and have no overview of the git(1) codebase. I have a "arena/code.extern.repos/" directory tree and use a shell script which manages it (e.g. "$ arena-manager update" to pull all repos). The output of this script is (also) redirected into a log file via tee(1), and for git(1) invocations the resulting output is not very useful.

I agree that it is maybe silly to not use --no-progress from within the script (the script comes from cvs(1) background). Maybe i should change it to not use git porcelain but directly script the plumbing - :-) -, but i am *very* new to git(1) and in the meanwhile this simple patch pimps up my log output.

-- Ciao, Steffen sdaoden(*)(gmail.com) () ascii ribbon campaign - against html e-mail /\ www.asciiribbon.org - against proprietary attachments

@ Tay Ray Chuan <rctay89@gmail.com> wrote (2011-06-28 18:27+0200):

Ok, in the meanwhile i have understood what you ment, but unfortunately i have thrown away the logfile which caused me to write the patch, and even more unfortunately i could not reproduce in the last hour or what any 'xy ^Mxy ^M' dump with the current state of the repos. A short 'git grep -F progress' is useless from my current point of view (and with the amount of my free time). The script contains these git(1) related commands anyway:

    git pull -v --ff-only --stat --prune && git gc
    git svn rebase [not at the time of the log: && git gc]

But that is true: i have had a log file with the mentioned content.

-- Ciao, Steffen sdaoden(*)(gmail.com) () ascii ribbon campaign - against html e-mail /\ www.asciiribbon.org - against proprietary attachments