[PATCH] simplifying branch.c:install_branch_config() if()

[PATCH] simplifying branch.c:install_branch_config() if()

[Nemina Amarasinghe]

Signed-off-by: Nemina Amarasinghe <neminaa@gmail.com>
---
 branch.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/branch.c b/branch.c
index 723a36b..fd93603 100644
--- a/branch.c
+++ b/branch.c
@@ -87,16 +87,11 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 				  _("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)
+		else if (!remote_is_branch)
 			printf_ln(rebasing ?
 				  _("Branch %s set up to track remote ref %s by rebasing.") :
 				  _("Branch %s set up to track remote ref %s."),
 				  local, remote);
-		else if (!remote_is_branch && !origin)
-			printf_ln(rebasing ?
-				  _("Branch %s set up to track local ref %s by rebasing.") :
-				  _("Branch %s set up to track local ref %s."),
-				  local, remote);
 		else
 			die("BUG: impossible combination of %d and %p",
 			    remote_is_branch, origin);
-- 
1.9.0.152.g6ab4ae2

Re: [PATCH] simplifying branch.c:install_branch_config() if()

[Jonathan Nieder]

Hi,

Nemina Amarasinghe wrote:

> Signed-off-by: Nemina Amarasinghe <neminaa@gmail.com>

The above is a place to explain why this is a good change.  For example:

	When it prints a message indicating what it has done,
	install_branch_config() treats the (!remote_is_branch && origin)
	and (!remote_is_branch && !origin) cases separately.  But they
	are the same, and it uses the same message for both.

	Simplify by just checking for !remote_is_branch.

	Noticed using the magic-identical-branch-checker tool.

	Signed-off-by: ...

(That's just a first random hypothetical guess --- of course please do
not use it but put your own rationale for the change there.)

[...]
> --- a/branch.c
> +++ b/branch.c
> @@ -87,16 +87,11 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
>  				  _("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)
> +		else if (!remote_is_branch)
>  			printf_ln(rebasing ?
>  				  _("Branch %s set up to track remote ref %s by rebasing.") :
>  				  _("Branch %s set up to track remote ref %s."),
>  				  local, remote);
> -		else if (!remote_is_branch && !origin)
> -			printf_ln(rebasing ?
> -				  _("Branch %s set up to track local ref %s by rebasing.") :
> -				  _("Branch %s set up to track local ref %s."),
> -				  local, remote);

Is this safe?  Today branch.c::create_branch checks that the argument
to e.g. --set-upstream-to is either in refs/heads/* or the image of
some remote, but what is making sure that remains true tomorrow?

So if changing this, I would be happier if the "if" condition were
still (!remote_is_branch && origin) so the impossible case could emit
a BUG.

Hope that helps,
Jonathan

Re: [PATCH] simplifying branch.c:install_branch_config() if()

[Nemina Amarasinghe]

 Is this safe?  Today branch.c::create_branch 
checks that the argument
> to e.g. --set-upstream-to is either in 
refs/heads/* or the image of
> some remote, but what is making sure that remains 
true tomorrow?
> 
> So if changing this, I would be happier if the 
"if" condition were
> still (!remote_is_branch && origin) so the 
impossible case could emit
> a BUG.
> 
> Hope that helps,
> Jonathan
> 

Thanks for the coments Jonathan,
Yes you are correct... I was just thought about how 
to simplify this chain of if() statement. Not the 
big picture. Now I understand when I change or 
implenet something I have to think not only about 
the current matter but abot the future also.

Nemina