threads / patch / 50502

patch, 10 partsdropping more unused function parameters

Subject: [PATCH 0/10] dropping more unused function parameters

## tl;dr

11 messages between Feb 14, 2019 and Feb 14, 2019. Diffs are folded; open one to read it.

replies: 10people: 1as markdown or json

Jeff King· Feb 14, 2019, 05:47 UTC · lore

Here are ten more patches from my exploration of -Wunused-parameters. Most of these are quite old and not urgent, but things seem relatively quiet now, and they all merge cleanly with "pu".

In a sense they should all be trivial to review, as they only remove unused parameters. But the thing to consider is whether that parameter's lack of use is actually a bug, or if it's simply cruft (and I argue in each case in favor of cruft).

  [01/10]: diff: drop options parameter from diffcore_fix_diff_index()
  [02/10]: diff: drop unused color reset parameters
  [03/10]: diff: drop unused emit data parameter from sane_truncate_line()
  [04/10]: diff: drop complete_rewrite parameter from run_external_diff()
  [05/10]: merge-recursive: drop several unused parameters
  [06/10]: pack-objects: drop unused parameter from oe_map_new_pack()
  [07/10]: files-backend: drop refs parameter from split_symref_update()
  [08/10]: ref-filter: drop unused buf/sz pairs
  [09/10]: ref-filter: drop unused "obj" parameters
  [10/10]: ref-filter: drop unused "sz" parameters
 diff-lib.c           |  2 +-
 diff.c               | 31 ++++++++++++-------------------
 diff.h               |  2 +-
 merge-recursive.c    | 19 +++++++------------
 pack-objects.c       |  3 +--
 pack-objects.h       |  6 +++---
 ref-filter.c         | 36 ++++++++++++++++++------------------
 refs/files-backend.c |  5 ++---
 8 files changed, 45 insertions(+), 59 deletions(-)
-Peff
Jeff King· Feb 14, 2019, 05:48 UTC · re: Jeff King · lore

[PATCH 01/10] diff: drop options parameter from diffcore_fix_diff_index()

The sole purpose of this function is to fix the sorting order of the queued diff entries. It doesn't need to know about any diff options, so we can drop the unused parameter.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff-lib.c | 2 +-
 diff.c     | 2 +-
 diff.h     | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
Show changes to 3 files +3 −3

diff-lib.c, diff.c, diff.h

diff --git a/diff-lib.c b/diff-lib.c
index 23c8d351b3..a838c219ec 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -531,7 +531,7 @@ int run_diff_index(struct rev_info *revs, int cached)
 		exit(128);
 
 	diff_set_mnemonic_prefix(&revs->diffopt, "c/", cached ? "i/" : "w/");
-	diffcore_fix_diff_index(&revs->diffopt);
+	diffcore_fix_diff_index();
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
 	trace_performance_leave("diff-index");
diff --git a/diff.c b/diff.c
index 5306c48652..3e7d11fb70 100644
--- a/diff.c
+++ b/diff.c
@@ -6224,7 +6224,7 @@ static int diffnamecmp(const void *a_, const void *b_)
 	return strcmp(name_a, name_b);
 }
 
