| Message ID | 20210117110337.429994-3-kmarek@pdinc.us (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Option to modify revision mark for root commits | expand |
Kyle Marek <kmarek@pdinc.us> writes: > where <barrier> sets rev_info.break_revision_mark, the revision mark > used for root commits. Please make sure that the body of the proposed log message begins with a full sentence, not as a continuation of a sentence that the title started (as a consequence, the title must be understandable without the help of the beginning part of the body, too). > Signed-off-by: Kyle Marek <kmarek@pdinc.us> > --- > diff --git a/revision.c b/revision.c > index 8556923de8..51deab2326 100644 > --- a/revision.c > +++ b/revision.c > @@ -2402,10 +2402,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg > revs->show_signature = 0; > } else if (!strcmp(arg, "--show-linear-break")) { > revs->break_bar = " .........."; > + revs->break_revision_mark = "#"; > revs->track_linear = 1; > revs->track_first_time = 1; > } else if (skip_prefix(arg, "--show-linear-break=", &optarg)) { > revs->break_bar = xstrdup(optarg); > + revs->break_revision_mark = xstrdup(optarg); > revs->track_linear = 1; > revs->track_first_time = 1; > } else if (skip_prefix(arg, "--show-notes=", &optarg) || In other words, revs->break_revision_mark is left NULL unless --show-linear-break is given. > @@ -4192,8 +4192,8 @@ const char *get_revision_mark(const struct rev_info *revs, const struct commit * > else > return ">"; > } else if (revs->graph) { > - if (!commit->parents) > - return "#"; > + if (revs->break_revision_mark && !commit->parents) > + return revs->break_revision_mark; And that causes this to break. Now "--graph" alone won't show '#' for the root commits, despite that is what [1/2] wanted to do. Here is a fix-up, plus some minimum tests. The part to teach left-right codepath to show L/R is a fix-up to [1/2], not to this step. You might want to change them to some left/right punctuation letters, like () or []. The other hunks in revision.c are fixes to step [2/2]. I didn't test a custom --show-linear-break='My break line' in the attachedtest, so that it can be squashed into your [1/2] to test the feature that step adds. You should be able to add tests for that feature in this step [2/2] on top. I still am skeptical that spending 3 more letters to denote roots is worth it, though. revision.c | 11 ++-- t/t6020-rev-list-boundary.sh | 132 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 5 deletions(-) diff --git c/revision.c w/revision.c index 33fbef5c08..55521c53af 100644 --- c/revision.c +++ w/revision.c @@ -2402,7 +2402,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg revs->show_signature = 0; } else if (!strcmp(arg, "--show-linear-break")) { revs->break_bar = " .........."; - revs->break_revision_mark = "#"; revs->track_linear = 1; revs->track_first_time = 1; } else if (skip_prefix(arg, "--show-linear-break=", &optarg)) { @@ -4219,12 +4218,14 @@ const char *get_revision_mark(const struct rev_info *revs, const struct commit * return "="; else if (!revs || revs->left_right) { if (commit->object.flags & SYMMETRIC_LEFT) - return "<"; + return commit->parents ? "<" : "L"; else - return ">"; + return commit->parents ? ">" : "R"; } else if (revs->graph) { - if (revs->break_revision_mark && !commit->parents) - return revs->break_revision_mark; + if (!commit->parents) + return (revs->break_revision_mark + ? revs->break_revision_mark + : "#"); return "*"; } else if (revs->cherry_mark) return "+"; diff --git c/t/t6020-rev-list-boundary.sh w/t/t6020-rev-list-boundary.sh new file mode 100755 index 0000000000..35614e9baf --- /dev/null +++ w/t/t6020-rev-list-boundary.sh @@ -0,0 +1,132 @@ +#!/bin/sh + +test_description='rev-list/log boundary and root' + +. ./test-lib.sh + +test_expect_success 'setup' ' + test_commit A && + test_commit B && + git reset --hard A && + test_commit C && + + git checkout --orphan side && + git rm -fr . && + test_commit X && + test_commit Y && + + test_tick && git merge --allow-unrelated-histories -m "M" B && + test_tick && git merge -m "N" C && + test_commit Z +' + +test_expect_success 'log with boundary' ' + git log --graph --boundary --format='%s' ^A ^X Z >actual && + sed -e "s/Q$//" >expect <<-\EOF && + * Z + * N + |\ Q + | * C + * | M + |\ \ Q + | * | B + | |/ Q + * | Y + o | X + / Q + o A + EOF + test_cmp expect actual +' + +test_expect_success 'log --left-right with symmetric boundary' ' + git log --graph --left-right --boundary --format='%s' B...C >actual && + sed -e "s/Q$//" >expect <<-\EOF && + > C + | < B + |/ Q + o A + EOF + test_cmp expect actual +' + +test_expect_success 'log --left-right with asymmetric boundary' ' + git log --graph --left-right --boundary --format='%s' ^A ^X Z >actual && + sed -e "s/Q$//" >expect <<-\EOF && + > Z + > N + |\ Q + | > C + > | M + |\ \ Q + | > | B + | |/ Q + > | Y + o | X + / Q + o A + EOF + test_cmp expect actual +' + +test_expect_success 'log down to root' ' + git log --graph --format='%s' Z >actual && + sed -e "s/Q$//" >expect <<-\EOF && + * Z + * N + |\ Q + | * C + * | M + |\ \ Q + | * | B + | |/ Q + | # A + * Y + # X + EOF + test_cmp expect actual +' + +test_expect_success 'log down to root' ' + git log --graph --format='%s' B Y >actual && + sed -e "s/Q$//" >expect <<-\EOF && + * Y + # X + * B + # A + EOF + test_cmp expect actual +' + +test_expect_success 'log that happens to show root' ' + git log --graph -3 --format='%s' B Y >actual && + sed -e "s/Q$//" >expect <<-\EOF && + * Y + # X + * B + EOF + test_cmp expect actual +' + +test_expect_success 'log --left-right down to root' ' + git log --graph --left-right --format='%s' B...Y >actual && + sed -e "s/Q$//" >expect <<-\EOF && + > Y + R X + < B + L A + EOF + test_cmp expect actual +' + +test_expect_success 'log --left-right that happens to show root' ' + git log --graph -3 --left-right --format='%s' B...Y >actual && + sed -e "s/Q$//" >expect <<-\EOF && + > Y + R X + < B + EOF + test_cmp expect actual +' + +test_done
Junio C Hamano <gitster@pobox.com> writes: > In other words, revs->break_revision_mark is left NULL unless > --show-linear-break is given. > >> @@ -4192,8 +4192,8 @@ const char *get_revision_mark(const struct rev_info *revs, const struct commit * >> else >> return ">"; >> } else if (revs->graph) { >> - if (!commit->parents) >> - return "#"; >> + if (revs->break_revision_mark && !commit->parents) >> + return revs->break_revision_mark; > > And that causes this to break. Now "--graph" alone won't show '#' > for the root commits, despite that is what [1/2] wanted to do. > > Here is a fix-up, plus some minimum tests. Having said all that, I do not mind if the new markings were activated only when --show-linear-break option (or a separate new option) is given. But if that is where we want to go, your [1/2] that uses the new markings unconditionally is a regression. A better organization, if we wanted to have multiple and smaller steps than a single whole thing, would be: [1/2] Introduce a new "--mark-root-commits" option, or abuse the existing "--show-linear-break" option, and change "*<>" marking used for commits to "#LR" (or whatever appropriate) when the option is in effect. Document the behaviour and add tests. [2/2] Introduce "--show-linear-break=<custom-value>" option. Document the behaviour and add tests. If you apply [1/2] and [2/2] with the earlier fixes I sent, you'll see many fallouts from existing tests, as the representation of the root commit is changed unconditionally. We view breakages of tests as a rough estimate of how badly end-user scripts could break, and the picture was not very pretty. And that is why I am suggesting the above "only do the new markings when asked, not unconditionally" approach. I still am skeptical that spending 3 more letters to denote roots is worth it, though. Thanks.
On 1/17/21 9:09 PM, Junio C Hamano wrote: > Junio C Hamano<gitster@pobox.com> writes: > >> In other words, revs->break_revision_mark is left NULL unless >> --show-linear-break is given. >> >>> @@ -4192,8 +4192,8 @@ const char *get_revision_mark(const struct rev_info *revs, const struct commit * >>> else >>> return ">"; >>> } else if (revs->graph) { >>> - if (!commit->parents) >>> - return "#"; >>> + if (revs->break_revision_mark && !commit->parents) >>> + return revs->break_revision_mark; >> And that causes this to break. Now "--graph" alone won't show '#' >> for the root commits, despite that is what [1/2] wanted to do. >> >> Here is a fix-up, plus some minimum tests. > Having said all that, I do not mind if the new markings were > activated only when --show-linear-break option (or a separate new > option) is given. But if that is where we want to go, your [1/2] > that uses the new markings unconditionally is a regression. > > A better organization, if we wanted to have multiple and smaller > steps than a single whole thing, would be: > > [1/2] Introduce a new "--mark-root-commits" option, or abuse the > existing "--show-linear-break" option, and change "*<>" > marking used for commits to "#LR" (or whatever appropriate) > when the option is in effect. Document the behaviour and add > tests. > > [2/2] Introduce "--show-linear-break=<custom-value>" option. > Document the behaviour and add tests. > > If you apply [1/2] and [2/2] with the earlier fixes I sent, you'll > see many fallouts from existing tests, as the representation of the > root commit is changed unconditionally. We view breakages of tests > as a rough estimate of how badly end-user scripts could break, and > the picture was not very pretty. And that is why I am suggesting > the above "only do the new markings when asked, not unconditionally" > approach. Sorry. I didn't make this clear. It is not an accident that patch 1 denotes root commits unconditionally and patch 2 makes it optional. I present two choices. If we prefer to unconditionally denote root commits, patch 2 may be left out, otherwise, patch 1 should be squashed away. I didn't have an opinion towards either option, but you make a good point about end-user scripts. > I still am skeptical that spending 3 more letters to denote roots is > worth it, though. Me too, but I think a user-defined mark needs to be a string to support Unicode characters.
Kyle Marek <kmarek@pdinc.us> writes: > Me too, but I think a user-defined mark needs to be a string to > support Unicode characters. Ahh, I didn't even consider making it user-defined. As it seems a lot safer to make this an optional feature, it does sort-of make sense to let the letters used for root & left-root be customizable, and it does make sense to take a multi-byte character, but I am not sure what implications it has if we allowed any string without ensuring that it occupies one display column.
On 1/18/21 4:01 PM, Junio C Hamano wrote: > Kyle Marek <kmarek@pdinc.us> writes: > >> Me too, but I think a user-defined mark needs to be a string to >> support Unicode characters. > Ahh, I didn't even consider making it user-defined. > > As it seems a lot safer to make this an optional feature, it does > sort-of make sense to let the letters used for root & left-root be > customizable, and it does make sense to take a multi-byte character, > but I am not sure what implications it has if we allowed any string > without ensuring that it occupies one display column. Does git, or a dependency library, have the ability to interpret TERM and locale to determine on-screen character count/size? If not, maybe let users use multi-character strings, but call it misuse of the option that will mess offset that row of the --graph output until we have something to determine on-screen size.
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index 002379056a..93adb77c19 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -1104,6 +1104,13 @@ This implies the `--topo-order` option by default, but the do not belong to a linear branch. This option puts a barrier in between them in that case. If `<barrier>` is specified, it is the string that will be shown instead of the default one. ++ +When --graph is used with --oneline, there is usually no vertical +space between commits, so the graph edge is not drawn. This can make +it hard to see that a history may end at one commit, while an +unrelated history starts at the next commit. This option changes the +revision mark for root commits. If `<barrier>` is specified, it is +used as the new revision mark instead of the default one. ifdef::git-rev-list[] --count:: diff --git a/log-tree.c b/log-tree.c index fd0dde97ec..f62300e404 100644 --- a/log-tree.c +++ b/log-tree.c @@ -962,7 +962,7 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit) if (opt->line_level_traverse) return line_log_print(opt, commit); - if (opt->track_linear && !opt->linear && !opt->reverse_output_stage) + if (!opt->graph && opt->track_linear && !opt->linear && !opt->reverse_output_stage) fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar); shown = log_tree_diff(opt, commit, &log); if (!shown && opt->loginfo && opt->always_show_header) { diff --git a/revision.c b/revision.c index 8556923de8..51deab2326 100644 --- a/revision.c +++ b/revision.c @@ -2402,10 +2402,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg revs->show_signature = 0; } else if (!strcmp(arg, "--show-linear-break")) { revs->break_bar = " .........."; + revs->break_revision_mark = "#"; revs->track_linear = 1; revs->track_first_time = 1; } else if (skip_prefix(arg, "--show-linear-break=", &optarg)) { revs->break_bar = xstrdup(optarg); + revs->break_revision_mark = xstrdup(optarg); revs->track_linear = 1; revs->track_first_time = 1; } else if (skip_prefix(arg, "--show-notes=", &optarg) || @@ -2530,8 +2532,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("--show-linear-break and --graph are incompatible"); return 1; } @@ -4192,8 +4192,8 @@ const char *get_revision_mark(const struct rev_info *revs, const struct commit * else return ">"; } else if (revs->graph) { - if (!commit->parents) - return "#"; + if (revs->break_revision_mark && !commit->parents) + return revs->break_revision_mark; return "*"; } else if (revs->cherry_mark) return "+"; diff --git a/revision.h b/revision.h index 086ff10280..83b2ecef56 100644 --- a/revision.h +++ b/revision.h @@ -297,6 +297,7 @@ struct rev_info { struct commit_list *previous_parents; const char *break_bar; + const char *break_revision_mark; struct revision_sources *sources;
where <barrier> sets rev_info.break_revision_mark, the revision mark used for root commits. Signed-off-by: Kyle Marek <kmarek@pdinc.us> --- Documentation/rev-list-options.txt | 7 +++++++ log-tree.c | 2 +- revision.c | 8 ++++---- revision.h | 1 + 4 files changed, 13 insertions(+), 5 deletions(-)