Re: [PATCH] git-parse-remote: Support dummy remote `.' in branch.<name>.remote

From: Junio C Hamano <junkio@cox.net>
Date: Mar 10, 2007, 00:17 UTC
Message-ID: <7v1wjy3qas.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <45F182DC.9090507@lu.unisi.ch>

Paolo Bonzini <paolo.bonzini@lu.unisi.ch> writes:

Show 6 quoted lines

> This patch adds support for a dummy remote `.' to avoid having to declare
> a fake remote like
>
> 	[remote "local"]
> 		url = .
> 		fetch = refs/heads/*:refs/heads/*

For one thing, I do not think it would help Andy's original example of reusing what we already have in refs/remotes hierarchy if you faked it with refs/heads/*:refs/heads/* wildcard like the above.

Why not

	[remote "local"]
        	url = .
                fetch = refs/heads/*

or even

	[remote "local"]
        	url = .
                fetch = refs/*

except that we currently do not have non-storing globbing pattern defined?

I am actually Ok with the idea of built-in ".", but I really do not like it to even pretend storing the fetched results in local branches. If you stop thinking in terms of "tracking fetch" that stores the result in some local branch, you do not have to have branch.forcelocalupdates.

By the way, I do not understand the semantics of that configuration variable at all.

Show 5 quoted lines

> +branch.forcelocalupdates::
> +	If set to true, merges from the local repository (i.e. when
> +	branch.<name>.remote is the special setting `.`) are performed
> +	even if they do not result in a fast forward update.  The default
> +	is true.

Really, when does fetching from refs/heads/<blah> into refs/heads/<blah> result in anything but "already up-to-date"?

I think you can get rid of this ridiculous "we obtain refs/heads/master from ourselves and store it in the same branch of ours". The only thing you want here is to leave FETCH_HEAD in a state that is usable by git-pull. Wouldn't that be much cleaner?

Show 14 quoted lines

> @@ -128,6 +134,13 @@ get_remote_default_refs_for_fetch () {
>  	case "$data_source" in
>  	'')
>  		set explicit "HEAD:" ;;
> +	builtin)
> +		if test $(git-config --bool "branch.forcelocalupdates" || echo true) = true
> +		then
> +			set $(expand_refs_wildcard . +refs/heads/*:refs/heads/*)
> +		else
> +			set $(expand_refs_wildcard . refs/heads/*:refs/heads/*)
> +		fi ;;
>  	config)
>  		set $(expand_refs_wildcard "$1" \
>  			$(git-config --get-all "remote.$1.fetch")) ;;

Especially this above hunk is not something I'd like to see so the patch won't be applied as-is in this round, but just for future reference...

The --bool option makes sure the git-config command either says "true", "false" or silently exit with non-zero, in which case you would let echo say "true". In that sense, it is not a problem in this particular patch, but in general it makes me worried when I see somebody feeds $() without quoting to test as its argument.