# [PATCH] simplifying branch.c:install_branch_config() if() 3 messages from 2014-03-14 to 2014-03-14. Participants: Nemina Amarasinghe, Jonathan Nieder. Thread: https://gitlist.dev/t/36154 ## Nemina Amarasinghe, 2014-03-14 01:45 Subject: [PATCH] simplifying branch.c:install_branch_config() if() Message-ID: <1394761558-4888-1-git-send-email-neminaa@gmail.com> URL: https://gitlist.dev/e/1394761558-4888-1-git-send-email-neminaa%40gmail.com ``` Signed-off-by: Nemina Amarasinghe --- 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 ``` ## Jonathan Nieder, 2014-03-14 02:10 Subject: Re: [PATCH] simplifying branch.c:install_branch_config() if() Message-ID: <20140314021052.GH15625@google.com> URL: https://gitlist.dev/e/20140314021052.GH15625%40google.com In-Reply-To: <1394761558-4888-1-git-send-email-neminaa@gmail.com> ``` Hi, Nemina Amarasinghe wrote: > Signed-off-by: Nemina Amarasinghe 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 ``` ## Nemina Amarasinghe, 2014-03-14 04:22 Subject: Re: [PATCH] simplifying branch.c:install_branch_config() if() Message-ID: URL: https://gitlist.dev/e/loom.20140314T051213-863%40post.gmane.org In-Reply-To: <20140314021052.GH15625@google.com> ``` 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 ```