[PATCH 00/16] Trivial patches

[PATCH 00/16] Trivial patches

[Felipe Contreras]

Most of these have been sent before, but were not applied for one reason or
another.

Felipe Contreras (16):
  merge: simplify ff-only option
  t: replace pulls with merges
  pull: cleanup documentation
  fetch: add missing documentation
  revision: add missing include
  shortlog: add missing declaration
  branch: trivial style fix
  sha1-name: trivial style cleanup
  transport-helper: trivial style fix
  describe: trivial style fixes
  pretty: trivial style fix
  revision: trivial style fixes
  diff: trivial style fix
  run-command: trivial style fixes
  setup: trivial style fixes
  add: avoid yoda conditions

 Documentation/git-fetch.txt            |  3 +++
 Documentation/git-pull.txt             |  4 ++--
 builtin/add.c                          |  2 +-
 builtin/branch.c                       |  3 +--
 builtin/describe.c                     |  7 +++----
 builtin/diff.c                         |  2 +-
 builtin/merge.c                        | 11 ++---------
 pretty.c                               |  2 +-
 revision.c                             | 14 ++++++--------
 revision.h                             |  1 +
 run-command.c                          | 13 +++++--------
 setup.c                                |  4 ++--
 sha1_name.c                            |  1 -
 shortlog.h                             |  2 ++
 t/annotate-tests.sh                    |  2 +-
 t/t4200-rerere.sh                      |  2 +-
 t/t9114-git-svn-dcommit-merge.sh       |  2 +-
 t/t9500-gitweb-standalone-no-errors.sh |  2 +-
 transport-helper.c                     |  1 +
 19 files changed, 35 insertions(+), 43 deletions(-)

-- 
1.8.4.2+fc1

[PATCH 01/16] merge: simplify ff-only option

[Felipe Contreras]

No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/merge.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 02a69c1..41fb66d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -186,13 +186,6 @@ static int option_parse_n(const struct option *opt,
 	return 0;
 }
 
