[PATCH 1/2] t3401: modernize style

[PATCH 1/2] t3401: modernize style

[Martin von Zweigbergk]

Put the opening quote starting each test on the same line as the
test_expect_* invocation. Also make sure to use tabs for indentation.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
 t/t3401-rebase-partial.sh |   67 ++++++++++++++++++++++-----------------------
 1 files changed, 33 insertions(+), 34 deletions(-)

diff --git a/t/t3401-rebase-partial.sh b/t/t3401-rebase-partial.sh
index aea6685..d7c874c 100755
--- a/t/t3401-rebase-partial.sh
+++ b/t/t3401-rebase-partial.sh
@@ -11,51 +11,50 @@ local branch.
 '
 . ./test-lib.sh
 
-test_expect_success \
-    'prepare repository with topic branch' \
-    'echo First > A &&
-     git update-index --add A &&
-     git commit -m "Add A." &&
+test_expect_success 'prepare repository with topic branch' '
+	echo First > A &&
+	git update-index --add A &&
+	git commit -m "Add A." &&
 
-     git checkout -b my-topic-branch &&
+	git checkout -b my-topic-branch &&
 
-     echo Second > B &&
-     git update-index --add B &&
-     git commit -m "Add B." &&
+	echo Second > B &&
+	git update-index --add B &&
+	git commit -m "Add B." &&
 
-     echo AnotherSecond > C &&
-     git update-index --add C &&
-     git commit -m "Add C." &&
+	echo AnotherSecond > C &&
+	git update-index --add C &&
+	git commit -m "Add C." &&
 
-     git checkout -f master &&
+	git checkout -f master &&
 
-     echo Third >> A &&
-     git update-index A &&
-     git commit -m "Modify A."
+	echo Third >> A &&
+	git update-index A &&
+	git commit -m "Modify A."
 '
 
-test_expect_success \
-    'pick top patch from topic branch into master' \
-    'git cherry-pick my-topic-branch^0 &&
-     git checkout -f my-topic-branch &&
-     git branch master-merge master &&
-     git branch my-topic-branch-merge my-topic-branch
+test_expect_success 'pick top patch from topic branch into master' '
+	git cherry-pick my-topic-branch^0 &&
+	git checkout -f my-topic-branch &&
+	git branch master-merge master &&
+	git branch my-topic-branch-merge my-topic-branch
 '
 
-test_debug \
-    'git cherry master &&
-     git format-patch -k --stdout --full-index master >/dev/null &&
-     gitk --all & sleep 1
+test_debug '
+	git cherry master &&
+	git format-patch -k --stdout --full-index master >/dev/null &&
+	gitk --all & sleep 1
 '
 
-test_expect_success \
-    'rebase topic branch against new master and check git am did not get halted' \
-    'git rebase master && test ! -d .git/rebase-apply'
+test_expect_success 'rebase topic branch against new master and check git am did not get halted' '
+	git rebase master &&
+	test ! -d .git/rebase-apply
+'
 
-test_expect_success \
-	'rebase --merge topic branch that was partially merged upstream' \
-	'git checkout -f my-topic-branch-merge &&
-	 git rebase --merge master-merge &&
-	 test ! -d .git/rebase-merge'
+test_expect_success 'rebase --merge topic branch that was partially merged upstream' '
+	git checkout -f my-topic-branch-merge &&
+	git rebase --merge master-merge &&
+	test ! -d .git/rebase-merge
+'
 
 test_done
-- 
1.7.8.237.gcc4e3

[PATCH 2/2] t3401: use test_commit in setup

[Martin von Zweigbergk]

Simplify t3401 by using test_commit in the setup. This lets us refer
to commits using their tags and there is no longer a need to create
the branch my-topic-branch-merge. Also, the branch master-merge points
to the same commit as master (even before this change), so that branch
does not need to be created either.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
 t/t3401-rebase-partial.sh |   31 ++++++++-----------------------
 1 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/t/t3401-rebase-partial.sh b/t/t3401-rebase-partial.sh
