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

---
 progress.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

index 3971f49..c548de4 100644
@@ -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

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 <[email protected]> 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 <[email protected]> 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

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?

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

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.

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

index d5ffa1c..17e1793 100644
@@ -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

Why?

Nicolas

@ Nicolas Pitre <[email protected]> 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

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 <[email protected]> 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:

---
 builtin/checkout.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index 4eaedff..6fb6d48 100644
@@ -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

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 <[email protected]> [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."

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: [email protected]

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.

---

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

index c0a96e6..77ad4f3 100644
@@ -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::

index 4eaedff..7297843 100644
@@ -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

From: Steffen Daode Nurpmeso <[email protected]>

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

From: Steffen Daode Nurpmeso <[email protected]>

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.

---
 progress.c |   15 +++++++++++++++
 progress.h |    2 ++
 2 files changed, 17 insertions(+), 0 deletions(-)

index 3971f49..c86f83f 100644
@@ -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");

index 611e4c4..eed5b58 100644
@@ -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

From: Steffen Daode Nurpmeso <[email protected]>

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

---
 unpack-trees.c |   30 +++++++++++++++++++++++-------
 1 files changed, 23 insertions(+), 7 deletions(-)

index cc616c3..95cd8a6 100644
@@ -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

@ Tay Ray Chuan <[email protected]> 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 <[email protected]> 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