[PATCH/RFC] Encapsulating isatty(2) calls into new progress_is_desired()

[PATCH/RFC] Encapsulating isatty(2) calls into new progress_is_desired()

[Steffen Daode Nurpmeso]

Adds a progress_is_desired() function and replaces most calls of
isatty(2) in the codebase with it.

Signed-off-by: Steffen Daode Nurpmeso <sdaoden@gmail.com>
---
Ok, i yelled at it and here you have a patch which performs the
encapsulation, just in case you agree.
I'm using static init, the short glance i had at the code made me
think thread safety is not an issue once this is called?
I think caching the value is worth an .align; it's a syscall
AFAIK.  (It's not used yet, however.)

And regardless of the fact that i'm stupid  Jeff King is right,
frontend parsing is important, and then CR plays an important role.
It's painful when you try to get some usable info out of these
closed proprietary status messages.  Sorry, really sorry.
I *really* love 'git add -i' and 'git rebase -i', these are
*fantastic* features!  Not scriptable through cron(8) though. :-)
Line tagging is also nice for parsing, by the way, as in

> wx-account: message 42/1 (6837/6837 bytes) .. sync
@ xy-account: xy@z.com -> pop.gmail.com/74.125.39.109:995
- xy-account: 0 messages (0 bytes) [action=seen]
< org.kernel.git: 22 tickets ok (0 errors).
< org.openbsd.misc: 20 tickets ok (0 errors).
= Dispatched 42 tickets to 2 targets.

 builtin/merge.c          |    5 +++--
 builtin/pack-objects.c   |    2 +-
 builtin/prune-packed.c   |    2 +-
 builtin/unpack-objects.c |    2 +-
 progress.c               |    8 ++++++++
 progress.h               |    1 +
 remote-curl.c            |    2 +-
 transport.c              |    7 ++++---
 transport.h              |    2 +-
 9 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 325891e..51fe030 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -695,8 +695,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 			o.subtree_shift = "";
 
 		o.renormalize = option_renormalize;
-		o.show_rename_progress =
-			show_progress == -1 ? isatty(2) : show_progress;
+		o.show_rename_progress = ((show_progress == -1)
+						? progress_is_desired()
+						: show_progress);
 
 		for (x = 0; x < xopts_nr; x++)
 			if (parse_merge_opt(&o, xopts[x]))
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f402a84..f3ee648 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2138,7 +2138,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	if (!pack_compression_seen && core_compression_seen)
 		pack_compression_level = core_compression_level;
 
