Re: [PATCH 1/3] fixup! refs: RFC: Reftable support for git-core
- From
Jeff King <peff@peff.net>
- Date
- Sep 2, 2021, 09:05 UTC
- Message-ID
- <YTCTyzfIQlme4xee@coredump.intra.peff.net>
- In-Reply-To
- <20210902053023.44006-2-carenas@gmail.com>
On Wed, Sep 01, 2021 at 10:30:21PM -0700, Carlo Marcelo Arenas Belón wrote:
> need to reorder the variables to hopefully make it easier to see why > they might not be used since assert will compile out itself with -DNDEBUG.
This should probably lead with the reason for the patch (avoiding errors with NDEBUG), and then mention any other bits (like "we also reorder for clarity"). That makes the point of the patch easier to see.
Show 14 quoted lines
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 61ee144e19..5d733b0496 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -127,10 +127,11 @@ static void clear_reftable_log_record(struct reftable_log_record *log)
>
> static void fill_reftable_log_record(struct reftable_log_record *log)
> {
> - const char *info = git_committer_info(0);
> struct ident_split split = { NULL };
> - int result = split_ident_line(&split, info, strlen(info));
> int sign = 1;
> + MAYBE_UNUSED const char *info = git_committer_info(0);
> + MAYBE_UNUSED int result = split_ident_line(&split, info, strlen(info));I don't think you need MAYBE_UNUSED on "info"; it is always used, even if the assert isn't compiled.
> assert(0 == result);
IMHO this would be better converted to:
if (result)
BUG("unable to parse output of get_committer_info()");That solves the problem, avoids introducing head-scratching constructs like MAYBE_UNUSED, and gives readers more of a clue about what we expected.
-Peff
PS I do think it's pretty ugly that we have to re-split the ident from git_committer_info(), which just took the individual pieces and stuffed them into a string! But our ident functions are a little cumbersome here (we even have fmt_name(), but I think it's too picky about IDENT_STRICT). I suspect it could be fixed with some refactoring, but that's way out of scope for your series (and for the reftable series itself), so let's leave it for now.