| Message ID | patch-v3-2.2-396c3338533-20220421T152704Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | show-brach: segfault fix from Gregory David | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > From: Gregory David <gregory.david@p1sec.com> > > If run `show-branch` with `--current` and `--reflog` simultaneously, a > SEGFAULT appears. > > The bug is that we read over the end of the `reflog_msg` array after > having `append_one_rev()` for the current branch without supplying a > convenient message to it. > > It seems that it has been introduced in: > Commit 1aa68d6735 (show-branch: --current includes the current branch., > 2006-01-11) > > Signed-off-by: Gregory DAVID <gregory.david@p1sec.com> > Thanks-to: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/show-branch.c | 13 +++++++++++++ > t/t3202-show-branch.sh | 43 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/builtin/show-branch.c b/builtin/show-branch.c > index 499ef76a508..50c17fb5c31 100644 > --- a/builtin/show-branch.c > +++ b/builtin/show-branch.c > @@ -821,6 +821,19 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) > } > if (!has_head) { > const char *name = head; > + struct object_id oid; > + char *ref; > + char *msg; > + timestamp_t ts; > + int tz; > + > + if (!dwim_ref(*av, strlen(*av), &oid, &ref, 0)) > + die(_("no such ref %s"), *av); > + read_ref_at(get_main_ref_store(the_repository), ref, > + flags, 0, i, &oid, &msg, &ts, &tz, NULL); Do we really mean to pass 'i', which is the number of other things we have added, to read_ref_at()? Does that mean git show-branch -g4 --current shows the 4th entry from the current branch while git show-branch -g2 --current shows the second? > + reflog_msg[ref_name_cnt] = fmt_reflog(msg, ts, tz, > + "(%s) (current) %s"); This unconditionally reads reflog and adds to reflog_msg[], without even seeing if we are actually showing reflog entries. Is that sensible? I am wondering if we should have factored out a bit more in the previous step. Instead of "here is what we read from read_ref_at(), format it", I wonder if the interface should be "here is a ref and the offset N; format the Nth entry". And then this part can become something like (I do not know about 'i', but that is meant to be the reflog offset, i.e. "HEAD@{i}"). if (!has_head) { if (reflog) { dwim_ref to learn ref; reflog_msg[ref_name_cnt] = new_helper(ref, i); } else { skip_prefix(name, "refs/heads/", head); append_one_rev(name); } } In any case, I am not sure if it even makes sense to allow the reflog listing mode with "current" in the first place, and a simpler option may be to just forbid the combination. After all, when I adeed "--current" to "git show-branch" in 1aa68d67 (show-branch: --current includes the current branch., 2006-01-11), it was clearly meant to be used with "other branches"---"I would list branches I care about and I use as anchoring points on the command line, and I may or may not be on one of these main branches. Please make sure I can view the current one with respect to these other branches" is what "--current" is about, and mixing it with "how do recent reflog entries relate to each other" does not make much sense.
diff --git a/builtin/show-branch.c b/builtin/show-branch.c index 499ef76a508..50c17fb5c31 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -821,6 +821,19 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) } if (!has_head) { const char *name = head; + struct object_id oid; + char *ref; + char *msg; + timestamp_t ts; + int tz; + + if (!dwim_ref(*av, strlen(*av), &oid, &ref, 0)) + die(_("no such ref %s"), *av); + read_ref_at(get_main_ref_store(the_repository), ref, + flags, 0, i, &oid, &msg, &ts, &tz, NULL); + + reflog_msg[ref_name_cnt] = fmt_reflog(msg, ts, tz, + "(%s) (current) %s"); skip_prefix(name, "refs/heads/", &name); append_one_rev(name); } diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh index 7a1be73ce87..7f6ffcf8a57 100755 --- a/t/t3202-show-branch.sh +++ b/t/t3202-show-branch.sh @@ -161,4 +161,47 @@ test_expect_success 'show branch --reflog=2' ' test_cmp actual expect ' +test_expect_success 'show branch --reflog=2 --current' ' + sed "s/^> //" >expect <<-\EOF && + > ! [refs/heads/branch10@{0}] (4 years, 5 months ago) commit: branch10 + > ! [refs/heads/branch10@{1}] (4 years, 5 months ago) commit: branch10 + > * [branch10] (4 years, 5 months ago) (current) branch: Created from initial + > --- + > + * [refs/heads/branch10@{0}] branch10 + > ++* [refs/heads/branch10@{1}] initial + EOF + git show-branch --reflog=2 --current >actual && + test_cmp actual expect +' + +test_expect_success 'show branch --current' ' + sed "s/^> //" >expect <<-\EOF && + > ! [branch1] branch1 + > ! [branch2] branch2 + > ! [branch3] branch3 + > ! [branch4] branch4 + > ! [branch5] branch5 + > ! [branch6] branch6 + > ! [branch7] branch7 + > ! [branch8] branch8 + > ! [branch9] branch9 + > * [branch10] branch10 + > ! [master] initial + > ----------- + > * [branch10] branch10 + > + [branch9] branch9 + > + [branch8] branch8 + > + [branch7] branch7 + > + [branch6] branch6 + > + [branch5] branch5 + > + [branch4] branch4 + > + [branch3] branch3 + > + [branch2] branch2 + > + [branch1] branch1 + > +++++++++*+ [master] initial + EOF + git show-branch --current >actual && + test_cmp actual expect +' + test_done