Re: [PATCH] simplified the chain if() statements of install_branch_config() function in branch.c
On Tue, Mar 11, 2014 at 2:30 AM, Nemina Amarasinghe wrote:
> Eric Sunshine sunshineco.com> writes:
>>
>> On Mon, Mar 10, 2014 at 3:58 AM, Nemina Amarasinghe
> gmail.com> wrote:
>> > Nemina Amarasinghe gmail.com> writes:
>> >
>> > Sorry for the first patch. Something went wrong. This is the correct one
>>
>> In addition to the tautological issue pointed out by Matthieu, please
>> note that this version of the patch is not the correct one. It won't
>> apply to the git code. At a guess, it appears that this patch is
>> against some other modification you made to the git code before this
>> change, or perhaps you committed it incorrectly. In any event, when
>> you resubmit, please be sure that the new version can be applied to
>> commit.c as it exists in git.git itself.
>>
>
> Thank you very much for your comments Eric. I will resubmit the patch.
>
> Just a quick question
>
> in this code
>
> if (flag & BRANCH_CONFIG_VERBOSE) {
> if (remote_is_branch && origin)
> printf_ln(rebasing ?
> _("Branch %s set up to track remote branch %s from %s by
> rebasing.") :
> _("Branch %s set up to track remote branch %s from %s."),
> local, shortname, origin);
> else if (remote_is_branch && !origin)
> printf_ln(rebasing ?
> _("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)
> 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 remote ref %s by rebasing.") :
> _("Branch %s set up to track remote ref %s."),
> local, remote);
> else
> die("BUG: impossible combination of %d and %p",
> remote_is_branch, origin);
> }
This code does not exist in git.git. Somehow, the code you are
consulting has incorrectly substituted 'remote' for 'local' in the
strings of the (!remote_is_branch && !origin) case. Whether this is
due to a local modification you made or some some other reason, it is
leading you astray in your proposed changes and breaking your patches
so that they cannot be applied to git.git.
Probably easiest at this point would be for you to start from scratch
by making a new clone of git.git and examining branch.c as it actually
exists in the 'master' branch.
> These "local" and "remote" variables are independent from the "origin" right?
> So, If that the case couldn't we just use the bellow function
>
>
> 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);
>
> instead of
>
> else if (!remote_is_branch && origin)
> 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 remote ref %s by rebasing.") :
> _("Branch %s set up to track remote ref %s."),
> local, remote);
>
> Thanks
> 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
--
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] simplified the chain if() statements of install_branch_config() function in branch.c
Eric Sunshine sunshineco.com> writes:
>
> On Mon, Mar 10, 2014 at 3:58 AM, Nemina Amarasinghe
gmail.com> wrote:
> > Nemina Amarasinghe gmail.com> writes:
> >
> > Sorry for the first patch. Something went wrong. This is the correct one
>
> In addition to the tautological issue pointed out by Matthieu, please
> note that this version of the patch is not the correct one. It won't
> apply to the git code. At a guess, it appears that this patch is
> against some other modification you made to the git code before this
> change, or perhaps you committed it incorrectly. In any event, when
> you resubmit, please be sure that the new version can be applied to
> commit.c as it exists in git.git itself.
>
Thank you very much for your comments Eric. I will resubmit the patch.
Just a quick question
in this code
if (flag & BRANCH_CONFIG_VERBOSE) {
if (remote_is_branch && origin)
printf_ln(rebasing ?
_("Branch %s set up to track remote branch %s from %s by
rebasing.") :
_("Branch %s set up to track remote branch %s from %s."),
local, shortname, origin);
else if (remote_is_branch && !origin)
printf_ln(rebasing ?
_("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)
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 remote ref %s by rebasing.") :
_("Branch %s set up to track remote ref %s."),
local, remote);
else
die("BUG: impossible combination of %d and %p",
remote_is_branch, origin);
}
These "local" and "remote" variables are independent from the "origin" right?
So, If that the case couldn't we just use the bellow function
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);
instead of
else if (!remote_is_branch && origin)
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 remote ref %s by rebasing.") :
_("Branch %s set up to track remote ref %s."),
local, remote);
Thanks
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
Re: [PATCH] simplified the chain if() statements of install_branch_config() function in branch.c
On Mon, Mar 10, 2014 at 3:58 AM, Nemina Amarasinghe wrote:
> Nemina Amarasinghe gmail.com> writes:
>
> Sorry for the first patch. Something went wrong. This is the correct one
In addition to the tautological issue pointed out by Matthieu, please
note that this version of the patch is not the correct one. It won't
apply to the git code. At a guess, it appears that this patch is
against some other modification you made to the git code before this
change, or perhaps you committed it incorrectly. In any event, when
you resubmit, please be sure that the new version can be applied to
commit.c as it exists in git.git itself.
>
> From aebfa60feb643280c89b54e5ab87f9d960cde452 Mon Sep 17 00:00:00 2001
> From: Nemina Amarasinghe
> Date: Mon, 10 Mar 2014 13:02:55 +0530
> Subject: [PATCH] simplified the chain if() statements of
> install_brach_config() function in branch.c
>
> ---
> branch.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/branch.c b/branch.c
> index d3b9d49..0304a7a 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -87,12 +87,7 @@ 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)
> - 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)
> + else if (!remote_is_branch && (origin || !origin))
> printf_ln(rebasing ?
> _("Branch %s set up to track remote ref %s
> by rebasing.") :
> _("Branch %s set up to track remote ref
> %s."),
> --
> 1.9.0.152.g6ab4ae2
>
>
> --
> 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
--
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] simplified the chain if() statements of install_branch_config() function in branch.c
Nemina Amarasinghe writes: >> > ((!remote_is_branch && origin) || (!remote_is_branch || !origin)) >> >> Is it? >> >> The above is the same as (!remote_is_branch || !origin). What you wrote >> before is the same as (!remote_is_branch). >> >> Maybe you should try copy&paste from the expressions you are trying to >> combine to make sure that what you start with makes sense. >> > OMG.. Really sorry for that... that was a silly mistake. > This is the one.. > > ((!remote_is_branch && origin) || (!remote_is_branch && !origin)) That is, indeed, perfectly equivalent to (!remote_is_branch). If you write (!remote_is_branch && (origin || !origin)) then you will have people (and possibly also the compiler) loudly wondering about what you are trying to say here. The suspicion would be that either this is a result of a typo or is supposed to be an annoyingly obtuse replacement for a /* TODO: treat origin and !origin differently */ kind of comment. -- David Kastrup -- 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] simplified the chain if() statements of install_branch_config() function in branch.c
> > ((!remote_is_branch && origin) || (!remote_is_branch || !origin)) > > Is it? > > The above is the same as (!remote_is_branch || !origin). What you wrote > before is the same as (!remote_is_branch). > > Maybe you should try copy&paste from the expressions you are trying to > combine to make sure that what you start with makes sense. > OMG.. Really sorry for that... that was a silly mistake. This is the one.. ((!remote_is_branch && origin) || (!remote_is_branch && !origin)) -- 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] simplified the chain if() statements of install_branch_config() function in branch.c
Nemina Amarasinghe writes: >> >> Nemina Amarasinghe gmail.com> writes: >> >> Is it me, or is (origin || !origin) a tautology? >> > Thanks for the advices Matthieu. I will go through the documentations again. > Is there anything wrong with my logic? > What I wanted to express is > ((!remote_is_branch && origin) || (!remote_is_branch || !origin)) Is it? The above is the same as (!remote_is_branch || !origin). What you wrote before is the same as (!remote_is_branch). Maybe you should try copy&paste from the expressions you are trying to combine to make sure that what you start with makes sense. -- David Kastrup -- 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] simplified the chain if() statements of install_branch_config() function in branch.c
Nemina Amarasinghe writes: >> >> Nemina Amarasinghe gmail.com> writes: >> >> Is it me, or is (origin || !origin) a tautology? >> > Thanks for the advices Matthieu. I will go through the documentations again. > Is there anything wrong with my logic? > What I wanted to express is > ((!remote_is_branch && origin) || (!remote_is_branch || !origin)) (The second || should be a && in this sentence) I'm not saying your rewrite is incorrect, but there's no reason to keep (origin || !origin) in a logical formula. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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] simplified the chain if() statements of install_branch_config() function in branch.c
> > Nemina Amarasinghe gmail.com> writes: > > Is it me, or is (origin || !origin) a tautology? > Thanks for the advices Matthieu. I will go through the documentations again. Is there anything wrong with my logic? What I wanted to express is ((!remote_is_branch && origin) || (!remote_is_branch || !origin)) -- 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] simplified the chain if() statements of install_branch_config() function in branch.c
Matthieu Moy writes:
> Nemina Amarasinghe writes:
>
>> Nemina Amarasinghe gmail.com> writes:
>>
>> Sorry for the first patch. Something went wrong. This is the correct one
>
> Please, re-read Documentation/SubmittingPatches. In short, don't inline
> patch headers and don't forget the sign-off.
>
>> Subject: [PATCH] simplified the chain if() statements of
>> install_brach_config() function in branch.c
>
> Keep the subject line short (ideally <50 characters), and avoid past
> tense. We usually use imperative (the patch asks the codebase to
> refactor itself => "simplify if statements ...". We usually prefix the
> subject line with the place/subsystem where the change is done =>
> "branch.c: simplify if ...".
>
>> -else if (!remote_is_branch && origin)
>> -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)
>> +else if (!remote_is_branch && (origin || !origin))
>
> Is it me, or is (origin || !origin) a tautology?
Since it _is_ you, that question is a tautology because of its first
part already. But the second one would be just as good.
--
David Kastrup
--
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] simplified the chain if() statements of install_branch_config() function in branch.c
Nemina Amarasinghe writes:
> Nemina Amarasinghe gmail.com> writes:
>
> Sorry for the first patch. Something went wrong. This is the correct one
Please, re-read Documentation/SubmittingPatches. In short, don't inline
patch headers and don't forget the sign-off.
> Subject: [PATCH] simplified the chain if() statements of
> install_brach_config() function in branch.c
Keep the subject line short (ideally <50 characters), and avoid past
tense. We usually use imperative (the patch asks the codebase to
refactor itself => "simplify if statements ...". We usually prefix the
subject line with the place/subsystem where the change is done =>
"branch.c: simplify if ...".
> - else if (!remote_is_branch && origin)
> - 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)
> + else if (!remote_is_branch && (origin || !origin))
Is it me, or is (origin || !origin) a tautology?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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] simplified the chain if() statements of install_branch_config() function in branch.c
Nemina Amarasinghe gmail.com> writes:
Sorry for the first patch. Something went wrong. This is the correct one
>From aebfa60feb643280c89b54e5ab87f9d960cde452 Mon Sep 17 00:00:00 2001
From: Nemina Amarasinghe
Date: Mon, 10 Mar 2014 13:02:55 +0530
Subject: [PATCH] simplified the chain if() statements of
install_brach_config() function in branch.c
---
branch.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/branch.c b/branch.c
index d3b9d49..0304a7a 100644
--- a/branch.c
+++ b/branch.c
@@ -87,12 +87,7 @@ 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)
- 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)
+ else if (!remote_is_branch && (origin || !origin))
printf_ln(rebasing ?
_("Branch %s set up to track remote ref %s by
rebasing.") :
_("Branch %s set up to track remote ref %s."),
--
1.9.0.152.g6ab4ae2
--
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
[PATCH] simplified the chain if() statements of install_branch_config() function in branch.c
Did some simple changing to the chain of if() statements in function
install_branch_config() of branch.c
This was a micro-project for GSOC 2014
>From aebfa60feb643280c89b54e5ab87f9d960cde452 Mon Sep 17 00:00:00 2001
From: Nemina Amarasinghe
Date: Mon, 10 Mar 2014 13:02:55 +0530
Subject: [PATCH] simplified the chain if() statements of
install_branch_config() function in branch.c
---
branch.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/branch.c b/branch.c
index d3b9d49..0304a7a 100644
--- a/branch.c
+++ b/branch.c
@@ -87,7 +87,12 @@ 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 || !origin))
+ else if (!remote_is_branch && origin)
+ 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 remote ref %s by
rebasing.") :
_("Branch %s set up to track remote ref %s."),
--
1.9.0.152.g6ab4ae2
--
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