-void diffcore_fix_diff_index(struct diff_options *options)
+void diffcore_fix_diff_index(void)
 {
 	struct diff_queue_struct *q = &diff_queued_diff;
 	QSORT(q->queue, q->nr, diffnamecmp);
diff --git a/diff.h b/diff.h
index b512d0477a..8f55aad5ba 100644
--- a/diff.h
+++ b/diff.h
@@ -367,7 +367,7 @@ int git_config_rename(const char *var, const char *value);
 #define DIFF_PICKAXE_IGNORE_CASE	32
 
 void diffcore_std(struct diff_options *);
-void diffcore_fix_diff_index(struct diff_options *);
+void diffcore_fix_diff_index(void);
 
 #define COMMON_DIFF_OPTIONS_HELP \
 "\ncommon diff options:\n" \
-- 
2.21.0.rc1.567.g648f076c3f
Jeff King· Feb 14, 2019, 05:48 UTC · re: Jeff King · lore

[PATCH 02/10] diff: drop unused color reset parameters

Several of the emit_* functions take a "reset" color parameter, but never actually look at it (instead, they call into emit_diff_symbol, which handles the colors itself). Let's drop these unused parameters.

Note that emit_line() does still take a color/reset pair, and actually uses it. It cannot be refactored to match these other functions because it's the thing that emit_diff_symbol eventually calls into (i.e., it does not by itself know which colors to use, and must be told by the caller).

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)
Show changes to diff.c +8 −13
diff --git a/diff.c b/diff.c
index 3e7d11fb70..de45bbd76e 100644
--- a/diff.c
+++ b/diff.c
@@ -1613,8 +1613,7 @@ static int new_blank_line_at_eof(struct emit_callback *ecbdata, const char *line
 	return ws_blank_line(line, len, ecbdata->ws_rule);
 }
 
