Pratyush Yadav <[email protected]> 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/
