Re: [PATCH] Re: Clarify documentation on the "ours" merge strategy.

Thomas Rast [off-list ref] writes:

Then again, I'm not sure if resolve vs. recursive makes a difference
in a rebase.  Octopus is weird for a two-head merge, I'm not sure why

 ...

+	If there is no `-s` option 'git-merge-recursive' is used
+	instead.  This implies --merge.
++
+Due to the peculiarities of 'git-rebase' (see \--merge above) the only
+built-in strategy that is actually useful is 'subtree'.

"recursive is just a slower and sometimes buggier alternative to resolve
but can handle renames" may mean "people do not have much reason to choose
resolve over recursive".  But that is quite different from "resolve is not
useful here _due to_ the peculiarities of rebase".  Wouldn't anybody who
thinks "resolve vs. recursive would not make a difference in a rebase"
also think "resolve vs. recursive would not make a difference anywhere"?

58634db (rebase: Allow merge strategies to be used when rebasing,
2006-06-21) added "-m" and "-s" to rebase to solve the problem of rebasing
against an upstream that has moved files.  What the commit actually did
was to use recursive (by default) while giving longer rope to the users by
choosing other strategies with "-s", without making any judgement as to
why other strategies may possibly be useful.

Perhaps there is some different issue at the root of this one.  Why would
anybody be tempted to say "-s ours" while running a rebase?  What did the
user want to see it do (instead of being a no-op because "ours" by
definition ignores the tree the change is replayed from)?

It is easy to dismiss it as a user misconception and it also is tempting
to think that it would be helped with updated description of "ours" to
dispel that misconception, but there may be some user wish that is totally
different from "ours merge" strategy but still can be validly labelled
using a word "ours" by somebody who does not know the way the word "ours"
is used in the git land, and satisfying that unknown user wish might be
the real solution to this issue.

Junio C Hamano:

Perhaps there is some different issue at the root of this one.  Why would 
anybody be tempted to say "-s ours" while running a rebase?  What did the 
user want to see it do (instead of being a no-op because "ours" by 
definition ignores the tree the change is replayed from)?

The reason why I wanted it in my initial example was due to me misreading 
the documentation of "ours".

My scenario is like this:

I have my web site under Git control (used to be CVS). Some parts of the web 
site is updated in-place (blog comments being saved as HTML directly in the 
web tree), whereas all other edits are done in clones of the repsository. 
These changes are then added and committed to the checked out web tree and 
pushed to the central repo.

In some cases, I wish to edit the comments in one of my clones (to remove 
spam not stopped by my spam filters, for instance), but editing these risks 
creating a conflict if there has been other changes in the mean time.

The web tree checkout script uses rebase to avoid introducing merge commits 
every time the blog comment is updated, as it in 99 % of cases is unrelated 
to any other changes found in the central repo.

In the few cases where the blog comment update from the web tree conflicts 
with a change in the central repo, I want the "git pull --rebase" call to 
overwrite any changes in the central repo with my changes in the web tree 
(meaning that I would later have to manually re-delete the spam comments, 
but I can live with that).

It is easy to dismiss it as a user misconception and it also is tempting 
to think that it would be helped with updated description of "ours" to 
dispel that misconception, but there may be some user wish that is totally 
different from "ours merge" strategy but still can be validly labelled 
using a word "ours" by somebody who does not know the way the word "ours" 
is used in the git land, and satisfying that unknown user wish might be 
the real solution to this issue.

Yes, I am apparently looking for something that is not available in the Git 
codebase yet. :-)

-- 
\\// Peter - http://www.softwolves.pp.se/

On 2009.11.11 23:55:09 -0800, Junio C Hamano wrote:

58634db (rebase: Allow merge strategies to be used when rebasing,
2006-06-21) added "-m" and "-s" to rebase to solve the problem of rebasing
against an upstream that has moved files.  What the commit actually did
was to use recursive (by default) while giving longer rope to the users by
choosing other strategies with "-s", without making any judgement as to
why other strategies may possibly be useful.

At least the original reason for 58634db became (partially?) moot half a
year later, thanks to 579c9bb19 "Use merge-recursive in git-am -3".
Rebase already falls back to recursive merging in am, so using rebase -m
with the recursive strategy just stops it from trying the fast path,
right?

That should probably be reflected in the man page, but honestly I have
no idea what to write there now. The note about recursive should go, but
keeping only "Use merging strategies to rebase" doesn't actually look
like it's going to be helpful in any way.

Perhaps there is some different issue at the root of this one.  Why would
anybody be tempted to say "-s ours" while running a rebase?  What did the
user want to see it do (instead of being a no-op because "ours" by
definition ignores the tree the change is replayed from)?

Given the few requests I've seen of it (here + #git), I'd guess that
the user wants "git rebase -s ours $up" to do either:

