Re: [PATCH] install_branch_config(): switch from 'else-if' series to 'nested if-else'

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Nishhal Gaba <nishgaba9@xxxxxxxxx> writes:

> From: Nishchal <nishgaba9@xxxxxxxxx>

Set user.email/user.name and sendemail.from to the same address to avoid
this inline From:.

> I am Nishchal Gaba, a GSOC 2014 aspirant, submitting patch in response to microproject(8)

This part of your message is the commit message. It should justify why
the change is good, but who you are is not very interesting here (think
of someone running "git log" or "git blame" a year from now and going
through your commit, what would he expect?). The first sentence could go
below the ---.

Please, wrap your messages (less than 80 characters per line).

> Similar Execution Time, but increased readability

Why capitalize Execution Time?

> +		if (origin){

Here and below: space before {

> +			if(remote_is_branch)

space before (

> +				printf_ln(rebasing ?
>  				  _("Branch %s set up to track remote branch %s from %s by rebasing.") :
>  				  _("Branch %s set up to track remote branch %s from %s."),
>  				  local, shortname, origin);
> -		else if (remote_is_branch && !origin)
> -			printf_ln(rebasing ?
> -				  _("Branch %s set up to track local branch %s by rebasing.") :
> -				  _("Branch %s set up to track local branch %s."),
> -				  local, shortname);
> -		else if (!remote_is_branch && origin)
> -			printf_ln(rebasing ?
> +			else
> +				printf_ln(rebasing ?
>  				  _("Branch %s set up to track remote ref %s by rebasing.") :
>  				  _("Branch %s set up to track remote ref %s."),

At this point, it would make sense to me to factor the printf_ln call
like

const char *msg;
if (...)
	msg = rebasing ? _("...") : _("...");
else
	msg = rebasing ? _("...") : _("...");
printf_ln(msg, local, shortname);

(but that's very subjective)

> -		else if (!remote_is_branch && !origin)
> -			printf_ln(rebasing ?
> +		}
> +
> +		else if (!origin){

Err, isn't this the else branch of "if (origin)" ? If so, why repeat
"!origin", and more specifically, isn't the next "else" branch dead
code:

> +		}
> +
>  		else
>  			die("BUG: impossible combination of %d and %p",
>  			    remote_is_branch, origin);

I mean: obviously, it has to be dead code, but it seems a bit strange to
read

if (x)
	...
else if (!x)
	...
else
	die(...)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]