{"thread":{"id":"59273","subject":"[PATCH 0/1] range-diff: let '--abbrev' option takes effect","startedAt":"2023-02-20T14:25:02Z","lastAt":"2023-02-28T23:44:58Z","messageCount":5,"participants":["Teng Long","Junio C Hamano","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"472336","messageId":"20230220142444.18739-1-tenglong.tl@alibaba-inc.com","threadId":"59273","inReplyTo":null,"subject":"[PATCH 0/1] range-diff: let '--abbrev' option takes effect","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-02-20T14:24:43Z","receivedAt":"2023-02-20T14:25:02Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nLet's support --abbrev in range-diff output.\n\nIn some other sub-commands, there may be similar ineffective\nsituations. If found, it will be continuously updated\nin subsequent patches.\n\nTeng Long (1):\n range-diff: let '--abbrev' option takes effect\n\n range-diff.c | 11 +++++++----\n t/t3206-range-diff.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 39 insertions(+), 4 deletions(-)\n\n-- \n2.39.2.459.g1d9d282d\n\n"},{"id":"472337","messageId":"20230220142444.18739-2-tenglong.tl@alibaba-inc.com","threadId":"59273","inReplyTo":"20230220142444.18739-1-tenglong.tl@alibaba-inc.com","subject":"[PATCH 1/1] range-diff: let '--abbrev' option takes effect","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-02-20T14:24:44Z","receivedAt":"2023-02-20T14:25:05Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nAs mentioned in 'git-range-diff.txt': \"`git range-diff` also accepts the\nregular diff options (see linkgit:git-diff[1])...\", but '--abbrev' is not\nin the \"regular\" scope.\n\nIn Git, the \"abbrev\" of an object may not be a fixed value in different\nrepositories, depending on the needs of the them(Linus mentioned in\ne6c587c7 in 2016: \"the Linux kernel project needs 11 to 12 hexdigits\"\nat that time ), that's why a user may want to display abbrev according\nto a specified length.\n\nAlthough a similar effect can be achieved through configuration (like:\ngit -c core.abbrev=), but based on ease of use (many users may not\nknow that the -c option can be specified) and the description in existing\ndocument, supporting users to directly use '--abbrev', could be a good way.\n\nSigned-off-by: Teng Long \n---\n range-diff.c | 11 +++++++----\n t/t3206-range-diff.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 39 insertions(+), 4 deletions(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 8255ab43..086365df 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -383,11 +383,14 @@ static void output_pair_header(struct diff_options *diffopt,\n \tconst char *color_new = diff_get_color_opt(diffopt, DIFF_FILE_NEW);\n \tconst char *color_commit = diff_get_color_opt(diffopt, DIFF_COMMIT);\n \tconst char *color;\n+\tchar abbrev = diffopt->abbrev;\n+\n+\tif (abbrev < 0)\n+\t\tabbrev = DEFAULT_ABBREV;\n \n \tif (!dashes->len)\n \t\tstrbuf_addchars(dashes, '-',\n-\t\t\t\tstrlen(find_unique_abbrev(oid,\n-\t\t\t\t\t\t\t DEFAULT_ABBREV)));\n+\t\t\t\tstrlen(find_unique_abbrev(oid, abbrev)));\n \n \tif (!b_util) {\n \t\tcolor = color_old;\n@@ -409,7 +412,7 @@ static void output_pair_header(struct diff_options *diffopt,\n \t\tstrbuf_addf(buf, \"%*s: %s \", patch_no_width, \"-\", dashes->buf);\n \telse\n \t\tstrbuf_addf(buf, \"%*d: %s \", patch_no_width, a_util->i + 1,\n-\t\t\t find_unique_abbrev(&a_util->oid, DEFAULT_ABBREV));\n+\t\t\t find_unique_abbrev(&a_util->oid, abbrev));\n \n \tif (status == '!')\n \t\tstrbuf_addf(buf, \"%s%s\", color_reset, color);\n@@ -421,7 +424,7 @@ static void output_pair_header(struct diff_options *diffopt,\n \t\tstrbuf_addf(buf, \" %*s: %s\", patch_no_width, \"-\", dashes->buf);\n \telse\n \t\tstrbuf_addf(buf, \" %*d: %s\", patch_no_width, b_util->i + 1,\n-\t\t\t find_unique_abbrev(&b_util->oid, DEFAULT_ABBREV));\n+\t\t\t find_unique_abbrev(&b_util->oid, abbrev));\n \n \tcommit = lookup_commit_reference(the_repository, oid);\n \tif (commit) {\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 84dd0cd2..b5f4d6a6 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -33,6 +33,26 @@ test_expect_success 'setup' '\n \tu3 sha256:736c4bc\n \tu4 sha256:673e77d\n \n+\t# topic (abbrev=10)\n+\tt1_abbrev sha1:4de457d2c0\n+\tt2_abbrev sha1:fccce22f8c\n+\tt3_abbrev sha1:147e64ef53\n+\tt4_abbrev sha1:a63e992599\n+\tt1_abbrev sha256:b89f8b9092\n+\tt2_abbrev sha256:5f12aadf34\n+\tt3_abbrev sha256:ea8b273a6c\n+\tt4_abbrev sha256:14b73361fc\n+\n+\t# unmodified (abbrev=10)\n+\tu1_abbrev sha1:35b9b25f76\n+\tu2_abbrev sha1:de345ab3de\n+\tu3_abbrev sha1:9af6654000\n+\tu4_abbrev sha1:2901f773f3\n+\tu1_abbrev sha256:e3731be242\n+\tu2_abbrev sha256:14fadf8cee\n+\tu3_abbrev sha256:736c4bcb44\n+\tu4_abbrev sha256:673e77d589\n+\n \t# reordered\n \tr1 sha1:aca177a\n \tr2 sha1:14ad629\n@@ -153,6 +173,18 @@ test_expect_success 'simple A B C (unmodified)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'simple A..B A..C (unmodified) with --abbrev' '\n+\tgit range-diff --no-color --abbrev=10 main..topic main..unmodified \\\n+\t\t>actual &&\n+\tcat >expect <<-EOF &&\n+\t1: $(test_oid t1_abbrev) = 1: $(test_oid u1_abbrev) s/5/A/\n+\t2: $(test_oid t2_abbrev) = 2: $(test_oid u2_abbrev) s/4/A/\n+\t3: $(test_oid t3_abbrev) = 3: $(test_oid u3_abbrev) s/11/B/\n+\t4: $(test_oid t4_abbrev) = 4: $(test_oid u4_abbrev) s/12/B/\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'A^! and A^- (unmodified)' '\n \tgit range-diff --no-color topic^! unmodified^-1 >actual &&\n \tcat >expect <<-EOF &&\n-- \n2.39.2.459.g1d9d282d\n\n"},{"id":"472410","messageId":"xmqqzg96d6f2.fsf@gitster.g","threadId":"59273","inReplyTo":"20230220142444.18739-2-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH 1/1] range-diff: let '--abbrev' option takes effect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-21T22:01:37Z","receivedAt":"2023-02-21T22:01:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> diff --git a/range-diff.c b/range-diff.c\n> index 8255ab43..086365df 100644\n> --- a/range-diff.c\n> +++ b/range-diff.c\n> @@ -383,11 +383,14 @@ static void output_pair_header(struct diff_options *diffopt,\n> \tconst char *color_new = diff_get_color_opt(diffopt, DIFF_FILE_NEW);\n> \tconst char *color_commit = diff_get_color_opt(diffopt, DIFF_COMMIT);\n> \tconst char *color;\n> +\tchar abbrev = diffopt->abbrev;\n> +\n> +\tif (abbrev < 0)\n> +\t\tabbrev = DEFAULT_ABBREV;\n\nOK. I _think_ this explicit defaulting to DEFAULT_ABBREV does not\nneed to exist, as find_unique_abbrev() falls back to the default\nwhen given a negative value anyway, but it is good to be explicit.\n\nWill queue. Thanks.\n"},{"id":"472850","messageId":"1156586e-9dbe-335e-7e33-74eea7913e86@web.de","threadId":"59273","inReplyTo":"20230220142444.18739-2-tenglong.tl@alibaba-inc.com","subject":"[PATCH] range-diff: avoid compiler warning when char is unsigned","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-02-28T16:13:27Z","receivedAt":"2023-02-28T16:13:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Since 2b15969f61 (range-diff: let '--abbrev' option takes effect,\n2023-02-20), GCC 11.3 on Ubuntu 22.04 on aarch64 warns (and errors\nout if the make variable DEVELOPER is set):\n\nrange-diff.c: In function ‘output_pair_header’:\nrange-diff.c:388:20: error: comparison is always false due to limited range of data type [-Werror=type-limits]\n 388 | if (abbrev < 0)\n | ^\ncc1: all warnings being treated as errors\n\nThat's because char is unsigned on that platform. Use int instead, just\nlike in struct diff_options, to copy the value faithfully.\n\nSigned-off-by: René Scharfe \n---\n range-diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 086365dffb..4bd65ab749 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -383,7 +383,7 @@ static void output_pair_header(struct diff_options *diffopt,\n \tconst char *color_new = diff_get_color_opt(diffopt, DIFF_FILE_NEW);\n \tconst char *color_commit = diff_get_color_opt(diffopt, DIFF_COMMIT);\n \tconst char *color;\n-\tchar abbrev = diffopt->abbrev;\n+\tint abbrev = diffopt->abbrev;\n\n \tif (abbrev < 0)\n \t\tabbrev = DEFAULT_ABBREV;\n--\n2.34.1\n"},{"id":"472874","messageId":"xmqqh6v5pd6x.fsf@gitster.g","threadId":"59273","inReplyTo":"1156586e-9dbe-335e-7e33-74eea7913e86@web.de","subject":"Re: [PATCH] range-diff: avoid compiler warning when char is unsigned","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-28T23:44:54Z","receivedAt":"2023-02-28T23:44:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Since 2b15969f61 (range-diff: let '--abbrev' option takes effect,\n> 2023-02-20), GCC 11.3 on Ubuntu 22.04 on aarch64 warns (and errors\n> out if the make variable DEVELOPER is set):\n>\n> range-diff.c: In function ‘output_pair_header’:\n> range-diff.c:388:20: error: comparison is always false due to limited range of data type [-Werror=type-limits]\n> 388 | if (abbrev < 0)\n> | ^\n> cc1: all warnings being treated as errors\n>\n> That's because char is unsigned on that platform. Use int instead, just\n> like in struct diff_options, to copy the value faithfully.\n>\n> Signed-off-by: René Scharfe \n> ---\n> range-diff.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nVery clearly explained. The patch does make sense.\n\nThanks.\n\n> diff --git a/range-diff.c b/range-diff.c\n> index 086365dffb..4bd65ab749 100644\n> --- a/range-diff.c\n> +++ b/range-diff.c\n> @@ -383,7 +383,7 @@ static void output_pair_header(struct diff_options *diffopt,\n> \tconst char *color_new = diff_get_color_opt(diffopt, DIFF_FILE_NEW);\n> \tconst char *color_commit = diff_get_color_opt(diffopt, DIFF_COMMIT);\n> \tconst char *color;\n> -\tchar abbrev = diffopt->abbrev;\n> +\tint abbrev = diffopt->abbrev;\n>\n> \tif (abbrev < 0)\n> \t\tabbrev = DEFAULT_ABBREV;\n> --\n> 2.34.1\n"}]}