MB=$(git merge-base $up HEAD)
git filter-branch --parent-filter "sed -e s/$MB/$up/" -- HEAD --not $up

i.e. just re-attach things to upstream, ignoring whatever upstream did
(git-svn users seem to want something like that sometimes to be able to
dcommit. Dunno if they have some hatred against the other users of their
svn repo ;-))

Or the user wants the infamous "resolve conflicts to want I did", often
enough without thinking about what that actually means and how it can
easily lead to total crap. (Yes, I'm biased...)

Björn

Quoting Peter Krefting [off-list ref]

The web tree checkout script uses rebase to avoid introducing merge
commits every time the blog comment is updated, as it in 99 % of cases
is unrelated to any other changes found in the central repo.

In the few cases where the blog comment update from the web tree
conflicts with a change in the central repo, I want the "git pull
--rebase" call to overwrite any changes in the central repo with my
changes in the web tree (meaning that I would later have to manually
re-delete the spam comments, but I can live with that).

That sounds like "-Xours" merge option that was discussed some time 
ago. See

    http://thread.gmane.org/gmane.comp.version-control.git/76650/focus=89021

I remember that Junio and Petr were against it because it would 
encourage a bad workflow. Dscho was against the syntax used to 
pass the options also.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Nanako Shiraishi [off-list ref] writes:

Quoting Peter Krefting [off-list ref]

quoted

The web tree checkout script uses rebase to avoid introducing merge
commits every time the blog comment is updated, as it in 99 % of cases
is unrelated to any other changes found in the central repo.

In the few cases where the blog comment update from the web tree
conflicts with a change in the central repo, I want the "git pull
--rebase" call to overwrite any changes in the central repo with my
changes in the web tree (meaning that I would later have to manually
re-delete the spam comments, but I can live with that).

That sounds like "-Xours" merge option that was discussed some time 
ago. See

    http://thread.gmane.org/gmane.comp.version-control.git/76650/focus=89021

I remember that Junio and Petr were against it because it would 
encourage a bad workflow. Dscho was against the syntax used to 
pass the options also.

Yeah, Björn seems to speculate the same.

Even though I still think -Xours/-Xtheirs are nonsense options in the
context of source code management, I suspect that they might be exactly
what Peter needs in this situation.

As long as the changes made on the "web tree" side only consist of
user-generated blog contents and never touch framework code that is
controlled by his "central repo" side (and that condition should
hold true; otherwise Peter's web site is seriously broken from the
security point of view and no SCM can fix that), running a merge with
the fabled -Xours option in the "web tree" to slurp in the changes made on
the "central repo" side does not sound like an unreasonable thing to do.

Junio convinced me that it is not possible to limit the choice to only
'subtree', so here's a short series that implements the other changes
I had already posted in diff form.

I also implemented Nicolas's suggestion to reject -s ours outright;
I'm not really happy with starting a blacklist there, but maybe it
helps the next unwary user.  I split it because even if you reject
3/3, I think the first two should go in as extra documentation.

Thomas Rast (3):
  Documentation: clarify 'ours' merge strategy
  rebase docs: clarify --merge and --strategy
  rebase: refuse to rebase with -s ours

 Documentation/git-rebase.txt       |   14 +++++++++++---
 Documentation/merge-strategies.txt |    5 +++--
 git-rebase--interactive.sh         |    4 ++++
 git-rebase.sh                      |    4 ++++
 4 files changed, 22 insertions(+), 5 deletions(-)

Add a paragraph about the swapped sides in a --merge rebase, which was
otherwise only documented in the sources.

Add a paragraph about the effects of the 'ours' strategy to the -s
description.  Also remove the mention of the 'octopus' strategy, which
was copied from the git-merge description but is pointless in a
rebase.

Signed-off-by: Thomas Rast <redacted>
---
 Documentation/git-rebase.txt |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 33e0ef1..5fa9100 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt

@@ -228,13 +228,20 @@ OPTIONS
 	Use merging strategies to rebase.  When the recursive (default) merge
 	strategy is used, this allows rebase to be aware of renames on the
 	upstream side.
++
+Note that in a rebase merge (hence merge conflict), the sides are
+swapped: "theirs" is the to-be-applied patch, and "ours" is the so-far
+rebased series, starting with <upstream>.
 
 -s <strategy>::
 --strategy=<strategy>::
 	Use the given merge strategy.
-	If there is no `-s` option, a built-in list of strategies
-	is used instead ('git-merge-recursive' when merging a single
-	head, 'git-merge-octopus' otherwise).  This implies --merge.
+	If there is no `-s` option 'git-merge-recursive' is used
+	instead.  This implies --merge.
++
+Due to the peculiarities of 'git-rebase' (see \--merge above), using
+the 'ours' strategy simply discards all patches from the <branch>,
+which makes little sense.
 
 -q::
 --quiet::

-- 
1.6.5.2.420.gf6c057.dirty

Make it clear in the docs that the merge takes the tree of HEAD and
ignores everything in the other branches.  This should hopefully clear
up confusion, usually caused by the user looking for a strategy that
resolves all conflict hunks in favour of HEAD (which is completely
different and currently not supported).

Signed-off-by: Thomas Rast <redacted>
---
 Documentation/merge-strategies.txt |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index 4365b7e..42910a3 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt

@@ -29,8 +29,9 @@ octopus::
 	pulling or merging more than one branch.
 
 ours::
-	This resolves any number of heads, but the result of the
-	merge is always the current branch head.  It is meant to
+	This resolves any number of heads, but the resulting tree of the
+	merge is always that of the current branch head, effectively
+	ignoring all changes from all other branches.  It is meant to
 	be used to supersede old development history of side
 	branches.
 

-- 
1.6.5.2.420.gf6c057.dirty

Using the "ours" strategy with rebase just discards all changes,
turning <branch> into <upstream> (or <newbase> if given).  This is
unlikely to be what the user wants, so simply refuse to do it.

Also document what would happen near the -s option, and point the user
at it with the error message.

Signed-off-by: Thomas Rast <redacted>
---
 Documentation/git-rebase.txt |    3 ++-
 git-rebase--interactive.sh   |    4 ++++
 git-rebase.sh                |    4 ++++
 3 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 5fa9100..2203e63 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt

@@ -241,7 +241,8 @@ rebased series, starting with <upstream>.
 +
 Due to the peculiarities of 'git-rebase' (see \--merge above), using
 the 'ours' strategy simply discards all patches from the <branch>,
-which makes little sense.
+which makes little sense.  Thus 'git-rebase' does not accept this
+strategy.
 
 -q::
 --quiet::

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 53ad248..c6bc156 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -584,6 +584,10 @@ first and then run 'git rebase --continue' again."
 			STRATEGY="-s $2"
 			shift ;;
 		esac
+		if test "$STRATEGY" = "-s ours"
+		then
+			die "Refusing to rebase with 'ours' strategy; see git help rebase."
+		fi
 		;;
 	-m)
 		# we use merge anyway

