Re: [PATCH 1/3] fixup! refs: RFC: Reftable support for git-core

[Jeff King <peff@peff.net>]

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.

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