{"thread":{"id":"51846","subject":"diff.renames not working?","startedAt":"2019-09-13T20:24:21Z","lastAt":"2019-09-16T17:35:51Z","messageCount":5,"participants":["Robert Dailey","Bryan Turner","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"382331","messageId":"CAHd499BT35jvPtsuD9gfJB0HJ=NxtzyQOaiD7-=sHJbFYhphpg@mail.gmail.com","threadId":"51846","inReplyTo":null,"subject":"diff.renames not working?","fromName":"Robert Dailey","fromEmail":"rcdailey.lists@gmail.com","sentAt":"2019-09-13T20:24:06Z","receivedAt":"2019-09-13T20:24:21Z","isPatch":false,"sender":{"key":"rcdailey.lists@gmail.com","avatar":null},"body":"I'm using Git version 2.23. I have the `diff.renames` setting set to\n`copies`. My code base has a file named `JniPaymentManager.hpp` (and\n`cpp`) that had too much code in it, so I refactored this file by\nsplitting out a significant portion of the code in it to another file,\nnamed `ZPayClient.hpp` (and `cpp`). When I diff the topic branch\nagainst the mainline, it shows it as a copy with a percentage:\n\n```\n$ git diff --name-status master...topic\nC055    Jni/JniPaymentManager.cpp    ZPay/ZPayClient.cpp\nC070    Jni/JniPaymentManager.hpp    ZPay/ZPayClient.hpp\n```\n\nNow my goal is to diff `ZPayClient.hpp` and see the changes to the\nmoved-out portion of code as it relates to the original state of that\ncode in `JniPaymentManager.hpp`. To do this, I tried this command:\n\n```\n$ git diff master...topic -- ZPay/ZPayClient.hpp\n```\n\nThe unified diff header I got back is:\n\n```\ndiff --git ZPay/ZPayClient.hpp ZPay/ZPayClient.hpp\nnew file mode 100644\nindex 00000000..6ebc2a9a\n--- /dev/null\n+++ ZPay/ZPayClient.hpp\n```\n\nHmm, it's treating it as a new file. Even though I have `diff.renames`\nset to `copies`? Even though `diff --name-status` acknowledges the\nrelationship with the original file for the code on `master`? This is\nconfusing...\n\nOut of curiosity, I thought I'd try this command:\n\n```\ngit diff --follow master...topic -- ZPay/ZPayClient.hpp\n```\n\nAnd I get this unified diff header:\n\n```\ndiff --git Jni/JniPaymentManager.hpp ZPay/ZPayClient.hpp\nsimilarity index 70%\ncopy from Jni/JniPaymentManager.hpp\ncopy to ZPay/ZPayClient.hpp\nindex fc18e2d2..6ebc2a9a 100644\n--- Jni/JniPaymentManager.hpp\n+++ ZPay/ZPayClient.hpp\n```\n\nNow this looks more like it. I can actually see a useful diff here,\ninstead of everything looking like a new file. But there is a lot of\nconfusion here:\n\n1. `diff --follow` is not a documented[1] option. Why does it work?\n2. `diff -M` doesn't actually work either. It should, though. In fact,\nI expected it to work as `--follow` does. But it doesn't.\n3. The `diff.renames` config doesn't seem to be working here, when it should.\n\nCan someone explain the behavior I'm seeing? I really am confused\nabout all this...\n\n[1]: https://git-scm.com/docs/git-diff\n"},{"id":"382348","messageId":"CAGyf7-F95A848hdM-CWPZvuvaSHF7p4qEftGoXMHNM5nyfDkeg@mail.gmail.com","threadId":"51846","inReplyTo":"CAHd499BT35jvPtsuD9gfJB0HJ=NxtzyQOaiD7-=sHJbFYhphpg@mail.gmail.com","subject":"Re: diff.renames not working?","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2019-09-13T22:36:15Z","receivedAt":"2019-09-13T22:36:28Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Fri, Sep 13, 2019 at 1:24 PM Robert Dailey <rcdailey.lists@gmail.com> wrote:\n>\n> 2. `diff -M` doesn't actually work either. It should, though. In fact,\n> I expected it to work as `--follow` does. But it doesn't.\n\nJust a small point of clarification: Is -M really what you mean here?\nGiven you indicated you have \"diff.renames=copies\", wouldn't you need\n-C? -M only detects renames, not copies.\n\n(I haven't looked too deeply into the rest, but this detail caught my eye.)\n\nBest regards,\nBryan Turner\n\n\n> 3. The `diff.renames` config doesn't seem to be working here, when it should.\n>\n> Can someone explain the behavior I'm seeing? I really am confused\n> about all this...\n>\n> [1]: https://git-scm.com/docs/git-diff\n"},{"id":"382357","messageId":"20190914033017.GA30458@sigill.intra.peff.net","threadId":"51846","inReplyTo":"CAHd499BT35jvPtsuD9gfJB0HJ=NxtzyQOaiD7-=sHJbFYhphpg@mail.gmail.com","subject":"Re: diff.renames not working?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-14T03:30:17Z","receivedAt":"2019-09-14T03:30:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 13, 2019 at 03:24:06PM -0500, Robert Dailey wrote:\n\n> Now my goal is to diff `ZPayClient.hpp` and see the changes to the\n> moved-out portion of code as it relates to the original state of that\n> code in `JniPaymentManager.hpp`. To do this, I tried this command:\n> \n> ```\n> $ git diff master...topic -- ZPay/ZPayClient.hpp\n> ```\n> \n> The unified diff header I got back is:\n> \n> ```\n> diff --git ZPay/ZPayClient.hpp ZPay/ZPayClient.hpp\n> new file mode 100644\n> index 00000000..6ebc2a9a\n> --- /dev/null\n> +++ ZPay/ZPayClient.hpp\n> ```\n> \n> Hmm, it's treating it as a new file. Even though I have `diff.renames`\n> set to `copies`? Even though `diff --name-status` acknowledges the\n> relationship with the original file for the code on `master`? This is\n> confusing...\n\nThis is due to the way that rename detection works. When looking for\nrenames, the tree diff gives us a series of candidate \"sources\" that\nwere deleted and candidates \"dests\" that were added. And then we try to\nmatch them up.\n\nCopy detection differs in that it uses any file touched in the diff as a\nsource, not just deletions.\n\nAnd then \"--find-copies-harder\" uses even unmodified files as sources\n(but see below).\n\nSo here's a simplified setup:\n\n  git init repo\n  cd repo\n\n  seq 100 >a\n  git add a\n  git commit -m a\n\n  cp a b\n  seq 99 >a\n  git add a b\n  git commit -m b\n\nand then we can try these commands:\n\n  # this won't find a rename, because \"a\" was not deleted\n  git diff-tree --name-status -M HEAD^ HEAD\n\n  # this will find the copy, because now we consider \"a\" a source\n  git diff-tree --name-status -C HEAD^ HEAD\n\n  # this _won't_ find the copy, because we limited our tree diff to\n  # just look at \"b\"; hence we don't even consider \"a\" a source\n  git diff-tree --name-status -C HEAD^ HEAD -- b\n\nAnd that last one is the one that confused you. Naively it seems like\ndoing this would work (two \"-C\" are the same as \"--find-copies-harder\"):\n\n  git diff-tree --name-status -C -C HEAD^ HEAD -- b\n\nbut it doesn't. That's because we're still using the tree diff to find\nsources, and just adding unmodified entries to the source list. But our\npathspec prevents the diff from even considering \"a\". While this might\nseem useless at first, you can imagine something like:\n\n  git diff-tree -C -C HEAD^ HEAD -- subdir/\n\nwhich would consider all files in subdir as sources, but not those\noutside (which may be especially important for performance).\n\nBut there's no (clean) way to expand the set of paths that we consider\nas sources without also showing them in the output. There are two\nuseful variants I could imagine, though:\n\n  - a way to consider _all_ paths in the repository, not just those in\n    the pathspec, as sources, but show only the entries from the\n    pathspec. This could probably be a \"harder\" version of\n    \"--find-copies-harder\", something like \"-C -C -C <revs> -- b\".\n    Naturally this would be even more expensive in a big repo.\n\n  - a way to independently specify the source pathspec and the\n    output-limiting pathspec. This is a cheaper version of the one\n    above, where you could look at a subset of the tree a sources, but\n    limit the set of shown paths even further. It's not conceptually\n    that difficult, but syntactically it gets weird since you have two\n    lists of pathspecs on the command-line.\n\nI think the first one wouldn't be _too_ hard if somebody is interested\nin getting their feet wet with the diffcore-rename.c code. The second is\nprobably not worth the effort.\n\n> Out of curiosity, I thought I'd try this command:\n> \n> ```\n> git diff --follow master...topic -- ZPay/ZPayClient.hpp\n> ```\n\nSo yes, that does work, and is why I added the \"(clean)\" qualifier\nabove. It behaves like the \"-C -C -C\" I proposed. But the fact that it\ndoes so is entirely accidental. What happens is this:\n\n  - we're a little sloppy about what constitutes a traversal option and\n    what is a diff option. Many diff commands rely on setup_revisions(),\n    which parses both. So \"diff --follow\" probably _should_ be flagged\n    as an error, but isn't.\n\n  - the implementation of \"--follow\" works by doing a separate,\n    from-scratch tree-level diff on each commit (it _has_ to ignore your\n    pathspec, since by definition it allows only a single file to be in\n    the pathspec). And then rather than throwing away that result, it\n    feeds it to the rest of the diff pipeline, which then shows the\n    output you expected.\n\nSo it does do what you want, but only for the single-file case. And\ncertainly it was never intended to, and that might change in the future.\n\n> Now this looks more like it. I can actually see a useful diff here,\n> instead of everything looking like a new file. But there is a lot of\n> confusion here:\n> \n> 1. `diff --follow` is not a documented[1] option. Why does it work?\n\nAccident. :) See above.\n\n> 2. `diff -M` doesn't actually work either. It should, though. In fact,\n> I expected it to work as `--follow` does. But it doesn't.\n\nIt doesn't work because this is a copy, not a rename.\n\n> 3. The `diff.renames` config doesn't seem to be working here, when it should.\n\nIt does, but the pathspec prevents it from finding a source candidate.\n\n-Peff\n"},{"id":"382423","messageId":"CAHd499CJ9oZNNM68w27D0ifehJ1EksWBSbk7s45ZUZJAvM9z=g@mail.gmail.com","threadId":"51846","inReplyTo":"20190914033017.GA30458@sigill.intra.peff.net","subject":"Re: diff.renames not working?","fromName":"Robert Dailey","fromEmail":"rcdailey.lists@gmail.com","sentAt":"2019-09-16T14:25:28Z","receivedAt":"2019-09-16T14:25:43Z","isPatch":false,"sender":{"key":"rcdailey.lists@gmail.com","avatar":null},"body":"On Fri, Sep 13, 2019 at 10:30 PM Jeff King <peff@peff.net> wrote:\n> SNIP\n>\n> > Now this looks more like it. I can actually see a useful diff here,\n> > instead of everything looking like a new file. But there is a lot of\n> > confusion here:\n> >\n> > 1. `diff --follow` is not a documented[1] option. Why does it work?\n>\n> Accident. :) See above.\n>\n> > 2. `diff -M` doesn't actually work either. It should, though. In fact,\n> > I expected it to work as `--follow` does. But it doesn't.\n>\n> It doesn't work because this is a copy, not a rename.\n>\n> > 3. The `diff.renames` config doesn't seem to be working here, when it should.\n>\n> It does, but the pathspec prevents it from finding a source candidate.\n\nJeff, thanks so much. All of your examples help to contrast the\ndifferent behavior. I thought -M did copies & renames, that was my\nmisunderstanding. But the fact that you explained that the file spec\nwould prevent it from working as I'd like anyway means it doesn't\nmatter too much.\n\nUltimately my goal is to use the pathspec to filter what is visible, I\ndon't necessarily intend it to obscure my results due to internal\nbehavior. I realize there are performance obligations too (as you\ndescribed) but I feel like something like this should be\nstraightforward for end users. I consider myself a step above most\npeople with my understanding of Git and how it functions, and even I\nwas confused by this. Especially when it comes to folks I work with at\nmy day job, I just don't see them making sense of this.\n\nTo me, as transparently as possible, moves and copies should be\nhandled intrinsically. I realize there are technical challenges\n(performance, ambiguities, etc), but anything that can be done to help\nwith that would go a long way with most users. Maybe a 3rd option from\nyour list to solve this issue might be adding some metadata to commits\nor blob object types to record the pathspec of the file it was a copy\nor move of. Even if this required you to first do a git cp/mv before\nmodifying the content, or a strict similarity threshold, I think it\nwould go a long way. That means that `diff` can utilize the metadata\nin the object itself to find the path specs to follow, instead of\nrequiring some complex deduction logic that has a steep performance\ncost.\n"},{"id":"382429","messageId":"xmqqlfuob0ur.fsf@gitster-ct.c.googlers.com","threadId":"51846","inReplyTo":"20190914033017.GA30458@sigill.intra.peff.net","subject":"Re: diff.renames not working?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-16T17:35:40Z","receivedAt":"2019-09-16T17:35:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>   - a way to independently specify the source pathspec and the\n>     output-limiting pathspec. This is a cheaper version of the one\n>     above, where you could look at a subset of the tree a sources, but\n>     limit the set of shown paths even further. It's not conceptually\n>     that difficult, but syntactically it gets weird since you have two\n>     lists of pathspecs on the command-line.\n\nYes, the pathspec has always been input limiter and I think we have\ndiscussed adding separate output limiter from time to time, but\nnothing has happened.\n\nBack in the scripted porcelain days, it would conceptually have been\nsimpler and cleaner, as you probably would run \"diff-tree --raw\"\nwith no (or \"input\") pathspec, filter its output with \"output\"\npathspec, and then convert the raw diff to a patch ( we used to have\nsuch a filter, before we gave the -p option to all three \"diff\"\nfamily backends).\n\nIntroducing an option to say \"Pretend as if the system supported\noutput pathspec, and use the pathspec as such, without any input\npathspec\" feels like a dirty hack compared to conceptual purity of\nhaving two separate sets, and having to run without any input\npathspec may not be usable in truly large project, but I suspect it\nmay be good enough for most people (i.e. your \"first one\" that\nwouldn't be too hard).  I am not sure if I like it, though X-<.\n\n>> ```\n>> git diff --follow master...topic -- ZPay/ZPayClient.hpp\n>> ```\n>\n> So yes, that does work, and is why I added the \"(clean)\" qualifier\n> above. It behaves like the \"-C -C -C\" I proposed. But the fact that it\n> does so is entirely accidental. What happens is this:\n\nI am kinda surprised that \"diff\" gets affected by \"--follow\" (I knew\nthat the command line parser would take that option meant for \"log\"\nfamily without complaining) at all.\n"}]}