index d7c874c..1aac22c 100755
--- a/t/t3401-rebase-partial.sh
+++ b/t/t3401-rebase-partial.sh
@@ -12,32 +12,17 @@ local branch.
 . ./test-lib.sh
 
 test_expect_success 'prepare repository with topic branch' '
-	echo First > A &&
-	git update-index --add A &&
-	git commit -m "Add A." &&
-
+	test_commit A &&
 	git checkout -b my-topic-branch &&
-
-	echo Second > B &&
-	git update-index --add B &&
-	git commit -m "Add B." &&
-
-	echo AnotherSecond > C &&
-	git update-index --add C &&
-	git commit -m "Add C." &&
-
+	test_commit B &&
+	test_commit C &&
 	git checkout -f master &&
-
-	echo Third >> A &&
-	git update-index A &&
-	git commit -m "Modify A."
+	test_commit A2 A.t
 '
 
 test_expect_success 'pick top patch from topic branch into master' '
-	git cherry-pick my-topic-branch^0 &&
-	git checkout -f my-topic-branch &&
-	git branch master-merge master &&
-	git branch my-topic-branch-merge my-topic-branch
+	git cherry-pick C &&
+	git checkout -f my-topic-branch
 '
 
 test_debug '
@@ -52,8 +37,8 @@ test_expect_success 'rebase topic branch against new master and check git am did
 '
 
 test_expect_success 'rebase --merge topic branch that was partially merged upstream' '
-	git checkout -f my-topic-branch-merge &&
-	git rebase --merge master-merge &&
+	git reset --hard C &&
+	git rebase --merge master &&
 	test ! -d .git/rebase-merge
 '
 
-- 
1.7.8.237.gcc4e3

Re: [PATCH 2/2] t3401: use test_commit in setup

[Ramkumar Ramachandra]

Hi again,

Martin von Zweigbergk wrote:
> Simplify t3401 by using test_commit in the setup. This lets us refer
> to commits using their tags and there is no longer a need to create
> the branch my-topic-branch-merge. Also, the branch master-merge points
> to the same commit as master (even before this change), so that branch
> does not need to be created either.

The terms "tag" and "branch" here have no significance, so
de-emphasizing them to "ref" is probably a good idea.  Isn't the truth
more like: instead of creating commits and creating refs to track
those commits by hand, use test_commit to achieve the same result in a
single step?

Cheers.

-- Ram

Re: [PATCH 2/2] t3401: use test_commit in setup

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Martin von Zweigbergk wrote:

>> Simplify t3401 by using test_commit in the setup. This lets us refer
>> to commits using their tags and there is no longer a need to create
>> the branch my-topic-branch-merge. Also, the branch master-merge points
>> to the same commit as master (even before this change), so that branch
>> does not need to be created either.
>
> The terms "tag" and "branch" here have no significance, so
> de-emphasizing them to "ref" is probably a good idea.

Wha?  No, "tag" refers to refs under refs/tags/, and "branch" refers to
refs/heads/, just like usual.

I like the patch, for what it's worth.

Re: [PATCH 1/2] t3401: modernize style

[Ramkumar Ramachandra]

Hi Martin,

No cover letter, so I'm assuming these are just two random one-off
patches. The motivation is unclear: lazy afternoon? :P

Martin von Zweigbergk wrote:
> [...]
> diff --git a/t/t3401-rebase-partial.sh b/t/t3401-rebase-partial.sh
> index aea6685..d7c874c 100755
> --- a/t/t3401-rebase-partial.sh
> +++ b/t/t3401-rebase-partial.sh
> @@ -11,51 +11,50 @@ local branch.
>  '
>  . ./test-lib.sh
>
> -test_expect_success \
> -    'prepare repository with topic branch' \
> -    'echo First > A &&
> -     git update-index --add A &&
> -     git commit -m "Add A." &&
> +test_expect_success 'prepare repository with topic branch' '
> +       echo First > A &&
> +       git update-index --add A &&
> +       git commit -m "Add A." &&

