Re: [PATCH] pull, fetch: add --set-upstream option
- From
Matthieu Moy <matthieu.moy@matthieu-moy.fr>
- Date
- Aug 19, 2019, 09:08 UTC
- Message-ID
- <86a7c5cykq.fsf@matthieu-moy.fr>
- In-Reply-To
- <20190814171404.zqtd4xctjobgpzby@localhost.localdomain>
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.
Show 5 quoted lines
>> -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.
Show 20 quoted lines
>> + 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.
Show 14 quoted lines
>> + 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.
Show 5 quoted lines
> 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".
Show 12 quoted lines
>
> 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/