* [PATCH 1/3] log: fix memory leak if --graph is passed multiple times @ 2022-02-09 16:23 Alex Henrie 2022-02-09 16:23 ` [PATCH 2/3] log: add a config option for --graph Alex Henrie ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Alex Henrie @ 2022-02-09 16:23 UTC (permalink / raw) To: git, paulus; +Cc: 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] log: add a config option for --graph 2022-02-09 16:23 [PATCH 1/3] log: fix memory leak if --graph is passed multiple times Alex Henrie @ 2022-02-09 16:23 ` Alex Henrie 2022-02-09 18:25 ` Junio C Hamano 2022-02-09 16:23 ` [PATCH 3/3] gitk: set log.graph=false when running `git log` Alex Henrie 2022-02-09 18:16 ` [PATCH 1/3] log: fix memory leak if --graph is passed multiple times Junio C Hamano 2 siblings, 1 reply; 9+ messages in thread From: Alex Henrie @ 2022-02-09 16:23 UTC (permalink / raw) To: git, paulus; +Cc: 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] log: add a config option for --graph 2022-02-09 16:23 ` [PATCH 2/3] log: add a config option for --graph Alex Henrie @ 2022-02-09 18:25 ` Junio C Hamano 2022-02-10 16:49 ` Alex Henrie 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2022-02-09 18:25 UTC (permalink / raw) To: Alex Henrie; +Cc: git, paulus 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] log: add a config option for --graph 2022-02-09 18:25 ` Junio C Hamano @ 2022-02-10 16:49 ` Alex Henrie 0 siblings, 0 replies; 9+ messages in thread From: Alex Henrie @ 2022-02-10 16:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git mailing list, paulus 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] gitk: set log.graph=false when running `git log` 2022-02-09 16:23 [PATCH 1/3] log: fix memory leak if --graph is passed multiple times Alex Henrie 2022-02-09 16:23 ` [PATCH 2/3] log: add a config option for --graph Alex Henrie @ 2022-02-09 16:23 ` Alex Henrie 2022-02-09 18:26 ` Junio C Hamano 2022-02-09 18:16 ` [PATCH 1/3] log: fix memory leak if --graph is passed multiple times Junio C Hamano 2 siblings, 1 reply; 9+ messages in thread From: Alex Henrie @ 2022-02-09 16:23 UTC (permalink / raw) To: git, paulus; +Cc: 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] gitk: set log.graph=false when running `git log` 2022-02-09 16:23 ` [PATCH 3/3] gitk: set log.graph=false when running `git log` Alex Henrie @ 2022-02-09 18:26 ` Junio C Hamano 2022-02-10 16:50 ` Alex Henrie 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2022-02-09 18:26 UTC (permalink / raw) To: Alex Henrie; +Cc: git, paulus 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] gitk: set log.graph=false when running `git log` 2022-02-09 18:26 ` Junio C Hamano @ 2022-02-10 16:50 ` Alex Henrie 2022-02-10 20:15 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Alex Henrie @ 2022-02-10 16:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git mailing list, paulus 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] gitk: set log.graph=false when running `git log` 2022-02-10 16:50 ` Alex Henrie @ 2022-02-10 20:15 ` Junio C Hamano 0 siblings, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2022-02-10 20:15 UTC (permalink / raw) To: Alex Henrie; +Cc: Git mailing list, paulus 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] log: fix memory leak if --graph is passed multiple times 2022-02-09 16:23 [PATCH 1/3] log: fix memory leak if --graph is passed multiple times Alex Henrie 2022-02-09 16:23 ` [PATCH 2/3] log: add a config option for --graph Alex Henrie 2022-02-09 16:23 ` [PATCH 3/3] gitk: set log.graph=false when running `git log` Alex Henrie @ 2022-02-09 18:16 ` Junio C Hamano 2 siblings, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2022-02-09 18:16 UTC (permalink / raw) To: Alex Henrie; +Cc: git, paulus 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")) { ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-02-10 20:15 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-09 16:23 [PATCH 1/3] log: fix memory leak if --graph is passed multiple times Alex Henrie 2022-02-09 16:23 ` [PATCH 2/3] log: add a config option for --graph Alex Henrie 2022-02-09 18:25 ` Junio C Hamano 2022-02-10 16:49 ` Alex Henrie 2022-02-09 16:23 ` [PATCH 3/3] gitk: set log.graph=false when running `git log` Alex Henrie 2022-02-09 18:26 ` Junio C Hamano 2022-02-10 16:50 ` Alex Henrie 2022-02-10 20:15 ` Junio C Hamano 2022-02-09 18:16 ` [PATCH 1/3] log: fix memory leak if --graph is passed multiple times Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.