[PATCH 1/2] t3401: modernize style

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 <redacted>
---
 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

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 <redacted>
---
 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

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:

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

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

Ramkumar Ramachandra wrote:

Martin von Zweigbergk wrote:

quoted

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.

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:

quoted

+       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.

[...]

quoted

+       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

On Fri, Dec 9, 2011 at 12:07 PM, Jonathan Nieder [off-list ref] wrote:

Ramkumar Ramachandra wrote:

quoted

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.

quoted

Martin von Zweigbergk wrote:

quoted

quoted

+       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).

quoted

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.