From patchwork Thu Sep 27 12:44:30 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?SZEDER_G=C3=A1bor?= X-Patchwork-Id: 10617967 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id EAB331759 for ; Thu, 27 Sep 2018 12:44:56 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D86842B425 for ; Thu, 27 Sep 2018 12:44:56 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id CBE302B432; Thu, 27 Sep 2018 12:44:56 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C26302B425 for ; Thu, 27 Sep 2018 12:44:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727334AbeI0TDB (ORCPT ); Thu, 27 Sep 2018 15:03:01 -0400 Received: from mail-wm1-f45.google.com ([209.85.128.45]:53603 "EHLO mail-wm1-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727111AbeI0TDA (ORCPT ); Thu, 27 Sep 2018 15:03:00 -0400 Received: by mail-wm1-f45.google.com with SMTP id b19-v6so5778006wme.3 for ; Thu, 27 Sep 2018 05:44:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=q3MdaH6bbL6ji957pBbkJ9qsx3BOWb/Na/SM+TOnOxs=; b=PBbFa5iEMqVFqwvH6CMY1jPd8NeN4owgyOTjcd74Zd/qQa31XrFJwGF0ReZPVXsSQ9 hdVK2Iwu0bICw/6VjdOfncpLER6AR4uoOVV2ovrRrecRqMOU+5vGFQ7oynh8EGij5pBt InVqM2N6ir/MoCUTb2dmNV3eJbkx5UPzLycUztT4YytxOqZ0Ovn8Jf/ZYDNVe/A8nWdG sw9Or1C2Hvt/3BoRCdGEh1YykiV3xXegh0wZcpXCnv14DNEFVnG7gjJ6LzF9zGDVG+gD UYHWtSf/+2504QqLowghgFjhTWu7K0kWztJleMm0ch5MZ6qUStkljDrbs1S3hrXdZbJm 5FVQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=q3MdaH6bbL6ji957pBbkJ9qsx3BOWb/Na/SM+TOnOxs=; b=oyYj4NUc3W9u24q8k1Bpc6mozJTxcnyMz3Zu2PEsSO911Wou5cWkopQfF8SBY8AjwZ T3DVJWR0rgzUSYytgHPK7pj9NVr0DJ/FivdO1GGrS6/MFHjZ+3yoCpEj4mvKQfRE1lVb Hb9jiWzu7kaCR1le+uilfTlP07cX3G7gTukZBizaJTOu8njIvfAfBbdWReUC0fmOIcgf AqSzyymZOmZYabb9wH1PLgKwQc0E/PFmqw+/oC277sF5yXIKJ2qXg4tXyX1sUaZw20J1 HIgDHiMgDUYhqhz3S6AtQq8pIcC6FV0GPD6PyYmE2OK88bD1TT5kPjkV7c0MYYzK03je VLMw== X-Gm-Message-State: ABuFfoitEIuyJD84GSc/b7OZ6sEDKefbdqkplaw86X4qxA5Qj8HKrKsL OLcLWBLjJ3mMwQuxpg8xIobyyld5 X-Google-Smtp-Source: ACcGV63hYjX9wCUF2sVj8X43KMWuOr4I4RmuhKfy2tHt3ZLikxZO+PBzHnmDSz5/l1RKGiCbTUMrpg== X-Received: by 2002:a1c:c011:: with SMTP id q17-v6mr7950481wmf.37.1538052291046; Thu, 27 Sep 2018 05:44:51 -0700 (PDT) Received: from localhost.localdomain (x4dbd8656.dyn.telefonica.de. [77.189.134.86]) by smtp.gmail.com with ESMTPSA id c8-v6sm1938543wrn.43.2018.09.27.05.44.49 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 27 Sep 2018 05:44:50 -0700 (PDT) From: =?utf-8?q?SZEDER_G=C3=A1bor?= To: git@vger.kernel.org Cc: Junio C Hamano , Duy Nguyen , Thomas Gummerer , =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBC?= =?utf-8?b?amFybWFzb24=?= , Paul-Sebastian Ungureanu , =?utf-8?q?SZED?= =?utf-8?q?ER_G=C3=A1bor?= Subject: [PATCH v2 1/5] split-index: add tests to demonstrate the racy split index problem Date: Thu, 27 Sep 2018 14:44:30 +0200 Message-Id: <20180927124434.30835-2-szeder.dev@gmail.com> X-Mailer: git-send-email 2.19.0.361.gafc87ffe72 In-Reply-To: <20180927124434.30835-1-szeder.dev@gmail.com> References: <20180927124434.30835-1-szeder.dev@gmail.com> MIME-Version: 1.0 Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Ever since the split index feature was introduced [1], refreshing a split index is prone to a variant of the classic racy git problem. There are a couple of unrelated tests in the test suite that occasionally fail when run with 'GIT_TEST_SPLIT_INDEX=yes', but 't1700-split-index.sh', the only test script focusing solely on split index, has never noticed this issue, because it only cares about how the index is split under various circumstances and all the different ways to turn the split index feature on and off. Add a dedicated test script 't1701-racy-split-index.sh' to exercise the split index feature in racy situations as well; kind of a "t0010-racy-git.sh for split index" but with modern style (the tests do everything in &&-chained list of commands in 'test_expect_...' blocks, and use 'test_cmp' for more informative output on failure). The tests cover the following sequences of index splitting, updating, and racy file modifications, with the last two cases demonstrating the racy split index problem: 1. Split the index while adding a racily clean file: echo "cached content" >file git update-index --split-index --add file echo "dirty worktree" >file # size stays the same This case already works properly. Even though the cache entry's stat data matches with the modifid file in the worktree, subsequent git commands will notice that the (split) index and the file have the same mtime, and then will go on to check the file's content and notice its dirtiness. 2. Add a racily clean file to an already split index: git update-index --split-index echo "cached content" >file git update-index --add file echo "dirty worktree" >file This case already works properly. After the second 'git update-index' writes the newly added file's cache entry to the new split index, it basically works in the same way as case #1. 3. Split the index when it (i.e. the not yet splitted index) contains a racily clean cache entry, i.e. an entry whose cached stat data matches with the corresponding file in the worktree and the cached mtime matches that of the index: echo "cached content" >file git update-index --add file echo "dirty worktree" >file # ... wait ... git update-index --split-index --add other-file This case already works properly. The shared index is written by do_write_index(), i.e. the same function that is responsible for writing "regular" and split indexes as well. This function cleverly notices the racily clean cache entry, and writes the entry to the new shared index with smudged stat data, i.e. file size set to 0. When subsequent git commands read the index, they will notice that the smudged stat data doesn't match with the file in the worktree, and then go on to check the file's content and notice its dirtiness. 4. Update the split index when it contains a racily clean cache entry: git update-index --split-index echo "cached content" >file git update-index --add file echo "dirty worktree" >file # ... wait ... git update-index --add other-file This case already works properly. After the second 'git update-index' the newly added file's cache entry is only stored in the split index. If a cache entry is present in the split index (even if it is a replacement of an outdated entry in the shared index), then it will always be included in the new split index on subsequent split index updates (until the file is removed or a new shared index is written), independently from whether the entry is racily clean or not. When do_write_index() writes the new split index, it notices the racily clean cache entry, and smudges its stat date. Subsequent git commands reading the index will notice the smudged stat data and then go on to check the file's content and notice its dirtiness. 5. Update the split index when a racily clean cache entry is stored only in the shared index: echo "cached content" >file git update-index --split-index --add file echo "dirty worktree" >file # ... wait ... git update-index --add other-file This case fails due to the racy split index problem. In the second 'git update-index' prepare_to_write_split_index() decides, among other things, which cache entries stored only in the shared index should be replaced in the new split index. Alas, this function never looks out for racily clean cache entries, and since the file's stat data in the worktree hasn't changed since the shared index was written, the entry won't be replaced in the new split index. Consequently, do_write_index() doesn't even get this racily clean cache entry, and can't smudge its stat data. Subsequent git commands will then see that the index has more recent mtime than the file and that the (not smudged) cached stat data still matches with the file in the worktree, and, ultimately, will erroneously consider the file clean. 6. Update the split index after unpack_trees() copied a racily clean cache entry from the shared index: echo "cached content" >file git update-index --split-index --add file echo "dirty worktree" >file # ... wait ... git read-tree -m HEAD This case fails due to the racy split index problem. This basically fails for the same reason as case #5 above, but there is one important difference, which warrants the dedicated test. While that second 'git update-index' in case #5 updates index_state in place, in this case 'git read-tree -m' calls unpack_trees(), which throws out the entire index, and constructs a new one from the (potentially updated) copies of the original's cache entries. Consequently, when prepare_to_write_split_index() gets to work on this reconstructed index, it takes a different code path than in case #5 when deciding which cache entries in the shared index should be replaced. The result is the same, though: the racily clean cache entry goes unnoticed, it isn't added to the split index with smudged stat data, and subsequent git commands will then erroneously consider the file clean. Note that in the last two 'test_expect_failure' cases I omitted the '#' (as in nr. of trial) from the tests' name on purpose for now, as it confuses 'prove' into thinking that those tests failed unexpectedly. [1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge branch 'nd/split-index', 2014-07-16). Signed-off-by: SZEDER Gábor --- t/t1701-racy-split-index.sh | 218 ++++++++++++++++++++++++++++++++++++ 1 file changed, 218 insertions(+) create mode 100755 t/t1701-racy-split-index.sh diff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh new file mode 100755 index 0000000000..ebde418d7e --- /dev/null +++ b/t/t1701-racy-split-index.sh @@ -0,0 +1,218 @@ +#!/bin/sh + +# This test can give false success if your machine is sufficiently +# slow or all trials happened to happen on second boundaries. + +test_description='racy split index' + +. ./test-lib.sh + +test_expect_success 'setup' ' + # Only split the index when the test explicitly says so. + sane_unset GIT_TEST_SPLIT_INDEX GIT_FSMONITOR_TEST && + git config splitIndex.maxPercentChange 100 && + + echo "cached content" >racy-file && + git add racy-file && + git commit -m initial && + + echo something >other-file && + # No raciness with this file. + test-tool chmtime =-20 other-file && + + echo "+cached content" >expect +' + +check_cached_diff () { + git diff-index --patch --cached $EMPTY_TREE racy-file >diff && + tail -1 diff >actual && + test_cmp expect actual +} + +trials="0 1 2 3 4" +for trial in $trials +do + test_expect_success "split the index while adding a racily clean file #$trial" ' + rm -f .git/index .git/sharedindex.* && + + # The next three commands must be run within the same + # second (so both writes to racy-file result in the same + # mtime) to create the interesting racy situation. + echo "cached content" >racy-file && + + # Update and split the index. The cache entry of + # racy-file will be stored only in the shared index. + git update-index --split-index --add racy-file && + + # File size must stay the same. + echo "dirty worktree" >racy-file && + + # Subsequent git commands should notice that racy-file + # and the split index have the same mtime, and check + # the content of the file to see if it is actually + # clean. + check_cached_diff + ' +done + +for trial in $trials +do + test_expect_success "add a racily clean file to an already split index #$trial" ' + rm -f .git/index .git/sharedindex.* && + + git update-index --split-index && + + # The next three commands must be run within the same + # second. + echo "cached content" >racy-file && + + # Update the split index. The cache entry of racy-file + # will be stored only in the split index. + git update-index --add racy-file && + + # File size must stay the same. + echo "dirty worktree" >racy-file && + + # Subsequent git commands should notice that racy-file + # and the split index have the same mtime, and check + # the content of the file to see if it is actually + # clean. + check_cached_diff + ' +done + +for trial in $trials +do + test_expect_success "split the index when the index contains a racily clean cache entry #$trial" ' + rm -f .git/index .git/sharedindex.* && + + # The next three commands must be run within the same + # second. + echo "cached content" >racy-file && + + git update-index --add racy-file && + + # File size must stay the same. + echo "dirty worktree" >racy-file && + + # Now wait a bit to ensure that the split index written + # below will get a more recent mtime than racy-file. + sleep 1 && + + # Update and split the index when the index contains + # the racily clean cache entry of racy-file. + # A corresponding replacement cache entry with smudged + # stat data should be added to the new split index. + git update-index --split-index --add other-file && + + # Subsequent git commands should notice the smudged + # stat data in the replacement cache entry and that it + # doesnt match with the file the worktree. + check_cached_diff + ' +done + +for trial in $trials +do + test_expect_success "update the split index when it contains a new racily clean cache entry #$trial" ' + rm -f .git/index .git/sharedindex.* && + + git update-index --split-index && + + # The next three commands must be run within the same + # second. + echo "cached content" >racy-file && + + # Update the split index. The cache entry of racy-file + # will be stored only in the split index. + git update-index --add racy-file && + + # File size must stay the same. + echo "dirty worktree" >racy-file && + + # Now wait a bit to ensure that the split index written + # below will get a more recent mtime than racy-file. + sleep 1 && + + # Update the split index when the racily clean cache + # entry of racy-file is only stored in the split index. + # An updated cache entry with smudged stat data should + # be added to the new split index. + git update-index --add other-file && + + # Subsequent git commands should notice the smudged + # stat data. + check_cached_diff + ' +done + +for trial in $trials +do + test_expect_failure "update the split index when a racily clean cache entry is stored only in the shared index $trial" ' + rm -f .git/index .git/sharedindex.* && + + # The next three commands must be run within the same + # second. + echo "cached content" >racy-file && + + # Update and split the index. The cache entry of + # racy-file will be stored only in the shared index. + git update-index --split-index --add racy-file && + + # File size must stay the same. + echo "dirty worktree" >racy-file && + + # Now wait a bit to ensure that the split index written + # below will get a more recent mtime than racy-file. + sleep 1 && + + # Update the split index when the racily clean cache + # entry of racy-file is only stored in the shared index. + # A corresponding replacement cache entry with smudged + # stat data should be added to the new split index. + # + # Alas, such a smudged replacement entry is not added! + git update-index --add other-file && + + # Subsequent git commands should notice the smudged + # stat data. + check_cached_diff + ' +done + +for trial in $trials +do + test_expect_failure "update the split index after unpack trees() copied a racily clean cache entry from the shared index $trial" ' + rm -f .git/index .git/sharedindex.* && + + # The next three commands must be run within the same + # second. + echo "cached content" >racy-file && + + # Update and split the index. The cache entry of + # racy-file will be stored only in the shared index. + git update-index --split-index --add racy-file && + + # File size must stay the same. + echo "dirty worktree" >racy-file && + + # Now wait a bit to ensure that the split index written + # below will get a more recent mtime than racy-file. + sleep 1 && + + # Update the split index after unpack_trees() copied the + # racily clean cache entry of racy-file from the shared + # index. A corresponding replacement cache entry + # with smudged stat data should be added to the new + # split index. + # + # Alas, such a smudged replacement entry is not added! + git read-tree -m HEAD && + + # Subsequent git commands should notice the smudged + # stat data. + check_cached_diff + ' +done + +test_done From patchwork Thu Sep 27 12:44:31 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?SZEDER_G=C3=A1bor?= X-Patchwork-Id: 10617969 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 71230175A for ; Thu, 27 Sep 2018 12:45:03 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 603FA2B425 for ; Thu, 27 Sep 2018 12:45:03 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 52A442B434; Thu, 27 Sep 2018 12:45:03 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8F6092B425 for ; Thu, 27 Sep 2018 12:45:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727462AbeI0TDH (ORCPT ); Thu, 27 Sep 2018 15:03:07 -0400 Received: from mail-wm1-f66.google.com ([209.85.128.66]:50186 "EHLO mail-wm1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727338AbeI0TDH (ORCPT ); Thu, 27 Sep 2018 15:03:07 -0400 Received: by mail-wm1-f66.google.com with SMTP id s12-v6so5784699wmc.0 for ; Thu, 27 Sep 2018 05:44:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=AzPSiOjg32LeNAhG1EFOWRaiq+eFto7o/uTrmmms1ks=; b=R5w/s6JfsvvbM1PPK13V6pfQ1bIJ0ywDTP97kRkYhP1c0i6B4XLizi5vLk+sgmTSaM kezXcnBMzcYNKtIikcpB+YYX1Tmn/dGqCo7W3xYYnxJUk5/kov7kMsV7oQKqCDFpTNtq nSLqTRQIFSVXb4dKR/rUOeIfa9d7msYvogVS3H12+iKL/SQh4lOukr8xaJMEfs+xF0sh AWpiiKZd2aPQqxZ3Uaghg5hv3HLhjvGJiazNFsWRIC8BZIpiTHWubw9KXW84jmmteD+Q 6iZ/u/coxVzoKIgpDPk9wvoDoc6dV/3mwO/rqYvh1Btrxc6K60FzDCgb+P6MLWVab87z nrSQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=AzPSiOjg32LeNAhG1EFOWRaiq+eFto7o/uTrmmms1ks=; b=nLEYE30HlAUWA+GPeqGGe4ctZPULv2dPgGxMxds7i6M7dcXP/KCBgHe1qWa7cb1rR/ LgcSVbJwso9HXRaHxWsSlhI/36v2bgkuoqRZ1dT0q7qmCVmF9nUjy3RgpBYYs0gW/HeV WuzwNoIbhhTkwMXgoSFOjKyNgZTq0s1TQ3aUEF+Al2YmV1k0Yf8xOA7ZgK0pQkzXj2Cw 7OHdKHPr7IcGgw2vGRPZj5kenx5khX3L4+Zxa02qs5fR11En4YQz8hK1UsK/PnFJRZHB 2SOvtG8GfnLT1DI91dGUPmkonrHvDa/1OPRCBCnUXCZCRej4GBR37Tcu5bBko+rx0tj0 IlfA== X-Gm-Message-State: ABuFfojsxMNUgfOEwGIKgSuoF9cupdRZptk0i6XvUHJIBlt/Zd/BBVU3 +lUxGpE8hvUZHY/myc1g+xLMvP1R X-Google-Smtp-Source: ACcGV60RtWsCwPKJhGgTBKoLQ+kx1mc32yCkL6vZzuJHI5jkNLpwv7ulS9lo9FGQM22NKZYDkwstZg== X-Received: by 2002:a1c:af0f:: with SMTP id y15-v6mr8207998wme.51.1538052298272; Thu, 27 Sep 2018 05:44:58 -0700 (PDT) Received: from localhost.localdomain (x4dbd8656.dyn.telefonica.de. [77.189.134.86]) by smtp.gmail.com with ESMTPSA id c8-v6sm1938543wrn.43.2018.09.27.05.44.56 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 27 Sep 2018 05:44:57 -0700 (PDT) From: =?utf-8?q?SZEDER_G=C3=A1bor?= To: git@vger.kernel.org Cc: Junio C Hamano , Duy Nguyen , Thomas Gummerer , =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBC?= =?utf-8?b?amFybWFzb24=?= , Paul-Sebastian Ungureanu , =?utf-8?q?SZED?= =?utf-8?q?ER_G=C3=A1bor?= Subject: [PATCH v2 2/5] t1700-split-index: date back files to avoid racy situations Date: Thu, 27 Sep 2018 14:44:31 +0200 Message-Id: <20180927124434.30835-3-szeder.dev@gmail.com> X-Mailer: git-send-email 2.19.0.361.gafc87ffe72 In-Reply-To: <20180927124434.30835-1-szeder.dev@gmail.com> References: <20180927124434.30835-1-szeder.dev@gmail.com> MIME-Version: 1.0 Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 't1700-split-index.sh' checks that the index was split correctly under various circumstances and that all the different ways to turn the split index feature on and off work correctly. To do so, most of its tests use 'test-tool dump-split-index' to see which files have their cache entries in the split index. All these tests assume that all cache entries are written to the shared index (called "base" throughout these tests) when a new shared index is created. This is an implementation detail: most git commands (basically all except 'git update-index') don't care or know at all about split index or whether a cache entry is stored in the split or shared index. As demonstrated in the previous patch, refreshing a split index is prone to a variant of the classic racy git issue. The next patch will fix this issue, but while doing so it will also slightly change this behaviour: only cache entries with mtime in the past will be written only to the newly created shared index, but racily clean cache entries will be written to the new split index (with smudged stat data). While this upcoming change won't at all affect any git commands, it will violate the above mentioned assumption of 't1700's tests. Since these tests create or modify files and create or refresh the split index in rapid succession, there are plenty of racily clean cache entries to be dealt with, which will then be written to the new split indexes, and, ultimately, will cause several tests in 't1700' to fail. Let's prepare 't1700-split-index.sh' for this upcoming change and modify its tests to avoid racily clean files by backdating the mtime of any file modifications (and since a lot of tests create or modify files, encapsulate it into a helper function). Signed-off-by: SZEDER Gábor --- t/t1700-split-index.sh | 49 ++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh index be22398a85..c8acbc50d0 100755 --- a/t/t1700-split-index.sh +++ b/t/t1700-split-index.sh @@ -8,6 +8,13 @@ test_description='split index mode tests' sane_unset GIT_TEST_SPLIT_INDEX sane_unset GIT_FSMONITOR_TEST +# Create a file named as $1 with content read from stdin. +# Set the file's mtime to a few seconds in the past to avoid racy situations. +create_non_racy_file () { + cat >"$1" && + test-tool chmtime =-5 "$1" +} + test_expect_success 'enable split index' ' git config splitIndex.maxPercentChange 100 && git update-index --split-index && @@ -31,7 +38,7 @@ test_expect_success 'enable split index' ' ' test_expect_success 'add one file' ' - : >one && + create_non_racy_file one && git update-index --add one && git ls-files --stage >ls-files.actual && cat >ls-files.expect <<-EOF && @@ -83,7 +90,7 @@ test_expect_success 'enable split index again, "one" now belongs to base index"' ' test_expect_success 'modify original file, base index untouched' ' - echo modified >one && + echo modified | create_non_racy_file one && git update-index one && git ls-files --stage >ls-files.actual && cat >ls-files.expect <<-EOF && @@ -102,7 +109,7 @@ test_expect_success 'modify original file, base index untouched' ' ' test_expect_success 'add another file, which stays index' ' - : >two && + create_non_racy_file two && git update-index --add two && git ls-files --stage >ls-files.actual && cat >ls-files.expect <<-EOF && @@ -155,7 +162,7 @@ test_expect_success 'remove file in base index' ' ' test_expect_success 'add original file back' ' - : >one && + create_non_racy_file one && git update-index --add one && git ls-files --stage >ls-files.actual && cat >ls-files.expect <<-EOF && @@ -174,7 +181,7 @@ test_expect_success 'add original file back' ' ' test_expect_success 'add new file' ' - : >two && + create_non_racy_file two && git update-index --add two && git ls-files --stage >actual && cat >expect <<-EOF && @@ -218,7 +225,7 @@ test_expect_success 'rev-parse --shared-index-path' ' test_expect_success 'set core.splitIndex config variable to true' ' git config core.splitIndex true && - : >three && + create_non_racy_file three && git update-index --add three && git ls-files --stage >ls-files.actual && cat >ls-files.expect <<-EOF && @@ -253,9 +260,9 @@ test_expect_success 'set core.splitIndex config variable to false' ' test_cmp expect actual ' -test_expect_success 'set core.splitIndex config variable to true' ' +test_expect_success 'set core.splitIndex config variable back to true' ' git config core.splitIndex true && - : >three && + create_non_racy_file three && git update-index --add three && BASE=$(test-tool dump-split-index .git/index | grep "^base") && test-tool dump-split-index .git/index | sed "/^own/d" >actual && @@ -265,7 +272,7 @@ test_expect_success 'set core.splitIndex config variable to true' ' deletions: EOF test_cmp expect actual && - : >four && + create_non_racy_file four && git update-index --add four && test-tool dump-split-index .git/index | sed "/^own/d" >actual && cat >expect <<-EOF && @@ -279,7 +286,7 @@ test_expect_success 'set core.splitIndex config variable to true' ' test_expect_success 'check behavior with splitIndex.maxPercentChange unset' ' git config --unset splitIndex.maxPercentChange && - : >five && + create_non_racy_file five && git update-index --add five && BASE=$(test-tool dump-split-index .git/index | grep "^base") && test-tool dump-split-index .git/index | sed "/^own/d" >actual && @@ -289,7 +296,7 @@ test_expect_success 'check behavior with splitIndex.maxPercentChange unset' ' deletions: EOF test_cmp expect actual && - : >six && + create_non_racy_file six && git update-index --add six && test-tool dump-split-index .git/index | sed "/^own/d" >actual && cat >expect <<-EOF && @@ -303,7 +310,7 @@ test_expect_success 'check behavior with splitIndex.maxPercentChange unset' ' test_expect_success 'check splitIndex.maxPercentChange set to 0' ' git config splitIndex.maxPercentChange 0 && - : >seven && + create_non_racy_file seven && git update-index --add seven && BASE=$(test-tool dump-split-index .git/index | grep "^base") && test-tool dump-split-index .git/index | sed "/^own/d" >actual && @@ -313,7 +320,7 @@ test_expect_success 'check splitIndex.maxPercentChange set to 0' ' deletions: EOF test_cmp expect actual && - : >eight && + create_non_racy_file eight && git update-index --add eight && BASE=$(test-tool dump-split-index .git/index | grep "^base") && test-tool dump-split-index .git/index | sed "/^own/d" >actual && @@ -326,17 +333,17 @@ test_expect_success 'check splitIndex.maxPercentChange set to 0' ' ' test_expect_success 'shared index files expire after 2 weeks by default' ' - : >ten && + create_non_racy_file ten && git update-index --add ten && test $(ls .git/sharedindex.* | wc -l) -gt 2 && just_under_2_weeks_ago=$((5-14*86400)) && test-tool chmtime =$just_under_2_weeks_ago .git/sharedindex.* && - : >eleven && + create_non_racy_file eleven && git update-index --add eleven && test $(ls .git/sharedindex.* | wc -l) -gt 2 && just_over_2_weeks_ago=$((-1-14*86400)) && test-tool chmtime =$just_over_2_weeks_ago .git/sharedindex.* && - : >twelve && + create_non_racy_file twelve && git update-index --add twelve && test $(ls .git/sharedindex.* | wc -l) -le 2 ' @@ -344,12 +351,12 @@ test_expect_success 'shared index files expire after 2 weeks by default' ' test_expect_success 'check splitIndex.sharedIndexExpire set to 16 days' ' git config splitIndex.sharedIndexExpire "16.days.ago" && test-tool chmtime =$just_over_2_weeks_ago .git/sharedindex.* && - : >thirteen && + create_non_racy_file thirteen && git update-index --add thirteen && test $(ls .git/sharedindex.* | wc -l) -gt 2 && just_over_16_days_ago=$((-1-16*86400)) && test-tool chmtime =$just_over_16_days_ago .git/sharedindex.* && - : >fourteen && + create_non_racy_file fourteen && git update-index --add fourteen && test $(ls .git/sharedindex.* | wc -l) -le 2 ' @@ -358,13 +365,13 @@ test_expect_success 'check splitIndex.sharedIndexExpire set to "never" and "now" git config splitIndex.sharedIndexExpire never && just_10_years_ago=$((-365*10*86400)) && test-tool chmtime =$just_10_years_ago .git/sharedindex.* && - : >fifteen && + create_non_racy_file fifteen && git update-index --add fifteen && test $(ls .git/sharedindex.* | wc -l) -gt 2 && git config splitIndex.sharedIndexExpire now && just_1_second_ago=-1 && test-tool chmtime =$just_1_second_ago .git/sharedindex.* && - : >sixteen && + create_non_racy_file sixteen && git update-index --add sixteen && test $(ls .git/sharedindex.* | wc -l) -le 2 ' @@ -379,7 +386,7 @@ do # Create one new shared index file git config core.sharedrepository "$mode" && git config core.splitIndex true && - : >one && + create_non_racy_file one && git update-index --add one && echo "$modebits" >expect && test_modebits .git/index >actual && From patchwork Thu Sep 27 12:44:32 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?SZEDER_G=C3=A1bor?= X-Patchwork-Id: 10617971 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id E3B191759 for ; Thu, 27 Sep 2018 12:45:09 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D15DD2B425 for ; Thu, 27 Sep 2018 12:45:09 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C4F2C2B432; Thu, 27 Sep 2018 12:45:09 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 67DE42B425 for ; Thu, 27 Sep 2018 12:45:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727471AbeI0TDO (ORCPT ); Thu, 27 Sep 2018 15:03:14 -0400 Received: from mail-wm1-f67.google.com ([209.85.128.67]:34609 "EHLO mail-wm1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727338AbeI0TDO (ORCPT ); Thu, 27 Sep 2018 15:03:14 -0400 Received: by mail-wm1-f67.google.com with SMTP id y3-v6so3286502wma.1 for ; Thu, 27 Sep 2018 05:45:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=PZ74sbVMbbs/4wm6yqrs3zBlQsbEwGazIiQrHu1Oq+E=; b=QOSUlzkXK74hcRxHLlyK+uZHY0SsfbpAWmbaACBycyE8VJxoYxYWzwe0L8E0cBwREd Mpx8NdfwwPNCvCvm1JVJCpWUJ/ZujD7hxCxdm8Z2HN2DpTqwKRiUrkWDVj3udrjmspBm HcCwqIovnr8WvNpdDXDeYiaCtHUm8mW9eClmiXPBpSH/6lmCna3RuaIrsJcr20uRwPX3 3sIbyY8p898f4TDYVS4VqaIXkyCH4tD7vAKNJml1+zMmHkab8aw/3PDpsHmh8GEtVy8J BLqH2KsxsVrL17B95vDT3nVGq3WUfbG+27mfB8nM2ev+1+iUfgUysP/Z0VoQVQ+92Qeb jSRg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=PZ74sbVMbbs/4wm6yqrs3zBlQsbEwGazIiQrHu1Oq+E=; b=CB4B1smZ11JBsSn1qQQfBvHG2ArWd7Hx0FRmunNyJiQbc6pCWJuUwDyYFWVn1uOs4r cz9zL+gETqV/MiAtuvz7vheMa+rmkDjYQH1163RfZkvWQXzgiACMUBnKPF4wMxyqs0ci v2BppnrVqOc37Xz5HvtOn7+aAnzVKxAfDG6k9R1fSB7st8RlhKD2X2SfFOyNdKpwYKs1 G3vfVpJshFo5T737GTV3fBCeM44cJDHhJcJ4HA9P2Fp0AM+Q8jr3h5mwu1NUl5iiNTu3 0DjahSr1XUWoVMHuFj9q3haBdKSFrlcvl4oYTMOn8R1PZ+x6Amc/FMJGU5DN1nkc8GQM GYNg== X-Gm-Message-State: ABuFfogznEy/eZ9diVssxs2nOWUmf+hAGBtPv+iuCIuLivnH1DrGThuL hQFwrLKepJ1GYJ4AXZhM7XV1Lq91 X-Google-Smtp-Source: ACcGV61G0F3nVUhI09xnNWaBQAQXFvoHC+rpggQcc6RLrUuwhUEytJUlk7bRsob27c4Uq8xMD6zL7Q== X-Received: by 2002:a1c:be06:: with SMTP id o6-v6mr7377155wmf.65.1538052304717; Thu, 27 Sep 2018 05:45:04 -0700 (PDT) Received: from localhost.localdomain (x4dbd8656.dyn.telefonica.de. [77.189.134.86]) by smtp.gmail.com with ESMTPSA id c8-v6sm1938543wrn.43.2018.09.27.05.45.03 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 27 Sep 2018 05:45:03 -0700 (PDT) From: =?utf-8?q?SZEDER_G=C3=A1bor?= To: git@vger.kernel.org Cc: Junio C Hamano , Duy Nguyen , Thomas Gummerer , =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBC?= =?utf-8?b?amFybWFzb24=?= , Paul-Sebastian Ungureanu , =?utf-8?q?SZED?= =?utf-8?q?ER_G=C3=A1bor?= Subject: [PATCH v2 3/5] split-index: count the number of deleted entries Date: Thu, 27 Sep 2018 14:44:32 +0200 Message-Id: <20180927124434.30835-4-szeder.dev@gmail.com> X-Mailer: git-send-email 2.19.0.361.gafc87ffe72 In-Reply-To: <20180927124434.30835-1-szeder.dev@gmail.com> References: <20180927124434.30835-1-szeder.dev@gmail.com> MIME-Version: 1.0 Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 'struct split_index' contains the field 'nr_deletions', whose name with the 'nr_' prefix suggests that it contains the number of deleted cache entries. However, barring its initialization to 0, this field is only ever set to 1, indicating that there is at least one deleted entry, but not the number of deleted entries. Luckily, this doesn't cause any issues (other than confusing the reader, that is), because the only place reading this field uses it in the same sense, i.e.: 'if (si->nr_deletions)'. To avoid confusion, we could either rename this field to something like 'has_deletions' to make its name match its role, or make it a counter of deleted cache entries to match its name. Let's make it a counter, to keep it in sync with the related field 'nr_replacements', which does contain the number of replaced cache entries. This will also give developers debugging the split index code easy access to the number of deleted cache entries. Signed-off-by: SZEDER Gábor --- split-index.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/split-index.c b/split-index.c index 84f067e10d..548272ec33 100644 --- a/split-index.c +++ b/split-index.c @@ -111,7 +111,7 @@ static void mark_entry_for_delete(size_t pos, void *data) die("position for delete %d exceeds base index size %d", (int)pos, istate->cache_nr); istate->cache[pos]->ce_flags |= CE_REMOVE; - istate->split_index->nr_deletions = 1; + istate->split_index->nr_deletions++; } static void replace_entry(size_t pos, void *data) From patchwork Thu Sep 27 12:44:33 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?SZEDER_G=C3=A1bor?= X-Patchwork-Id: 10617973 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 5BEB21759 for ; Thu, 27 Sep 2018 12:45:18 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4A5222B425 for ; Thu, 27 Sep 2018 12:45:18 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 3E5EB2B432; Thu, 27 Sep 2018 12:45:18 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0B8692B425 for ; Thu, 27 Sep 2018 12:45:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727485AbeI0TDW (ORCPT ); Thu, 27 Sep 2018 15:03:22 -0400 Received: from mail-wm1-f54.google.com ([209.85.128.54]:39715 "EHLO mail-wm1-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727249AbeI0TDW (ORCPT ); Thu, 27 Sep 2018 15:03:22 -0400 Received: by mail-wm1-f54.google.com with SMTP id q8-v6so5846296wmq.4 for ; Thu, 27 Sep 2018 05:45:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=CZWRuDfZW7XVIVqcgh5co9NhPtsov5RBr/R9jWUiA/c=; b=X1M2iuPufKHhF2NalmcU3fx0+erFTV3ZldMxNZEmHhOFK7LPCetMTSs+Bl1mIAWBZZ DfR5nm+/AO314RoMt+uFfd1UF1GLhWc+ikIZoFMKlFWW+UjP4pmlEP0z6+KwZkrUtXHw 42fb3JoGu+3muflxoZuYNG+7NCvAuijEv+njtyl7fTwhEsax1pxq5jjfe14u7ITHKSp0 ogc9AQuuT1dUOyULIcyNN0L01WgsteuzhD8UCz20UEEsI6h4D8QIF5J5yvRY6YMnDKc0 VSBQ/WyHCMHpNNmLWlpEJHeDYWO0bQC3hvBJEWOOg51LXxhWsreIV+WYvO3oyCLIBSga NkbQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=CZWRuDfZW7XVIVqcgh5co9NhPtsov5RBr/R9jWUiA/c=; b=rVPIrCRCm7+h2CZ4j3rHUvQyIONJMr8+sXcyvUUfv52qPP9PR64Bf9hE/Rq59D5CgF AcvGtICheGXuqxBsqTwRZlw+WwV7D744aLgHzo2OHfoniKm2NnEgY/MSKhrGjf0OKGQq ugSukcOC5d8Se0LWYiI3TneynJToSK/oUABcuwC6xMld37WMq0R20tymYtj8n+cmsIkJ 1Ro3k5StXhRKDqetxYDWI4yWMcTXaCc2fVpbi1h06dHSQzTxMUnzIBqiH68ie3VJKBm3 SvB8urkCRqDAXpwb8/teqY3+as1glyD/BfVweMM2/cghbbj3BQYU7gGKzyAtH3YJeCoA ojkQ== X-Gm-Message-State: ABuFfogQXHwehbqpAfk27Gwnlg7u2A40tLJ8vb3Cr2SjMRCMjsEI/j+O uAidvhEL1Dslh8qBlfgl2SRQG62j X-Google-Smtp-Source: ACcGV61Cb951ecARnrbyhx4HFUfypwHixCR3wsihxoLTnYDw3TbJ7Nh4Smrhn57cmFu56B1N8TN7yQ== X-Received: by 2002:a1c:6782:: with SMTP id b124-v6mr7609911wmc.30.1538052312148; Thu, 27 Sep 2018 05:45:12 -0700 (PDT) Received: from localhost.localdomain (x4dbd8656.dyn.telefonica.de. [77.189.134.86]) by smtp.gmail.com with ESMTPSA id c8-v6sm1938543wrn.43.2018.09.27.05.45.10 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 27 Sep 2018 05:45:11 -0700 (PDT) From: =?utf-8?q?SZEDER_G=C3=A1bor?= To: git@vger.kernel.org Cc: Junio C Hamano , Duy Nguyen , Thomas Gummerer , =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBC?= =?utf-8?b?amFybWFzb24=?= , Paul-Sebastian Ungureanu , =?utf-8?q?SZED?= =?utf-8?q?ER_G=C3=A1bor?= Subject: [PATCH v2 4/5] split-index: don't compare stat data of entries already marked for split index Date: Thu, 27 Sep 2018 14:44:33 +0200 Message-Id: <20180927124434.30835-5-szeder.dev@gmail.com> X-Mailer: git-send-email 2.19.0.361.gafc87ffe72 In-Reply-To: <20180927124434.30835-1-szeder.dev@gmail.com> References: <20180927124434.30835-1-szeder.dev@gmail.com> MIME-Version: 1.0 Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP When unpack_trees() constructs a new index, it copies cache entries from the original index [1]. prepare_to_write_split_index() has to deal with this, and it has a dedicated code path for copied entries that are present in the shared index, where it compares the cached data in the corresponding copied and original entries. If the cached data matches, then they are considered the same; if it differs, then the copied entry will be marked for inclusion as a replacement entry in the just about to be written split index by setting the CE_UPDATE_IN_BASE flag. However, a cache entry already has its CE_UPDATE_IN_BASE flag set upon reading the split index, if the entry already has a replacement entry there, or upon refreshing the cached stat data, if the corresponding file was modified. The state of this flag is then preserved when unpack_trees() copies a cache entry from the shared index. So modify prepare_to_write_split_index() to check the copied cache entries' CE_UPDATE_IN_BASE flag first, and skip the thorough comparison of cached data if the flag is already set. Note that comparing the cached data in copied and original entries in the shared index might actually be entirely unnecessary. In theory all code paths refreshing the cached stat data of an entry in the shared index should set the CE_UPDATE_IN_BASE flag in that entry, and unpack_trees() should preserve this flag when copying cache entries. This means that the cached data is only ever changed if the CE_UPDATE_IN_BASE flag is set as well. Our test suite seems to confirm this: instrumenting the conditions in question and running the test suite repeatedly with 'GIT_TEST_SPLIT_INDEX=yes' showed that the cached data in a copied entry differs from the data in the shared entry only if its CE_UPDATE_IN_BASE flag is indeed set. In practice, however, our test suite doesn't have 100% coverage, GIT_TEST_SPLIT_INDEX is inherently random, and I certainly can't claim to possess complete understanding of what goes on in unpack_trees()... Therefore I kept the comparison of the cached data when CE_UPDATE_IN_BASE is not set, just in case that an unnoticed or future code path were to accidentally miss setting this flag upon refreshing the cached stat data or unpack_trees() were to drop this flag while copying a cache entry. [1] Note that when unpack_trees() constructs the new index and decides that a cache entry should now refer to different content than what was recorded in the original index (e.g. 'git read-tree -m HEAD^'), then that can't really be considered a copy of the original, but rather the creation of a new entry. Notably and pertinent to the split index feature, such a new entry doesn't have a reference to the original's shared index entry anymore, i.e. its 'index' field is set to 0. Consequently, such an entry is treated by prepare_to_write_split_index() as an entry not present in the shared index and it will be added to the new split index, while the original entry will be marked as deleted, and neither the above discussion nor the changes in this patch apply to them. Signed-off-by: SZEDER Gábor --- split-index.c | 79 ++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 62 insertions(+), 17 deletions(-) diff --git a/split-index.c b/split-index.c index 548272ec33..7d8799f6b7 100644 --- a/split-index.c +++ b/split-index.c @@ -204,19 +204,34 @@ void prepare_to_write_split_index(struct index_state *istate) * that are not marked with either CE_MATCHED or * CE_UPDATE_IN_BASE. If istate->cache[i] is a * duplicate, deduplicate it. */ for (i = 0; i < istate->cache_nr; i++) { struct cache_entry *base; - /* namelen is checked separately */ - const unsigned int ondisk_flags = - CE_STAGEMASK | CE_VALID | CE_EXTENDED_FLAGS; - unsigned int ce_flags, base_flags, ret; ce = istate->cache[i]; - if (!ce->index) + if (!ce->index) { + /* + * During simple update index operations this + * is a cache entry that is not present in + * the shared index. It will be added to the + * split index. + * + * However, it might also represent a file + * that already has a cache entry in the + * shared index, but a new index has just + * been constructed by unpack_trees(), and + * this entry now refers to different content + * than what was recorded in the original + * index, e.g. during 'read-tree -m HEAD^' or + * 'checkout HEAD^'. In this case the + * original entry in the shared index will be + * marked as deleted, and this entry will be + * added to the split index. + */ continue; + } if (ce->index > si->base->cache_nr) { ce->index = 0; continue; } ce->ce_flags |= CE_MATCHED; /* or "shared" */ base = si->base->cache[ce->index - 1]; @@ -224,24 +239,54 @@ void prepare_to_write_split_index(struct index_state *istate) continue; if (ce->ce_namelen != base->ce_namelen || strcmp(ce->name, base->name)) { ce->index = 0; continue; } - ce_flags = ce->ce_flags; - base_flags = base->ce_flags; - /* only on-disk flags matter */ - ce->ce_flags &= ondisk_flags; - base->ce_flags &= ondisk_flags; - ret = memcmp(&ce->ce_stat_data, &base->ce_stat_data, - offsetof(struct cache_entry, name) - - offsetof(struct cache_entry, ce_stat_data)); - ce->ce_flags = ce_flags; - base->ce_flags = base_flags; - if (ret) - ce->ce_flags |= CE_UPDATE_IN_BASE; + /* + * This is the copy of a cache entry that is present + * in the shared index, created by unpack_trees() + * while it constructed a new index. + */ + if (ce->ce_flags & CE_UPDATE_IN_BASE) { + /* + * Already marked for inclusion in the split + * index, either because the corresponding + * file was modified and the cached stat data + * was refreshed, or because the original + * entry already had a replacement entry in + * the split index. + * Nothing to do. + */ + } else { + /* + * Thoroughly compare the cached data to see + * whether it should be marked for inclusion + * in the split index. + * + * This comparison might be unnecessary, as + * code paths modifying the cached data do + * set CE_UPDATE_IN_BASE as well. + */ + const unsigned int ondisk_flags = + CE_STAGEMASK | CE_VALID | + CE_EXTENDED_FLAGS; + unsigned int ce_flags, base_flags, ret; + ce_flags = ce->ce_flags; + base_flags = base->ce_flags; + /* only on-disk flags matter */ + ce->ce_flags &= ondisk_flags; + base->ce_flags &= ondisk_flags; + ret = memcmp(&ce->ce_stat_data, &base->ce_stat_data, + offsetof(struct cache_entry, name) - + offsetof(struct cache_entry, ce_stat_data)); + ce->ce_flags = ce_flags; + base->ce_flags = base_flags; + if (ret) + ce->ce_flags |= CE_UPDATE_IN_BASE; + } discard_cache_entry(base); si->base->cache[ce->index - 1] = ce; } for (i = 0; i < si->base->cache_nr; i++) { ce = si->base->cache[i]; if ((ce->ce_flags & CE_REMOVE) || From patchwork Thu Sep 27 12:44:34 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?SZEDER_G=C3=A1bor?= X-Patchwork-Id: 10617975 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 045D41759 for ; Thu, 27 Sep 2018 12:45:25 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E8B602B429 for ; Thu, 27 Sep 2018 12:45:24 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D16222B43F; Thu, 27 Sep 2018 12:45:24 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A1EC12B429 for ; Thu, 27 Sep 2018 12:45:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727357AbeI0TD2 (ORCPT ); Thu, 27 Sep 2018 15:03:28 -0400 Received: from mail-wm1-f65.google.com ([209.85.128.65]:53485 "EHLO mail-wm1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727249AbeI0TD2 (ORCPT ); Thu, 27 Sep 2018 15:03:28 -0400 Received: by mail-wm1-f65.google.com with SMTP id b19-v6so5779504wme.3 for ; Thu, 27 Sep 2018 05:45:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=ky+UCLJZHI+j79EXGIR4aU8Lw9HDhcRJltlqC86ck3g=; b=bn+ey3WccZ1VNCZxqB7tV2yrxVgcgTjY4n2+4sD3rIQ1hpZs1HwuYj4vfEqtlXzpL/ 71IPYkKHI6jKHcXsduUbBcQPuGDiqjOo1XFkhEEdjUDjYdnZQGsMl2EHjdrFvC0KU3nl ISMMcJ4H3R+ImEvUv8A61smwYnJvG0SvKliUl6mqfNV5C3xLBUhzXZg59iVhR5zoPa0g 9410arhEDKA+fRDwdPFk1tlTerMS9CudcF49Q0h508MmJxqUI9iWAICfUrETPceaVm8L IkWQHShoPo3FYFsFAltdXTpWo9IsH37tAdWLl2nWPPO1s+lBnLme64YhtLlI4avEbGOf 2qWQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=ky+UCLJZHI+j79EXGIR4aU8Lw9HDhcRJltlqC86ck3g=; b=FzT5AJVWtxtbGBiNHyrJpDvSRt2LKakUtNtCfWxL0msib9TtbmKoONaAqB/QQN/hIh uYjKdCRUC53QOUJ+O/HNFyBylMrkr9ghiR3slldT3hdHF2Xb5k0XplOdEVdO6TtwtJzA aKQUF1Q4zDNGieBL5Q+A1mM4xW/g+ACVwrBUwId1XeoWngt/DNNvEkudpYv6CZGozPE5 kZ6ppLGT8u3XbnWevU8nzfQEeMJUcvp7I9Zg/sOAbC84g+T8APYBfsV7MRdO9JSIgaQt 2Y9YZzyCROtWFLWgRHt5g+Q6L+8lSxWeN9mazGLNRVRxtxKrvamgkGE1z4bITiPaIa+e S1UQ== X-Gm-Message-State: ABuFfohnnr1YCEtu9MY6D64/FOs0MR7nohdjsAZTRJc+X5x4Cn8+v0R9 FTBUqBpWhSANKzraSHp6fb92VcFl X-Google-Smtp-Source: ACcGV60KMWkxtDlyRNgAxi7jEfYXAO44J2pHPueGCefcFEpIfEzZ1yCRuDAQFxw/CZX0GQV770uxtw== X-Received: by 2002:a1c:e4c3:: with SMTP id b186-v6mr7574099wmh.116.1538052319430; Thu, 27 Sep 2018 05:45:19 -0700 (PDT) Received: from localhost.localdomain (x4dbd8656.dyn.telefonica.de. [77.189.134.86]) by smtp.gmail.com with ESMTPSA id c8-v6sm1938543wrn.43.2018.09.27.05.45.18 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 27 Sep 2018 05:45:18 -0700 (PDT) From: =?utf-8?q?SZEDER_G=C3=A1bor?= To: git@vger.kernel.org Cc: Junio C Hamano , Duy Nguyen , Thomas Gummerer , =?utf-8?b?w4Z2YXIgQXJuZmrDtnLDsCBC?= =?utf-8?b?amFybWFzb24=?= , Paul-Sebastian Ungureanu , =?utf-8?q?SZED?= =?utf-8?q?ER_G=C3=A1bor?= Subject: [PATCH v2 5/5] split-index: smudge and add racily clean cache entries to split index Date: Thu, 27 Sep 2018 14:44:34 +0200 Message-Id: <20180927124434.30835-6-szeder.dev@gmail.com> X-Mailer: git-send-email 2.19.0.361.gafc87ffe72 In-Reply-To: <20180927124434.30835-1-szeder.dev@gmail.com> References: <20180927124434.30835-1-szeder.dev@gmail.com> MIME-Version: 1.0 Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Ever since the split index feature was introduced [1], refreshing a split index is prone to a variant of the classic racy git problem. Consider the following sequence of commands updating the split index when the shared index contains a racily clean cache entry, i.e. an entry whose cached stat data matches with the corresponding file in the worktree and the cached mtime matches that of the index: echo "cached content" >file git update-index --split-index --add file echo "dirty worktree" >file # size stays the same! # ... wait ... git update-index --add other-file Normally, when a non-split index is updated, then do_write_index() (the function responsible for writing all kinds of indexes, "regular", split, and shared) recognizes racily clean cache entries, and writes them with smudged stat data, i.e. with file size set to 0. When subsequent git commands read the index, they will notice that the smudged stat data doesn't match with the file in the worktree, and then go on to check the file's content and notice its dirtiness. In the above example, however, in the second 'git update-index' prepare_to_write_split_index() decides which cache entries stored only in the shared index should be replaced in the new split index. Alas, this function never looks out for racily clean cache entries, and since the file's stat data in the worktree hasn't changed since the shared index was written, it won't be replaced in the new split index. Consequently, do_write_index() doesn't even get this racily clean cache entry, and can't smudge its stat data. Subsequent git commands will then see that the index has more recent mtime than the file and that the (not smudged) cached stat data still matches with the file in the worktree, and, ultimately, will erroneously consider the file clean. Modify prepare_to_write_split_index() to recognize racily clean cache entries, and mark them to be added to the split index. Note that there are two places where it should check raciness: first those cache entries that are only stored in the shared index, and then those that have been copied by unpack_trees() from the shared index while it constructed a new index. This way do_write_index() will get these racily clean cache entries as well, and will then write them with smudged stat data to the new split index. Note that after this change if the index is split when it contains a racily clean cache entry, then a smudged cache entry will be written both to the new shared and to the new split indexes. This doesn't affect regular git commands: as far as they are concerned this is just an entry in the split index replacing an outdated entry in the shared index. It did affect a few tests in 't1700-split-index.sh', though, because they actually check which entries are stored in the split index; a previous patch in this series has already made the necessary adjustments in 't1700'. And racily clean cache entries and index splitting are rare enough to not worry about the resulting duplicated smudged cache entries, and the additional complexity required to prevent them is not worth it. Several tests failed occasionally when the test suite was run with 'GIT_TEST_SPLIT_INDEX=yes'. Here are those that I managed to trace back to this racy split index problem, starting with those failing more frequently, with a link to a failing Travis CI build job for each. The highlighted line [2] shows when the racy file was written, which is not always in the failing test but in a preceeding setup test. t3903-stash.sh: https://travis-ci.org/git/git/jobs/385542084#L5858 t4024-diff-optimize-common.sh: https://travis-ci.org/git/git/jobs/386531969#L3174 t4015-diff-whitespace.sh: https://travis-ci.org/git/git/jobs/360797600#L8215 t2200-add-update.sh: https://travis-ci.org/git/git/jobs/382543426#L3051 t0090-cache-tree.sh: https://travis-ci.org/git/git/jobs/416583010#L3679 There might be others, e.g. perhaps 't1000-read-tree-m-3way.sh' and others using 'lib-read-tree-m-3way.sh', but I couldn't confirm yet. [1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge branch 'nd/split-index', 2014-07-16). [2] Note that those highlighted lines are in the 'after failure' fold, and your browser might unhelpfully fold it up before you could take a good look. Signed-off-by: SZEDER Gábor --- cache.h | 2 ++ read-cache.c | 2 +- split-index.c | 42 ++++++++++++++++++++++++++++++++++++- t/t1701-racy-split-index.sh | 8 ++----- 4 files changed, 46 insertions(+), 8 deletions(-) diff --git a/cache.h b/cache.h index 4d014541ab..3f419b6c79 100644 --- a/cache.h +++ b/cache.h @@ -781,6 +781,8 @@ extern void *read_blob_data_from_index(const struct index_state *, const char *, #define CE_MATCH_REFRESH 0x10 /* don't refresh_fsmonitor state or do stat comparison even if CE_FSMONITOR_VALID is true */ #define CE_MATCH_IGNORE_FSMONITOR 0X20 +extern int is_racy_timestamp(const struct index_state *istate, + const struct cache_entry *ce); extern int ie_match_stat(struct index_state *, const struct cache_entry *, struct stat *, unsigned int); extern int ie_modified(struct index_state *, const struct cache_entry *, struct stat *, unsigned int); diff --git a/read-cache.c b/read-cache.c index 7b1354d759..8f644f68b4 100644 --- a/read-cache.c +++ b/read-cache.c @@ -337,7 +337,7 @@ static int is_racy_stat(const struct index_state *istate, ); } -static int is_racy_timestamp(const struct index_state *istate, +int is_racy_timestamp(const struct index_state *istate, const struct cache_entry *ce) { return (!S_ISGITLINK(ce->ce_mode) && diff --git a/split-index.c b/split-index.c index 7d8799f6b7..d989b27286 100644 --- a/split-index.c +++ b/split-index.c @@ -235,8 +235,39 @@ void prepare_to_write_split_index(struct index_state *istate) } ce->ce_flags |= CE_MATCHED; /* or "shared" */ base = si->base->cache[ce->index - 1]; - if (ce == base) + if (ce == base) { + /* The entry is present in the shared index. */ + if (ce->ce_flags & CE_UPDATE_IN_BASE) { + /* + * Already marked for inclusion in + * the split index, either because + * the corresponding file was + * modified and the cached stat data + * was refreshed, or because there + * is already a replacement entry in + * the split index. + * Nothing more to do here. + */ + } else if (!ce_uptodate(ce) && + is_racy_timestamp(istate, ce)) { + /* + * A racily clean cache entry stored + * only in the shared index: it must + * be added to the split index, so + * the subsequent do_write_index() + * can smudge its stat data. + */ + ce->ce_flags |= CE_UPDATE_IN_BASE; + } else { + /* + * The entry is only present in the + * shared index and it was not + * refreshed. + * Just leave it there. + */ + } continue; + } if (ce->ce_namelen != base->ce_namelen || strcmp(ce->name, base->name)) { ce->index = 0; @@ -257,6 +288,15 @@ void prepare_to_write_split_index(struct index_state *istate) * the split index. * Nothing to do. */ + } else if (!ce_uptodate(ce) && + is_racy_timestamp(istate, ce)) { + /* + * A copy of a racily clean cache entry from + * the shared index. It must be added to + * the split index, so the subsequent + * do_write_index() can smudge its stat data. + */ + ce->ce_flags |= CE_UPDATE_IN_BASE; } else { /* * Thoroughly compare the cached data to see diff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh index ebde418d7e..7f16f5f7a3 100755 --- a/t/t1701-racy-split-index.sh +++ b/t/t1701-racy-split-index.sh @@ -148,7 +148,7 @@ done for trial in $trials do - test_expect_failure "update the split index when a racily clean cache entry is stored only in the shared index $trial" ' + test_expect_success "update the split index when a racily clean cache entry is stored only in the shared index #$trial" ' rm -f .git/index .git/sharedindex.* && # The next three commands must be run within the same @@ -170,8 +170,6 @@ do # entry of racy-file is only stored in the shared index. # A corresponding replacement cache entry with smudged # stat data should be added to the new split index. - # - # Alas, such a smudged replacement entry is not added! git update-index --add other-file && # Subsequent git commands should notice the smudged @@ -182,7 +180,7 @@ done for trial in $trials do - test_expect_failure "update the split index after unpack trees() copied a racily clean cache entry from the shared index $trial" ' + test_expect_success "update the split index after unpack trees() copied a racily clean cache entry from the shared index #$trial" ' rm -f .git/index .git/sharedindex.* && # The next three commands must be run within the same @@ -205,8 +203,6 @@ do # index. A corresponding replacement cache entry # with smudged stat data should be added to the new # split index. - # - # Alas, such a smudged replacement entry is not added! git read-tree -m HEAD && # Subsequent git commands should notice the smudged