Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> From: Gregory David <gregory.david@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Thanks-to: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
> 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.