diff --git a/git-rebase.sh b/git-rebase.sh
index 6ec155c..2d7d566 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh

@@ -306,6 +306,10 @@ do
 			strategy="$2"
 			shift ;;
 		esac
+		if test $strategy = ours
+		then
+			die "Refusing to rebase with 'ours' strategy; see git help rebase."
+		fi
 		do_merge=t
 		;;
 	-n|--no-stat)

-- 
1.6.5.2.420.gf6c057.dirty

Heya,

On Sun, Nov 15, 2009 at 19:25, Thomas Rast [off-list ref] wrote:

+               if test "$STRATEGY" = "-s ours"

Is this solid? Would "-s  ours" (two spaces) work?

-- 
Cheers,

Sverre Rabbelier

Sverre Rabbelier wrote:

Heya,

On Sun, Nov 15, 2009 at 19:25, Thomas Rast [off-list ref] wrote:

quoted

+               if test "$STRATEGY" = "-s ours"

Is this solid? Would "-s  ours" (two spaces) work?

Well, the variable is set by the case immediately before the new test:

	case "$#,$1" in
	*,*=*)
		STRATEGY="-s "$(expr "z$1" : 'z-[^=]*=\(.*\)') ;;
	1,*)
		usage ;;
	*)
		STRATEGY="-s $2"
		shift ;;
	esac

I didn't want to split that for a direct comparison with the second
half of the value, but unless I'm missing something, you'd have to say
-s ' ours' to make the test fail.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Thomas Rast [off-list ref] writes:

I also implemented Nicolas's suggestion to reject -s ours outright;
I'm not really happy with starting a blacklist there, but maybe it
helps the next unwary user.

I am inclined to agree with you and Nicolas on this, but I'll let the list
decide if [3/3] is a good idea.

I'd rewrite [3/3] in the following way to keep it easier to maintain the
blacklist, like this.

            case "$1" in
    -       ours)
    +       ours | theirs | octopus | subtree)
                    die "Refusing to rebase with $1; see git help rebase."
            esac

It would also make it easier to turn this into a whitelist if we choose
to,

 git-rebase--interactive.sh |    5 +----
 git-rebase.sh              |    5 +----
 git-sh-setup.sh            |    7 +++++++
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 53d35f3..de7448b 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -571,10 +571,7 @@ first and then run 'git rebase --continue' again."
 			STRATEGY="-s $2"
 			shift ;;
 		esac
-		if test "$STRATEGY" = "-s ours"
-		then
-			die "Refusing to rebase with 'ours' strategy; see git help rebase."
-		fi
+		git_check_merge_strategy_used_in_rebase "${STRATEGY#-s }"
 		;;
 	-m)
 		# we use merge anyway

