From: Richard Hansen <rhansen@bbn.com>
To: Felipe Contreras <felipe.contreras@gmail.com>, git@vger.kernel.org
Cc: Andreas Krey <a.krey@gmx.de>, John Keeping <john@keeping.me.uk>,
Jeff King <peff@peff.net>, Philip Oakley <philipoakley@iee.org>,
"Brian M. Carlson" <sandals@crustytoothpaste.net>,
"W. Trevor King" <wking@tremily.us>
Subject: Re: [PATCH v6 1/7] pull: rename pull.rebase to pull.mode
Date: Fri, 02 May 2014 16:45:55 -0400 [thread overview]
Message-ID: <53640403.30600@bbn.com> (raw)
In-Reply-To: <1398988808-29678-2-git-send-email-felipe.contreras@gmail.com>
On 2014-05-01 20:00, Felipe Contreras wrote:
> Also 'branch.<name>.rebase' to 'branch.<name>.pullmode'.
>
> This way we can add more modes and the default can be something else,
> namely it can be set to merge-ff-only, so eventually we can reject
> non-fast-forward merges by default.
>
> The old configurations still work, but get deprecated.
s/get/are/
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> Documentation/config.txt | 39 ++++++++++++++++++++++-----------------
> Documentation/git-pull.txt | 2 +-
> branch.c | 4 ++--
> builtin/remote.c | 14 ++++++++++++--
> git-pull.sh | 31 +++++++++++++++++++++++++++++--
> t/t3200-branch.sh | 40 ++++++++++++++++++++--------------------
> t/t5601-clone.sh | 4 ++--
> 7 files changed, 88 insertions(+), 46 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c26a7c8..c028aeb 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -708,7 +708,7 @@ branch.autosetupmerge::
> branch.autosetuprebase::
> When a new branch is created with 'git branch' or 'git checkout'
> that tracks another branch, this variable tells Git to set
> - up pull to rebase instead of merge (see "branch.<name>.rebase").
> + up pull to rebase instead of merge (see "branch.<name>.pullmode").
> When `never`, rebase is never automatically set to true.
> When `local`, rebase is set to true for tracked branches of
> other local branches.
Should branch.autosetuprebase be replaced with a new
branch.autosetupmode setting?
> @@ -764,15 +764,17 @@ 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.
To me this sentence implies that 'rebase' would rebase the fetched
branch onto HEAD, when it's actually the other way around.
> The possible values are 'merge',
> + 'rebase', and 'rebase-preserve'.
While the name 'merge' is mostly self-explanatory, I think it needs
further clarification: Does 'merge' imply --no-ff? Or --ff? Or the
value of merge.ff? Which side will be the first parent?
Similarly, 'rebase' could use some clarification:
* the local branch is rebased onto the pulled branch, not the other
way around
* it doesn't simply do 'git rebase FETCH_HEAD' -- it also walks the
reflog of the upstream ref until it finds an ancestor of the local
branch
> See "pull.mode" for doing this in a
> + non branch-specific manner.
I find this sentence to be a bit unclear and would prefer something
like: "Defaults to the value of pull.mode."
> +
> - When preserve, also pass `--preserve-merges` along to 'git rebase'
> - so that locally committed merge commits will not be flattened
> - by running 'git pull'.
> + When 'rebase-preserve', also pass `--preserve-merges` along to
> + 'git rebase' so that locally committed merge commits will not be
> + flattened by running 'git pull'.
> ++
> + It was named 'branch.<name>.rebase' but that is deprecated now.
To me this sentence implies that .rebase was simply renamed to .pullmode
with no other changes. I'd prefer something like this:
branch.<name>.rebase::
Deprecated in favor of branch.<name>.pullmode.
(Same goes for pull.rebase.)
> +
> *NOTE*: this is a possibly dangerous operation; do *not* use
> it unless you understand the implications (see linkgit:git-rebase[1]
> @@ -1881,15 +1883,18 @@ pretty.<name>::
> Note that an alias with the same name as a built-in format
> will be silently ignored.
>
> -pull.rebase::
> - When true, rebase branches on top of the fetched branch, instead
> - of merging the default branch from the default remote when "git
> - pull" is run. See "branch.<name>.rebase" for setting this on a
> - per-branch basis.
> +pull.mode::
> + 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 "branch.<name>.pullmode" for doing
> + this in a non branch-specific manner.
> ++
> + When 'rebase-preserve', also pass `--preserve-merges` along to
> + 'git rebase' so that locally committed merge commits will not be
> + flattened by running 'git pull'.
> ++
> +
> - When preserve, also pass `--preserve-merges` along to 'git rebase'
> - so that locally committed merge commits will not be flattened
> - by running 'git pull'.
The default value should be documented. Also, rather than copy+paste
the description from branch.<name>.pullmode, I'd prefer a brief
reference. For example:
pull.mode::
See branch.<name>.pullmode. Defaults to 'merge'.
-Richard
next prev parent reply other threads:[~2014-05-02 20:46 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-02 0:00 [PATCH v6 0/7] Reject non-ff pulls by default Felipe Contreras
2014-05-02 0:00 ` [PATCH v6 1/7] pull: rename pull.rebase to pull.mode Felipe Contreras
2014-05-02 14:13 ` W. Trevor King
2014-05-02 20:45 ` Richard Hansen [this message]
2014-05-02 21:12 ` Felipe Contreras
2014-05-02 23:51 ` Richard Hansen
2014-05-02 0:00 ` [PATCH v6 2/7] pull: migrate all the tests " Felipe Contreras
2014-05-02 0:00 ` [PATCH v6 3/7] pull: refactor $rebase variable into $mode Felipe Contreras
2014-05-02 0:00 ` [PATCH v6 4/7] pull: add --merge option Felipe Contreras
2014-05-02 1:37 ` brian m. carlson
2014-05-02 2:41 ` Felipe Contreras
2014-05-02 19:32 ` brian m. carlson
2014-05-02 20:14 ` Felipe Contreras
2014-05-02 20:44 ` brian m. carlson
2014-05-02 0:00 ` [PATCH v6 5/7] pull: add merge-ff-only option Felipe Contreras
2014-05-02 14:57 ` W. Trevor King
2014-05-02 0:00 ` [PATCH v6 6/7] pull: add warning on non-ff merges Felipe Contreras
2014-05-02 0:00 ` [PATCH v6 7/7] pull: only allow ff merges by default Felipe Contreras
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53640403.30600@bbn.com \
--to=rhansen@bbn.com \
--cc=a.krey@gmx.de \
--cc=felipe.contreras@gmail.com \
--cc=git@vger.kernel.org \
--cc=john@keeping.me.uk \
--cc=peff@peff.net \
--cc=philipoakley@iee.org \
--cc=sandals@crustytoothpaste.net \
--cc=wking@tremily.us \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).