git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Derrick Stolee <stolee@gmail.com>
To: Tom Saeger <tom.saeger@oracle.com>,
	Eric Sunshine <sunshine@sunshineco.com>
Cc: Junio C Hamano <gitster@pobox.com>, Git List <git@vger.kernel.org>
Subject: Re: should git maintenance prefetch be taught to honor remote.fetch refspec?
Date: Sat, 3 Apr 2021 18:41:08 -0400	[thread overview]
Message-ID: <8ce278cd-5faa-c3fc-d2cf-6ee99259b21f@gmail.com> (raw)
In-Reply-To: <20210403202142.w4b25fhptcaguxyx@brm-x62-17.us.oracle.com>

On 4/3/2021 4:21 PM, Tom Saeger wrote:
> On Fri, Apr 02, 2021 at 06:32:56PM -0400, Eric Sunshine wrote:
>> On Fri, Apr 2, 2021 at 4:43 PM Derrick Stolee <stolee@gmail.com> wrote:
>>> I have a branch available [1], but I'm seeing some failures only
>>> on FreeBSD [2] and I can't understand why that platform is failing
>>> this test. The current version (as of this writing) does not do
>>> the substring replacement technique, and hence it just gives up
>>> on exact matches. I will try the substring approach as an
>>> alternative and see where that gets me.
>>>
>>> [1] https://github.com/gitgitgadget/git/pull/924
>>> [2] https://github.com/gitgitgadget/git/pull/924/checks?check_run_id=2256079534
>>
>> The "+" in patterns such as `+refs/heads/\\*:refs/prefetch...` is what
>> is throwing it off. FreeBSD `grep` doesn't seem to like it, though
>> it's not clear why. Escaping it the same way as you escaped "*"
>> doesn't make it work. Replacing "+" with catchall "." does work, so
>> that's one way to fix it.
>>
>> However, all the escaping you need to do in these refspec patterns to
>> pass them to `grep` is ugly. A much better solution may be to change
>> the `grep` in test-lib-functions.sh:test_subcommand() to `grep -F` to
>> force it to match literally. That way, you can drop all the backslash
>> escaping, including those in front of "[" and "]". A cursory audit of
>> callers test_subcommand() seems to indicate that none of them pass
>> regex patterns, so using `-F` is probably safe and a good idea.

Yes, this is an excellent idea. Thanks.

>> By the way, the `coccinelle` check is also "failing", correctly
>> suggesting that you change:
>>
>>     strbuf_addf(&replace, ":refs/prefetch/");
>>
>> to:
>>
>>     strbuf_addstr(&replace, ":refs/prefetch/");
>>
>> in `builtin/gc.c`.

Good point! I'll get that, too.

> Was curious - so rolled all Eric's feedback into a patch.
> 
> This passes 'make test' and 'make coccicheck' locally, not sure about FreeBSD.
> 
> Stolee - can you squash this onto your PR and try it if you agree?

I made my own version of a patch before seeing this one, but
thanks for working on it.
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 6348e8d7339c..e9a1cf3e227a 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1652,9 +1652,9 @@ test_subcommand () {
>  
>  	if test -n "$negate"
>  	then
> -		! grep "\[$expr\]"
> +		! grep -F "$expr"
>  	else
> -		grep "\[$expr\]"
> +		grep -F "$expr"
>  	fi

Specifically I would use "[$expr]" here so it includes the
terminating characters of the JSON array that we are testing
against. It can be important that we have the complete set
of arguments, so these brackets are important.

Thanks, all. I'm taking a look at a mechanism for doing this
without munging the raw string directly, as Junio mentioned.
It might require adding an output format method in the refspec
API, but that's probably a good thing, anyway.

Thanks,
-Stolee

      reply	other threads:[~2021-04-03 22:41 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-01 18:49 should git maintenance prefetch be taught to honor remote.fetch refspec? Tom Saeger
2021-04-01 19:07 ` Derrick Stolee
2021-04-01 19:42   ` Tom Saeger
2021-04-01 20:14   ` Junio C Hamano
2021-04-01 22:11     ` Tom Saeger
2021-04-01 22:25     ` Derrick Stolee
2021-04-02 18:27       ` Tom Saeger
2021-04-02 20:43         ` Derrick Stolee
2021-04-02 21:07           ` Derrick Stolee
2021-04-02 21:39             ` Tom Saeger
2021-04-02 22:09               ` Junio C Hamano
2021-04-02 22:27                 ` Tom Saeger
2021-04-02 21:15           ` Tom Saeger
2021-04-02 21:19           ` Junio C Hamano
2021-04-02 21:33             ` Tom Saeger
2021-04-04 20:25             ` Derrick Stolee
2021-04-04 23:10               ` Junio C Hamano
2021-04-05 13:20                 ` Derrick Stolee
2021-04-05 18:48                   ` Junio C Hamano
2021-04-05 20:38                     ` Tom Saeger
2021-04-05 20:47                       ` Junio C Hamano
2021-04-05 20:49                         ` Derrick Stolee
2021-04-05 20:50                           ` Tom Saeger
2021-04-05 20:54                             ` Derrick Stolee
2021-04-02 22:32           ` Eric Sunshine
2021-04-03 20:21             ` Tom Saeger
2021-04-03 22:41               ` Derrick Stolee [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8ce278cd-5faa-c3fc-d2cf-6ee99259b21f@gmail.com \
    --to=stolee@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sunshine@sunshineco.com \
    --cc=tom.saeger@oracle.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).