diff --git a/git-rebase.sh b/git-rebase.sh
index 2d7d566..dd9ec63 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh

@@ -306,10 +306,7 @@ do
 			strategy="$2"
 			shift ;;
 		esac
-		if test $strategy = ours
-		then
-			die "Refusing to rebase with 'ours' strategy; see git help rebase."
-		fi
+		git_check_merge_strategy_used_in_rebase "$strategy"
 		do_merge=t
 		;;
 	-n|--no-stat)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index c41c2f7..724955f 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh

@@ -199,3 +199,10 @@ case $(uname -s) in
 	}
 	;;
 esac
+
+git_check_merge_strategy_used_in_rebase () {
+	case "$1" in
+	ours)
+		die "Refusing to rebase with $1; see git help rebase."
+	esac
+}

Thomas Rast [off-list ref] writes:

Add a paragraph about the swapped sides in a --merge rebase, which was
otherwise only documented in the sources.

Add a paragraph about the effects of the 'ours' strategy to the -s
description.  Also remove the mention of the 'octopus' strategy, which
was copied from the git-merge description but is pointless in a
rebase.

Instead of saying "peculiarities" without saying what is peculiar about
it, it might be better to give an explanation that would help the reader
understand why they appear "swapped".

Here is my attempt.  Thoughts?

 Documentation/git-rebase.txt |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index e802421..a6f8182 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt

@@ -229,9 +229,11 @@ OPTIONS
 	strategy is used, this allows rebase to be aware of renames on the
 	upstream side.
 +
-Note that in a rebase merge (hence merge conflict), the sides are
-swapped: "theirs" is the to-be-applied patch, and "ours" is the so-far
-rebased series, starting with <upstream>.
+Note that a rebase merge works by replaying each commit from the working
+branch on top of the <upstream> branch.  Because of this, when a merge
+conflict happens, the side reported as 'ours' is the so-far rebased
+series, starting with <upstream>, and 'theirs' is the working branch.  In
+other words, the sides are swapped.
 
 -s <strategy>::
 --strategy=<strategy>::

@@ -239,7 +241,9 @@ rebased series, starting with <upstream>.
 	If there is no `-s` option 'git-merge-recursive' is used
 	instead.  This implies --merge.
 +
-Due to the peculiarities of 'git-rebase' (see \--merge above), using
+Because 'git-rebase' replays each commit from the working branch
+on top of the <upstream> branch using the given strategy,
+(see \--merge above), using
 the 'ours' strategy simply discards all patches from the <branch>,
 which makes little sense.  Thus 'git-rebase' does not accept this
 strategy.

Junio C Hamano wrote:

This is much nicer than mine!

-Due to the peculiarities of 'git-rebase' (see \--merge above), using
+Because 'git-rebase' replays each commit from the working branch
+on top of the <upstream> branch using the given strategy,
+(see \--merge above), using
 the 'ours' strategy simply discards all patches from the <branch>,
 which makes little sense.  Thus 'git-rebase' does not accept this
 strategy.

Here I'm not sure if it makes such a big difference, since we already
explained the problem in --merge (and point to it).  But yours is fine
too.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Junio C Hamano wrote:

I'd rewrite [3/3] in the following way to keep it easier to maintain the
blacklist, like this.

            case "$1" in
    -       ours)
    +       ours | theirs | octopus | subtree)

I agree with the rewrite; it's easier to maintain, even though it's
now in a quite strange place.

However, I think the example you gave is misleading: 'subtree' is a
useful strategy if you want to rebase across a subtree merge boundary,
isn't it?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Junio C Hamano:

Even though I still think -Xours/-Xtheirs are nonsense options in the 
context of source code management, I suspect that they might be exactly 
what Peter needs in this situation.

Yes, it sounds like it would. That is not something I would use for source 
code management, but it would fit this, and some other use-cases I have, 
quite nicely.

I tend to use Git not only for source code management, but also for document 
synchronisation across machines which may, or may not, be connected to a 
network at any given time. Git is very nice for that sort of work.

; otherwise Peter's web site is seriously broken from the security point 
of view and no SCM can fix that),

Indeed. If it that was the case, I deserve whatever problems I get :-)

-- 
\\// Peter - http://www.softwolves.pp.se/

Hi,

On Sun, 15 Nov 2009, Thomas Rast wrote:

Using the "ours" strategy with rebase just discards all changes, turning 
<branch> into <upstream> (or <newbase> if given).  This is unlikely to 
be what the user wants, so simply refuse to do it.

"Unlikely" or "impossible"?

Besides, I find it rather arbitrary that the "ours" strategy is refused, 
but none of the user-provided merge strategies.  IOW disallowing "ours" 
may very well foster unreasonable expectations.

Ciao,
Dscho