On Thu, 21 Apr 2022 17:51:30 +0200, Ævar Arnfjörð Bjarmason wrote:
> It's really helpful to have these full examples in the commit
> message. Thanks.
>From the previous contribution process, I think it is necessary
to provide sufficient information in patch, which can help to save
reviewer's time and find problems faster.
> Hrm, so re my comment on 5/5 are you trying to use the "try to open" as
> a timer to see what our start time is?
>
> I think probably not, in which case this whole variable flip-around is
> something we won't need.
>
> But if we do actually need it perhaps a sub-region for the timing?
Yes, I looked back on it and found it's redundant and unnecessary now.
Will delete the two related references in the code in next patch.
> Nit: I think these "goto" patterns are best if your "int ret = N" picks
> an "N" which is the default that you'll "goto", i.e. if you pick "ret =
> 0" you'll just need "goto done" here....
>
> > + goto done;
> > + }
> > + ret = open_pack_bitmap(r, bitmap_git);
>
> ...which we may then override here.
>
> Just saves you the assignment and the {}, but it tends to add up in
> longer functions
Make sense.
> > +done:
> > + trace2_region_leave("pack-bitmap", "open_bitmap", r);
> > + return ret;
> > }
>
> Looks good, aside from the 5/5 comments that much of the data string
> logging looks like it becomes redundant in the end due to error() giving
> us the same thing.
Oops, I will read the comments and reply them in 5/5.
> I haven't tested but part of this test change looks like it'll break
> under bisect, i.e. you changed 1/2 of these strings in 3/5. Did you try
> with "git rebase -i -x 'make test T=t*bitmap*" or similar?
Yes, they should be in the same commit in 3/5, but now I the new warning
text seems like is much verbose which suggested by Taylor Blau, and I will
roll back the new warning message and fix the broken test under bisect.
Thanks.
