Re: [RFC PATCH 1/2] notes.c: introduce "--no-blankline" option

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 13, 2022, 06:06 UTC
Message-ID: <xmqqsfjsi7eq.fsf@gitster.g>
In-Reply-To: <20221013055654.39628-2-tenglong.tl@alibaba-inc.com>

Teng Long <dyroneteng@gmail.com> writes:

> +--no-blank-line::
> +	When appending note, do not insert a blank line between
> +	the note of given object and the note to be appended.
> +

--blank-line::
--no-blank-line::
	Controls if a blank line to split paragraphs is inserted
        when appending (the default is true).

Show 9 quoted lines

> diff --git a/builtin/notes.c b/builtin/notes.c
> index be51f69225..1ca0476a27 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -562,6 +562,7 @@ static int copy(int argc, const char **argv, const char *prefix)
>  static int append_edit(int argc, const char **argv, const char *prefix)
>  {
>  	int allow_empty = 0;
> +	int no_blankline = 0;

Use

	int blankline = 1;

to avoid double negative, which is confusing and error prone.

Show 6 quoted lines

> @@ -584,6 +585,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  			parse_reuse_arg),
>  		OPT_BOOL(0, "allow-empty", &allow_empty,
>  			N_("allow storing empty note")),
> +		OPT_BOOL(0, "no-blankline", &no_blankline,
> +			N_("do not initially add a blank line")),

	OPT_BOOL(0, "blank-line", &blankline,
		 N_("insert paragraph break before appending to an existing note")),

Show 7 quoted lines

> @@ -619,7 +622,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  		char *prev_buf = read_object_file(note, &type, &size);
>  
>  		strbuf_grow(&d.buf, size + 1);
> -		if (d.buf.len && prev_buf && size)
> +		if (!no_blankline && d.buf.len && prev_buf && size)
>  			strbuf_insertstr(&d.buf, 0, "\n");

Then, the conditional would read more naturally without double negation.

		if (blank_line && d.buf.len && prev_buf && size)

I do not know and I am not judging (yet) if the goal of the patch is sensible (in other words, if we should have such an option), but if we were to do so, I would expect the implementation to look more like what I outlined above.

Thanks.