Han-Wen Nienhuys [off-list ref] writes:
On Tue, Nov 30, 2021 at 12:31 AM Junio C Hamano [off-list ref] wrote:
quoted
quoted
+ if (!(flags & REF_SKIP_REFNAME_VERIFICATION) &&
+ ((new_oid && !is_null_oid(new_oid)) ?
+ check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
+ !refname_is_safe(refname))) {
So, if somebody passes REF_SKIP_REFNAME_VERIFICATION in flags, we
will not do the check.
Again, like 3/6, this new bit is flipped on by test-helper
somewhere? Again I do not see anybody doing so in these 6 patches,
but I should double check.
The test helper takes the flag as an argument, in decimal. If you look
for 2048, you should find it.
Awful---when the symbolic constants change in the code, the test
will silently break?
It has been this way since 80f2a609 (t/helper: add test-ref-store to
test ref-store functions, 2017-03-26), so it is nothing new, but at
some point, we should do a better job. Even if it is used only as a
tool for testing, we shouldn't have to force developers to write in
assembly ;-)
Perhaps when the dust settles after this series graduates to be a
part of released version. It may be a good bite-sized microproject
material for aspiring new developers.
On Wed, Dec 01, 2021 at 11:00:04AM -0800, Junio C Hamano wrote:
quoted
The test helper takes the flag as an argument, in decimal. If you look
for 2048, you should find it.
Awful---when the symbolic constants change in the code, the test
will silently break?
Agreed, this is quite nasty.
It has been this way since 80f2a609 (t/helper: add test-ref-store to
test ref-store functions, 2017-03-26), so it is nothing new, but at
some point, we should do a better job. Even if it is used only as a
tool for testing, we shouldn't have to force developers to write in
assembly ;-)
Sort of. The code to pass the flags was added then, but nobody was using
it until now.
At least for ref updates. Symref creation allowed "1" for force, but
that is a true boolean. It looks like some pack-refs calls pass "3" for
PACK_REFS_PRUNE|PACK_REFS_ALL.
So it may be considered a "new" issue in that sense.
Perhaps when the dust settles after this series graduates to be a
part of released version. It may be a good bite-sized microproject
material for aspiring new developers.
It is annoying to have to plumb through the names of the flags, but in
practice I don't think we need all (or even most) of them. And it's
equally annoying not to be able to grep for the flags. I already had a
hard enough time just grepping for the callers, and resorted to:
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 3986665037..1b7a124a86 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -271,6 +271,8 @@ int cmd__ref_store(int argc, const char **argv)
const char *func;
struct command *cmd;
+ trace_argv_printf(argv, "trace: test-tool:");
+
setup_git_directory();
argv = get_store(argv + 1, &refs);
to find them.
-Peff
On Wed, Dec 1, 2021 at 8:26 PM Jeff King [off-list ref] wrote:
On Wed, Dec 01, 2021 at 11:00:04AM -0800, Junio C Hamano wrote:
quoted
quoted
The test helper takes the flag as an argument, in decimal. If you look
for 2048, you should find it.
Awful---when the symbolic constants change in the code, the test
will silently break?
Agreed, this is quite nasty.
I've added parsing symbolic constants in v3.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado