Hi Szeder,
On Sun, Mar 17, 2019 at 02:05:39PM +0100, SZEDER Gábor wrote:
> On Sun, Mar 17, 2019 at 03:15:50AM -0700, Denton Liu wrote:
> > Sorry for taking so long to do a reroll, I've been pretty busy this week
> > and I've only been able to find the time now.
>
> No problem, and thank you for sticking with it!
>
>
> > diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
> > index ca4a740da0..f035e4a507 100755
> > --- a/t/t7502-commit-porcelain.sh
> > +++ b/t/t7502-commit-porcelain.sh
> > @@ -16,7 +16,9 @@ commit_msg_is () {
> > # Arguments: [<prefix] [<commit message>] [<commit options>]
> > check_summary_oneline() {
> > test_tick &&
> > - git commit ${3+"$3"} -m "$2" | head -1 > act &&
> > + git commit ${3+"$3"} -m "$2" >act &&
> > + head -1 <act >tmp &&
> > + mv tmp act &&
>
> Here you could swap the order of when you write to 'act' and 'tmp',
> and thus make the 'mv' unnecessary, like this:
>
> git commit [...] >tmp &&
> head -1 act >tmp &&
> [...rest of the test...]
>
> Note also that 'head' (or 'sed' in later tests) can open its input
> file on its own, there's no need to redirect its standard input.
>
> This is a recurring pattern in patches 1, 4, 8, and 9.
>
The reason I did it this way was because earlier, Junio said:
> > - git cat-file commit HEAD | sed -e '1,/^$/d' >actual &&
> > + git cat-file commit HEAD >tmp &&
> > + sed -e '1,/^$/d' <tmp >actual &&
>
> The intermediary file may want a name better than 'tmp', if it is to
> be left behind, but this will do for now.
So I opted to write the tests in a way where a tmp file won't be
produced. This pattern was shamelessly stolen from
'set up mod-256 conflict scenario' in t7600 where it does the following:
# 256 near-identical stanzas...
for i in $(test_seq 1 256); do
for j in 1 2 3 4 5; do
echo $i-$j
done
done >file &&
git add file &&
git commit -m base &&
# one side changes the first line of each to "master"
sed s/-1/-master/ <file >tmp &&
mv tmp file &&
git commit -am master &&
Good point about the heads and seds, though. I completely forgot that
they accept a file argument.
> > @@ -142,8 +144,8 @@ test_expect_success 'sign off' '
> > >positive &&
> > git add positive &&
> > git commit -s -m "thank you" &&
> > - actual=$(git cat-file commit HEAD | sed -ne "s/Signed-off-by: //p") &&
> > - expected=$(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/") &&
> > + actual=$(git cat-file commit HEAD >tmp && sed -ne "s/Signed-off-by:
> > //p" <tmp && rm tmp) &&
> > + expected=$(git var GIT_COMMITTER_IDENT >tmp && sed -e "s/>.*/>/" <tmp
> > && rm tmp) &&
> > test "z$actual" = "z$expected"
>
> May I ask you to go one step further in restructuring this and the
> following tests? :) Instead of using 'test' to compare the contents
> of the $actual and $expected variables, use 'test_cmp' to compare the
> 'actual' and 'expected' files, something like:
>
> git cat-file commit HEAD >tmp &&
> sed -ne "s/Signed-off-by: //p" tmp >actual &&
> git var GIT_COMMITTER_IDENT >tmp &&
> sed -e "s/>.*/>/" >expected &&
> test_cmp expected actual
Will do.
Thanks,
Denton