Style nit: >[^ ] is prevalent FWIW.

$ git grep '> [^ ]' t/ | wc -l
3091
$ git grep '>[^ ]' t/ | wc -l
9271

Sure, the regular expressions aren't tailored to make sure that only
redirections are caught, but I suppose it's safe to assume that one
number is significantly larger than the other.

> [...]
> -test_expect_success \
> -    'rebase topic branch against new master and check git am did not get halted' \
> -    'git rebase master && test ! -d .git/rebase-apply'
> +test_expect_success 'rebase topic branch against new master and check git am did not get halted' '
> +       git rebase master &&
> +       test ! -d .git/rebase-apply
> +'

While at it, why not change this "test ! -d" to
"test_path_is_missing"?  My rationale is that you're touching the file
anyway to do a generic cleanup; might as well finish it.

Thanks.

-- Ram

Re: [PATCH 1/2] t3401: modernize style

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> The motivation is unclear: lazy afternoon? :P

Perhaps he was reading the list and after noticing a few patches in
the same vein, realized that this test script could be made easier to
read, too.

[...]
> Martin von Zweigbergk wrote:

>> +       echo First > A &&
>> +       git update-index --add A &&
>> +       git commit -m "Add A." &&
>
> Style nit: >[^ ] is prevalent FWIW.

At first it wasn't clear to me what you meant here.  Was it that
quoted text in an email should start with a non-space character, like
a tab?

Finally I caught on that you mean that redirection operators tend to
be flush against the filename they are redirecting to.

[...]
>> +       test ! -d .git/rebase-apply
>> +'
>
> While at it, why not change this "test ! -d" to
> "test_path_is_missing"?

Sounds like a useful hint.  The benefits are that it would catch
failures that make .git/rebase-apply into an ordinary file, and more
useful output from "sh t3401-* -v -i" when the test fails.  The
main downside I can think of is that the test script would not run
against versions of the test harness before v1.7.3.3~5^2~1 (test-lib:
user-friendly alternatives to test [-d|-f|-e], 2010-08-10).

The patch looks good to me, too.  Thanks, both.

Sincerely,
Jonathan

Re: [PATCH 1/2] t3401: modernize style

[Martin von Zweigbergk]

On Fri, Dec 9, 2011 at 12:07 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ramkumar Ramachandra wrote:
>
>> The motivation is unclear: lazy afternoon? :P
>
> Perhaps he was reading the list and after noticing a few patches in
> the same vein, realized that this test script could be made easier to
> read, too.

Sort of. These patches have been sitting in my repo since late Sept
and the patches you mention made me decide to send them out now. The
reason I did this back then was while trying to fix rebase to pick the
right patches when used with --onto. See this old discussion:
http://thread.gmane.org/gmane.comp.version-control.git/161917/focus=162041.
Also in the same series are patches teach rebase to only feed the
commit names to git-am (wrapped in a silly "From $sha1 Mon
Sep 17 00:00:00 2001" to please git-mailsplit). These patches have
been taking way too long, which is why I'm sending these little
cleanups separately.

>> Martin von Zweigbergk wrote:
>
>>> +       echo First > A &&
>>> +       git update-index --add A &&
>>> +       git commit -m "Add A." &&
>>
>> Style nit: >[^ ] is prevalent FWIW.
>
> Finally I caught on that you mean that redirection operators tend to
> be flush against the filename they are redirecting to.

So did I. I think I'll leave the code unchanged, though, because the
end result, after patch 2/2 is unaffected anyways (it removes
redirections).

>> While at it, why not change this "test ! -d" to
>> "test_path_is_missing"?

Will bake it into patch 2/2 if you don't mind. Unless there are other
comments, that would mean this patch can be applies as is, Junio.

> The patch looks good to me, too.  Thanks, both.

Thanks to you both, too.