threads / patch / 23588

patch, 2 partsrequest-pull: protect against OPTIONS_KEEPDASHDASH from environment

Subject: [PATCH 0/2] request-pull: protect against OPTIONS_KEEPDASHDASH from environment

## tl;dr

6 messages between Apr 24, 2010 and Apr 26, 2010. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Jonathan Nieder· Apr 24, 2010, 12:10 UTC · lore
Hi,

At last, the mysterious t7400 test failure has been solved, thanks to Gerrit Pape. It was caused by an assumption in git that a certain variable not in its namespace would not be set.

Here’s a problem of the same kind, though harder to trip. I can’t imagine why anyone would be exporting the OPTIONS_KEEPDASHDASH variable, but it is not in git’s namespace so it seems better not to make any assumptions.

Jonathan Nieder (2):
  tests for request-pull
  request-pull: protect against OPTIONS_KEEPDASHDASH from environment
 contrib/git-resurrect.sh |    1 +
 git-request-pull.sh      |    1 +
 t/t5150-request-pull.sh  |  228 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 230 insertions(+), 0 deletions(-)
 create mode 100644 t/t5150-request-pull.sh
Jonathan Nieder· Apr 24, 2010, 12:11 UTC · re: Jonathan Nieder · lore

[PATCH 1/2] tests for request-pull

Test that request-pull handles failure to push cleanly, writes pull requests that produce the correct effect when followed, and uses a predictable format.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t5150-request-pull.sh |  214 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 214 insertions(+), 0 deletions(-)
 create mode 100644 t/t5150-request-pull.sh
