Re: [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>"

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
	Stefan Beller <sbeller@google.com>
Subject: Re: [PATCH v3 7/8] push: add DWYM support for "git push refs/remotes/...:<dst>"
Date: Mon, 29 Oct 2018 09:05:03 +0100	[thread overview]
Message-ID: <87k1m1kwk0.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <xmqq7ei144gn.fsf@gitster-ct.c.googlers.com>

On Mon, Oct 29 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> This is the first use of the %N$<fmt> style of printf format in
>> the *.[ch] files in our codebase. It's supported by POSIX[2] and
>> there's existing uses for it in po/*.po files,...
>
> For now, I'll eject this from 'pu', as I had spent way too much time
> trying to make it and other topics work there.

I was compiling with DEVELOPER=1 but as it turns out:

    CFLAGS="-O0" DEVELOPER=1

Wasn't doing what I thought, i.e. we just take 'CFLAGS' from the
command-line and don't add any of the DEVELOPER #leftoverbits to
it. Will fix this and other issues raised.

>     CC remote.o
> remote.c: In function 'show_push_unqualified_ref_name_error':
> remote.c:1035:2: error: $ operand number used after format without operand number [-Werror=format=]
>   error(_("The destination you provided is not a full refname (i.e.,\n"
>   ^~~~~
> cc1: all warnings being treated as errors
> Makefile:2323: recipe for target 'remote.o' failed
> make: *** [remote.o] Error 1

Will fix this and other issues raised. FWIW clang gives a much better
error about the actual issue:

    remote.c:1042:46: error: cannot mix positional and non-positional arguments in format string [-Werror,-Wformat]
                    "- Checking if the <src> being pushed ('%2$s')\n"

I.e. this on top fixes it:

    -               "- Looking for a ref that matches '%s' on the remote side.\n"
    -               "- Checking if the <src> being pushed ('%s')\n"
    +               "- Looking for a ref that matches '%1$s' on the remote side.\n"
    +               "- Checking if the <src> being pushed ('%2$s')\n"

Maybe  this whole thing isn't worth it and I should just do:

    @@ -1042 +1042 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
    -               "- Checking if the <src> being pushed ('%2$s')\n"
    +               "- Checking if the <src> being pushed ('%s')\n"
    @@ -1047 +1047 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
    -             dst_value, matched_src_name);
    +             dst_value, matched_src_name, matched_src_name);

But I'm leaning on the side of keeping it for the self-documentation
aspect of "this is a repeated parameter". Your objections to this whole
thing being a stupid idea non-withstanding.