Re: [PATCH] t5614: don't use subshells

Stefan Beller [off-list ref] writes:

 Unlike the prior patch we would not want this patch to fall through
 to origin/maint fast, but allow cooking?

I do not see anything that makes this treated differently from the
other fix.  The only difference in behaviour is that "lines" file is
now created at the root level of the trash repository's working tree
instead of tested clones', and I do not think any test depends on
the number of untracked paths in the trash working tree or tries to
make sure a file called "lines" is not in there.

Having said that, I wonder if we want further reduction of the
repetition.  Each test except "setup" in this script does an
identical thing with very small set of parameters:

    - make sure super_clone will be removed when done.
    - clone file://$pwd/. to super_clone but with different set of options.
    - check the commits in super_clone and super_clone/sub.

So, the above would ideally become something like

do_test 3 3 --recurse-submodules

where the helper would look like

do_test () {
	cnt_super=$1 cnt_sub=$2 &&
        shift 2 &&
        test_when_finished "rm -fr super_clone" &&
        git clone "$@" "file://$pwd/." super_clone &&
        git -C super_clone log --oneline >lines &&
        test_line_count = $cnt_super lines &&
        git -C super_clone/sub log --oneline >lines &&
        test_line_count = $cnt_sub lines
}

Would it rob too much flexibility from future tests to be added to
this script if we did it that way?

On Tue, Jun 21, 2016 at 12:18:29PM -0700, Junio C Hamano wrote:

Stefan Beller [off-list ref] writes:

quoted

 Unlike the prior patch we would not want this patch to fall through
 to origin/maint fast, but allow cooking?

I do not see anything that makes this treated differently from the
other fix.  The only difference in behaviour is that "lines" file is
now created at the root level of the trash repository's working tree
instead of tested clones', and I do not think any test depends on
the number of untracked paths in the trash working tree or tries to
make sure a file called "lines" is not in there.

I think it is only that the other patch is actually fixing something,
whereas this is cleanup. So the cost/benefit equation is different. I
agree neither is high-risk and a test cleanup is generally OK for maint
(the other is a serious-ish regression IMHO).

Having said that, I wonder if we want further reduction of the
repetition.  Each test except "setup" in this script does an
identical thing with very small set of parameters:

    - make sure super_clone will be removed when done.
    - clone file://$pwd/. to super_clone but with different set of options.
    - check the commits in super_clone and super_clone/sub.

So, the above would ideally become something like

do_test 3 3 --recurse-submodules

where the helper would look like

do_test () {
	cnt_super=$1 cnt_sub=$2 &&
        shift 2 &&
        test_when_finished "rm -fr super_clone" &&
        git clone "$@" "file://$pwd/." super_clone &&
        git -C super_clone log --oneline >lines &&
        test_line_count = $cnt_super lines &&
        git -C super_clone/sub log --oneline >lines &&
        test_line_count = $cnt_sub lines
}

Would it rob too much flexibility from future tests to be added to
this script if we did it that way?

I think that's an improvement, too. Even if we add further tests, they
don't have to follow the same format. I would give the function a better
name than "do_test" though. :P

-Peff

On Tue, Jun 21, 2016 at 12:38 PM, Jeff King [off-list ref] wrote:

On Tue, Jun 21, 2016 at 12:18:29PM -0700, Junio C Hamano wrote:

quoted

Stefan Beller [off-list ref] writes:

quoted

 Unlike the prior patch we would not want this patch to fall through
 to origin/maint fast, but allow cooking?

I do not see anything that makes this treated differently from the
other fix.  The only difference in behaviour is that "lines" file is
now created at the root level of the trash repository's working tree
instead of tested clones', and I do not think any test depends on
the number of untracked paths in the trash working tree or tries to
make sure a file called "lines" is not in there.

I think it is only that the other patch is actually fixing something,
whereas this is cleanup. So the cost/benefit equation is different. I
agree neither is high-risk and a test cleanup is generally OK for maint
(the other is a serious-ish regression IMHO).

I agree on the cost/benefit equation being different. I considered this patch
a "normal risk" thing, which by our standards is not going through to maint
directly, but is cooked at various heat levels before.

quoted

Having said that, I wonder if we want further reduction of the
repetition.  Each test except "setup" in this script does an
identical thing with very small set of parameters:

    - make sure super_clone will be removed when done.
    - clone file://$pwd/. to super_clone but with different set of options.
    - check the commits in super_clone and super_clone/sub.

So, the above would ideally become something like

do_test 3 3 --recurse-submodules

where the helper would look like

do_test () {
      cnt_super=$1 cnt_sub=$2 &&
        shift 2 &&
        test_when_finished "rm -fr super_clone" &&
        git clone "$@" "file://$pwd/." super_clone &&
        git -C super_clone log --oneline >lines &&
        test_line_count = $cnt_super lines &&
        git -C super_clone/sub log --oneline >lines &&
        test_line_count = $cnt_sub lines
}

Would it rob too much flexibility from future tests to be added to
this script if we did it that way?

I think that's an improvement, too. Even if we add further tests, they
don't have to follow the same format. I would give the function a better
name than "do_test" though. :P

I thought about implementing this, but I was unsure how much flexibility it
is robbing, as we don't know in which direction we're going to extend the
tests if at all. (Are we testing more options or do we want to inject more
commands like "git submodule update --keep-configured-depth" ?
That kept me away from doing the super short do_test)

Thanks,
Stefan

-Peff