git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCHv5 6/9] builtin/notes: Split create_note() to clarify add vs. remove logic

From
Johan Herland <johan@herland.net>
Date
Nov 12, 2014, 00:40 UTC
Message-ID
<1415752816-22782-7-git-send-email-johan@herland.net>
In-Reply-To
<1415752816-22782-1-git-send-email-johan@herland.net>

create_note() has a non-trivial interface, and comprises three loosely related parts:

 1. launching the editor with the note contents, if needed
 2. appending to an existing note, if append_only was given
 3. adding or removing the resulting note, based on whether it's non-empty

Split it along those lines to make the logic clearer: The first part goes into a new function - prepare_note_data(), with a simpler interface. The second part is moved into append_edit(), which is the only user of this code. Finally, the add vs. remove decision is moved into the callers (add() and append_edit()), keeping the logic for writing the actual note object in a separate function: write_note_data().

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
 builtin/notes.c | 103 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 54 insertions(+), 49 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index acdedbd..ed32d63 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -159,9 +159,8 @@ static void write_commented_object(int fd, const unsigned char *object)
 		    sha1_to_hex(object));
 }
 
-static void create_note(const unsigned char *object, struct note_data *d,
-			int append_only, const unsigned char *prev,
-			unsigned char *result)
+static void prepare_note_data(const unsigned char *object, struct note_data *d,
+		const unsigned char *old_note)
 {
 	if (d->use_editor || !d->given) {
 		int fd;
@@ -175,8 +174,8 @@ static void create_note(const unsigned char *object, struct note_data *d,
 
 		if (d->given)
 			write_or_die(fd, d->buf.buf, d->buf.len);
-		else if (prev && !append_only)
-			copy_obj_to_fd(fd, prev);
+		else if (old_note)
+			copy_obj_to_fd(fd, old_note);
 
 		strbuf_addch(&buf, '\n');
 		strbuf_add_commented_lines(&buf, note_template, strlen(note_template));
@@ -194,33 +193,16 @@ static void create_note(const unsigned char *object, struct note_data *d,
 		}
 		stripspace(&d->buf, 1);
 	}
+}
 
-	if (prev && append_only) {
-		/* Append buf to previous note contents */
-		unsigned long size;
-		enum object_type type;
-		char *prev_buf = read_sha1_file(prev, &type, &size);
-
-		strbuf_grow(&d->buf, size + 1);
-		if (d->buf.len && prev_buf && size)
-			strbuf_insert(&d->buf, 0, "\n", 1);
-		if (prev_buf && size)
-			strbuf_insert(&d->buf, 0, prev_buf, size);
-		free(prev_buf);
-	}
-
-	if (!d->buf.len) {
-		fprintf(stderr, _("Removing note for object %s\n"),
-			sha1_to_hex(object));
-		hashclr(result);
-	} else {
-		if (write_sha1_file(d->buf.buf, d->buf.len, blob_type, result)) {
-			error(_("unable to write note object"));
-			if (d->edit_path)
-				error(_("The note contents have been left in %s"),
-				      d->edit_path);
-			exit(128);
-		}
+static void write_note_data(struct note_data *d, unsigned char *sha1)
+{
+	if (write_sha1_file(d->buf.buf, d->buf.len, blob_type, sha1)) {
+		error(_("unable to write note object"));
+		if (d->edit_path)
+			error(_("The note contents have been left in %s"),
+				d->edit_path);
+		exit(128);
 	}
 }
 
@@ -403,7 +385,6 @@ static int add(int argc, const char **argv, const char *prefix)
 	const char *object_ref;
 	struct notes_tree *t;
 	unsigned char object[20], new_note[20];
-	char logmsg[100];
 	const unsigned char *note;
 	struct note_data d = { 0, 0, NULL, STRBUF_INIT };
 	struct option options[] = {
@@ -463,17 +444,20 @@ static int add(int argc, const char **argv, const char *prefix)
 			sha1_to_hex(object));
 	}
 
-	create_note(object, &d, 0, note, new_note);
-	free_note_data(&d);
-
-	if (is_null_sha1(new_note))
+	prepare_note_data(object, &d, note);
+	if (d.buf.len) {
+		write_note_data(&d, new_note);
+		if (add_note(t, object, new_note, combine_notes_overwrite))
+			die("BUG: combine_notes_overwrite failed");
+		commit_notes(t, "Notes added by 'git notes add'");
+	} else {
+		fprintf(stderr, _("Removing note for object %s\n"),
+			sha1_to_hex(object));
 		remove_note(t, object);
-	else if (add_note(t, object, new_note, combine_notes_overwrite))
-		die("BUG: combine_notes_overwrite failed");
+		commit_notes(t, "Notes removed by 'git notes add'");
+	}
 
-	snprintf(logmsg, sizeof(logmsg), "Notes %s by 'git notes %s'",
-		 is_null_sha1(new_note) ? "removed" : "added", "add");
-	commit_notes(t, logmsg);
+	free_note_data(&d);
 	free_notes(t);
 	return 0;
 }
