Re: [PATCH v5 1/6] pull: rename pull.rename to pull.mode

From: Philip Oakley <philipoakley@iee.org>
Date: Apr 29, 2014, 23:00 UTC
Message-ID: <7B859CB604CE45AEA1DE2C9A9EF6C2B9@PhilipOakley>
In-Reply-To: <53602210aff0c_10a211e3308d@nysa.notmuch>

From: "Felipe Contreras" <felipe.contreras@gmail.com>

Show 25 quoted lines

> Philip Oakley wrote:
>> From: "Felipe Contreras" <felipe.contreras@gmail.com>
>> > Also 'branch.<name>.rebase' to 'branch.<name>.pullmode'.
>>
>> Sorry I haven't commented earlier. Because the 0/6 explanation isn't 
>> a
>> commit, a few extra words would be useful to capture what the 0/6 
>> cover
>> letter said to start the patch series cleanly/clearly e.g. start with
>>
>>     Begin the "Reject non-ff pulls by default" process by creating 
>> new
>>     config variables which will allow extra options, to replace the 
>> old
>> pull
>>     configuration options.
>>
>> I didn't immediately grasp why the 'replacement' was happening, 
>> rather
>> than it being a creation and a deprecation.
>
> How about this:
>
>> > This way 'pull.mode' can be set to 'merge', and the default can be
>> > something else.

My initial problem was the word sequence order which, in the original, started with the 'replace' statement, which tripped me up. So I was looking for some preparatory wording to set the scene and avoid the trip. (I'm assuming you were noting that you already had the subsequent "This way 'pull.mode'.." statement, unfortunately I'd already tripped up by then in the original, hence the suggestions for the scene setting)

Show 37 quoted lines

>
> This will allow us to eventually add a 'merge-ff-only' option, and
> eventually set it as default to solve the problem of not rejecting
> non-ff pulls by default.
>
>> > The old configurations still work, but get deprecated.
>> >
>> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>
>> > --- a/Documentation/config.txt
>> > +++ b/Documentation/config.txt
>
>> > @@ -764,15 +764,15 @@ branch.<name>.mergeoptions::
>> >  option values containing whitespace characters are currently not
>> >  supported.
>> >
>> > -branch.<name>.rebase::
>> > - When true, rebase the branch <name> on top of the fetched branch,
>> > - instead of merging the default branch from the default remote 
>> > when
>> > - "git pull" is run. See "pull.rebase" for doing this in a non
>> > - branch-specific manner.
>> > +branch.<name>.pullmode::
>> > + When "git pull" is run, this determines if it would either merge 
>> > or
>> > + rebase the fetched branch. The possible values are 'merge',
>> > + 'rebase', and 'rebase-preserve'. See "pull.mode" for doing this 
>> > in a
>> > + non branch-specific manner.
>>
>> I'd think it useful to add that:
>>     branch.<name>.rebase is deprecated.
>> given the large amount of internet cruft about this older config
>> variable name
>
> I don't recall deprecated configurations in the documentation, but I
> don't see a problem adding that either.

I'm sure that Junio will be able to form an opinion, which could go either way. On the one hand we try to avoid endless deprecation notices that then never get removed. On the other, we have the internet advice to set those old variables, so its worth associating them with their newer replacement with the warning. It'll be some else's judgment call at the end of the day.

>
> -- 
> Felipe Contreras
>

-- Philip