Re: [PATCH 2/4] push: introduce new push.default mode "simple"

[Jeff King <peff@peff.net>]

On Fri, Apr 20, 2012 at 04:59:02PM +0200, Matthieu Moy wrote:

> it has the same name remotely. If not, give an error that suggest the
> right command to push explicitely to 'upstream' or 'current'.

s/suggest/&s/

> beneficial on the next pull. Lacking better argument, we chose to deny
> the push, because it will be easier to change in the future is someone
> shows us wrong.

s/is/if/

> Original-patch-by: Jeff King <peff@peff.net>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> Except for the broken-ness, this adds the last line in the warning message:
> 
> "To chose either option permanently, read about push.default in git-config(1)"

I don't think that makes sense if I have set "push.default" to "simple"
myself. IOW, shouldn't that get added later, when it eventually becomes
the default (and then, only when it was chosen because it is the
default, not because somebody explicitly said they wanted it)?

> @@ -837,7 +839,7 @@ static int git_default_push_config(const char *var, const char *value)
>  			push_default = PUSH_DEFAULT_CURRENT;
>  		else {
>  			error("Malformed value for %s: %s", var, value);
> -			return error("Must be one of nothing, matching, "
> +			return error("Must be one of simple, nothing, matching, "
>  				     "tracking or current.");

Not your fault, but should this be s/tracking/upstream/ in the context
line?

> diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
> index c334c51..949dbdf 100755
> --- a/t/t5528-push-default.sh
> +++ b/t/t5528-push-default.sh
> @@ -13,6 +13,22 @@ test_expect_success 'setup bare remotes' '
>  	git push parent2 HEAD
>  '
>  
> +# $1 = local revision
> +# $2 = remote repository
> +# $3 = remote revision (tested to be equal to the local one)
> +check_pushed_commit () {
> +	git rev-parse "$1" > expect &&
> +	git --git-dir="$2" rev-parse "$3" > actual &&
> +	test_cmp expect actual
> +}

This is an extremely minor nit, but a test failure is often easier to
read if you use "log -1 --format=%s" here, assuming that the commits are
given reasonable subjects (so you get "-two\n+one" or similar instead of
some commit ids which aren't meaningful).

Also, I notice you take a repo argument here, but then all of the
callers just pass "repo1" (and the test_push_success function hardcodes
repo1).

> +test_expect_success 'push to existing branch, upstream configured with different name' '
> +	test_config branch.master.remote repo1 &&
> +	test_config branch.master.merge refs/heads/other-name &&
> +	git checkout master &&
> +	test_commit eight &&
> +	test_push_success upstream other-name &&
> +	test_commit nine &&
> +	test_must_fail git -c push.default=simple push &&
> +	test_push_success current master &&
> +	test_must_fail check_pushed_commit HEAD repo1 other-name
> +'

In the final must_fail, wouldn't it be more robust to use an affirmative
check that it was not touched, instead of checking that we failed to
find it to be equal (which could fail for other, unrelated reasons)?
Like this:

  check_pushed_commit HEAD^ repo1 other-name

I also wonder if it would make sense to wrap the whole "failed push"
thing in a function like this:

  test_push_failure() {
          git --git-dir=repo1 log -1 --format=%s "$2" >expect &&
          test_must_fail git -c push.default="$1" &&
          git --git-dir=repo1 log -1 --format=%s "$2" >actual &&
          test_cmp expect actual
  }

and then the earlier failures could also get this extra double-check for
free (that push not only reported failure, but that it failed without
actually touching the remote end, not for some unrelated reason).

That's all minor stuff; the bulk of the patch looks good to me.

-Peff