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

Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Nov 18, 2011, 01:27 UTC
Message-ID
<20111118012746.GA22485@elie.hsd1.il.comcast.net>
In-Reply-To
<CACsJy8A25SyLVKv8GwkYaHBJwU5tHqgdJK6L-upF9HWseFzCtQ@mail.gmail.com>
Nguyen Thai Ngoc Duy wrote:
> Anyway to detect if __VA_ARGS__ is supported at compile time? I guess
> #ifdef __GNUC__ is the last resort.

Why would one want to do that? Either your feature requires C99 (so the build can error out if __VA_ARGS__ support is missing) or it doesn't.

> notes-merge.c introduces __VA_ARGS__ since v1.7.4 so we may want to do
> something there too.

Good catch. How about this patch? It seems I forgot to send it out last May and it has been rotting in my local tree ever since.

-- >8 --
Subject: notes merge: eliminate OUTPUT macro

The macro is variadic, which breaks support for pre-C99 compilers, and it hides an "if", which can make code hard to understand on first reading if some arguments have side-effects.

The OUTPUT macro seems to have been inspired by the "output" function from merge-recursive. But that function in merge-recursive exists to indent output based on the level of recursion and there is no similar justification for such a function in "notes merge".

Noticed with 'make CC="gcc -std=c89 -pedantic"':
 notes-merge.c:24:22: warning: anonymous variadic macros were introduced in C99 [-Wvariadic-macros]
Encouraged-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 notes-merge.c |  104 +++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 61 insertions(+), 43 deletions(-)
diff --git a/notes-merge.c b/notes-merge.c
index baaf31f4..d0e5034d 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -21,14 +21,6 @@ void init_notes_merge_options(struct notes_merge_options *o)
 	o->verbosity = NOTES_MERGE_VERBOSITY_DEFAULT;
 }
 
