mbox series

[v2,0/3] notes.c: introduce "--blank-line" option

Message ID cover.1667828335.git.dyroneteng@gmail.com (mailing list archive)
Headers show
Series notes.c: introduce "--blank-line" option | expand

Message

Teng Long Nov. 7, 2022, 1:57 p.m. UTC 
From: Teng Long <dyroneteng@gmail.com>

Diff from RFC Patch v1:

* optimize the commit-msg and docs of introducing new "--blank-line" option.

* drop unreachable code in "append_edit()". Ævar found that some code has been
unreachable in patch v1. I think it's because, after the commit "notes.c: fixed
tip when target and append note are both empty", for example in this patch, the
situation of "removing an existing note" should be impossible unless a BUG when
trying to do append. The tests are passed, but I'm not sure I fully understand
the original design.

Thanks to Junio C Hamano, Ævar Arnfjörð Bjarmason and Phillip Wood for
the help in v1.

Teng Long (3):
  notes.c: introduce "--blank-line" option
  notes.c: fixed tip when target and append note are both empty
  notes.c: drop unreachable code in "append_edit()"

 Documentation/git-notes.txt | 11 +++++++++--
 builtin/notes.c             | 27 +++++++++++++++++++--------
 t/t3301-notes.sh            | 15 ++++++++++++++-
 3 files changed, 42 insertions(+), 11 deletions(-)

