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

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

[Steffen Daode Nurpmeso]

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(-)

diff --git a/progress.c b/progress.c
index 3971f49..c548de4 100644
--- a/progress.c
+++ b/progress.c
@@ -27,6 +27,7 @@ struct throughput {
 
 struct progress {
 	const char *title;
+	const char *eol;
 	int last_value;
 	unsigned total;
 	unsigned last_percent;
@@ -90,7 +91,7 @@ static int display(struct progress *progress, unsigned n, const char *done)
 
 	progress->last_value = n;
 	tp = (progress->throughput) ? progress->throughput->display : "";
-	eol = done ? done : "   \r";
+	eol = done ? done : progress->eol;
 	if (progress->total) {
 		unsigned percent = n * 100 / progress->total;
 		if (percent != progress->last_percent || progress_update) {
@@ -219,6 +220,7 @@ struct progress *start_progress_delay(const char *title, unsigned total,
 		return NULL;
 	}
 	progress->title = title;
+	progress->eol = isatty(fileno(stderr)) ? "   \r" : "\n";
 	progress->total = total;
 	progress->last_value = -1;
 	progress->last_percent = -1;
-- 
1.7.6.rc0

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

[Steffen Daode Nurpmeso]

@ Tay Ray Chuan <rctay89@gmail.com> wrote (2011-06-28 18:27+0200):
> hmm, shouldn't the onus of checking tty fall on to callers of
> progress* functions?

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

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

[Steffen Daode Nurpmeso]

@ Tay Ray Chuan <rctay89@gmail.com> wrote (2011-06-28 18:27+0200):
> hmm, shouldn't the onus of checking tty fall on to callers of
> progress* functions?

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

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

[Junio C Hamano]

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

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

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.

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

[Steffen Daode Nurpmeso]

@ Junio C Hamano <gitster@pobox.com> wrote (2011-06-28 20:33+0200):
> [..]
> I thought that we try to disable the progress pretty much
> everywhere when we are not talking to a tty[..]

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

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

[Steffen Daode Nurpmeso]

@ Junio C Hamano <gitster@pobox.com> wrote (2011-06-28 20:33+0200):
> [..]
> I thought that we try to disable the progress pretty much
> everywhere when we are not talking to a tty[..]

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

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

[Jeff King]

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

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

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?

[PATCH/RFC] sideband: remove line padding (was: Re: [PATCH] progress: use \r as EOL only if isatty(stderr) is true)

[Steffen Daode Nurpmeso]

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(-)

diff --git a/sideband.c b/sideband.c
index d5ffa1c..17e1793 100644
--- a/sideband.c
+++ b/sideband.c
@@ -14,26 +14,13 @@
 
 #define PREFIX "remote:"
 
-#define ANSI_SUFFIX "\033[K"
-#define DUMB_SUFFIX "        "
-
-#define FIX_SIZE 10  /* large enough for any of the above */
-
 int recv_sideband(const char *me, int in_stream, int out)
 {
-	unsigned pf = strlen(PREFIX);
-	unsigned sf;
-	char buf[LARGE_PACKET_MAX + 2*FIX_SIZE];
-	char *suffix, *term;
+	unsigned pf = sizeof(PREFIX) - 1;
+	char buf[LARGE_PACKET_MAX + 2*sizeof(PREFIX)];
 	int skip_pf = 0;
 
 	memcpy(buf, PREFIX, pf);
-	term = getenv("TERM");
-	if (term && strcmp(term, "dumb"))
-		suffix = ANSI_SUFFIX;
-	else
-		suffix = DUMB_SUFFIX;
-	sf = strlen(suffix);
 
 	while (1) {
 		int band, len;
@@ -82,20 +69,8 @@ int recv_sideband(const char *me, int in_stream, int out)
 						break;
 				}
 
-				/*
-				 * Let's insert a suffix to clear the end
-				 * of the screen line if a line break was
-				 * found.  Also, if we don't skip the
-				 * prefix, then a non-empty string must be
-				 * present too.
-				 */
-				if (brk > (skip_pf ? 0 : (pf+1 + 1))) {
-					char save[FIX_SIZE];
-					memcpy(save, b + brk, sf);
-					b[brk + sf - 1] = b[brk - 1];
-					memcpy(b + brk - 1, suffix, sf);
-					fprintf(stderr, "%.*s", brk + sf, b);
-					memcpy(b + brk, save, sf);
+				if ((unsigned)brk > (skip_pf ? 0 : (pf+1 +1))) {
+					fprintf(stderr, "%.*s", brk, b);
 					len -= brk;
 				} else {
 					int l = brk ? brk : len;
@@ -133,7 +108,7 @@ ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet
 		char hdr[5];
 
 		n = sz;
-		if (packet_max - 5 < n)
+		if ((unsigned)packet_max - 5 < n)
 			n = packet_max - 5;
 		if (0 <= band) {
 			sprintf(hdr, "%04x", n + 5);
-- 
1.7.6.1.ge79e.dirty

Re: [PATCH/RFC] sideband: remove line padding (was: Re: [PATCH] progress: use \r as EOL only if isatty(stderr) is true)

[Nicolas Pitre]

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

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

Why?


Nicolas

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

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

[Miles Bader]

Junio C Hamano <gitster@pobox.com> writes:
> All valid arguments against the change, I think.  Thanks.

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

-- 
My books focus on timeless truths.  -- Donald Knuth

Re: [PATCH/RFC] sideband: remove line padding (was: Re: [PATCH] progress: use \r as EOL only if isatty(stderr) is true)

[Steffen Daode Nurpmeso]

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

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

Re: [PATCH/RFC] sideband: remove line padding (was: Re: [PATCH] progress: use \r as EOL only if isatty(stderr) is true)

[Nicolas Pitre]

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

> @ Nicolas Pitre <nico@fluxnic.net> wrote (2011-06-29 20:15+0200):
> > Why?
> 
> 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.)

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

> So, then, i'm the right person to turn the big wheel:

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

[PATCH] checkout: be quiet if not on isatty()

[Steffen Daode Nurpmeso]

(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):
> [..]
> I thought that we try to disable the progress pretty much
> everywhere when we are not talking to a tty[..]

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:

[PATCH] checkout: be quiet if not on isatty()

[Steffen Daode Nurpmeso]

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

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 4eaedff..6fb6d48 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -958,6 +958,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	gitmodules_config();
 	git_config(git_checkout_config, &opts);
 
+	opts.quiet = !isatty(2);
 	opts.track = BRANCH_TRACK_UNSPECIFIED;
 
 	argc = parse_options(argc, argv, prefix, options, checkout_usage,
-- 
1.7.6.537.ga80e5.dirty

Re: [PATCH] checkout: be quiet if not on isatty()

[Junio C Hamano]

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

> Signed-off-by: Steffen Daode Nurpmeso <sdaoden@gmail.com>

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.

> ---
>  builtin/checkout.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 4eaedff..6fb6d48 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -958,6 +958,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  	gitmodules_config();
>  	git_config(git_checkout_config, &opts);
>  
> +	opts.quiet = !isatty(2);
>  	opts.track = BRANCH_TRACK_UNSPECIFIED;
>  
>  	argc = parse_options(argc, argv, prefix, options, checkout_usage,

Re: [PATCH] checkout: be quiet if not on isatty()

[martin f krafft]

[-- Attachment #1: Type: text/plain, Size: 691 bytes --]

also sprach Junio C Hamano <gitster@pobox.com> [2011.08.28.0822 +0200]:
> 	In general, the progress output should not be given unless the
>         output is sent to a tty (i.e. an interactive session).

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

-- 
martin | http://madduck.net/ | http://two.sentenc.es/
 
/.ing an issue is like asking an infinite number of monkeys for advice
                                                   -- in #debian-devel
 
spamtraps: madduck.bogus@madduck.net

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/sig-policy/999bbcc4/current) --]
[-- Type: application/pgp-signature, Size: 1124 bytes --]

[PATCH] checkout: add --verbose, and restrict progress reporting (was: Re: [PATCH] checkout: be quiet if not on isatty())

[Steffen Daode Nurpmeso]

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(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index c0a96e6..77ad4f3 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -8,9 +8,9 @@ git-checkout - Checkout a branch or paths to the working tree
 SYNOPSIS
 --------
 [verse]
-'git checkout' [-q] [-f] [-m] [<branch>]
-'git checkout' [-q] [-f] [-m] [--detach] [<commit>]
-'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] <new_branch>] [<start_point>]
+'git checkout' [-v] [-q] [-f] [-m] [<branch>]
+'git checkout' [-v] [-q] [-f] [-m] [--detach] [<commit>]
+'git checkout' [-v] [-q] [-f] [-m] [[-b|-B|--orphan] <new_branch>] [<start_point>]
 'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <paths>...
 'git checkout' [-p|--patch] [<tree-ish>] [--] [<paths>...]
 
@@ -66,9 +66,14 @@ file can be discarded to re-create the original conflicted merge result.
 
 OPTIONS
 -------
+-v::
+--verbose::
+	Be verbose, force progress reporting.
+
 -q::
 --quiet::
-	Quiet, suppress feedback messages.
+	Be quiet, suppress feedback messages and progress reporting.
+	Overrides "--verbose", if given.
 
 -f::
 --force::
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 4eaedff..7297843 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -27,6 +27,7 @@ static const char * const checkout_usage[] = {
 };
 
 struct checkout_opts {
+	int verbose;
 	int quiet;
 	int merge;
 	int force;
@@ -325,7 +326,7 @@ static int reset_tree(struct tree *tree, struct checkout_opts *o, int worktree)
 	opts.reset = 1;
 	opts.merge = 1;
 	opts.fn = oneway_merge;
-	opts.verbose_update = !o->quiet;
+	opts.verbose_update = o->verbose;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
 	parse_tree(tree);
@@ -402,7 +403,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 		topts.update = 1;
 		topts.merge = 1;
 		topts.gently = opts->merge && old->commit;
-		topts.verbose_update = !opts->quiet;
+		topts.verbose_update = opts->verbose;
 		topts.fn = twoway_merge;
 		topts.dir = xcalloc(1, sizeof(*topts.dir));
 		topts.dir->flags |= DIR_SHOW_IGNORED;
@@ -927,7 +928,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	int patch_mode = 0;
 	int dwim_new_local_branch = 1;
 	struct option options[] = {
-		OPT__QUIET(&opts.quiet, "suppress progress reporting"),
+		OPT__VERBOSE(&opts.verbose, "force progress reporting"),
+		OPT__QUIET(&opts.quiet, "suppress feedback reporting"),
 		OPT_STRING('b', NULL, &opts.new_branch, "branch",
 			   "create and checkout a new branch"),
 		OPT_STRING('B', NULL, &opts.new_branch_force, "branch",
@@ -958,6 +960,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	gitmodules_config();
 	git_config(git_checkout_config, &opts);
 
+	if (opts.quiet)
+		opts.verbose = 0;
+	else if (!opts.verbose)
+		opts.verbose = isatty(2);
 	opts.track = BRANCH_TRACK_UNSPECIFIED;
 
 	argc = parse_options(argc, argv, prefix, options, checkout_usage,
-- 
1.7.7.rc0.dirty

[PATCH 0/2 RFC] Add update_progress(), divert checkout messages

[sdaoden]

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(-)

-- 
1.7.7.rc0.dirty

[PATCH 1/2] progress: add update_progress()

[sdaoden]

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(-)

diff --git a/progress.c b/progress.c
index 3971f49..c86f83f 100644
--- a/progress.c
+++ b/progress.c
@@ -234,6 +234,21 @@ struct progress *start_progress(const char *title, unsigned total)
 	return start_progress_delay(title, total, 0, 0);
 }
 
+void update_progress(struct progress *progress, const char *title,
+			unsigned total)
+{
+	if (!progress)
+		return;
+	progress->title = title;
+	progress->total = total;
+	progress->last_value = -1;
+	progress->last_percent = -1;
+	if (progress->delay < 0)
+		progress->delay = 0;
+	clear_progress_signal();
+	set_progress_signal();
+}
+
 void stop_progress(struct progress **p_progress)
 {
 	stop_progress_msg(p_progress, "done");
diff --git a/progress.h b/progress.h
index 611e4c4..eed5b58 100644
--- a/progress.h
+++ b/progress.h
@@ -8,6 +8,8 @@ int display_progress(struct progress *progress, unsigned n);
 struct progress *start_progress(const char *title, unsigned total);
 struct progress *start_progress_delay(const char *title, unsigned total,
 				       unsigned percent_treshold, unsigned delay);
+void update_progress(struct progress *progress, const char *title,
+			unsigned total);
 void stop_progress(struct progress **progress);
 void stop_progress_msg(struct progress **progress, const char *msg);
 
-- 
1.7.7.rc0.dirty

[PATCH 2/2] unpack-trees: divert check_updates() output via update_progress()

[sdaoden]

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(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index cc616c3..95cd8a6 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -178,26 +178,34 @@ static void unlink_entry(struct cache_entry *ce)
 static struct checkout state;
 static int check_updates(struct unpack_trees_options *o)
 {
-	unsigned cnt = 0, total = 0;
+	unsigned rm_cnt, co_cnt, cnt;
 	struct progress *progress = NULL;
 	struct index_state *index = &o->result;
 	int i;
 	int errs = 0;
 
 	if (o->update && o->verbose_update) {
-		for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
+		rm_cnt = co_cnt = 0;
+		for (cnt = 0; cnt < index->cache_nr; cnt++) {
 			struct cache_entry *ce = index->cache[cnt];
-			if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
-				total++;
+			switch (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE)) {
+			case CE_UPDATE:
+				co_cnt++;
+				break;
+			default:
+				rm_cnt++;
+				break;
+			}
 		}
 
-		progress = start_progress_delay("Checking out files",
-						total, 50, 1);
+		progress = start_progress_delay("Updating work tree: "
+						"removing files",
+						rm_cnt, 64, 1);
 		cnt = 0;
 	}
-
 	if (o->update)
 		git_attr_set_direction(GIT_ATTR_CHECKOUT, &o->result);
+
 	for (i = 0; i < index->cache_nr; i++) {
 		struct cache_entry *ce = index->cache[i];
 
@@ -211,6 +219,13 @@ static int check_updates(struct unpack_trees_options *o)
 	remove_marked_cache_entries(&o->result);
 	remove_scheduled_dirs();
 
+	if (co_cnt > 0) {
+		update_progress(progress,
+				"Updating work tree: checking out files",
+				co_cnt);
+		cnt = 0;
+	}
+
 	for (i = 0; i < index->cache_nr; i++) {
 		struct cache_entry *ce = index->cache[i];
 
@@ -222,6 +237,7 @@ static int check_updates(struct unpack_trees_options *o)
 			}
 		}
 	}
+
 	stop_progress(&progress);
 	if (o->update)
 		git_attr_set_direction(GIT_ATTR_CHECKIN, NULL);
-- 
1.7.7.rc0.dirty