[RFC WIP PATCH] merge: implement -s theirs -X N

[RFC WIP PATCH] merge: implement -s theirs -X N

[Ævar Arnfjörð Bjarmason]

We have a -s ours, but not a -s theirs. This is a WIP patch to implement
that. It works, but I haven't dealt with this part of the internal API
before, comments most welcome.

The purpose of this is that I'm working with a rollout tool that is
capable of doing hotfixes on top of old commits on "master".

It does this by cherry-picking a commit from origin/master, and then
merges it with origin/master & pushes it back, before finally reset
--hard to the cherry-pick & rolling out.

The reason it's doing this is to maintain the guarantee that all rolled
out commits are reachable from "master", and to handle the more general
case where original work is made during a hotfix, we don't want to then
do a subsequent "normal" rollout and miss the fix.

In cases where I detect (by looking at patch-id's) that the only commits
that are being hotfixed are those cherry-picked from upstream, I want to
fully resolve the merge in favor of @{u}, i.e. end up with the same tree
SHA-1. This is the inverse of -s ours, but we have no -s theirs, this
implements that.

The `-s recursive -X theirs strategy` won't do, because that will just
resolve conflicts in favor of @{u}, but will screw up the well-known
cases where a merge produces bad results with no conflicts, due to edge
cases where patches apply cleanly but produce broken programs.

So this can be used as `-s theirs -X 2 @{u}` to implement that. The `-s
ours` strategy is then equivalent ot `-s theirs -X 1` (1st commit), and
we can do any value of `-X N` for octopus merges.

As noted `-s theirs` should be the same as supplying it with `-X 2`, but
I haven't implemented that yet.

Questions:

 1. Should I be calling read-tree here with run_command_v_opt(), or is
    there some internal API I should be using?

 2. Currently merge-ours is just a no-op since we take the current HEAD,
    but maybe it would be cleaner to implement it in terms of this
    thing, also to get immediate test coverage for all the -s ours
    stuff. We'd be reading the tree redundantly into the index, but
    maybe it's worth it for the coverage...

 3. Is there a better name for this than -s theirs? Maybe `-s nth -X N`?

diff --git a/.gitignore b/.gitignore
index 833ef3b0b7..65d62b191f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -93,6 +93,7 @@
 /git-merge-recursive
 /git-merge-resolve
 /git-merge-subtree
+/git-merge-theirs
 /git-mergetool
 /git-mergetool--lib
 /git-mktag
diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index 4a58aad4b8..a84482aafc 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -117,6 +117,12 @@ ours::
 	branches.  Note that this is different from the -Xours option to
 	the 'recursive' merge strategy.

+theirs::
+	This resolves any number of heads, but the resulting tree of
+	the merge is always that of the Nth branch head specified via
+	`-X n`. If it's not specified it'll default ot `-X 2`,
+	supplying `-X 1` makes this equivalent to the `ours` strategy.
+
 subtree::
 	This is a modified recursive strategy. When merging trees A and
 	B, if B corresponds to a subtree of A, B is first adjusted to
diff --git a/Makefile b/Makefile
index f181687250..00ded0c37c 100644
--- a/Makefile
+++ b/Makefile
@@ -998,6 +998,7 @@ BUILTIN_OBJS += builtin/merge-file.o
 BUILTIN_OBJS += builtin/merge-index.o
 BUILTIN_OBJS += builtin/merge-ours.o
 BUILTIN_OBJS += builtin/merge-recursive.o
+BUILTIN_OBJS += builtin/merge-theirs.o
 BUILTIN_OBJS += builtin/merge-tree.o
 BUILTIN_OBJS += builtin/mktag.o
 BUILTIN_OBJS += builtin/mktree.o
@@ -2815,6 +2816,7 @@ check-docs::
 		case "$$v" in \
 		git-merge-octopus | git-merge-ours | git-merge-recursive | \
 		git-merge-resolve | git-merge-subtree | \
+		git-merge-theirs \
 		git-fsck-objects | git-init-db | \
 		git-remote-* | git-stage | \
 		git-?*--?* ) continue ;; \
