{"thread":{"id":"57406","subject":"[PATCH v3 1/4] log: fix memory leak if --graph is passed multiple times","startedAt":"2022-02-11T16:37:22Z","lastAt":"2022-02-11T20:12:02Z","messageCount":13,"participants":["Alex Henrie","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"448222","messageId":"20220211163627.598166-1-alexhenrie24@gmail.com","threadId":"57406","inReplyTo":null,"subject":"[PATCH v3 1/4] log: fix memory leak if --graph is passed multiple times","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2022-02-11T16:36:24Z","receivedAt":"2022-02-11T16:37:22Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\nv3: no changes\n---\n graph.c    | 12 ++++++++++++\n graph.h    |  5 +++++\n revision.c |  1 +\n 3 files changed, 18 insertions(+)\n\ndiff --git a/graph.c b/graph.c\nindex e3828eb8f2..568b6e7cd4 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -401,6 +401,18 @@ struct git_graph *graph_init(struct rev_info *opt)\n \treturn graph;\n }\n \n+void graph_clear(struct git_graph *graph)\n+{\n+\tif (!graph)\n+\t\treturn;\n+\n+\tfree(graph->columns);\n+\tfree(graph->new_columns);\n+\tfree(graph->mapping);\n+\tfree(graph->old_mapping);\n+\tfree(graph);\n+}\n+\n static void graph_update_state(struct git_graph *graph, enum graph_state s)\n {\n \tgraph->prev_state = graph->state;\ndiff --git a/graph.h b/graph.h\nindex 8313e293c7..e88632a014 100644\n--- a/graph.h\n+++ b/graph.h\n@@ -139,6 +139,11 @@ void graph_set_column_colors(const char **colors, unsigned short colors_max);\n  */\n struct git_graph *graph_init(struct rev_info *opt);\n \n+/*\n+ * Free a struct git_graph.\n+ */\n+void graph_clear(struct git_graph *graph);\n+\n /*\n  * Update a git_graph with a new commit.\n  * This will cause the graph to begin outputting lines for the new commit\ndiff --git a/revision.c b/revision.c\nindex ad4286fbdd..816061f3d9 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2426,6 +2426,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--graph\")) {\n \t\trevs->topo_order = 1;\n \t\trevs->rewrite_parents = 1;\n+\t\tgraph_clear(revs->graph);\n \t\trevs->graph = graph_init(revs);\n \t} else if (!strcmp(arg, \"--encode-email-headers\")) {\n \t\trevs->encode_email_headers = 1;\n-- \n2.35.1\n\n"},{"id":"448223","messageId":"20220211163627.598166-2-alexhenrie24@gmail.com","threadId":"57406","inReplyTo":"20220211163627.598166-1-alexhenrie24@gmail.com","subject":"[PATCH v3 2/4] log: add a --no-graph option","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2022-02-11T16:36:25Z","receivedAt":"2022-02-11T16:37:26Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"It's useful to be able to countermand a previous --graph option, for\nexample if `git log --graph` is run via an alias.\n\nSigned-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\nv3: don't pass a regular expression with parentheses to grep, so that\nthe tests pass in all configurations on GitHub\n---\n builtin/blame.c    |  1 +\n builtin/shortlog.c |  1 +\n revision.c         | 19 ++++++++++---\n revision.h         |  1 +\n t/t4202-log.sh     | 69 ++++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 87 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 7fafeac408..ef831de5ac 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -934,6 +934,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\tparse_revision_opt(&revs, &ctx, options, blame_opt_usage);\n \t}\n parse_done:\n+\trevision_opts_finish(&revs);\n \tno_whole_file_rename = !revs.diffopt.flags.follow_renames;\n \txdl_opts |= revs.diffopt.xdl_opts & XDF_INDENT_HEURISTIC;\n \trevs.diffopt.flags.follow_renames = 0;\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex e7f7af5de3..228d782754 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -388,6 +388,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \t\tparse_revision_opt(&rev, &ctx, options, shortlog_usage);\n \t}\n parse_done:\n+\trevision_opts_finish(&rev);\n \targc = parse_options_end(&ctx);\n \n \tif (nongit && argc > 1) {\ndiff --git a/revision.c b/revision.c\nindex 816061f3d9..a39fd1c278 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2424,10 +2424,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->pretty_given = 1;\n \t\trevs->abbrev_commit = 1;\n \t} else if (!strcmp(arg, \"--graph\")) {\n-\t\trevs->topo_order = 1;\n-\t\trevs->rewrite_parents = 1;\n \t\tgraph_clear(revs->graph);\n \t\trevs->graph = graph_init(revs);\n+\t} else if (!strcmp(arg, \"--no-graph\")) {\n+\t\tgraph_clear(revs->graph);\n+\t\trevs->graph = NULL;\n \t} else if (!strcmp(arg, \"--encode-email-headers\")) {\n \t\trevs->encode_email_headers = 1;\n \t} else if (!strcmp(arg, \"--no-encode-email-headers\")) {\n@@ -2524,8 +2525,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\t\tunkv[(*unkc)++] = arg;\n \t\treturn opts;\n \t}\n-\tif (revs->graph && revs->track_linear)\n-\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--show-linear-break\", \"--graph\");\n \n \treturn 1;\n }\n@@ -2544,6 +2543,17 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,\n \tctx->argc -= n;\n }\n \n+void revision_opts_finish(struct rev_info *revs)\n+{\n+\tif (revs->graph && revs->track_linear)\n+\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--show-linear-break\", \"--graph\");\n+\n+\tif (revs->graph) {\n+\t\trevs->topo_order = 1;\n+\t\trevs->rewrite_parents = 1;\n+\t}\n+}\n+\n static int for_each_bisect_ref(struct ref_store *refs, each_ref_fn fn,\n \t\t\t       void *cb_data, const char *term)\n {\n@@ -2786,6 +2796,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tbreak;\n \t\t}\n \t}\n+\trevision_opts_finish(revs);\n \n \tif (prune_data.nr) {\n \t\t/*\ndiff --git a/revision.h b/revision.h\nindex 3f66147bfd..5a507db202 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -372,6 +372,7 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,\n #define REVARG_COMMITTISH 02\n int handle_revision_arg(const char *arg, struct rev_info *revs,\n \t\t\tint flags, unsigned revarg_opt);\n+void revision_opts_finish(struct rev_info *revs);\n \n /**\n  * Reset the flags used by the revision walking api. You can use this to do\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex dc884107de..a7d5edf720 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1671,6 +1671,75 @@ test_expect_success 'log --graph with --name-only' '\n \ttest_cmp_graph --name-only tangle..reach\n '\n \n+test_expect_success '--no-graph countermands --graph' '\n+\tgit log >expect &&\n+\tgit log --graph --no-graph >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--graph countermands --no-graph' '\n+\tgit log --graph >expect &&\n+\tgit log --no-graph --graph >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--no-graph does not unset --topo-order' '\n+\tgit log --topo-order >expect &&\n+\tgit log --topo-order --no-graph >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--no-graph does not unset --parents' '\n+\tgit log --parents >expect &&\n+\tgit log --parents --no-graph >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--reverse and --graph conflict' '\n+\ttest_must_fail git log --reverse --graph 2>stderr &&\n+\ttest_i18ngrep \"cannot be used together\" stderr\n+'\n+\n+test_expect_success '--reverse --graph --no-graph works' '\n+\tgit log --reverse >expect &&\n+\tgit log --reverse --graph --no-graph >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--show-linear-break and --graph conflict' '\n+\ttest_must_fail git log --show-linear-break --graph 2>stderr &&\n+\ttest_i18ngrep \"cannot be used together\" stderr\n+'\n+\n+test_expect_success '--show-linear-break --graph --no-graph works' '\n+\tgit log --show-linear-break >expect &&\n+\tgit log --show-linear-break --graph --no-graph >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--no-walk and --graph conflict' '\n+\ttest_must_fail git log --no-walk --graph 2>stderr &&\n+\ttest_i18ngrep \"cannot be used together\" stderr\n+'\n+\n+test_expect_success '--no-walk --graph --no-graph works' '\n+\tgit log --no-walk >expect &&\n+\tgit log --no-walk --graph --no-graph >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--walk-reflogs and --graph conflict' '\n+\ttest_must_fail git log --walk-reflogs --graph 2>stderr &&\n+\t(test_i18ngrep \"cannot combine\" stderr ||\n+\t\ttest_i18ngrep \"cannot be used together\" stderr)\n+'\n+\n+test_expect_success '--walk-reflogs --graph --no-graph works' '\n+\tgit log --walk-reflogs >expect &&\n+\tgit log --walk-reflogs --graph --no-graph >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'dotdot is a parent directory' '\n \tmkdir -p a/b &&\n \t( echo sixth && echo fifth ) >expect &&\n-- \n2.35.1\n\n"},{"id":"448224","messageId":"20220211163627.598166-4-alexhenrie24@gmail.com","threadId":"57406","inReplyTo":"20220211163627.598166-1-alexhenrie24@gmail.com","subject":"[PATCH v3 4/4] gitk: pass --no-graph to `git log`","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2022-02-11T16:36:27Z","receivedAt":"2022-02-11T16:37:28Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\nv3: no changes\n---\n gitk-git/gitk | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/gitk-git/gitk b/gitk-git/gitk\nindex 23d9dd1fe0..24099ce0b8 100755\n--- a/gitk-git/gitk\n+++ b/gitk-git/gitk\n@@ -411,8 +411,8 @@ proc start_rev_list {view} {\n     }\n \n     if {[catch {\n-        set fd [open [concat | git log --no-color -z --pretty=raw $show_notes \\\n-                        --parents --boundary $args \"--\" $files] r]\n+        set fd [open [concat | git log --no-color --no-graph -z --pretty=raw \\\n+                        $show_notes --parents --boundary $args \"--\" $files] r]\n     } err]} {\n         error_popup \"[mc \"Error executing git log:\"] $err\"\n         return 0\n@@ -559,8 +559,9 @@ proc updatecommits {} {\n         set args $vorigargs($view)\n     }\n     if {[catch {\n-        set fd [open [concat | git log --no-color -z --pretty=raw $show_notes \\\n-                        --parents --boundary $args \"--\" $vfilelimit($view)] r]\n+        set fd [open [concat | git log --no-color --no-graph -z --pretty=raw\n+                        $show_notes --parents --boundary $args \"--\"\n+                        $vfilelimit($view)] r]\n     } err]} {\n         error_popup \"[mc \"Error executing git log:\"] $err\"\n         return\n-- \n2.35.1\n\n"},{"id":"448225","messageId":"20220211163627.598166-3-alexhenrie24@gmail.com","threadId":"57406","inReplyTo":"20220211163627.598166-1-alexhenrie24@gmail.com","subject":"[PATCH v3 3/4] log: add a log.graph config option","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2022-02-11T16:36:26Z","receivedAt":"2022-02-11T16:37:28Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"A coworker recently asked me how to turn on --graph by default in\n`git log`. I googled it and found that several people have asked that\nbefore on Stack Overflow, with no good solution:\nhttps://stackoverflow.com/questions/43555256/how-do-i-make-git-log-graph-the-default\n\nAdd a log.graph option to turn on graph mode in the absence of any\nincompatible options.\n\nSigned-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\nv3: no changes\n---\n Documentation/config/log.txt |  5 +++++\n Documentation/git-log.txt    |  5 +++++\n builtin/log.c                |  6 ++++++\n revision.c                   | 10 +++++++++\n revision.h                   |  2 ++\n t/t4202-log.sh               | 42 ++++++++++++++++++++++++++++++++++++\n 6 files changed, 70 insertions(+)\n\ndiff --git a/Documentation/config/log.txt b/Documentation/config/log.txt\nindex 456eb07800..3e356cfce6 100644\n--- a/Documentation/config/log.txt\n+++ b/Documentation/config/log.txt\n@@ -35,6 +35,11 @@ log.follow::\n \ti.e. it cannot be used to follow multiple files and does not work well\n \ton non-linear history.\n \n+log.graph::\n+\tIf true, makes linkgit:git-log[1], linkgit:git-show[1], and\n+\tlinkgit:git-whatchanged[1] assume `--graph` unless an incompatible\n+\toption is also specified.\n+\n log.graphColors::\n \tA list of colors, separated by commas, that can be used to draw\n \thistory lines in `git log --graph`.\ndiff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\nindex 20e87cecf4..7e9e0f8afe 100644\n--- a/Documentation/git-log.txt\n+++ b/Documentation/git-log.txt\n@@ -214,6 +214,11 @@ log.follow::\n \ti.e. it cannot be used to follow multiple files and does not work well\n \ton non-linear history.\n \n+log.graph::\n+\tIf `true`, `git log` and related commands will act as if the\n+\t`--graph` option was passed to them unless an incompatible option is\n+\talso specified.\n+\n log.showRoot::\n \tIf `false`, `git log` and related commands will not treat the\n \tinitial commit as a big creation event.  Any root commits in\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 4b493408cc..5eaefab046 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -48,6 +48,7 @@ static int default_show_root = 1;\n static int default_follow;\n static int default_show_signature;\n static int default_encode_email_headers = 1;\n+static int default_graph;\n static int decoration_style;\n static int decoration_given;\n static int use_mailmap_config = 1;\n@@ -156,6 +157,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)\n \trev->show_signature = default_show_signature;\n \trev->encode_email_headers = default_encode_email_headers;\n \trev->diffopt.flags.allow_textconv = 1;\n+\trev->graph_default = default_graph;\n \n \tif (default_date_mode)\n \t\tparse_date_format(default_date_mode, &rev->date_mode);\n@@ -519,6 +521,10 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t\tdefault_show_signature = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"log.graph\")) {\n+\t\tdefault_graph = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \n \tif (grep_config(var, value, cb) < 0)\n \t\treturn -1;\ndiff --git a/revision.c b/revision.c\nindex a39fd1c278..55e12cd401 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2426,9 +2426,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--graph\")) {\n \t\tgraph_clear(revs->graph);\n \t\trevs->graph = graph_init(revs);\n+\t\trevs->graph_default = 0;\n \t} else if (!strcmp(arg, \"--no-graph\")) {\n \t\tgraph_clear(revs->graph);\n \t\trevs->graph = NULL;\n+\t\trevs->graph_default = 0;\n \t} else if (!strcmp(arg, \"--encode-email-headers\")) {\n \t\trevs->encode_email_headers = 1;\n \t} else if (!strcmp(arg, \"--no-encode-email-headers\")) {\n@@ -2796,6 +2798,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tbreak;\n \t\t}\n \t}\n+\tif (revs->graph_default &&\n+\t    !revs->graph &&\n+\t    /* check for incompatible options */\n+\t    !revs->track_linear &&\n+\t    !revs->reverse &&\n+\t    !revs->reflog_info &&\n+\t    !revs->no_walk)\n+\t\trevs->graph = graph_init(revs);\n \trevision_opts_finish(revs);\n \n \tif (prune_data.nr) {\ndiff --git a/revision.h b/revision.h\nindex 5a507db202..62a0ef4c53 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -249,6 +249,8 @@ struct rev_info {\n \n \t/* Display history graph */\n \tstruct git_graph *graph;\n+\t/* whether to initialize graph if it has not been initialized already */\n+\tint graph_default;\n \n \t/* special limits */\n \tint skip_count;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex a7d5edf720..9f052ccce6 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1695,6 +1695,20 @@ test_expect_success '--no-graph does not unset --parents' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'log.graph=true behaves like --graph' '\n+\tgit log --graph >expect &&\n+\ttest_config log.graph true &&\n+\tgit log >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--no-graph countermands log.graph=true' '\n+\tgit log >expect &&\n+\ttest_config log.graph true &&\n+\tgit log --no-graph >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--reverse and --graph conflict' '\n \ttest_must_fail git log --reverse --graph 2>stderr &&\n \ttest_i18ngrep \"cannot be used together\" stderr\n@@ -1706,6 +1720,13 @@ test_expect_success '--reverse --graph --no-graph works' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--reverse ignores log.graph' '\n+\tgit log --reverse >expect &&\n+\ttest_config log.graph true &&\n+\tgit log --reverse >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--show-linear-break and --graph conflict' '\n \ttest_must_fail git log --show-linear-break --graph 2>stderr &&\n \ttest_i18ngrep \"cannot be used together\" stderr\n@@ -1717,6 +1738,13 @@ test_expect_success '--show-linear-break --graph --no-graph works' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--show-linear-break ignores log.graph' '\n+\tgit log --show-linear-break >expect &&\n+\ttest_config log.graph true &&\n+\tgit log --show-linear-break >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--no-walk and --graph conflict' '\n \ttest_must_fail git log --no-walk --graph 2>stderr &&\n \ttest_i18ngrep \"cannot be used together\" stderr\n@@ -1728,6 +1756,13 @@ test_expect_success '--no-walk --graph --no-graph works' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--no-walk ignores log.graph' '\n+\tgit log --no-walk >expect &&\n+\ttest_config log.graph true &&\n+\tgit log --no-walk >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--walk-reflogs and --graph conflict' '\n \ttest_must_fail git log --walk-reflogs --graph 2>stderr &&\n \t(test_i18ngrep \"cannot combine\" stderr ||\n@@ -1740,6 +1775,13 @@ test_expect_success '--walk-reflogs --graph --no-graph works' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--walk-reflogs ignores log.graph' '\n+\tgit log --walk-reflogs >expect &&\n+\ttest_config log.graph true &&\n+\tgit log --walk-reflogs >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'dotdot is a parent directory' '\n \tmkdir -p a/b &&\n \t( echo sixth && echo fifth ) >expect &&\n-- \n2.35.1\n\n"},{"id":"448234","messageId":"xmqq1r09clxl.fsf@gitster.g","threadId":"57406","inReplyTo":"20220211163627.598166-4-alexhenrie24@gmail.com","subject":"Re: [PATCH v3 4/4] gitk: pass --no-graph to `git log`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-11T18:12:06Z","receivedAt":"2022-02-11T18:12:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie <alexhenrie24@gmail.com> writes:\n\n> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n> ---\n> v3: no changes\n> ---\n>  gitk-git/gitk | 9 +++++----\n>  1 file changed, 5 insertions(+), 4 deletions(-)\n\nPlease base a patch on gitk part to base on Paul's tree, not mine,\nmeaning that the first few lines of diff should begin like so:\n\n\tdiff --git a/gitk b/gitk\n\tindex ...\n\t--- a/gitk\n\t+++ b/gitk\n\nand not as part of the series.\n\nWhat the first two patches want to do is a good thing regardless, so\nI'll take a deeper look at them and queue them.  I am very skeptical\nto log.graph=yes/no configuration for obvious reasons that setting\nsuch a variable *will* break existing tools and users.  It is not\neven \"it might break but we don't know until we try\", as this patch\nloudly demonstrates.\n\nThanks.\n\n> diff --git a/gitk-git/gitk b/gitk-git/gitk\n> index 23d9dd1fe0..24099ce0b8 100755\n> --- a/gitk-git/gitk\n> +++ b/gitk-git/gitk\n> @@ -411,8 +411,8 @@ proc start_rev_list {view} {\n>      }\n>  \n>      if {[catch {\n> -        set fd [open [concat | git log --no-color -z --pretty=raw $show_notes \\\n> -                        --parents --boundary $args \"--\" $files] r]\n> +        set fd [open [concat | git log --no-color --no-graph -z --pretty=raw \\\n> +                        $show_notes --parents --boundary $args \"--\" $files] r]\n>      } err]} {\n>          error_popup \"[mc \"Error executing git log:\"] $err\"\n>          return 0\n> @@ -559,8 +559,9 @@ proc updatecommits {} {\n>          set args $vorigargs($view)\n>      }\n>      if {[catch {\n> -        set fd [open [concat | git log --no-color -z --pretty=raw $show_notes \\\n> -                        --parents --boundary $args \"--\" $vfilelimit($view)] r]\n> +        set fd [open [concat | git log --no-color --no-graph -z --pretty=raw\n> +                        $show_notes --parents --boundary $args \"--\"\n> +                        $vfilelimit($view)] r]\n>      } err]} {\n>          error_popup \"[mc \"Error executing git log:\"] $err\"\n>          return\n"},{"id":"448235","messageId":"xmqqsfspb5jz.fsf@gitster.g","threadId":"57406","inReplyTo":"20220211163627.598166-1-alexhenrie24@gmail.com","subject":"Re: [PATCH v3 1/4] log: fix memory leak if --graph is passed multiple times","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-11T18:51:12Z","receivedAt":"2022-02-11T18:51:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie <alexhenrie24@gmail.com> writes:\n\n> +void graph_clear(struct git_graph *graph)\n> +{\n> +\tif (!graph)\n> +\t\treturn;\n> +\n> +\tfree(graph->columns);\n> +\tfree(graph->new_columns);\n> +\tfree(graph->mapping);\n> +\tfree(graph->old_mapping);\n\nThese four are pointer members that graph_init() allocates storage\nfor, so releasing resources held by these four members in clear()\nmakes it symmetrical.\n\nThe .columns and .new_columns members are arrays of \"struct column\",\nbut each element in the array does not own any external memory, so\nfreeing the arrays is sufficient.  The .mapping and .old_mapping\nmembers are arrays of int and freeing them is also sufficient.\n\nLooking good.\n\n> +\tfree(graph);\n> +}\n> +\n>  static void graph_update_state(struct git_graph *graph, enum graph_state s)\n>  {\n>  \tgraph->prev_state = graph->state;\n> diff --git a/graph.h b/graph.h\n> index 8313e293c7..e88632a014 100644\n> --- a/graph.h\n> +++ b/graph.h\n> @@ -139,6 +139,11 @@ void graph_set_column_colors(const char **colors, unsigned short colors_max);\n>   */\n>  struct git_graph *graph_init(struct rev_info *opt);\n>  \n> +/*\n> + * Free a struct git_graph.\n> + */\n> +void graph_clear(struct git_graph *graph);\n> +\n>  /*\n>   * Update a git_graph with a new commit.\n>   * This will cause the graph to begin outputting lines for the new commit\n> diff --git a/revision.c b/revision.c\n> index ad4286fbdd..816061f3d9 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2426,6 +2426,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n>  \t} else if (!strcmp(arg, \"--graph\")) {\n>  \t\trevs->topo_order = 1;\n>  \t\trevs->rewrite_parents = 1;\n> +\t\tgraph_clear(revs->graph);\n>  \t\trevs->graph = graph_init(revs);\n\nMakes sense.  Will queue.\n\n>  \t} else if (!strcmp(arg, \"--encode-email-headers\")) {\n>  \t\trevs->encode_email_headers = 1;\n"},{"id":"448236","messageId":"xmqqa6exb51o.fsf@gitster.g","threadId":"57406","inReplyTo":"20220211163627.598166-2-alexhenrie24@gmail.com","subject":"Re: [PATCH v3 2/4] log: add a --no-graph option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-11T19:02:11Z","receivedAt":"2022-02-11T19:02:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie <alexhenrie24@gmail.com> writes:\n\n> It's useful to be able to countermand a previous --graph option, for\n> example if `git log --graph` is run via an alias.\n>\n> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n> ---\n> v3: don't pass a regular expression with parentheses to grep, so that\n> the tests pass in all configurations on GitHub\n> ---\n>  builtin/blame.c    |  1 +\n>  builtin/shortlog.c |  1 +\n>  revision.c         | 19 ++++++++++---\n>  revision.h         |  1 +\n>  t/t4202-log.sh     | 69 ++++++++++++++++++++++++++++++++++++++++++++++\n>  5 files changed, 87 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 7fafeac408..ef831de5ac 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -934,6 +934,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n>  \t\tparse_revision_opt(&revs, &ctx, options, blame_opt_usage);\n>  \t}\n>  parse_done:\n> +\trevision_opts_finish(&revs);\n\nThis ...\n\n> diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> index e7f7af5de3..228d782754 100644\n> --- a/builtin/shortlog.c\n> +++ b/builtin/shortlog.c\n> @@ -388,6 +388,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n>  \t\tparse_revision_opt(&rev, &ctx, options, shortlog_usage);\n>  \t}\n>  parse_done:\n> +\trevision_opts_finish(&rev);\n>  \targc = parse_options_end(&ctx);\n>  \n>  \tif (nongit && argc > 1) {\n\n... and this.  It is a bit scary that we have to make sure all the\nusers of parse_revision_opt() users need to call this new helper.\nDidn't we recently gain new documentation to help novices write\ntheir first revision-traversal-API-using program?  Does it need to\nbe updated for this change (I didn't check)?\n\n> diff --git a/revision.c b/revision.c\n> index 816061f3d9..a39fd1c278 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2424,10 +2424,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n>  \t\trevs->pretty_given = 1;\n>  \t\trevs->abbrev_commit = 1;\n>  \t} else if (!strcmp(arg, \"--graph\")) {\n> -\t\trevs->topo_order = 1;\n> -\t\trevs->rewrite_parents = 1;\n>  \t\tgraph_clear(revs->graph);\n>  \t\trevs->graph = graph_init(revs);\n> +\t} else if (!strcmp(arg, \"--no-graph\")) {\n> +\t\tgraph_clear(revs->graph);\n> +\t\trevs->graph = NULL;\n>  \t} else if (!strcmp(arg, \"--encode-email-headers\")) {\n>  \t\trevs->encode_email_headers = 1;\n>  \t} else if (!strcmp(arg, \"--no-encode-email-headers\")) {\n> @@ -2524,8 +2525,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n>  \t\t\tunkv[(*unkc)++] = arg;\n>  \t\treturn opts;\n>  \t}\n> -\tif (revs->graph && revs->track_linear)\n> -\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--show-linear-break\", \"--graph\");\n>  \n>  \treturn 1;\n>  }\n\nAs a later \"--no\" can clear an earlier \"--graph\", we cannot\nincrementally check if options are compatible, until the end, at\nwhich time we can be sure that \"--graph\" is being asked.\n\n> +void revision_opts_finish(struct rev_info *revs)\n> +{\n> +\tif (revs->graph && revs->track_linear)\n> +\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--show-linear-break\", \"--graph\");\n\nInherited from the original, but we may want to wrap this line.\n\n> +\tif (revs->graph) {\n> +\t\trevs->topo_order = 1;\n> +\t\trevs->rewrite_parents = 1;\n> +\t}\n> +}\n> +\n\nOK.\n\n> @@ -2786,6 +2796,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>  \t\t\tbreak;\n>  \t\t}\n>  \t}\n> +\trevision_opts_finish(revs);\n\nOK.\n\nWill queue.  Thanks.\n"},{"id":"448237","messageId":"CAMMLpeTBstt8qo2q6r=YCEi-h-G-EkjY=d7kHKeepzTvz1gmJg@mail.gmail.com","threadId":"57406","inReplyTo":"xmqq1r09clxl.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] gitk: pass --no-graph to `git log`","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2022-02-11T19:05:17Z","receivedAt":"2022-02-11T19:05:33Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Fri, Feb 11, 2022 at 11:12 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Alex Henrie <alexhenrie24@gmail.com> writes:\n>\n> >  gitk-git/gitk | 9 +++++----\n> >  1 file changed, 5 insertions(+), 4 deletions(-)\n>\n> Please base a patch on gitk part to base on Paul's tree, not mine,\n> meaning that the first few lines of diff should begin like so:\n>\n>         diff --git a/gitk b/gitk\n>         index ...\n>         --- a/gitk\n>         +++ b/gitk\n>\n> and not as part of the series.\n\nOkay. I'll hang onto this patch until the previous one in the series\nis accepted.\n\n> What the first two patches want to do is a good thing regardless, so\n> I'll take a deeper look at them and queue them.  I am very skeptical\n> to log.graph=yes/no configuration for obvious reasons that setting\n> such a variable *will* break existing tools and users.  It is not\n> even \"it might break but we don't know until we try\", as this patch\n> loudly demonstrates.\n\nWhat if we make log.graph=true also require feature.experimental=true?\nThe log.graph option would really be a useful feature for people who\nuse Git exclusively from the CLI without any external tools. It seems\nthat the main challenge is how to give others time to adjust.\n\nThanks for all the feedback,\n\n-Alex\n"},{"id":"448238","messageId":"xmqq5yplb46m.fsf@gitster.g","threadId":"57406","inReplyTo":"CAMMLpeTBstt8qo2q6r=YCEi-h-G-EkjY=d7kHKeepzTvz1gmJg@mail.gmail.com","subject":"Re: [PATCH v3 4/4] gitk: pass --no-graph to `git log`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-11T19:20:49Z","receivedAt":"2022-02-11T19:20:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie <alexhenrie24@gmail.com> writes:\n\n> What if we make log.graph=true also require feature.experimental=true?\n\nNo.  feature.experimental is to give people an opt-in opportunity\nfor features that we are considering to enable by default.\n\n> The log.graph option would really be a useful feature for people who\n> use Git exclusively from the CLI without any external tools. It seems\n> that the main challenge is how to give others time to adjust.\n\nThose who want to see log by default must need to twaek their\nconfiguration.  Instead of doing \"git config log.graph true\" and\nbreaking tools, they can do \"git config alias.mylog 'log --graph'\"\nwith the same ease, without breaking anything.\n\nSo...\n\n"},{"id":"448239","messageId":"CAMMLpeQ_L+ogWtVj53gLB9ouVZeQ9-cr8oT63fGD4jJ_R4xj5Q@mail.gmail.com","threadId":"57406","inReplyTo":"xmqqa6exb51o.fsf@gitster.g","subject":"Re: [PATCH v3 2/4] log: add a --no-graph option","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2022-02-11T19:20:42Z","receivedAt":"2022-02-11T19:21:00Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Fri, Feb 11, 2022 at 12:02 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Alex Henrie <alexhenrie24@gmail.com> writes:\n>\n> > --- a/builtin/blame.c\n> > +++ b/builtin/blame.c\n> > @@ -934,6 +934,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n> >               parse_revision_opt(&revs, &ctx, options, blame_opt_usage);\n> >       }\n> >  parse_done:\n> > +     revision_opts_finish(&revs);\n>\n> This ...\n>\n> > diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> > index e7f7af5de3..228d782754 100644\n> > --- a/builtin/shortlog.c\n> > +++ b/builtin/shortlog.c\n> > @@ -388,6 +388,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n> >               parse_revision_opt(&rev, &ctx, options, shortlog_usage);\n> >       }\n> >  parse_done:\n> > +     revision_opts_finish(&rev);\n> >       argc = parse_options_end(&ctx);\n> >\n> >       if (nongit && argc > 1) {\n>\n> ... and this.  It is a bit scary that we have to make sure all the\n> users of parse_revision_opt() users need to call this new helper.\n> Didn't we recently gain new documentation to help novices write\n> their first revision-traversal-API-using program?  Does it need to\n> be updated for this change (I didn't check)?\n\nI don't see any documentation on how to use parse_revision_opt\ndirectly; I only see documentation on how to use setup_revisions,\nwhose interface did not change.\n\nAnother approach would be to make a parse_rev_options_step function\nthat wraps parse_options_step and does the final steps when\nparse_options_step returns PARSE_OPT_DONE. Would that be better?\n\n> > diff --git a/revision.c b/revision.c\n> > index 816061f3d9..a39fd1c278 100644\n> > --- a/revision.c\n> > +++ b/revision.c\n> > @@ -2424,10 +2424,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> >               revs->pretty_given = 1;\n> >               revs->abbrev_commit = 1;\n> >       } else if (!strcmp(arg, \"--graph\")) {\n> > -             revs->topo_order = 1;\n> > -             revs->rewrite_parents = 1;\n> >               graph_clear(revs->graph);\n> >               revs->graph = graph_init(revs);\n> > +     } else if (!strcmp(arg, \"--no-graph\")) {\n> > +             graph_clear(revs->graph);\n> > +             revs->graph = NULL;\n> >       } else if (!strcmp(arg, \"--encode-email-headers\")) {\n> >               revs->encode_email_headers = 1;\n> >       } else if (!strcmp(arg, \"--no-encode-email-headers\")) {\n> > @@ -2524,8 +2525,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> >                       unkv[(*unkc)++] = arg;\n> >               return opts;\n> >       }\n> > -     if (revs->graph && revs->track_linear)\n> > -             die(_(\"options '%s' and '%s' cannot be used together\"), \"--show-linear-break\", \"--graph\");\n> >\n> >       return 1;\n> >  }\n>\n> As a later \"--no\" can clear an earlier \"--graph\", we cannot\n> incrementally check if options are compatible, until the end, at\n> which time we can be sure that \"--graph\" is being asked.\n\nExactly. This is intentional, to avoid erroring out unnecessarily.\n\n-Alex\n"},{"id":"448242","messageId":"xmqqwni19nrd.fsf@gitster.g","threadId":"57406","inReplyTo":"xmqq5yplb46m.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] gitk: pass --no-graph to `git log`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-11T20:00:54Z","receivedAt":"2022-02-11T20:01:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Alex Henrie <alexhenrie24@gmail.com> writes:\n>\n>> What if we make log.graph=true also require feature.experimental=true?\n>\n> No.  feature.experimental is to give people an opt-in opportunity\n> for features that we are considering to enable by default.\n>\n>> The log.graph option would really be a useful feature for people who\n>> use Git exclusively from the CLI without any external tools. It seems\n>> that the main challenge is how to give others time to adjust.\n\nLet me clarify the first point by stating it a bit differently.\n\nfeature.experimental is all about this:\n\n    We have an idea for this new feature.  We made it useful, and\n    also made it not to regress the end-user experience for those\n    who do not need the new feature, to the best of our ability.\n    But there may be use cases we failed to consider while doing\n    so.  So let's ask early adopters, who may use Git in contexts\n    that are very different from ours, to try testing it out in\n    their daily use, to see if there are unexpected glitches.\n\nYou do not have to argue how the --graph feature may be useful for\ncharacter terminal users.  We already know it is, otherwise we\nwouldn't have added it in the first place.\n\nAnd arguing how --graph feature is useful does not help prove\nanything, when at the issue is if it is a good idea to allow the\nlog.graph configuration variable to affect (unfortunate) scripts\npeople wrote around \"git log\", instead of using plumbing commands,\nnegatively.  We already know it will hurt to force everybody to\nupdate their script to explicitly pass --no-graph on the command\nline.  This series hasn't done any of the \"not to regress to the\nbest of our ability\" part.\n\nIf there were an agreement on the general direction to _forbid_ use\nof \"git log\" in scripts, which would require coordinated efforts to\nhelp people migrate over time, e.g.\n\n - improve plumbing by adding features that people piled only on\n   \"git log\" without allowing plumbing users the same over time.\n\n - perhaps an automated way to convert scripts that use \"git log\" to\n   instead use \"git log --no-graph\"\n\nto help script writers migrate away from \"git log\", adding log.graph\nconfiguration variable may become very a good idea.\n\nBut without such effort starting at the same time and gaining\nconsensus (or already underway), just adding such a variable to\nbreak existing scripts would not be a good idea worth asking the\nearly adopters to test.  We already know it would break scripts.\n"},{"id":"448248","messageId":"CAMMLpeTWvN45jTmoFHBS2+E+wEuBKTAnxXh3+RWzJc0O2rYy2A@mail.gmail.com","threadId":"57406","inReplyTo":"xmqq5yplb46m.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] gitk: pass --no-graph to `git log`","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2022-02-11T20:08:26Z","receivedAt":"2022-02-11T20:08:41Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Fri, Feb 11, 2022 at 12:20 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Alex Henrie <alexhenrie24@gmail.com> writes:\n>\n> > What if we make log.graph=true also require feature.experimental=true?\n>\n> No.  feature.experimental is to give people an opt-in opportunity\n> for features that we are considering to enable by default.\n>\n> > The log.graph option would really be a useful feature for people who\n> > use Git exclusively from the CLI without any external tools. It seems\n> > that the main challenge is how to give others time to adjust.\n>\n> Those who want to see log by default must need to twaek their\n> configuration.  Instead of doing \"git config log.graph true\" and\n> breaking tools, they can do \"git config alias.mylog 'log --graph'\"\n> with the same ease, without breaking anything.\n>\n> So...\n\nYeah, that's not a bad solution. I actually have `git graph` aliased to\n`git log --graph --abbrev-commit --pretty=oneline` for this purpose.\nIf a lot of people are doing that, maybe a new command should be added\nto Git itself. It seems like there's not much demand for that at the\nmoment though.\n\n-Alex\n"},{"id":"448249","messageId":"CAMMLpeQJ69i6OCGtbu7sV5rAzg4Od3fOsREMDRDW0vD+m3L3HA@mail.gmail.com","threadId":"57406","inReplyTo":"xmqqwni19nrd.fsf@gitster.g","subject":"Re: [PATCH v3 4/4] gitk: pass --no-graph to `git log`","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2022-02-11T20:11:45Z","receivedAt":"2022-02-11T20:12:02Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Fri, Feb 11, 2022 at 1:00 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> > Alex Henrie <alexhenrie24@gmail.com> writes:\n> >\n> >> What if we make log.graph=true also require feature.experimental=true?\n> >\n> > No.  feature.experimental is to give people an opt-in opportunity\n> > for features that we are considering to enable by default.\n> >\n> >> The log.graph option would really be a useful feature for people who\n> >> use Git exclusively from the CLI without any external tools. It seems\n> >> that the main challenge is how to give others time to adjust.\n>\n> Let me clarify the first point by stating it a bit differently.\n>\n> feature.experimental is all about this:\n>\n>     We have an idea for this new feature.  We made it useful, and\n>     also made it not to regress the end-user experience for those\n>     who do not need the new feature, to the best of our ability.\n>     But there may be use cases we failed to consider while doing\n>     so.  So let's ask early adopters, who may use Git in contexts\n>     that are very different from ours, to try testing it out in\n>     their daily use, to see if there are unexpected glitches.\n>\n> You do not have to argue how the --graph feature may be useful for\n> character terminal users.  We already know it is, otherwise we\n> wouldn't have added it in the first place.\n>\n> And arguing how --graph feature is useful does not help prove\n> anything, when at the issue is if it is a good idea to allow the\n> log.graph configuration variable to affect (unfortunate) scripts\n> people wrote around \"git log\", instead of using plumbing commands,\n> negatively.  We already know it will hurt to force everybody to\n> update their script to explicitly pass --no-graph on the command\n> line.  This series hasn't done any of the \"not to regress to the\n> best of our ability\" part.\n>\n> If there were an agreement on the general direction to _forbid_ use\n> of \"git log\" in scripts, which would require coordinated efforts to\n> help people migrate over time, e.g.\n>\n>  - improve plumbing by adding features that people piled only on\n>    \"git log\" without allowing plumbing users the same over time.\n>\n>  - perhaps an automated way to convert scripts that use \"git log\" to\n>    instead use \"git log --no-graph\"\n>\n> to help script writers migrate away from \"git log\", adding log.graph\n> configuration variable may become very a good idea.\n>\n> But without such effort starting at the same time and gaining\n> consensus (or already underway), just adding such a variable to\n> break existing scripts would not be a good idea worth asking the\n> early adopters to test.  We already know it would break scripts.\n\nOkay. Thanks for the explanation!\n\n-Alex\n"}]}