On Mon, Aug 04, 2025 at 06:46:50AM -0700, Junio C Hamano wrote:
> > + /* All modes require at least a remote name. */
> > + if (!argc)
> > + usage_with_options(builtin_remote_sethead_usage, options);
> > +
> > + strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
> > + remote = remote_get(argv[0]);
>
> I do not know about compilers, but a sample of one, to this human it
> is more obvious ;-).
OK, cleaned up patch is below. Hopefully I am not stealing Denton's
thunder, but this seemed trivial enough that I wanted to get it off my
plate and never think of it again. ;)
> > and the line it complains about is:
> >
> > if (filter && strncmp(test[i].name, filter, matchlen))
> > ...
> > At any rate I agree that "0" is the appropriate value here, and
> > assigning it to shut up the compiler is the best approach.
>
> ... simply because we know the value in matchlen does not matter
> when filter is NULL? I think that would work and I would be happy
> with a less noisy compilation.
>
> But any other value like 99 would equally well work, which is a bit
> disturbing ;-).
It's true that any value would work with the current code. But I think
"0" makes the most sense because it is counting bytes in "filter". If
"filter" is NULL, then we have zero matched bytes. So if anybody _did_
look at it, they'd hopefully do the right thing.
BTW, this clar code comes from libgit2. They may want to fix it
upstream, too. +cc Patrick.
-- >8 --
Subject: [PATCH] remote: bail early from set_head() if missing remote name
In "git remote set-head", we can take varying numbers of arguments
depending on whether we saw the "-d" or "-a" options. But the first
argument is always the remote name.
The current code is somewhat awkward in that it conditionally handles
the remote name up-front like this:
if (argc)
remote = ...from argv[0]...
and then only later decides to bail if we do not have the right number
of arguments for the options we saw.
This makes it hard to figure out if "remote" is always set when it needs
to be. Both for humans, but also for compilers; with -Og, gcc complains
that "remote" can be accessed without being initialized (although this
is not true, as we'd always die with a usage message in that case).
Let's instead enforce the presence of the remote argument up front,
which fixes the compiler warning and is easier to understand. It does
mean duplicating the code to print a usage message, but it's a single
line.
Noticed-by: Denton Liu <liu.denton@xxxxxxxxx>
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
builtin/remote.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 5dd6cbbaee..f0e49a5681 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1474,10 +1474,13 @@ static int set_head(int argc, const char **argv, const char *prefix,
};
argc = parse_options(argc, argv, prefix, options,
builtin_remote_sethead_usage, 0);
- if (argc) {
- strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
- remote = remote_get(argv[0]);
- }
+
+ /* All modes require at least a remote name. */
+ if (!argc)
+ usage_with_options(builtin_remote_sethead_usage, options);
+
+ strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
+ remote = remote_get(argv[0]);
if (!opt_a && !opt_d && argc == 2) {
head_name = xstrdup(argv[1]);
--
2.50.1.786.g492fc26cdf