diff --git a/builtin.h b/builtin.h
index 42378f3aa4..a48eaf3a67 100644
--- a/builtin.h
+++ b/builtin.h
@@ -187,6 +187,7 @@ extern int cmd_merge_index(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_ours(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_file(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_recursive(int argc, const char **argv, const char *prefix);
+extern int cmd_merge_theirs(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_tree(int argc, const char **argv, const char *prefix);
 extern int cmd_mktag(int argc, const char **argv, const char *prefix);
 extern int cmd_mktree(int argc, const char **argv, const char *prefix);
diff --git a/builtin/merge-ours.c b/builtin/merge-ours.c
index c84c6e05e9..da5ba0b370 100644
--- a/builtin/merge-ours.c
+++ b/builtin/merge-ours.c
@@ -18,7 +18,6 @@ int cmd_merge_ours(int argc, const char **argv, const char *prefix)
 {
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage(builtin_merge_ours_usage);
-
 	/*
 	 * The contents of the current index becomes the tree we
 	 * commit.  The index must match HEAD, or this merge cannot go
diff --git a/builtin/merge-theirs.c b/builtin/merge-theirs.c
new file mode 100644
index 0000000000..545680ebc6
--- /dev/null
+++ b/builtin/merge-theirs.c
@@ -0,0 +1,72 @@
+/*
+ * Copyright (c) 2018 Ævar Arnfjörð Bjarmason
+ *
+ * Resolve the merge by picking the Nth (per -X) parent's tree is our
+ * new tree.
+ */
+#include "git-compat-util.h"
+#include "builtin.h"
+#include "run-command.h"
+#include "diff.h"
+
+static const char builtin_merge_theirs_usage[] =
+	"git merge-theirs <base>... -- HEAD <remote>...";
+
+static void read_tree_hex_oid(const char *hex_oid)
+{
+	int i = 0;
+	const char *args[4];
+
+	args[i++] = "read-tree";
+	args[i++] = hex_oid;
+	args[i] = NULL;
+
+	if (run_command_v_opt(args, RUN_GIT_CMD))
+		die(_("read-tree failed"));
+}
+
+int cmd_merge_theirs(int argc, const char **argv, const char *prefix)
+{
+	const char *mainline_str;
+	const int argc_offset = 3;
+	char *end;
+	int mainline;
+	const char *branch;
+	struct object_id commit;
+
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage(builtin_merge_theirs_usage);
+	if (argc < 6)
+		usage(builtin_merge_theirs_usage);
+
+	/*
+	 * Parse the --N part of `git merge-theirs --N base -- HEAD
+	 * other-branch [other-branch-2 ...]`.
+	 *
+	 * TODO: Handle no -X argument being equivalent to -X 2.
+	 */
+	mainline_str = argv[1];
+	if (!mainline_str[2])
+		usage(builtin_merge_theirs_usage);
+	mainline = strtol(mainline_str + 2, &end, 10);
+	if (*end || mainline <= 0)
+		die(_("'-s theirs -X N' expects a number greater than zero"));
+	if (mainline >= (argc - argc_offset))
+		die(_("'-s theirs -X N' must come with a corresponding Nth commit to merge!"));
+
+	/* Have the branch name */
+	branch = argv[argc_offset + mainline];
+	if (get_oid(branch, &commit))
+		die(_("could not resolve ref '%s'"), branch);
+
+	/* Final sanity checks, copied from merge-ours.c */
+	if (read_cache() < 0)
+		die_errno("read_cache failed");
+	if (index_differs_from("HEAD", NULL, 0))
+		exit(2);
+
+	/* Read the Nth tree */
+	read_tree_hex_oid(oid_to_hex(&commit));
+
+	exit(0);
+}
diff --git a/builtin/merge.c b/builtin/merge.c
index 9db5a2cf16..dd623530d8 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -81,6 +81,7 @@ static struct strategy all_strategy[] = {
 	{ "octopus",    DEFAULT_OCTOPUS },
 	{ "resolve",    0 },
 	{ "ours",       NO_FAST_FORWARD | NO_TRIVIAL },
+	{ "theirs",     NO_FAST_FORWARD | NO_TRIVIAL },
 	{ "subtree",    NO_FAST_FORWARD | NO_TRIVIAL },
 };

diff --git a/git.c b/git.c
index 3a89893712..8e87accbb9 100644
--- a/git.c
+++ b/git.c
@@ -434,6 +434,7 @@ static struct cmd_struct commands[] = {
 	{ "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-theirs", cmd_merge_theirs, RUN_SETUP | NO_PARSEOPT },
 	{ "merge-tree", cmd_merge_tree, RUN_SETUP | NO_PARSEOPT },
 	{ "mktag", cmd_mktag, RUN_SETUP | NO_PARSEOPT },
 	{ "mktree", cmd_mktree, RUN_SETUP },

Re: [RFC WIP PATCH] merge: implement -s theirs -X N

[Stefan Beller]

On Wed, Apr 18, 2018 at 3:48 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> We have a -s ours, but not a -s theirs. This is a WIP patch to implement
> that. It works, but I haven't dealt with this part of the internal API
> before, comments most welcome.

I hope reference pointers are welcome, too.
https://public-inbox.org/git/xmqqzi9iazrp.fsf@gitster.mtv.corp.google.com/

Re: [RFC WIP PATCH] merge: implement -s theirs -X N

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Questions:
>
>  1. Should I be calling read-tree here with run_command_v_opt(), or is
>     there some internal API I should be using?

The internal is unpack_trees(), which is usabe as a library-ish API
reasonably cleanly and easily.  For a project large enough where the
perforce can become an issue, the overhead to spawn read-tree as an
external process would be dwarfed by the cost of real processing of
merging multiple trees into an in-core index, but it involves two
extra writing and reading the index file back and forth compared to
the approach to use unpack_trees() to do everything in core.  As
long as the "now make sure that the contents of the index file is
that of the tree of the N-th parent" code is cleanly isolated in the
implementation, it is probably better to refrain from premature
optimization.

>  2. Currently merge-ours is just a no-op since we take the current HEAD,
>     but maybe it would be cleaner to implement it in terms of this
>     thing, also to get immediate test coverage for all the -s ours
>     stuff. We'd be reading the tree redundantly into the index, but
>     maybe it's worth it for the coverage...

I doubt that it would be a sensible approach.  If anything, even if
we lived in an alternate universe where "-s ours" weren't invented
and "-s become-nth-tree -W $N" feature gets first introduced, I
would imagine that we would introduce a code to special case "-s
ours" (aka "take the current HEAD") as an obvious optimization for
the common and trivial case, essentially splitting the "unification"
you are suggesting here.

>  3. Is there a better name for this than -s theirs? Maybe `-s nth -X N`?

I tend to agree that "-s thiers -X N" is horrible; "-s ours -X N"
would slightly be a better choice as it does not have to introduce a
new option.  "-s nth -X N" does not sound all that bad.

"Does this feature make sense?" was not among the questions listed,
and I am not ready to answer to it yet anyway, so ...

Re: [RFC WIP PATCH] merge: implement -s theirs -X N

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> We have a -s ours, but not a -s theirs. This is a WIP patch to implement
> that. It works, but I haven't dealt with this part of the internal API
> before, comments most welcome.
>
> The purpose of this is that I'm working with a rollout tool that is
> capable of doing hotfixes on top of old commits on "master".
>
> It does this by cherry-picking a commit from origin/master, and then
> merges it with origin/master & pushes it back, before finally reset
> --hard to the cherry-pick & rolling out.
>
> The reason it's doing this is to maintain the guarantee that all rolled
> out commits are reachable from "master", and to handle the more general
> case where original work is made during a hotfix, we don't want to then
> do a subsequent "normal" rollout and miss the fix.

This question has nothing to do with your "-s theirs" but let me see
if I got the above correctly.  Suppose you have a deployed branch
(say, "prod"), all developments happen on "master" elsewhere that
can be seen as "origin/master", so you may have a few fixes that is
not yet in "prod" you would want to cherry-pick from origin/master.

    $ git checkout prod
    $ git cherry-pick origin/master~2
    $ git cherry-pick origin/master

Let's say that "master" had a fix at HEAD~2, HEAD~1 is a feature
enhancement that is not yet ready for "prod", and HEAD is another
fix.  Up to this point you successfully back-ported the fixes to
"prod".

Then you do merge the tip into "master", i.e.

    $ git checkout origin/master && git merge -s ours prod
    $ git push origin HEAD:master
    $ git checkout prod

to make sure that the "master" at the source of truth knows that
it already has what our "prod" with these two cherry-picks have.

Is that what is going on here?

I am just wondering what would and should happen to the non-fix
commit in the middle in the above example.  Perhaps your workflow
automatically does the right thing to it, perhaps not.


[Footnote]

Obviously you can do this the other way around if you had "-s
theirs", i.e. instead of the last two lines from the above sequence,
you could do

    $ git merge -s nth -X 2 origin/master
    $ git push origin HEAD:master
    $ git reset --hard HEAD@{1}

but it is not all that interesting (at least to me) either way, as a
larger issue with the above I'd imagine people would see is that
even temporarily you would expose "master" material in that working
tree you usually have "prod" checkout.  That would irritate those
who consider that "push to deploy" aka "live site is actually a
working tree" is sensible more than the lack of "-s theirs" I would
think.

Re: [RFC WIP PATCH] merge: implement -s theirs -X N

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 19 2018, Junio C. Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> We have a -s ours, but not a -s theirs. This is a WIP patch to implement
>> that. It works, but I haven't dealt with this part of the internal API
>> before, comments most welcome.
>>
>> The purpose of this is that I'm working with a rollout tool that is
>> capable of doing hotfixes on top of old commits on "master".
>>
>> It does this by cherry-picking a commit from origin/master, and then
>> merges it with origin/master & pushes it back, before finally reset
>> --hard to the cherry-pick & rolling out.
>>
>> The reason it's doing this is to maintain the guarantee that all rolled
>> out commits are reachable from "master", and to handle the more general
>> case where original work is made during a hotfix, we don't want to then
>> do a subsequent "normal" rollout and miss the fix.
>
> This question has nothing to do with your "-s theirs" but let me see
> if I got the above correctly.  Suppose you have a deployed branch
> (say, "prod"), all developments happen on "master" elsewhere that
> can be seen as "origin/master", so you may have a few fixes that is
> not yet in "prod" you would want to cherry-pick from origin/master.
>
>     $ git checkout prod
>     $ git cherry-pick origin/master~2
>     $ git cherry-pick origin/master
>
> Let's say that "master" had a fix at HEAD~2, HEAD~1 is a feature
> enhancement that is not yet ready for "prod", and HEAD is another
> fix.  Up to this point you successfully back-ported the fixes to
> "prod".
>
> Then you do merge the tip into "master", i.e.
>
>     $ git checkout origin/master && git merge -s ours prod
>     $ git push origin HEAD:master
>     $ git checkout prod
>
> to make sure that the "master" at the source of truth knows that
> it already has what our "prod" with these two cherry-picks have.
>
> Is that what is going on here?
>
> I am just wondering what would and should happen to the non-fix
> commit in the middle in the above example.  Perhaps your workflow
> automatically does the right thing to it, perhaps not.
>
>
> [Footnote]
>
> Obviously you can do this the other way around if you had "-s
> theirs", i.e. instead of the last two lines from the above sequence,
> you could do
>
>     $ git merge -s nth -X 2 origin/master
>     $ git push origin HEAD:master
>     $ git reset --hard HEAD@{1}
>
> but it is not all that interesting (at least to me) either way, as a
> larger issue with the above I'd imagine people would see is that
> even temporarily you would expose "master" material in that working
> tree you usually have "prod" checkout.  That would irritate those
> who consider that "push to deploy" aka "live site is actually a
> working tree" is sensible more than the lack of "-s theirs" I would
> think.

Yeah this -s theirs is redundant to just doing it the other way around
as you describe.

The reason I want it is to always do the hotfix merge the same way
whether I'm dealing with a case where there's original work in the
hotfix (rare) or the case where there's just stuff to "prod"
cherry-picked from "master" (common).

I.e. I have:

 1. No original work on the hotfix. As determined by comparing the
    patch-id output of @{u}.. and ..@{u} and seeing if the patch ids I
    have cherry-picked are from commits that exist since there was last
    a full rollout.

 2. Original work during the hotfix on top of "prod", which we'll then
    want in the next rollout (when it'll be synced with "master").

Only #1 should use `-s theirs -X 2`, but #2 will just use the normal
merge strategy, i.e. it's possible we'll conflict, but then we should
resolve the conflict and push the fix to "master" (or at least
explicitly decide not to keep it).

I think that supporting this use-case explicitly in git is better than
having some unintuitive workaround where I'll first need to check out
the other branch purely because git-merge has an artificial limitation
of the "ours" driver having no mode to pick the Nth commit.

Re: [RFC WIP PATCH] merge: implement -s theirs -X N

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Thu, Apr 19 2018, Junio C. Hamano wrote:
>
>> This question has nothing to do with your "-s theirs" but let me see
>> if I got the above correctly.  Suppose you have a deployed branch
>> (say, "prod"), all developments happen on "master" elsewhere that
>> can be seen as "origin/master", so you may have a few fixes that is
>> not yet in "prod" you would want to cherry-pick from origin/master.
>>
>>     $ git checkout prod
>>     $ git cherry-pick origin/master~2
>>     $ git cherry-pick origin/master
>>
>> Let's say that "master" had a fix at HEAD~2, HEAD~1 is a feature
>> enhancement that is not yet ready for "prod", and HEAD is another
>> fix.  Up to this point you successfully back-ported the fixes to
>> "prod".
>>
>> Then you do merge the tip into "master", i.e.
>>
>>     $ git checkout origin/master && git merge -s ours prod
>>     $ git push origin HEAD:master
>>     $ git checkout prod
>>
>> to make sure that the "master" at the source of truth knows that
>> it already has what our "prod" with these two cherry-picks have.
>>
>> Is that what is going on here?
>>
>> I am just wondering what would and should happen to the non-fix
>> commit in the middle in the above example.  Perhaps your workflow
>> automatically does the right thing to it, perhaps not.
>>
>>
>> [Footnote]
>> ...
> Yeah this -s theirs is redundant to just doing it the other way around
> as you describe.
> ...

Heh, you responded to a much less relevant footnote without
addressing the main part of the message which was a more interesting
question to me ;-)

Re: [RFC WIP PATCH] merge: implement -s theirs -X N

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 19 2018, Junio C. Hamano wrote:

I suppose this is more in reply to
xmqqpo2ud9ih.fsf@gitster-ct.c.googlers.com. I was trying to answer all
your questions in my 87r2nbeh1r.fsf@evledraar.gmail.com, but I think it
wasn't clear, hopefully this inline clears things up.

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> We have a -s ours, but not a -s theirs. This is a WIP patch to implement
>> that. It works, but I haven't dealt with this part of the internal API
>> before, comments most welcome.
>>
>> The purpose of this is that I'm working with a rollout tool that is
>> capable of doing hotfixes on top of old commits on "master".
>>
>> It does this by cherry-picking a commit from origin/master, and then
>> merges it with origin/master & pushes it back, before finally reset
>> --hard to the cherry-pick & rolling out.
>>
>> The reason it's doing this is to maintain the guarantee that all rolled
>> out commits are reachable from "master", and to handle the more general
>> case where original work is made during a hotfix, we don't want to then
>> do a subsequent "normal" rollout and miss the fix.
>
> This question has nothing to do with your "-s theirs" but let me see
> if I got the above correctly.  Suppose you have a deployed branch
> (say, "prod"), all developments happen on "master" elsewhere that
> can be seen as "origin/master", so you may have a few fixes that is
> not yet in "prod" you would want to cherry-pick from origin/master.
>
>     $ git checkout prod
>     $ git cherry-pick origin/master~2
>     $ git cherry-pick origin/master
>
> Let's say that "master" had a fix at HEAD~2, HEAD~1 is a feature
> enhancement that is not yet ready for "prod", and HEAD is another
> fix.  Up to this point you successfully back-ported the fixes to
> "prod".
>
> Then you do merge the tip into "master", i.e.
>
>     $ git checkout origin/master && git merge -s ours prod
>     $ git push origin HEAD:master
>     $ git checkout prod
>
> to make sure that the "master" at the source of truth knows that
> it already has what our "prod" with these two cherry-picks have.
>
> Is that what is going on here?

Yes, the idea is that all commits that are rolled out are reachable from
"master". In my case there's no "prod" branch.

Instead we just have a tag on an older version of "master", and we're
applying a hotfix (usually one cherry-picked commit) to that tag.

We then push a merge of that origin/master back to origin/master so that:

 1. The commit is reachable from "master" as discussed. I.e. a "git
    clone" with the default tag follow logic will bring in that tag & I
    just need to bring in master to bring in every thing ever rolled
    out, not bring in every tag.

 2. Because the working tree is now in a state where we're at that
    hotfix tag, and we want to fast-forward to origin/master on the next
    "real" rollout.

    We could just 'reset --hard' in those cases, but it's easier to
    always be able to do a "git pull" (except when initiating a hotfix).

> I am just wondering what would and should happen to the non-fix
> commit in the middle in the above example.  Perhaps your workflow
> automatically does the right thing to it, perhaps not.

If there's a non-fix commit that contains any original work in the
hotfix I won't be able to use this ~-s theirs" strategy, and I detect
such case swith the patch-id method discussed in
87r2nbeh1r.fsf@evledraar.gmail.com. Then we'll need to use the default
merge logic.

That's actually what we're doing now, but after running into several
bugs with merges gone wrong I'm hoping to fix it, and without bending
over backwards to use -s ours by doing the merge the other way around.