Range-diff against v1:
1:  d69bd0a011 ! 1:  2381947abd notes.c: introduce "--no-blankline" option
    @@ Metadata
     Author: Teng Long <dyroneteng@gmail.com>
     
      ## Commit message ##
    -    notes.c: introduce "--no-blankline" option
    +    notes.c: introduce "--blank-line" option
     
         When appending to a given object which has note and if the appended
         note is not empty too, we will insert a blank line at first. The
         blank line serves as a split line, but sometimes we just want to
    -    omit it then append on the heels of the target note.
    +    omit it then append on the heels of the target note. So, we add
    +    a new "OPT_BOOL()" option to determain whether a new blank line
    +    is insert at first.
     
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
     
    @@ Documentation/git-notes.txt: SYNOPSIS
      'git notes' add [-f] [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
      'git notes' copy [-f] ( --stdin | <from-object> [<to-object>] )
     -'git notes' append [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
    -+'git notes' append [--allow-empty] [--no-blankline] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
    ++'git notes' append [--allow-empty] [--blank-line] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
      'git notes' edit [--allow-empty] [<object>]
      'git notes' show [<object>]
      'git notes' merge [-v | -q] [-s <strategy> ] <notes-ref>
    @@ Documentation/git-notes.txt: OPTIONS
      	Allow an empty note object to be stored. The default behavior is
      	to automatically remove empty notes.
      
    ++--blank-line::
     +--no-blank-line::
    -+	When appending note, do not insert a blank line between
    -+	the note of given object and the note to be appended.
    ++	Controls if a blank line to split paragraphs is inserted
    ++	when appending (the default is true).
     +
      --ref <ref>::
      	Manipulate the notes tree in <ref>.  This overrides
    @@ builtin/notes.c: 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;
    ++	int blankline = 1;
      	const char *object_ref;
      	struct notes_tree *t;
      	struct object_id object, new_note;
    @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char
      			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")),
      		OPT_END()
      	};
      	int edit = !strcmp(argv[0], "edit");
    @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char
      
      		strbuf_grow(&d.buf, size + 1);
     -		if (d.buf.len && prev_buf && size)
    -+		if (!no_blankline && d.buf.len && prev_buf && size)
    ++		if (blankline && d.buf.len && prev_buf && size)
      			strbuf_insertstr(&d.buf, 0, "\n");
      		if (prev_buf && size)
      			strbuf_insert(&d.buf, 0, prev_buf, size);
    @@ t/t3301-notes.sh: test_expect_success 'listing non-existing notes fails' '
      '
      
     +test_expect_success 'append to existing note without a beginning blank line' '
    -+	cat >expect <<-EOF &&
    ++	cat >expect <<-\EOF &&
     +		Initial set of notes
     +		Appended notes
     +	EOF
     +	git notes add -m "Initial set of notes" &&
    -+	git notes append --no-blankline -m "Appended notes" &&
    ++	git notes append --no-blank-line -m "Appended notes" &&
     +	git notes show >actual &&
     +	test_cmp expect actual
     +'
2:  c581cb24b6 ! 2:  5dbe014a09 notes.c: fixed tip when target and append note are both empty
    @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char
     -	char *logmsg;
     +	char *logmsg = NULL;
      	const char * const *usage;
    - 	struct note_data d = { 0, 0, NULL, STRBUF_INIT };
    -+	struct note_data cp = { 0, 0, NULL, STRBUF_INIT };
    +-	struct note_data d = { 0, 0, NULL, STRBUF_INIT };
    ++	struct note_data d = {
    ++		.given = 0,
    ++		.use_editor = 0,
    ++		.edit_path = NULL,
    ++		.buf = STRBUF_INIT
    ++	};
    ++
      	struct option options[] = {
      		OPT_CALLBACK_F('m', "message", &d, N_("message"),
      			N_("note contents as a string"), PARSE_OPT_NONEG,
     @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)
    - 
    - 	prepare_note_data(&object, &d, edit && note ? note : NULL);
    - 
    -+	strbuf_addbuf(&cp.buf, &d.buf);
    -+
    - 	if (note && !edit) {
    - 		/* Append buf to previous note contents */
    - 		unsigned long size;
    -@@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)
      		if (add_note(t, &object, &new_note, combine_notes_overwrite))
      			BUG("combine_notes_overwrite failed");
      		logmsg = xstrfmt("Notes added by 'git notes %s'", argv[0]);
     +		commit_notes(the_repository, t, logmsg);
    -+	} else if (!cp.buf.len) {
    ++	} else if (!d.buf.len && !note) {
     +		fprintf(stderr,
     +			_("Both original and appended notes are empty in %s, do nothing\n"),
     +			oid_to_hex(&object));
    @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char
      
      	free(logmsg);
      	free_note_data(&d);
    -+	free_note_data(&cp);
    - 	free_notes(t);
    - 	return 0;
    - }
     
      ## t/t3301-notes.sh ##
     @@ t/t3301-notes.sh: test_expect_success 'git notes append == add when there is no existing note' '
-:  ---------- > 3:  2475ea0c04 notes.c: drop unreachable code in "append_edit()"

Comments

Ævar Arnfjörð Bjarmason Nov. 7, 2022, 2:57 p.m. UTC | #1 
On Mon, Nov 07 2022, Teng Long wrote:

> From: Teng Long <dyroneteng@gmail.com>
> [...]
> * drop unreachable code in "append_edit()". Ævar found that some code has been
> unreachable in patch v1. I think it's because, after the commit "notes.c: fixed
> tip when target and append note are both empty", for example in this patch, the
> situation of "removing an existing note" should be impossible unless a BUG when
> trying to do append. The tests are passed, but I'm not sure I fully understand
> the original design.

I suggested squashing that BUG() in 3/3 into 2/3, but reading this again
I think it should come first.

I.e. this seems to me like the code in cd067d3bf4e (Builtin-ify
git-notes, 2010-02-13) might have just been blindly carried forward to
both "create" and "edit" in 52694cdabbf (builtin/notes: split
create_note() to clarify add vs. remove logic, 2014-11-12).

But it would be good to have confirmation, e.g. if you check out
52694cdabbf and remove that "Removing note" branch from add() does it
fail tests at the time, but not in the case of append_edit()?
Teng Long Nov. 9, 2022, 7:05 a.m. UTC | #2 
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com> writes:

> > From: Teng Long <dyroneteng@gmail.com>
> > [...]
> > * drop unreachable code in "append_edit()". Ævar found that some code has been
> > unreachable in patch v1. I think it's because, after the commit "notes.c: fixed
> > tip when target and append note are both empty", for example in this patch, the
> > situation of "removing an existing note" should be impossible unless a BUG when
> > trying to do append. The tests are passed, but I'm not sure I fully understand
> > the original design.
>
> I suggested squashing that BUG() in 3/3 into 2/3, but reading this again
> I think it should come first.
>
> I.e. this seems to me like the code in cd067d3bf4e (Builtin-ify
> git-notes, 2010-02-13) might have just been blindly carried forward to
> both "create" and "edit" in 52694cdabbf (builtin/notes: split
> create_note() to clarify add vs. remove logic, 2014-11-12).
>
> But it would be good to have confirmation, e.g. if you check out
> 52694cdabbf and remove that "Removing note" branch from add() does it
> fail tests at the time, but not in the case of append_edit()?

Thanks for mention that. I look back to 52694cdabbf and remove "Removing note"
will make the test fail, because the notes operation "append" is different with
"add", the latter supports to overwrite the existing note then let the
"removing" happen (e.g. execute `git notes add -f -F /dev/null` on an existing
note), but the former will not because it only does "appends" but not doing
"overwrites".

So, I think may just remove the "Removing note" code in append_edit() is OK.

Thanks.
Teng Long Nov. 9, 2022, 7:06 a.m. UTC | #3 
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com> writes:

> > From: Teng Long <dyroneteng@gmail.com>
> > [...]
> > * drop unreachable code in "append_edit()". Ævar found that some code has been
> > unreachable in patch v1. I think it's because, after the commit "notes.c: fixed
> > tip when target and append note are both empty", for example in this patch, the
> > situation of "removing an existing note" should be impossible unless a BUG when
> > trying to do append. The tests are passed, but I'm not sure I fully understand
> > the original design.
>
> I suggested squashing that BUG() in 3/3 into 2/3, but reading this again
> I think it should come first.
>
> I.e. this seems to me like the code in cd067d3bf4e (Builtin-ify
> git-notes, 2010-02-13) might have just been blindly carried forward to
> both "create" and "edit" in 52694cdabbf (builtin/notes: split
> create_note() to clarify add vs. remove logic, 2014-11-12).
>
> But it would be good to have confirmation, e.g. if you check out
> 52694cdabbf and remove that "Removing note" branch from add() does it
> fail tests at the time, but not in the case of append_edit()?

Thanks for mention that. I look back to 52694cdabbf and remove "Removing note"
will make the test fail, because the notes operation "append" is different with
"add", the latter supports to overwrite the existing note then let the
"removing" happen (e.g. execute `git notes add -f -F /dev/null` on an existing
note), but the former will not because it only does "appends" but not doing
"overwrites".

So, I think may just remove the "Removing note" code in append_edit() is OK.

Thanks.