| Message ID | a763a7d15b8e92dec61c1fa328ecb647b6f61682.1626901619.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Sparse Index: Integrate with 'git add' | expand |
On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Derrick Stolee <dstolee@microsoft.com> > > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > t/t1092-sparse-checkout-compatibility.sh | 37 ++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh > index 91e30d6ec22..a3c01d588d8 100755 > --- a/t/t1092-sparse-checkout-compatibility.sh > +++ b/t/t1092-sparse-checkout-compatibility.sh > @@ -114,6 +114,16 @@ test_expect_success 'setup' ' > git add . && > git commit -m "file to dir" && > > + for side in left right > + do > + git checkout -b merge-$side base && > + echo $side >>deep/deeper2/a && > + echo $side >>folder1/a && > + echo $side >>folder2/a && > + git add . && > + git commit -m "$side" || return 1 Why is this "|| return 1" here? It looks like there are a number of other cases of this in the file too, which I must have overlooked previously, because I don't understand any of them. > + done && > + > git checkout -b deepest base && > echo "updated deepest" >deep/deeper1/deepest/a && > git commit -a -m "update deepest" && > @@ -482,6 +492,33 @@ test_expect_success 'merge' ' > test_all_match git rev-parse HEAD^{tree} > ' > > +test_expect_success 'merge with conflict outside cone' ' > + init_repos && > + > + test_all_match git checkout -b merge-tip merge-left && > + test_all_match git status --porcelain=v2 && > + test_all_match test_must_fail git merge -m merge merge-right && > + test_all_match git status --porcelain=v2 && > + > + # resolve the conflict in different ways: > + # 1. revert to the base > + test_all_match git checkout base -- deep/deeper2/a && > + test_all_match git status --porcelain=v2 && > + > + # 2. add the file with conflict markers > + test_all_match git add folder1/a && > + test_all_match git status --porcelain=v2 && > + > + # 3. rename the file to another sparse filename But...that doesn't resolve the conflict. Shouldn't this be titled "accept the conflict & rename the file elsewhere"? > + run_on_all mv folder2/a folder2/z && > + test_all_match git add folder2 && 'mv' rather than 'git mv', then followed by 'git add'? Any reason for this order rather than git add followed by git mv? Also, if you really do want to move first, did you use mv instead of "git mv" due to the latter's shortcoming of only operating on stage 0? (https://lore.kernel.org/git/CABPp-BGJdwpwhQUp4Wa4bKBp4hQFB9OM3N1FXH7SzY0mvLDa7Q@mail.gmail.com/) Regardless of order, though, I still think mv or add should require a --force to rename or add a file outside the sparsity paths given the deferred negative surprises for users around such files. (Or come up with a solid way to remove those surprises.) > + test_all_match git status --porcelain=v2 && > + > + test_all_match git merge --continue && > + test_all_match git status --porcelain=v2 && > + test_all_match git rev-parse HEAD^{tree} > +' > + > test_expect_success 'merge with outside renames' ' > init_repos &&
On Fri, Jul 23, 2021 at 1:34 PM Elijah Newren <newren@gmail.com> wrote: > On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > + for side in left right > > + do > > + git checkout -b merge-$side base && > > + echo $side >>deep/deeper2/a && > > + echo $side >>folder1/a && > > + echo $side >>folder2/a && > > + git add . && > > + git commit -m "$side" || return 1 > > Why is this "|| return 1" here? > > It looks like there are a number of other cases of this in the file > too, which I must have overlooked previously, because I don't > understand any of them. A shell for-loop won't automatically terminate just because some command in its body fails. Instead it will run to completion and return the status of the last command of the last iteration, which may not be the iteration which failed, thus a failure can be hidden. Therefore, we need to proactively stop the loop iteration _and_ ensure that the return status of the loop itself reflects the failure, which we do by `|| return 1`. (If this loop was inside a subshell, we'd use `|| exit 1` instead.)
On Fri, Jul 23, 2021 at 10:44 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Fri, Jul 23, 2021 at 1:34 PM Elijah Newren <newren@gmail.com> wrote: > > On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget > > <gitgitgadget@gmail.com> wrote: > > > + for side in left right > > > + do > > > + git checkout -b merge-$side base && > > > + echo $side >>deep/deeper2/a && > > > + echo $side >>folder1/a && > > > + echo $side >>folder2/a && > > > + git add . && > > > + git commit -m "$side" || return 1 > > > > Why is this "|| return 1" here? > > > > It looks like there are a number of other cases of this in the file > > too, which I must have overlooked previously, because I don't > > understand any of them. > > A shell for-loop won't automatically terminate just because some > command in its body fails. Instead it will run to completion and > return the status of the last command of the last iteration, which may > not be the iteration which failed, thus a failure can be hidden. > Therefore, we need to proactively stop the loop iteration _and_ ensure > that the return status of the loop itself reflects the failure, which > we do by `|| return 1`. (If this loop was inside a subshell, we'd use > `|| exit 1` instead.) Ah, thanks for the explanation.
On 7/23/2021 1:34 PM, Elijah Newren wrote: > On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> >> From: Derrick Stolee <dstolee@microsoft.com> ... >> + # 3. rename the file to another sparse filename > > But...that doesn't resolve the conflict. Shouldn't this be titled > "accept the conflict & rename the file elsewhere"? Sure. I'm less focused on the content of the file and more the steps the user might have taken to resolve the conflict. >> + run_on_all mv folder2/a folder2/z && >> + test_all_match git add folder2 && > > 'mv' rather than 'git mv', then followed by 'git add'? Any reason for > this order rather than git add followed by git mv? I'm trying to mimic that a user might realize that a filename might need to be renamed (say, because a naming convention changed that is causing the conflict) and I don't expect users to use 'git mv' to do this action. > Also, if you really do want to move first, did you use mv instead of > "git mv" due to the latter's shortcoming of only operating on stage 0? > (https://lore.kernel.org/git/CABPp-BGJdwpwhQUp4Wa4bKBp4hQFB9OM3N1FXH7SzY0mvLDa7Q@mail.gmail.com/) 'git mv' had not occurred to me as a thing to do in this case. I'm focused on ensuring that 'git add' works as expected to update the index in response to filesystem changes. > Regardless of order, though, I still think mv or add should require a > --force to rename or add a file outside the sparsity paths given the > deferred negative surprises for users around such files. (Or come up > with a solid way to remove those surprises.) --force is focused on _ignored_ files. I imagine that it could be repurposed to allow entries outside of the sparse-checkout definition, but we would want to be careful for users who are adding the entire directory, not just the individual files, as they might _also_ get any ignored files that exist in that directory. That might justify creating a new option instead. Further, the error message reported when adding something outside of the sparse cone should probably mention whatever option exists as a way for users to bypass this limitation. I'll collect my thoughts (in response to your detailed thoughts shared on my cover letter) and start a new thread about hardening this behavior. I've got an internal ticket tracking this, and I want to wrap my head around all of the interesting commands (add, mv, rm, update-index?) and create a full recommendation to bring as an RFC. Of course, if someone else wants to create this clear vision in the meantime I will not complain. Thanks, -Stolee
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 91e30d6ec22..a3c01d588d8 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -114,6 +114,16 @@ test_expect_success 'setup' ' git add . && git commit -m "file to dir" && + for side in left right + do + git checkout -b merge-$side base && + echo $side >>deep/deeper2/a && + echo $side >>folder1/a && + echo $side >>folder2/a && + git add . && + git commit -m "$side" || return 1 + done && + git checkout -b deepest base && echo "updated deepest" >deep/deeper1/deepest/a && git commit -a -m "update deepest" && @@ -482,6 +492,33 @@ test_expect_success 'merge' ' test_all_match git rev-parse HEAD^{tree} ' +test_expect_success 'merge with conflict outside cone' ' + init_repos && + + test_all_match git checkout -b merge-tip merge-left && + test_all_match git status --porcelain=v2 && + test_all_match test_must_fail git merge -m merge merge-right && + test_all_match git status --porcelain=v2 && + + # resolve the conflict in different ways: + # 1. revert to the base + test_all_match git checkout base -- deep/deeper2/a && + test_all_match git status --porcelain=v2 && + + # 2. add the file with conflict markers + test_all_match git add folder1/a && + test_all_match git status --porcelain=v2 && + + # 3. rename the file to another sparse filename + run_on_all mv folder2/a folder2/z && + test_all_match git add folder2 && + test_all_match git status --porcelain=v2 && + + test_all_match git merge --continue && + test_all_match git status --porcelain=v2 && + test_all_match git rev-parse HEAD^{tree} +' + test_expect_success 'merge with outside renames' ' init_repos &&