Re: [PATCH v2 4/5] bitmap: add trace2 outputs during open "bitmap" file

From: Teng Long <dyroneteng@gmail.com>
Date: May 6, 2022, 11:27 UTC
Message-ID: <20220506112722.33760-1-dyroneteng@gmail.com>
In-Reply-To: <220421.86k0bi77mb.gmgdl@evledraar.gmail.com>

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.

Show 7 quoted lines

> 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.

Show 12 quoted lines

> 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.

Show 8 quoted lines

> > +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.