Re: [PATCH 1/3] push: add --current

From: Nanako Shiraishi <nanako3@lavabit.com>
Date: Jul 19, 2009, 21:32 UTC
Message-ID: <20090720063244.6117@nanako3.lavabit.com>
In-Reply-To: <1247526462-17584-2-git-send-email-bonzini@gnu.org>

Quoting Paolo Bonzini <bonzini@gnu.org>:

Show 10 quoted lines

> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 2653388..0d6fcaa 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -9,7 +9,7 @@ git-push - Update remote refs along with associated objects
>  SYNOPSIS
>  --------
>  [verse]
> -'git push' [--all | --mirror | --tags] [--dry-run] [--receive-pack=<git-receive-pack>]
> +'git push' [--all | --mirror | --tags] [--current] [--dry-run] [--receive-pack=<git-receive-pack>]

Shouldn't this be "[--all | --mirror | --tags | --current]" instead? In other words, does it make sense to say "git push --all --current"?

Show 13 quoted lines

> @@ -71,6 +71,15 @@ nor in any Push line of the corresponding remotes file---see below).
>  	Instead of naming each ref to push, specifies that all
>  	refs under `$GIT_DIR/refs/heads/` be pushed.
>  
> +--current::
> +	Independent of the other options, restrict pushing to the current
> +	HEAD.
> +
> +	Refspecs given in the configuration is still used to find the
> +	destination name of the current branch.  However, this option
> +	cannot be specified if an explicit refspec is given on the
> +	command line, because it would be useless and possibly confusing.
> +

The second and subsequent paragraphs must be dedented, and the blank line between paragraphs should be replaced by a single line with '+' on it. You can find examples in the same file (e.g. description of <refspec>).

Instead of correcting a grammatical error you made here in patch 1/3 with a later patch 2/3, please fix it here.

Show 22 quoted lines

> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 2d2633f..3333ce9 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -586,4 +586,56 @@ test_expect_success 'push with branches containing #' '
>  	git checkout master
>  '
>  
> +test_expect_success 'push --current fails on empty repository' '
> +	git init &&
> +	mkdir b.git &&
> +	(cd b.git && git init --bare) &&
> +	echo a > b &&
> +	git add b &&
> +	git commit -m a &&
> +	git checkout -b branch &&
> +	echo bb > b &&
> +	git add b &&
> +	git commit -m branch &&
> +	git checkout master &&
> +	! git push --current b.git
> +'

If your final belief (which I happen to agree) is that

% git push --current % git push --current origin % git push --current over.there.example.com:project.git

should work as expected (that is, they should push the current branch to the same name) from a repository without any special configuration, don't say "push --current fails" on the title as if you think it is the right thing to do.

Instead, mark clearly that the code after this patch is still broken, like this:

	test_expect_failure 'push --current into an empty repository' '
		...
                git push --current b.git
	'

and change expect_failure to expect_success in the later patch that fixes the breakage.

Show 6 quoted lines

> +test_expect_success 'push --current succeeds when push is configured' '
> +	git config remote.bremote.url b.git &&
> +	git config remote.bremote.push refs/heads/master:refs/heads/master &&
> +	git push --current bremote &&
> +	test `git rev-parse master` = `cd b.git && git rev-parse master`
> +'

The style of the existing tests in the script isn't

	test `git rev-parse master` = `cd b.git && git rev-parse master`

but is

	test "$(git rev-parse master)" = "$(cd b.git && git rev-parse master)"

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/