threads / patch / 36106

patchsimplified the chain if() statements of install_branch_config() function in branch.c

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

## tl;dr

14 messages between Mar 10, 2014 and Mar 11, 2014. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

Nemina Amarasinghe· Mar 10, 2014, 07:39 UTC · lore

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(-)
Show changes to branch.c +6 −1
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
Nemina Amarasinghe· Mar 10, 2014, 07:58 UTC · re: Nemina Amarasinghe · lore

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

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(-)
Show changes to branch.c +1 −6
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
Matthieu Moy· Mar 10, 2014, 08:23 UTC · re: Nemina Amarasinghe · lore

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

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 ...".

Show 7 quoted lines
> -		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/
David Kastrup· Mar 10, 2014, 08:36 UTC · re: Matthieu Moy · lore

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

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 27 quoted lines
> 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
Nemina Amarasinghe· Mar 10, 2014, 08:51 UTC · re: Matthieu Moy · lore

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

Show 5 quoted lines
> 
> 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))

Matthieu Moy· Mar 10, 2014, 09:05 UTC · re: Nemina Amarasinghe · lore

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

Nemina Amarasinghe <neminaa@gmail.com> writes:
Show 9 quoted lines
>> 
>> 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/
David Kastrup· Mar 10, 2014, 09:08 UTC · re: Nemina Amarasinghe · lore

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

Nemina Amarasinghe <neminaa@gmail.com> writes:
Show 9 quoted lines
>> 
>> 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
Nemina Amarasinghe· Mar 10, 2014, 09:15 UTC · re: David Kastrup · lore

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

Show 10 quoted lines
> > ((!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))
David Kastrup· Mar 10, 2014, 09:27 UTC · re: Nemina Amarasinghe · lore

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

Nemina Amarasinghe <neminaa@gmail.com> writes:
Show 14 quoted lines
>> > ((!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
Eric Sunshine· Mar 10, 2014, 20:46 UTC · re: Nemina Amarasinghe · lore

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 <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.

Show 38 quoted lines
>
> 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
Nemina Amarasinghe· Mar 11, 2014, 06:30 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine <at> sunshineco.com> writes:
> 
> On Mon, Mar 10, 2014 at 3:58 AM, Nemina Amarasinghe <neminaa <at>
gmail.com> wrote:
Show 12 quoted lines
> > 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

Eric Sunshine· Mar 11, 2014, 08:06 UTC · re: Nemina Amarasinghe · lore

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 <neminaa@gmail.com> wrote:
Show 49 quoted lines
> 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.

Show 32 quoted lines
> 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
Nemina Amarasinghe· Mar 11, 2014, 07:16 UTC · re: Eric Sunshine · lore

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

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(-)
Show changes to branch.c +1 −6
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
Eric Sunshine· Mar 11, 2014, 08:11 UTC · re: Nemina Amarasinghe · lore

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

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.

Show 24 quoted lines
> 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

← back to recent threads