Re: [PATCH] fast-export: factor out print_oid()
From: Junio C Hamano <hidden>
Date: 2020-08-13 18:20:07
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2020-08-17 · Re: [PATCH] fast-export: factor out print_oid() · Junio C Hamano <hidden>
- 2020-08-17 · Re: [PATCH] fast-export: factor out print_oid() · Jeff King <hidden>
- 2020-08-16 · Re: [PATCH] fast-export: factor out print_oid() · René Scharfe <hidden>
- 2020-08-13 · Re: [PATCH] fast-export: factor out print_oid() · Jeff King <hidden>
- 2020-08-13 · Re: [PATCH] fast-export: factor out print_oid() · René Scharfe <hidden>
Taylor Blau [off-list ref] writes:
quoted
+static void print_oid(const struct object_id *oid, int anonymize) +{ + const char *oid_hex = oid_to_hex(oid); + if (anonymize) + oid_hex = anonymize_oid(oid_hex); + fputs(oid_hex, stdout); +} +The fact that this calls fputs makes this patch (in my own opinion) noisier than it needs to be. This is because of all of the factoring out of the other printfs. I'd expect that this looks something more like: - anonymize ? - anonymize_oid(oid_to_hex(&spec->oid)) : - oid_to_hex(&spec->oid)); + anonymize_oid(anonymize, &spec->oid)); without moving around all of the other printf code. ... So, I guess what I'm trying to say is that this patch doesn't look wrong, other than that it seems more invasive than I would have expected it to be.
Yes, that matches my knee-jerk reaction. I also shared Peff's reaction that the code to generate the message is now even more fragmented into pieces. Just for comparison purposes, I tried to fold anonymize_oid()'s body into René's print_oid() and adjusted the calling sites, which did not look too bad. So, I dunno. builtin/fast-export.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 9f37895d4c..bef2c01bd8 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c@@ -413,11 +413,16 @@ static char *generate_fake_oid(void *data) return hash_to_hex_algop_r(hex, out, the_hash_algo); } -static const char *anonymize_oid(const char *oid_hex) +static const char *anonymize_oid(const struct object_id *oid, int anonymize) { - static struct hashmap objs; - size_t len = strlen(oid_hex); - return anonymize_str(&objs, generate_fake_oid, oid_hex, len, NULL); + const char *oid_hex = oid_to_hex(oid); + if (anonymize) { + static struct hashmap objs; + size_t len = strlen(oid_hex); + return anonymize_str(&objs, generate_fake_oid, oid_hex, len, NULL); + } else { + return oid_hex; + } } static void show_filemodify(struct diff_queue_struct *q,
@@ -476,9 +481,7 @@ static void show_filemodify(struct diff_queue_struct *q, */ if (no_data || S_ISGITLINK(spec->mode)) printf("M %06o %s ", spec->mode, - anonymize ? - anonymize_oid(oid_to_hex(&spec->oid)) : - oid_to_hex(&spec->oid)); + anonymize_oid(&spec->oid, anonymize)); else { struct object *object = lookup_object(the_repository, &spec->oid);
@@ -726,10 +729,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev, if (mark) printf(":%d\n", mark); else - printf("%s\n", - anonymize ? - anonymize_oid(oid_to_hex(&obj->oid)) : - oid_to_hex(&obj->oid)); + printf("%s\n", anonymize_oid(&obj->oid, anonymize)); i++; }