@@ -602,17 +586,38 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 	t = init_notes_check(argv[0]);
 	note = get_note(t, object);
 
-	create_note(object, &d, !edit, note, new_note);
-	free_note_data(&d);
+	prepare_note_data(object, &d, edit ? note : NULL);
 
-	if (is_null_sha1(new_note))
-		remove_note(t, object);
-	else if (add_note(t, object, new_note, combine_notes_overwrite))
-		die("BUG: combine_notes_overwrite failed");
+	if (note && !edit) {
+		/* Append buf to previous note contents */
+		unsigned long size;
+		enum object_type type;
+		char *prev_buf = read_sha1_file(note, &type, &size);
+
+		strbuf_grow(&d.buf, size + 1);
+		if (d.buf.len && prev_buf && size)
+			strbuf_insert(&d.buf, 0, "\n", 1);
+		if (prev_buf && size)
+			strbuf_insert(&d.buf, 0, prev_buf, size);
+		free(prev_buf);
+	}
 
-	snprintf(logmsg, sizeof(logmsg), "Notes %s by 'git notes %s'",
-		 is_null_sha1(new_note) ? "removed" : "added", argv[0]);
+	if (d.buf.len) {
+		write_note_data(&d, new_note);
+		if (add_note(t, object, new_note, combine_notes_overwrite))
+			die("BUG: combine_notes_overwrite failed");
+		snprintf(logmsg, sizeof(logmsg), "Notes added by 'git notes %s'",
+			argv[0]);
+	} else {
+		fprintf(stderr, _("Removing note for object %s\n"),
+			sha1_to_hex(object));
+		remove_note(t, object);
+		snprintf(logmsg, sizeof(logmsg), "Notes removed by 'git notes %s'",
+			argv[0]);
+	}
 	commit_notes(t, logmsg);
+
+	free_note_data(&d);
 	free_notes(t);
 	return 0;
 }

-- 
2.0.0.rc4.501.gdaf83ca
Previous: Johan HerlandNext: Johan Herland
Message 7 of 14 in “[PATCHv5 0/9] Handling empty notes”
  1. Johan HerlandNov 12, 2014
  2. 1/9 builtin/notes: Fix premature failure when trying to add the empty blobJohan Herland, Nov 12, 2014
  3. 2/9 t3301: Verify that 'git notes' removes empty notes by defaultJohan Herland, Nov 12, 2014
  4. 3/9 builtin/notes: Improve namingJohan Herland, Nov 12, 2014
  5. 4/9 builtin/notes: Refactor note file path into struct note_dataJohan Herland, Nov 12, 2014
  6. 5/9 builtin/notes: Simplify early exit code in add()Johan Herland, Nov 12, 2014
  7. 6/9 builtin/notes: Split create_note() to clarify add vs. remove logicJohan Herland, Nov 12, 2014
  8. 7/9 builtin/notes: Add --allow-empty, to allow storing empty notesJohan Herland, Nov 12, 2014
  9. 8/9 notes: Empty notes should be shown by 'git log'Johan Herland, Nov 12, 2014
  10. 9/9 t3301: ModernizeJohan Herland, Nov 12, 2014
  11. Eric SunshineNov 12, 2014
  12. Johan HerlandNov 12, 2014
  13. Junio C HamanoNov 12, 2014
  14. Johan HerlandNov 12, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.