-static void emit_add_line(const char *reset,
-			  struct emit_callback *ecbdata,
+static void emit_add_line(struct emit_callback *ecbdata,
 			  const char *line, int len)
 {
 	unsigned flags = WSEH_NEW | ecbdata->ws_rule;
@@ -1624,16 +1623,14 @@ static void emit_add_line(const char *reset,
 	emit_diff_symbol(ecbdata->opt, DIFF_SYMBOL_PLUS, line, len, flags);
 }
 
-static void emit_del_line(const char *reset,
-			  struct emit_callback *ecbdata,
+static void emit_del_line(struct emit_callback *ecbdata,
 			  const char *line, int len)
 {
 	unsigned flags = WSEH_OLD | ecbdata->ws_rule;
 	emit_diff_symbol(ecbdata->opt, DIFF_SYMBOL_MINUS, line, len, flags);
 }
 
-static void emit_context_line(const char *reset,
-			      struct emit_callback *ecbdata,
+static void emit_context_line(struct emit_callback *ecbdata,
 			      const char *line, int len)
 {
 	unsigned flags = WSEH_CONTEXT | ecbdata->ws_rule;
@@ -1742,7 +1739,6 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
 			       int prefix, const char *data, int size)
 {
 	const char *endp = NULL;
-	const char *reset = diff_get_color(ecb->color_diff, DIFF_RESET);
 
 	while (0 < size) {
 		int len;
@@ -1751,10 +1747,10 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
 		len = endp ? (endp - data + 1) : size;
 		if (prefix != '+') {
 			ecb->lno_in_preimage++;
-			emit_del_line(reset, ecb, data, len);
+			emit_del_line(ecb, data, len);
 		} else {
 			ecb->lno_in_postimage++;
-			emit_add_line(reset, ecb, data, len);
+			emit_add_line(ecb, data, len);
 		}
 		size -= len;
 		data += len;
@@ -2325,7 +2321,6 @@ static void find_lno(const char *line, struct emit_callback *ecbdata)
 static void fn_out_consume(void *priv, char *line, unsigned long len)
 {
 	struct emit_callback *ecbdata = priv;
-	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
 	struct diff_options *o = ecbdata->opt;
 
 	o->found_changes = 1;
@@ -2392,16 +2387,16 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 	switch (line[0]) {
 	case '+':
 		ecbdata->lno_in_postimage++;
-		emit_add_line(reset, ecbdata, line + 1, len - 1);
+		emit_add_line(ecbdata, line + 1, len - 1);
 		break;
 	case '-':
 		ecbdata->lno_in_preimage++;
-		emit_del_line(reset, ecbdata, line + 1, len - 1);
+		emit_del_line(ecbdata, line + 1, len - 1);
 		break;
 	case ' ':
 		ecbdata->lno_in_postimage++;
 		ecbdata->lno_in_preimage++;
-		emit_context_line(reset, ecbdata, line + 1, len - 1);
+		emit_context_line(ecbdata, line + 1, len - 1);
 		break;
 	default:
 		/* incomplete line at the end */
-- 
2.21.0.rc1.567.g648f076c3f
Jeff King· Feb 14, 2019, 05:48 UTC · re: Jeff King · lore

[PATCH 03/10] diff: drop unused emit data parameter from sane_truncate_line()

We pass the "struct emit_callback" (which contains all of the context for our diff) into sane_truncate_line(), but that function doesn't actually use it. In theory we might eventually develop a diff option that impacts this, but in the meantime let's not mislead anybody reading the code. Since the function is static, it would be easy to pass it again if it should ever become useful.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to diff.c +2 −2
diff --git a/diff.c b/diff.c
index de45bbd76e..d24843b531 100644
--- a/diff.c
+++ b/diff.c
@@ -2287,7 +2287,7 @@ const char *diff_line_prefix(struct diff_options *opt)
 	return msgbuf->buf;
 }
 
-static unsigned long sane_truncate_line(struct emit_callback *ecb, char *line, unsigned long len)
+static unsigned long sane_truncate_line(char *line, unsigned long len)
 {
 	const char *cp;
 	unsigned long allot;
@@ -2351,7 +2351,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 	if (line[0] == '@') {
 		if (ecbdata->diff_words)
 			diff_words_flush(ecbdata);
-		len = sane_truncate_line(ecbdata, line, len);
+		len = sane_truncate_line(line, len);
 		find_lno(line, ecbdata);
 		emit_hunk_header(ecbdata, line, len);
 		return;
-- 
2.21.0.rc1.567.g648f076c3f
Jeff King· Feb 14, 2019, 05:49 UTC · re: Jeff King · lore

[PATCH 04/10] diff: drop complete_rewrite parameter from run_external_diff()

Our builtin_diff() wants to know whether break-detection found a complete rewrite, because it changes how the diff is shown. However, when calling out to an external diff, we don't pass this information along (and doing so would require designing a new interface to the user-provided program).

Let's drop the unused parameter to make this fact more clear.
Signed-off-by: Jeff King <peff@peff.net>
---
An alternative is to pass $COMPLETE_REWRITE in the environment. That
would avoid disrupting existing external diff callers, and they could
peek at the information if they really cared. But given that nobody has
actually asked for this, I'm inclined not to add a new interface element
that we'd then have to support forever.
 diff.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
Show changes to diff.c +1 −3
diff --git a/diff.c b/diff.c
index d24843b531..e3f263769b 100644
--- a/diff.c
+++ b/diff.c
@@ -4178,7 +4178,6 @@ static void run_external_diff(const char *pgm,
 			      struct diff_filespec *one,
 			      struct diff_filespec *two,
 			      const char *xfrm_msg,
-			      int complete_rewrite,
 			      struct diff_options *o)
 {
 	struct argv_array argv = ARGV_ARRAY_INIT;
@@ -4336,8 +4335,7 @@ static void run_diff_cmd(const char *pgm,
 	}
 
 	if (pgm) {
-		run_external_diff(pgm, name, other, one, two, xfrm_msg,
-				  complete_rewrite, o);
+		run_external_diff(pgm, name, other, one, two, xfrm_msg, o);
 		return;
 	}
 	if (one && two)
-- 
2.21.0.rc1.567.g648f076c3f
Jeff King· Feb 14, 2019, 05:50 UTC · re: Jeff King · lore

[PATCH 05/10] merge-recursive: drop several unused parameters

There are a few functions related to directory renames that have unused parameters. After consulting with the author in [1], these seem to be leftover cruft from the development process, and not signs of any bug. Let's drop them.

[1] https://public-inbox.org/git/CABPp-BHobf8wbBsXF97scNQCzkxQukziODRXq6JOOWq61cAd9g@mail.gmail.com/
Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 merge-recursive.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)
Show changes to merge-recursive.c +7 −12
diff --git a/merge-recursive.c b/merge-recursive.c
index 4851825aeb..f270fa66f3 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1402,8 +1402,7 @@ static int merge_mode_and_contents(struct merge_options *o,
 
 static int handle_rename_via_dir(struct merge_options *o,
 				 struct diff_filepair *pair,
-				 const char *rename_branch,
-				 const char *other_branch)
+				 const char *rename_branch)
 {
 	/*
 	 * Handle file adds that need to be renamed due to directory rename
@@ -2213,8 +2212,7 @@ static void handle_directory_level_conflicts(struct merge_options *o,
 	remove_hashmap_entries(dir_re_merge, &remove_from_merge);
 }
 
-static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs,
-					     struct tree *tree)
+static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs)
 {
 	struct hashmap *dir_renames;
 	struct hashmap_iter iter;
@@ -2460,8 +2458,7 @@ static void apply_directory_rename_modifications(struct merge_options *o,
 						 struct tree *o_tree,
 						 struct tree *a_tree,
 						 struct tree *b_tree,
-						 struct string_list *entries,
-						 int *clean)
+						 struct string_list *entries)
 {
 	struct string_list_item *item;
 	int stage = (tree == a_tree ? 2 : 3);
@@ -2632,8 +2629,7 @@ static struct string_list *get_renames(struct merge_options *o,
 			apply_directory_rename_modifications(o, pair, new_path,
 							     re, tree, o_tree,
 							     a_tree, b_tree,
-							     entries,
-							     clean_merge);
+							     entries);
 	}
 
 	hashmap_iter_init(&collisions, &iter);
@@ -2944,8 +2940,8 @@ static int detect_and_process_renames(struct merge_options *o,
 	merge_pairs = get_diffpairs(o, common, merge);
 
 	if (o->detect_directory_renames) {
-		dir_re_head = get_directory_renames(head_pairs, head);
-		dir_re_merge = get_directory_renames(merge_pairs, merge);
+		dir_re_head = get_directory_renames(head_pairs);
+		dir_re_merge = get_directory_renames(merge_pairs);
 
 		handle_directory_level_conflicts(o,
 						 dir_re_head, head,
@@ -3268,8 +3264,7 @@ static int process_entry(struct merge_options *o,
 			clean_merge = 1;
 			if (handle_rename_via_dir(o,
 						  conflict_info->pair1,
-						  conflict_info->branch1,
-						  conflict_info->branch2))
+						  conflict_info->branch1))
 				clean_merge = -1;
 			break;
 		case RENAME_ADD:
-- 
2.21.0.rc1.567.g648f076c3f
Jeff King· Feb 14, 2019, 05:50 UTC · re: Jeff King · lore

[PATCH 06/10] pack-objects: drop unused parameter from oe_map_new_pack()

Since 43fa44fa3b (pack-objects: move in_pack out of struct object_entry, 2018-04-14), we store the source pack for each object as a small index rather than as a pointer. When we see a new pack that has no allocated index, we fall back to generating an array of pointers by calling oe_map_new_pack().

Perhaps counter-intuitively, that function does not need to actually see our new index-less pack. It only allocates and populates the array with the existing packs, after which oe_set_in_pack() actually adds the new pack to the array.

Let's drop the unused "struct packed_git" argument to oe_map_new_pack() to avoid confusion.

Signed-off-by: Jeff King <peff@peff.net>
---
 pack-objects.c | 3 +--
 pack-objects.h | 6 +++---
 2 files changed, 4 insertions(+), 5 deletions(-)
Show changes to 2 files +4 −5

pack-objects.c, pack-objects.h

diff --git a/pack-objects.c b/pack-objects.c
index e7cd337bee..ce33b8906e 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -119,8 +119,7 @@ static void prepare_in_pack_by_idx(struct packing_data *pdata)
  * this fall back code, just stay simple and fall back to using
  * in_pack[] array.
  */
-void oe_map_new_pack(struct packing_data *pack,
-		     struct packed_git *p)
+void oe_map_new_pack(struct packing_data *pack)
 {
 	uint32_t i;
 
diff --git a/pack-objects.h b/pack-objects.h
index 6bfacc7d2c..6fde7ce27c 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -247,14 +247,14 @@ static inline struct packed_git *oe_in_pack(const struct packing_data *pack,
 		return pack->in_pack[e - pack->objects];
 }
 
-void oe_map_new_pack(struct packing_data *pack,
-		     struct packed_git *p);
+void oe_map_new_pack(struct packing_data *pack);
+
 static inline void oe_set_in_pack(struct packing_data *pack,
 				  struct object_entry *e,
 				  struct packed_git *p)
 {
 	if (!p->index)
-		oe_map_new_pack(pack, p);
+		oe_map_new_pack(pack);
 	if (pack->in_pack_by_idx)
 		e->in_pack_idx = p->index;
 	else
-- 
2.21.0.rc1.567.g648f076c3f
Jeff King· Feb 14, 2019, 05:50 UTC · re: Jeff King · lore

[PATCH 07/10] files-backend: drop refs parameter from split_symref_update()

This parameter was added in fcc42ea0c9 (split_symref_update(): add a files_ref_store argument, 2016-09-04) without comment, but never used. The splitting is purely mechanical, and doesn't depend on the particular ref-store. Let's drop this parameter in the name of simplicity.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs/files-backend.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
Show changes to refs/files-backend.c +2 −3
diff --git a/refs/files-backend.c b/refs/files-backend.c
index dd8abe9185..ef053f716c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2254,8 +2254,7 @@ static int split_head_update(struct ref_update *update,
  * Note that the new update will itself be subject to splitting when
  * the iteration gets to it.
  */
-static int split_symref_update(struct files_ref_store *refs,
-			       struct ref_update *update,
+static int split_symref_update(struct ref_update *update,
 			       const char *referent,
 			       struct ref_transaction *transaction,
 			       struct string_list *affected_refnames,
@@ -2449,7 +2448,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 			 * of processing the split-off update, so we
 			 * don't have to do it here.
 			 */
-			ret = split_symref_update(refs, update,
+			ret = split_symref_update(update,
 						  referent.buf, transaction,
 						  affected_refnames, err);
 			if (ret)
-- 
2.21.0.rc1.567.g648f076c3f
Jeff King· Feb 14, 2019, 05:50 UTC · re: Jeff King · lore

[PATCH 08/10] ref-filter: drop unused buf/sz pairs

The grab_tag_values() and grab_commit_values() functions take both the "struct object" as well as the buf/sz pair for the actual object bytes. However, neither function uses the latter, as they pull the data directly from the parsed object struct.

Let's get rid of these misleading parameters.
Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to ref-filter.c +4 −4
diff --git a/ref-filter.c b/ref-filter.c
index 422a9c9ae3..c9333b3181 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -913,7 +913,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 }
 
 /* See grab_values */
-static void grab_tag_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
+static void grab_tag_values(struct atom_value *val, int deref, struct object *obj)
 {
 	int i;
 	struct tag *tag = (struct tag *) obj;
@@ -935,7 +935,7 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob
 }
 
 /* See grab_values */
-static void grab_commit_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
+static void grab_commit_values(struct atom_value *val, int deref, struct object *obj)
 {
 	int i;
 	struct commit *commit = (struct commit *) obj;
@@ -1269,12 +1269,12 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v
 {
 	switch (obj->type) {
 	case OBJ_TAG:
-		grab_tag_values(val, deref, obj, buf, sz);
+		grab_tag_values(val, deref, obj);
 		grab_sub_body_contents(val, deref, obj, buf, sz);
 		grab_person("tagger", val, deref, obj, buf, sz);
 		break;
 	case OBJ_COMMIT:
-		grab_commit_values(val, deref, obj, buf, sz);
+		grab_commit_values(val, deref, obj);
 		grab_sub_body_contents(val, deref, obj, buf, sz);
 		grab_person("author", val, deref, obj, buf, sz);
 		grab_person("committer", val, deref, obj, buf, sz);
-- 
2.21.0.rc1.567.g648f076c3f
Jeff King· Feb 14, 2019, 05:50 UTC · re: Jeff King · lore

[PATCH 09/10] ref-filter: drop unused "obj" parameters

The grab_person() and grab_sub_body_contents() functions take both an object struct and a buf/sz pair of the object bytes. However, they use only the latter, since "struct object" does not contain the parsed ident (nor the whole commit message, of course).

Let's get rid of these misleading "struct object" parameters. It's possible we may want them in the future (e.g., to generate error messages that mention the object id), but since these are static functions, we can easily add them back in later (and if we do want that information, it's likely we'd pass it through a more generalized "parsing context" struct anyway).

Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
Show changes to ref-filter.c +7 −7
diff --git a/ref-filter.c b/ref-filter.c
index c9333b3181..9ba18b220c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1064,7 +1064,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam
 }
 
 /* See grab_values */
-static void grab_person(const char *who, struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
+static void grab_person(const char *who, struct atom_value *val, int deref, void *buf, unsigned long sz)
 {
 	int i;
 	int wholen = strlen(who);
@@ -1192,7 +1192,7 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size
 }
 
 /* See grab_values */
-static void grab_sub_body_contents(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
+static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf, unsigned long sz)
 {
 	int i;
 	const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;
@@ -1270,14 +1270,14 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v
 	switch (obj->type) {
 	case OBJ_TAG:
 		grab_tag_values(val, deref, obj);
-		grab_sub_body_contents(val, deref, obj, buf, sz);
-		grab_person("tagger", val, deref, obj, buf, sz);
+		grab_sub_body_contents(val, deref, buf, sz);
+		grab_person("tagger", val, deref, buf, sz);
 		break;
 	case OBJ_COMMIT:
 		grab_commit_values(val, deref, obj);
-		grab_sub_body_contents(val, deref, obj, buf, sz);
-		grab_person("author", val, deref, obj, buf, sz);
-		grab_person("committer", val, deref, obj, buf, sz);
+		grab_sub_body_contents(val, deref, buf, sz);
+		grab_person("author", val, deref, buf, sz);
+		grab_person("committer", val, deref, buf, sz);
 		break;
 	case OBJ_TREE:
 		/* grab_tree_values(val, deref, obj, buf, sz); */
-- 
2.21.0.rc1.567.g648f076c3f
Jeff King· Feb 14, 2019, 05:51 UTC · re: Jeff King · lore

[PATCH 10/10] ref-filter: drop unused "sz" parameters

Many of our grab_* functions, which parse the object content, take a buf/sz pair of the object bytes. However, the functions which actually parse the buffers (like find_wholine() and find_subpos()) never look at "sz", and instead use functions like strchr() and strchrnul() that assume the result is NUL-terminated.

This is OK in practice (and common for Git's parsing code), since we always allocate an extra NUL when loading an object into memory (and likewise, we are OK with stopping parsing if a commit or tag contains an embedded NUL).

Let's drop these extra "sz" parameters, as they are misleading about how the functions intend to access the buffer. We can drop from both the functions mentioned above, which in turn lets us drop from their callers, cascading all the way up to the top-level grab_values().

Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)
Show changes to ref-filter.c +14 −14
diff --git a/ref-filter.c b/ref-filter.c
index 9ba18b220c..09b5fb439f 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -968,7 +968,7 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object
 	}
 }
 
-static const char *find_wholine(const char *who, int wholen, const char *buf, unsigned long sz)
+static const char *find_wholine(const char *who, int wholen, const char *buf)
 {
 	const char *eol;
 	while (*buf) {
@@ -1064,7 +1064,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam
 }
 
 /* See grab_values */
-static void grab_person(const char *who, struct atom_value *val, int deref, void *buf, unsigned long sz)
+static void grab_person(const char *who, struct atom_value *val, int deref, void *buf)
 {
 	int i;
 	int wholen = strlen(who);
@@ -1085,7 +1085,7 @@ static void grab_person(const char *who, struct atom_value *val, int deref, void
 		    !starts_with(name + wholen, "date"))
 			continue;
 		if (!wholine)
-			wholine = find_wholine(who, wholen, buf, sz);
+			wholine = find_wholine(who, wholen, buf);
 		if (!wholine)
 			return; /* no point looking for it */
 		if (name[wholen] == 0)
@@ -1105,7 +1105,7 @@ static void grab_person(const char *who, struct atom_value *val, int deref, void
 	if (strcmp(who, "tagger") && strcmp(who, "committer"))
 		return; /* "author" for commit object is not wanted */
 	if (!wholine)
-		wholine = find_wholine(who, wholen, buf, sz);
+		wholine = find_wholine(who, wholen, buf);
 	if (!wholine)
 		return;
 	for (i = 0; i < used_atom_cnt; i++) {
@@ -1123,7 +1123,7 @@ static void grab_person(const char *who, struct atom_value *val, int deref, void
 	}
 }
 
-static void find_subpos(const char *buf, unsigned long sz,
+static void find_subpos(const char *buf,
 			const char **sub, unsigned long *sublen,
 			const char **body, unsigned long *bodylen,
 			unsigned long *nonsiglen,
@@ -1192,7 +1192,7 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size
 }
 
 /* See grab_values */
-static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf, unsigned long sz)
+static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 {
 	int i;
 	const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;
@@ -1212,7 +1212,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf,
 		    !starts_with(name, "contents"))
 			continue;
 		if (!subpos)
-			find_subpos(buf, sz,
+			find_subpos(buf,
 				    &subpos, &sublen,
 				    &bodypos, &bodylen, &nonsiglen,
 				    &sigpos, &siglen);
@@ -1265,19 +1265,19 @@ static void fill_missing_values(struct atom_value *val)
  * pointed at by the ref itself; otherwise it is the object the
  * ref (which is a tag) refers to.
  */
-static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
+static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf)
 {
 	switch (obj->type) {
 	case OBJ_TAG:
 		grab_tag_values(val, deref, obj);
-		grab_sub_body_contents(val, deref, buf, sz);
-		grab_person("tagger", val, deref, buf, sz);
+		grab_sub_body_contents(val, deref, buf);
+		grab_person("tagger", val, deref, buf);
 		break;
 	case OBJ_COMMIT:
 		grab_commit_values(val, deref, obj);
-		grab_sub_body_contents(val, deref, buf, sz);
-		grab_person("author", val, deref, buf, sz);
-		grab_person("committer", val, deref, buf, sz);
+		grab_sub_body_contents(val, deref, buf);
+		grab_person("author", val, deref, buf);
+		grab_person("committer", val, deref, buf);
 		break;
 	case OBJ_TREE:
 		/* grab_tree_values(val, deref, obj, buf, sz); */
@@ -1516,7 +1516,7 @@ static int get_object(struct ref_array_item *ref, int deref, struct object **obj
 			return strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
 					       oid_to_hex(&oi->oid), ref->refname);
 		}
-		grab_values(ref->value, deref, *obj, oi->content, oi->size);
+		grab_values(ref->value, deref, *obj, oi->content);
 	}
 
 	grab_common_values(ref->value, deref, oi);
-- 
2.21.0.rc1.567.g648f076c3f

← back to recent threads