{"thread":{"id":"36154","subject":"[PATCH] simplifying branch.c:install_branch_config() if()","startedAt":"2014-03-14T01:45:58Z","lastAt":"2014-03-14T04:22:56Z","messageCount":3,"participants":["Nemina Amarasinghe","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"236704","messageId":"1394761558-4888-1-git-send-email-neminaa@gmail.com","threadId":"36154","inReplyTo":null,"subject":"[PATCH] simplifying branch.c:install_branch_config() if()","fromName":"Nemina Amarasinghe","fromEmail":"neminaa@gmail.com","sentAt":"2014-03-14T01:45:58Z","receivedAt":"2014-03-14T01:45:58Z","isPatch":true,"sender":{"key":"neminaa@gmail.com","avatar":null},"body":"Signed-off-by: Nemina Amarasinghe <neminaa@gmail.com>\n---\n branch.c | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 723a36b..fd93603 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -87,16 +87,11 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n \t\t\t\t  _(\"Branch %s set up to track local branch %s by rebasing.\") :\n \t\t\t\t  _(\"Branch %s set up to track local branch %s.\"),\n \t\t\t\t  local, shortname);\n-\t\telse if (!remote_is_branch && origin)\n+\t\telse if (!remote_is_branch)\n \t\t\tprintf_ln(rebasing ?\n \t\t\t\t  _(\"Branch %s set up to track remote ref %s by rebasing.\") :\n \t\t\t\t  _(\"Branch %s set up to track remote ref %s.\"),\n \t\t\t\t  local, remote);\n-\t\telse if (!remote_is_branch && !origin)\n-\t\t\tprintf_ln(rebasing ?\n-\t\t\t\t  _(\"Branch %s set up to track local ref %s by rebasing.\") :\n-\t\t\t\t  _(\"Branch %s set up to track local ref %s.\"),\n-\t\t\t\t  local, remote);\n \t\telse\n \t\t\tdie(\"BUG: impossible combination of %d and %p\",\n \t\t\t    remote_is_branch, origin);\n-- \n1.9.0.152.g6ab4ae2\n"},{"id":"236705","messageId":"20140314021052.GH15625@google.com","threadId":"36154","inReplyTo":"1394761558-4888-1-git-send-email-neminaa@gmail.com","subject":"Re: [PATCH] simplifying branch.c:install_branch_config() if()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-03-14T02:10:52Z","receivedAt":"2014-03-14T02:10:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nNemina Amarasinghe wrote:\n\n> Signed-off-by: Nemina Amarasinghe <neminaa@gmail.com>\n\nThe above is a place to explain why this is a good change.  For example:\n\n\tWhen it prints a message indicating what it has done,\n\tinstall_branch_config() treats the (!remote_is_branch && origin)\n\tand (!remote_is_branch && !origin) cases separately.  But they\n\tare the same, and it uses the same message for both.\n\n\tSimplify by just checking for !remote_is_branch.\n\n\tNoticed using the magic-identical-branch-checker tool.\n\n\tSigned-off-by: ...\n\n(That's just a first random hypothetical guess --- of course please do\nnot use it but put your own rationale for the change there.)\n\n[...]\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -87,16 +87,11 @@ void install_branch_config(int flag, const char *local, const char *origin, cons\n>  \t\t\t\t  _(\"Branch %s set up to track local branch %s by rebasing.\") :\n>  \t\t\t\t  _(\"Branch %s set up to track local branch %s.\"),\n>  \t\t\t\t  local, shortname);\n> -\t\telse if (!remote_is_branch && origin)\n> +\t\telse if (!remote_is_branch)\n>  \t\t\tprintf_ln(rebasing ?\n>  \t\t\t\t  _(\"Branch %s set up to track remote ref %s by rebasing.\") :\n>  \t\t\t\t  _(\"Branch %s set up to track remote ref %s.\"),\n>  \t\t\t\t  local, remote);\n> -\t\telse if (!remote_is_branch && !origin)\n> -\t\t\tprintf_ln(rebasing ?\n> -\t\t\t\t  _(\"Branch %s set up to track local ref %s by rebasing.\") :\n> -\t\t\t\t  _(\"Branch %s set up to track local ref %s.\"),\n> -\t\t\t\t  local, remote);\n\nIs this safe?  Today branch.c::create_branch checks that the argument\nto e.g. --set-upstream-to is either in refs/heads/* or the image of\nsome remote, but what is making sure that remains true tomorrow?\n\nSo if changing this, I would be happier if the \"if\" condition were\nstill (!remote_is_branch && origin) so the impossible case could emit\na BUG.\n\nHope that helps,\nJonathan\n"},{"id":"236710","messageId":"loom.20140314T051213-863@post.gmane.org","threadId":"36154","inReplyTo":"20140314021052.GH15625@google.com","subject":"Re: [PATCH] simplifying branch.c:install_branch_config() if()","fromName":"Nemina Amarasinghe","fromEmail":"neminaa@gmail.com","sentAt":"2014-03-14T04:22:56Z","receivedAt":"2014-03-14T04:22:56Z","isPatch":true,"sender":{"key":"neminaa@gmail.com","avatar":null},"body":" Is this safe?  Today branch.c::create_branch \nchecks that the argument\n> to e.g. --set-upstream-to is either in \nrefs/heads/* or the image of\n> some remote, but what is making sure that remains \ntrue tomorrow?\n> \n> So if changing this, I would be happier if the \n\"if\" condition were\n> still (!remote_is_branch && origin) so the \nimpossible case could emit\n> a BUG.\n> \n> Hope that helps,\n> Jonathan\n> \n\nThanks for the coments Jonathan,\nYes you are correct... I was just thought about how \nto simplify this chain of if() statement. Not the \nbig picture. Now I understand when I change or \nimplenet something I have to think not only about \nthe current matter but abot the future also.\n\nNemina\n"}]}