From patchwork Tue Dec 1 22:25:16 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Elijah Newren X-Patchwork-Id: 11944211 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6DCE2C71156 for ; Tue, 1 Dec 2020 22:26:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 11E8E20870 for ; Tue, 1 Dec 2020 22:26:11 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="cJmz1KA0" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726815AbgLAW0K (ORCPT ); Tue, 1 Dec 2020 17:26:10 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35500 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725916AbgLAW0J (ORCPT ); Tue, 1 Dec 2020 17:26:09 -0500 Received: from mail-wr1-x444.google.com (mail-wr1-x444.google.com [IPv6:2a00:1450:4864:20::444]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A08A2C0613D4 for ; Tue, 1 Dec 2020 14:25:23 -0800 (PST) Received: by mail-wr1-x444.google.com with SMTP id k14so5363369wrn.1 for ; Tue, 01 Dec 2020 14:25:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=message-id:in-reply-to:references:from:date:subject:fcc :content-transfer-encoding:mime-version:to:cc; bh=A+pnDjNzYC12lo4A9P68XXv7/ynTxmaaizYaLQ8ZpLM=; b=cJmz1KA0jCsIPC/qozPhox2GsXE3eNJveZFs66FoNK9BQh9zV2kWyPKqscIvSfAs57 x5rW65i53X1vwAu2aFYR1coG5tLiAXA/yoUNUOHqFrmfUgGD/GpQ27qOiaFhqgdXb+tR kwUGKtd3+fBfBxdMiWLB+q1jCWqIf0CCM2AV9cVf33aB+/VaZ4r88D4opSwSYGoacnx5 3NvmDGp3U97Uhpimr0qp0b02ctjdLsjX+wWwxgRqBDhEN+jv776h6a+xX6CjynNYh2a3 GLs6SRbSuxipyPXwojqBLXd4zrj23i+O369sMHPzGcHVjadFVIU0uiesU/jAHWsLT7YW nbgg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:in-reply-to:references:from:date :subject:fcc:content-transfer-encoding:mime-version:to:cc; bh=A+pnDjNzYC12lo4A9P68XXv7/ynTxmaaizYaLQ8ZpLM=; b=uO8eNYilfgZsyMVcphmUjhaseYfKGiPUcAUdeSPHHc8v5+oXUba9w8lcCQsbh2QcW+ ggnaewkaVMMT3KLoElz6q+i8VDJWY/SOBXk9N5xjlg/LRlPihPzuOx7VQZ6AhtUshiKh pyE2umxS/JqTWAXP71rLLmBPZ1pUEQ6eUPdmV3ugoWKYIsIAPurpPXwuOJ2fPDoZ+FC8 QdqM1cUD9arpslN2ggZ59WfJu6Say01bIIm9+v1oTVsKj2fjn4vyCHdhKJB2VKRB24Lv fEVbKuP5WgnhLisBGQHZA8XbVe4nPjETczQjn/M4pf+1twSUO7fZ2PTqFvZLokm76StT w9qg== X-Gm-Message-State: AOAM530Goh52VW166qA9mwXES7SXKRH9wAbP5JON7DOUhBS3P4OLIuHh bpyZ0EJoO/M4+bK9WE+CUF8FQs4Ca08= X-Google-Smtp-Source: ABdhPJxQ8t5fXtKQfOi1YcktF8w6BFsYA+Ke52tCJyKXPcN+i6BGo4ZeZhZOyuobtjlc6dmsrZpp0Q== X-Received: by 2002:adf:f9c4:: with SMTP id w4mr6659345wrr.64.1606861522074; Tue, 01 Dec 2020 14:25:22 -0800 (PST) Received: from [127.0.0.1] ([13.74.141.28]) by smtp.gmail.com with ESMTPSA id t15sm1655290wmn.19.2020.12.01.14.25.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 01 Dec 2020 14:25:21 -0800 (PST) Message-Id: <2155bbfe205588387208690abf8071d462c69d64.1606861519.git.gitgitgadget@gmail.com> In-Reply-To: References: Date: Tue, 01 Dec 2020 22:25:16 +0000 Subject: [PATCH v2 1/3] t7012: add a testcase demonstrating stash apply bugs in sparse checkouts Fcc: Sent MIME-Version: 1.0 To: git@vger.kernel.org Cc: matheus.bernardino@usp.br, dstolee@microsoft.com, Elijah Newren , chris.torek@gmail.com, Elijah Newren , Elijah Newren Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org From: Elijah Newren From: Elijah Newren Applying stashes in sparse-checkouts, particularly when the patterns used to define the sparseness have changed between when the stash was created and when it is applied, has a number of bugs. The primary problem is that stashes are sometimes only partially applied. In most such cases, it does so silently without any warning or error being displayed and with 0 exit status. There are, however, a few cases when non-translated error messages are shown and the stash application aborts early. The first is when there are files present despite the SKIP_WORKTREE bit being set, in which case the error message shown is: error: Entry 'PATHNAME' not uptodate. Cannot merge. The other situation is when a stash contains new files to add to the working tree; in this case, the code aborts early but still has the stash partially applied, and shows the following error message: error: NEWFILE: does not exist and --remove not passed fatal: Unable to process path NEWFILE Add a test that can trigger all three of these problems. Have it carefully check that the working copy and SKIP_WORKTREE bits are as expected after the stash application. The test is currently marked as expected to fail, but subsequent commits will implement the fixes and toggle the expectation. Signed-off-by: Elijah Newren --- t/t7012-skip-worktree-writing.sh | 88 ++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh index 7476781979..a184ee97fb 100755 --- a/t/t7012-skip-worktree-writing.sh +++ b/t/t7012-skip-worktree-writing.sh @@ -149,6 +149,94 @@ test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' ' --diff-filter=D -- keep-me.t ' +test_expect_failure 'stash restore in sparse checkout' ' + test_create_repo stash-restore && + ( + cd stash-restore && + + mkdir subdir && + echo A >subdir/A && + echo untouched >untouched && + echo removeme >removeme && + echo modified >modified && + git add . && + git commit -m Initial && + + echo AA >>subdir/A && + echo addme >addme && + echo tweaked >>modified && + rm removeme && + git add addme && + + git stash push && + + git sparse-checkout set subdir && + + # Ensure after sparse-checkout we only have expected files + cat >expect <<-EOF && + S modified + S removeme + H subdir/A + S untouched + EOF + git ls-files -t >actual && + test_cmp expect actual && + + test_path_is_missing addme && + test_path_is_missing modified && + test_path_is_missing removeme && + test_path_is_file subdir/A && + test_path_is_missing untouched && + + # Put a file in the working directory in the way + echo in the way >modified && + git stash apply && + + # Ensure stash vivifies modifies paths... + cat >expect <<-EOF && + H addme + H modified + H removeme + H subdir/A + S untouched + EOF + git ls-files -t >actual && + test_cmp expect actual && + + # ...and that the paths show up in status as changed... + cat >expect <<-EOF && + A addme + M modified + D removeme + M subdir/A + ?? actual + ?? expect + ?? modified.stash.XXXXXX + EOF + git status --porcelain | \ + sed -e s/stash......./stash.XXXXXX/ >actual && + test_cmp expect actual && + + # ...and that working directory reflects the files correctly + test_path_is_file addme && + test_path_is_file modified && + test_path_is_missing removeme && + test_path_is_file subdir/A && + test_path_is_missing untouched && + + # ...including that we have the expected "modified" file... + cat >expect <<-EOF && + modified + tweaked + EOF + test_cmp expect modified && + + # ...and that the other "modified" file is still present... + echo in the way >expect && + test_cmp expect modified.stash.* + ) +' + #TODO test_expect_failure 'git-apply adds file' false #TODO test_expect_failure 'git-apply updates file' false #TODO test_expect_failure 'git-apply removes file' false From patchwork Tue Dec 1 22:25:17 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Elijah Newren X-Patchwork-Id: 11944207 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 18D8EC71155 for ; Tue, 1 Dec 2020 22:26:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 93D3320870 for ; Tue, 1 Dec 2020 22:26:06 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="NdP/bftN" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726788AbgLAW0F (ORCPT ); Tue, 1 Dec 2020 17:26:05 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35504 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725916AbgLAW0F (ORCPT ); Tue, 1 Dec 2020 17:26:05 -0500 Received: from mail-wm1-x341.google.com (mail-wm1-x341.google.com [IPv6:2a00:1450:4864:20::341]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A4344C0613D6 for ; Tue, 1 Dec 2020 14:25:24 -0800 (PST) Received: by mail-wm1-x341.google.com with SMTP id v14so8164065wml.1 for ; Tue, 01 Dec 2020 14:25:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=message-id:in-reply-to:references:from:date:subject:fcc :content-transfer-encoding:mime-version:to:cc; bh=Hod6uIiSDkvsyl5su55lYSYEY+V0tKlVE7mSCnMpxVA=; b=NdP/bftNFC4UCPuswy4UlJlZmk7SWdMXUTOefMpeHELSOYj148rICQ7NeUIbcT9C5k LBCRn9XWd4AsedvedKjCxvjvkltWQb2J7QrvgB7800Tse6whD1hqBcJlUNZCHXjRpS6w 53AjS8KbKa0zqOEKWcovzkVxKvMSJma9ROE4y06YOCSpISXdDMhLCzZaKAFUUSvgQAVD AMukc8N3JYtmdp7MyqjX9hykwWl3Pv/hjusqCR1c18IBJxVO8jDGyQFbIBN438hYBS1t yC8IgRDlGIEQ+vkoTxhYoP8IVHwY39NEX3c7iiPYxh7uqZBqpzT9GvdQ1M56HANv8xCm hoYQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:in-reply-to:references:from:date :subject:fcc:content-transfer-encoding:mime-version:to:cc; bh=Hod6uIiSDkvsyl5su55lYSYEY+V0tKlVE7mSCnMpxVA=; b=dLR4lDb1YxDx3CcVWDxbwFK+7NJpElUdKUXdRcfUQZ0+aa0Fm3oRfhraf/tjvpZzmg 9FnkVgXBCKJ0FPe918DLJmTkBc8NWPbYVgxpu6LvB1p0SJdzFeeg0FKzPUhYvzM4Sbzb L7Nx84JgxW5Phd4Dk1diP/K4KlvSao0ST/z0bB+N5kAg3bFOPloYO+CCsBeqb3V8XGXv WvUb7kT1fU1KX9wkXZKJkaZKg+eKnSWFsHpbgjdzAzS9t5Oz5qFi5XBv4JhXcmfpkh2m 5m959FKelG7w6bZ/jBEBl6kYKemDMa8EhrVgefV3mwhLrLregns7CvuY44NkBxPoj7aQ xmJw== X-Gm-Message-State: AOAM531AT7YazVAjD8Y0E8cxs5arSqG8UFHFLkTiZ6ydaemYnItYik4k W31UvgRSV2wTdjVA8Pcc4ONNFQRcS5s= X-Google-Smtp-Source: ABdhPJyCG63IoQrBDqRm1oCCqugujn+mFCSpq5/FCi6V2wjFgd7iyV+KwF7++/Rr0GFjyZn0K6bfEg== X-Received: by 2002:a7b:c5c6:: with SMTP id n6mr4716653wmk.131.1606861523074; Tue, 01 Dec 2020 14:25:23 -0800 (PST) Received: from [127.0.0.1] ([13.74.141.28]) by smtp.gmail.com with ESMTPSA id x4sm1495103wrv.81.2020.12.01.14.25.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 01 Dec 2020 14:25:22 -0800 (PST) Message-Id: <1fa263cf3c3d1b0c20ad89e6454a7b903a07f193.1606861519.git.gitgitgadget@gmail.com> In-Reply-To: References: Date: Tue, 01 Dec 2020 22:25:17 +0000 Subject: [PATCH v2 2/3] stash: remove unnecessary process forking Fcc: Sent MIME-Version: 1.0 To: git@vger.kernel.org Cc: matheus.bernardino@usp.br, dstolee@microsoft.com, Elijah Newren , chris.torek@gmail.com, Elijah Newren , Elijah Newren Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org From: Elijah Newren From: Elijah Newren When stash was converted from shell to a builtin, it merely transliterated the forking of various git commands from shell to a C program that would fork the same commands. Some of those were converted over to actual library calls, but much of the pipeline-of-commands design still remains. Fix some of this by replacing the portion corresponding to git diff-index --cached --name-only --diff-filter=A $CTREE >"$a" git read-tree --reset $CTREE git update-index --add --stdin <"$a" rm -f "$a" into a library function that does the same thing. (The read-tree --reset was already partially converted over to a library call, but as an independent piece.) Note here that this came after a merge operation was performed. The merge machinery always stages anything that cleanly merges, and the above code only runs if there are no conflicts. Its purpose is to make it so that when there are no conflicts, all the changes from the stash are unstaged. However, that causes brand new files from the stash to become untracked, so the code above first saves those files off and then re-adds them afterwards. We replace the whole series of commands with a simple function that will unstage files that are not newly added. This doesn't fix any bugs in the usage of these commands, it simply matches the existing behavior but makes it into a single atomic operation that we can then operate on as a whole. A subsequent commit will take advantage of this to fix issues with these commands in sparse-checkouts. This conversion incidentally fixes t3906.1, because the separate update-index process would die with the following error messages: error: uninitialized_sub: is a directory - add files inside instead fatal: Unable to process path uninitialized_sub The unstaging of the directory as a submodule meant it was no longer tracked, and thus as an uninitialized directory it could not be added back using `git update-index --add`, thus resulting in this error and early abort. Most of the submodule tests in 3906 continue to fail after this change, this change was just enough to push the first of those tests to success. Signed-off-by: Elijah Newren --- builtin/stash.c | 119 ++++++++++++++++++++++---------------- t/lib-submodule-update.sh | 16 ++--- 2 files changed, 78 insertions(+), 57 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index 3f811f3050..a5465b565a 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -325,35 +325,6 @@ static void add_diff_to_buf(struct diff_queue_struct *q, } } -static int get_newly_staged(struct strbuf *out, struct object_id *c_tree) -{ - struct child_process cp = CHILD_PROCESS_INIT; - const char *c_tree_hex = oid_to_hex(c_tree); - - /* - * diff-index is very similar to diff-tree above, and should be - * converted together with update_index. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "diff-index", "--cached", "--name-only", - "--diff-filter=A", NULL); - strvec_push(&cp.args, c_tree_hex); - return pipe_command(&cp, NULL, 0, out, 0, NULL, 0); -} - -static int update_index(struct strbuf *out) -{ - struct child_process cp = CHILD_PROCESS_INIT; - - /* - * Update-index is very complicated and may need to have a public - * function exposed in order to remove this forking. - */ - cp.git_cmd = 1; - strvec_pushl(&cp.args, "update-index", "--add", "--stdin", NULL); - return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0); -} - static int restore_untracked(struct object_id *u_tree) { int res; @@ -385,6 +356,75 @@ static int restore_untracked(struct object_id *u_tree) return res; } +static void unstage_changes_unless_new(struct object_id *orig_tree) +{ + /* + * When we enter this function, there has been a clean merge of + * relevant trees, and the merge logic always stages whatever merges + * cleanly. We want to unstage those changes, unless it corresponds + * to a file that didn't exist as of orig_tree. + */ + + struct diff_options diff_opts; + struct lock_file lock = LOCK_INIT; + int i; + + /* + * Step 1: get a difference between orig_tree (which corresponding + * to the index before a merge was run) and the current index + * (reflecting the changes brought in by the merge). + */ + diff_setup(&diff_opts); + diff_opts.flags.recursive = 1; + diff_opts.detect_rename = 0; + diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT; + diff_setup_done(&diff_opts); + + do_diff_cache(orig_tree, &diff_opts); + diffcore_std(&diff_opts); + + /* Iterate over the paths that changed due to the merge... */ + for (i = 0; i < diff_queued_diff.nr; i++) { + struct diff_filepair *p; + struct cache_entry *ce; + int pos; + + /* Look up the path's position in the current index. */ + p = diff_queued_diff.queue[i]; + pos = index_name_pos(&the_index, p->two->path, + strlen(p->two->path)); + + /* + * Step 2: "unstage" changes, as long as they are still tracked + */ + if (p->one->oid_valid) { + /* + * Path existed in orig_tree; restore index entry + * from that tree in order to "unstage" the changes. + */ + int option = ADD_CACHE_OK_TO_REPLACE; + if (pos < 0) + option = ADD_CACHE_OK_TO_ADD; + + ce = make_cache_entry(&the_index, + p->one->mode, + &p->one->oid, + p->one->path, + 0, 0); + add_index_entry(&the_index, ce, option); + } + } + diff_flush(&diff_opts); + + /* + * Step 3: write the new index to disk + */ + repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR); + if (write_locked_index(&the_index, &lock, + COMMIT_LOCK | SKIP_IF_UNCHANGED)) + die(_("Unable to write index.")); +} + static int do_apply_stash(const char *prefix, struct stash_info *info, int index, int quiet) { @@ -467,26 +507,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, if (reset_tree(&index_tree, 0, 0)) return -1; } else { - struct strbuf out = STRBUF_INIT; - - if (get_newly_staged(&out, &c_tree)) { - strbuf_release(&out); - return -1; - } - - if (reset_tree(&c_tree, 0, 1)) { - strbuf_release(&out); - return -1; - } - - ret = update_index(&out); - strbuf_release(&out); - if (ret) - return -1; - - /* read back the result of update_index() back from the disk */ - discard_cache(); - read_cache(); + unstage_changes_unless_new(&c_tree); } if (!quiet) { diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index 87a759149f..c6d7aab6df 100644 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -316,14 +316,7 @@ test_submodule_switch_common () { command="$1" ######################### Appearing submodule ######################### # Switching to a commit letting a submodule appear creates empty dir ... - if test "$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES" = 1 - then - # Restoring stash fails to restore submodule index entry - RESULT="failure" - else - RESULT="success" - fi - test_expect_$RESULT "$command: added submodule creates empty directory" ' + test_expect_success "$command: added submodule creates empty directory" ' prolog && reset_work_tree_to no_submodule && ( @@ -337,6 +330,13 @@ test_submodule_switch_common () { ) ' # ... and doesn't care if it already exists. + if test "$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES" = 1 + then + # Restoring stash fails to restore submodule index entry + RESULT="failure" + else + RESULT="success" + fi test_expect_$RESULT "$command: added submodule leaves existing empty directory alone" ' prolog && reset_work_tree_to no_submodule && From patchwork Tue Dec 1 22:25:18 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Elijah Newren X-Patchwork-Id: 11944209 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3020FC8300F for ; Tue, 1 Dec 2020 22:26:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C22D620870 for ; Tue, 1 Dec 2020 22:26:08 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="SkYb30by" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726805AbgLAW0H (ORCPT ); Tue, 1 Dec 2020 17:26:07 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35508 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725916AbgLAW0G (ORCPT ); Tue, 1 Dec 2020 17:26:06 -0500 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 B5DB6C0617A6 for ; Tue, 1 Dec 2020 14:25:25 -0800 (PST) Received: by mail-wr1-x42f.google.com with SMTP id l1so5341765wrb.9 for ; Tue, 01 Dec 2020 14:25:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=message-id:in-reply-to:references:from:date:subject:fcc :content-transfer-encoding:mime-version:to:cc; bh=rVc0WPPpwP15Cm/Gntip41uoVq7VtIBkPszoyu2g6is=; b=SkYb30byGT/OGaMqlKDcwu3t4BS6KiHuXTYvSAtwuTSxvhULB2yQYO4Nk9LNQq7ZZx 521Q9OQNUu5/ieE2bWWBcnAoglb2jmF087wNL1mQWsjfxJk3Zg6azIr6IXKUBjH5nMHU lVdz4LSGfm+Pz1sbkjx8X2OLs3hi4n4ocEpUXTpCMGz2Y4EBvoHCPVCH7Ccd9a+6w1vN ucl8lH/sfEBAYrNJ4kcCLDAVWpbKoIKpDBxoPtKLu6zM93kavn9XM8hYqFd+q7e4IQc4 RXujLcauY2iGrfLOs2zCOlFrFvtlqRIUcmNH5Q2sgatC2dVtjvQs+SJZG4dLusuMTSNU a4nQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:in-reply-to:references:from:date :subject:fcc:content-transfer-encoding:mime-version:to:cc; bh=rVc0WPPpwP15Cm/Gntip41uoVq7VtIBkPszoyu2g6is=; b=NZ9gPyIsBqWLcZ4d/3AKUcTyeNQWAs+IaXZumQxy8p/RiwIZtcUPqNQR+HCNb0YT0n pGli2UT1lFIinKMc2XIXlK3GQpLXjdHJtln8dMuXa6f8706e2QAmt1UuFuoj01s1S8Xs PO2EA5h7D5z+HVL+PJokgphKX6roo+lgG6a5+PLSM4UnCvzLzRCbjgZWmktH/9lhdsaN 1KBd9JrcUkKjYb6YyqPJJfpVLWKj05b2XXO8ONl8CmXpnThvIVt1I8aus1Z7Wf99Zwrx 6YVycfJ82DX3bu1Ou//xat33hVhhNEWzA9G+m9hKKYqAaDzWWjXfSnRrjot2ibNKV7n8 byWg== X-Gm-Message-State: AOAM533R78FLwSUBc2HgfaFjmgkev4SQPR5tP/5c2i9yDvt90UFaIGH0 ilxQ5qr6+evbUGAslvwhXQUY3wyVCxA= X-Google-Smtp-Source: ABdhPJwyab/5/jnRVENPlXSraPG9dnidaxTZpZ97hRdcUWUf3Q9bZcedsLbWDrM/DCaTW8HL4Symjg== X-Received: by 2002:a5d:5222:: with SMTP id i2mr6991075wra.247.1606861524107; Tue, 01 Dec 2020 14:25:24 -0800 (PST) Received: from [127.0.0.1] ([13.74.141.28]) by smtp.gmail.com with ESMTPSA id j6sm1394388wrq.38.2020.12.01.14.25.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 01 Dec 2020 14:25:23 -0800 (PST) Message-Id: In-Reply-To: References: Date: Tue, 01 Dec 2020 22:25:18 +0000 Subject: [PATCH v2 3/3] stash: fix stash application in sparse-checkouts Fcc: Sent MIME-Version: 1.0 To: git@vger.kernel.org Cc: matheus.bernardino@usp.br, dstolee@microsoft.com, Elijah Newren , chris.torek@gmail.com, Elijah Newren , Elijah Newren Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org From: Elijah Newren From: Elijah Newren sparse-checkouts are built on the patterns in the $GIT_DIR/info/sparse-checkout file, where commands have modified behavior for paths that do not match those patterns. The differences in behavior, as far as the bugs concerned here, fall into three different categories (with git subcommands that fall into each category listed): * commands that only look at files matching the patterns: * status * diff * clean * update-index * commands that remove files from the working tree that do not match the patterns, and restore files that do match them: * read-tree * switch * checkout * reset (--hard) * commands that omit writing files to the working tree that do not match the patterns, unless those files are not clean: * merge * rebase * cherry-pick * revert There are some caveats above, e.g. a plain `git diff` ignores files outside the sparsity patterns but will show diffs for paths outside the sparsity patterns when revision arguments are passed. (Technically, diff is treating the sparse paths as matching HEAD.) So, there is some internal inconsistency among these commands. There are also additional commands that should behave differently in the face of sparse-checkouts, as the sparse-checkout documentation alludes to, but the above is sufficient for me to explain how `git stash` is affected. What is relevant here is that logically 'stash' should behave like a merge; it three-way merges the changes the user had in progress at stash creation time, the HEAD at the time the stash was created, and the current HEAD, in order to get the stashed changes applied to the current branch. However, this simplistic view doesn't quite work in practice, because stash tweaks it a bit due to two factors: (1) flags like --keep-index and --include-untracked (why we used two different verbs, 'keep' and 'include', is a rant for another day) modify what should be staged at the end and include more things that should be quasi-merged, (2) stash generally wants changes to NOT be staged. It only provides exceptions when (a) some of the changes had conflicts and thus we want to use stages to denote the clean merges and higher order stages to mark the conflicts, or (b) if there is a brand new file we don't want it to become untracked. stash has traditionally gotten this special behavior by first doing a merge, and then when it's clean, applying a pipeline of commands to modify the result. This series of commands for unstaging-non-newly-added-files came from the following commands: git diff-index --cached --name-only --diff-filter=A $CTREE >"$a" git read-tree --reset $CTREE git update-index --add --stdin <"$a" rm -f "$a" Looking back at the different types of special sparsity handling listed at the beginning of this message, you may note that we have at least one of each type covered here: merge, diff-index, and read-tree. The weird mix-and-match led to 3 different bugs: (1) If a path merged cleanly and it didn't match the sparsity patterns, the merge backend would know to avoid writing it to the working tree and keep the SKIP_WORKTREE bit, simply only updating it in the index. Unfortunately, the subsequent commands would essentially undo the changes in the index and thus simply toss the changes altogether since there was nothing left in the working tree. This means the stash is only partially applied. (2) If a path existed in the worktree before `git stash apply` despite having the SKIP_WORKTREE bit set, then the `git read-tree --reset` would print an error message of the form error: Entry 'modified' not uptodate. Cannot merge. and cause stash to abort early. (3) If there was a brand new file added by the stash, then the diff-index command would save that pathname to the temporary file, the read-tree --reset would remove it from the index, and the update-index command would barf due to no such file being present in the working copy; it would print a message of the form: error: NEWFILE: does not exist and --remove not passed fatal: Unable to process path NEWFILE and then cause stash to abort early. Basically, the whole idea of unstage-unless-brand-new requires special care when you are dealing with a sparse-checkout. Fix these problems by applying the following simple rule: When we unstage files, if they have the SKIP_WORKTREE bit set, clear that bit and write the file out to the working directory. (*) If there's already a file present in the way, rename it first. This fixes all three problems in t7012.13 and allows us to mark it as passing. Signed-off-by: Elijah Newren --- builtin/stash.c | 50 ++++++++++++++++++++++++++++++-- t/t7012-skip-worktree-writing.sh | 2 +- 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index a5465b565a..84886e84e0 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -363,12 +363,23 @@ static void unstage_changes_unless_new(struct object_id *orig_tree) * relevant trees, and the merge logic always stages whatever merges * cleanly. We want to unstage those changes, unless it corresponds * to a file that didn't exist as of orig_tree. + * + * However, if any SKIP_WORKTREE path is modified relative to + * orig_tree, then we want to clear the SKIP_WORKTREE bit and write + * it to the worktree before unstaging. */ + struct checkout state = CHECKOUT_INIT; struct diff_options diff_opts; struct lock_file lock = LOCK_INIT; int i; + /* If any entries have skip_worktree set, we'll have to check 'em out */ + state.force = 1; + state.quiet = 1; + state.refresh_cache = 1; + state.istate = &the_index; + /* * Step 1: get a difference between orig_tree (which corresponding * to the index before a merge was run) and the current index @@ -395,7 +406,42 @@ static void unstage_changes_unless_new(struct object_id *orig_tree) strlen(p->two->path)); /* - * Step 2: "unstage" changes, as long as they are still tracked + * Step 2: Place changes in the working tree + * + * Stash is about restoring changes *to the working tree*. + * So if the merge successfully got a new version of some + * path, but left it out of the working tree, then clear the + * SKIP_WORKTREE bit and write it to the working tree. + */ + if (pos >= 0 && ce_skip_worktree(active_cache[pos])) { + struct stat st; + + ce = active_cache[pos]; + if (!lstat(ce->name, &st)) { + /* Conflicting path present; relocate it */ + struct strbuf new_path = STRBUF_INIT; + int fd; + + strbuf_addf(&new_path, + "%s.stash.XXXXXX", ce->name); + fd = xmkstemp(new_path.buf); + close(fd); + printf(_("WARNING: Untracked file in way of " + "tracked file! Renaming\n " + " %s -> %s\n" + " to make room.\n"), + ce->name, new_path.buf); + if (rename(ce->name, new_path.buf)) + die("Failed to move %s to %s\n", + ce->name, new_path.buf); + strbuf_release(&new_path); + } + checkout_entry(ce, &state, NULL, NULL); + ce->ce_flags &= ~CE_SKIP_WORKTREE; + } + + /* + * Step 3: "unstage" changes, as long as they are still tracked */ if (p->one->oid_valid) { /* @@ -417,7 +463,7 @@ static void unstage_changes_unless_new(struct object_id *orig_tree) diff_flush(&diff_opts); /* - * Step 3: write the new index to disk + * Step 4: write the new index to disk */ repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR); if (write_locked_index(&the_index, &lock, diff --git a/t/t7012-skip-worktree-writing.sh b/t/t7012-skip-worktree-writing.sh index a184ee97fb..e5c6a038fb 100755 --- a/t/t7012-skip-worktree-writing.sh +++ b/t/t7012-skip-worktree-writing.sh @@ -149,7 +149,7 @@ test_expect_success '--ignore-skip-worktree-entries leaves worktree alone' ' --diff-filter=D -- keep-me.t ' -test_expect_failure 'stash restore in sparse checkout' ' +test_expect_success 'stash restore in sparse checkout' ' test_create_repo stash-restore && ( cd stash-restore &&