patch, 16 partsTrivial patches

> 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")),

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

> 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);

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

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

> 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

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

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

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

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