Re: [PATCH] pull, fetch: add --set-upstream option

[Matthieu Moy <Matthieu.Moy@matthieu-moy.fr>]

Pratyush Yadav <me@yadavpratyush.com> writes:

> This is not really a review. Just some minor nitpicks I spotted while 
> reading through.

Thanks for the comments.

>> -static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity, deepen_relative;
>> +static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity, deepen_relative, set_upstream;
>
> This line is getting pretty long. I think it is a good idea to split it 
> into two.

Indeed, and it was already >80 characters, I've split it.

>> +	if (set_upstream) {
>> +		struct branch *branch = branch_get("HEAD");
>> +		struct ref *rm;
>> +		struct ref *source_ref = NULL;
>> +
>> +		/*
>> +		 * We're setting the upstream configuration for the current branch. The
>> +		 * relevent upstream is the fetched branch that is meant to be merged with
>> +		 * the current one, i.e. the one fetched to FETCH_HEAD.
>> +		 *
>> +		 * When there are several such branches, consider the request ambiguous and
>> +		 * err on the safe side by doing nothing and just emit a warning.
>> +		 */
>
> The comment lines cross the 80 column boundary. The usual convention in 
> this project is to try to keep lines below 80 columns. For strings IMO 
> an exception can be allowed because breaking them up makes it harder to 
> grep for them. But comments are the easiest to format.
>
> Are you using a tab size of 4?

I'm not, but it's possible that the original authors had. Anyway, I've
wrapped it.

>> +		for (rm = ref_map; rm; rm = rm->next) {
>> +			if (!rm->peer_ref) {
>> +				if (source_ref) {
>> +					warning(_("multiple branch detected, incompatible with --set-upstream"));
>> +					goto skip;
>> +				} else {
>> +					source_ref = rm;
>> +				}
>> +			}
>> +		}
>> +		if (source_ref) {
>> +			if (!strcmp(source_ref->name, "HEAD") || 
>
> This line has a trailing space.

Fixed.

> So this should change to something like:
>
> 				install_branch_config(0, branch->name,
> 						      transport->remote->name,
> 						      source_ref->name);

I've added a newline after the comma, I don't like mixing "several
arguments on the same line" and "one argument per line".

>  
> Maybe this discrepancy is because you are using the wrong tab size?
>
>> +			} else if (starts_with(source_ref->name, "refs/remotes/")) {
>> +				warning(_("not setting upstream for a remote remote-tracking branch"));
>> +			} else if (starts_with(source_ref->name, "refs/tags/")) {
>> +				warning(_("not setting upstream for a remote tag"));
>> +			} else {
>> +				warning(_("unknown branch type"));
>> +			}
>
> No need to wrap single line if statements in braces.

Fixed.

>> +#tests for fetch --set-upstream
>
> Add a space after the '#'. Same in other comments below.

Fixed.

Thanks. Version 2 fixing all these follows.

-- 
Matthieu Moy
https://matthieu-moy.fr/