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

[Jonathan Nieder]

Hi,

Nemina Amarasinghe wrote:

 Signed-off-by: Nemina Amarasinghe nemi...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html