Re: [PATCH 1/8] push: change push to fail if short ref name does not exist

From: Daniel Barkalow <barkalow@iabervon.org>
Date: Oct 27, 2007, 21:42 UTC
Message-ID: <Pine.LNX.4.64.0710271716120.7345@iabervon.org>
In-Reply-To: <11935038081211-git-send-email-prohaska@zib.de>

On Sat, 27 Oct 2007, Steffen Prohaska wrote:

Show 9 quoted lines

> You can use a branch's shortname to push it. Push used to create
> the branch on the remote side if it did not yet exist. If you
> specified the wrong branch accidentally it was created. A safety
> valve that pushes only existing branches may help to avoid
> errors.
> 
> This commit changes push to fail if the remote ref does not yet
> exist and the refspec does not start with refs/. Remote refs must
> explicitly be created with their full name.

I agree with the change (and I think it's appropriate for master or next), but your implementation is a bit too clever for my tastes.

Show 15 quoted lines

> Signed-off-by: Steffen Prohaska <prohaska@zib.de>
> ---
>  remote.c              |    5 +++--
>  t/t5516-fetch-push.sh |   34 ++++++++++++++++++++++++++++++++--
>  2 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/remote.c b/remote.c
> index 170015a..ec992c9 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -611,6 +611,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
>  	struct ref *matched_src, *matched_dst;
>  
>  	const char *dst_value = rs->dst;
> +	const char * const orig_dst_value = rs->dst ? rs->dst : rs->src;

"lit_dst_value" would probably be a better description, and it might be worth handling the rs->dst == NULL case where it's handled for dst_value. Technically, this variable, when it doesn't match the final value of dst_value, has a value that dst_value never had.

Show 10 quoted lines

>  
>  	if (rs->pattern)
>  		return errs;
> @@ -647,12 +648,12 @@ static int match_explicit(struct ref *src, struct ref *dst,
>  	case 1:
>  		break;
>  	case 0:
> -		if (!memcmp(dst_value, "refs/", 5))
> +		if (!memcmp(orig_dst_value , "refs/", 5))
>  			matched_dst = make_linked_ref(dst_value, dst_tail);

This should also be orig_dst_value, too. I know that dst_value and orig_dst_value must be the same in this case, but it takes a bunch of analysis to demonstrate that, and it's more intuitive to use the value you've just checked anyway, and also to have all of case 0 use the differently-computed destination.

Show 5 quoted lines

>  		else
>  			error("dst refspec %s does not match any "
>  			      "existing ref on the remote and does "
> -			      "not start with refs/.", dst_value);
> +			      "not start with refs/.", orig_dst_value);

Maybe the error should provide a hint if dst_value is not the same as orig_dst_value? (if (!rs->dst) error("Did you mean %s?\n", dst_value);)

	-Daniel
*This .sig left intentionally blank*