[RFC,v2,5/7] date: push pager.h dependency up

Commit Message

Calvin Wan Aug. 10, 2023, 4:36 p.m. UTC

In order for date.c to be included in git-std-lib, the dependency to
pager.h must be removed since it has dependencies on many other files
not in git-std-lib. We achieve this by passing a boolean for
"pager_in_use", rather than checking for it in parse_date_format() so
callers of the function will have that dependency.

Signed-off-by: Calvin Wan <[e-mail]>
---
 builtin/blame.c      | 2 +-
 builtin/log.c        | 2 +-
 date.c               | 5 ++---
 date.h               | 2 +-
 ref-filter.c         | 3 ++-
 revision.c           | 3 ++-
 t/helper/test-date.c | 3 ++-
 7 files changed, 11 insertions(+), 9 deletions(-)

Comments

Glen Choo Aug. 10, 2023, 11:41 p.m. UTC | #1

Calvin Wan <[e-mail]> writes:

> In order for date.c to be included in git-std-lib, the dependency to
> pager.h must be removed since it has dependencies on many other files
> not in git-std-lib.

Dependencies aside, I doubt callers of Git libraries want Git's
pager-handling logic bundled in git-std-lib ;)

> @@ -1003,13 +1002,13 @@ static enum date_mode_type parse_date_type(const char *format, const char **end)
>  	die("unknown date format %s", format);
>  }
>  
> -void parse_date_format(const char *format, struct date_mode *mode)
> +void parse_date_format(const char *format, struct date_mode *mode, int pager_in_use)
>  {
>  	const char *p;
>  
>  	/* "auto:foo" is "if tty/pager, then foo, otherwise normal" */
>  	if (skip_prefix(format, "auto:", &p)) {
> -		if (isatty(1) || pager_in_use())
> +		if (isatty(1) || pager_in_use)
>  			format = p;
>  		else
>  			format = "default";

Hm, it feels odd to ship a parsing option that changes based on whether
the caller isatty or not. Ideally we would stub this "switch the value
of auto" logic too.

Without reading ahead, I'm not sure if there are other sorts of "library
influencing process-wide" oddities like the one here and in the previous
patch. I think it would be okay for us to merge this series with these,
as long as we advertise to callers that the library boundary isn't very
clean yet, and we eventually clean it up.

Jonathan Tan Aug. 14, 2023, 10:17 p.m. UTC | #2

Calvin Wan <[e-mail]> writes:
> In order for date.c to be included in git-std-lib, the dependency to
> pager.h must be removed since it has dependencies on many other files
> not in git-std-lib. We achieve this by passing a boolean for
> "pager_in_use", rather than checking for it in parse_date_format() so
> callers of the function will have that dependency.

Instead of doing it as you describe here, could this be another stub
instead? That way, we don't need to change the code here.

I don't feel strongly about this, though, so if other reviewers think
that the approach in this patch makes the code better, I'm OK with that.

Patch

diff --git a/builtin/blame.c b/builtin/blame.c
index 9a3f9facea..665511570d 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -714,7 +714,7 @@  static int git_blame_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "blame.date")) {
 		if (!value)
 			return config_error_nonbool(var);
-		parse_date_format(value, &blame_date_mode);
+		parse_date_format(value, &blame_date_mode, pager_in_use());
 		return 0;
 	}
 	if (!strcmp(var, "blame.ignorerevsfile")) {
diff --git a/builtin/log.c b/builtin/log.c
index 03954fb749..a72ce30c2e 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -185,7 +185,7 @@  static void cmd_log_init_defaults(struct rev_info *rev)
 	rev->diffopt.flags.allow_textconv = 1;
 
 	if (default_date_mode)
-		parse_date_format(default_date_mode, &rev->date_mode);
+		parse_date_format(default_date_mode, &rev->date_mode, pager_in_use());
 }
 
 static void set_default_decoration_filter(struct decoration_filter *decoration_filter)
diff --git a/date.c b/date.c
index 619ada5b20..55f73ce2e0 100644
--- a/date.c
+++ b/date.c
@@ -7,7 +7,6 @@ 
 #include "git-compat-util.h"
 #include "date.h"
 #include "gettext.h"
-#include "pager.h"
 #include "strbuf.h"
 
 /*
@@ -1003,13 +1002,13 @@  static enum date_mode_type parse_date_type(const char *format, const char **end)
 	die("unknown date format %s", format);
 }
 
-void parse_date_format(const char *format, struct date_mode *mode)
+void parse_date_format(const char *format, struct date_mode *mode, int pager_in_use)
 {
 	const char *p;
 
 	/* "auto:foo" is "if tty/pager, then foo, otherwise normal" */
 	if (skip_prefix(format, "auto:", &p)) {
-		if (isatty(1) || pager_in_use())
+		if (isatty(1) || pager_in_use)
 			format = p;
 		else
 			format = "default";
diff --git a/date.h b/date.h
index 6136212a19..d9bd6dc09f 100644
--- a/date.h
+++ b/date.h
@@ -53,7 +53,7 @@  const char *show_date(timestamp_t time, int timezone, const struct date_mode *mo
  * be used with strbuf_addftime(), in which case you'll need to call
  * date_mode_release() later.
  */
-void parse_date_format(const char *format, struct date_mode *mode);
+void parse_date_format(const char *format, struct date_mode *mode, int pager_in_use);
 
 /**
  * Release a "struct date_mode", currently only required if
diff --git a/ref-filter.c b/ref-filter.c
index 2ed0ecf260..1b96bb7822 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -28,6 +28,7 @@ 
 #include "worktree.h"
 #include "hashmap.h"
 #include "strvec.h"
+#include "pager.h"
 
 static struct ref_msg {
 	const char *gone;
@@ -1323,7 +1324,7 @@  static void grab_date(const char *buf, struct atom_value *v, const char *atomnam
 	formatp = strchr(atomname, ':');
 	if (formatp) {
 		formatp++;
-		parse_date_format(formatp, &date_mode);
+		parse_date_format(formatp, &date_mode, pager_in_use());
 	}
 
 	if (!eoemail)
diff --git a/revision.c b/revision.c
index 985b8b2f51..c7efd11914 100644
--- a/revision.c
+++ b/revision.c
@@ -46,6 +46,7 @@ 
 #include "resolve-undo.h"
 #include "parse-options.h"
 #include "wildmatch.h"
+#include "pager.h"
 
 volatile show_early_output_fn_t show_early_output;
 
@@ -2577,7 +2578,7 @@  static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->date_mode.type = DATE_RELATIVE;
 		revs->date_mode_explicit = 1;
 	} else if ((argcount = parse_long_opt("date", argv, &optarg))) {
-		parse_date_format(optarg, &revs->date_mode);
+		parse_date_format(optarg, &revs->date_mode, pager_in_use());
 		revs->date_mode_explicit = 1;
 		return argcount;
 	} else if (!strcmp(arg, "--log-size")) {
diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index 0683d46574..b3927a95b3 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -1,5 +1,6 @@ 
 #include "test-tool.h"
 #include "date.h"
+#include "pager.h"
 #include "trace.h"
 
 static const char *usage_msg = "\n"
@@ -37,7 +38,7 @@  static void show_dates(const char **argv, const char *format)
 {
 	struct date_mode mode = DATE_MODE_INIT;
 
-	parse_date_format(format, &mode);
+	parse_date_format(format, &mode, pager_in_use());
 	for (; *argv; argv++) {
 		char *arg;
 		timestamp_t t;