From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-3.7 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, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE shortcircuit=no autolearn=ham autolearn_force=no version=3.4.6 Received: from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20]) by dcvr.yhbt.net (Postfix) with ESMTP id DF0701F545 for ; Wed, 26 Jul 2023 13:08:40 +0000 (UTC) Authentication-Results: dcvr.yhbt.net; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20221208 header.b=CVnCRzjX; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233561AbjGZNIg (ORCPT ); Wed, 26 Jul 2023 09:08:36 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35060 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233235AbjGZNIb (ORCPT ); Wed, 26 Jul 2023 09:08:31 -0400 Received: from mail-wr1-x42f.google.com (mail-wr1-x42f.google.com [IPv6:2a00:1450:4864:20::42f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E09B91FFC for ; Wed, 26 Jul 2023 06:08:25 -0700 (PDT) Received: by mail-wr1-x42f.google.com with SMTP id ffacd0b85a97d-3159d5e409dso706494f8f.0 for ; Wed, 26 Jul 2023 06:08:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1690376904; x=1690981704; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:reply-to:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to; bh=6AZgJj5Y88IY4ZJTtEzozoXzCt8kPGCwODt+ePPlnJM=; b=CVnCRzjXktxQbz+rTROnLc0mJHu3/mtMA1fsl38t852H4TdrdEewBuFzL5d21CJN2j RACYtuInjwwRvpKKh1D5WHJ882mqlNkk92otqIjUpe462jrI++zKbnOUlUTGGGHCqFHa Fqd8lchvNUPMzRQ7/GmXx2Mr8cWzcD+duPso01f9Hfu5mqeDxpEucOvoKjfswXjTqVUu 0gbV3oVhJBO6a74bA9C78RYeeCLzp0q42dvflExUvd3+VjITcTorWAw1xF7vGsOdnyuN o+xdwNV0fr0DdB7a6qARnK0i6f4vh1cRhwl1BKnPFgiPHtJEQL2+pen45YiUQPbl33mv aM9Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690376904; x=1690981704; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:reply-to:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=6AZgJj5Y88IY4ZJTtEzozoXzCt8kPGCwODt+ePPlnJM=; b=PG7MqbFhH1B+QjorK/5aHcuJtB1KdcHqOVhsAMJRsonCcmz+3mE1UU/JecaTArc5kF L/O/K1L6GQsOtCBL2UNZ5QVTjOQYsyc4UbnQFu9e3VRAzo8C4t//Ora92VwvFfpeUfGm 3Vcq1aYkp5pw6sX2XBlaZLVdS6fu20niqTvlYVWEH6rqxXv1KAw84gEYXJtzd3aDc+5A PWtZiPeoymyMNGCLLAB2gzFmZcbwGSqUFEgHoVBXn+KZAIKurpJESsSXe9cCFcsulbEQ QneV18mvR9Bd6sOjBFGT5kCQiYGg5kj4twgzsnWQIpIwmv2TGQ4GvEXwexHexskNgI4r uu0A== X-Gm-Message-State: ABy/qLYdsoFlO5MQPImDIJTlNsENVkjJpAMXr1Y/Sxit1FO6b+huW+mF h/d/ixoVkfDwkqq5usRx110= X-Google-Smtp-Source: APBJJlG8B/5waXJG6Sf2fUBr7gfjoD8VAX1vN5vvdldQRxxIr6etJJeBgUdjD56BA01cTxq6RogWhw== X-Received: by 2002:adf:f8c7:0:b0:311:360e:ea3a with SMTP id f7-20020adff8c7000000b00311360eea3amr4960980wrq.34.1690376904056; Wed, 26 Jul 2023 06:08:24 -0700 (PDT) Received: from [192.168.1.195] ([90.242.235.211]) by smtp.googlemail.com with ESMTPSA id e1-20020adfe381000000b00313f031876esm19824930wrm.43.2023.07.26.06.08.23 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 26 Jul 2023 06:08:23 -0700 (PDT) Message-ID: Date: Wed, 26 Jul 2023 14:08:22 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Reply-To: phillip.wood@dunelm.org.uk Subject: Re: [PATCH v2 5/6] rebase: fix rewritten list for failed pick Content-Language: en-US To: Glen Choo , Phillip Wood via GitGitGadget , git@vger.kernel.org Cc: Johannes Schindelin , Junio C Hamano , Stefan Haller , Phillip Wood References: From: Phillip Wood In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Hi Glen On 25/07/2023 17:46, Glen Choo wrote: > Phillip Wood writes: >>>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh >>>> index c1fe55dc2c1..a657167befd 100755 >>>> --- a/t/t3404-rebase-interactive.sh >>>> +++ b/t/t3404-rebase-interactive.sh >>>> @@ -1289,6 +1289,10 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' ' >>>> test_cmp_rev HEAD F && >>>> rm file6 && >>>> test_path_is_missing .git/rebase-merge/author-script && >>>> + test_path_is_missing .git/rebase-merge/patch && >>>> + test_path_is_missing .git/MERGE_MSG && >>>> + test_path_is_missing .git/rebase-merge/message && >>>> + test_path_is_missing .git/rebase-merge/stopped-sha && >>> >>> This also seems to be testing implementation details, and if so, it >>> would be worth removing them. >> >> With the exception of the "patch" file which exists solely for the >> benefit of the user this is testing an invariant of the implementation >> which isn't ideal. I'm worried that removing these checks will mask some >> subtle regression in the future. I think it is unlikely that the names >> of these files will change in the future as we try to avoid changes that >> would cause a rebase to fail if git is upgraded while it has stopped for >> the user to resolve conflicts. I did think about whether we could add >> some BUG() statements to sequencer.c instead. Unfortunately I don't >> think it is that easy for the sequencer to know when these files should >> be missing without relying on the logic that we are tying to test. > > Unfortunately, it's been a while since I reviewed this patch, so forgive > me if I'm rusty. So you're saying that this test is about checking > invariants that we want to preserve between Git versions. Not really. One of the reasons why testing the implementation rather than the user observable behavior is a bad idea is that when the implementation is changed the test is likely to start failing or keep passing without checking anything useful. I was trying to say that in this case we're unlikely to change this aspect of the implementation because it would be tricky to do so without inconveniencing users who upgrade git while rebase is stopped for a conflict resolution and so it is unlikely that this test will be affected by future changes to the implementation. > IIRC, there was an earlier patch would be different from an where we > tested that author-script is missing, but what we really want is for the > pick to stop. Is the same thing happening here? E.g. is 'testing for > missing stopped-sha' a stand-in for 'testing that the rewritten list is > correct'? If so, it would be nice to test that specifically, but if > that's infeasible, a clarifying comment will probably suffice. Yes this patch adds a test to t5407-post-rewrite-hook.sh to do that but it only checks a failing "pick" command. The reason I think it is useful to add these test_path_is_missing checks is that they are checking failing "squash" and "merge" commands as well. Maybe I should just bite the bullet see how tricky it is to extend the post-rewrite-hook test to cover those cases as well. Best Wishes Phillip