Re: [RFC PATCH 2/3] mergetools/kdiff3: allow opting-out of auto-merges
On Thu, May 9, 2013 at 10:41 PM, Charles Bailey wrote: > On Thu, May 09, 2013 at 03:17:30PM -0700, David Aguilar wrote: >> Generally, "mergetool..cmd" is not general enough since we've >> always special cased the base vs. no-base code paths and we run >> different commands depending on whether a base is available. > > Then this is a deficiency of the ".cmd" mechanism which should provide > an "if all else fails" way to do things, even if ugly. We could simply > add a BASELESS variable to the eval environment of the custom command. > (Do we always create an empty file for $BASE, now?) > >> We could drop --auto altogether, which maybe is a better course of >> action since it makes the behavior predictable and un-surprising, but >> I do not know if anyone has come to rely on kdiff3's "auto-merge" >> feature (hence the extended Cc: list). > > I disagree, I think that a mergetool should be allowed to be as > helpful as possible and if it can resolve the merge unaided then this > is no bad thing. I've worked with a number of people who were very > happy with the current kdiff3 behaviour. Nothing prevents you from > verifying the merge and fixing it up if it wasn't done perfectly by > the tool, although I haven't ever encountered this with git+kdiff3. That's the bit of info I was needing. I can always tell the one person who ran into this that "that's how it is" and explain why it's a good thing. "one person" < "number of people" so it's an easy decision, and we don't need to make the tool worse to cater to someone that's new to git. Thanks Charles, -- David -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/3] mergetools/kdiff3: allow opting-out of auto-merges
On Thu, May 09, 2013 at 03:17:30PM -0700, David Aguilar wrote: > Generally, "mergetool..cmd" is not general enough since we've > always special cased the base vs. no-base code paths and we run > different commands depending on whether a base is available. Then this is a deficiency of the ".cmd" mechanism which should provide an "if all else fails" way to do things, even if ugly. We could simply add a BASELESS variable to the eval environment of the custom command. (Do we always create an empty file for $BASE, now?) > We could drop --auto altogether, which maybe is a better course of > action since it makes the behavior predictable and un-surprising, but > I do not know if anyone has come to rely on kdiff3's "auto-merge" > feature (hence the extended Cc: list). I disagree, I think that a mergetool should be allowed to be as helpful as possible and if it can resolve the merge unaided then this is no bad thing. I've worked with a number of people who were very happy with the current kdiff3 behaviour. Nothing prevents you from verifying the merge and fixing it up if it wasn't done perfectly by the tool, although I haven't ever encountered this with git+kdiff3. Charles. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/3] mergetools/kdiff3: allow opting-out of auto-merges
On Thu, May 9, 2013 at 12:15 PM, Junio C Hamano wrote: > John Keeping writes: > >> On Thu, May 09, 2013 at 09:10:51AM -0700, Junio C Hamano wrote: >>> David Aguilar writes: >>> >>> > Marked "RFC" because I am kinda against adding more configuration >>> > variables. >>> >>> Just like "git merge" has -X escape hatch to allow us to >>> pass backend-specific options, perhaps you can add a mechanism to >>> "git mergetool" to let the user pass --no-auto from the command >>> line? >> >> We already have "mergetool..cmd" which allows a completely custom >> command line to be specified. > > Then probably it is a good idea to drop this patch and replace it > with a documentation patch to suggest those who do not want --auto > to use that mechanism? Generally, "mergetool..cmd" is not general enough since we've always special cased the base vs. no-base code paths and we run different commands depending on whether a base is available. It might make sense to extend that mechanism to allow "mergetool..merge2cmd" for the no-base form and allow .cmd to remain as-is for the full 3-arg base form, but IMO that's an even worse solution for the end user as they now need to configure two separate variables and know all the intimate details about how to configure it. OTOH, a boolean git configuration flag is much easier and simpler for the user. The -X--no-auto idea could work, but I was hoping for a "set once and forget" fix I could hand to a user, which would remove the need for such a feature. Here's the background behind why I wrote this patch -- yesterday someone at work asked, "does git mergetool not work with stash?" This caught me by surprise, "yes, of course it does", I replied, but they showed me a case where the behavior was confusing. They had done "git stash && git pull --rebase && git stash pop". That last pop created a merge conflict in one file. They then ran "git mergetool" to resolve the merge but they never saw the GUI and their changes were automatically staged. From a user's POV this is unintuitive and confusing behavior because they were expecting to run kdiff3, but instead it silently staged the file with kdiff3's auto-merged result. We could drop --auto altogether, which maybe is a better course of action since it makes the behavior predictable and un-surprising, but I do not know if anyone has come to rely on kdiff3's "auto-merge" feature (hence the extended Cc: list). Because I do not know exactly why "kdiff3" thinks these are "trivial merges" it can resolve while "git stash pop" disagrees and cannot trivially merge them, I seems that dropping "--auto" really may be the better choice all around. In general, git tries to be extremely careful and defers to the user in the face of ambiguity. Dropping --auto would make sense from that POV since kdiff3 is preventing the user from inspecting the result. In other words, if there are no strong reasons to keep --auto then I would not be opposed to changing the default. OTOH if there are users that want it and some that don't, then the config variable is helpful, and maybe it can be flipped the other way: call it "mergetool.kdiff3.autoMerge", keep it False today, and maybe change the default to True with Git 2.0. But I would really prefer no configuration and the most intuitive and least-surprising behavior. I think I've convinced myself that dropping --auto altogether could accomplish this, but I do not want to pull the rug out from under existing users, *if they exist*. If everyone in this discussion is in the camp of, "I didn't even know it worked that way" then changing the behavior is not so big a deal and we can do without extra variables. Would it be acceptable for me to rework this patch to drop --auto altogether? -- David -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/3] mergetools/kdiff3: allow opting-out of auto-merges
John Keeping writes: > On Thu, May 09, 2013 at 09:10:51AM -0700, Junio C Hamano wrote: >> David Aguilar writes: >> >> > Marked "RFC" because I am kinda against adding more configuration >> > variables. >> >> Just like "git merge" has -X escape hatch to allow us to >> pass backend-specific options, perhaps you can add a mechanism to >> "git mergetool" to let the user pass --no-auto from the command >> line? > > We already have "mergetool..cmd" which allows a completely custom > command line to be specified. Then probably it is a good idea to drop this patch and replace it with a documentation patch to suggest those who do not want --auto to use that mechanism? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/3] mergetools/kdiff3: allow opting-out of auto-merges
On Thu, May 09, 2013 at 09:10:51AM -0700, Junio C Hamano wrote: > David Aguilar writes: > > > Marked "RFC" because I am kinda against adding more configuration > > variables. > > Just like "git merge" has -X escape hatch to allow us to > pass backend-specific options, perhaps you can add a mechanism to > "git mergetool" to let the user pass --no-auto from the command > line? We already have "mergetool..cmd" which allows a completely custom command line to be specified. IIUC this can be used to override the built-in command for tool names that already exist. I'm not sure an extra -X buys us much on top of this, but perhaps it would be useful for one-off usage. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/3] mergetools/kdiff3: allow opting-out of auto-merges
David Aguilar writes: > Marked "RFC" because I am kinda against adding more configuration > variables. Just like "git merge" has -X escape hatch to allow us to pass backend-specific options, perhaps you can add a mechanism to "git mergetool" to let the user pass --no-auto from the command line? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 2/3] mergetools/kdiff3: allow opting-out of auto-merges
By default, "git mergetool" passes the `--auto` flag to `kdiff3` when
merging a file. The `--auto` flag tells `kdiff3` to skip showing the
GUI and automatically save the merged result when it is able to
trivially resolve a merge.
Some users prefer to eyeball the merged result using mergetool and the
use of `--auto` prevents them from doing so. Add a configuration
variable to allow opting-out of the auto-merge feature.
Signed-off-by: David Aguilar
---
Marked "RFC" because I am kinda against adding more configuration
variables. Someone ran into this and I did personally find the
behavior a bit surprising. Alternatively, we *could* change the
default behavior, but I am not convinced that doing so is a good idea
either, hence this patch. Other then the "kinda against",
it does make the behavior less surprising.
Documentation/merge-config.txt | 9 +
mergetools/kdiff3 | 20 ++--
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index d78d6d8..3bd5f21 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -59,6 +59,15 @@ merge.tool::
include::mergetools-merge.txt[]
+mergetool.kdiff3.manualMerge::
+ Tell Git that not to use `kdiff3`'s auto-merge feature.
+ By default, "git mergetool" passes the `--auto` flag to `kdiff3`
+ when merging a file. The `--auto` flag tells `kdiff3` to skip
+ showing the GUI and automatically save the merged result when it
+ is able to trivially resolve a merge. The `--auto` flag will
+ not be used when this variable is set to `true`. False by
+ default.
+
merge.verbosity::
Controls the amount of output shown by the recursive merge
strategy. Level 0 outputs nothing except a final error
diff --git a/mergetools/kdiff3 b/mergetools/kdiff3
index a30034f..d0acda9 100644
--- a/mergetools/kdiff3
+++ b/mergetools/kdiff3
@@ -1,3 +1,18 @@
+kdiff3_initialized=
+kdiff3_args=--auto
+
+kdiff3_init () {
+ if test -n "$kdiff3_initialized"
+ then
+ return
+ fi
+ if test "$(git config --bool mergetool.kdiff3.manualMerge)" = true
+ then
+ kdiff3_args=
+ fi
+ kdiff3_initialized=true
+}
+
diff_cmd () {
"$merge_tool_path" \
--L1 "$MERGED (A)" --L2 "$MERGED (B)" \
@@ -5,16 +20,17 @@ diff_cmd () {
}
merge_cmd () {
+ kdiff3_init
if $base_present
then
- "$merge_tool_path" --auto \
+ "$merge_tool_path" $kdiff3_args \
--L1 "$MERGED (Base)" \
--L2 "$MERGED (Local)" \
--L3 "$MERGED (Remote)" \
-o "$MERGED" "$BASE" "$LOCAL" "$REMOTE" \
>/dev/null 2>&1
else
- "$merge_tool_path" --auto \
+ "$merge_tool_path" $kdiff3_args \
--L1 "$MERGED (Local)" \
--L2 "$MERGED (Remote)" \
-o "$MERGED" "$LOCAL" "$REMOTE" \
--
1.8.3.rc1.38.g0f1704c
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
