From: Omar Polo <op@omarpolo.com>
Subject: gotadmin: log progress to stderr
To: gameoftrees@openbsd.org
Date: Thu, 06 Jul 2023 10:17:21 +0200

I'd like for the upcoming dump subcommand to output on stdout, so this
changes gotadmin' progres_* functions to output the progress on stderr
and adjusts the regress when needed.

I'm currently reusing the pack_progress() function that's why I
noticed.  I could write a new callback that logs on stderr, and sooner
or later `dump' will need its own progress function, but would be the
only one, so this is really to see what do you think and see if I can
get by without a -o flag for dump.

I'm still not completely convinced in using stderr for progress info,
but the nice thing is that it allows to do `gotadmin dump > foo' and
see the progress at the same time.


diff /home/op/w/got
commit - 27555e8e6053ab0cc846d201757b588d5a79293f
path + /home/op/w/got
blob - 5551b60b73215eb11a82c034e1d2601ed9a15c31
file + gotadmin/gotadmin.c
--- gotadmin/gotadmin.c
+++ gotadmin/gotadmin.c
@@ -479,16 +479,16 @@ print_load_info(int print_colored, int print_found, in
     int ncolored, int nfound, int ntrees)
 {
 	if (print_colored) {
-		printf("%d commit%s colored", ncolored,
+		fprintf(stderr, "%d commit%s colored", ncolored,
 		    ncolored == 1 ? "" : "s");
 	}
 	if (print_found) {
-		printf("%s%d object%s found",
+		fprintf(stderr, "%s%d object%s found",
 		    ncolored > 0 ? "; " : "",
 		    nfound, nfound == 1 ? "" : "s");
 	}
 	if (print_trees) {
-		printf("; %d tree%s scanned", ntrees,
+		fprintf(stderr, "; %d tree%s scanned", ntrees,
 		    ntrees == 1 ? "" : "s");
 	}
 }
@@ -528,16 +528,15 @@ pack_progress(void *arg, int ncolored, int nfound, int
 
 	if ((print_colored || print_found || print_trees) &&
 	    !a->loading_done) {
-		printf("\r");
+		fprintf(stderr, "\r");
 		print_load_info(print_colored, print_found, print_trees,
 		    ncolored, nfound, ntrees);
 		a->printed_something = 1;
-		fflush(stdout);
 		return NULL;
 	} else if (!a->loading_done) {
-		printf("\r");
+		fprintf(stderr, "\r");
 		print_load_info(1, 1, 1, ncolored, nfound, ntrees);
-		printf("\n");
+		fprintf(stderr, "\n");
 		a->loading_done = 1;
 	}
 
@@ -585,23 +584,21 @@ pack_progress(void *arg, int ncolored, int nfound, int
 	}
 
 	if (print_searching || print_total || print_deltify || print_written)
-		printf("\r");
+		fprintf(stderr, "\r");
 	if (print_searching)
-		printf("packing %d reference%s", ncommits,
+		fprintf(stderr, "packing %d reference%s", ncommits,
 		    ncommits == 1 ? "" : "s");
 	if (print_total)
-		printf("; %d object%s", nobj_total,
+		fprintf(stderr, "; %d object%s", nobj_total,
 		    nobj_total == 1 ? "" : "s");
 	if (print_deltify)
-		printf("; deltify: %d%%", p_deltify);
+		fprintf(stderr, "; deltify: %d%%", p_deltify);
 	if (print_written)
-		printf("; writing pack: %*s %d%%", FMT_SCALED_STRSIZE - 2,
-		    scaled_size, p_written);
+		fprintf(stderr, "; writing pack: %*s %d%%",
+		    FMT_SCALED_STRSIZE - 2, scaled_size, p_written);
 	if (print_searching || print_total || print_deltify ||
-	    print_written) {
+	    print_written)
 		a->printed_something = 1;
-		fflush(stdout);
-	}
 	return NULL;
 }
 
@@ -647,15 +644,14 @@ pack_index_progress(void *arg, off_t packfile_size, in
 
 	}
 	if (print_size || print_indexed || print_resolved)
-		printf("\r");
+		fprintf(stderr, "\r");
 	if (print_size)
-		printf("%*s packed", FMT_SCALED_STRSIZE - 2, scaled_size);
+		fprintf(stderr, "%*s packed", FMT_SCALED_STRSIZE - 2,
+		    scaled_size);
 	if (print_indexed)
-		printf("; indexing %d%%", p_indexed);
+		fprintf(stderr, "; indexing %d%%", p_indexed);
 	if (print_resolved)
-		printf("; resolving deltas %d%%", p_resolved);
-	if (print_size || print_indexed || print_resolved)
-		fflush(stdout);
+		fprintf(stderr, "; resolving deltas %d%%", p_resolved);
 
 	return NULL;
 }
@@ -809,7 +805,7 @@ cmd_pack(int argc, char *argv[])
 	    force_refdelta, pack_progress, &ppa, check_cancelled, NULL);
 	if (error) {
 		if (ppa.printed_something)
-			printf("\n");
+			fprintf(stderr, "\n");
 		goto done;
 	}
 
@@ -1177,27 +1173,28 @@ cleanup_progress(void *arg, int nloose, int ncommits, 
 		return NULL;
 
 	if (print_loose || print_commits || print_purged || print_redundant)
-		printf("\r");
+		fprintf(stderr, "\r");
 	if (print_loose)
-		printf("%d loose object%s", nloose, nloose == 1 ? "" : "s");
+		fprintf(stderr, "%d loose object%s", nloose,
+		    nloose == 1 ? "" : "s");
 	if (print_commits)
-		printf("; %d commit%s scanned", ncommits,
+		fprintf(stderr, "; %d commit%s scanned", ncommits,
 		    ncommits == 1 ? "" : "s");
 	if (print_purged || print_redundant) {
 		if (a->dry_run) {
-			printf("; could purge %d object%s", npurged,
+			fprintf(stderr, "; could purge %d object%s", npurged,
 			    npurged == 1 ? "" : "s");
 		} else {
-			printf("; purged %d object%s", npurged,
+			fprintf(stderr, "; purged %d object%s", npurged,
 			    npurged == 1 ? "" : "s");
 		}
 	}
 	if (print_redundant) {
 		if (a->dry_run) {
-			printf(", %d pack file%s", nredundant,
+			fprintf(stderr, ", %d pack file%s", nredundant,
 			    nredundant == 1 ? "" : "s");
 		} else {
-			printf(", %d pack file%s", nredundant,
+			fprintf(stderr, ", %d pack file%s", nredundant,
 			    nredundant == 1 ? "" : "s");
 		}
 	}
@@ -1338,7 +1335,7 @@ cmd_cleanup(int argc, char *argv[])
 	    check_cancelled, NULL);
 	if (error) {
 		if (cpa.printed_something)
-			printf("\n");
+			fprintf(stderr, "\n");
 		goto done;
 	}
 
@@ -1346,7 +1343,7 @@ cmd_cleanup(int argc, char *argv[])
 	    &pack_after, dry_run, ncommits, nloose, npacked,
 	    cleanup_progress, &cpa, check_cancelled, NULL);
 	if (cpa.printed_something)
-		printf("\n");
+		fprintf(stderr, "\n");
 	if (error)
 		goto done;
 
blob - f6ddbb9657a29cfb443245457d038aee5b06d364
file + regress/cmdline/cleanup.sh
--- regress/cmdline/cleanup.sh
+++ regress/cmdline/cleanup.sh
@@ -157,7 +157,7 @@ test_cleanup_redundant_loose_objects() {
 		return 1
 	fi
 
-	gotadmin pack -r $testroot/repo > /dev/null
+	gotadmin pack -q -r $testroot/repo
 
 	npacked0=`gotadmin info -r $testroot/repo | grep '^packed objects:' | \
 		cut -d ':' -f 2 | tr -d ' '`
@@ -250,7 +250,7 @@ test_cleanup_redundant_pack_files() {
 	fi
 
 	# create a redundant pack with an associated .keep file
-	hash=$(gotadmin pack -a -r "$testroot/repo" \
+	hash=$(gotadmin pack -a -r "$testroot/repo" 2>/dev/null \
 		| awk '/^Indexed/{print $2}')
 	kpack="$testroot/repo/.git/objects/pack/pack-$hash"
 	touch "${kpack%.pack}.keep"
@@ -259,15 +259,15 @@ test_cleanup_redundant_pack_files() {
 	for i in `jot 5`; do
 		echo "alpha $i" > $testroot/repo/alpha
 		git_commit "$testroot/repo" -m "edit #$i"
-		gotadmin pack -r "$testroot/repo" >/dev/null
+		gotadmin pack -q -r "$testroot/repo"
 	done
 
 	# create two packs with all the objects
-	gotadmin pack -a -r "$testroot/repo" >/dev/null
-	gotadmin pack -a -r "$testroot/repo" >/dev/null
+	gotadmin pack -q -a -r "$testroot/repo"
+	gotadmin pack -q -a -r "$testroot/repo"
 
-	gotadmin cleanup -r "$testroot/repo" | grep 'pack files? purged' \
-		| tail -1 > $testroot/stdout
+	(gotadmin cleanup -r "$testroot/repo"  | grep 'pack files? purged' \
+		| tail -1) > $testroot/stdout 2>/dev/null
 
 	echo "5 pack files purged" > $testroot/stdout.expected
 	if cmp -s "$testroot/stdout.expected" "$testroot/stdout"; then
@@ -300,10 +300,10 @@ test_cleanup_redundant_pack_files() {
 		echo "alpha again $i" > $testroot/repo/alpha
 		git_commit "$testroot/repo" -m "edit $i"
 	done
-	gotadmin pack -r "$testroot/repo" >/dev/null
+	gotadmin pack -q -r "$testroot/repo"
 
-	gotadmin cleanup -r "$testroot/repo" | grep 'pack files? purged' \
-	    | tail -1 > $testroot/stdout
+	(gotadmin cleanup -r "$testroot/repo" | grep 'pack files? purged' \
+	    | tail -1) > $testroot/stdout 2>/dev/null
 
 	echo "0 pack files purged" > $testroot/stdout.expected
 	if cmp -s "$testroot/stdout.expected" "$testroot/stdout"; then
@@ -328,13 +328,13 @@ test_cleanup_redundant_pack_files() {
 	for i in `jot 5`; do
 		echo "alpha $i" > $testroot/repo/alpha
 		git_commit "$testroot/repo" -m "edit #$i"
-		gotadmin pack -r "$testroot/repo" >/dev/null
+		gotadmin pack -q -r "$testroot/repo"
 	done
 
-	gotadmin pack -a -x master -r "$testroot/repo" >/dev/null
+	gotadmin pack -aq -x master -r "$testroot/repo"
 
-	gotadmin cleanup -r "$testroot/repo" | grep 'pack files? purged' \
-		| tail -1 > $testroot/stdout
+	(gotadmin cleanup -r "$testroot/repo" | grep 'pack files? purged' \
+		| tail -1) > $testroot/stdout 2>/dev/null
 
 	echo "6 pack files purged" > $testroot/stdout.expected
 	if cmp -s "$testroot/stdout.expected" "$testroot/stdout"; then
@@ -400,7 +400,7 @@ test_cleanup_missing_pack_file() {
 		return 1
 	fi
 
-	gotadmin pack -r $testroot/repo > $testroot/stdout
+	gotadmin pack -r $testroot/repo > $testroot/stdout 2>/dev/null
 	packname=`grep ^Wrote $testroot/stdout | cut -d ' ' -f2`
 	packhash=`echo $packname | sed -e 's:^objects/pack/pack-::' \
 		-e 's/.pack$//'`
blob - f2acf22971de6d69864669240411b42ba43f58bc
file + regress/cmdline/pack.sh
--- regress/cmdline/pack.sh
+++ regress/cmdline/pack.sh
@@ -41,7 +41,7 @@ test_pack_all_loose_objects() {
 		return 1
 	fi
 
-	gotadmin pack -r $testroot/repo > $testroot/stdout
+	gotadmin pack -r $testroot/repo > $testroot/stdout 2>/dev/null
 	ret=$?
 	if [ $ret -ne 0 ]; then
 		echo "gotadmin pack failed unexpectedly" >&2
@@ -109,7 +109,7 @@ test_pack_exclude() {
 	echo a new line >> $testroot/wt/alpha
 	(cd $testroot/wt && got commit -m "edit alpha" >/dev/null)
 
-	gotadmin pack -r $testroot/repo -x master > $testroot/stdout
+	gotadmin pack -r $testroot/repo -x master >$testroot/stdout 2>/dev/null
 	ret=$?
 	if [ $ret -ne 0 ]; then
 		echo "gotadmin pack failed unexpectedly" >&2
@@ -214,7 +214,8 @@ test_pack_exclude_tag() {
 	echo a new line >> $testroot/wt/alpha
 	(cd $testroot/wt && got commit -m "edit alpha" >/dev/null)
 
-	gotadmin pack -r $testroot/repo -x refs/tags/1.0 > $testroot/stdout
+	gotadmin pack -r $testroot/repo -x refs/tags/1.0 > $testroot/stdout \
+		2>/dev/null
 	ret=$?
 	if [ $ret -ne 0 ]; then
 		echo "gotadmin pack failed unexpectedly" >&2
@@ -314,7 +315,7 @@ test_pack_include() {
 	(cd $testroot/wt && got commit -m "edit alpha" >/dev/null)
 	local commit1=`git_show_branch_head $testroot/repo mybranch`
 
-	gotadmin pack -r $testroot/repo master > $testroot/stdout
+	gotadmin pack -r $testroot/repo master >$testroot/stdout 2>/dev/null
 	ret=$?
 	if [ $ret -ne 0 ]; then
 		echo "gotadmin pack failed unexpectedly" >&2
@@ -474,7 +475,7 @@ test_pack_loose_only() {
 		return 1
 	fi
 
-	gotadmin pack -r $testroot/repo mybranch > $testroot/stdout
+	gotadmin pack -r $testroot/repo mybranch >$testroot/stdout 2>/dev/null
 	ret=$?
 	if [ $ret -ne 0 ]; then
 		echo "gotadmin pack failed unexpectedly" >&2
@@ -583,7 +584,8 @@ test_pack_all_objects() {
 
 	# pack mybranch, including already packed objects on the
 	# 'master' branch which are reachable from mybranch
-	gotadmin pack -r $testroot/repo -a mybranch > $testroot/stdout
+	gotadmin pack -r $testroot/repo -a mybranch > $testroot/stdout \
+		2>/dev/null
 	ret=$?
 	if [ $ret -ne 0 ]; then
 		echo "gotadmin pack failed unexpectedly" >&2