Re: [PATCH v2 4/4] lib-submodule-update: pass OVERWRITING_FAIL

On Wed, May 20, 2020 at 08:24:18PM -0400, Denton Liu wrote:

I think there are really two separate issues being addressed by this patch.

One is using "test_must_fail" with a helper function. IMHO this is not something we should go too far in trying to deal with. It's mostly a style issue about how test_must_fail is meant to be used, and there's no real downside if we occasionally use it for a non-git command.

I don't mind cleaning up spots where it's obviously misused, but in this case we're introducing a lot of new complexity to handle the "sometimes" nature of this call. And I'm not sure it's worth it on its own.

Now this second concern I think is much more interesting, because it impacts the test results. And it's really not even about test_must_fail in particular, but is a general problem with checking failure in any compound operation. We may expect the early parts to succeed, but the later parts to fail, and we want to tell the difference. And that's true whether you're using "!", test_must_fail, etc.

You solve it here by passing OVERWRITING_FAIL down into the callback functions. And that does work. But I think it may be easier to understand if we invert the responsibility. Let the outer caller specify two callbacks: one for setup/prep that must succeed, and one for a single operation where we might expect success or failure.

The changes in lib-submodule-update look something like:

  @@ -326,7 +327,8 @@ test_submodule_switch_common() {
                  (
                          cd submodule_update &&
                          git branch -t add_sub1 origin/add_sub1 &&
  -                       $command add_sub1 &&
  +                       $prep add_sub1 &&
  +                       $command &&
                          test_superproject_content origin/add_sub1 &&
                          test_dir_is_empty sub1 &&
                          git submodule update --init --recursive &&

And in the caller we can simplify greatly:

  diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
  index f916729a12..e3ae7c89f1 100755
  --- a/t/t5572-pull-submodule.sh
  +++ b/t/t5572-pull-submodule.sh
  @@ -11,36 +11,13 @@ reset_branch_to_HEAD () {
   	git branch --set-upstream-to="origin/$1" "$1"
   }
   
  -git_pull () {
  -	reset_branch_to_HEAD "$1" &&
  -	git pull
  -}
  -
   # pulls without conflicts
  -test_submodule_switch "git_pull"
  -
  -git_pull_ff () {
  -	reset_branch_to_HEAD "$1" &&
  -	git pull --ff
  -}
  -
  -test_submodule_switch "git_pull_ff"
  -
  -git_pull_ff_only () {
  -	reset_branch_to_HEAD "$1" &&
  -	git pull --ff-only
  -}
  -
  -test_submodule_switch "git_pull_ff_only"
  -
  -git_pull_noff () {
  -	reset_branch_to_HEAD "$1" &&
  -	git pull --no-ff
  -}
  -
  +test_submodule_switch "reset_branch_to_HEAD" "git pull"
  +test_submodule_switch "reset_branch_to_HEAD" "git pull -ff"
  +test_submodule_switch "reset_branch_to_HEAD" "git pull --ff-only"
   KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1
   KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
  -test_submodule_switch "git_pull_noff"
  +test_submodule_switch "reset_branch_to_HEAD" "git pull --no-ff"
   
   test_expect_success 'pull --recurse-submodule setup' '
   	test_create_repo child &&

I suspect this approach involves touching more lines than yours (it has to add $prep everywhere $command is used). But IMHO the result is much simpler to understand, because there's no spooky-action-at-a-distance from magic environment variables.

A more complete patch is below, which is enough to get t5572 passing. I think it would need the other test_submodule_switch() function updated, and other scripts would need to adapt to the 2-arg style.

-Peff