From: Jonathan Nieder <jrnieder@gmail.com>
To: Nemina Amarasinghe <neminaa@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] simplifying branch.c:install_branch_config() if()
Date: Thu, 13 Mar 2014 19:10:52 -0700 [thread overview]
Message-ID: <20140314021052.GH15625@google.com> (raw)
In-Reply-To: <1394761558-4888-1-git-send-email-neminaa@gmail.com>
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
next prev parent reply other threads:[~2014-03-14 2:11 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-03-14 1:45 [PATCH] simplifying branch.c:install_branch_config() if() Nemina Amarasinghe
2014-03-14 2:10 ` Jonathan Nieder [this message]
2014-03-14 4:22 ` Nemina Amarasinghe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140314021052.GH15625@google.com \
--to=jrnieder@gmail.com \
--cc=git@vger.kernel.org \
--cc=neminaa@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).