Show changes to t/t5150-request-pull.sh +214 −0
diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
new file mode 100644
index 0000000..e012a36
--- /dev/null
+++ b/t/t5150-request-pull.sh
@@ -0,0 +1,214 @@
+#!/bin/sh
+
+test_description='Test workflows involving pull request.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+
+	git init --bare upstream.git &&
+	git init --bare downstream.git &&
+	git clone upstream.git upstream-private &&
+	git clone downstream.git local &&
+
+	trash_url="file://$TRASH_DIRECTORY" &&
+	downstream_url="$trash_url/downstream.git/" &&
+	upstream_url="$trash_url/upstream.git/" &&
+
+	(
+		cd upstream-private &&
+		cat <<-\EOT >mnemonic.txt &&
+		Thirtey days hath November,
+		Aprile, June, and September:
+		EOT
+		git add mnemonic.txt &&
+		test_tick &&
+		git commit -m "\"Thirty days\", a reminder of month lengths" &&
+		git tag -m "version 1" -a initial &&
+		git push --tags origin master
+	) &&
+	(
+		cd local &&
+		git remote add upstream "$trash_url/upstream.git" &&
+		git fetch upstream &&
+		git pull upstream master &&
+		cat <<-\EOT >>mnemonic.txt &&
+		Of twyecescore-eightt is but eine,
+		And all the remnante be thrycescore-eine.
+		O’course Leap yare comes an’pynes,
+		Ev’rie foure yares, gote it ryghth.
+		An’twyecescore-eight is but twyecescore-nyne.
+		EOT
+		git add mnemonic.txt &&
+		test_tick &&
+		git commit -m "More detail" &&
+		git tag -m "version 2" -a full &&
+		git checkout -b simplify HEAD^ &&
+		mv mnemonic.txt mnemonic.standard &&
+		cat <<-\EOT >mnemonic.clarified &&
+		Thirty days has September,
+		All the rest I can’t remember.
+		EOT
+		git add -N mnemonic.standard mnemonic.clarified &&
+		git commit -a -m "Adapt to use modern, simpler English
+
+But keep the old version, too, in case some people prefer it." &&
+		git checkout master
+	)
+
+'
+
+test_expect_success 'setup: two scripts for reading pull requests' '
+
+	downstream_url_for_sed=$(
+		printf "%s\n" "$downstream_url" |
+		sed -e '\''s/\\/\\\\/g'\'' -e '\''s/[[/.*^$]/\\&/g'\''
+	) &&
+
+	cat <<-\EOT >read-request.sed &&
+	#!/bin/sed -nf
+	/ in the git repository at:$/! d
+	n
+	/^$/! q
+	n
+	s/^[ 	]*\(.*\) \([^ ]*\)/please pull\
+	\1\
+	\2/p
+	q
+	EOT
+
+	cat <<-EOT >fuzz.sed
+	#!/bin/sed -nf
+	s/$_x40/OBJECT_NAME/g
+	s/A U Thor/AUTHOR/g
+	s/        [^ ].*/        SUBJECT/g
+	s/$downstream_url_for_sed/URL/g
+	s/for-upstream/BRANCH/g
+	s/mnemonic.txt/FILENAME/g
+	/^ FILENAME | *[0-9]* [-+]*\$/ b diffstat
+	/^AUTHOR ([0-9]*):\$/ b shortlog
+	p
+	b
+	: diffstat
+	n
+	/ [0-9]* files changed/ {
+		a\
+	DIFFSTAT
+		b
+	}
+	b diffstat
+	: shortlog
+	/^        [a-zA-Z]/ n
+	/^[a-zA-Z]* ([0-9]*):\$/ n
+	/^\$/ N
+	/^\n[a-zA-Z]* ([0-9]*):\$/! {
+		a\
+	SHORTLOG
+		D
+	}
+	n
+	b shortlog
+	EOT
+
+'
+
+test_expect_success 'pull request when forgot to push' '
+
+	rm -fr downstream.git &&
+	git init --bare downstream.git &&
+	(
+		cd local &&
+		git checkout initial &&
+		git merge --ff-only master &&
+		test_must_fail git request-pull initial "$downstream_url" \
+			2>../err
+	) &&
+	grep "No branch of.*is at:\$" err &&
+	grep "Are you sure you pushed" err
+
+'
+
+test_expect_success 'pull request after push' '
+
+	rm -fr downstream.git &&
+	git init --bare downstream.git &&
+	(
+		cd local &&
+		git checkout initial &&
+		git merge --ff-only master &&
+		git push origin master:for-upstream &&
+		git request-pull initial origin >../request
+	) &&
+	sed -nf read-request.sed <request >digest &&
+	cat digest &&
+	{
+		read task &&
+		read repository &&
+		read branch
+	} <digest &&
+	(
+		cd upstream-private &&
+		git checkout initial &&
+		git pull --ff-only "$repository" "$branch"
+	) &&
+	test "$branch" = for-upstream &&
+	test_cmp local/mnemonic.txt upstream-private/mnemonic.txt
+
+'
+
+test_expect_success 'request names an appropriate branch' '
+
+	rm -fr downstream.git &&
+	git init --bare downstream.git &&
+	(
+		cd local &&
+		git checkout initial &&
+		git merge --ff-only master &&
+		git push --tags origin master simplify &&
+		git push origin master:for-upstream &&
+		git request-pull initial "$downstream_url" >../request
+	) &&
+	sed -nf read-request.sed <request >digest &&
+	cat digest &&
+	{
+		read task &&
+		read repository &&
+		read branch
+	} <digest &&
+	{
+		test "$branch" = master ||
+		test "$branch" = for-upstream
+	}
+
+'
+
+test_expect_success 'pull request format' '
+
+	rm -fr downstream.git &&
+	git init --bare downstream.git &&
+	cat <<-\EOT >expect &&
+	The following changes since commit OBJECT_NAME:
+	  AUTHOR (1):
+	        SUBJECT
+
+	are available in the git repository at:
+
+	  URL BRANCH
+
+	SHORTLOG
+
+	DIFFSTAT
+	EOT
+	(
+		cd local &&
+		git checkout initial &&
+		git merge --ff-only master &&
+		git push origin master:for-upstream &&
+		git request-pull initial "$downstream_url" >../request
+	) &&
+	<request sed -nf fuzz.sed >request.fuzzy &&
+	test_cmp expect request.fuzzy
+
+'
+
+test_done

-- 
1.7.1.rc1
Jonathan Nieder· Apr 24, 2010, 12:29 UTC · re: Jonathan Nieder · lore

[PATCH] adapt request-pull tests for new pull request format

10eb0007 (request-pull: avoid mentioning that the start point is a single commit, 2010-01-29), changed the pull request format, so the test needs some changes to still pass:

 - tolerate a missing blank line between “in the git repository at:”
   and the name of repository and branch
 - recognize subject and date in the new request format
 - update the expected request template to match the new format
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t5150-request-pull.sh |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
Show changes to t/t5150-request-pull.sh +5 −5
diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
index 4c7f48a..b31b828 100644
--- a/t/t5150-request-pull.sh
+++ b/t/t5150-request-pull.sh
@@ -69,8 +69,7 @@ test_expect_success 'setup: two scripts for reading pull requests' '
 	#!/bin/sed -nf
 	/ in the git repository at:$/! d
 	n
-	/^$/! q
-	n
+	/^$/ n
 	s/^[ 	]*\(.*\) \([^ ]*\)/please pull\
 	\1\
 	\2/p
@@ -81,7 +80,9 @@ test_expect_success 'setup: two scripts for reading pull requests' '
 	#!/bin/sed -nf
 	s/$_x40/OBJECT_NAME/g
 	s/A U Thor/AUTHOR/g
+	s/[-0-9]\{10\} [:0-9]\{8\} [-+][0-9]\{4\}/DATE/g
 	s/        [^ ].*/        SUBJECT/g
+	s/  [^ ].* (DATE)/  SUBJECT (DATE)/g
 	s/$downstream_url_for_sed/URL/g
 	s/for-upstream/BRANCH/g
 	s/mnemonic.txt/FILENAME/g
@@ -188,11 +189,10 @@ test_expect_success 'pull request format' '
 	git init --bare downstream.git &&
 	cat <<-\EOT >expect &&
 	The following changes since commit OBJECT_NAME:
-	  AUTHOR (1):
-	        SUBJECT
 
-	are available in the git repository at:
+	  SUBJECT (DATE)
 
+	are available in the git repository at:
 	  URL BRANCH
 
 	SHORTLOG

-- 
1.7.1.rc1
Jonathan Nieder· Apr 24, 2010, 12:15 UTC · re: Jonathan Nieder · lore

[PATCH 2/2] request-pull: protect against OPTIONS_KEEPDASHDASH from environment

Like most git commands, request-pull supports a -- delimiter to allow callers to pass arguments that would otherwise be treated as an option afterwards. The internal OPTIONS_KEEPDASHDASH variable is passed empty to git-sh-setup to indicate that request-pull itself does not care about the position of the -- delimiter. But if the user has that variable in her environment, request-pull will see the “--” and fail.

Empty it explicitly to guard against this. While at it, make the corresponding fix to git-resurrect, too (all other scripts in git.git already protect themselves).

Cc: Thomas Rast <trast@student.ethz.ch>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Forgot to mention: this series is against maint.  For master, the test
needs a fixup to pass with v1.7.1-rc0~144^2 (request-pull: avoid
mentioning that the start point is a single commit, 2010-01-29), which
I’ll send as a reply.

Thanks for reading. As always, thoughts and simplifying suggestions are welcome.

 contrib/git-resurrect.sh |    1 +
 git-request-pull.sh      |    1 +
 t/t5150-request-pull.sh  |   14 ++++++++++++++
 3 files changed, 16 insertions(+), 0 deletions(-)
Show changes to 3 files +16 −0

contrib/git-resurrect.sh, git-request-pull.sh, t/t5150-request-pull.sh

diff --git a/contrib/git-resurrect.sh b/contrib/git-resurrect.sh
index c364dda..a4ed4c3 100755
--- a/contrib/git-resurrect.sh
+++ b/contrib/git-resurrect.sh
@@ -9,6 +9,7 @@ other/Merge <other> into <name> (respectively) commit subjects, which
 is rather slow but allows you to resurrect other people's topic
 branches."
 
+OPTIONS_KEEPDASHDASH=
 OPTIONS_SPEC="\
 git resurrect $USAGE
 --
diff --git a/git-request-pull.sh b/git-request-pull.sh
index 630cedd..b0a0311 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -8,6 +8,7 @@ USAGE='<start> <url> [<end>]'
 LONG_USAGE='Summarizes the changes between two commits to the standard output,
 and includes the given URL in the generated summary.'
 SUBDIRECTORY_OK='Yes'
+OPTIONS_KEEPDASHDASH=
 OPTIONS_SPEC='git request-pull [options] start url [end]
 --
 p    show patch text as well
diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
index e012a36..4c7f48a 100644
--- a/t/t5150-request-pull.sh
+++ b/t/t5150-request-pull.sh
@@ -211,4 +211,18 @@ test_expect_success 'pull request format' '
 
 '
 
+test_expect_success 'request-pull ignores OPTIONS_KEEPDASHDASH poison' '
+
+	(
+		cd local &&
+		OPTIONS_KEEPDASHDASH=Yes &&
+		export OPTIONS_KEEPDASHDASH &&
+		git checkout initial &&
+		git merge --ff-only master &&
+		git push origin master:for-upstream &&
+		git request-pull -- initial "$downstream_url" >../request
+	)
+
+'
+
 test_done

-- 
1.7.1.rc1
Thomas Rast· Apr 25, 2010, 12:34 UTC · re: Jonathan Nieder · lore

Re: [PATCH 2/2] request-pull: protect against OPTIONS_KEEPDASHDASH from environment

Jonathan Nieder wrote:
> Empty it explicitly to guard against this.  While at it, make the
> corresponding fix to git-resurrect, too (all other scripts in git.git
> already protect themselves).
Ack.  Thanks for thinking of contrib :-)
-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Miklos Vajna· Apr 26, 2010, 00:12 UTC · re: Jonathan Nieder · lore

Re: [PATCH 0/2] request-pull: protect against OPTIONS_KEEPDASHDASH from environment

On Sat, Apr 24, 2010 at 07:10:10AM -0500, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 8 quoted lines
> At last, the mysterious t7400 test failure has been solved, thanks to
> Gerrit Pape.  It was caused by an assumption in git that a certain
> variable not in its namespace would not be set.
> 
> Here???s a problem of the same kind, though harder to trip.  I can???t
> imagine why anyone would be exporting the OPTIONS_KEEPDASHDASH
> variable, but it is not in git???s namespace so it seems better not
> to make any assumptions.

Ack - I really just did cosmetics on request-pull's output, nothing more. :)

← back to recent threads