-static int option_parse_ff_only(const struct option *opt,
-			  const char *arg, int unset)
-{
-	fast_forward = FF_ONLY;
-	return 0;
-}
-
 static struct option builtin_merge_options[] = {
 	{ OPTION_CALLBACK, 'n', NULL, NULL, NULL,
 		N_("do not show a diffstat at the end of the merge"),
@@ -210,9 +203,9 @@ static struct option builtin_merge_options[] = {
 	OPT_BOOL('e', "edit", &option_edit,
 		N_("edit message before committing")),
 	OPT_SET_INT(0, "ff", &fast_forward, N_("allow fast-forward (default)"), FF_ALLOW),
-	{ OPTION_CALLBACK, 0, "ff-only", NULL, NULL,
+	{ OPTION_SET_INT, 0, "ff-only", &fast_forward, NULL,
 		N_("abort if fast-forward is not possible"),
-		PARSE_OPT_NOARG | PARSE_OPT_NONEG, option_parse_ff_only },
+		PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, FF_ONLY },
 	OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
 	OPT_BOOL(0, "verify-signatures", &verify_signatures,
 		N_("Verify that the named commit has a valid GPG signature")),
-- 
1.8.4.2+fc1

Re: [PATCH 01/16] merge: simplify ff-only option

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> No functional changes.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/merge.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 02a69c1..41fb66d 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -186,13 +186,6 @@ static int option_parse_n(const struct option *opt,
>  	return 0;
>  }
>  
> -static int option_parse_ff_only(const struct option *opt,
> -			  const char *arg, int unset)
> -{
> -	fast_forward = FF_ONLY;
> -	return 0;
> -}
> -
>  static struct option builtin_merge_options[] = {
>  	{ OPTION_CALLBACK, 'n', NULL, NULL, NULL,
>  		N_("do not show a diffstat at the end of the merge"),
> @@ -210,9 +203,9 @@ static struct option builtin_merge_options[] = {
>  	OPT_BOOL('e', "edit", &option_edit,
>  		N_("edit message before committing")),
>  	OPT_SET_INT(0, "ff", &fast_forward, N_("allow fast-forward (default)"), FF_ALLOW),
> -	{ OPTION_CALLBACK, 0, "ff-only", NULL, NULL,
> +	{ OPTION_SET_INT, 0, "ff-only", &fast_forward, NULL,
>  		N_("abort if fast-forward is not possible"),
> -		PARSE_OPT_NOARG | PARSE_OPT_NONEG, option_parse_ff_only },
> +		PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, FF_ONLY },
>  	OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
>  	OPT_BOOL(0, "verify-signatures", &verify_signatures,
>  		N_("Verify that the named commit has a valid GPG signature")),

Looks good; thanks.

[PATCH 02/16] t: replace pulls with merges

[Felipe Contreras]

This is what the code intended.

No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/annotate-tests.sh                    | 2 +-
 t/t4200-rerere.sh                      | 2 +-
 t/t9114-git-svn-dcommit-merge.sh       | 2 +-
 t/t9500-gitweb-standalone-no-errors.sh | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index 99caa42..c9d105d 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -92,7 +92,7 @@ test_expect_success 'blame 2 authors + 1 branch2 author' '
 '
 
 test_expect_success 'merge branch1 & branch2' '
-	git pull . branch1
+	git merge branch1
 '
 
 test_expect_success 'blame 2 authors + 2 merged-in authors' '
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 7f6666f..cf19eb7 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -172,7 +172,7 @@ test_expect_success 'first postimage wins' '
 	git show second^:a1 | sed "s/To die: t/To die! T/" >a1 &&
 	git commit -q -a -m third &&
 
-	test_must_fail git pull . first &&
+	test_must_fail git merge first &&
 	# rerere kicked in
 	! grep "^=======\$" a1 &&
 	test_cmp expect a1
diff --git a/t/t9114-git-svn-dcommit-merge.sh b/t/t9114-git-svn-dcommit-merge.sh
index f524d2f..d33d714 100755
--- a/t/t9114-git-svn-dcommit-merge.sh
+++ b/t/t9114-git-svn-dcommit-merge.sh
@@ -62,7 +62,7 @@ test_expect_success 'setup git mirror and merge' '
 	echo friend > README &&
 	cat tmp >> README &&
 	git commit -a -m "friend" &&
-	git pull . merge
+	git merge merge
 	'
 
 test_debug 'gitk --all & sleep 1'
diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index 718014d..e74b9ab 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -328,7 +328,7 @@ test_expect_success \
 	 git add b &&
 	 git commit -a -m "On branch" &&
 	 git checkout master &&
-	 git pull . b &&
+	 git merge b &&
 	 git tag merge_commit'
 
 test_expect_success \
-- 
1.8.4.2+fc1

[PATCH 03/16] pull: cleanup documentation

[Felipe Contreras]

'origin/master' is very clear, no need to specify the 'remotes/' prefix,
or babysit the user.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-pull.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index beea10b..03a39bc 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -39,7 +39,7 @@ Assume the following history exists and the current branch is
 "`master`":
 
 ------------
-	  A---B---C master on origin
+	  A---B---C origin/master
 	 /
     D---E---F---G master
 ------------
@@ -51,7 +51,7 @@ result in a new commit along with the names of the two parent commits
 and a log message from the user describing the changes.
 
 ------------
-	  A---B---C remotes/origin/master
+	  A---B---C origin/master
 	 /         \
     D---E---F---G---H master
 ------------
-- 
1.8.4.2+fc1

Re: [PATCH 03/16] pull: cleanup documentation

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> 'origin/master' is very clear, no need to specify the 'remotes/' prefix,
> or babysit the user.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/git-pull.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index beea10b..03a39bc 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -39,7 +39,7 @@ Assume the following history exists and the current branch is
>  "`master`":
>  
>  ------------
> -	  A---B---C master on origin
> +	  A---B---C origin/master
>  	 /
>      D---E---F---G master
>  ------------

This change is wrong; the illustration depicts the distributed world
(i.e. a fetch has not happened yet).  The next sentence after this
picture reads:

    Then "`git pull`" will fetch and replay the changes from the remote
    `master` branch since it diverged from the local `master` 

In other words, your (remotes/)origin/master has _not_ caught up to
the reality.

> @@ -51,7 +51,7 @@ result in a new commit along with the names of the two parent commits
>  and a log message from the user describing the changes.
>  
>  ------------
> -	  A---B---C remotes/origin/master
> +	  A---B---C origin/master
>  	 /         \
>      D---E---F---G---H master
>  ------------

This is a good change, especially in today's world.

Thanks.

Re: [PATCH 03/16] pull: cleanup documentation

[Felipe Contreras]

On Thu, Oct 31, 2013 at 12:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:

>> --- a/Documentation/git-pull.txt
>> +++ b/Documentation/git-pull.txt
>> @@ -39,7 +39,7 @@ Assume the following history exists and the current branch is
>>  "`master`":
>>
>>  ------------
>> -       A---B---C master on origin
>> +       A---B---C origin/master
>>        /
>>      D---E---F---G master
>>  ------------
>
> This change is wrong; the illustration depicts the distributed world
> (i.e. a fetch has not happened yet).

That is an irrelevant implementation detail, specially at this high
level. In the user's mind origin/master means master on origin.

If you want to be pedantic, this is the "reality":

  ------------
      D---E---F---G master
  ------------

-- 
Felipe Contreras

Re: [PATCH 03/16] pull: cleanup documentation

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Thu, Oct 31, 2013 at 12:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> --- a/Documentation/git-pull.txt
>>> +++ b/Documentation/git-pull.txt
>>> @@ -39,7 +39,7 @@ Assume the following history exists and the current branch is
>>>  "`master`":
>>>
>>>  ------------
>>> -       A---B---C master on origin
>>> +       A---B---C origin/master
>>>        /
>>>      D---E---F---G master
>>>  ------------
>>
>> This change is wrong; the illustration depicts the distributed world
>> (i.e. a fetch has not happened yet).
>
> That is an irrelevant implementation detail, specially at this high
> level. In the user's mind origin/master means master on origin.

You are wrong.  In the user's mind, origin/master means the commit
that used to be at master on origin, and the point of this
illustration is to make them understand that they live in a
distributed world, where their last observation will go stale over
time.

>
> If you want to be pedantic, this is the "reality":
>
>   ------------
>       D---E---F---G master
>   ------------

You are wrong again.  The "reality" is more like this:

      origin/master in your repository
      |
      v
      A---B---C master at origin
     /
    D---E---F---G master in your repository

if you really want to write origin/master somewhere in this
illustration.

Re: [PATCH 03/16] pull: cleanup documentation

[Max Horn]

[-- Attachment #1: Type: text/plain, Size: 846 bytes --]


On 31.10.2013, at 20:00, Junio C Hamano <gitster@pobox.com> wrote:

> Felipe Contreras <felipe.contreras@gmail.com> writes:

[...]

> 
>> 
>> If you want to be pedantic, this is the "reality":
>> 
>>  ------------
>>      D---E---F---G master
>>  ------------
> 
> You are wrong again.  The "reality" is more like this:
> 
>      origin/master in your repository
>      |
>      v
>      A---B---C master at origin
>     /
>    D---E---F---G master in your repository
> 
> if you really want to write origin/master somewhere in this
> illustration.

Actually, I kind of like that. After just reading the existing phrasing in git-pull.txt, I doubt that a newbie would catch the difference between "origin/master" and "master at origin". With this illustration, it's very clearly conveyed that there is a difference.


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

Re: [PATCH 03/16] pull: cleanup documentation

[Junio C Hamano]

Max Horn <max@quendi.de> writes:

>> ... The "reality" is more like this:
>> 
>>      origin/master in your repository
>>      |
>>      v
>>      A---B---C master at origin
>>     /
>>    D---E---F---G master in your repository
>> 
>> if you really want to write origin/master somewhere in this
>> illustration.
>
> Actually, I kind of like that. After just reading the existing
> phrasing in git-pull.txt, I doubt that a newbie would catch the
> difference between "origin/master" and "master at origin". With this
> illustration, it's very clearly conveyed that there is a difference.

Yeah, after re-reading the part of the documentation with the
illustration replaced with the above, I was coming to the same
conclusion.

Re: [PATCH 03/16] pull: cleanup documentation

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Max Horn <max@quendi.de> writes:
>
>>> ... The "reality" is more like this:
>>> 
>>>      origin/master in your repository
>>>      |
>>>      v
>>>      A---B---C master at origin
>>>     /
>>>    D---E---F---G master in your repository
>>> 
>>> if you really want to write origin/master somewhere in this
>>> illustration.
>>
>> Actually, I kind of like that. After just reading the existing
>> phrasing in git-pull.txt, I doubt that a newbie would catch the
>> difference between "origin/master" and "master at origin". With this
>> illustration, it's very clearly conveyed that there is a difference.
>
> Yeah, after re-reading the part of the documentation with the
> illustration replaced with the above, I was coming to the same
> conclusion.

As Felipe spotted in his response, "A" is not something you already
have, so the picture need to look like the amended patch below.

The other reason the original did not say "origin/master" is because
this holds true even if you do not have such a remote-tracking
branch for the master at the origin, but the illustration that shows
the history after "git pull" finishes spells remotes/origin/master
out, so I think it would be an improvement to make the two pictures
consistent by drawing where the origin/master is before this "git
pull" is run.

Something like this, perhaps.  Note that I think "on" sounds funny
and it should probably be "at" instead, but this weatherbaloon patch
does not change it.

-- >8 --
Subject: doc/pull: clarify the illustrations

The second illustration that shows the history after "git pull"
spelled the remote-tracking branch with "remotes/" prefix, which
is not necessary.  Drop it.

To match the assumption that a remote-tracking branch is used to
keep track of the advancement of the master at the origin, update
the first illustration that shows the history before "git pull"
to show the distinction between the master currently at origin and
the stale origin/master remote-tracking branch.

Noticed-by: Felipe Contreras <felipe.contreras@gmail.com>
Helped-by: Max Horn <max@quendi.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-pull.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index beea10b..e83f3ce 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -42,6 +42,8 @@ Assume the following history exists and the current branch is
 	  A---B---C master on origin
 	 /
     D---E---F---G master
+        ^
+	origin/master in your repository
 ------------
 
 Then "`git pull`" will fetch and replay the changes from the remote
@@ -51,7 +53,7 @@ result in a new commit along with the names of the two parent commits
 and a log message from the user describing the changes.
 
 ------------
-	  A---B---C remotes/origin/master
+	  A---B---C origin/master
 	 /         \
     D---E---F---G---H master
 ------------

Re: [PATCH 03/16] pull: cleanup documentation

[Felipe Contreras]

On Thu, Oct 31, 2013 at 2:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:

> The other reason the original did not say "origin/master" is because
> this holds true even if you do not have such a remote-tracking
> branch for the master at the origin, but the illustration that shows
> the history after "git pull" finishes spells remotes/origin/master
> out, so I think it would be an improvement to make the two pictures
> consistent by drawing where the origin/master is before this "git
> pull" is run.

So you care about "reality" when it fits your argument, but not when
it doesn't. Got it.

-- 
Felipe Contreras

Re: [PATCH 03/16] pull: cleanup documentation

[David Aguilar]

On Thu, Oct 31, 2013 at 03:16:57PM -0600, Felipe Contreras wrote:
> On Thu, Oct 31, 2013 at 2:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Junio C Hamano <gitster@pobox.com> writes:
> 
> > The other reason the original did not say "origin/master" is because
> > this holds true even if you do not have such a remote-tracking
> > branch for the master at the origin, but the illustration that shows
> > the history after "git pull" finishes spells remotes/origin/master
> > out, so I think it would be an improvement to make the two pictures
> > consistent by drawing where the origin/master is before this "git
> > pull" is run.
> 
> So you care about "reality" when it fits your argument, but not when
> it doesn't. Got it.

What exactly do these flippant remarks accomplish?
Keep these to yourself.  No one deserves this treatment nor does
anyone care to read it.
-- 
David

Re: [PATCH 03/16] pull: cleanup documentation

[Felipe Contreras]

On Thu, Oct 31, 2013 at 5:40 PM, David Aguilar <davvid@gmail.com> wrote:
> On Thu, Oct 31, 2013 at 03:16:57PM -0600, Felipe Contreras wrote:
>> On Thu, Oct 31, 2013 at 2:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> > Junio C Hamano <gitster@pobox.com> writes:
>>
>> > The other reason the original did not say "origin/master" is because
>> > this holds true even if you do not have such a remote-tracking
>> > branch for the master at the origin, but the illustration that shows
>> > the history after "git pull" finishes spells remotes/origin/master
>> > out, so I think it would be an improvement to make the two pictures
>> > consistent by drawing where the origin/master is before this "git
>> > pull" is run.
>>
>> So you care about "reality" when it fits your argument, but not when
>> it doesn't. Got it.
>
> What exactly do these flippant remarks accomplish?
> Keep these to yourself.  No one deserves this treatment nor does
> anyone care to read it.

Nobody is forcing you to read them.

However, they happen to be true. Junio used as an argument that
'origin/master' is not better than 'master on origin' because the
"real" 'origin/master' might be pointing to something else, however,
when he himself realized that 'origin/master' might not exist at all,
then suddenly the "real" 'origin/master' is not so important.

If this was a rational discussion I would ask you to point out where
exactly the previous explanation is incorrect, but alas, if experience
serves, this is not one of those.

The facts are very simple, Junio's proposal:

------------
          A---B---C master on origin
         /
    D---E---F---G master
        ^
       origin/master in your repository
------------

Assumes that:

1) The user did 'git pull origin', not 'git pull $url', or any other remote
2) 'origin/master' points to E, which might not be true, the user
might have issued a 'git fetch', or a previous pull might have been
cancelled

Both issues are overridden by the comment "Assume the following
history exists", so one has to assume that origin/master points to E,
but if one assumes that, then my proposal is also correct:

------------
         A---B---C origin/master
        /
    D---E---F---G master
------------

Because one has to assume that 'origin/master' points to C, which is
entirely possible. But more importantly, for the purposes of
explaining 'git pull' it lightens the mental load needed by the user.

Either Junio's argument applies to both proposals, or none, but
selectively cherry-picking where the argument applies is akin to a
feminist that argues men and women are equal, but men should pick the
check in a restaurant.

-- 
Felipe Contreras

Re: [PATCH 03/16] pull: cleanup documentation

[David Aguilar]

On Thu, Oct 31, 2013 at 07:56:03PM -0600, Felipe Contreras wrote:
> On Thu, Oct 31, 2013 at 5:40 PM, David Aguilar <davvid@gmail.com> wrote:
> > On Thu, Oct 31, 2013 at 03:16:57PM -0600, Felipe Contreras wrote:
> >> On Thu, Oct 31, 2013 at 2:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >> > Junio C Hamano <gitster@pobox.com> writes:
> >>
> >> > The other reason the original did not say "origin/master" is because
> >> > this holds true even if you do not have such a remote-tracking
> >> > branch for the master at the origin, but the illustration that shows
> >> > the history after "git pull" finishes spells remotes/origin/master
> >> > out, so I think it would be an improvement to make the two pictures
> >> > consistent by drawing where the origin/master is before this "git
> >> > pull" is run.
> >>
> >> So you care about "reality" when it fits your argument, but not when
> >> it doesn't. Got it.
> >
> > What exactly do these flippant remarks accomplish?
> > Keep these to yourself.  No one deserves this treatment nor does
> > anyone care to read it.
> 
> Nobody is forcing you to read them.

You're missing the *key* point.
No one wants to interact with a rude arrogant loudmouth.

You could have explained this without being sarcastic and annoying.
It does not help your cause.


Sure, these are "subjective" people skills (that don't "matter"
to the code) but this is a social activity.  If your defense is
to whine and say, "well, people on LKML are rude and flippant
all the time!" then that's a lame argument that's not
even true.

Your patch may in fact be a good one, but your anti-discussions
hurt them.  Discussion are good, not an insane attack fest.
What fun is that?  It's soooo lame.

That's all I'm saying.  You can defend it but no one cares.

Anyways, I'm out to have fun at a party with friends.
Try and get out, have fun, and come back when you're not looking
for someone to abuse.
-- 
David

Re: [PATCH 03/16] pull: cleanup documentation

[Felipe Contreras]

On Thu, Oct 31, 2013 at 8:48 PM, David Aguilar <davvid@gmail.com> wrote:
> On Thu, Oct 31, 2013 at 07:56:03PM -0600, Felipe Contreras wrote:

>> Nobody is forcing you to read them.
>
> You're missing the *key* point.
> No one wants to interact with a rude arrogant loudmouth.

Then don't. Problem solved.

-- 
Felipe Contreras

Re: [PATCH 03/16] pull: cleanup documentation

[SZEDER Gábor]

On Thu, Oct 31, 2013 at 09:50:51PM -0600, Felipe Contreras wrote:
> On Thu, Oct 31, 2013 at 8:48 PM, David Aguilar <davvid@gmail.com> wrote:
> > On Thu, Oct 31, 2013 at 07:56:03PM -0600, Felipe Contreras wrote:
> 
> >> Nobody is forcing you to read them.
> >
> > You're missing the *key* point.
> > No one wants to interact with a rude arrogant loudmouth.
> 
> Then don't. Problem solved.

Nope, it just recreates another old problem.  We've been there before,
not long ago:

On Sat, Oct 12, 2013 at 02:24:50AM -0500, Felipe Contreras wrote:
> Clearly, a lot of my patches have not been reviewed properly [...]


Best,
Gábor

Re: [PATCH 03/16] pull: cleanup documentation

[Felipe Contreras]

On Fri, Nov 1, 2013 at 5:05 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> On Thu, Oct 31, 2013 at 09:50:51PM -0600, Felipe Contreras wrote:
>> On Thu, Oct 31, 2013 at 8:48 PM, David Aguilar <davvid@gmail.com> wrote:
>> > On Thu, Oct 31, 2013 at 07:56:03PM -0600, Felipe Contreras wrote:
>>
>> >> Nobody is forcing you to read them.
>> >
>> > You're missing the *key* point.
>> > No one wants to interact with a rude arrogant loudmouth.
>>
>> Then don't. Problem solved.
>
> Nope, it just recreates another old problem.  We've been there before,
> not long ago:

Don't worry. That was the last time I sent those patches.

I will only resend the patch series that receive attention.

Happy?

-- 
Felipe Contreras

Re: [PATCH 03/16] pull: cleanup documentation

[Felipe Contreras]

On Thu, Oct 31, 2013 at 1:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, Oct 31, 2013 at 12:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>>> --- a/Documentation/git-pull.txt
>>>> +++ b/Documentation/git-pull.txt
>>>> @@ -39,7 +39,7 @@ Assume the following history exists and the current branch is
>>>>  "`master`":
>>>>
>>>>  ------------
>>>> -       A---B---C master on origin
>>>> +       A---B---C origin/master
>>>>        /
>>>>      D---E---F---G master
>>>>  ------------
>>>
>>> This change is wrong; the illustration depicts the distributed world
>>> (i.e. a fetch has not happened yet).
>>
>> That is an irrelevant implementation detail, specially at this high
>> level. In the user's mind origin/master means master on origin.
>
> You are wrong.  In the user's mind, origin/master means the commit
> that used to be at master on origin, and the point of this
> illustration is to make them understand that they live in a
> distributed world, where their last observation will go stale over
> time.

Wrong. That would make sense in 'git fetch', but here the point of the
illustration is to make them understand what 'git pull' will do,
namely a merge.

Which refs point to C at which points in time irrelevant information,
the user wants to know that 'git pull' will create a merge.

>> If you want to be pedantic, this is the "reality":
>>
>>   ------------
>>       D---E---F---G master
>>   ------------
>
> You are wrong again.  The "reality" is more like this:
>
>       origin/master in your repository
>       |
>       v
>       A---B---C master at origin
>      /
>     D---E---F---G master in your repository
>
> if you really want to write origin/master somewhere in this
> illustration.

Wrong. You probably mean:

------------
          A---B---C master on origin
         /
    D---E origin/master
         \
          F---G master
------------

But 'master on origin' doesn't exist in "reality" according to you, so:

------------
    D---E origin/master
         \
          F---G master
------------

-- 
Felipe Contreras

Re: [PATCH 03/16] pull: cleanup documentation

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Thu, Oct 31, 2013 at 1:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> On Thu, Oct 31, 2013 at 12:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>>> --- a/Documentation/git-pull.txt
>>>>> +++ b/Documentation/git-pull.txt
>>>>> @@ -39,7 +39,7 @@ Assume the following history exists and the current branch is
>>>>>  "`master`":
>>>>>
>>>>>  ------------
>>>>> -       A---B---C master on origin
>>>>> +       A---B---C origin/master
>>>>>        /
>>>>>      D---E---F---G master
>>>>>  ------------
>>>>
>>>> This change is wrong; the illustration depicts the distributed world
>>>> (i.e. a fetch has not happened yet).
>>>
>>> That is an irrelevant implementation detail, specially at this high
>>> level. In the user's mind origin/master means master on origin.
>>
>> You are wrong.  In the user's mind, origin/master means the commit
>> that used to be at master on origin, and the point of this
>> illustration is to make them understand that they live in a
>> distributed world, where their last observation will go stale over
>> time.
>
> Wrong. That would make sense in 'git fetch', but here the point of the
> illustration is to make them understand what 'git pull' will do,
> namely a merge.
>
> Which refs point to C at which points in time irrelevant information,
> the user wants to know that 'git pull' will create a merge.

Merge with what, and how do the users know what will be merged?

Think.

The users need to learn that origin/master they were told to use
with "git log origin/master.." etc. trails reality, "git fetch" is
how they can get them in sync, and the reason they do not need to
run "git fetch" separately when they "git pull" is because it is run
for them internally.

That is what the illustration and the paragraph that follows teach
them.

I've said enough on this.

Re: [PATCH 03/16] pull: cleanup documentation

[Felipe Contreras]

On Thu, Oct 31, 2013 at 2:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, Oct 31, 2013 at 1:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> On Thu, Oct 31, 2013 at 12:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>>
>>>>>> --- a/Documentation/git-pull.txt
>>>>>> +++ b/Documentation/git-pull.txt
>>>>>> @@ -39,7 +39,7 @@ Assume the following history exists and the current branch is
>>>>>>  "`master`":
>>>>>>
>>>>>>  ------------
>>>>>> -       A---B---C master on origin
>>>>>> +       A---B---C origin/master
>>>>>>        /
>>>>>>      D---E---F---G master
>>>>>>  ------------
>>>>>
>>>>> This change is wrong; the illustration depicts the distributed world
>>>>> (i.e. a fetch has not happened yet).
>>>>
>>>> That is an irrelevant implementation detail, specially at this high
>>>> level. In the user's mind origin/master means master on origin.
>>>
>>> You are wrong.  In the user's mind, origin/master means the commit
>>> that used to be at master on origin, and the point of this
>>> illustration is to make them understand that they live in a
>>> distributed world, where their last observation will go stale over
>>> time.
>>
>> Wrong. That would make sense in 'git fetch', but here the point of the
>> illustration is to make them understand what 'git pull' will do,
>> namely a merge.
>>
>> Which refs point to C at which points in time irrelevant information,
>> the user wants to know that 'git pull' will create a merge.
>
> Merge with what,

Merge with C.

> and how do the users know what will be merged?

They don't, not after they run 'git pull' anyway.

> The users need to learn that origin/master they were told to use
> with "git log origin/master.." etc. trails reality,

Yes, but they don't *need* to learn it *right now*. All they need to
learn is that 'git pull' will do a merge with 'master' from 'origin',
AKA 'origin/master'.

-- 
Felipe Contreras

[PATCH 04/16] fetch: add missing documentation

[Felipe Contreras]

There's no mention of the 'origin' default, or the fact that the
upstream tracking branch remote is used.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-fetch.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
index e08a028..7e75dc4 100644
--- a/Documentation/git-fetch.txt
+++ b/Documentation/git-fetch.txt
@@ -37,6 +37,9 @@ or from several repositories at once if <group> is given and
 there is a remotes.<group> entry in the configuration file.
 (See linkgit:git-config[1]).
 
+When no remote is specified, by the default the `origin` remote will be used,
+unless there's an upstream branch configured for the current branch.
+
 OPTIONS
 -------
 include::fetch-options.txt[]
-- 
1.8.4.2+fc1

Re: [PATCH 04/16] fetch: add missing documentation

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> There's no mention of the 'origin' default, or the fact that the
> upstream tracking branch remote is used.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/git-fetch.txt | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
> index e08a028..7e75dc4 100644
> --- a/Documentation/git-fetch.txt
> +++ b/Documentation/git-fetch.txt
> @@ -37,6 +37,9 @@ or from several repositories at once if <group> is given and
>  there is a remotes.<group> entry in the configuration file.
>  (See linkgit:git-config[1]).
>  
> +When no remote is specified, by the default the `origin` remote will be used,

I recall there were typofix comments on this line.

> +unless there's an upstream branch configured for the current branch.

Also there was a phrasing comment on this one, I think.

Resending without rerolling is not very much appreciated.


> +
>  OPTIONS
>  -------
>  include::fetch-options.txt[]

Re: [PATCH 04/16] fetch: add missing documentation

[Felipe Contreras]

On Thu, Oct 31, 2013 at 12:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:

>> diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
>> index e08a028..7e75dc4 100644
>> --- a/Documentation/git-fetch.txt
>> +++ b/Documentation/git-fetch.txt
>> @@ -37,6 +37,9 @@ or from several repositories at once if <group> is given and
>>  there is a remotes.<group> entry in the configuration file.
>>  (See linkgit:git-config[1]).
>>
>> +When no remote is specified, by the default the `origin` remote will be used,
>
> I recall there were typofix comments on this line.

--- a/Documentation/git-fetch.txt
+++ b/Documentation/git-fetch.txt
@@ -37,7 +37,7 @@ or from several repositories at once if <group> is given and
 there is a remotes.<group> entry in the configuration file.
 (See linkgit:git-config[1]).

-When no remote is specified, by the default the `origin` remote will be used,
+When no remote is specified, by default the `origin` remote will be used,
 unless there's an upstream branch configured for the current branch.

 OPTIONS

>> +unless there's an upstream branch configured for the current branch.
>
> Also there was a phrasing comment on this one, I think.

There was no constructive comment, no alternative was proposed.

The conclusion of the discussion (at least mine) is that the phrasing is fine.

> Resending without rerolling is not very much appreciated.

I missed a valid comment in one of my 160 pending patches. Sue me.

-- 
Felipe Contreras

[PATCH 05/16] revision: add missing include

[Felipe Contreras]

Otherwise we might not have 'struct diff_options'.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 revision.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/revision.h b/revision.h
index e7f1d21..89132df 100644
--- a/revision.h
+++ b/revision.h
@@ -5,6 +5,7 @@
 #include "grep.h"
 #include "notes.h"
 #include "commit.h"
+#include "diff.h"
 
 #define SEEN		(1u<<0)
 #define UNINTERESTING   (1u<<1)
-- 
1.8.4.2+fc1

Re: [PATCH 05/16] revision: add missing include

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Otherwise we might not have 'struct diff_options'.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  revision.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/revision.h b/revision.h
> index e7f1d21..89132df 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -5,6 +5,7 @@
>  #include "grep.h"
>  #include "notes.h"
>  #include "commit.h"
> +#include "diff.h"
>  
>  #define SEEN		(1u<<0)
>  #define UNINTERESTING   (1u<<1)

This is a step in the right direction to change the contract between
this header file and its consumers, but I think it falls short of
doing a good job at it.

The rule used to be that "if you use a declaration in revision.h,
you must include diff.h before including it, even if you do not use
any declaration made in diff.h yourself". 

The new rule this patch introduces is "if you use a declaration in
foo.h, include foo.h, period---foo.h should handle its requirement
on its own internally and consumers should not have to care", which
is much saner.

But the patch needs to also remove '#include "diff.h"' from existing
consumers that themselves do not use any declaration from "diff.h"
(e.g. bundle.c; there are others), while keeping the inclusion in
those that do (e.g. builtin/commit.c). That can be a separate patch
that immediately follow this one, or a part of the same patch.

Thanks.

[PATCH 06/16] shortlog: add missing declaration

[Felipe Contreras]

Otherwise we would have to include commit.h.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 shortlog.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/shortlog.h b/shortlog.h
index de4f86f..54bc07c 100644
--- a/shortlog.h
+++ b/shortlog.h
@@ -19,6 +19,8 @@ struct shortlog {
 	struct string_list mailmap;
 };
 
+struct commit;
+
 void shortlog_init(struct shortlog *log);
 
 void shortlog_add_commit(struct shortlog *log, struct commit *commit);
-- 
1.8.4.2+fc1

Re: [PATCH 06/16] shortlog: add missing declaration

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Otherwise we would have to include commit.h.

Was there a reason why commit.h is not included here, just like
revision.h would include diff.h, so that users of shortlog.h do not
have to worry about including commit.h themselves?

Note: not requesting the patch to be changed; just inquiring the
reasoning behind a different approach to solve related/same problem.

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  shortlog.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/shortlog.h b/shortlog.h
> index de4f86f..54bc07c 100644
> --- a/shortlog.h
> +++ b/shortlog.h
> @@ -19,6 +19,8 @@ struct shortlog {
>  	struct string_list mailmap;
>  };
>  
> +struct commit;
> +
>  void shortlog_init(struct shortlog *log);
>  
>  void shortlog_add_commit(struct shortlog *log, struct commit *commit);

Re: [PATCH 06/16] shortlog: add missing declaration

[Felipe Contreras]

On Thu, Oct 31, 2013 at 1:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Otherwise we would have to include commit.h.
>
> Was there a reason why commit.h is not included here, just like
> revision.h would include diff.h, so that users of shortlog.h do not
> have to worry about including commit.h themselves?

Because you can't do:

struct diff_options;
struct diff_options diffopt;

The storage size is not known, but you can do:

struct diff_options;
struct diff_options *diffopt;

-- 
Felipe Contreras

Re: [PATCH 06/16] shortlog: add missing declaration

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Thu, Oct 31, 2013 at 1:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> Otherwise we would have to include commit.h.
>>
>> Was there a reason why commit.h is not included here, just like
>> revision.h would include diff.h, so that users of shortlog.h do not
>> have to worry about including commit.h themselves?
>
> Because you can't do:
>
> struct diff_options;
> struct diff_options diffopt;
>
> The storage size is not known, but you can do:
>
> struct diff_options;
> struct diff_options *diffopt;

But so can you do

	struct diff_options *diffopt;

without the declaration, no?  That is:

	$ cat >x.c <<\EOF
        struct foo {
        	struct bar *ptr;
	};

        int foo_is_null(struct foo *foo)
        {
        	return foo == 0;
	}
	EOF
        $ gcc -Wall -c x.c

Re: [PATCH 06/16] shortlog: add missing declaration

[Felipe Contreras]

On Thu, Oct 31, 2013 at 2:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, Oct 31, 2013 at 1:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> Otherwise we would have to include commit.h.
>>>
>>> Was there a reason why commit.h is not included here, just like
>>> revision.h would include diff.h, so that users of shortlog.h do not
>>> have to worry about including commit.h themselves?
>>
>> Because you can't do:
>>
>> struct diff_options;
>> struct diff_options diffopt;
>>
>> The storage size is not known, but you can do:
>>
>> struct diff_options;
>> struct diff_options *diffopt;
>
> But so can you do
>
>         struct diff_options *diffopt;
>
> without the declaration, no?  That is:

Yes.

-- 
Felipe Contreras

[PATCH 07/16] branch: trivial style fix

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/branch.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index ad0f86d..5696cf0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -975,9 +975,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("no such branch '%s'"), argv[0]);
 		}
 
-		if (!branch_has_merge_config(branch)) {
+		if (!branch_has_merge_config(branch))
 			die(_("Branch '%s' has no upstream information"), branch->name);
-		}
 
 		strbuf_addf(&buf, "branch.%s.remote", branch->name);
 		git_config_set_multivar(buf.buf, NULL, NULL, 1);
-- 
1.8.4.2+fc1

Re: [PATCH 07/16] branch: trivial style fix

[Junio C Hamano]

Looks good; thanks.

[PATCH 08/16] sha1-name: trivial style cleanup

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 sha1_name.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sha1_name.c b/sha1_name.c
index 0e5fe7f..e9c2999 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -343,7 +343,6 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
 	return status;
 }
 
-
 int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)
 {
 	char hex_pfx[40];
-- 
1.8.4.2+fc1

[PATCH 09/16] transport-helper: trivial style fix

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 transport-helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/transport-helper.c b/transport-helper.c
index b32e2d6..673b7c2 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -269,6 +269,7 @@ static const char *unsupported_options[] = {
 	TRANS_OPT_THIN,
 	TRANS_OPT_KEEP
 	};
+
 static const char *boolean_options[] = {
 	TRANS_OPT_THIN,
 	TRANS_OPT_KEEP,
-- 
1.8.4.2+fc1

[PATCH 10/16] describe: trivial style fixes

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/describe.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index b9d3603..6f62109 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -9,7 +9,7 @@
 #include "hash.h"
 #include "argv-array.h"
 
-#define SEEN		(1u<<0)
+#define SEEN		(1u << 0)
 #define MAX_TAGS	(FLAG_BITS - 1)
 
 static const char * const describe_usage[] = {
@@ -36,7 +36,6 @@ static const char *diff_index_args[] = {
 	"diff-index", "--quiet", "HEAD", "--", NULL
 };
 
-
 struct commit_name {
 	struct commit_name *next;
 	unsigned char peeled[20];
@@ -46,6 +45,7 @@ struct commit_name {
 	unsigned char sha1[20];
 	char *path;
 };
+
 static const char *prio_names[] = {
 	"head", "lightweight", "annotated",
 };
@@ -488,9 +488,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 	} else if (dirty) {
 		die(_("--dirty is incompatible with commit-ishes"));
 	} else {
-		while (argc-- > 0) {
+		while (argc-- > 0)
 			describe(*argv++, argc == 0);
-		}
 	}
 	return 0;
 }
-- 
1.8.4.2+fc1

[PATCH 11/16] pretty: trivial style fix

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 pretty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index b4e32b7..962e82b 100644
--- a/pretty.c
+++ b/pretty.c
@@ -497,7 +497,7 @@ void pp_user_info(struct pretty_print_context *pp,
 static int is_empty_line(const char *line, int *len_p)
 {
 	int len = *len_p;
-	while (len && isspace(line[len-1]))
+	while (len && isspace(line[len - 1]))
 		len--;
 	*len_p = len;
 	return !len;
-- 
1.8.4.2+fc1

[PATCH 12/16] revision: trivial style fixes

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 revision.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/revision.c b/revision.c
index 3fdea51..956040c 100644
--- a/revision.c
+++ b/revision.c
@@ -1519,7 +1519,7 @@ struct cmdline_pathspec {
 static void append_prune_data(struct cmdline_pathspec *prune, const char **av)
 {
 	while (*av) {
-		ALLOC_GROW(prune->path, prune->nr+1, prune->alloc);
+		ALLOC_GROW(prune->path, prune->nr + 1, prune->alloc);
 		prune->path[prune->nr++] = *(av++);
 	}
 }
@@ -1531,7 +1531,7 @@ static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb,
 		int len = sb->len;
 		if (len && sb->buf[len - 1] == '\n')
 			sb->buf[--len] = '\0';
-		ALLOC_GROW(prune->path, prune->nr+1, prune->alloc);
+		ALLOC_GROW(prune->path, prune->nr + 1, prune->alloc);
 		prune->path[prune->nr++] = xstrdup(sb->buf);
 	}
 }
@@ -2134,7 +2134,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		 *	call init_pathspec() to set revs->prune_data here.
 		 * }
 		 */
-		ALLOC_GROW(prune_data.path, prune_data.nr+1, prune_data.alloc);
+		ALLOC_GROW(prune_data.path, prune_data.nr + 1, prune_data.alloc);
 		prune_data.path[prune_data.nr++] = NULL;
 		parse_pathspec(&revs->prune_data, 0, 0,
 			       revs->prefix, prune_data.path);
@@ -2987,7 +2987,7 @@ static struct commit *get_revision_internal(struct rev_info *revs)
 	if (revs->max_count) {
 		c = get_revision_1(revs);
 		if (c) {
-			while (0 < revs->skip_count) {
+			while (revs->skip_count > 0) {
 				revs->skip_count--;
 				c = get_revision_1(revs);
 				if (!c)
@@ -3002,9 +3002,8 @@ static struct commit *get_revision_internal(struct rev_info *revs)
 	if (c)
 		c->object.flags |= SHOWN;
 
-	if (!revs->boundary) {
+	if (!revs->boundary)
 		return c;
-	}
 
 	if (!c) {
 		/*
@@ -3050,9 +3049,8 @@ struct commit *get_revision(struct rev_info *revs)
 
 	if (revs->reverse) {
 		reversed = NULL;
-		while ((c = get_revision_internal(revs))) {
+		while ((c = get_revision_internal(revs)))
 			commit_list_insert(c, &reversed);
-		}
 		revs->commits = reversed;
 		revs->reverse = 0;
 		revs->reverse_output_stage = 1;
-- 
1.8.4.2+fc1

[PATCH 13/16] diff: trivial style fix

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index 2fb8c5d..adb93a9 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -169,7 +169,7 @@ static int builtin_diff_tree(struct rev_info *revs,
 	if (ent1->item->flags & UNINTERESTING)
 		swap = 1;
 	sha1[swap] = ent0->item->sha1;
-	sha1[1-swap] = ent1->item->sha1;
+	sha1[1 - swap] = ent1->item->sha1;
 	diff_tree_sha1(sha1[0], sha1[1], "", &revs->diffopt);
 	log_tree_diff_flush(revs);
 	return 0;
-- 
1.8.4.2+fc1

[PATCH 14/16] run-command: trivial style fixes

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 run-command.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/run-command.c b/run-command.c
index 1b7f88e..3914d9c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -406,13 +406,12 @@ fail_pipe:
 					unsetenv(*cmd->env);
 			}
 		}
-		if (cmd->git_cmd) {
+		if (cmd->git_cmd)
 			execv_git_cmd(cmd->argv);
-		} else if (cmd->use_shell) {
+		else if (cmd->use_shell)
 			execv_shell_cmd(cmd->argv);
-		} else {
+		else
 			sane_execvp(cmd->argv[0], (char *const*) cmd->argv);
-		}
 		if (errno == ENOENT) {
 			if (!cmd->silent_exec_failure)
 				error("cannot run %s: %s", cmd->argv[0],
@@ -446,7 +445,6 @@ fail_pipe:
 		cmd->pid = -1;
 	}
 	close(notify_pipe[0]);
-
 }
 #else
 {
@@ -480,11 +478,10 @@ fail_pipe:
 	if (cmd->env)
 		env = make_augmented_environ(cmd->env);
 
-	if (cmd->git_cmd) {
+	if (cmd->git_cmd)
 		cmd->argv = prepare_git_cmd(cmd->argv);
-	} else if (cmd->use_shell) {
+	else if (cmd->use_shell)
 		cmd->argv = prepare_shell_cmd(cmd->argv);
-	}
 
 	cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env, cmd->dir,
 				  fhin, fhout, fherr);
-- 
1.8.4.2+fc1

[PATCH 15/16] setup: trivial style fixes

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 setup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/setup.c b/setup.c
index dbf4138..5432a31 100644
--- a/setup.c
+++ b/setup.c
@@ -563,7 +563,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 {
 	const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
 	struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
-	static char cwd[PATH_MAX+1];
+	static char cwd[PATH_MAX + 1];
 	const char *gitdirenv, *ret;
 	char *gitfile;
 	int len, offset, offset_parent, ceil_offset = -1;
@@ -578,7 +578,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 	if (nongit_ok)
 		*nongit_ok = 0;
 
-	if (!getcwd(cwd, sizeof(cwd)-1))
+	if (!getcwd(cwd, sizeof(cwd) - 1))
 		die_errno("Unable to read current working directory");
 	offset = len = strlen(cwd);
 
-- 
1.8.4.2+fc1

Re: [PATCH 15/16] setup: trivial style fixes

[Junio C Hamano]

Patches 7 through this one looks good; thanks.

[PATCH 16/16] add: avoid yoda conditions

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/add.c b/builtin/add.c
index 226f758..9b30356 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -429,7 +429,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	argc--;
 	argv++;
 
-	if (0 <= addremove_explicit)
+	if (addremove_explicit >= 0)
 		addremove = addremove_explicit;
 	else if (take_worktree_changes && ADDREMOVE_DEFAULT)
 		addremove = 0; /* "-u" was given but not "-A" */
-- 
1.8.4.2+fc1

Re: [PATCH 16/16] add: avoid yoda conditions

[Martin von Zweigbergk]

I was recently confused by the yoda condition in this block of code from [1]

+ for (i = 0; i < revs.nr; i++)
+ if (&bases->item->object == &revs.commit[i]->object)
+ break; /* found */
+ if (revs.nr <= i)

I think I was particularly surprised because it came so soon after the
"i < revs.nr". I didn't bother commenting because it seemed too
subjective and the code base has tons of these. Something as simple as

  git grep '[0-9] [<>]' *.c

finds a bunch (probably with lots of false positives and negatives).

I guess what I'm trying to say is that either we accept them and get
used to reading them without being surprised, or we can change a bit
more than one at a time perhaps? I understand that this was an
occurrence you just happened to run into, and I'm not saying that a
patch has to deal with _all_ occurrences. I'm more just wondering if
we want mention our position, whatever it is, in CodingGuidelines.

Martin

[1] http://thread.gmane.org/gmane.comp.version-control.git/236252/focus=236716

On Thu, Oct 31, 2013 at 2:25 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/add.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 226f758..9b30356 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -429,7 +429,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>         argc--;
>         argv++;
>
> -       if (0 <= addremove_explicit)
> +       if (addremove_explicit >= 0)
>                 addremove = addremove_explicit;
>         else if (take_worktree_changes && ADDREMOVE_DEFAULT)
>                 addremove = 0; /* "-u" was given but not "-A" */
> --
> 1.8.4.2+fc1
>
> --
> 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 16/16] add: avoid yoda conditions

[Felipe Contreras]

On Thu, Oct 31, 2013 at 1:48 PM, Martin von Zweigbergk
<martinvonz@gmail.com> wrote:

> I guess what I'm trying to say is that either we accept them and get
> used to reading them without being surprised, or we can change a bit
> more than one at a time perhaps? I understand that this was an
> occurrence you just happened to run into, and I'm not saying that a
> patch has to deal with _all_ occurrences. I'm more just wondering if
> we want mention our position, whatever it is, in CodingGuidelines.

Yes, I'm all in favor of updating CodingGuidelines with that.

-- 
Felipe Contreras

Re: [PATCH 16/16] add: avoid yoda conditions

[Junio C Hamano]

Martin von Zweigbergk <martinvonz@gmail.com> writes:

> I was recently confused by the yoda condition in this block of code from [1]
>
> + for (i = 0; i < revs.nr; i++)
> + if (&bases->item->object == &revs.commit[i]->object)
> + break; /* found */
> + if (revs.nr <= i)
>
> I think I was particularly surprised because it came so soon after the
> "i < revs.nr". I didn't bother commenting because it seemed too
> subjective and the code base has tons of these.

That follows "visual/textual order should follow the actual
ordering" principle.  Think of a number-line you learn in elementary
school arithmetic class, and try to place revs.nr and i on it.

I agree that there is no justification to write "if 0 == something",
when "if something == 0" suffices.  The latter reads better and that
is why the phrase "yoda condition" was invented.

But the situation is different when both sides are not constants,
and especially when "<" and "<=" are involved..

Re: [PATCH 16/16] add: avoid yoda conditions

[Felipe Contreras]

On Thu, Oct 31, 2013 at 2:31 PM, Junio C Hamano <gitster@pobox.com> wrote:

> I agree that there is no justification to write "if 0 == something",
> when "if something == 0" suffices.  The latter reads better and that
> is why the phrase "yoda condition" was invented.
>
> But the situation is different when both sides are not constants,
> and especially when "<" and "<=" are involved..

To me revs.nr is virtually a constant, I'm comparing i to revs.nr, not
the other way around.

I believe I explained this already, but here it goes again:

if (1.60 < john.size)

This makes no sense, "if 1.69 is smaller than john"?

The situation doesn't change when you use a variable:

if (size_limit < john.size)

Translates to "if size limit is smaller than john", and still makes no sense.

-- 
Felipe Contreras

Re: [PATCH 00/16] Trivial patches

[Max Horn]

[-- Attachment #1: Type: text/plain, Size: 2300 bytes --]


On 31.10.2013, at 10:25, Felipe Contreras <felipe.contreras@gmail.com> wrote:

> Most of these have been sent before, but were not applied for one reason or
> another.

All of these look fine and sensible to me. Some of the latter patches in the series might be a bit subjective (e.g. I personally don't mind "yoda" conditions at all), but none do harm, and most are a clear improvement. So I am all for applying this.

Cheers,
Max

> 
> Felipe Contreras (16):
>  merge: simplify ff-only option
>  t: replace pulls with merges
>  pull: cleanup documentation
>  fetch: add missing documentation
>  revision: add missing include
>  shortlog: add missing declaration
>  branch: trivial style fix
>  sha1-name: trivial style cleanup
>  transport-helper: trivial style fix
>  describe: trivial style fixes
>  pretty: trivial style fix
>  revision: trivial style fixes
>  diff: trivial style fix
>  run-command: trivial style fixes
>  setup: trivial style fixes
>  add: avoid yoda conditions
> 
> Documentation/git-fetch.txt            |  3 +++
> Documentation/git-pull.txt             |  4 ++--
> builtin/add.c                          |  2 +-
> builtin/branch.c                       |  3 +--
> builtin/describe.c                     |  7 +++----
> builtin/diff.c                         |  2 +-
> builtin/merge.c                        | 11 ++---------
> pretty.c                               |  2 +-
> revision.c                             | 14 ++++++--------
> revision.h                             |  1 +
> run-command.c                          | 13 +++++--------
> setup.c                                |  4 ++--
> sha1_name.c                            |  1 -
> shortlog.h                             |  2 ++
> t/annotate-tests.sh                    |  2 +-
> t/t4200-rerere.sh                      |  2 +-
> t/t9114-git-svn-dcommit-merge.sh       |  2 +-
> t/t9500-gitweb-standalone-no-errors.sh |  2 +-
> transport-helper.c                     |  1 +
> 19 files changed, 35 insertions(+), 43 deletions(-)
> 
> -- 
> 1.8.4.2+fc1
> 
> --
> 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
> 


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 235 bytes --]