{"thread":{"id":"57773","subject":"[PATCH] show-branch: fix SEGFAULT when `--current` and `--reflog` together","startedAt":"2022-04-21T13:34:10Z","lastAt":"2022-04-21T21:25:57Z","messageCount":7,"participants":["Gregory David","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"454078","messageId":"225b410d-2d98-8c0b-c289-22f753c175d4@p1sec.com","threadId":"57773","inReplyTo":null,"subject":"[PATCH] show-branch: fix SEGFAULT when `--current` and `--reflog` together","fromName":"Gregory David","fromEmail":"gregory.david@p1sec.com","sentAt":"2022-04-21T13:34:03Z","receivedAt":"2022-04-21T13:34:10Z","isPatch":true,"sender":{"key":"gregory.david@p1sec.com","avatar":"https://gravatar.com/avatar/4fdf1717095ab69bdb7ecf30c8f0efdf273e91591f65b043562a93b27239a944?d=mp&s=160"},"body":"If run `show-branch` with `--current` and `--reflog` simultaneously, a\nSEGFAULT appears.\n\nThe bug is that we read over the end of the `reflog_msg` array after\nhaving `append_one_rev()` for the current branch without supplying a\nconvenient message to it.\n\nIt seems that it has been introduced in:\nCommit 1aa68d6735 (show-branch: --current includes the current branch.,\n2006-01-11)\n\nSigned-off-by: Gregory DAVID <gregory.david@p1sec.com>\nThanks-to: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/show-branch.c  | 22 +++++++++++++++++++--\n t/t3202-show-branch.sh | 43 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 63 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex e12c5e80e3..c8d830b7c6 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -812,8 +812,26 @@ int cmd_show_branch(int ac, const char **av, const\nchar *prefix)\n \t\t}\n \t\tif (!has_head) {\n \t\t\tconst char *name = head;\n-\t\t\tskip_prefix(name, \"refs/heads/\", &name);\n-\t\t\tappend_one_rev(name);\n+\t\t\tstruct object_id oid;\n+\t\t\tchar *ref;\n+\t\t\tunsigned int flags = 0;\n+\t\t\tchar *log_msg = 0;\n+\t\t\tchar *end_log_msg;\n+\t\t\ttimestamp_t timestamp;\n+\t\t\tint tz;\n+\n+      if (!dwim_ref(*av, strlen(*av), &oid, &ref, 0))\n+        die(_(\"no such ref %s\"), *av);\n+      read_ref_at(get_main_ref_store(the_repository),\n+                  ref, flags, 0, i, &oid, &log_msg,\n+                  &timestamp, &tz, NULL);\n+      end_log_msg = strchr(log_msg, '\\n');\n+      if (end_log_msg)\n+        *end_log_msg = '\\0';\n+      skip_prefix(name, \"refs/heads/\", &name);\n+      append_one_rev(name);\n+      reflog_msg[ref_name_cnt - 1] = xstrfmt(\"(%s) (current) %s\",\nshow_date(timestamp, tz, DATE_MODE(RELATIVE)), (!log_msg || !*log_msg) ?\n\"(none)\" : log_msg);\n+      free(log_msg);\n \t\t}\n \t}\n diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\nindex 7a1be73ce8..7f6ffcf8a5 100755\n--- a/t/t3202-show-branch.sh\n+++ b/t/t3202-show-branch.sh\n@@ -161,4 +161,47 @@ test_expect_success 'show branch --reflog=2' '\n \ttest_cmp actual expect\n '\n +test_expect_success 'show branch --reflog=2 --current' '\n+\tsed \"s/^>\t//\" >expect <<-\\EOF &&\n+\t>\t! [refs/heads/branch10@{0}] (4 years, 5 months ago) commit: branch10\n+\t>\t ! [refs/heads/branch10@{1}] (4 years, 5 months ago) commit: branch10\n+\t>\t  * [branch10] (4 years, 5 months ago) (current) branch: Created\nfrom initial\n+\t>\t---\n+\t>\t+ * [refs/heads/branch10@{0}] branch10\n+\t>\t++* [refs/heads/branch10@{1}] initial\n+\tEOF\n+\tgit show-branch --reflog=2 --current >actual &&\n+\ttest_cmp actual expect\n+'\n+\n+test_expect_success 'show branch --current' '\n+\tsed \"s/^>\t//\" >expect <<-\\EOF &&\n+\t>\t! [branch1] branch1\n+\t>\t ! [branch2] branch2\n+\t>\t  ! [branch3] branch3\n+\t>\t   ! [branch4] branch4\n+\t>\t    ! [branch5] branch5\n+\t>\t     ! [branch6] branch6\n+\t>\t      ! [branch7] branch7\n+\t>\t       ! [branch8] branch8\n+\t>\t        ! [branch9] branch9\n+\t>\t         * [branch10] branch10\n+\t>\t          ! [master] initial\n+\t>\t-----------\n+\t>\t         *  [branch10] branch10\n+\t>\t        +   [branch9] branch9\n+\t>\t       +    [branch8] branch8\n+\t>\t      +     [branch7] branch7\n+\t>\t     +      [branch6] branch6\n+\t>\t    +       [branch5] branch5\n+\t>\t   +        [branch4] branch4\n+\t>\t  +         [branch3] branch3\n+\t>\t +          [branch2] branch2\n+\t>\t+           [branch1] branch1\n+\t>\t+++++++++*+ [master] initial\n+\tEOF\n+\tgit show-branch --current >actual &&\n+\ttest_cmp actual expect\n+'\n+\n test_done\n-- \n2.35.1"},{"id":"454083","messageId":"cover-v3-0.2-00000000000-20220421T152704Z-avarab@gmail.com","threadId":"57773","inReplyTo":"225b410d-2d98-8c0b-c289-22f753c175d4@p1sec.com","subject":"[PATCH v3 0/2] show-brach: segfault fix from Gregory David","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-21T15:33:46Z","receivedAt":"2022-04-21T15:34:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This is a version of\nhttps://lore.kernel.org/git/225b410d-2d98-8c0b-c289-22f753c175d4@p1sec.com/\nthat applies with \"git am\", the patch got munged in transit (from the\nlooks of it from Thunderbird's wrapping).\n\nI added a 1/2 to factor out some shared code used by the segfault fix,\nand made the now-smaller 2/2 use that function.\n\nI also renamed some varibles to reduce wrapping, and made the\nappend_one_rev() come after the ref formatting. Other similar code\nabove does that, it looks to me like they're equivalent\n(append_one_rev increments ref_name_cnt).\n\nGregory David (1):\n  show-branch: fix SEGFAULT when `--current` and `--reflog` together\n\nÆvar Arnfjörð Bjarmason (1):\n  show-branch: refactor in preparation for next commit\n\n builtin/show-branch.c  | 47 ++++++++++++++++++++++++++++++------------\n t/t3202-show-branch.sh | 43 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 77 insertions(+), 13 deletions(-)\n\n-- \n2.36.0.876.g7efc8a7728c\n\n"},{"id":"454084","messageId":"patch-v3-1.2-879930b7a66-20220421T152704Z-avarab@gmail.com","threadId":"57773","inReplyTo":"cover-v3-0.2-00000000000-20220421T152704Z-avarab@gmail.com","subject":"[PATCH v3 1/2] show-branch: refactor in preparation for next commit","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-21T15:33:47Z","receivedAt":"2022-04-21T15:34:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the code in cmd_show_branch() that formats a reflog message for\nus into a function, and change the \"flags\" variable that we never\nchange into a \"const\", in addition to moving it up a scope in\npreparation for the subsequent commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/show-branch.c | 34 +++++++++++++++++++++-------------\n 1 file changed, 21 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 330b0553b9d..499ef76a508 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -618,6 +618,24 @@ static int parse_reflog_param(const struct option *opt, const char *arg,\n \treturn 0;\n }\n \n+static char *fmt_reflog(char *const logmsg, const timestamp_t ts, const int tz,\n+\t\t\tconst char *fmt)\n+{\n+\tchar *const end = strchr(logmsg, '\\n');\n+\tconst char *msg;\n+\tchar *ret;\n+\n+\tif (end)\n+\t\t*end = '\\0';\n+\n+\tmsg = *logmsg ? logmsg : \"(none)\";\n+\n+\tret = xstrfmt(fmt, show_date(ts, tz, DATE_MODE(RELATIVE)), msg);\n+\tfree(logmsg);\n+\n+\treturn ret;\n+}\n+\n int cmd_show_branch(int ac, const char **av, const char *prefix)\n {\n \tstruct commit *rev[MAX_REVS], *commit;\n@@ -640,6 +658,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \tint topics = 0;\n \tint dense = 1;\n \tconst char *reflog_base = NULL;\n+\tconst unsigned int flags = 0;\n \tstruct option builtin_show_branch_options[] = {\n \t\tOPT_BOOL('a', \"all\", &all_heads,\n \t\t\t N_(\"show remote-tracking and local branches\")),\n@@ -720,7 +739,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\tstruct object_id oid;\n \t\tchar *ref;\n \t\tint base = 0;\n-\t\tunsigned int flags = 0;\n \n \t\tif (ac == 0) {\n \t\t\tstatic const char *fake_av[2];\n@@ -761,8 +779,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\tfor (i = 0; i < reflog; i++) {\n \t\t\tchar *logmsg;\n \t\t\tchar *nth_desc;\n-\t\t\tconst char *msg;\n-\t\t\tchar *end;\n \t\t\ttimestamp_t timestamp;\n \t\t\tint tz;\n \n@@ -773,16 +789,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t\tbreak;\n \t\t\t}\n \n-\t\t\tend = strchr(logmsg, '\\n');\n-\t\t\tif (end)\n-\t\t\t\t*end = '\\0';\n-\n-\t\t\tmsg = (*logmsg == '\\0') ? \"(none)\" : logmsg;\n-\t\t\treflog_msg[i] = xstrfmt(\"(%s) %s\",\n-\t\t\t\t\t\tshow_date(timestamp, tz,\n-\t\t\t\t\t\t\t  DATE_MODE(RELATIVE)),\n-\t\t\t\t\t\tmsg);\n-\t\t\tfree(logmsg);\n+\t\t\treflog_msg[i] = fmt_reflog(logmsg, timestamp, tz,\n+\t\t\t\t\t\t   \"(%s) %s\");\n \n \t\t\tnth_desc = xstrfmt(\"%s@{%d}\", *av, base+i);\n \t\t\tappend_ref(nth_desc, &oid, 1);\n-- \n2.36.0.876.g7efc8a7728c\n\n"},{"id":"454085","messageId":"patch-v3-2.2-396c3338533-20220421T152704Z-avarab@gmail.com","threadId":"57773","inReplyTo":"cover-v3-0.2-00000000000-20220421T152704Z-avarab@gmail.com","subject":"[PATCH v3 2/2] show-branch: fix SEGFAULT when `--current` and `--reflog` together","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-21T15:33:48Z","receivedAt":"2022-04-21T15:34:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Gregory David <gregory.david@p1sec.com>\n\nIf run `show-branch` with `--current` and `--reflog` simultaneously, a\nSEGFAULT appears.\n\nThe bug is that we read over the end of the `reflog_msg` array after\nhaving `append_one_rev()` for the current branch without supplying a\nconvenient message to it.\n\nIt seems that it has been introduced in:\nCommit 1aa68d6735 (show-branch: --current includes the current branch.,\n2006-01-11)\n\nSigned-off-by: Gregory DAVID <gregory.david@p1sec.com>\nThanks-to: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/show-branch.c  | 13 +++++++++++++\n t/t3202-show-branch.sh | 43 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 56 insertions(+)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 499ef76a508..50c17fb5c31 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -821,6 +821,19 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t}\n \t\tif (!has_head) {\n \t\t\tconst char *name = head;\n+\t\t\tstruct object_id oid;\n+\t\t\tchar *ref;\n+\t\t\tchar *msg;\n+\t\t\ttimestamp_t ts;\n+\t\t\tint tz;\n+\n+\t\t\tif (!dwim_ref(*av, strlen(*av), &oid, &ref, 0))\n+\t\t\t\tdie(_(\"no such ref %s\"), *av);\n+\t\t\tread_ref_at(get_main_ref_store(the_repository), ref,\n+\t\t\t\t    flags, 0, i, &oid, &msg, &ts, &tz, NULL);\n+\n+\t\t\treflog_msg[ref_name_cnt] = fmt_reflog(msg, ts, tz,\n+\t\t\t\t\t\t\t      \"(%s) (current) %s\");\n \t\t\tskip_prefix(name, \"refs/heads/\", &name);\n \t\t\tappend_one_rev(name);\n \t\t}\ndiff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\nindex 7a1be73ce87..7f6ffcf8a57 100755\n--- a/t/t3202-show-branch.sh\n+++ b/t/t3202-show-branch.sh\n@@ -161,4 +161,47 @@ test_expect_success 'show branch --reflog=2' '\n \ttest_cmp actual expect\n '\n \n+test_expect_success 'show branch --reflog=2 --current' '\n+\tsed \"s/^>\t//\" >expect <<-\\EOF &&\n+\t>\t! [refs/heads/branch10@{0}] (4 years, 5 months ago) commit: branch10\n+\t>\t ! [refs/heads/branch10@{1}] (4 years, 5 months ago) commit: branch10\n+\t>\t  * [branch10] (4 years, 5 months ago) (current) branch: Created from initial\n+\t>\t---\n+\t>\t+ * [refs/heads/branch10@{0}] branch10\n+\t>\t++* [refs/heads/branch10@{1}] initial\n+\tEOF\n+\tgit show-branch --reflog=2 --current >actual &&\n+\ttest_cmp actual expect\n+'\n+\n+test_expect_success 'show branch --current' '\n+\tsed \"s/^>\t//\" >expect <<-\\EOF &&\n+\t>\t! [branch1] branch1\n+\t>\t ! [branch2] branch2\n+\t>\t  ! [branch3] branch3\n+\t>\t   ! [branch4] branch4\n+\t>\t    ! [branch5] branch5\n+\t>\t     ! [branch6] branch6\n+\t>\t      ! [branch7] branch7\n+\t>\t       ! [branch8] branch8\n+\t>\t        ! [branch9] branch9\n+\t>\t         * [branch10] branch10\n+\t>\t          ! [master] initial\n+\t>\t-----------\n+\t>\t         *  [branch10] branch10\n+\t>\t        +   [branch9] branch9\n+\t>\t       +    [branch8] branch8\n+\t>\t      +     [branch7] branch7\n+\t>\t     +      [branch6] branch6\n+\t>\t    +       [branch5] branch5\n+\t>\t   +        [branch4] branch4\n+\t>\t  +         [branch3] branch3\n+\t>\t +          [branch2] branch2\n+\t>\t+           [branch1] branch1\n+\t>\t+++++++++*+ [master] initial\n+\tEOF\n+\tgit show-branch --current >actual &&\n+\ttest_cmp actual expect\n+'\n+\n test_done\n-- \n2.36.0.876.g7efc8a7728c\n\n"},{"id":"454099","messageId":"xmqqr15qi9tg.fsf@gitster.g","threadId":"57773","inReplyTo":"patch-v3-1.2-879930b7a66-20220421T152704Z-avarab@gmail.com","subject":"Re: [PATCH v3 1/2] show-branch: refactor in preparation for next commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-21T18:13:31Z","receivedAt":"2022-04-21T18:13:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Move the code in cmd_show_branch() that formats a reflog message for\n> us into a function,\n\nMakes quite a lot of sense.\n\n\"factoring out the formatting of reflog message, given a single\nreflog entry\", is a very good unit of work that is independently\ngood and can be evaluated as such.\n\n> and change the \"flags\" variable that we never\n> change into a \"const\", in addition to moving it up a scope in\n> preparation for the subsequent commit.\n\nPerhaps doing that as part of the next step is better (or not doing\nit if it can be helped)?  Mixing it in contaminates a good isolation\ndone with an unrelated change.  We somehow ended up seeing too many\n\"while at it\" in the recent patches, and I think that needs to stop.\nWe should go back to better patch hygiene.\n\nOther than that, looks sensible.  Let's see what 2/2 does.\n\nThanks.\n\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  builtin/show-branch.c | 34 +++++++++++++++++++++-------------\n>  1 file changed, 21 insertions(+), 13 deletions(-)\n>\n> diff --git a/builtin/show-branch.c b/builtin/show-branch.c\n> index 330b0553b9d..499ef76a508 100644\n> --- a/builtin/show-branch.c\n> +++ b/builtin/show-branch.c\n> @@ -618,6 +618,24 @@ static int parse_reflog_param(const struct option *opt, const char *arg,\n>  \treturn 0;\n>  }\n>  \n> +static char *fmt_reflog(char *const logmsg, const timestamp_t ts, const int tz,\n> +\t\t\tconst char *fmt)\n> +{\n> +\tchar *const end = strchr(logmsg, '\\n');\n> +\tconst char *msg;\n> +\tchar *ret;\n> +\n> +\tif (end)\n> +\t\t*end = '\\0';\n> +\n> +\tmsg = *logmsg ? logmsg : \"(none)\";\n> +\n> +\tret = xstrfmt(fmt, show_date(ts, tz, DATE_MODE(RELATIVE)), msg);\n> +\tfree(logmsg);\n> +\n> +\treturn ret;\n> +}\n> +\n>  int cmd_show_branch(int ac, const char **av, const char *prefix)\n>  {\n>  \tstruct commit *rev[MAX_REVS], *commit;\n> @@ -640,6 +658,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n>  \tint topics = 0;\n>  \tint dense = 1;\n>  \tconst char *reflog_base = NULL;\n> +\tconst unsigned int flags = 0;\n>  \tstruct option builtin_show_branch_options[] = {\n>  \t\tOPT_BOOL('a', \"all\", &all_heads,\n>  \t\t\t N_(\"show remote-tracking and local branches\")),\n> @@ -720,7 +739,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n>  \t\tstruct object_id oid;\n>  \t\tchar *ref;\n>  \t\tint base = 0;\n> -\t\tunsigned int flags = 0;\n>  \n>  \t\tif (ac == 0) {\n>  \t\t\tstatic const char *fake_av[2];\n> @@ -761,8 +779,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n>  \t\tfor (i = 0; i < reflog; i++) {\n>  \t\t\tchar *logmsg;\n>  \t\t\tchar *nth_desc;\n> -\t\t\tconst char *msg;\n> -\t\t\tchar *end;\n>  \t\t\ttimestamp_t timestamp;\n>  \t\t\tint tz;\n>  \n> @@ -773,16 +789,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n>  \t\t\t\tbreak;\n>  \t\t\t}\n>  \n> -\t\t\tend = strchr(logmsg, '\\n');\n> -\t\t\tif (end)\n> -\t\t\t\t*end = '\\0';\n> -\n> -\t\t\tmsg = (*logmsg == '\\0') ? \"(none)\" : logmsg;\n> -\t\t\treflog_msg[i] = xstrfmt(\"(%s) %s\",\n> -\t\t\t\t\t\tshow_date(timestamp, tz,\n> -\t\t\t\t\t\t\t  DATE_MODE(RELATIVE)),\n> -\t\t\t\t\t\tmsg);\n> -\t\t\tfree(logmsg);\n> +\t\t\treflog_msg[i] = fmt_reflog(logmsg, timestamp, tz,\n> +\t\t\t\t\t\t   \"(%s) %s\");\n>  \n>  \t\t\tnth_desc = xstrfmt(\"%s@{%d}\", *av, base+i);\n>  \t\t\tappend_ref(nth_desc, &oid, 1);\n"},{"id":"454140","messageId":"xmqqczhai8qv.fsf@gitster.g","threadId":"57773","inReplyTo":"patch-v3-2.2-396c3338533-20220421T152704Z-avarab@gmail.com","subject":"Re: [PATCH v3 2/2] show-branch: fix SEGFAULT when `--current` and `--reflog` together","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-21T18:36:40Z","receivedAt":"2022-04-21T18:37:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> From: Gregory David <gregory.david@p1sec.com>\n>\n> If run `show-branch` with `--current` and `--reflog` simultaneously, a\n> SEGFAULT appears.\n>\n> The bug is that we read over the end of the `reflog_msg` array after\n> having `append_one_rev()` for the current branch without supplying a\n> convenient message to it.\n>\n> It seems that it has been introduced in:\n> Commit 1aa68d6735 (show-branch: --current includes the current branch.,\n> 2006-01-11)\n>\n> Signed-off-by: Gregory DAVID <gregory.david@p1sec.com>\n> Thanks-to: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  builtin/show-branch.c  | 13 +++++++++++++\n>  t/t3202-show-branch.sh | 43 ++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 56 insertions(+)\n>\n> diff --git a/builtin/show-branch.c b/builtin/show-branch.c\n> index 499ef76a508..50c17fb5c31 100644\n> --- a/builtin/show-branch.c\n> +++ b/builtin/show-branch.c\n> @@ -821,6 +821,19 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n>  \t\t}\n>  \t\tif (!has_head) {\n>  \t\t\tconst char *name = head;\n> +\t\t\tstruct object_id oid;\n> +\t\t\tchar *ref;\n> +\t\t\tchar *msg;\n> +\t\t\ttimestamp_t ts;\n> +\t\t\tint tz;\n> +\n> +\t\t\tif (!dwim_ref(*av, strlen(*av), &oid, &ref, 0))\n> +\t\t\t\tdie(_(\"no such ref %s\"), *av);\n> +\t\t\tread_ref_at(get_main_ref_store(the_repository), ref,\n> +\t\t\t\t    flags, 0, i, &oid, &msg, &ts, &tz, NULL);\n\nDo we really mean to pass 'i', which is the number of other things we have added,\nto read_ref_at()?  Does that mean\n\n    git show-branch -g4 --current\n\nshows the 4th entry from the current branch while\n\n    git show-branch -g2 --current\n\nshows the second?\n\n> +\t\t\treflog_msg[ref_name_cnt] = fmt_reflog(msg, ts, tz,\n> +\t\t\t\t\t\t\t      \"(%s) (current) %s\");\n\nThis unconditionally reads reflog and adds to reflog_msg[], without\neven seeing if we are actually showing reflog entries.  Is that\nsensible?\n\nI am wondering if we should have factored out a bit more in the\nprevious step.  Instead of \"here is what we read from read_ref_at(),\nformat it\", I wonder if the interface should be \"here is a ref and\nthe offset N; format the Nth entry\".  And then this part can become\nsomething like (I do not know about 'i', but that is meant to be the\nreflog offset, i.e. \"HEAD@{i}\").\n\n\tif (!has_head) {\n\t\tif (reflog) {\n\t\t\tdwim_ref to learn ref;\n\t\t\treflog_msg[ref_name_cnt] = new_helper(ref, i);\n\t\t} else {\n\t\t\tskip_prefix(name, \"refs/heads/\", head);\n\t\t\tappend_one_rev(name);\n\t\t}\n\t}\n\nIn any case, I am not sure if it even makes sense to allow the\nreflog listing mode with \"current\" in the first place, and a simpler\noption may be to just forbid the combination.  After all, when I\nadeed \"--current\" to \"git show-branch\" in 1aa68d67 (show-branch:\n--current includes the current branch., 2006-01-11), it was clearly\nmeant to be used with \"other branches\"---\"I would list branches I\ncare about and I use as anchoring points on the command line, and I\nmay or may not be on one of these main branches. Please make sure I\ncan view the current one with respect to these other branches\" is\nwhat \"--current\" is about, and mixing it with \"how do recent reflog\nentries relate to each other\" does not make much sense.\n"},{"id":"454182","messageId":"xmqqh76mf7s4.fsf_-_@gitster.g","threadId":"57773","inReplyTo":"xmqqczhai8qv.fsf@gitster.g","subject":"[PATCH] show-branch: -g and --current are incompatible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-21T21:25:47Z","receivedAt":"2022-04-21T21:25:57Z","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> In any case, I am not sure if it even makes sense to allow the\n> reflog listing mode with \"current\" in the first place, and a simpler\n> option may be to just forbid the combination.  After all, when I\n> adeed \"--current\" to \"git show-branch\" in 1aa68d67 (show-branch:\n> --current includes the current branch., 2006-01-11), it was clearly\n> meant to be used with \"other branches\"---\"I would list branches I\n> care about and I use as anchoring points on the command line, and I\n> may or may not be on one of these main branches. Please make sure I\n> can view the current one with respect to these other branches\" is\n> what \"--current\" is about, and mixing it with \"how do recent reflog\n> entries relate to each other\" does not make much sense.\n\nSo, here is an alternative \"fix\" along that line.\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: [PATCH] show-branch: -g and --current are incompatible\n\nWhen \"--current\" is given to \"git show-branch\" running in the\n\"--reflog\" mode, the code tries to reference a \"reflog\" message\nthat does not even exist.  This is because the --current is not\nprepared to work in that mode.\n\nThe reason \"--current\" exists is to support this request:\n\n    I list branches on the command line.  These are the branchesI\n    care about and I use as anchoring points. I may or may not be on\n    one of these main branches.  Please make sure I can view the\n    commits on the current branch with respect to what is in these\n    other branches.\n\nAnd to serve that request, the code checks if the current branch is\namong the ones listed on the command line, and adds it only if it is\nnot to the end of one array, which essentially lists the objects.\nThe reflog mode additionally uses another array to list reflog\nmessages, which the \"--current\" code does not add to.  This leaves\none uninitialized slot at the end of the array of reflog messages,\nand causes the program to show garbage or segfault.\n\nCatch the unsupported (and meaningless) combination and exit with a\nusage error.\n\nThere are other combinations of options that are incompatible but\nhave not been tested.  Add test to cover them while adding coverage\nfor this new combination.\n\nReported-by: Gregory David <gregory.david@p1sec.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/show-branch.c  |  4 ++++\n t/t3202-show-branch.sh | 14 ++++++++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git c/builtin/show-branch.c w/builtin/show-branch.c\nindex e12c5e80e3..73e8edbf02 100644\n--- c/builtin/show-branch.c\n+++ w/builtin/show-branch.c\n@@ -711,6 +711,10 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t\t\"--all/--remotes/--independent/--merge-base\");\n \t}\n \n+\tif (with_current_branch && reflog)\n+\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t    \"--reflog\", \"--current\");\n+\n \t/* If nothing is specified, show all branches by default */\n \tif (ac <= topics && all_heads + all_remotes == 0)\n \t\tall_heads = 1;\ndiff --git c/t/t3202-show-branch.sh w/t/t3202-show-branch.sh\nindex 7a1be73ce8..f2b9199007 100755\n--- c/t/t3202-show-branch.sh\n+++ w/t/t3202-show-branch.sh\n@@ -161,4 +161,18 @@ test_expect_success 'show branch --reflog=2' '\n \ttest_cmp actual expect\n '\n \n+# incompatible options\n+while read combo\n+do\n+\ttest_expect_success \"show-branch $combo (should fail)\" '\n+\t\ttest_must_fail git show-branch $combo 2>error &&\n+\t\tgrep -e \"cannot be used together\" -e \"usage:\" error\n+\t'\n+done <<\\EOF\n+--all --reflog\n+--merge-base --reflog\n+--list --merge-base\n+--reflog --current\n+EOF\n+\n test_done\n"}]}