{"thread":{"id":"40336","subject":"[PATCH 2/3] Add tests for \"pretty: Pass graph width to pretty formatting for use in '%>|(N)'\"","startedAt":"2015-09-11T23:25:11Z","lastAt":"2015-12-22T22:54:45Z","messageCount":6,"participants":["Josef Kufner","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"269810","messageId":"1442013913-2970-1-git-send-email-josef@kufner.cz","threadId":"40336","inReplyTo":"xmqqk2rwzlhi.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 1/3] pretty: Pass graph width to pretty formatting for use in '%>|(N)'","fromName":"Josef Kufner","fromEmail":"josef@kufner.cz","sentAt":"2015-09-11T23:25:11Z","receivedAt":"2015-09-11T23:25:11Z","isPatch":true,"sender":{"key":"josef@kufner.cz","avatar":"https://avatars.githubusercontent.com/u/16572?v=4"},"body":"Pass graph width to pretty formatting, so N in '%>|(N)' includes columns\nconsumed by graph rendered when --graph option is in use.\n\nExample:\n git log --all --graph --pretty='format: [%>|(20)%h] %ar%d'\n\n All commit hashes should be aligned at 20th column from edge of the\n terminal, not from the edge of the graph.\n\nSigned-off-by: Josef Kufner \n---\n commit.h | 1 +\n graph.c | 7 +++++++\n graph.h | 5 +++++\n log-tree.c | 2 ++\n pretty.c | 1 +\n 5 files changed, 16 insertions(+)\n\ndiff --git a/commit.h b/commit.h\nindex 5d58be0..0a9a707 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -160,6 +160,7 @@ struct pretty_print_context {\n \t * should not be counted on by callers.\n \t */\n \tstruct string_list in_body_headers;\n+\tint graph_width;\n };\n \n struct userformat_want {\ndiff --git a/graph.c b/graph.c\nindex c25a09a..4802411 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -671,6 +671,13 @@ static void graph_output_padding_line(struct git_graph *graph,\n \tgraph_pad_horizontally(graph, sb, graph->num_new_columns * 2);\n }\n \n+\n+int graph_width(struct git_graph *graph)\n+{\n+\treturn graph->width;\n+}\n+\n+\n static void graph_output_skip_line(struct git_graph *graph, struct strbuf *sb)\n {\n \t/*\ndiff --git a/graph.h b/graph.h\nindex 0be62bd..3f48c19 100644\n--- a/graph.h\n+++ b/graph.h\n@@ -68,6 +68,11 @@ int graph_next_line(struct git_graph *graph, struct strbuf *sb);\n \n \n /*\n+ * Return current width of the graph in on-screen characters.\n+ */\n+int graph_width(struct git_graph *graph);\n+\n+/*\n * graph_show_*: helper functions for printing to stdout\n */\n \ndiff --git a/log-tree.c b/log-tree.c\nindex 7b1b57a..08fd5b6 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -686,6 +686,8 @@ void show_log(struct rev_info *opt)\n \tctx.output_encoding = get_log_output_encoding();\n \tif (opt->from_ident.mail_begin && opt->from_ident.name_begin)\n \t\tctx.from_ident = &opt->from_ident;\n+\tif (opt->graph)\n+\t\tctx.graph_width = graph_width(opt->graph);\n \tpretty_print_commit(&ctx, commit, &msgbuf);\n \n \tif (opt->add_signoff)\ndiff --git a/pretty.c b/pretty.c\nindex 151c2ae..f1cf9e2 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1297,6 +1297,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */\n \t\tif (!start)\n \t\t\tstart = sb->buf;\n \t\toccupied = utf8_strnwidth(start, -1, 1);\n+\t\toccupied += c->pretty_ctx->graph_width;\n \t\tpadding = (-padding) - occupied;\n \t}\n \twhile (1) {\n-- \n2.5.1\n"},{"id":"269808","messageId":"1442013913-2970-2-git-send-email-josef@kufner.cz","threadId":"40336","inReplyTo":"1442013913-2970-1-git-send-email-josef@kufner.cz","subject":"[PATCH 2/3] Add tests for \"pretty: Pass graph width to pretty formatting for use in '%>|(N)'\"","fromName":"Josef Kufner","fromEmail":"josef@kufner.cz","sentAt":"2015-09-11T23:25:12Z","receivedAt":"2015-09-11T23:25:12Z","isPatch":true,"sender":{"key":"josef@kufner.cz","avatar":"https://avatars.githubusercontent.com/u/16572?v=4"},"body":"Signed-off-by: Josef Kufner \n---\n t/t4205-log-pretty-formats.sh | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 7398605..3358837 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -319,6 +319,18 @@ EOF\n \ttest_cmp expected actual\n '\n \n+# Note: Space between 'message' and 'two' should be in the same column as in previous test.\n+test_expect_success 'right alignment formatting at the nth column with --graph. i18n.logOutputEncoding' '\n+\tgit -c i18n.logOutputEncoding=$test_encoding log --graph --pretty=\"tformat:%h %>|(40)%s\" >actual &&\n+\tqz_to_tab_space <expected &&\n+* $head1 message two\n+* $head2 message one\n+* $head3 add bar\n+* $head4 $(commit_msg)\n+EOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'right alignment formatting with no padding' '\n \tgit log --pretty=\"tformat:%>(1)%s\" >actual &&\n \tcat <expected &&\n@@ -330,6 +342,17 @@ EOF\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'right alignment formatting with no padding and with --graph' '\n+\tgit log --graph --pretty=\"tformat:%>(1)%s\" >actual &&\n+\tcat <expected &&\n+* message two\n+* message one\n+* add bar\n+* $(commit_msg)\n+EOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'right alignment formatting with no padding. i18n.logOutputEncoding' '\n \tgit -c i18n.logOutputEncoding=$test_encoding log --pretty=\"tformat:%>(1)%s\" >actual &&\n \tcat <expected &&\n-- \n2.5.1\n"},{"id":"269809","messageId":"1442013913-2970-3-git-send-email-josef@kufner.cz","threadId":"40336","inReplyTo":"1442013913-2970-1-git-send-email-josef@kufner.cz","subject":"[PATCH 3/3] t4205: remove unnecessary use of qz_to_tab_space","fromName":"Josef Kufner","fromEmail":"josef@kufner.cz","sentAt":"2015-09-11T23:25:13Z","receivedAt":"2015-09-11T23:25:13Z","isPatch":true,"sender":{"key":"josef@kufner.cz","avatar":"https://avatars.githubusercontent.com/u/16572?v=4"},"body":"Many existing tests in this script pipes the result thru\nqz_to_tab_space, but the payload does not need any Q or Z to be replaced\nto tab or space. Redirect the input directly into iconv or use cat\ninstead.\n\nSigned-off-by: Josef Kufner \n---\n t/t4205-log-pretty-formats.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 3358837..1f191cf 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -299,7 +299,7 @@ EOF\n \n test_expect_success 'right alignment formatting at the nth column' '\n \tgit log --pretty=\"tformat:%h %>|(40)%s\" >actual &&\n-\tqz_to_tab_space <expected &&\n+\tcat <expected &&\n $head1 message two\n $head2 message one\n $head3 add bar\n@@ -310,7 +310,7 @@ EOF\n \n test_expect_success 'right alignment formatting at the nth column. i18n.logOutputEncoding' '\n \tgit -c i18n.logOutputEncoding=$test_encoding log --pretty=\"tformat:%h %>|(40)%s\" >actual &&\n-\tqz_to_tab_space <expected &&\n+\ticonv -f utf-8 -t $test_encoding <expected &&\n $head1 message two\n $head2 message one\n $head3 add bar\n@@ -322,7 +322,7 @@ EOF\n # Note: Space between 'message' and 'two' should be in the same column as in previous test.\n test_expect_success 'right alignment formatting at the nth column with --graph. i18n.logOutputEncoding' '\n \tgit -c i18n.logOutputEncoding=$test_encoding log --graph --pretty=\"tformat:%h %>|(40)%s\" >actual &&\n-\tqz_to_tab_space <expected &&\n+\ticonv -f utf-8 -t $test_encoding <expected &&\n * $head1 message two\n * $head2 message one\n * $head3 add bar\n@@ -423,7 +423,7 @@ EOF\n old_head1=$(git rev-parse --verify HEAD~0)\n test_expect_success 'center alignment formatting with no padding. i18n.logOutputEncoding' '\n \tgit -c i18n.logOutputEncoding=$test_encoding log --pretty=\"tformat:%><(1)%s\" >actual &&\n-\tcat <expected &&\n+\ticonv -f utf-8 -t $test_encoding <expected &&\n message two\n message one\n add bar\n-- \n2.5.1\n"},{"id":"270419","messageId":"xmqqa8sfa7di.fsf@gitster.mtv.corp.google.com","threadId":"40336","inReplyTo":"1442013913-2970-1-git-send-email-josef@kufner.cz","subject":"Re: [PATCH 1/3] pretty: Pass graph width to pretty formatting for use in '%>|(N)'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-21T19:40:25Z","receivedAt":"2015-09-21T19:40:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josef Kufner writes:\n\n> Pass graph width to pretty formatting, so N in '%>|(N)' includes columns\n> consumed by graph rendered when --graph option is in use.\n>\n> Example:\n> git log --all --graph --pretty='format: [%>|(20)%h] %ar%d'\n>\n> All commit hashes should be aligned at 20th column from edge of the\n> terminal, not from the edge of the graph.\n>\n> Signed-off-by: Josef Kufner \n> ---\n\n[CC'ed Duy for ideas, as the \"%>|(ALIGN)\" thing is his invention]\n\nIf you imagine an entry for a commit in the \"git log\" output as a\nrectangle that has its commit log message, \"--graph\" draws a bunch\nof lines on the left hand side and place these rectangles on the\nright of these lines. Space allocated to each of these rectangles\nmay and do begin at a different column. For example, here is an\noutput from\n\n $ git log -12 --graph --oneline\n\n * 7d211c9 Merge branch 'jk/graph-format-padding' into pu\n |\\\n | * ead86db pretty: pass graph width to pretty formatting\n * | 5be4874 Merge branch 'ld/p4-detached-head' into pu\n |\\ \\\n | * | 4086903 git-p4: work with a detached head\n | * | 6d2be82 git-p4: add failing test for submit from detached\n head\n * | | 7cec6a3 Merge branch 'ls/p4-lfs' into pu\n |\\ \\ \\\n | * | | 5fac7db git-p4: add Git LFS backend for large file system\n | * | | 53b938f git-p4: add support for large file systems\n | * | | 760e31c git-p4: return an empty list if a list config has\n no values\n | * | | c1e88b8 git-p4: add gitConfigInt reader\n | * | | 465af7a git-p4: add optional type specifier to gitConfig\n reader\n * | | | 5197bd3 Merge branch 'nd/clone-linked-checkout' into pu\n\nI can understand why you would want to include the varying width of\nthe river on the left hand side in the \"space allocated for the\ncommit\", and under that mental model, the patch makes sense, but at\nthe same time, it is also a perfectly good specification to make\n\"%>|(20)%h\" pad \"%h\" to 20 columns from the left edge of the space\ngiven to the commit.\n\nI have a suspicion that 50% of the users would appreciate this\nchange, and the remainder of the users see this break their\nexpectation. To avoid such a regression, we may be better off if we\nintroduced a new alignment operator that is different from '%>|(N)'\nso that this new behaviour is available to those who want to use it,\nwithout negatively affecting existing uses.\n\nAnd the use of ctx->graph_width in the hunk below would be the only\nthing that needs to be changed (i.e. added to occupied only when the\nnew alignment operator is used).\n\nOther than that, this round of reroll looks ready for 'next'.\n\nDuy what do you think?\n\nThanks.\n\n> diff --git a/pretty.c b/pretty.c\n> index 151c2ae..f1cf9e2 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1297,6 +1297,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */\n> \t\tif (!start)\n> \t\t\tstart = sb->buf;\n> \t\toccupied = utf8_strnwidth(start, -1, 1);\n> +\t\toccupied += c->pretty_ctx->graph_width;\n> \t\tpadding = (-padding) - occupied;\n> \t}\n> \twhile (1) {\n"},{"id":"274894","messageId":"CACsJy8C8DCi_pZDd=m1hE70t7Z+MX3j6WcMe-Hp-u0y5VvwKqg@mail.gmail.com","threadId":"40336","inReplyTo":"xmqqa8sfa7di.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] pretty: Pass graph width to pretty formatting for use in '%>|(N)'","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-12-22T17:03:19Z","receivedAt":"2015-12-22T17:03:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"(I'm joining the newer thread [1] back to this one, thanks for\nreminding me about this)\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/282771\n\nOn Tue, Sep 22, 2015 at 2:40 AM, Junio C Hamano wrote:\n> Josef Kufner writes:\n>\n>> Pass graph width to pretty formatting, so N in '%>|(N)' includes columns\n>> consumed by graph rendered when --graph option is in use.\n>>\n>> Example:\n>> git log --all --graph --pretty='format: [%>|(20)%h] %ar%d'\n>>\n>> All commit hashes should be aligned at 20th column from edge of the\n>> terminal, not from the edge of the graph.\n>>\n>> Signed-off-by: Josef Kufner \n>> ---\n>\n> [CC'ed Duy for ideas, as the \"%>|(ALIGN)\" thing is his invention]\n>\n> If you imagine an entry for a commit in the \"git log\" output as a\n> rectangle that has its commit log message, \"--graph\" draws a bunch\n> of lines on the left hand side and place these rectangles on the\n> right of these lines. Space allocated to each of these rectangles\n> may and do begin at a different column. For example, here is an\n> output from\n>\n> $ git log -12 --graph --oneline\n>\n> * 7d211c9 Merge branch 'jk/graph-format-padding' into pu\n> |\\\n> | * ead86db pretty: pass graph width to pretty formatting\n> * | 5be4874 Merge branch 'ld/p4-detached-head' into pu\n> |\\ \\\n> | * | 4086903 git-p4: work with a detached head\n> | * | 6d2be82 git-p4: add failing test for submit from detached\n> head\n> * | | 7cec6a3 Merge branch 'ls/p4-lfs' into pu\n> |\\ \\ \\\n> | * | | 5fac7db git-p4: add Git LFS backend for large file system\n> | * | | 53b938f git-p4: add support for large file systems\n> | * | | 760e31c git-p4: return an empty list if a list config has\n> no values\n> | * | | c1e88b8 git-p4: add gitConfigInt reader\n> | * | | 465af7a git-p4: add optional type specifier to gitConfig\n> reader\n> * | | | 5197bd3 Merge branch 'nd/clone-linked-checkout' into pu\n>\n> I can understand why you would want to include the varying width of\n> the river on the left hand side in the \"space allocated for the\n> commit\", and under that mental model, the patch makes sense, but at\n> the same time, it is also a perfectly good specification to make\n> \"%>|(20)%h\" pad \"%h\" to 20 columns from the left edge of the space\n> given to the commit.\n>\n> I have a suspicion that 50% of the users would appreciate this\n> change, and the remainder of the users see this break their\n> expectation. To avoid such a regression, we may be better off if we\n> introduced a new alignment operator that is different from '%>|(N)'\n> so that this new behaviour is available to those who want to use it,\n> without negatively affecting existing uses.\n\nI tend to agree with Josef. >|(N) is about columns relative to the\nleft edge of the screen. You can already use >(N) to be relative to\nthe left edge of the space given to the commit.\n-- \nDuy\n"},{"id":"274922","messageId":"xmqqbn9iccwa.fsf@gitster.mtv.corp.google.com","threadId":"40336","inReplyTo":"CACsJy8C8DCi_pZDd=m1hE70t7Z+MX3j6WcMe-Hp-u0y5VvwKqg@mail.gmail.com","subject":"Re: [PATCH 1/3] pretty: Pass graph width to pretty formatting for use in '%>|(N)'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-22T22:54:45Z","receivedAt":"2015-12-22T22:54:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> (I'm joining the newer thread [1] back to this one, thanks for\n> reminding me about this)\n>\n> [1] http://thread.gmane.org/gmane.comp.version-control.git/282771\n>\n> On Tue, Sep 22, 2015 at 2:40 AM, Junio C Hamano wrote:\n>> Josef Kufner writes:\n>>\n>>> Pass graph width to pretty formatting, so N in '%>|(N)' includes columns\n>>> consumed by graph rendered when --graph option is in use.\n>>>\n>>> Example:\n>>> git log --all --graph --pretty='format: [%>|(20)%h] %ar%d'\n>>>\n>>> All commit hashes should be aligned at 20th column from edge of the\n>>> terminal, not from the edge of the graph.\n>>>\n>>> Signed-off-by: Josef Kufner \n>>> ---\n>>\n>> [CC'ed Duy for ideas, as the \"%>|(ALIGN)\" thing is his invention]\n>> ...\n>>\n>> I have a suspicion that 50% of the users would appreciate this\n>> change, and the remainder of the users see this break their\n>> expectation. To avoid such a regression, we may be better off if we\n>> introduced a new alignment operator that is different from '%>|(N)'\n>> so that this new behaviour is available to those who want to use it,\n>> without negatively affecting existing uses.\n>\n> I tend to agree with Josef. >|(N) is about columns relative to the\n> left edge of the screen. You can already use >(N) to be relative to\n> the left edge of the space given to the commit.\n\nOK. I didn't check if >(N) with that old patch still behaves that\nway, but if that is sensible, then it is not breaking anything, so\nit may be a good idea to salvage it. I dunno.\n"}]}