Re: [PATCH 07/10] Change incorrect "remote branch" to "remote tracking branch" in C code

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Matthieu Moy wrote:
>
>> --- a/branch.h
>> +++ b/branch.h
>> @@ -22,7 +22,7 @@ void create_branch(const char *head, const char *name, const char *start_name,
>>  void remove_branch_state(void);
>>  
>>  /*
>> - * Configure local branch "local" to merge remote branch "remote"
>> + * Configure local branch "local" to merge remote-tracking branch "remote"
>>   * taken from origin "origin".
>>   */
>>  #define BRANCH_CONFIG_VERBOSE 01
>
> Is this really more accurate?

Good question ;-).

> I thought what install_branch_config does is to configure local
> branch "local" as downstream to remote branch "remote" from origin
> "origin". That means:
>
>  - "git pull" fetches that remote and then merges the corresponding
>    remote-tracking branch
>  - "git remote show" compares the local branch to the remote branch
>  - "git branch -v" compares the local branch to the remote-tracking
>    branch
>
> and so on.

(there's also "git status" which says how many commits are in a
branch and not the other)

Well, actually, this configuration creates a relationship from the
local branch to both the remote-tracking and the remote branch.
Disconnected operations (git status for example) will deal with the
remote-tracking, but "pull" will pull from the actually remote one.

Anyway, the original wording was not ambiguous (thanks to the "taken
from origin" part), so I'm fine with dropping this hunk.

>> index 3b0b614..4243ef0 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -359,7 +359,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>>  			what = rm->name + 10;
>>  		}
>>  		else if (!prefixcmp(rm->name, "refs/remotes/")) {
>> -			kind = "remote branch";
>> +			kind = "remote-tracking branch";
>
> For use by "git merge" and other "git fmt-merge-msg"-like consumers.  Good.
>
>> --- a/builtin/remote.c
>> +++ b/builtin/remote.c
>> @@ -791,9 +791,9 @@ static int rm(int argc, const char **argv)
>>  
>>  	if (skipped.nr) {
>>  		fprintf(stderr, skipped.nr == 1 ?
>> -			"Note: A non-remote branch was not removed; "
>> +			"Note: A non-remote-tracking branch was not removed; "
>>  			"to delete it, use:\n" :
>> -			"Note: Non-remote branches were not removed; "
>> +			"Note: Non-remote-tracking branches were not removed; "
>>  			"to delete them, use:\n");
>
> This wording is ugly.

I didn't like the double-dash either, but couldn't find anything
better.

> Maybe something to this effect would be better?
>
> 	Note: A ref outside the refs/remotes/ hierarchy was not removed:

Actually, that's even more accurate. The non-deleted branch is somehow
a remote-tracking since "fetch" feeds it, and the code detecting this
case is:

	/* don't delete non-remote-tracking refs */
	if (prefixcmp(refname, "refs/remotes")) {
		/* advise user how to delete local branches */
		if (!prefixcmp(refname, "refs/heads/"))
			string_list_append(branches->skipped,
					   abbrev_branch(refname));
		/* silently skip over other non-remote refs */
		return 0;
	}

hence, really what you wrote. I just disagree with the "ref", since
according to the code, only branches show this note, other are skept
silently.

So, I'll make this:

Note: A branch outside the refs/remotes/ hierarchy was not removed:

>> --- a/t/t5505-remote.sh
>> +++ b/t/t5505-remote.sh
>> @@ -107,16 +107,16 @@ test_expect_success 'remove remote' '
>>  )
>>  '
>>  
>> -test_expect_success 'remove remote protects non-remote branches' '
>> +test_expect_success 'remove remote protects non-remote-tracking branches' '
>
> Maybe:
>
> 	test_expect_success 'remove remote protects local branches' '
>
> since that is what is important in practice.

I buy that too.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/