-	progress = isatty(2);
+	progress = progress_is_desired();
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 
diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c
index f9463de..cc48807 100644
--- a/builtin/prune-packed.c
+++ b/builtin/prune-packed.c
@@ -71,7 +71,7 @@ void prune_packed_objects(int opts)
 
 int cmd_prune_packed(int argc, const char **argv, const char *prefix)
 {
-	int opts = isatty(2) ? VERBOSE : 0;
+	int opts = progress_is_desired() ? VERBOSE : 0;
 	const struct option prune_packed_options[] = {
 		OPT_BIT('n', "dry-run", &opts, "dry run", DRY_RUN),
 		OPT_NEGBIT('q', "quiet", &opts, "be quiet", VERBOSE),
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index f63973c..14cea36 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -501,7 +501,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)
 
 	git_config(git_default_config, NULL);
 
-	quiet = !isatty(2);
+	quiet = !progress_is_desired();
 
 	for (i = 1 ; i < argc; i++) {
 		const char *arg = argv[i];
diff --git a/progress.c b/progress.c
index c548de4..6baadd6 100644
--- a/progress.c
+++ b/progress.c
@@ -209,6 +209,14 @@ int display_progress(struct progress *progress, unsigned n)
 	return progress ? display(progress, n, NULL) : 0;
 }
 
+int progress_is_desired(void)
+{
+	static long eitty /*= 0*/;
+	if (!eitty)
+		eitty = isatty(2) ? 1 : -1;
+	return (eitty == 1);
+}
+
 struct progress *start_progress_delay(const char *title, unsigned total,
 				       unsigned percent_treshold, unsigned delay)
 {
diff --git a/progress.h b/progress.h
index 611e4c4..d7020d3 100644
--- a/progress.h
+++ b/progress.h
@@ -5,6 +5,7 @@ struct progress;
 
 void display_throughput(struct progress *progress, off_t total);
 int display_progress(struct progress *progress, unsigned n);
+int progress_is_desired(void);
 struct progress *start_progress(const char *title, unsigned total);
 struct progress *start_progress_delay(const char *title, unsigned total,
 				       unsigned percent_treshold, unsigned delay);
diff --git a/remote-curl.c b/remote-curl.c
index b5be25c..b47097a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -841,7 +841,7 @@ int main(int argc, const char **argv)
 	}
 
 	options.verbosity = 1;
-	options.progress = !!isatty(2);
+	options.progress = progress_is_desired();
 	options.thin = 1;
 
 	remote = remote_get(argv[1]);
diff --git a/transport.c b/transport.c
index c9c8056..8c67cfa 100644
--- a/transport.c
+++ b/transport.c
@@ -883,7 +883,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 	const char *helper;
 	struct transport *ret = xcalloc(1, sizeof(*ret));
 
-	ret->progress = isatty(2);
+	ret->progress = progress_is_desired();
 
 	if (!remote)
 		die("No remote provided to transport_get()");
@@ -998,9 +998,10 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
 	 *
 	 *   1. Report progress, if force_progress is 1 (ie. --progress).
 	 *   2. Don't report progress, if verbosity < 0 (ie. -q/--quiet ).
-	 *   3. Report progress if isatty(2) is 1.
+	 *   3. Report progress if progress default policies apply.
 	 **/
-	transport->progress = force_progress || (verbosity >= 0 && isatty(2));
+	transport->progress = (force_progress ||
+				(verbosity >= 0 && progress_is_desired()));
 }
 
 int transport_push(struct transport *transport,
diff --git a/transport.h b/transport.h
index 161d724..6b73240 100644
--- a/transport.h
+++ b/transport.h
@@ -82,7 +82,7 @@ struct transport {
 	signed verbose : 3;
 	/**
 	 * Transports should not set this directly, and should use this
-	 * value without having to check isatty(2), -q/--quiet
+	 * value without having to check progress_is_desired(), -q/--quiet
 	 * (transport->verbose < 0), etc. - checking has already been done
 	 * in transport_set_verbosity().
 	 **/
-- 
1.7.6.1.ge79e.dirty

Re: [PATCH/RFC] Encapsulating isatty(2) calls into new progress_is_desired()

[Junio C Hamano]

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

> Adds a progress_is_desired() function and replaces most calls of
> isatty(2) in the codebase with it.

Two comments, and perhaps a half.

One is trivial: want_progress() might be shorter and nicer.

The other is that we have isatty() for different file descriptors, and I
wonder if we want to consolidate them further and/or give them names, in a
separate patch. Here is a quick analysis.


* Most of the isatty(2) calls are indeed "want_progress()", except for one.

builtin/merge.c:	show_progress == -1 ? isatty(2) : show_progress;

	When "show_progress" is unset (denoted by -1), take the hint from
	stderr to decide if we show the progress output.

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

	Default "progress" to 0 (no progress output) or 1 (give progress
	for the write-out phase only when sending the pack to
	filesystem). Later, command line parsing may update this variable.

builtin/unpack-objects.c:	quiet = !isatty(2);

	Default to give progress when connected to a tty.

remote-curl.c:	options.progress = !!isatty(2);

	Default to give progress when connected to a tty.

transport.c:	ret->progress = isatty(2);

	Default to give progress when connected to a tty.

transport.c:	transport->progress = force_progress || (verbosity >= 0 && isatty(2));
transport.c-}

	Default to give progress when connected to a tty.

pager.c:	if (isatty(2))
pager.c-		dup2(pager_process.in, 2);
pager.c-	close(pager_process.in);

	This should not be renamed to want_progress() even though it is
	isatty(2).


* Many places ask "Are we in a keyboard-interactive session?" using
  isatty(0) and change their behaviour accordingly. We might want a
  symbolic name for them.

builtin/commit.c:		if (isatty(0))
builtin/commit.c-			fprintf(stderr, _("(reading log message from standard input)\n"));

        This is to warn against "git commit -F -" that forgot to redirect
        an upstream pipe into the command;
        i.e. "script-to-generate-message | git commit -F -" is what the
        user probably wanted to.

builtin/pack-redundant.c:	if (!isatty(0)) {
builtin/pack-redundant.c-		while (fgets(buf, sizeof(buf), stdin)) {
builtin/pack-redundant.c-			sha1 = xmalloc(20);

	The command _can_ be fed a list of object names from the standard
	input, but we do not expect anybody typing the object names from
	the keyboard.

	Side note: I may be more consistent to do the "warn" thing commit
	does (see above) instead; I also suspect this command outlived its
	usefulness.

builtin/prune-packed.c:	int opts = isatty(2) ? VERBOSE : 0;

	Default to give progress when connected to a tty.

builtin/revert.c:	if (isatty(0))
builtin/revert.c-		edit = 1;

	"git revert" was designed to force you to edit the message to
	justify why reverting the given commit is the right thing, but
	in a non-interactive session, opening an editor is not a good
	thing to do.

builtin/shortlog.c:	if (!nongit && !rev.pending.nr && isatty(0))
builtin/shortlog.c-		add_head_to_pending(&rev);
builtin/shortlog.c-	if (rev.pending.nr == 0) {
builtin/shortlog.c:		if (isatty(0))
builtin/shortlog.c-			fprintf(stderr, _("(reading log message from standard input)\n"));
builtin/shortlog.c-		read_from_stdin(&log);

	"git shortlog" (without any revisions argument) can work as a
	filter to read "log" output (e.g. "git log ... | git shortlog")
	and condense it down, in which case it will not be reading from
	the terminal .  Without such an upstream process, "git shortlog"
	internally runs "git log HEAD" and gives its output.

* We decide to color the output when spitting out to a terminal using
  isatty(1).

builtin/config.c:			stdout_is_tty = isatty(1);
builtin/config.c-		return get_colorbool(argc != 0);

	This is to default for color output on terminal output, and is
	consistent with the one in color.c below.

color.c-	if (stdout_is_tty < 0)
color.c:		stdout_is_tty = isatty(1);
color.c-	if (stdout_is_tty || (pager_in_use() && pager_use_color)) {
color.c-		char *term = getenv("TERM");

	Default to give color output on a capable terminal when
	configruation says auto.

pager.c:	const char *pager = git_pager(isatty(1));

	Use pager on tty by default.


The remaining half is about various ways the "progress", "verbose" and
"quiet" are all mixed up (obviously "progress" is part of being "verbose",
but "verbose" can and do mean giving more information other than
progress), and they are different ways in which the values to give to the
internal variables are determined (some use isatty(2) to initialize the
default and then let option parser to update it, some run option parser
first and if the variable is unset use isatty(2) to give it default). We
might want to think about making them more uniform (or we may not).

Re: [PATCH/RFC] Encapsulating isatty(2) calls into new progress_is_desired()

[Steffen Daode Nurpmeso]

@ Junio C Hamano <gitster@pobox.com> wrote (2011-06-30 01:17+0200):
> Two comments, and perhaps a half.

Git looks pretty transparent through your eyes, so thanks for your
mail.  I really have appreciated to read it.
(The very idea itself is a shallow no-go that way.)

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