| Message ID | 20241001170629.7768-1-chandrapratap3519@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | unit-tests: use xstrfmt() instead of a char buffer in t-reftable-stack | expand |
Chandra Pratap <chandrapratap3519@gmail.com> writes: > A char buffer is used to hold refname values as formatted strings > in the reftable_stack_add() test in t/unit-tests/t-reftable-stack.c. > This can be replaced with a single call to xstrfmt() making the test > conciser. It may make the test more concise, but would we now need to worry about leaking .refname? It turns out that we do not have to, as we were already storing the result of xstrdup() to .refname, so we must have been freeing them already (or we are not making existing leak worse, if .refname were leaking). I've heard some noises about using our helper functions in tests, in that a buggy helper function of ours would interfere with testing the thing(s) we truly want to test, but we have been using xstrdup() and replacing it with xstrfmt(), so it is not like we are making things worse in that regard. Not that I entirely buy the "don't use git to test git" argument. One thing that this worsens is that we now have two copies of the literal "branch%02"PRIuMAX string. If we ever want to change one of them, we must remember to change the other to match. Perhaps with another constant, this patch would become perfect, like this? > Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com> > --- > t/unit-tests/t-reftable-stack.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c > index 31d563d992..2d7cfbf8aa 100644 > --- a/t/unit-tests/t-reftable-stack.c > +++ b/t/unit-tests/t-reftable-stack.c > @@ -523,14 +523,12 @@ static void t_reftable_stack_add(void) > check(!err); > > for (i = 0; i < N; i++) { > - char buf[256]; + static const char fmt[] = "branch%02"PRIuMAX; > - snprintf(buf, sizeof(buf), "branch%02"PRIuMAX, (uintmax_t)i); > - refs[i].refname = xstrdup(buf); > + refs[i].refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i); + refs[i].refname = xstrfmt(fmt, (uintmax_t)i); > refs[i].update_index = i + 1; > refs[i].value_type = REFTABLE_REF_VAL1; > t_reftable_set_hash(refs[i].value.val1, i, GIT_SHA1_FORMAT_ID); > > - logs[i].refname = xstrdup(buf); > + logs[i].refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i); + logs[i].refname = xstrfmt(fmt, (uintmax_t)i); > logs[i].update_index = N + i + 1; > logs[i].value_type = REFTABLE_LOG_UPDATE; > logs[i].value.update.email = xstrdup("identity@invalid");
On Tue, Oct 1, 2024 at 3:23 PM Junio C Hamano <gitster@pobox.com> wrote: > One thing that this worsens is that we now have two copies of the > literal "branch%02"PRIuMAX string. If we ever want to change one of > them, we must remember to change the other to match. > > Perhaps with another constant, this patch would become perfect, like > this? > > + static const char fmt[] = "branch%02"PRIuMAX; > > > - snprintf(buf, sizeof(buf), "branch%02"PRIuMAX, (uintmax_t)i); > > - refs[i].refname = xstrdup(buf); > > + refs[i].refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i); > > + refs[i].refname = xstrfmt(fmt, (uintmax_t)i); > > > refs[i].update_index = i + 1; > > refs[i].value_type = REFTABLE_REF_VAL1; > > t_reftable_set_hash(refs[i].value.val1, i, GIT_SHA1_FORMAT_ID); > > > > - logs[i].refname = xstrdup(buf); > > + logs[i].refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i); > > + logs[i].refname = xstrfmt(fmt, (uintmax_t)i); Even simpler would be merely to xstrdup() the string which was already formatted by xstrfmt(), I would think. refs[i].refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i); ... logs[i].refname = xstrdup(refs[i].refname);
diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c index 31d563d992..2d7cfbf8aa 100644 --- a/t/unit-tests/t-reftable-stack.c +++ b/t/unit-tests/t-reftable-stack.c @@ -523,14 +523,12 @@ static void t_reftable_stack_add(void) check(!err); for (i = 0; i < N; i++) { - char buf[256]; - snprintf(buf, sizeof(buf), "branch%02"PRIuMAX, (uintmax_t)i); - refs[i].refname = xstrdup(buf); + refs[i].refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i); refs[i].update_index = i + 1; refs[i].value_type = REFTABLE_REF_VAL1; t_reftable_set_hash(refs[i].value.val1, i, GIT_SHA1_FORMAT_ID); - logs[i].refname = xstrdup(buf); + logs[i].refname = xstrfmt("branch%02"PRIuMAX, (uintmax_t)i); logs[i].update_index = N + i + 1; logs[i].value_type = REFTABLE_LOG_UPDATE; logs[i].value.update.email = xstrdup("identity@invalid");
A char buffer is used to hold refname values as formatted strings in the reftable_stack_add() test in t/unit-tests/t-reftable-stack.c. This can be replaced with a single call to xstrfmt() making the test conciser. Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com> --- t/unit-tests/t-reftable-stack.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)