[PATCH v3 1/4] log: fix memory leak if --graph is passed multiple times

[PATCH v3 1/4] log: fix memory leak if --graph is passed multiple times

[Alex Henrie]

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
v3: no changes
---
 graph.c    | 12 ++++++++++++
 graph.h    |  5 +++++
 revision.c |  1 +
 3 files changed, 18 insertions(+)

diff --git a/graph.c b/graph.c
index e3828eb8f2..568b6e7cd4 100644
--- a/graph.c
+++ b/graph.c
@@ -401,6 +401,18 @@ struct git_graph *graph_init(struct rev_info *opt)
 	return graph;
 }
 
+void graph_clear(struct git_graph *graph)
+{
+	if (!graph)
+		return;
+
+	free(graph->columns);
+	free(graph->new_columns);
+	free(graph->mapping);
+	free(graph->old_mapping);
+	free(graph);
+}
+
 static void graph_update_state(struct git_graph *graph, enum graph_state s)
 {
 	graph->prev_state = graph->state;
diff --git a/graph.h b/graph.h
index 8313e293c7..e88632a014 100644
--- a/graph.h
+++ b/graph.h
@@ -139,6 +139,11 @@ void graph_set_column_colors(const char **colors, unsigned short colors_max);
  */
 struct git_graph *graph_init(struct rev_info *opt);
 
+/*
+ * Free a struct git_graph.
+ */
+void graph_clear(struct git_graph *graph);
+
 /*
  * Update a git_graph with a new commit.
  * This will cause the graph to begin outputting lines for the new commit
diff --git a/revision.c b/revision.c
index ad4286fbdd..816061f3d9 100644
--- a/revision.c
+++ b/revision.c
@@ -2426,6 +2426,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--graph")) {
 		revs->topo_order = 1;
 		revs->rewrite_parents = 1;
+		graph_clear(revs->graph);
 		revs->graph = graph_init(revs);
 	} else if (!strcmp(arg, "--encode-email-headers")) {
 		revs->encode_email_headers = 1;
-- 
2.35.1

[PATCH v3 2/4] log: add a --no-graph option

[Alex Henrie]

It's useful to be able to countermand a previous --graph option, for
example if `git log --graph` is run via an alias.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
v3: don't pass a regular expression with parentheses to grep, so that
the tests pass in all configurations on GitHub
---
 builtin/blame.c    |  1 +
 builtin/shortlog.c |  1 +
 revision.c         | 19 ++++++++++---
 revision.h         |  1 +
 t/t4202-log.sh     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 87 insertions(+), 4 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 7fafeac408..ef831de5ac 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -934,6 +934,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		parse_revision_opt(&revs, &ctx, options, blame_opt_usage);
 	}
 parse_done:
+	revision_opts_finish(&revs);
 	no_whole_file_rename = !revs.diffopt.flags.follow_renames;
 	xdl_opts |= revs.diffopt.xdl_opts & XDF_INDENT_HEURISTIC;
 	revs.diffopt.flags.follow_renames = 0;
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index e7f7af5de3..228d782754 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -388,6 +388,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 		parse_revision_opt(&rev, &ctx, options, shortlog_usage);
 	}
 parse_done:
+	revision_opts_finish(&rev);
 	argc = parse_options_end(&ctx);
 
 	if (nongit && argc > 1) {
diff --git a/revision.c b/revision.c
index 816061f3d9..a39fd1c278 100644
--- a/revision.c
+++ b/revision.c
@@ -2424,10 +2424,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->pretty_given = 1;
 		revs->abbrev_commit = 1;
 	} else if (!strcmp(arg, "--graph")) {
-		revs->topo_order = 1;
-		revs->rewrite_parents = 1;
 		graph_clear(revs->graph);
 		revs->graph = graph_init(revs);
+	} else if (!strcmp(arg, "--no-graph")) {
+		graph_clear(revs->graph);
+		revs->graph = NULL;
 	} else if (!strcmp(arg, "--encode-email-headers")) {
 		revs->encode_email_headers = 1;
 	} else if (!strcmp(arg, "--no-encode-email-headers")) {
@@ -2524,8 +2525,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 			unkv[(*unkc)++] = arg;
 		return opts;
 	}
-	if (revs->graph && revs->track_linear)
-		die(_("options '%s' and '%s' cannot be used together"), "--show-linear-break", "--graph");
 
 	return 1;
 }
@@ -2544,6 +2543,17 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 	ctx->argc -= n;
 }
 
+void revision_opts_finish(struct rev_info *revs)
+{
+	if (revs->graph && revs->track_linear)
+		die(_("options '%s' and '%s' cannot be used together"), "--show-linear-break", "--graph");
+
+	if (revs->graph) {
+		revs->topo_order = 1;
+		revs->rewrite_parents = 1;
+	}
+}
+
 static int for_each_bisect_ref(struct ref_store *refs, each_ref_fn fn,
 			       void *cb_data, const char *term)
 {
@@ -2786,6 +2796,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			break;
 		}
 	}
+	revision_opts_finish(revs);
 
 	if (prune_data.nr) {
 		/*
diff --git a/revision.h b/revision.h
index 3f66147bfd..5a507db202 100644
--- a/revision.h
+++ b/revision.h
@@ -372,6 +372,7 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 #define REVARG_COMMITTISH 02
 int handle_revision_arg(const char *arg, struct rev_info *revs,
 			int flags, unsigned revarg_opt);
+void revision_opts_finish(struct rev_info *revs);
 
 /**
  * Reset the flags used by the revision walking api. You can use this to do
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index dc884107de..a7d5edf720 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1671,6 +1671,75 @@ test_expect_success 'log --graph with --name-only' '
 	test_cmp_graph --name-only tangle..reach
 '
 
+test_expect_success '--no-graph countermands --graph' '
+	git log >expect &&
+	git log --graph --no-graph >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--graph countermands --no-graph' '
+	git log --graph >expect &&
+	git log --no-graph --graph >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--no-graph does not unset --topo-order' '
+	git log --topo-order >expect &&
+	git log --topo-order --no-graph >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--no-graph does not unset --parents' '
+	git log --parents >expect &&
+	git log --parents --no-graph >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--reverse and --graph conflict' '
+	test_must_fail git log --reverse --graph 2>stderr &&
+	test_i18ngrep "cannot be used together" stderr
+'
+
+test_expect_success '--reverse --graph --no-graph works' '
+	git log --reverse >expect &&
+	git log --reverse --graph --no-graph >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--show-linear-break and --graph conflict' '
+	test_must_fail git log --show-linear-break --graph 2>stderr &&
+	test_i18ngrep "cannot be used together" stderr
+'
+
+test_expect_success '--show-linear-break --graph --no-graph works' '
+	git log --show-linear-break >expect &&
+	git log --show-linear-break --graph --no-graph >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--no-walk and --graph conflict' '
+	test_must_fail git log --no-walk --graph 2>stderr &&
+	test_i18ngrep "cannot be used together" stderr
+'
+
+test_expect_success '--no-walk --graph --no-graph works' '
+	git log --no-walk >expect &&
+	git log --no-walk --graph --no-graph >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--walk-reflogs and --graph conflict' '
+	test_must_fail git log --walk-reflogs --graph 2>stderr &&
+	(test_i18ngrep "cannot combine" stderr ||
+		test_i18ngrep "cannot be used together" stderr)
+'
+
+test_expect_success '--walk-reflogs --graph --no-graph works' '
+	git log --walk-reflogs >expect &&
+	git log --walk-reflogs --graph --no-graph >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'dotdot is a parent directory' '
 	mkdir -p a/b &&
 	( echo sixth && echo fifth ) >expect &&
-- 
2.35.1

[PATCH v3 3/4] log: add a log.graph config option

[Alex Henrie]

A coworker recently asked me how to turn on --graph by default in
`git log`. I googled it and found that several people have asked that
before on Stack Overflow, with no good solution:
https://stackoverflow.com/questions/43555256/how-do-i-make-git-log-graph-the-default

Add a log.graph option to turn on graph mode in the absence of any
incompatible options.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
v3: no changes
---
 Documentation/config/log.txt |  5 +++++
 Documentation/git-log.txt    |  5 +++++
 builtin/log.c                |  6 ++++++
 revision.c                   | 10 +++++++++
 revision.h                   |  2 ++
 t/t4202-log.sh               | 42 ++++++++++++++++++++++++++++++++++++
 6 files changed, 70 insertions(+)

diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
index 456eb07800..3e356cfce6 100644
--- a/Documentation/config/log.txt
+++ b/Documentation/config/log.txt
@@ -35,6 +35,11 @@ log.follow::
 	i.e. it cannot be used to follow multiple files and does not work well
 	on non-linear history.
 
+log.graph::
+	If true, makes linkgit:git-log[1], linkgit:git-show[1], and
+	linkgit:git-whatchanged[1] assume `--graph` unless an incompatible
+	option is also specified.
+
 log.graphColors::
 	A list of colors, separated by commas, that can be used to draw
 	history lines in `git log --graph`.
diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 20e87cecf4..7e9e0f8afe 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -214,6 +214,11 @@ log.follow::
 	i.e. it cannot be used to follow multiple files and does not work well
 	on non-linear history.
 
+log.graph::
+	If `true`, `git log` and related commands will act as if the
+	`--graph` option was passed to them unless an incompatible option is
+	also specified.
+
 log.showRoot::
 	If `false`, `git log` and related commands will not treat the
 	initial commit as a big creation event.  Any root commits in
diff --git a/builtin/log.c b/builtin/log.c
index 4b493408cc..5eaefab046 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -48,6 +48,7 @@ static int default_show_root = 1;
 static int default_follow;
 static int default_show_signature;
 static int default_encode_email_headers = 1;
+static int default_graph;
 static int decoration_style;
 static int decoration_given;
 static int use_mailmap_config = 1;
@@ -156,6 +157,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)
 	rev->show_signature = default_show_signature;
 	rev->encode_email_headers = default_encode_email_headers;
 	rev->diffopt.flags.allow_textconv = 1;
+	rev->graph_default = default_graph;
 
 	if (default_date_mode)
 		parse_date_format(default_date_mode, &rev->date_mode);
@@ -519,6 +521,10 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		default_show_signature = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "log.graph")) {
+		default_graph = git_config_bool(var, value);
+		return 0;
+	}
 
 	if (grep_config(var, value, cb) < 0)
 		return -1;
diff --git a/revision.c b/revision.c
index a39fd1c278..55e12cd401 100644
--- a/revision.c
+++ b/revision.c
@@ -2426,9 +2426,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--graph")) {
 		graph_clear(revs->graph);
 		revs->graph = graph_init(revs);
+		revs->graph_default = 0;
 	} else if (!strcmp(arg, "--no-graph")) {
 		graph_clear(revs->graph);
 		revs->graph = NULL;
+		revs->graph_default = 0;
 	} else if (!strcmp(arg, "--encode-email-headers")) {
 		revs->encode_email_headers = 1;
 	} else if (!strcmp(arg, "--no-encode-email-headers")) {
@@ -2796,6 +2798,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			break;
 		}
 	}
+	if (revs->graph_default &&
+	    !revs->graph &&
+	    /* check for incompatible options */
+	    !revs->track_linear &&
+	    !revs->reverse &&
+	    !revs->reflog_info &&
+	    !revs->no_walk)
+		revs->graph = graph_init(revs);
 	revision_opts_finish(revs);
 
 	if (prune_data.nr) {
diff --git a/revision.h b/revision.h
index 5a507db202..62a0ef4c53 100644
--- a/revision.h
+++ b/revision.h
@@ -249,6 +249,8 @@ struct rev_info {
 
 	/* Display history graph */
 	struct git_graph *graph;
+	/* whether to initialize graph if it has not been initialized already */
+	int graph_default;
 
 	/* special limits */
 	int skip_count;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index a7d5edf720..9f052ccce6 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1695,6 +1695,20 @@ test_expect_success '--no-graph does not unset --parents' '
 	test_cmp expect actual
 '
 
+test_expect_success 'log.graph=true behaves like --graph' '
+	git log --graph >expect &&
+	test_config log.graph true &&
+	git log >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--no-graph countermands log.graph=true' '
+	git log >expect &&
+	test_config log.graph true &&
+	git log --no-graph >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success '--reverse and --graph conflict' '
 	test_must_fail git log --reverse --graph 2>stderr &&
 	test_i18ngrep "cannot be used together" stderr
@@ -1706,6 +1720,13 @@ test_expect_success '--reverse --graph --no-graph works' '
 	test_cmp expect actual
 '
 
+test_expect_success '--reverse ignores log.graph' '
+	git log --reverse >expect &&
+	test_config log.graph true &&
+	git log --reverse >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success '--show-linear-break and --graph conflict' '
 	test_must_fail git log --show-linear-break --graph 2>stderr &&
 	test_i18ngrep "cannot be used together" stderr
@@ -1717,6 +1738,13 @@ test_expect_success '--show-linear-break --graph --no-graph works' '
 	test_cmp expect actual
 '
 
+test_expect_success '--show-linear-break ignores log.graph' '
+	git log --show-linear-break >expect &&
+	test_config log.graph true &&
+	git log --show-linear-break >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success '--no-walk and --graph conflict' '
 	test_must_fail git log --no-walk --graph 2>stderr &&
 	test_i18ngrep "cannot be used together" stderr
@@ -1728,6 +1756,13 @@ test_expect_success '--no-walk --graph --no-graph works' '
 	test_cmp expect actual
 '
 
+test_expect_success '--no-walk ignores log.graph' '
+	git log --no-walk >expect &&
+	test_config log.graph true &&
+	git log --no-walk >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success '--walk-reflogs and --graph conflict' '
 	test_must_fail git log --walk-reflogs --graph 2>stderr &&
 	(test_i18ngrep "cannot combine" stderr ||
@@ -1740,6 +1775,13 @@ test_expect_success '--walk-reflogs --graph --no-graph works' '
 	test_cmp expect actual
 '
 
+test_expect_success '--walk-reflogs ignores log.graph' '
+	git log --walk-reflogs >expect &&
+	test_config log.graph true &&
+	git log --walk-reflogs >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'dotdot is a parent directory' '
 	mkdir -p a/b &&
 	( echo sixth && echo fifth ) >expect &&
-- 
2.35.1

[PATCH v3 4/4] gitk: pass --no-graph to `git log`

[Alex Henrie]

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
v3: no changes
---
 gitk-git/gitk | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 23d9dd1fe0..24099ce0b8 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -411,8 +411,8 @@ proc start_rev_list {view} {
     }
 
     if {[catch {
-        set fd [open [concat | git log --no-color -z --pretty=raw $show_notes \
-                        --parents --boundary $args "--" $files] r]
+        set fd [open [concat | git log --no-color --no-graph -z --pretty=raw \
+                        $show_notes --parents --boundary $args "--" $files] r]
     } err]} {
         error_popup "[mc "Error executing git log:"] $err"
         return 0
@@ -559,8 +559,9 @@ proc updatecommits {} {
         set args $vorigargs($view)
     }
     if {[catch {
-        set fd [open [concat | git log --no-color -z --pretty=raw $show_notes \
-                        --parents --boundary $args "--" $vfilelimit($view)] r]
+        set fd [open [concat | git log --no-color --no-graph -z --pretty=raw
+                        $show_notes --parents --boundary $args "--"
+                        $vfilelimit($view)] r]
     } err]} {
         error_popup "[mc "Error executing git log:"] $err"
         return
-- 
2.35.1

Re: [PATCH v3 4/4] gitk: pass --no-graph to `git log`

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
> v3: no changes
> ---
>  gitk-git/gitk | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Please base a patch on gitk part to base on Paul's tree, not mine,
meaning that the first few lines of diff should begin like so:

	diff --git a/gitk b/gitk
	index ...
	--- a/gitk
	+++ b/gitk

and not as part of the series.

What the first two patches want to do is a good thing regardless, so
I'll take a deeper look at them and queue them.  I am very skeptical
to log.graph=yes/no configuration for obvious reasons that setting
such a variable *will* break existing tools and users.  It is not
even "it might break but we don't know until we try", as this patch
loudly demonstrates.

Thanks.

> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 23d9dd1fe0..24099ce0b8 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -411,8 +411,8 @@ proc start_rev_list {view} {
>      }
>  
>      if {[catch {
> -        set fd [open [concat | git log --no-color -z --pretty=raw $show_notes \
> -                        --parents --boundary $args "--" $files] r]
> +        set fd [open [concat | git log --no-color --no-graph -z --pretty=raw \
> +                        $show_notes --parents --boundary $args "--" $files] r]
>      } err]} {
>          error_popup "[mc "Error executing git log:"] $err"
>          return 0
> @@ -559,8 +559,9 @@ proc updatecommits {} {
>          set args $vorigargs($view)
>      }
>      if {[catch {
> -        set fd [open [concat | git log --no-color -z --pretty=raw $show_notes \
> -                        --parents --boundary $args "--" $vfilelimit($view)] r]
> +        set fd [open [concat | git log --no-color --no-graph -z --pretty=raw
> +                        $show_notes --parents --boundary $args "--"
> +                        $vfilelimit($view)] r]
>      } err]} {
>          error_popup "[mc "Error executing git log:"] $err"
>          return

Re: [PATCH v3 1/4] log: fix memory leak if --graph is passed multiple times

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

> +void graph_clear(struct git_graph *graph)
> +{
> +	if (!graph)
> +		return;
> +
> +	free(graph->columns);
> +	free(graph->new_columns);
> +	free(graph->mapping);
> +	free(graph->old_mapping);

These four are pointer members that graph_init() allocates storage
for, so releasing resources held by these four members in clear()
makes it symmetrical.

The .columns and .new_columns members are arrays of "struct column",
but each element in the array does not own any external memory, so
freeing the arrays is sufficient.  The .mapping and .old_mapping
members are arrays of int and freeing them is also sufficient.

Looking good.

> +	free(graph);
> +}
> +
>  static void graph_update_state(struct git_graph *graph, enum graph_state s)
>  {
>  	graph->prev_state = graph->state;
> diff --git a/graph.h b/graph.h
> index 8313e293c7..e88632a014 100644
> --- a/graph.h
> +++ b/graph.h
> @@ -139,6 +139,11 @@ void graph_set_column_colors(const char **colors, unsigned short colors_max);
>   */
>  struct git_graph *graph_init(struct rev_info *opt);
>  
> +/*
> + * Free a struct git_graph.
> + */
> +void graph_clear(struct git_graph *graph);
> +
>  /*
>   * Update a git_graph with a new commit.
>   * This will cause the graph to begin outputting lines for the new commit
> diff --git a/revision.c b/revision.c
> index ad4286fbdd..816061f3d9 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2426,6 +2426,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  	} else if (!strcmp(arg, "--graph")) {
>  		revs->topo_order = 1;
>  		revs->rewrite_parents = 1;
> +		graph_clear(revs->graph);
>  		revs->graph = graph_init(revs);

Makes sense.  Will queue.

>  	} else if (!strcmp(arg, "--encode-email-headers")) {
>  		revs->encode_email_headers = 1;

Re: [PATCH v3 2/4] log: add a --no-graph option

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

> It's useful to be able to countermand a previous --graph option, for
> example if `git log --graph` is run via an alias.
>
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
> v3: don't pass a regular expression with parentheses to grep, so that
> the tests pass in all configurations on GitHub
> ---
>  builtin/blame.c    |  1 +
>  builtin/shortlog.c |  1 +
>  revision.c         | 19 ++++++++++---
>  revision.h         |  1 +
>  t/t4202-log.sh     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 87 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 7fafeac408..ef831de5ac 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -934,6 +934,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  		parse_revision_opt(&revs, &ctx, options, blame_opt_usage);
>  	}
>  parse_done:
> +	revision_opts_finish(&revs);

This ...

> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index e7f7af5de3..228d782754 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -388,6 +388,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
>  		parse_revision_opt(&rev, &ctx, options, shortlog_usage);
>  	}
>  parse_done:
> +	revision_opts_finish(&rev);
>  	argc = parse_options_end(&ctx);
>  
>  	if (nongit && argc > 1) {

... and this.  It is a bit scary that we have to make sure all the
users of parse_revision_opt() users need to call this new helper.
Didn't we recently gain new documentation to help novices write
their first revision-traversal-API-using program?  Does it need to
be updated for this change (I didn't check)?

> diff --git a/revision.c b/revision.c
> index 816061f3d9..a39fd1c278 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2424,10 +2424,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  		revs->pretty_given = 1;
>  		revs->abbrev_commit = 1;
>  	} else if (!strcmp(arg, "--graph")) {
> -		revs->topo_order = 1;
> -		revs->rewrite_parents = 1;
>  		graph_clear(revs->graph);
>  		revs->graph = graph_init(revs);
> +	} else if (!strcmp(arg, "--no-graph")) {
> +		graph_clear(revs->graph);
> +		revs->graph = NULL;
>  	} else if (!strcmp(arg, "--encode-email-headers")) {
>  		revs->encode_email_headers = 1;
>  	} else if (!strcmp(arg, "--no-encode-email-headers")) {
> @@ -2524,8 +2525,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  			unkv[(*unkc)++] = arg;
>  		return opts;
>  	}
> -	if (revs->graph && revs->track_linear)
> -		die(_("options '%s' and '%s' cannot be used together"), "--show-linear-break", "--graph");
>  
>  	return 1;
>  }

As a later "--no" can clear an earlier "--graph", we cannot
incrementally check if options are compatible, until the end, at
which time we can be sure that "--graph" is being asked.

> +void revision_opts_finish(struct rev_info *revs)
> +{
> +	if (revs->graph && revs->track_linear)
> +		die(_("options '%s' and '%s' cannot be used together"), "--show-linear-break", "--graph");

Inherited from the original, but we may want to wrap this line.

> +	if (revs->graph) {
> +		revs->topo_order = 1;
> +		revs->rewrite_parents = 1;
> +	}
> +}
> +

OK.

> @@ -2786,6 +2796,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  			break;
>  		}
>  	}
> +	revision_opts_finish(revs);

OK.

Will queue.  Thanks.

Re: [PATCH v3 4/4] gitk: pass --no-graph to `git log`

[Alex Henrie]

On Fri, Feb 11, 2022 at 11:12 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> >  gitk-git/gitk | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
>
> Please base a patch on gitk part to base on Paul's tree, not mine,
> meaning that the first few lines of diff should begin like so:
>
>         diff --git a/gitk b/gitk
>         index ...
>         --- a/gitk
>         +++ b/gitk
>
> and not as part of the series.

Okay. I'll hang onto this patch until the previous one in the series
is accepted.

> What the first two patches want to do is a good thing regardless, so
> I'll take a deeper look at them and queue them.  I am very skeptical
> to log.graph=yes/no configuration for obvious reasons that setting
> such a variable *will* break existing tools and users.  It is not
> even "it might break but we don't know until we try", as this patch
> loudly demonstrates.

What if we make log.graph=true also require feature.experimental=true?
The log.graph option would really be a useful feature for people who
use Git exclusively from the CLI without any external tools. It seems
that the main challenge is how to give others time to adjust.

Thanks for all the feedback,

-Alex

Re: [PATCH v3 2/4] log: add a --no-graph option

[Alex Henrie]

On Fri, Feb 11, 2022 at 12:02 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > --- a/builtin/blame.c
> > +++ b/builtin/blame.c
> > @@ -934,6 +934,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
> >               parse_revision_opt(&revs, &ctx, options, blame_opt_usage);
> >       }
> >  parse_done:
> > +     revision_opts_finish(&revs);
>
> This ...
>
> > diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> > index e7f7af5de3..228d782754 100644
> > --- a/builtin/shortlog.c
> > +++ b/builtin/shortlog.c
> > @@ -388,6 +388,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
> >               parse_revision_opt(&rev, &ctx, options, shortlog_usage);
> >       }
> >  parse_done:
> > +     revision_opts_finish(&rev);
> >       argc = parse_options_end(&ctx);
> >
> >       if (nongit && argc > 1) {
>
> ... and this.  It is a bit scary that we have to make sure all the
> users of parse_revision_opt() users need to call this new helper.
> Didn't we recently gain new documentation to help novices write
> their first revision-traversal-API-using program?  Does it need to
> be updated for this change (I didn't check)?

I don't see any documentation on how to use parse_revision_opt
directly; I only see documentation on how to use setup_revisions,
whose interface did not change.

Another approach would be to make a parse_rev_options_step function
that wraps parse_options_step and does the final steps when
parse_options_step returns PARSE_OPT_DONE. Would that be better?

> > diff --git a/revision.c b/revision.c
> > index 816061f3d9..a39fd1c278 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -2424,10 +2424,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
> >               revs->pretty_given = 1;
> >               revs->abbrev_commit = 1;
> >       } else if (!strcmp(arg, "--graph")) {
> > -             revs->topo_order = 1;
> > -             revs->rewrite_parents = 1;
> >               graph_clear(revs->graph);
> >               revs->graph = graph_init(revs);
> > +     } else if (!strcmp(arg, "--no-graph")) {
> > +             graph_clear(revs->graph);
> > +             revs->graph = NULL;
> >       } else if (!strcmp(arg, "--encode-email-headers")) {
> >               revs->encode_email_headers = 1;
> >       } else if (!strcmp(arg, "--no-encode-email-headers")) {
> > @@ -2524,8 +2525,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
> >                       unkv[(*unkc)++] = arg;
> >               return opts;
> >       }
> > -     if (revs->graph && revs->track_linear)
> > -             die(_("options '%s' and '%s' cannot be used together"), "--show-linear-break", "--graph");
> >
> >       return 1;
> >  }
>
> As a later "--no" can clear an earlier "--graph", we cannot
> incrementally check if options are compatible, until the end, at
> which time we can be sure that "--graph" is being asked.

Exactly. This is intentional, to avoid erroring out unnecessarily.

-Alex

Re: [PATCH v3 4/4] gitk: pass --no-graph to `git log`

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

> What if we make log.graph=true also require feature.experimental=true?

No.  feature.experimental is to give people an opt-in opportunity
for features that we are considering to enable by default.

> The log.graph option would really be a useful feature for people who
> use Git exclusively from the CLI without any external tools. It seems
> that the main challenge is how to give others time to adjust.

Those who want to see log by default must need to twaek their
configuration.  Instead of doing "git config log.graph true" and
breaking tools, they can do "git config alias.mylog 'log --graph'"
with the same ease, without breaking anything.

So...

Re: [PATCH v3 4/4] gitk: pass --no-graph to `git log`

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Alex Henrie <alexhenrie24@gmail.com> writes:
>
>> What if we make log.graph=true also require feature.experimental=true?
>
> No.  feature.experimental is to give people an opt-in opportunity
> for features that we are considering to enable by default.
>
>> The log.graph option would really be a useful feature for people who
>> use Git exclusively from the CLI without any external tools. It seems
>> that the main challenge is how to give others time to adjust.

Let me clarify the first point by stating it a bit differently.

feature.experimental is all about this:

    We have an idea for this new feature.  We made it useful, and
    also made it not to regress the end-user experience for those
    who do not need the new feature, to the best of our ability.
    But there may be use cases we failed to consider while doing
    so.  So let's ask early adopters, who may use Git in contexts
    that are very different from ours, to try testing it out in
    their daily use, to see if there are unexpected glitches.

You do not have to argue how the --graph feature may be useful for
character terminal users.  We already know it is, otherwise we
wouldn't have added it in the first place.

And arguing how --graph feature is useful does not help prove
anything, when at the issue is if it is a good idea to allow the
log.graph configuration variable to affect (unfortunate) scripts
people wrote around "git log", instead of using plumbing commands,
negatively.  We already know it will hurt to force everybody to
update their script to explicitly pass --no-graph on the command
line.  This series hasn't done any of the "not to regress to the
best of our ability" part.

If there were an agreement on the general direction to _forbid_ use
of "git log" in scripts, which would require coordinated efforts to
help people migrate over time, e.g.

 - improve plumbing by adding features that people piled only on
   "git log" without allowing plumbing users the same over time.

 - perhaps an automated way to convert scripts that use "git log" to
   instead use "git log --no-graph"

to help script writers migrate away from "git log", adding log.graph
configuration variable may become very a good idea.

But without such effort starting at the same time and gaining
consensus (or already underway), just adding such a variable to
break existing scripts would not be a good idea worth asking the
early adopters to test.  We already know it would break scripts.

Re: [PATCH v3 4/4] gitk: pass --no-graph to `git log`

[Alex Henrie]

On Fri, Feb 11, 2022 at 12:20 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > What if we make log.graph=true also require feature.experimental=true?
>
> No.  feature.experimental is to give people an opt-in opportunity
> for features that we are considering to enable by default.
>
> > The log.graph option would really be a useful feature for people who
> > use Git exclusively from the CLI without any external tools. It seems
> > that the main challenge is how to give others time to adjust.
>
> Those who want to see log by default must need to twaek their
> configuration.  Instead of doing "git config log.graph true" and
> breaking tools, they can do "git config alias.mylog 'log --graph'"
> with the same ease, without breaking anything.
>
> So...

Yeah, that's not a bad solution. I actually have `git graph` aliased to
`git log --graph --abbrev-commit --pretty=oneline` for this purpose.
If a lot of people are doing that, maybe a new command should be added
to Git itself. It seems like there's not much demand for that at the
moment though.

-Alex

Re: [PATCH v3 4/4] gitk: pass --no-graph to `git log`

[Alex Henrie]

On Fri, Feb 11, 2022 at 1:00 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Alex Henrie <alexhenrie24@gmail.com> writes:
> >
> >> What if we make log.graph=true also require feature.experimental=true?
> >
> > No.  feature.experimental is to give people an opt-in opportunity
> > for features that we are considering to enable by default.
> >
> >> The log.graph option would really be a useful feature for people who
> >> use Git exclusively from the CLI without any external tools. It seems
> >> that the main challenge is how to give others time to adjust.
>
> Let me clarify the first point by stating it a bit differently.
>
> feature.experimental is all about this:
>
>     We have an idea for this new feature.  We made it useful, and
>     also made it not to regress the end-user experience for those
>     who do not need the new feature, to the best of our ability.
>     But there may be use cases we failed to consider while doing
>     so.  So let's ask early adopters, who may use Git in contexts
>     that are very different from ours, to try testing it out in
>     their daily use, to see if there are unexpected glitches.
>
> You do not have to argue how the --graph feature may be useful for
> character terminal users.  We already know it is, otherwise we
> wouldn't have added it in the first place.
>
> And arguing how --graph feature is useful does not help prove
> anything, when at the issue is if it is a good idea to allow the
> log.graph configuration variable to affect (unfortunate) scripts
> people wrote around "git log", instead of using plumbing commands,
> negatively.  We already know it will hurt to force everybody to
> update their script to explicitly pass --no-graph on the command
> line.  This series hasn't done any of the "not to regress to the
> best of our ability" part.
>
> If there were an agreement on the general direction to _forbid_ use
> of "git log" in scripts, which would require coordinated efforts to
> help people migrate over time, e.g.
>
>  - improve plumbing by adding features that people piled only on
>    "git log" without allowing plumbing users the same over time.
>
>  - perhaps an automated way to convert scripts that use "git log" to
>    instead use "git log --no-graph"
>
> to help script writers migrate away from "git log", adding log.graph
> configuration variable may become very a good idea.
>
> But without such effort starting at the same time and gaining
> consensus (or already underway), just adding such a variable to
> break existing scripts would not be a good idea worth asking the
> early adopters to test.  We already know it would break scripts.

Okay. Thanks for the explanation!

-Alex