git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Felipe Contreras <felipe.contreras@gmail.com>
To: Richard Hansen <rhansen@bbn.com>,
	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:12:06 -0500	[thread overview]
Message-ID: <53640a26569a5_135215292ec43@nysa.notmuch> (raw)
In-Reply-To: <53640403.30600@bbn.com>

Richard Hansen wrote:
> 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?

Maybe. But if so, I think that should be done in another series.
Otherwise we'll never have a chance to change anything.

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

Right.

This actually interesting mode of thinking:

a) git pull --rebase

We want to rebase HEAD onto @{upstream}.

b) git pull --merge

We want to merge HEAD into @{upstream}. (Why are we doing the opposite?)

c) git pull --rebase github john

We weant to rebase github/john onto HEAD. (We are doing the opposite?)

d) git pull --merge github john

We want to merge github/john into HEAD.

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

'pull.mode=merge' will do the same as `git merge`, I don't see where or
how it can be explained more clearly.

> Which side will be the first parent?

The same as things currently are: the pulled branch into the current
branch (current branch is first parent).

We should probably change this, but that's out of scope of this series,
and hasn't been decided yet.

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

Hmm, might make sense.

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

Right.

> >  +
> >  *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'.

I'd say pull.mode is the important one.

-- 
Felipe Contreras

  reply	other threads:[~2014-05-02 21:22 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
2014-05-02 21:12     ` Felipe Contreras [this message]
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=53640a26569a5_135215292ec43@nysa.notmuch \
    --to=felipe.contreras@gmail.com \
    --cc=a.krey@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=john@keeping.me.uk \
    --cc=peff@peff.net \
    --cc=philipoakley@iee.org \
    --cc=rhansen@bbn.com \
    --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).