threads / patch / 36154

patchsimplifying branch.c:install_branch_config() if()

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

## tl;dr

3 messages between Mar 14, 2014 and Mar 14, 2014. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Nemina Amarasinghe· Mar 14, 2014, 01:45 UTC · lore

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

Show changes to branch.c +1 −6

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

Jonathan Nieder· Mar 14, 2014, 02:10 UTC · re: Nemina Amarasinghe · lore

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

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.)

[...]

Show 17 quoted lines

> --- 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

Nemina Amarasinghe· Mar 14, 2014, 04:22 UTC · re: Jonathan Nieder · lore

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

 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

Show 5 quoted lines

> 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