Re: [PATCH] diffcore-pickaxe doc: document -S and -G properly
Ramkumar Ramachandra artag...@gmail.com writes: -Sstring:: - Look for differences that introduce or remove an instance of - string. Note that this is different than the string simply - appearing in diff output; see the 'pickaxe' entry in - linkgit:gitdiffcore[7] for more details. + Look for commits where the specified string was added or + removed. More precisely, find commits that change the number + of occurrences of the specified string. Any time you say This means that, More precisely, etc. please check if you can rewrite it to lose everything before them (i.e. a vague sentence that needs to be clarified may not have to be there at all). ++ +It is often useful when you're looking for an exact string (like a +function prototype), and want to know the history of that string since +it first came into being. I think you should remind that the most useful case (and indeed the intended one) is for an exact string to be a multi-line block of text. People often get a (wrong) impression from the word string that it is meant to be used with a single-liner. -- 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: [PATCH] diffcore-pickaxe doc: document -S and -G properly
On Tue, May 14, 2013 at 10:12 AM, Ramkumar Ramachandra artag...@gmail.com wrote: The documentation of -S and -G is very sketchy. Completely rewrite the sections in Documentation/diff-options.txt and Documentation/gitdiffcore.txt. References: 52e9578 ([PATCH] Introducing software archaeologist's tool pickaxe.) f506b8e (git log/diff: add -Gregexp that greps in the patch text) Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- I spent some time reading the code and history to figure out what -S and -G really do. I hope I've done justice. Documentation/diff-options.txt | 35 +++--- Documentation/gitdiffcore.txt | 43 +++--- 2 files changed, 52 insertions(+), 26 deletions(-) diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt index 104579d..765abc5 100644 --- a/Documentation/diff-options.txt +++ b/Documentation/diff-options.txt @@ -383,14 +383,35 @@ ifndef::git-format-patch[] that matches other criteria, nothing is selected. -Sstring:: - Look for differences that introduce or remove an instance of - string. Note that this is different than the string simply - appearing in diff output; see the 'pickaxe' entry in - linkgit:gitdiffcore[7] for more details. + Look for commits where the specified string was added or + removed. More precisely, find commits that change the number + of occurrences of the specified string. ++ +It is often useful when you're looking for an exact string (like a +function prototype), and want to know the history of that string since +it first came into being. -Gregex:: It looks like you have deleted the -S and -G references here. Am I reading this right? - Look for differences whose added or removed line matches - the given regex. + Grep through the patch text of commits for added/removed lines + that match regex. `--pickaxe-regex` is implied in this + mode. ++ +To illustrate the difference between `-Sregex --pickaxe-regex` and +`-Gregex`, consider a commit with the following diff in the same +file: ++ + ++return !regexec(regexp, two-ptr, 1, regmatch, 0); +... +-hit = !regexec(regexp, mf2.ptr, 1, regmatch, 0); + ++ +While `git log -Gregexec\(regexp` will show this commit, `git log +-Sregexec\(regexp --pickaxe-regex` will not (because the number of +occurrences of that string didn't change). References to git-log seem out of place to me here in git-diffcore. I know it's only an example, but it seems that Git normally describes these 'reference selectors' more generically. The generic description may be more confusing to new users, but this patch is not the place to consider whether it should change. ++ +See the 'pickaxe' entry in linkgit:gitdiffcore[7] for more +information. --pickaxe-all:: When `-S` or `-G` finds a change, show all the changes in that @@ -399,7 +420,7 @@ ifndef::git-format-patch[] --pickaxe-regex:: Make the string not a plain string but an extended POSIX - regex to match. + regex to match. Implied when using `-G`. endif::git-format-patch[] -Oorderfile:: diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt index 568d757..39b9c51 100644 --- a/Documentation/gitdiffcore.txt +++ b/Documentation/gitdiffcore.txt @@ -222,25 +222,30 @@ version prefixed with '+'. diffcore-pickaxe: For Detecting Addition/Deletion of Specified String - -This transformation is used to find filepairs that represent -changes that touch a specified string, and is controlled by the --S option and the `--pickaxe-all` option to the 'git diff-*' -commands. - -When diffcore-pickaxe is in use, it checks if there are -filepairs whose result side and whose origin side have -different number of specified string. Such a filepair represents -the string appeared in this changeset. It also checks for the -opposite case that loses the specified string. - -When `--pickaxe-all` is not in effect, diffcore-pickaxe leaves -only such filepairs that touch the specified string in its -output. When `--pickaxe-all` is used, diffcore-pickaxe leaves all -filepairs intact if there is such a filepair, or makes the -output empty otherwise. The latter behaviour is designed to -make reviewing of the changes in the context of the whole -changeset easier. - +There are two kinds of pickaxe: the S kind (corresponding to 'git log +-S') and the G kind (corresponding to 'git log -G'). While the switches are called -S and -G, I do not think it is helpful to name the two pickaxe options as the S kind and the G kind. + +The S kind detects filepairs whose result side and origin side +have different number of occurrences of specified string. While +rename detection works as usual, 'git log -S' cannot omit
Re: [PATCH] diffcore-pickaxe doc: document -S and -G properly
On Tue, May 14, 2013 at 1:44 PM, Phil Hord phil.h...@gmail.com wrote: On Tue, May 14, 2013 at 10:12 AM, Ramkumar Ramachandra artag...@gmail.com wrote: -Sstring:: - Look for differences that introduce or remove an instance of - string. Note that this is different than the string simply - appearing in diff output; see the 'pickaxe' entry in - linkgit:gitdiffcore[7] for more details. + Look for commits where the specified string was added or + removed. More precisely, find commits that change the number + of occurrences of the specified string. ++ +It is often useful when you're looking for an exact string (like a +function prototype), and want to know the history of that string since +it first came into being. -Gregex:: It looks like you have deleted the -S and -G references here. Am I reading this right? Doy! Yes, I was reading it wrong. Sorry for the noise there. Phil -- 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: [PATCH] diffcore-pickaxe doc: document -S and -G properly
Junio C Hamano wrote: Any time you say This means that, More precisely, etc. please check if you can rewrite it to lose everything before them (i.e. a vague sentence that needs to be clarified may not have to be there at all). Right. I thought both are necessary in this case: the first sentence gives easy information to a first-timer. For someone who has played with it a bit, and wants to know more: the second line. ++ +It is often useful when you're looking for an exact string (like a +function prototype), and want to know the history of that string since +it first came into being. I think you should remind that the most useful case (and indeed the intended one) is for an exact string to be a multi-line block of text. People often get a (wrong) impression from the word string that it is meant to be used with a single-liner. Yes, I've been meaning to discuss that. I've been having some trouble with multi-line strings: zsh doesn't insert a TAB in the next line. The workaround I have is to write a shell script and execute that. How do you do it? -- 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: [PATCH] diffcore-pickaxe doc: document -S and -G properly
Phil Hord wrote: References to git-log seem out of place to me here in git-diffcore. I know it's only an example, but it seems that Git normally describes these 'reference selectors' more generically. The generic description may be more confusing to new users, but this patch is not the place to consider whether it should change. It's not for new users at all. The most useful application of -S and -G is in log. The translation from a log -G to a diffcore -G is not obvious at all, and warrants an explanation. Oh, and for the user. No user is going to type out `man gitdiffcore` out of the blue: she's most probably led there from log, and we're connecting the dots for her. While the switches are called -S and -G, I do not think it is helpful to name the two pickaxe options as the S kind and the G kind. How do you describe something precisely without loss of meaning? You stop abstracting unnecessarily. Read the sources: you will literally see DIFF_PICKAXE_KIND_G there. + +The S kind detects filepairs whose result side and origin side +have different number of occurrences of specified string. While +rename detection works as usual, 'git log -S' cannot omit commits The cannot omit feels like a confusing double-negative. How about includes instead? Intended. Omission is expected. Is it worth mentioning that something in the documentation is worth mentioning? You don't have to nitpick style. We can allow this much creative freedom in documentation. +in effect, diffcore-pickaxe leaves only such filepairs that touch the +specified string in its output. When in effect, diffcore-pickaxe +leaves all filepairs intact if there is such a filepair, or makes the +output empty otherwise. The latter behavior is designed to make +reviewing of the changes in the context of the whole changeset easier. I find this description (from the original code, not from this commit) somewhat confusing. It is written from the implementer's POV. I explained the entire -S and -G thing in terms of filepairs (and yes, that's implementation detail). Why would I want to explain this in any other terms? Does this seem clearer to you? [...] From diff-options.txt (the more end-user side): When -S or -G finds a change, show all the changes in that changeset, not just the files that contain the change in string. Not clear enough? -- 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: [PATCH] diffcore-pickaxe doc: document -S and -G properly
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: Any time you say This means that, More precisely, etc. please check if you can rewrite it to lose everything before them (i.e. a vague sentence that needs to be clarified may not have to be there at all). Right. I thought both are necessary in this case: the first sentence gives easy information to a first-timer. For someone who has played with it a bit, and wants to know more: the second line. ++ +It is often useful when you're looking for an exact string (like a +function prototype), and want to know the history of that string since +it first came into being. I think you should remind that the most useful case (and indeed the intended one) is for an exact string to be a multi-line block of text. People often get a (wrong) impression from the word string that it is meant to be used with a single-liner. Yes, I've been meaning to discuss that. I've been having some trouble with multi-line strings: zsh doesn't insert a TAB in the next line. The workaround I have is to write a shell script and execute that. How do you do it? I do not use zsh but with bash+readline the old tradition lnext can be used (see stty -a output and it typically is set to ^V), i.e. \C-v followed by \C-i should give you a literal HT. -- 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: [PATCH] diffcore-pickaxe doc: document -S and -G properly
Phil Hord phil.h...@gmail.com writes: Normally the pickaxe options limit the diff output to those files which contained the changes being searched; that is, those files which had modifications including the search string. With the --pickaxe-all option, the diff of the entire commit will be shown including files which did not have modifications including the search string. This is designed to make it easier to review the changes in the context of the whole commit. I find this very readable, even though diff output might be somewhat misleading (it is not output for the end user, but is passing to the next stage in the pipeline). -- 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: [PATCH] diffcore-pickaxe doc: document -S and -G properly
Junio C Hamano wrote: I do not use zsh but with bash+readline the old tradition lnext can be used (see stty -a output and it typically is set to ^V), i.e. \C-v followed by \C-i should give you a literal HT. Just looked it up: zsh has quoted-insert (^V) after which I can TAB. Still doesn't solve my problem though: I don't type out structs; I paste them the X clipboard (Emacs). And that doesn't work either on bash or zsh. What can we do to improve the interface? -- 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: [PATCH] diffcore-pickaxe doc: document -S and -G properly
Ramkumar Ramachandra wrote: What can we do to improve the interface? Write a better shell? Teach git gui blame to blame on arbitrary regions instead of single lines? I'm not sure what you're asking, mostly because I'm not sure who we is. -- 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: [PATCH] diffcore-pickaxe doc: document -S and -G properly
Ramkumar Ramachandra artag...@gmail.com writes: +The S kind detects filepairs whose result side and origin side +have different number of occurrences of specified string. While +rename detection works as usual, 'git log -S' cannot omit commits The cannot omit feels like a confusing double-negative. How about includes instead? Intended. Omission is expected. I think what makes this paragraph unnecessarily hard to read is the While rename works. With that, you are implying if you rename a file as a whole without changing the block of text you identify with the -S parameter, then such a change is not interesting as far as pickaxe is concerned. while that statement is logically correct, normal people are not that generous to read that much between your lines. I think that is one of the reasons why If you moved a string from file A to file B, log -S will flag that change as worth inspecting does not seem to logically follow and made Phil find your description confusing. Finding such a change indeed is a feature [*1*]; we need to flag such a change as worth inspecting to find where the code came from in order to dig deeper, so at least this cannot omit should be does not omit. [Footnote] *1* I suspect that your confusion may stem from not understanding what pickaxe was invented for. It is _not_ about finding the final answer, but is about stopping at a commit that is worth investigating further. It may help to read http://article.gmane.org/gmane.comp.version-control.git/217 and then its follow-up http://gitster.livejournal.com/35628.html -- 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: [PATCH] diffcore-pickaxe doc: document -S and -G properly
On Tue, May 14, 2013 at 2:44 PM, Ramkumar Ramachandra artag...@gmail.com wrote: Phil Hord wrote: References to git-log seem out of place to me here in git-diffcore. I know it's only an example, but it seems that Git normally describes these 'reference selectors' more generically. The generic description may be more confusing to new users, but this patch is not the place to consider whether it should change. It's not for new users at all. The most useful application of -S and -G is in log. The translation from a log -G to a diffcore -G is not obvious at all, and warrants an explanation. Oh, and for the user. No user is going to type out `man gitdiffcore` out of the blue: she's most probably led there from log, and we're connecting the dots for her. She may have been led here by some other help topic, too. Maybe the git log examples belong in the git-log documentation. While the switches are called -S and -G, I do not think it is helpful to name the two pickaxe options as the S kind and the G kind. How do you describe something precisely without loss of meaning? You stop abstracting unnecessarily. Read the sources: you will literally see DIFF_PICKAXE_KIND_G there. S and G are abstractions. DIFF_PICKAXE_KIND_G is an implementer's distinction. I agree this is a tricky subject. When writing technical documentation you must be precise and clear. It is easy to forget both. It is common to forget to be either clear or precise. It is very difficult to be both clear and precise. So, my suggestion is to use some meaningful names for the action of -S and -G. Relating these names to -S and -G in a clear way is likely to be difficult. + +The S kind detects filepairs whose result side and origin side +have different number of occurrences of specified string. While +rename detection works as usual, 'git log -S' cannot omit commits The cannot omit feels like a confusing double-negative. How about includes instead? Intended. Omission is expected. This is precision at the expense of clarity. Omission is not expected for the user who wants to ask this question: Where is that commit that added 'foo'? To Git she wants to ask Show me the commit that added or removed 'foo'. You and I both know that Git does this in reverse. Git translates this question into Show all the commits, but hide the ones which do not add or remove 'foo'. And so we say Git 'cannot omit' commits Is it worth mentioning that something in the documentation is worth mentioning? You don't have to nitpick style. We can allow this much creative freedom in documentation. I think I do. The concepts in here are complicated enough without loading the language with excess verbiage. I am chronically afflicted with this disease where I am always adding extra decorations to my sentences. I work hard to combat that, especially when dealing with technical writings. This makes it easy for me to recognize in other technical writing. I didn't look closely, but these unnecessary introductory clause appeared to be the only change in this paragraph. I do not think it adds anything, which is why I mentioned it. I could have been more clear and less flippant about it, though. I'm sorry if my manner was offensive. This is another chronic problem I seem to have. +in effect, diffcore-pickaxe leaves only such filepairs that touch the +specified string in its output. When in effect, diffcore-pickaxe +leaves all filepairs intact if there is such a filepair, or makes the +output empty otherwise. The latter behavior is designed to make +reviewing of the changes in the context of the whole changeset easier. I find this description (from the original code, not from this commit) somewhat confusing. It is written from the implementer's POV. I explained the entire -S and -G thing in terms of filepairs (and yes, that's implementation detail). Why would I want to explain this in any other terms? Clarity. What is a 'filepair' when I am getting a short-log? Internally there was a diff, and the diff involves pairs of files. But it's a tedious detail to the user which might send her off needlessly trying to understand the meaning of the term. But you are right; gitdiffcore.txt is about precision and implementation. The goal is to address the technical user who is trying to understand these operations in general, not just for log. 'filepairs' is a useful concept to lean on. The clearer description probably belongs in git-log. Does this seem clearer to you? [...] From diff-options.txt (the more end-user side): When -S or -G finds a change, show all the changes in that changeset, not just the files that contain the change in string. Not clear enough? Yes, it was clear enough to me in git-log. Perhaps it is not worth mentioning here. Phil -- 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
Re: [PATCH] diffcore-pickaxe doc: document -S and -G properly
Jonathan Nieder wrote: Write a better shell? Shell, editor. Both are very hard problems, and the successful projects last many years (emacs, zsh are over 20 years old). Teach git gui blame to blame on arbitrary regions instead of single lines? Or in my case: get magit to do log -S. Should we mention in the -S documentation that temporary shell script is the way to get multi-line input? -- 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: [PATCH] diffcore-pickaxe doc: document -S and -G properly
Ramkumar Ramachandra wrote: Should we mention in the -S documentation that temporary shell script is the way to get multi-line input? No, because for almost everyone it isn't. An example in the EXAMPLES section including an aside that you might have to hit ^V to enter a tab could be useful, though. -- 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: [PATCH] diffcore-pickaxe doc: document -S and -G properly
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: I do not use zsh but with bash+readline the old tradition lnext can be used (see stty -a output and it typically is set to ^V), i.e. \C-v followed by \C-i should give you a literal HT. Just looked it up: zsh has quoted-insert (^V) after which I can TAB. Still doesn't solve my problem though: I don't type out structs; I paste them the X clipboard (Emacs). And that doesn't work either on bash or zsh. What can we do to improve the interface? Heh, I seem to have just found a volunteer to finish the Linus's dream by following up on http://gitster.livejournal.com/35628.html (see section #5; I do not do GUI, neither Linus, so this has been a four-year old dream). -- 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: [PATCH] diffcore-pickaxe doc: document -S and -G properly
Junio C Hamano wrote: I think what makes this paragraph unnecessarily hard to read is the While rename works. With that, you are implying if you rename a file as a whole without changing the block of text you identify with the -S parameter, then such a change is not interesting as far as pickaxe is concerned. while that statement is logically correct, normal people are not that generous to read that much between your lines. Yes, I'm exactly implying that. But I don't want to lose meaning: in the previous sentence, I talk about filepairs. I want to point out that rename detection working at the filepair level is a perfectly normal thing. I think that is one of the reasons why If you moved a string from file A to file B, log -S will flag that change as worth inspecting does not seem to logically follow and made Phil find your description confusing. Sure, we can elaborate a bit more. Finding such a change indeed is a feature [*1*]; we need to flag such a change as worth inspecting to find where the code came from in order to dig deeper, so at least this cannot omit should be does not omit. What I was trying to say is that it's an accidental feature: the reason this feature exists is because diffcore is tied to filepairs (and rename detection works at the filepair level). You may argue that there's nothing wrong with this design, but consider what happens if you rebase on top of a big code move: it's completely broken. If git were a true content tracker, and file boundaries really did not matter, isn't this feature actually a deficiency? Ofcourse, the user doesn't need to know all this, and does not omit is a good suggestion. -- 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: [PATCH] diffcore-pickaxe doc: document -S and -G properly
Ramkumar Ramachandra artag...@gmail.com writes: What I was trying to say is that it's an accidental feature There is nothing accidental about it. It was a very conscious design decision. When a commit moves a file wholesale without affecting the block of code you are interested in, you know that whole block came from the file in the old tree at pre-rename location without looking at anywhere else. That is why renamed but pickaxe-uninteresting filepairs are dropped. When a commit moves (some lines of) the block of code you are interested in from one file to another, it may have been a single instance moving to another place, but it may well have been multiple copies consolidated into one (the new copy, pickaxe digging from future to past may see disappearing). That is a significant event worth digging into further by first stopping there and then inspecting the whole change with --pickaxe-all to see what changes that are similar to the change in question exist in the other parts (notice the multiple) of the tree. If you do not understand it, then you really should re-read $gmane/217 and then its explanation I wrote 4 years ago (both of which I already gave you URLs to in this thread). -- 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: [PATCH] diffcore-pickaxe doc: document -S and -G properly
Junio C Hamano wrote: When a commit moves a file wholesale without affecting the block of code you are interested in, you know that whole block came from the file in the old tree at pre-rename location without looking at anywhere else. That is why renamed but pickaxe-uninteresting filepairs are dropped. When a commit moves (some lines of) the block of code you are interested in from one file to another, it may have been a single instance moving to another place, but it may well have been multiple copies consolidated into one (the new copy, pickaxe digging from future to past may see disappearing). That is a significant event worth digging into further by first stopping there and then inspecting the whole change with --pickaxe-all to see what changes that are similar to the change in question exist in the other parts (notice the multiple) of the tree. Makes sense. I wasn't looking at it from a macro perspective. Yeah, I constantly re-read 217 and your follow-up. -- 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: [PATCH] diffcore-pickaxe doc: document -S and -G properly
On Tue, May 14, 2013 at 2:57 PM, Ramkumar Ramachandra artag...@gmail.com wrote: Junio C Hamano wrote: I do not use zsh but with bash+readline the old tradition lnext can be used (see stty -a output and it typically is set to ^V), i.e. \C-v followed by \C-i should give you a literal HT. Just looked it up: zsh has quoted-insert (^V) after which I can TAB. Still doesn't solve my problem though: I don't type out structs; I paste them the X clipboard (Emacs). And that doesn't work either on bash or zsh. What can we do to improve the interface? Don't use tabs in your code? P -- 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
