Hi,
On Wed, 18 Feb 2009, Jeff King wrote:
> On Wed, Feb 18, 2009 at 07:14:59PM +0100, Johannes Schindelin wrote:
>
> > A recent addition to the ref_item struct was not taken care of, leading
> > to a segmentation fault when accessing the (uninitialized) "dest" member.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> > ---
> >
> > Unfortunately not found by valgrind.
>
> Meaning that the bug was created after your valgrind testing (which
> takes a painfully long time to run, and so only happens occasionally),
> and therefore you found it by hand? Or meaning that even running the
> test suite with valgrind did not reveal the problem?
It bit me.
IOW I had to fix it before I could finish up the work for the day.
> If the latter, isn't that an indication that this code path was not
> being exercised by the test suite and it should be?
Like I said, I had to finish up some work for the day, that's why I did
not have time to add a test.
> Now if only we had a way of measuring test coverage...
Yes, I also want the gcov series. Patience, grass hopper, patience: after
1.6.2.
> > --- a/builtin-branch.c
> > +++ b/builtin-branch.c
> > @@ -441,7 +441,9 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str
> > is_descendant_of(head_commit, with_commit)) {
> > struct ref_item item;
> > item.name = xstrdup("(no branch)");
> > + item.len = strlen(item.name);
> > item.kind = REF_LOCAL_BRANCH;
> > + item.dest = NULL;
> > item.commit = head_commit;
> > if (strlen(item.name) > ref_list.maxwidth)
> > ref_list.maxwidth = strlen(item.name);
>
> Maybe replace the repeated strlens below with item.len? I.e., squash in
>
> diff --git a/builtin-branch.c b/builtin-branch.c
> index 13e4de8..14d4b91 100644
> --- a/builtin-branch.c
> +++ b/builtin-branch.c
> @@ -443,8 +443,8 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str
> item.kind = REF_LOCAL_BRANCH;
> item.dest = NULL;
> item.commit = head_commit;
> - if (strlen(item.name) > ref_list.maxwidth)
> - ref_list.maxwidth = strlen(item.name);
> + if (item.len > ref_list.maxwidth)
> + ref_list.maxwidth = item.len;
Yeah, I did not think of that. I checked that there are no other
instances where a member of ref_item was uninitialized, and that took
already more time than I had.
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
