{"thread":{"id":"33759","subject":"[PATCH 1/3] mergetools/kdiff3: do not use --auto when diffing","startedAt":"2013-05-09T09:13:28Z","lastAt":"2013-05-10T18:40:50Z","messageCount":11,"participants":["David Aguilar","John Keeping","Junio C Hamano","Charles Bailey"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"216756","messageId":"1368090810-40596-1-git-send-email-davvid@gmail.com","threadId":"33759","inReplyTo":null,"subject":"[PATCH 1/3] mergetools/kdiff3: do not use --auto when diffing","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-05-09T09:13:28Z","receivedAt":"2013-05-09T09:13:28Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"The `kdiff3 --auto` help message is, \"No GUI if all conflicts are auto-\nsolvable.\"  This flag was carried over from the original mergetool\ncommands.  diff_cmd() is for two-way comparisons only so remove the\nsuperfluous flag.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nThis one is not RFC; just a trivial fix.\n\n mergetools/kdiff3 | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/mergetools/kdiff3 b/mergetools/kdiff3\nindex 28fead4..a30034f 100644\n--- a/mergetools/kdiff3\n+++ b/mergetools/kdiff3\n@@ -1,5 +1,5 @@\n diff_cmd () {\n-\t\"$merge_tool_path\" --auto \\\n+\t\"$merge_tool_path\" \\\n \t\t--L1 \"$MERGED (A)\" --L2 \"$MERGED (B)\" \\\n \t\t\"$LOCAL\" \"$REMOTE\" >/dev/null 2>&1\n }\n-- \n1.8.3.rc1.38.g0f1704c\n"},{"id":"216757","messageId":"1368090810-40596-2-git-send-email-davvid@gmail.com","threadId":"33759","inReplyTo":"1368090810-40596-1-git-send-email-davvid@gmail.com","subject":"[RFC PATCH 2/3] mergetools/kdiff3: allow opting-out of auto-merges","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-05-09T09:13:29Z","receivedAt":"2013-05-09T09:13:29Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"By default, \"git mergetool\" passes the `--auto` flag to `kdiff3` when\nmerging a file.  The `--auto` flag tells `kdiff3` to skip showing the\nGUI and automatically save the merged result when it is able to\ntrivially resolve a merge.\n\nSome users prefer to eyeball the merged result using mergetool and the\nuse of `--auto` prevents them from doing so.  Add a configuration\nvariable to allow opting-out of the auto-merge feature.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nMarked \"RFC\" because I am kinda against adding more configuration\nvariables.  Someone ran into this and I did personally find the\nbehavior a bit surprising.  Alternatively, we *could* change the\ndefault behavior, but I am not convinced that doing so is a good idea\neither, hence this patch.  Other then the \"kinda against\",\nit does make the behavior less surprising.\n\n Documentation/merge-config.txt |  9 +++++++++\n mergetools/kdiff3              | 20 ++++++++++++++++++--\n 2 files changed, 27 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex d78d6d8..3bd5f21 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -59,6 +59,15 @@ merge.tool::\n \n include::mergetools-merge.txt[]\n \n+mergetool.kdiff3.manualMerge::\n+\tTell Git that not to use `kdiff3`'s auto-merge feature.\n+\tBy default, \"git mergetool\" passes the `--auto` flag to `kdiff3`\n+\twhen merging a file.  The `--auto` flag tells `kdiff3` to skip\n+\tshowing the GUI and automatically save the merged result when it\n+\tis able to trivially resolve a merge.  The `--auto` flag will\n+\tnot be used when this variable is set to `true`.  False by\n+\tdefault.\n+\n merge.verbosity::\n \tControls the amount of output shown by the recursive merge\n \tstrategy.  Level 0 outputs nothing except a final error\ndiff --git a/mergetools/kdiff3 b/mergetools/kdiff3\nindex a30034f..d0acda9 100644\n--- a/mergetools/kdiff3\n+++ b/mergetools/kdiff3\n@@ -1,3 +1,18 @@\n+kdiff3_initialized=\n+kdiff3_args=--auto\n+\n+kdiff3_init () {\n+\tif test -n \"$kdiff3_initialized\"\n+\tthen\n+\t\treturn\n+\tfi\n+\tif test \"$(git config --bool mergetool.kdiff3.manualMerge)\" = true\n+\tthen\n+\t\tkdiff3_args=\n+\tfi\n+\tkdiff3_initialized=true\n+}\n+\n diff_cmd () {\n \t\"$merge_tool_path\" \\\n \t\t--L1 \"$MERGED (A)\" --L2 \"$MERGED (B)\" \\\n@@ -5,16 +20,17 @@ diff_cmd () {\n }\n \n merge_cmd () {\n+\tkdiff3_init\n \tif $base_present\n \tthen\n-\t\t\"$merge_tool_path\" --auto \\\n+\t\t\"$merge_tool_path\" $kdiff3_args \\\n \t\t\t--L1 \"$MERGED (Base)\" \\\n \t\t\t--L2 \"$MERGED (Local)\" \\\n \t\t\t--L3 \"$MERGED (Remote)\" \\\n \t\t\t-o \"$MERGED\" \"$BASE\" \"$LOCAL\" \"$REMOTE\" \\\n \t\t>/dev/null 2>&1\n \telse\n-\t\t\"$merge_tool_path\" --auto \\\n+\t\t\"$merge_tool_path\" $kdiff3_args \\\n \t\t\t--L1 \"$MERGED (Local)\" \\\n \t\t\t--L2 \"$MERGED (Remote)\" \\\n \t\t\t-o \"$MERGED\" \"$LOCAL\" \"$REMOTE\" \\\n-- \n1.8.3.rc1.38.g0f1704c\n"},{"id":"216758","messageId":"1368090810-40596-3-git-send-email-davvid@gmail.com","threadId":"33759","inReplyTo":"1368090810-40596-2-git-send-email-davvid@gmail.com","subject":"[RFC PATCH 3/3] Makefile: avoid deprecation warnings on OS X 10.8","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-05-09T09:13:30Z","receivedAt":"2013-05-09T09:13:30Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Mac OS X Mountain Lion prints warnings when building git:\n\n\twarning: 'SHA1_Init' is deprecated\n\t(declared at /usr/include/openssl/sha.h:121)\n\nSilence the warnings by disabling OpenSSH in favor of BLK_SHA1.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nI know I can create config.mak, but do we prefer to have the default\nsettings be warning-free?  I do not see any other platforms that tweak\nNO_OPENSSL themselves, hence \"RFC\".  Is there a better way to do this?\nAre there any Darwin/PPC users that would be harmed by this patch?\n\n Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Makefile b/Makefile\nindex 0f931a2..3bb9ac2 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1055,6 +1055,7 @@ ifeq ($(uname_S),Darwin)\n \t\tendif\n \tendif\n \tPTHREAD_LIBS =\n+\tNO_OPENSSL = YesPlease\n endif\n \n ifndef CC_LD_DYNPATH\n-- \n1.8.3.rc1.38.g0f1704c\n"},{"id":"216772","messageId":"20130509151436.GJ25912@serenity.lan","threadId":"33759","inReplyTo":"1368090810-40596-3-git-send-email-davvid@gmail.com","subject":"Re: [RFC PATCH 3/3] Makefile: avoid deprecation warnings on OS X 10.8","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-09T15:14:36Z","receivedAt":"2013-05-09T15:14:36Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, May 09, 2013 at 02:13:30AM -0700, David Aguilar wrote:\n> Mac OS X Mountain Lion prints warnings when building git:\n> \n> \twarning: 'SHA1_Init' is deprecated\n> \t(declared at /usr/include/openssl/sha.h:121)\n> \n> Silence the warnings by disabling OpenSSH in favor of BLK_SHA1.\n> \n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n> I know I can create config.mak, but do we prefer to have the default\n> settings be warning-free?  I do not see any other platforms that tweak\n> NO_OPENSSL themselves, hence \"RFC\".  Is there a better way to do this?\n> Are there any Darwin/PPC users that would be harmed by this patch?\n\nDisabling OpenSSL also has the effect of disabling SSL support in\ngit-imap-send.  Does enabling BLK_SHA1 instead also remove the warnings?\n\nAlternatively, it seems that the recommended update is to use Apple's\nCommonCrypto library, as in this patch:\nhttps://gist.github.com/anonymous/4466305\n"},{"id":"216774","messageId":"7vli7ob0c4.fsf@alter.siamese.dyndns.org","threadId":"33759","inReplyTo":"1368090810-40596-2-git-send-email-davvid@gmail.com","subject":"Re: [RFC PATCH 2/3] mergetools/kdiff3: allow opting-out of auto-merges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-09T16:10:51Z","receivedAt":"2013-05-09T16:10:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> Marked \"RFC\" because I am kinda against adding more configuration\n> variables.\n\nJust like \"git merge\" has -X<option> escape hatch to allow us to\npass backend-specific options, perhaps you can add a mechanism to\n\"git mergetool\" to let the user pass --no-auto from the command\nline?\n"},{"id":"216798","messageId":"20130509172334.GK25912@serenity.lan","threadId":"33759","inReplyTo":"7vli7ob0c4.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH 2/3] mergetools/kdiff3: allow opting-out of auto-merges","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-09T17:23:34Z","receivedAt":"2013-05-09T17:23:34Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, May 09, 2013 at 09:10:51AM -0700, Junio C Hamano wrote:\n> David Aguilar <davvid@gmail.com> writes:\n> \n> > Marked \"RFC\" because I am kinda against adding more configuration\n> > variables.\n> \n> Just like \"git merge\" has -X<option> escape hatch to allow us to\n> pass backend-specific options, perhaps you can add a mechanism to\n> \"git mergetool\" to let the user pass --no-auto from the command\n> line?\n\nWe already have \"mergetool.<tool>.cmd\" which allows a completely custom\ncommand line to be specified.  IIUC this can be used to override the\nbuilt-in command for tool names that already exist.\n\nI'm not sure an extra -X<option> buys us much on top of this, but\nperhaps it would be useful for one-off usage.\n"},{"id":"216807","messageId":"7v38tw9d7r.fsf@alter.siamese.dyndns.org","threadId":"33759","inReplyTo":"20130509172334.GK25912@serenity.lan","subject":"Re: [RFC PATCH 2/3] mergetools/kdiff3: allow opting-out of auto-merges","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-09T19:15:36Z","receivedAt":"2013-05-09T19:15:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> On Thu, May 09, 2013 at 09:10:51AM -0700, Junio C Hamano wrote:\n>> David Aguilar <davvid@gmail.com> writes:\n>> \n>> > Marked \"RFC\" because I am kinda against adding more configuration\n>> > variables.\n>> \n>> Just like \"git merge\" has -X<option> escape hatch to allow us to\n>> pass backend-specific options, perhaps you can add a mechanism to\n>> \"git mergetool\" to let the user pass --no-auto from the command\n>> line?\n>\n> We already have \"mergetool.<tool>.cmd\" which allows a completely custom\n> command line to be specified.\n\nThen probably it is a good idea to drop this patch and replace it\nwith a documentation patch to suggest those who do not want --auto\nto use that mechanism?\n"},{"id":"216821","messageId":"CAJDDKr4S6=U1d1fWixaiAzf46KLBDNsi85fgvXy0Uu_aRJyoyw@mail.gmail.com","threadId":"33759","inReplyTo":"7v38tw9d7r.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH 2/3] mergetools/kdiff3: allow opting-out of auto-merges","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-05-09T22:17:30Z","receivedAt":"2013-05-09T22:17:30Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, May 9, 2013 at 12:15 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> John Keeping <john@keeping.me.uk> writes:\n>\n>> On Thu, May 09, 2013 at 09:10:51AM -0700, Junio C Hamano wrote:\n>>> David Aguilar <davvid@gmail.com> writes:\n>>>\n>>> > Marked \"RFC\" because I am kinda against adding more configuration\n>>> > variables.\n>>>\n>>> Just like \"git merge\" has -X<option> escape hatch to allow us to\n>>> pass backend-specific options, perhaps you can add a mechanism to\n>>> \"git mergetool\" to let the user pass --no-auto from the command\n>>> line?\n>>\n>> We already have \"mergetool.<tool>.cmd\" which allows a completely custom\n>> command line to be specified.\n>\n> Then probably it is a good idea to drop this patch and replace it\n> with a documentation patch to suggest those who do not want --auto\n> to use that mechanism?\n\nGenerally, \"mergetool.<tool>.cmd\" is not general enough since we've\nalways special cased the base vs. no-base code paths and we run\ndifferent commands depending on whether a base is available.\n\nIt might make sense to extend that mechanism to allow\n\"mergetool.<tool>.merge2cmd\" for the no-base form and allow .cmd to\nremain as-is for the full 3-arg base form, but IMO that's an even\nworse solution for the end user as they now need to configure two\nseparate variables and know all the intimate details about how to\nconfigure it.  OTOH, a boolean git configuration flag is much easier\nand simpler for the user.\n\nThe -X--no-auto idea could work, but I was hoping for a \"set once and\nforget\" fix I could hand to a user, which would remove the need for\nsuch a feature.\n\nHere's the background behind why I wrote this patch -- yesterday\nsomeone at work asked, \"does git mergetool not work with stash?\"  This\ncaught me by surprise, \"yes, of course it does\", I replied, but they\nshowed me a case where the behavior was confusing.\n\nThey had done \"git stash && git pull --rebase && git stash pop\".  That\nlast pop created a merge conflict in one file.\n\nThey then ran \"git mergetool\" to resolve the merge but they never saw\nthe GUI and their changes were automatically staged.  From a user's\nPOV this is unintuitive and confusing behavior because they were\nexpecting to run kdiff3, but instead it silently staged the file with\nkdiff3's auto-merged result.\n\nWe could drop --auto altogether, which maybe is a better course of\naction since it makes the behavior predictable and un-surprising, but\nI do not know if anyone has come to rely on kdiff3's \"auto-merge\"\nfeature (hence the extended Cc: list).\n\nBecause I do not know exactly why \"kdiff3\" thinks these are \"trivial\nmerges\" it can resolve while \"git stash pop\" disagrees and cannot\ntrivially merge them, I seems that dropping \"--auto\" really may be the\nbetter choice all around.  In general, git tries to be extremely\ncareful and defers to the user in the face of ambiguity.  Dropping\n--auto would make sense from that POV since kdiff3 is preventing the\nuser from inspecting the result.\n\nIn other words, if there are no strong reasons to keep --auto then I\nwould not be opposed to changing the default.  OTOH if there are users\nthat want it and some that don't, then the config variable is helpful,\nand maybe it can be flipped the other way:  call it\n\"mergetool.kdiff3.autoMerge\", keep it False today, and maybe change\nthe default to True with Git 2.0.\n\nBut I would really prefer no configuration and the most intuitive and\nleast-surprising behavior.  I think I've convinced myself that\ndropping --auto altogether could accomplish this, but I do not want to\npull the rug out from under existing users, *if they exist*.  If\neveryone in this discussion is in the camp of, \"I didn't even know it\nworked that way\" then changing the behavior is not so big a deal and\nwe can do without extra variables.\n\nWould it be acceptable for me to rework this patch to drop --auto altogether?\n--\nDavid\n"},{"id":"216836","messageId":"CAJDDKr5=tQu-TWaBKiwUzCKF5+=P6360puEgY_o9DtsbfQz1Vw@mail.gmail.com","threadId":"33759","inReplyTo":"20130509151436.GJ25912@serenity.lan","subject":"Re: [RFC PATCH 3/3] Makefile: avoid deprecation warnings on OS X 10.8","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-05-09T23:21:55Z","receivedAt":"2013-05-09T23:21:55Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, May 9, 2013 at 8:14 AM, John Keeping <john@keeping.me.uk> wrote:\n> On Thu, May 09, 2013 at 02:13:30AM -0700, David Aguilar wrote:\n>> Mac OS X Mountain Lion prints warnings when building git:\n>>\n>>       warning: 'SHA1_Init' is deprecated\n>>       (declared at /usr/include/openssl/sha.h:121)\n>>\n>> Silence the warnings by disabling OpenSSH in favor of BLK_SHA1.\n>>\n>> Signed-off-by: David Aguilar <davvid@gmail.com>\n>> ---\n>> I know I can create config.mak, but do we prefer to have the default\n>> settings be warning-free?  I do not see any other platforms that tweak\n>> NO_OPENSSL themselves, hence \"RFC\".  Is there a better way to do this?\n>> Are there any Darwin/PPC users that would be harmed by this patch?\n>\n> Disabling OpenSSL also has the effect of disabling SSL support in\n> git-imap-send.  Does enabling BLK_SHA1 instead also remove the warnings?\n\nThanks.  Yes, setting BLK_SHA1 does eliminate the warnings.\n\nI'll re-roll this patch and send it independently.\nIt's unrelated to the mergetool stuff so apologies for the wide\ninitial Cc: list.\n\n> Alternatively, it seems that the recommended update is to use Apple's\n> CommonCrypto library, as in this patch:\n> https://gist.github.com/anonymous/4466305\n\nIt seems like we'd want something like this in compat-util.h or\nsomewhere similar.  That'll be a bigger change so I'll try the\nBLK_SHA1 approach first since it is minimally invasive.  This would be\nnice to tackle once the dust has settled a bit.\n--\nDavid\n"},{"id":"216848","messageId":"20130510054130.GA29215@hashpling.org","threadId":"33759","inReplyTo":"CAJDDKr4S6=U1d1fWixaiAzf46KLBDNsi85fgvXy0Uu_aRJyoyw@mail.gmail.com","subject":"Re: [RFC PATCH 2/3] mergetools/kdiff3: allow opting-out of auto-merges","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2013-05-10T05:41:30Z","receivedAt":"2013-05-10T05:41:30Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Thu, May 09, 2013 at 03:17:30PM -0700, David Aguilar wrote:\n> Generally, \"mergetool.<tool>.cmd\" is not general enough since we've\n> always special cased the base vs. no-base code paths and we run\n> different commands depending on whether a base is available.\n\nThen this is a deficiency of the \".cmd\" mechanism which should provide\nan \"if all else fails\" way to do things, even if ugly. We could simply\nadd a BASELESS variable to the eval environment of the custom command.\n(Do we always create an empty file for $BASE, now?)\n\n> We could drop --auto altogether, which maybe is a better course of\n> action since it makes the behavior predictable and un-surprising, but\n> I do not know if anyone has come to rely on kdiff3's \"auto-merge\"\n> feature (hence the extended Cc: list).\n\nI disagree, I think that a mergetool should be allowed to be as\nhelpful as possible and if it can resolve the merge unaided then this\nis no bad thing. I've worked with a number of people who were very\nhappy with the current kdiff3 behaviour. Nothing prevents you from\nverifying the merge and fixing it up if it wasn't done perfectly by\nthe tool, although I haven't ever encountered this with git+kdiff3.\n\nCharles.\n"},{"id":"216926","messageId":"CAJDDKr4RUzFXJpuFjCV00TE0ZmrzZzVC4uO=2uJVSxoY_==JcQ@mail.gmail.com","threadId":"33759","inReplyTo":"20130510054130.GA29215@hashpling.org","subject":"Re: [RFC PATCH 2/3] mergetools/kdiff3: allow opting-out of auto-merges","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-05-10T18:40:50Z","receivedAt":"2013-05-10T18:40:50Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, May 9, 2013 at 10:41 PM, Charles Bailey <charles@hashpling.org> wrote:\n> On Thu, May 09, 2013 at 03:17:30PM -0700, David Aguilar wrote:\n>> Generally, \"mergetool.<tool>.cmd\" is not general enough since we've\n>> always special cased the base vs. no-base code paths and we run\n>> different commands depending on whether a base is available.\n>\n> Then this is a deficiency of the \".cmd\" mechanism which should provide\n> an \"if all else fails\" way to do things, even if ugly. We could simply\n> add a BASELESS variable to the eval environment of the custom command.\n> (Do we always create an empty file for $BASE, now?)\n>\n>> We could drop --auto altogether, which maybe is a better course of\n>> action since it makes the behavior predictable and un-surprising, but\n>> I do not know if anyone has come to rely on kdiff3's \"auto-merge\"\n>> feature (hence the extended Cc: list).\n>\n> I disagree, I think that a mergetool should be allowed to be as\n> helpful as possible and if it can resolve the merge unaided then this\n> is no bad thing. I've worked with a number of people who were very\n> happy with the current kdiff3 behaviour. Nothing prevents you from\n> verifying the merge and fixing it up if it wasn't done perfectly by\n> the tool, although I haven't ever encountered this with git+kdiff3.\n\nThat's the bit of info I was needing.\n\nI can always tell the one person who ran into this that \"that's how it\nis\" and explain why it's a good thing.\n\n\"one person\" < \"number of people\" so it's an easy decision, and we\ndon't need to make the tool worse to cater to someone that's new to\ngit.\n\nThanks Charles,\n--\nDavid\n"}]}