-#define OUTPUT(o, v, ...) \
-	do { \
-		if ((o)->verbosity >= (v)) { \
-			printf(__VA_ARGS__); \
-			puts(""); \
-		} \
-	} while (0)
-
 static int path_to_sha1(const char *path, unsigned char *sha1)
 {
 	char hex_sha1[40];
@@ -392,21 +384,26 @@ static int merge_one_change_manual(struct notes_merge_options *o,
 
 	strbuf_addf(&(o->commit_msg), "\t%s\n", sha1_to_hex(p->obj));
 
-	OUTPUT(o, 2, "Auto-merging notes for %s", sha1_to_hex(p->obj));
+	if (o->verbosity >= 2)
+		printf("Auto-merging notes for %s\n", sha1_to_hex(p->obj));
 	check_notes_merge_worktree(o);
 	if (is_null_sha1(p->local)) {
 		/* D/F conflict, checkout p->remote */
 		assert(!is_null_sha1(p->remote));
-		OUTPUT(o, 1, "CONFLICT (delete/modify): Notes for object %s "
-		       "deleted in %s and modified in %s. Version from %s "
-		       "left in tree.", sha1_to_hex(p->obj), lref, rref, rref);
+		if (o->verbosity >= 1)
+			printf("CONFLICT (delete/modify): Notes for object %s "
+				"deleted in %s and modified in %s. Version from %s "
+				"left in tree.\n",
+				sha1_to_hex(p->obj), lref, rref, rref);
 		write_note_to_worktree(p->obj, p->remote);
 	} else if (is_null_sha1(p->remote)) {
 		/* D/F conflict, checkout p->local */
 		assert(!is_null_sha1(p->local));
-		OUTPUT(o, 1, "CONFLICT (delete/modify): Notes for object %s "
-		       "deleted in %s and modified in %s. Version from %s "
-		       "left in tree.", sha1_to_hex(p->obj), rref, lref, lref);
+		if (o->verbosity >= 1)
+			printf("CONFLICT (delete/modify): Notes for object %s "
+				"deleted in %s and modified in %s. Version from %s "
+				"left in tree.\n",
+				sha1_to_hex(p->obj), rref, lref, lref);
 		write_note_to_worktree(p->obj, p->local);
 	} else {
 		/* "regular" conflict, checkout result of ll_merge() */
@@ -415,8 +412,9 @@ static int merge_one_change_manual(struct notes_merge_options *o,
 			reason = "add/add";
 		assert(!is_null_sha1(p->local));
 		assert(!is_null_sha1(p->remote));
-		OUTPUT(o, 1, "CONFLICT (%s): Merge conflict in notes for "
-		       "object %s", reason, sha1_to_hex(p->obj));
+		if (o->verbosity >= 1)
+			printf("CONFLICT (%s): Merge conflict in notes for "
+				"object %s\n", reason, sha1_to_hex(p->obj));
 		ll_merge_in_worktree(o, p);
 	}
 
@@ -438,24 +436,30 @@ static int merge_one_change(struct notes_merge_options *o,
 	case NOTES_MERGE_RESOLVE_MANUAL:
 		return merge_one_change_manual(o, p, t);
 	case NOTES_MERGE_RESOLVE_OURS:
-		OUTPUT(o, 2, "Using local notes for %s", sha1_to_hex(p->obj));
+		if (o->verbosity >= 2)
+			printf("Using local notes for %s\n",
+						sha1_to_hex(p->obj));
 		/* nothing to do */
 		return 0;
 	case NOTES_MERGE_RESOLVE_THEIRS:
-		OUTPUT(o, 2, "Using remote notes for %s", sha1_to_hex(p->obj));
+		if (o->verbosity >= 2)
+			printf("Using remote notes for %s\n",
+						sha1_to_hex(p->obj));
 		if (add_note(t, p->obj, p->remote, combine_notes_overwrite))
 			die("BUG: combine_notes_overwrite failed");
 		return 0;
 	case NOTES_MERGE_RESOLVE_UNION:
-		OUTPUT(o, 2, "Concatenating local and remote notes for %s",
-		       sha1_to_hex(p->obj));
+		if (o->verbosity >= 2)
+			printf("Concatenating local and remote notes for %s\n",
+							sha1_to_hex(p->obj));
 		if (add_note(t, p->obj, p->remote, combine_notes_concatenate))
 			die("failed to concatenate notes "
 			    "(combine_notes_concatenate)");
 		return 0;
 	case NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ:
-		OUTPUT(o, 2, "Concatenating unique lines in local and remote "
-		       "notes for %s", sha1_to_hex(p->obj));
+		if (o->verbosity >= 2)
+			printf("Concatenating unique lines in local and remote "
+				"notes for %s\n", sha1_to_hex(p->obj));
 		if (add_note(t, p->obj, p->remote, combine_notes_cat_sort_uniq))
 			die("failed to concatenate notes "
 			    "(combine_notes_cat_sort_uniq)");
@@ -518,8 +522,9 @@ static int merge_from_diffs(struct notes_merge_options *o,
 	conflicts = merge_changes(o, changes, &num_changes, t);
 	free(changes);
 
-	OUTPUT(o, 4, "Merge result: %i unmerged notes and a %s notes tree",
-	       conflicts, t->dirty ? "dirty" : "clean");
+	if (o->verbosity >= 4)
+		printf("Merge result: %i unmerged notes and a %s notes tree\n",
+			conflicts, t->dirty ? "dirty" : "clean");
 
 	return conflicts ? -1 : 1;
 }
@@ -616,33 +621,40 @@ int notes_merge(struct notes_merge_options *o,
 	if (!bases) {
 		base_sha1 = null_sha1;
 		base_tree_sha1 = EMPTY_TREE_SHA1_BIN;
-		OUTPUT(o, 4, "No merge base found; doing history-less merge");
+		if (o->verbosity >= 4)
+			printf("No merge base found; doing history-less merge\n");
 	} else if (!bases->next) {
 		base_sha1 = bases->item->object.sha1;
 		base_tree_sha1 = bases->item->tree->object.sha1;
-		OUTPUT(o, 4, "One merge base found (%.7s)",
-		       sha1_to_hex(base_sha1));
+		if (o->verbosity >= 4)
+			printf("One merge base found (%.7s)\n",
+				sha1_to_hex(base_sha1));
 	} else {
 		/* TODO: How to handle multiple merge-bases? */
 		base_sha1 = bases->item->object.sha1;
 		base_tree_sha1 = bases->item->tree->object.sha1;
-		OUTPUT(o, 3, "Multiple merge bases found. Using the first "
-		       "(%.7s)", sha1_to_hex(base_sha1));
+		if (o->verbosity >= 3)
+			printf("Multiple merge bases found. Using the first "
+				"(%.7s)\n", sha1_to_hex(base_sha1));
 	}
 
-	OUTPUT(o, 4, "Merging remote commit %.7s into local commit %.7s with "
-	       "merge-base %.7s", sha1_to_hex(remote->object.sha1),
-	       sha1_to_hex(local->object.sha1), sha1_to_hex(base_sha1));
+	if (o->verbosity >= 4)
+		printf("Merging remote commit %.7s into local commit %.7s with "
+			"merge-base %.7s\n", sha1_to_hex(remote->object.sha1),
+			sha1_to_hex(local->object.sha1),
+			sha1_to_hex(base_sha1));
 
 	if (!hashcmp(remote->object.sha1, base_sha1)) {
 		/* Already merged; result == local commit */
-		OUTPUT(o, 2, "Already up-to-date!");
+		if (o->verbosity >= 2)
+			printf("Already up-to-date!\n");
 		hashcpy(result_sha1, local->object.sha1);
 		goto found_result;
 	}
 	if (!hashcmp(local->object.sha1, base_sha1)) {
 		/* Fast-forward; result == remote commit */
-		OUTPUT(o, 2, "Fast-forward");
+		if (o->verbosity >= 2)
+			printf("Fast-forward\n");
 		hashcpy(result_sha1, remote->object.sha1);
 		goto found_result;
 	}
@@ -684,8 +696,9 @@ int notes_merge_commit(struct notes_merge_options *o,
 	int path_len = strlen(path), i;
 	const char *msg = strstr(partial_commit->buffer, "\n\n");
 
-	OUTPUT(o, 3, "Committing notes in notes merge worktree at %.*s",
-	       path_len - 1, path);
+	if (o->verbosity >= 3)
+		printf("Committing notes in notes merge worktree at %.*s\n",
+			path_len - 1, path);
 
 	if (!msg || msg[2] == '\0')
 		die("partial notes commit has empty message");
@@ -700,7 +713,9 @@ int notes_merge_commit(struct notes_merge_options *o,
 		unsigned char obj_sha1[20], blob_sha1[20];
 
 		if (ent->len - path_len != 40 || get_sha1_hex(relpath, obj_sha1)) {
-			OUTPUT(o, 3, "Skipping non-SHA1 entry '%s'", ent->name);
+			if (o->verbosity >= 3)
+				printf("Skipping non-SHA1 entry '%s'\n",
+								ent->name);
 			continue;
 		}
 
@@ -712,14 +727,16 @@ int notes_merge_commit(struct notes_merge_options *o,
 		if (add_note(partial_tree, obj_sha1, blob_sha1, NULL))
 			die("Failed to add resolved note '%s' to notes tree",
 			    ent->name);
-		OUTPUT(o, 4, "Added resolved note for object %s: %s",
-		       sha1_to_hex(obj_sha1), sha1_to_hex(blob_sha1));
+		if (o->verbosity >= 4)
+			printf("Added resolved note for object %s: %s\n",
+				sha1_to_hex(obj_sha1), sha1_to_hex(blob_sha1));
 	}
 
 	create_notes_commit(partial_tree, partial_commit->parents, msg,
 			    result_sha1);
-	OUTPUT(o, 4, "Finalized notes merge commit: %s",
-	       sha1_to_hex(result_sha1));
+	if (o->verbosity >= 4)
+		printf("Finalized notes merge commit: %s\n",
+			sha1_to_hex(result_sha1));
 	free(path);
 	return 0;
 }
@@ -731,7 +748,8 @@ int notes_merge_abort(struct notes_merge_options *o)
 	int ret;
 
 	strbuf_addstr(&buf, git_path(NOTES_MERGE_WORKTREE));
-	OUTPUT(o, 3, "Removing notes merge worktree at %s", buf.buf);
+	if (o->verbosity >= 3)
+		printf("Removing notes merge worktree at %s\n", buf.buf);
 	ret = remove_dir_recursively(&buf, 0);
 	strbuf_release(&buf);
 	return ret;

-- 
1.7.8.rc2
Previous: Jeff KingNext: Johan Herland
Message 21 of 27 in “nd/resolve-ref v2”
  1. 0/8 nd/resolve-ref v2Nguyễn Thái Ngọc Duy, Nov 17, 2011
  2. 1/8 Convert many resolve_ref() calls to read_ref*() and ref_exists()Nguyễn Thái Ngọc Duy, Nov 17, 2011
  3. 2/8 Rename resolve_ref() to resolve_ref_unsafe()Nguyễn Thái Ngọc Duy, Nov 17, 2011
  4. 3/8 Re-add resolve_ref() that always returns an allocated bufferNguyễn Thái Ngọc Duy, Nov 17, 2011
  5. 4/8 cmd_merge: convert to single exit pointNguyễn Thái Ngọc Duy, Nov 17, 2011
  6. Ramkumar RamachandraNov 17, 2011
  7. Jonathan NiederNov 17, 2011
  8. 5/8 Use resolve_ref() instead of resolve_ref_unsafe()Nguyễn Thái Ngọc Duy, Nov 17, 2011
  9. 6/8 Convert resolve_ref_unsafe+xstrdup to resolve_refNguyễn Thái Ngọc Duy, Nov 17, 2011
  10. Ramkumar RamachandraNov 17, 2011
  11. Nguyen Thai Ngoc DuyNov 18, 2011
  12. 7/8 Guard memory overwriting in resolve_ref_unsafe's static bufferNguyễn Thái Ngọc Duy, Nov 17, 2011
  13. 8/8 Enable GIT_DEBUG_MEMCHECK on git_pathname()Nguyễn Thái Ngọc Duy, Nov 17, 2011
  14. Ramkumar RamachandraNov 17, 2011
  15. Jeff KingNov 17, 2011
  16. Nguyen Thai Ngoc DuyNov 18, 2011
  17. Jeff KingNov 18, 2011
  18. Jonathan NiederNov 18, 2011
  19. Nguyen Thai Ngoc DuyNov 18, 2011
  20. Jeff KingNov 18, 2011
  21. Jonathan NiederNov 18, 2011
  22. Johan HerlandNov 18, 2011
  23. Jonathan NiederNov 18, 2011
  24. Junio C HamanoNov 18, 2011
  25. Bernhard R. LinkNov 18, 2011
  26. Jonathan NiederNov 17, 2011
  27. Nguyen Thai Ngoc DuyDec 6, 2011

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.