{"thread":{"id":"42939","subject":"git-mergetool reverse file ordering","startedAt":"2016-07-27T10:14:35Z","lastAt":"2016-08-17T21:18:49Z","messageCount":10,"participants":["Luis Gutierrez","David Aguilar","John Keeping","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"292306","messageId":"CAD8hE_yzNZDPkxRy8s4Fy2_dZN5ppWzLM_2xc01C-VAdR1Pj_g@mail.gmail.com","threadId":"42939","inReplyTo":null,"subject":"git-mergetool reverse file ordering","fromName":"Luis Gutierrez","fromEmail":"luisgutz@gmail.com","sentAt":"2016-07-27T10:14:28Z","receivedAt":"2016-07-27T10:14:35Z","isPatch":false,"sender":{"key":"luisgutz@gmail.com","avatar":null},"body":"Hi,\n\nAttached is a potential patch for reversing the order on which\ngit-mergetool presents the files to merge.\n\nCurrently, when running git-mergetool, it performs a sort of the files\nto merge by alphabetical ordering. When working on C, this has the\nannoying effect of presenting the merge for a .c* files before the\nheader files; which is always a bit harder to do. Reading the header\nfirst to figure out what the other dude changed is usually preferred.\n\nThe attach patch reverse the order (-r flag to sort) so *.h* are\nmerged before  *.c* files\n\nPS, given the simplicity of the patch, I have not tested it.\n\nRegards\n\nLuis\n\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex bf86270..cce3b0d 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -453,10 +453,10 @@ then\n \tthen\n \t\tfiles=$(git rerere remaining)\n \telse\n-\t\tfiles=$(git ls-files -u | sed -e 's/^[^\t]*\t//' | sort -u)\n+\t\tfiles=$(git ls-files -u | sed -e 's/^[^\t]*\t//' | sort -u -r)\n \tfi\n else\n-\tfiles=$(git ls-files -u -- \"$@\" | sed -e 's/^[^\t]*\t//' | sort -u)\n+\tfiles=$(git ls-files -u -- \"$@\" | sed -e 's/^[^\t]*\t//' | sort -u -r)\n fi\n \n if test -z \"$files\"\n"},{"id":"299261","messageId":"20160814034221.GB21057@gmail.com","threadId":"42939","inReplyTo":"CAD8hE_yzNZDPkxRy8s4Fy2_dZN5ppWzLM_2xc01C-VAdR1Pj_g@mail.gmail.com","subject":"Re: git-mergetool reverse file ordering","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2016-08-14T03:42:21Z","receivedAt":"2016-08-14T09:16:18Z","isPatch":false,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Jul 27, 2016 at 11:14:28AM +0100, Luis Gutierrez wrote:\n> Hi,\n> \n> Attached is a potential patch for reversing the order on which\n> git-mergetool presents the files to merge.\n> \n> Currently, when running git-mergetool, it performs a sort of the files\n> to merge by alphabetical ordering. When working on C, this has the\n> annoying effect of presenting the merge for a .c* files before the\n> header files; which is always a bit harder to do. Reading the header\n> first to figure out what the other dude changed is usually preferred.\n> \n> The attach patch reverse the order (-r flag to sort) so *.h* are\n> merged before  *.c* files\n> \n> PS, given the simplicity of the patch, I have not tested it.\n> \n> Regards\n> \n> Luis\n\nThanks for the sug, this is an interesting idea and I definitely\nsee why we would want something like this...\n\n> diff --git a/git-mergetool.sh b/git-mergetool.sh\n> index bf86270..cce3b0d 100755\n> --- a/git-mergetool.sh\n> +++ b/git-mergetool.sh\n> @@ -453,10 +453,10 @@ then\n>  \tthen\n>  \t\tfiles=$(git rerere remaining)\n>  \telse\n> -\t\tfiles=$(git ls-files -u | sed -e 's/^[^\t]*\t//' | sort -u)\n> +\t\tfiles=$(git ls-files -u | sed -e 's/^[^\t]*\t//' | sort -u -r)\n>  \tfi\n>  else\n> -\tfiles=$(git ls-files -u -- \"$@\" | sed -e 's/^[^\t]*\t//' | sort -u)\n> +\tfiles=$(git ls-files -u -- \"$@\" | sed -e 's/^[^\t]*\t//' | sort -u -r)\n>  fi\n>  \n>  if test -z \"$files\"\n\nWhile we won't take this patch as-is (please see\nDocumentation/SubmittingPatches for details about the patch\nsubmission process), I am interested in the use case you've\ndescribed.\n\nThis use case makes me wonder whether the sorting we do here is\nsomething that should be opened up a bit so that the it's not\nquite so set in stone.\n\nFor example, an extension to the approach taken by this patch\nwould be to have `mergetool.reverseOrder` git config boolean\noption that would tell us whether or not to use the \"-r\" flag\nwhen calling sort.\n\nBut, IMO that is too rigid, and only addresses this narrow use\ncase.  What if users want a case-insensitive sort, or some other\npreferred ordering?\n\nWe can address these concerns, and your use case, by opening it\nup. Something like,\n\n\tsort=$(git config mergetool.sort || echo sort -u)\n\nThat preserves the existing behavior, and it opens it up so that\nwe can accomplish the same result as this patch by doing:\n\n\tgit config mergetool.sort \"sort -u -r\"\n\nThen, if someone later writes a nicer C/C++-specific sort that\nsorts in the natural order but also keeps .h files before .c,\n.cpp, etc. files with the same basename, then they could do:\n\n\tgit config mergetool.sort crescent-fresh-sort\n\n...and it'll be totally crescent.\n\nThoughts?  Would you be interested in helping work up a patch\nfor this idea?  At a minimum we should also write a test case in\nt/t7610-mergetool.sh to verify that it works as advertised.\n\nLet me know if you have any questions.\n\ncheers,\n-- \nDavid\n"},{"id":"299266","messageId":"20160814103801.tpixnskdsf5ycj66@john.keeping.me.uk","threadId":"42939","inReplyTo":"20160814034221.GB21057@gmail.com","subject":"Re: git-mergetool reverse file ordering","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-08-14T10:38:01Z","receivedAt":"2016-08-14T10:38:12Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sat, Aug 13, 2016 at 08:42:21PM -0700, David Aguilar wrote:\n> This use case makes me wonder whether the sorting we do here is\n> something that should be opened up a bit so that the it's not\n> quite so set in stone.\n> \n> For example, an extension to the approach taken by this patch\n> would be to have `mergetool.reverseOrder` git config boolean\n> option that would tell us whether or not to use the \"-r\" flag\n> when calling sort.\n> \n> But, IMO that is too rigid, and only addresses this narrow use\n> case.  What if users want a case-insensitive sort, or some other\n> preferred ordering?\n> \n> We can address these concerns, and your use case, by opening it\n> up. Something like,\n> \n> \tsort=$(git config mergetool.sort || echo sort -u)\n\nWhy not reuse the existing diff.orderFile config variable?  (Also\nsupported by the -O option to git-diff).\n"},{"id":"299388","messageId":"CAD8hE_xR2mB3=_oot9cWxmFy7z4oBhjABNOo3aJKN=bOw5ybzQ@mail.gmail.com","threadId":"42939","inReplyTo":"20160814103801.tpixnskdsf5ycj66@john.keeping.me.uk","subject":"Re: git-mergetool reverse file ordering","fromName":"Luis Gutierrez","fromEmail":"luisgutz@gmail.com","sentAt":"2016-08-15T20:19:35Z","receivedAt":"2016-08-15T20:19:41Z","isPatch":false,"sender":{"key":"luisgutz@gmail.com","avatar":null},"body":"> Thoughts?  Would you be interested in helping work up a patch\n> for this idea?  At a minimum we should also write a test case in\n> t/t7610-mergetool.sh to verify that it works as advertised.\n....\n> Why not reuse the existing diff.orderFile config variable?  (Also\n> supported by the -O option to git-diff).\n\n\nI'll be happy to write a testcase, and to re-use the -O\n(diff.orderFile config var) option to git-diff as sugguested by John\nKeeping.\n\nIs this the final spec?\n\n\n\nI'll be happy to do that.\n\nLuis\n"},{"id":"299524","messageId":"20160817012554.GA12991@gmail.com","threadId":"42939","inReplyTo":"CAD8hE_xR2mB3=_oot9cWxmFy7z4oBhjABNOo3aJKN=bOw5ybzQ@mail.gmail.com","subject":"Re: git-mergetool reverse file ordering","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2016-08-17T01:25:54Z","receivedAt":"2016-08-17T01:32:52Z","isPatch":false,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Mon, Aug 15, 2016 at 09:19:35PM +0100, Luis Gutierrez wrote:\n> > Thoughts?  Would you be interested in helping work up a patch\n> > for this idea?  At a minimum we should also write a test case in\n> > t/t7610-mergetool.sh to verify that it works as advertised.\n> ....\n> > Why not reuse the existing diff.orderFile config variable?  (Also\n> > supported by the -O option to git-diff).\n> \n> \n> I'll be happy to write a testcase, and to re-use the -O\n> (diff.orderFile config var) option to git-diff as sugguested by John\n> Keeping.\n> \n> Is this the final spec?\n> \n> \n> \n> I'll be happy to do that.\n> \n> Luis\n\nHmm, I do like the idea of reusing the diff orderFile, but a\nmechanism for sorting arbitrary inputs based on the orderFile\nisn't currently exposed in a way that mergetool could use it.\n\n\nLooking at the code in mergetool, we basically need something\nthat has the same spec as \"sort\" itself, namely that it can take\narbitrary arbitrary input on stdin and sort it.\n\n\nImplementing the orderFile support would probably be best done\nin C.  Would we want to expose it as an internal helper?\n\ne.g.\n\tgit diff--order-helper <order-file>\n\ncould be used to perform the sorting.\n\nBut, that sort is honestly kinda crude.  It can't implement the\ninteresting case where we want bar.h to sort before bar.c but\nnot foo.h.\n\nIf we did the sort option, we could have both.\n\nThoughts?\n\n-- \nDavid\n"},{"id":"299526","messageId":"18a1ef52-1c5b-4f0a-5da2-777e816a1e3e@kdbg.org","threadId":"42939","inReplyTo":"20160817012554.GA12991@gmail.com","subject":"Re: git-mergetool reverse file ordering","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-08-17T06:05:15Z","receivedAt":"2016-08-17T06:05:26Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 17.08.2016 um 03:25 schrieb David Aguilar:\n> Hmm, I do like the idea of reusing the diff orderFile, but a\n> mechanism for sorting arbitrary inputs based on the orderFile\n> isn't currently exposed in a way that mergetool could use it.\n\nInstead of using 'git ls-files -u | sed ... | sort -u' you could use\n\n   git diff-files --name-status -O... | sed -ne '/^U/s/^..//p'\n\n> But, that sort is honestly kinda crude.  It can't implement the\n> interesting case where we want bar.h to sort before bar.c but\n> not foo.h.\n>\n> If we did the sort option, we could have both.\n\nI don't think that we need a new filter when the diff machinery is \ncapable of re-ordering files. We should enhance the diff machinery instead.\n\n-- Hannes\n\n"},{"id":"299527","messageId":"882f5f1d-19a6-7b4b-7c6a-7347981fee72@kdbg.org","threadId":"42939","inReplyTo":"18a1ef52-1c5b-4f0a-5da2-777e816a1e3e@kdbg.org","subject":"Re: git-mergetool reverse file ordering","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-08-17T06:10:46Z","receivedAt":"2016-08-17T06:10:51Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 17.08.2016 um 08:05 schrieb Johannes Sixt:\n> Am 17.08.2016 um 03:25 schrieb David Aguilar:\n>> Hmm, I do like the idea of reusing the diff orderFile, but a\n>> mechanism for sorting arbitrary inputs based on the orderFile\n>> isn't currently exposed in a way that mergetool could use it.\n>\n> Instead of using 'git ls-files -u | sed ... | sort -u' you could use\n>\n>   git diff-files --name-status -O... | sed -ne '/^U/s/^..//p'\n\nOr even better:\n\n     git diff-files --name-only --diff-filter=U -O...\n\n>\n>> But, that sort is honestly kinda crude.  It can't implement the\n>> interesting case where we want bar.h to sort before bar.c but\n>> not foo.h.\n>>\n>> If we did the sort option, we could have both.\n>\n> I don't think that we need a new filter when the diff machinery is\n> capable of re-ordering files. We should enhance the diff machinery instead.\n>\n> -- Hannes\n>\n\n"},{"id":"299530","messageId":"20160817064612.GA14619@gmail.com","threadId":"42939","inReplyTo":"882f5f1d-19a6-7b4b-7c6a-7347981fee72@kdbg.org","subject":"Re: git-mergetool reverse file ordering","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2016-08-17T06:46:12Z","receivedAt":"2016-08-17T06:46:25Z","isPatch":false,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Aug 17, 2016 at 08:10:46AM +0200, Johannes Sixt wrote:\n> Am 17.08.2016 um 08:05 schrieb Johannes Sixt:\n> > Am 17.08.2016 um 03:25 schrieb David Aguilar:\n> > > Hmm, I do like the idea of reusing the diff orderFile, but a\n> > > mechanism for sorting arbitrary inputs based on the orderFile\n> > > isn't currently exposed in a way that mergetool could use it.\n> > \n> > Instead of using 'git ls-files -u | sed ... | sort -u' you could use\n> > \n> >   git diff-files --name-status -O... | sed -ne '/^U/s/^..//p'\n> \n> Or even better:\n> \n>     git diff-files --name-only --diff-filter=U -O...\n\nNice!\n\n\n> > > But, that sort is honestly kinda crude.  It can't implement the\n> > > interesting case where we want bar.h to sort before bar.c but\n> > > not foo.h.\n> > > \n\nNote: I had a subtle typo above -- I meant to describe this desired\norder:\n\tbar.h\n\tbar.c\n\tfoo.h\n\tfoo.c\n\nwhich is not possible with an orderFile that has only:\n\n\t*.h\n\t*.c\n\nBut...\n\n> > > If we did the sort option, we could have both.\n> > \n> > I don't think that we need a new filter when the diff machinery is\n> > capable of re-ordering files. We should enhance the diff machinery instead.\n> > \n\nReading the code more thoroughly, I fully agree.\n\nSwitching to a diff-files invocation lets us reuse the\ndiff.orderFile machinery and does not require exposing any\nadditional helpers.\n\nWhile it would be *nice* if we had a way to sort foo.h before\nfoo.c and after bar.c, that's just a pie-in-the-sky idea.\nConsistency is king.\n\n\nThe only thing that using diff-files doesn't address is the\nrerere support in mergetool where it processes the files in\nthe order specified by \"git rerere remaining\".  This is why I\ninitially thought we needed a generic sort-like command.\n\nThat code path does not currently do any sorting (which may\nexplain why we didn't notice it if we were just looking for\n\"sort\" invocations) but maybe that's an edge case that we don't\nneed to address initially (if at all).\n\nWould it be okay for the rerere code path to not honor the\norderFile?  IMO if we documented the behavior then it would be\nacceptable, and perhaps can be improved as a follow-up.\n\nFor the basic support the implementation becomes really\nsimple: switch to using diff-files instead of ls-files.\n\nIf we did want consistency in the \"git rerere remaining\" code\npath, would it be acceptable to teach \"rerere\" about\n\"-O<orderfile>\" so that we could drive it from mergetool?\n\nI only suggest an option, and not config support, because I\nwould be hesitant to make \"rerere remaining\" unconditionally\nhonor the orderFile since that feature is diff-centric, while\n\"rerere remaining\" is a different beast altogether.  Adding a\nflag is a middle-ground that would allow mergetool to drive it\nwhile not suddently changing rerere's behavior.\n\n\nThe patches could then be:\n\n1. switch to diff-files, add tests, and document how\n   diff.orderFile affects mergetool.\n\n2. Teach mergetool about the \"-O<orderFile>\" flag so that it can\n   override the configuration, and add tests.  It could be\n   argued that this should be squashed into (1).\n\n3. (optional) teach \"rerere remaining\" to honor the\n   -O<orderfile> flag and teach mergetool to supply the option.\n\nSound good?\n\n\nciao,\n-- \nDavid\n"},{"id":"299531","messageId":"1c089d6b-0acc-a5da-91bf-1887b6eaedbb@kdbg.org","threadId":"42939","inReplyTo":"20160817064612.GA14619@gmail.com","subject":"Re: git-mergetool reverse file ordering","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-08-17T07:35:56Z","receivedAt":"2016-08-17T07:36:03Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 17.08.2016 um 08:46 schrieb David Aguilar:\n> The only thing that using diff-files doesn't address is the\n> rerere support in mergetool where it processes the files in\n> the order specified by \"git rerere remaining\".  This is why I\n> initially thought we needed a generic sort-like command.\n\nI see. This is actually an important code path. How about this code \nstructure:\n\nif test $# -eq 0\nthen\n\tcd_to_toplevel\n\n\tif test -e \"$GIT_DIR/MERGE_RR\"\n\tthen\n\t\tset -- $(git rerere remaining)\n\tfi\nfi\nfiles=$(git diff-files --name-only --diff-filter=U -- \"$@\")\n\nThis does not require an enhancement of rerere-remaining and still \ncaptures all three cases that currently go through separate branches. \n(Throw in some version of --ignore-submodules= if necessary, but I guess \nit is not.)\n\nWe do have a problem if there are file names with spaces, but it is not \na new problem.\n\n> The patches could then be:\n>\n> 1. switch to diff-files, add tests, and document how\n>    diff.orderFile affects mergetool.\n>\n> 2. Teach mergetool about the \"-O<orderFile>\" flag so that it can\n>    override the configuration, and add tests.  It could be\n>    argued that this should be squashed into (1).\n>\n> 3. (optional) teach \"rerere remaining\" to honor the\n>    -O<orderfile> flag and teach mergetool to supply the option.\n>\n> Sound good?\n\nSure, except that 3. won't be necessary.\n\n-- Hannes\n\n"},{"id":"299575","messageId":"20160817211828.GB14619@gmail.com","threadId":"42939","inReplyTo":"1c089d6b-0acc-a5da-91bf-1887b6eaedbb@kdbg.org","subject":"Re: git-mergetool reverse file ordering","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2016-08-17T21:18:28Z","receivedAt":"2016-08-17T21:18:49Z","isPatch":false,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"\nHi Luis and Hannes,\n\nOn Wed, Aug 17, 2016 at 09:35:56AM +0200, Johannes Sixt wrote:\n> Am 17.08.2016 um 08:46 schrieb David Aguilar:\n> > The only thing that using diff-files doesn't address is the\n> > rerere support in mergetool where it processes the files in\n> > the order specified by \"git rerere remaining\".  This is why I\n> > initially thought we needed a generic sort-like command.\n> \n> I see. This is actually an important code path. How about this code\n> structure:\n> \n> if test $# -eq 0\n> then\n> \tcd_to_toplevel\n> \n> \tif test -e \"$GIT_DIR/MERGE_RR\"\n> \tthen\n> \t\tset -- $(git rerere remaining)\n> \tfi\n> fi\n> files=$(git diff-files --name-only --diff-filter=U -- \"$@\")\n> \n\nBeautiful.\n\n> This does not require an enhancement of rerere-remaining and still captures\n> all three cases that currently go through separate branches. (Throw in some\n> version of --ignore-submodules= if necessary, but I guess it is not.)\n> \n> We do have a problem if there are file names with spaces, but it is not a\n> new problem.\n\nThanks for the heads-up about file names with spaces.  We set,\n\nIFS='\n'\n\nin git-mergetool--lib.sh so file names with spaces should be ok.\nNaturally, we won't be able to support paths with embedded\nnewlines, but that's not a new problem ;-)\n\nWe should probably also set core.quotePath=false when calling\ndiff-files so that git doesn't try to quote \"unusual\" paths,\ne.g.\n\n\tgit -c core.quotePath=false diff-files ...\n\nLastly, for anyone that's curious, I was wondering why we were\npassing \"-u\" to \"sort\", and why we won't need to use \"uniq\" in\nthe new version.\n\nThe reason is that \"ls-files -u\" lists the different index\nstages separately, and thus it reports duplicate paths.\n\n\"diff-files\" with \"--diff-filter=U\" does not do that, so that's\nanother benefit to be gained from this change.\n\nI think we've touched all the bases now.\n\nLuis, I hope that makes sense.  Let us know if any of this is\nunclear.\n\n\nciao,\n-- \nDavid\n"}]}