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

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

[Alex Henrie]

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 revision.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/revision.c b/revision.c
index ad4286fbdd..c03c387edd 100644
--- a/revision.c
+++ b/revision.c
@@ -2424,9 +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;
-		revs->graph = graph_init(revs);
+		if (!revs->graph) {
+			revs->topo_order = 1;
+			revs->rewrite_parents = 1;
+			revs->graph = graph_init(revs);
+		}
 	} else if (!strcmp(arg, "--encode-email-headers")) {
 		revs->encode_email_headers = 1;
 	} else if (!strcmp(arg, "--no-encode-email-headers")) {
-- 
2.32.0.2645.gc109162a1f

[PATCH 2/3] log: add a config option for --graph

[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.

It would be nice to have a --no-graph command line option to countermand
log.graph=true, but it's not clear how that would work with topo_order
and rewrite_parents, which can be set by other options. In any case, it
is still possible to countermand log.graph=true per invocation with
`git -c log.graph=false log`.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 Documentation/config/log.txt |  5 +++++
 Documentation/git-log.txt    |  5 +++++
 builtin/log.c                | 17 +++++++++++++++++
 t/t4202-log.sh               | 35 +++++++++++++++++++++++++++++++++++
 4 files changed, 62 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..1f0db54c84 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -35,6 +35,7 @@
 #include "repository.h"
 #include "commit-reach.h"
 #include "range-diff.h"
+#include "graph.h"
 
 #define MAIL_DEFAULT_WRAP 72
 #define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100
@@ -48,6 +49,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;
@@ -234,6 +236,17 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 		read_mailmap(rev->mailmap);
 	}
 
+	if (default_graph &&
+	    !rev->graph &&
+	    !rev->track_linear &&
+	    !rev->reverse &&
+	    !rev->reflog_info &&
+	    !rev->no_walk) {
+		rev->topo_order = 1;
+		rev->rewrite_parents = 1;
+		rev->graph = graph_init(rev);
+	}
+
 	if (rev->pretty_given && rev->commit_format == CMIT_FMT_RAW) {
 		/*
 		 * "log --pretty=raw" is special; ignore UI oriented
@@ -519,6 +532,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/t/t4202-log.sh b/t/t4202-log.sh
index 5049559861..3b6f7aff23 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1671,6 +1671,41 @@ test_expect_success 'log --graph with --name-only' '
 	test_cmp_graph --name-only tangle..reach
 '
 
+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 '--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 '--reverse ignores log.graph' '
+	git log --reverse >expect &&
+	test_config log.graph true &&
+	git log --reverse >actual &&
+	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 '--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 'dotdot is a parent directory' '
 	mkdir -p a/b &&
 	( echo sixth && echo fifth ) >expect &&
-- 
2.32.0.2645.gc109162a1f

Re: [PATCH 2/3] log: add a config option for --graph

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

> +	if (default_graph &&
> +	    !rev->graph &&

This part I can see why we want to check ;-)

> +	    !rev->track_linear &&
> +	    !rev->reverse &&
> +	    !rev->reflog_info &&
> +	    !rev->no_walk) {

But this makes me wonder how we plan to keep this list of "if the
user asked for any of these options, we shouldn't turn graph on by
default" up-to-date.  The scheme looks brittle.

Also, what would happen when a developer wants to add, say
log.reverse, configuration variable in the future?  I can see the
block to do the equivalent of this for .log.reverse begins with

	if (default_reverse &&
	    !rev->reverse &&

but I am not sure what other conditions need to be checked,
especially with 'graph'---should it check for !rev->graph or
default_graph of both?  Are we playing with a potential
combinatorial explosion?

> +		rev->topo_order = 1;
> +		rev->rewrite_parents = 1;
> +		rev->graph = graph_init(rev);

In any case, it probably makes sense to encapsulate these three
lines in a small helper function "when --graph is asked for on a
rev, call this function" in the PREVIOUS step of the series, and
change this patch to read more like

	if (default_graph &&
	    /* check for incompatible options */
	    !rev->track_linear && !rev->reverse && ...)
		rev_add_graph_option(rev);

Most importantly, the helper should be one that handles "wow, we see
that rev->graph is already populated, what should we do?", and not
this caller.  And that helper can be called unconditionally by the
command line parser when it finds !strcmp(arg, "--graph") yields
true.

Thanks.

Re: [PATCH 2/3] log: add a config option for --graph

[Alex Henrie]

On Wed, Feb 9, 2022 at 11:25 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > +     if (default_graph &&
> > +         !rev->graph &&
>
> This part I can see why we want to check ;-)
>
> > +         !rev->track_linear &&
> > +         !rev->reverse &&
> > +         !rev->reflog_info &&
> > +         !rev->no_walk) {
>
> But this makes me wonder how we plan to keep this list of "if the
> user asked for any of these options, we shouldn't turn graph on by
> default" up-to-date.  The scheme looks brittle.
>
> Also, what would happen when a developer wants to add, say
> log.reverse, configuration variable in the future?  I can see the
> block to do the equivalent of this for .log.reverse begins with
>
>         if (default_reverse &&
>             !rev->reverse &&
>
> but I am not sure what other conditions need to be checked,
> especially with 'graph'---should it check for !rev->graph or
> default_graph of both?  Are we playing with a potential
> combinatorial explosion?

In principle there's no reason why --graph and --reverse can't be
compatible. However, if both log.graph and log.reverse config options
are added while they are still incompatible, I think using them
together should trigger the existing option compatibility error in the
setup_revisions function:

        if (revs->reverse && revs->graph)
                die(_("options '%s' and '%s' cannot be used
together"), "--reverse", "--graph");

That said, we should make sure that --graph on the command line
overrides log.reverse in the config file (instead of dying), and the
same for --reverse overriding log.graph. How about this:

1. In git_log_config, initialize default_graph and default_reverse by
parsing log.graph and log.reverse.

2. In cmd_log_init_defaults, initialize rev->graph_default to
default_graph and rev->reverse_default to default_reverse.

3. In handle_revision_opt, set revs->graph if --graph is given, clear
revs->graph if --no-graph is given, and in either case clear
revs->graph_default. Set revs->reverse and revs->reverse_default
likewise.

4. In setup_revisions, if revs->graph_default is still true, set
revs->graph based on revs->reverse and revs->reverse_default. Set
revs->reverse likewise.

5. In setup_revisions, call a new revision_opts_finish function that
sets revs->topo_order and revs->rewrite_parents if necessary based on
the final value of revs->graph.

I think that would ensure that command line options interact correctly
with config options, plus it would allow adding a --no-graph command
line option, and the logic to handle defaults would be clearly linked
to the existing incompatible option checks. I realize that it will
take a bit of refactoring though. I'll send new patches shortly to
make it more clear.

Please let me know if any of your other feedback is still relevant in v2.

-Alex

[PATCH 3/3] gitk: set log.graph=false when running `git log`

[Alex Henrie]

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

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 23d9dd1fe0..1d8a5ff622 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -411,8 +411,9 @@ 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 -c log.graph=false log --no-color -z \
+                        --pretty=raw $show_notes --parents --boundary $args \
+                        "--" $files] r]
     } err]} {
         error_popup "[mc "Error executing git log:"] $err"
         return 0
@@ -559,8 +560,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 -c log.graph=false log --no-color -z \
+                        --pretty=raw $show_notes --parents --boundary $args \
+                        "--" $vfilelimit($view)] r]
     } err]} {
         error_popup "[mc "Error executing git log:"] $err"
         return
-- 
2.32.0.2645.gc109162a1f

Re: [PATCH 3/3] gitk: set log.graph=false when running `git log`

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

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

This may handle "gitk", but what about thousands other scripts
people have developed around "git log", I have to wonder.



> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 23d9dd1fe0..1d8a5ff622 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -411,8 +411,9 @@ 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 -c log.graph=false log --no-color -z \
> +                        --pretty=raw $show_notes --parents --boundary $args \
> +                        "--" $files] r]
>      } err]} {
>          error_popup "[mc "Error executing git log:"] $err"
>          return 0
> @@ -559,8 +560,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 -c log.graph=false log --no-color -z \
> +                        --pretty=raw $show_notes --parents --boundary $args \
> +                        "--" $vfilelimit($view)] r]
>      } err]} {
>          error_popup "[mc "Error executing git log:"] $err"
>          return

Re: [PATCH 3/3] gitk: set log.graph=false when running `git log`

[Alex Henrie]

On Wed, Feb 9, 2022 at 11:26 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> This may handle "gitk", but what about thousands other scripts
> people have developed around "git log", I have to wonder.

That's true, but every new preference carries that risk, and there's
not much we can do about it. Users who set log.graph=true will just
have to accept that it won't be compatible with every tool in the
wild.

-Alex

Re: [PATCH 3/3] gitk: set log.graph=false when running `git log`

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

> On Wed, Feb 9, 2022 at 11:26 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> This may handle "gitk", but what about thousands other scripts
>> people have developed around "git log", I have to wonder.
>
> That's true, but every new preference carries that risk, and there's
> not much we can do about it. Users who set log.graph=true will just
> have to accept that it won't be compatible with every tool in the
> wild.

I wouldn't call something that is known to break people before it is
released to the wild a "risk", though.

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

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>  revision.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index ad4286fbdd..c03c387edd 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2424,9 +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;
> -		revs->graph = graph_init(revs);
> +		if (!revs->graph) {
> +			revs->topo_order = 1;
> +			revs->rewrite_parents = 1;
> +			revs->graph = graph_init(revs);
> +		}

I understand the refs->graph part but are there ways to turn off
topo_order or rewrite_parents with _other_ options?  I.e.

    git log --graph --another-option --graph

if --another-option flips either bits off, would make the graph
code misbehave because it requires both of these bits set.

I think this is safe in the corrent code, simply because there do
not seem to be a way to unset these bits once they are set, but
I am not sure if that is something we want to rely on.

I think an ideal endgame should look more like

	} else if (!strcmp(arg, "--graph")) {
		revs->topo_order = 1;
		revs->rewrite_parents = 1;
+		graph_clear(revs->graph);
		revs->graph = graph_init(revs);

where graph_clear() is to release the resource held by the git_graph
struct that was previously prepared (and possibly used), and becomes
a no-op when given NULL (just like free(NULL) is OK).

But if we want to punt on introducing graph_clear(), perhaps

	} else if (!strcmp(arg, "--graph")) {
		revs->topo_order = 1;
		revs->rewrite_parents = 1;
		if (!revs->graph)
			revs->graph = graph_init(revs);

would be closer to the ideal endgame (and have an in-code comment to
hint future developers that (1) we are aware that this is not ideal,
and that (2) the right way is to clear the previously allocated one
before doing another init).

Thanks.

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