From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-3.9 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A, RCVD_IN_DNSWL_HI,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id A81081F4B4 for ; Sat, 10 Apr 2021 00:56:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235351AbhDJA4s (ORCPT ); Fri, 9 Apr 2021 20:56:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52222 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235249AbhDJA4s (ORCPT ); Fri, 9 Apr 2021 20:56:48 -0400 Received: from mail-oo1-xc2b.google.com (mail-oo1-xc2b.google.com [IPv6:2607:f8b0:4864:20::c2b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A681FC061762 for ; Fri, 9 Apr 2021 17:56:34 -0700 (PDT) Received: by mail-oo1-xc2b.google.com with SMTP id i25-20020a4aa1190000b02901bbd9429832so1763651ool.0 for ; Fri, 09 Apr 2021 17:56:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=Mq3m3vCnPWXHkywyMtPiTnSV8S5aRDYDdnWnT5K8m3g=; b=HyQzpP1CXnDpBk1+PKYL55noB9IuGo89CVZ4oJ/6wWif6sMXbWWHvs41uCtqJZ/r3c 6EhNEToW9Lp0VE9UTGmPj4rOLai0RCU9giSEu482hPX1rVARt/6u85H/5nXTC4IRr/XF BC4ceQ/yaHcuDvgOFqNMY+6XyHFW3l+R9lXve2Uw79Tv0cO6ZMXwfGMDo6zHzv5vjmRF CSdV2jFmYngRRYZU+u9ox2+X3cBofmc6G2N+J0I3DUgkiIDCnKoxO8bcXE4e+0eFhSD+ ZQ+5FurPiMat7WoxRhRv3ARTFYLbxP7QrOMtV5Eee811/CUuYdRZj0nj67alT68Xdh1w 75Ww== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Mq3m3vCnPWXHkywyMtPiTnSV8S5aRDYDdnWnT5K8m3g=; b=icgkwa3VHddjc511rxcTduufSdaZdJfZerasFyp3KSOE7sYk0KUJ74+CNFtz36quqr WfNvQ4AFmAwe5ZdZ/sKEHhszP1sdLtT3V9G23L7J2yZ2iELM3SW/wDxxXeaq3CRAchuo NePdNR3lEIO2LTfVvr0sVponJ5lZdYcjS70IXyTCwpgr2Slt9jllI0AbVJ+1w84bi4Ag 3RPRu4wSvV7tT2kJlWq66f21EJfzFq9b4c836WCMe+Ner4N8ha3tOIAx2DBgLQFfqr90 DEahi1sl9D+Q6FIx4D17VliyFwQQUhga3Qb1bokJaL6c8bwEPavOa5AR3z1aFuIxZA7s y9KQ== X-Gm-Message-State: AOAM532eJlHFTANGv3/JQFIlygbD4DNBet7E3BW7pLEBuAOeNJKpyXPO 2ulgKJTP2/DaSvWpVoiEr6Q= X-Google-Smtp-Source: ABdhPJxQV0o8YUDItPx8ejRiFRYDPDhsOm1UB26K+JQvYK7tHTRmzKiWzlYNft3dsRZSUGGpfF2uOg== X-Received: by 2002:a4a:e9a2:: with SMTP id t2mr13985396ood.15.1618016193779; Fri, 09 Apr 2021 17:56:33 -0700 (PDT) Received: from ?IPv6:2600:1700:e72:80a0:e556:d73b:6b67:5b9f? ([2600:1700:e72:80a0:e556:d73b:6b67:5b9f]) by smtp.gmail.com with ESMTPSA id f29sm963225ots.22.2021.04.09.17.56.32 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 09 Apr 2021 17:56:33 -0700 (PDT) Subject: Re: [PATCH 5/5] maintenance: allow custom refspecs during prefetch To: =?UTF-8?B?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?= Cc: Derrick Stolee via GitGitGadget , git@vger.kernel.org, tom.saeger@oracle.com, gitster@pobox.com, sunshine@sunshineco.com, Derrick Stolee , Derrick Stolee References: <7f6c127dac48409ddc8d30ad236182bee21c1957.1617627856.git.gitgitgadget@gmail.com> <87o8eqjx0w.fsf@evledraar.gmail.com> <87im4yjspp.fsf@evledraar.gmail.com> <243a0aa1-7c0a-d2ab-49ff-8ea8cacc1b81@gmail.com> <87im4vgsvx.fsf@evledraar.gmail.com> From: Derrick Stolee Message-ID: <36aa6837-722c-9ef0-84cc-77e982db9f6e@gmail.com> Date: Fri, 9 Apr 2021 20:56:31 -0400 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: <87im4vgsvx.fsf@evledraar.gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On 4/9/2021 3:28 PM, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Apr 09 2021, Derrick Stolee wrote: > >> On 4/7/2021 6:26 AM, Ævar Arnfjörð Bjarmason wrote: >>> I think converting the whole thing to something like the WIP/RFC patch >>> below is much better and more readable. >> >> This is an interesting approach. I don't see you using the ERR that you >> are inputting anywhere, so that seems like an unnecessary bloat to the >> consumers. But maybe I haven't discovered all of the places where this >> would be useful, but it seems better to pipe stderr to a file for later >> comparison when needed. > > Yes, it's probably not a good default here. For the test-lib.sh tests > there's check_sub_test_lib_test and check_sub_test_lib_test_err, most of > the tests only test stdout. > >>> +test_expect_process_tree () { >>> + depth= && >>> + >actual && >>> + cat >expect && >>> + cat <&3 >expect.err >>> + while test $# != 0 >>> + do >>> + case "$1" in >>> + --depth) >>> + depth="$2" >>> + shift >>> + ;; >>> + *) >>> + break >>> + ;; >>> + esac >>> + shift >>> + done && >> Do you have an example where this is being checked? Or can depth >> be left as 1 for now? > > It can probably be hardcoded, but I was hoping someone more familiar > with trace2 would chime in, but I'm fairly sure there's not a way to do > it without parsing the existing output with either some clever > grep/awk-ing of the PERF output, or stateful parsing of the JSON. > > I thought that for git maintenance tests perhaps something wanted to > assert that we didn't have maintenance invoking maintenance, or that > something expected to prune refs really invoked the relevant prune > command via "gc". > >>> + log="$(pwd)/proc-tree.txt" && >>> + >"$log" && >>> + GIT_TRACE2_PERF="$log" "$@" 2>actual.err && >>> + grep "child_start" proc-tree.txt >proc-tree-start.txt || : && >>> + if test -n "$depth" >>> + then >>> + grep " d$depth " proc-tree-start.txt >tmp.txt || : && >>> + mv tmp.txt proc-tree-start.txt >>> + fi && >>> + sed -e 's/^.*argv:\[//' -e 's/\]$//' actual && >>> + test_cmp expect actual && >>> + test_cmp expect.err actual.err >>> +} 7>&2 2>&4 >> >> I think similar ideas could apply to test_region. Giving it a try >> now. > > Probably, I didn't even notice that one... I gave this a few hours today, and I'm giving up. I'm the first to admit that I don't have the correct scripting skills to do some of these things. I've got what I tried below. It certainly looks like it would work. It solves the problem of "what if the test is flaky?" by ensuring that all subcommands (at depth 0) match the inputs exactly. However, the problem comes when trying to make that work for all of the maintenance tests, specifically the 'incremental-repack' task. That task dynamically computes a --batch-size=X parameter, and that is not stable across runs of the script. This was avoided in the past by only checking for the first of three subcommands when verifying that the 'incremental-repack' task worked. That is, except for the EXPENSIVE test that checks that the --batch-size maxes out at 2g. The thing that might make these changing parameters work is to allow the specified lines be a _prefix_ of the actual parameters. Or, let each line be a pattern that is checked against that line. Issues come up with how to handle this line-by-line check that I was unable to overcome. The good news is that the idea of adding a '--prefetch' option to 'git fetch' makes the change to t7900-maintenance.sh much easier, making this change to test_subcommand less of a priority. I include my attempt here as a patch. Feel free to take whatever you want of it, or none of it and start over. I do think that it makes the test script look much nicer. Thanks, -Stolee -- >8 -- >From 449d098f2a13860f44b2e6fb96fb2dd5872b511b Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Fri, 9 Apr 2021 08:42:26 -0400 Subject: [PATCH 1/2] test-lib: add test_subcommands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test_subcommand helper in test-lib-functions.sh satisfied a need to check for certain subcommands in Git processes. This is especially needed in t7900-maintenance.sh to ensure that the maintenance builtin properly calls certain subcommands based on command-line options, config values, and the state of the repository. However, test_subcommand has some complexities in its use that can be improved. First, it requires that the caller knows to create a log file with GIT_TRACE2_EVENT then supply that to test_subcommand. Further, it only checks that some exact subcommands exist or do not exist. It does not guarantee that the list of subcommands exactly matches a given list. Because of this drawback, the tests that check a subcommand does _not_ run are particularly flaky to slight changes in behavior. Introduce a new helper, test_subcommands, that resolves these drawbacks: 1. It runs the supplied command and handles the trace log itself. 2. It takes a list of commands overs stdin and compares this to the complete list of subcommands run from the top-level command. The helper does not test that the full subcommand tree matches, because that would cause tests to be too fragile to changes unrelated to the component being tested. This could easily be extended to allow the full tree with an option, if desired. To ensure we only check the first level, use GIT_TRACE2_PERF output and scan for " d0 " in the rows that include the "child_start" event. The last column includes a way to scrape the subcommand itself from the trace. Sometimes arguments are quoted, such as when passing a refspec with '*' to the subcommand. This makes it difficult to create a matching string within the single-quoted test definitions, so strip these single quotes from the arguments before matching the input. Only modify one test in t7900-maintenance.sh. The rest of the callers to test_subcommand will be converted to test_subcommands in a later change. Helped-by: Ævar Arnfjörð Bjarmason Signed-off-by: Derrick Stolee --- t/t7900-maintenance.sh | 16 +++++++--------- t/test-lib-functions.sh | 24 ++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 2412d8c5c0..e170ab7862 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -30,15 +30,13 @@ test_expect_success 'help text' ' ' test_expect_success 'run [--auto|--quiet]' ' - GIT_TRACE2_EVENT="$(pwd)/run-no-auto.txt" \ - git maintenance run 2>/dev/null && - GIT_TRACE2_EVENT="$(pwd)/run-auto.txt" \ - git maintenance run --auto 2>/dev/null && - GIT_TRACE2_EVENT="$(pwd)/run-no-quiet.txt" \ - git maintenance run --no-quiet 2>/dev/null && - test_subcommand git gc --quiet expect && + + log="$(pwd)"/subcommand-trace.txt && + + GIT_TRACE2_PERF="$log" "$@" 2>err && + grep "child_start" "$log" | grep " d0 " >processes || : && + + sed -e 's/^.*argv:\[//' -e 's/\]$//' -e "s/'//g" actual && + test_cmp expect actual && + + rm -f "$log" expect actual processes +} + # Check that the given command was invoked as part of the # trace2-format trace on stdin. # -- 2.31.1.vfs.0.0 --- >8 --- And here is the follow-up that attempts to change the rest of the tests in t7900-maintenance.sh. However, this leads to flaky tests because of the --batch-size changes. --- >8 --- >From 61f142ba7fcde8ae2a84f18005f384a4544b7741 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Fri, 9 Apr 2021 11:13:42 -0400 Subject: [PATCH 2/2] t7900: convert to test_subcommands Replace the remaining uses of test_subcommand with test_subcommands, allowing us to delete the old helper. Most of these replacements are straightforward. However, some are a bit more subtle, specifically because we now are checking the full ordered set of subcommands. Some places we were only testing one of multiple subcommands that would be run by a task. These are expanded to include the full set. When working with the prefetch task, the refspecs that are passed to the subcommand are normally quoted with single quotes in the GIT_TRACE2_PERF output, but those characters are removed in test_subcommands, allowing our tests in t7900-maintenance.sh to avoid escaping such quotes. Signed-off-by: Derrick Stolee --- t/t7900-maintenance.sh | 285 +++++++++++++++++++--------------------- t/test-lib-functions.sh | 33 ----- 2 files changed, 133 insertions(+), 185 deletions(-) diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index e170ab7862..8861435dcf 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -40,78 +40,65 @@ test_expect_success 'run [--auto|--quiet]' ' ' test_expect_success 'maintenance.auto config option' ' - GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 && - test_subcommand git maintenance run --auto --quiet .enabled' ' git config maintenance.gc.enabled false && git config maintenance.commit-graph.enabled true && - GIT_TRACE2_EVENT="$(pwd)/run-config.txt" git maintenance run 2>err && - test_subcommand ! git gc --quiet ' ' - GIT_TRACE2_EVENT="$(pwd)/run-commit-graph.txt" \ - git maintenance run --task=commit-graph 2>/dev/null && - GIT_TRACE2_EVENT="$(pwd)/run-gc.txt" \ - git maintenance run --task=gc 2>/dev/null && - GIT_TRACE2_EVENT="$(pwd)/run-commit-graph.txt" \ - git maintenance run --task=commit-graph 2>/dev/null && - GIT_TRACE2_EVENT="$(pwd)/run-both.txt" \ - git maintenance run --task=commit-graph --task=gc 2>/dev/null && - test_subcommand ! git gc --quiet /dev/null && - test_subcommand ! git commit-graph write --split --reachable --no-progress \ - did-run.txt <<-EOF && + git commit-graph write --split --reachable --no-progress + EOF - GIT_TRACE2_EVENT="$(pwd)/cg-no.txt" \ - git -c maintenance.commit-graph.auto=1 $COMMAND && - GIT_TRACE2_EVENT="$(pwd)/cg-negative-means-yes.txt" \ - git -c maintenance.commit-graph.auto="-1" $COMMAND && + test_subcommands git -c maintenance.commit-graph.auto=1 $COMMAND /dev/null && fetchargs="--prune --no-tags --no-write-fetch-head --recurse-submodules=no --refmap= --quiet" && - test_subcommand git fetch remote1 $fetchargs +refs/heads/\\*:refs/prefetch/remote1/\\* /dev/null && - test_subcommand ! git prune-packed --quiet /dev/null && - test_subcommand ! git prune-packed --quiet /dev/null && - test_subcommand git prune-packed --quiet /dev/null && - test_subcommand git prune-packed --quiet /dev/null && - test_subcommand git multi-pack-index repack \ - --no-progress --batch-size=2147483647 /dev/null && - test_subcommand ! git multi-pack-index write --no-progress /dev/null && - test_subcommand ! git multi-pack-index write --no-progress /dev/null && - test_subcommand git multi-pack-index write --no-progress after && - test_must_be_empty after && - test_subcommand git pack-refs --all --prune daily -> hourly' ' git config maintenance.incremental-repack.enabled true && git config maintenance.incremental-repack.schedule weekly && - GIT_TRACE2_EVENT="$(pwd)/hourly.txt" \ - git maintenance run --schedule=hourly 2>/dev/null && - test_subcommand git prune-packed --quiet hourly <<-EOF && + git prune-packed --quiet + EOF - GIT_TRACE2_EVENT="$(pwd)/daily.txt" \ - git maintenance run --schedule=daily 2>/dev/null && - test_subcommand git prune-packed --quiet daily <<-EOF && + git prune-packed --quiet + git commit-graph write --split --reachable --no-progress + EOF - GIT_TRACE2_EVENT="$(pwd)/weekly.txt" \ - git maintenance run --schedule=weekly 2>/dev/null && - test_subcommand git prune-packed --quiet weekly <<-EOF && + git prune-packed --quiet + git multi-pack-index write --no-progress + git multi-pack-index expire --no-progress + git multi-pack-index repack --no-progress --batch-size=655 + git commit-graph write --split --reachable --no-progress + EOF + + test_subcommands git maintenance run --schedule=hourly ... < -# -# For example, to look for an invocation of "git upload-pack -# /path/to/repo" -# -# GIT_TRACE2_EVENT=event.log git fetch ... && -# test_subcommand git upload-pack "$PATH"