Re: [RFC WIP PATCH] merge: implement -s theirs -X N
On Thu, Apr 19 2018, Junio C. Hamano wrote: I suppose this is more in reply to xmqqpo2ud9ih@gitster-ct.c.googlers.com. I was trying to answer all your questions in my 87r2nbeh1r@evledraar.gmail.com, but I think it wasn't clear, hopefully this inline clears things up. > Ævar Arnfjörð Bjarmasonwrites: > >> 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@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.
Re: [RFC WIP PATCH] merge: implement -s theirs -X N
Ævar Arnfjörð Bjarmasonwrites: > 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
On Thu, Apr 19 2018, Junio C. Hamano wrote: > Ævar Arnfjörð Bjarmasonwrites: > >> 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
Ævar Arnfjörð Bjarmasonwrites: > 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ð Bjarmasonwrites: > 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
On Wed, Apr 18, 2018 at 3:48 PM, Ævar Arnfjörð Bjarmasonwrote: > 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@gitster.mtv.corp.google.com/
[RFC WIP PATCH] merge: implement -s theirs -X N
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 &&