[PATCH] simplified the chain if() statements of install_branch_config() function in branch.c

[PATCH] simplified the chain if() statements of install_branch_config() function in branch.c

[Nemina Amarasinghe]

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 <neminaa@gmail.com>
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

Re: [PATCH] simplified the chain if() statements of install_branch_config() function in branch.c

[Nemina Amarasinghe]

Nemina Amarasinghe <neminaa <at> 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 <neminaa@gmail.com>
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

Re: [PATCH] simplified the chain if() statements of install_branch_config() function in branch.c

[Matthieu Moy]

Nemina Amarasinghe <neminaa@gmail.com> writes:

> Nemina Amarasinghe <neminaa <at> 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/

Re: [PATCH] simplified the chain if() statements of install_branch_config() function in branch.c

[David Kastrup]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Nemina Amarasinghe <neminaa@gmail.com> writes:
>
>> Nemina Amarasinghe <neminaa <at> 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

Re: [PATCH] simplified the chain if() statements of install_branch_config() function in branch.c

[Nemina Amarasinghe]

> 
> Nemina Amarasinghe <neminaa <at> 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))

Re: [PATCH] simplified the chain if() statements of install_branch_config() function in branch.c

[Matthieu Moy]

Nemina Amarasinghe <neminaa@gmail.com> writes:

>> 
>> Nemina Amarasinghe <neminaa <at> 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/

Re: [PATCH] simplified the chain if() statements of install_branch_config() function in branch.c

[David Kastrup]

Nemina Amarasinghe <neminaa@gmail.com> writes:

>> 
>> Nemina Amarasinghe <neminaa <at> 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

Re: [PATCH] simplified the chain if() statements of install_branch_config() function in branch.c

[Nemina Amarasinghe]

> > ((!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))

Re: [PATCH] simplified the chain if() statements of install_branch_config() function in branch.c

[David Kastrup]

Nemina Amarasinghe <neminaa@gmail.com> 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

Re: [PATCH] simplified the chain if() statements of install_branch_config() function in branch.c

[Eric Sunshine]

On Mon, Mar 10, 2014 at 3:58 AM, Nemina Amarasinghe <neminaa@gmail.com> wrote:
> Nemina Amarasinghe <neminaa <at> 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 <neminaa@gmail.com>
> 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 majordomo@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]

Eric Sunshine <sunshine <at> sunshineco.com> writes:

> 
> On Mon, Mar 10, 2014 at 3:58 AM, Nemina Amarasinghe <neminaa <at>
gmail.com> wrote:
> > Nemina Amarasinghe <neminaa <at> 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  

Re: [PATCH] simplified the chain if() statements of install_branch_config() function in branch.c

[Eric Sunshine]

On Tue, Mar 11, 2014 at 2:30 AM, Nemina Amarasinghe <neminaa@gmail.com> wrote:
> Eric Sunshine <sunshine <at> sunshineco.com> writes:
>>
>> On Mon, Mar 10, 2014 at 3:58 AM, Nemina Amarasinghe <neminaa <at>
> gmail.com> wrote:
>> > Nemina Amarasinghe <neminaa <at> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][GSoC]simplified branch.c:install_branch_config() if() statement

[Nemina Amarasinghe]

I hope this is the correct format for  patch. Please comment on this if
something is wrong. 

Signed-off-by:Nemina Amarasinghe <neminaa@gmail.com>
---
 branch.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/branch.c b/branch.c
index 0304a7a..fd93603 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)
 			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

Re: [PATCH][GSoC]simplified branch.c:install_branch_config() if() statement

[Eric Sunshine]

On Tue, Mar 11, 2014 at 3:16 AM, Nemina Amarasinghe <neminaa@gmail.com> wrote:
> Subject: simplified branch.c:install_branch_config() if() statement

Use imperative tone: "simplify ..."

> I hope this is the correct format for  patch. Please comment on this if
> something is wrong.

This commentary is relevant to the email discussion but not to the
commit description, so it should be placed below the "---" line just
under your sign-0ff.

> Signed-off-by:Nemina Amarasinghe <neminaa@gmail.com>
> ---
>  branch.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 0304a7a..fd93603 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)
>                         printf_ln(rebasing ?
>                                   _("Branch %s set up to track remote ref %s by rebasing.") :
>                                   _("Branch %s set up to track remote ref %s."),

The patch itself is broken in a couple ways.

First, it is whitespace-damaged, possibly due to being pasted into
your email client. Using "git send-email" can help avoid this problem.

Second, the code against which this patch was made does not exist in
git.git. You are likely making this change atop some other local
modifications which you already made. Simplest at this point would
probably be for you to make a fresh clone of git.git and start from
scratch by editing branch.c in the 'master' branch.

> --
> 1.9.0.152.g6ab4ae2