Re: Kidney blame's behaviour and edge cases
Julian Foad wrote on Fri, Feb 20, 2015 at 20:15:29 +: > I (Julian Foad) wrote: > > issue #4565 "reverse blame, aka kidney blame" > > [...] I want to see: > > > > * The first revision in which the line was changed (or deleted) after > > r140. > > The following help text explains how I think it should behave: > > [[[ > blame (praise, annotate, ann): Show when each line of a file was last (or > next) changed. > usage: blame [-rM:N] TARGET[@REV]... > > Annotate each line of a file with the revision number and author of the > last change (or optionally the next change) to that line. > > With no revision range (same as -r0:REV), or with '-r M:N' where M < N, > annotate each line that is present in revision N of the file, > with the last revision at or before rN that changed or added the line, > looking back no further than rM. > > With a reverse revision range '-r M:N' where M > N, > annotate each line that is present in revision N of the file, > with the NEXT revision AFTER rN that changed or DELETED the line, > looking forward no further than rM. > > Write the annotated result to standard output. > > If specified, REV determines in which revision the target is first > looked up. > ]]] > > Makes sense? As discussed on IRC: yes, makes sense. Thanks to you and to Bert for fixing this!
Re: Kidney blame's behaviour and edge cases
Bert fixed the code to work like this, in r1661208. I committed help text, much like I pasted here, in r1661211. - Julian I (Julian Foad) wrote: > I (Julian Foad) wrote: >> issue #4565 "reverse blame, aka kidney blame" >> [...] I want to see: >> >> * The first revision in which the line was changed (or deleted) after >> r140. > > The following help text explains how I think it should behave: > > [[[ > blame (praise, annotate, ann): Show when each line of a file was last (or > next) changed. > usage: blame [-rM:N] TARGET[@REV]... > > Annotate each line of a file with the revision number and author of the > last change (or optionally the next change) to that line. > > With no revision range (same as -r0:REV), or with '-r M:N' where M > < N, > annotate each line that is present in revision N of the file, > with the last revision at or before rN that changed or added the line, > looking back no further than rM. > > With a reverse revision range '-r M:N' where M > N, > annotate each line that is present in revision N of the file, > with the NEXT revision AFTER rN that changed or DELETED the line, > looking forward no further than rM. > > Write the annotated result to standard output. > > If specified, REV determines in which revision the target is first > looked up. > ]]] > > Makes sense? > > - Julian >
Re: Kidney blame's behaviour and edge cases
I (Julian Foad) wrote: > issue #4565 "reverse blame, aka kidney blame" > [...] I want to see: > > * The first revision in which the line was changed (or deleted) after > r140. The following help text explains how I think it should behave: [[[ blame (praise, annotate, ann): Show when each line of a file was last (or next) changed. usage: blame [-rM:N] TARGET[@REV]... Annotate each line of a file with the revision number and author of the last change (or optionally the next change) to that line. With no revision range (same as -r0:REV), or with '-r M:N' where M < N, annotate each line that is present in revision N of the file, with the last revision at or before rN that changed or added the line, looking back no further than rM. With a reverse revision range '-r M:N' where M > N, annotate each line that is present in revision N of the file, with the NEXT revision AFTER rN that changed or DELETED the line, looking forward no further than rM. Write the annotated result to standard output. If specified, REV determines in which revision the target is first looked up. ]]] Makes sense? - Julian
Re: Kidney blame's behaviour and edge cases
I filed issue #4565 "reverse blame, aka kidney blame" to track this enhancement, because I think it is useful to have an issue to coordinate any change we make in a release. It currently doesn't behave how I think it should. Try svn blame -r160:140 ^/subversion/trunk/subversion/svn/svn.c@166 Notice that nearly every line of the output is annotated as r1591022. This file was indeed changed in r1591022, but only three lines were changed: one added, two deleted. There may be an end-of-revision-range problem here, where all lines otherwise untouched are given a revision number but should instead be marked as "-". But if that's happening, it's still not the main problem here. The main problem seems to be that it's annotating each line of the old file (svn.c@140) with: * The last (youngest) revision in which the file was changed *before* the first revision in which this line was changed (or deleted) after r140. To me, that's not a good output. I want to see: * The first revision in which the line was changed (or deleted) after r140. To me, that's what would correspond to the logical 'reverse' of the normal blame. I note that the original email in this thread [1] did not include a description of the intended output in behavioural terms, and I think the description it included in terms of 'iota and kappa' was faulty. Anyone else tried it? What do you think? - Julian [1] 2013-06-13, archived at e.g. http://mail-archives.apache.org/mod_mbox/subversion-dev/201306.mbox/%3C20130613083546.GA2773@tarsus.local2%3E and http://svn.haxx.se/dev/archive-2013-06/0230.shtml
RE: Kidney blame's behaviour and edge cases
Paul burba added and shortly after reverted (as no longer used) code to separate these steps. You can probably revive that code. Bert From: Daniel Shahaf Sent: 14/06/2013 17:49 To: Bert Huijben Cc: Johan Corveleyn; Subversion Development Subject: Re: Kidney blame's behaviour and edge cases Daniel Shahaf wrote on Fri, Jun 14, 2013 at 17:41:18 +0200: > No typo, but that function doesn't help, since we don't have a revision > in which the file existed; the use-case is to do 'svn blame -r HEAD:0 > file' and have the 0 become the first revision in which the file > existed. > > It seem the fix is to use svn_client__repos_location_segments(), like > 'log' does. > > I am still not happy with the svn_client_blame5() patch I committed --- > specifically, with the way it opens at 'end' and then tries to move to > 'start' by re-doing the last part of svn_client__ra_session_from_path2(). > I think that part needs to be redone. I'm not sure how yet. > What I'd conceptually like to do is: 1 - Open RA session to repository root 2 - Resolve the two svn_opt_revision_t arguments to numbers; call the larger X 3 - Reparent to 'target@peg_revision -r X' 4 - Run blame But the current code doesn't separate (1) and (2). Daniel > Bert Huijben wrote on Fri, Jun 14, 2013 at 08:24:58 -0700: > > Svn_ra_get_deleted_rev() ? > > (could have a typo) > > > > Bert From: Daniel Shahaf > > Sent: 14/06/2013 17:11 > > To: Bert Huijben > > Cc: Johan Corveleyn; Subversion Development > > Subject: Re: Kidney blame's behaviour and edge cases > > Bert Huijben wrote on Fri, Jun 14, 2013 at 08:06:25 -0700: > > > I would guess 1 and twi are actually the same problem: no node found > > > via peg revision. > > > > > > Bert From: Johan Corveleyn > > > Sent: 14/06/2013 16:51 > > > To: Daniel Shahaf > > > Cc: Subversion Development > > > Subject: Re: Kidney blame's behaviour and edge cases > > > On Fri, Jun 14, 2013 at 1:33 PM, Daniel Shahaf wrote: > > > > Johan Corveleyn wrote on Fri, Jun 14, 2013 at 11:16:06 +0200: > > > >> On Fri, Jun 14, 2013 at 11:00 AM, Daniel Shahaf > > > >> wrote: > > > >> > Doug Robinson wrote on Thu, Jun 13, 2013 at 12:10:49 -0400: > > > >> >> Daniel: > > > >> >> > > > >> >> I think that simply enabling M > > >> >> create the > > > >> >> situation where the user makes a mistake, gets something they don't > > > >> >> expect > > > >> >> and tries to interpret it based on their desire - leading to > > > >> >> confusion. I > > > >> >> believe M > > >> >> should be > > > >> >> required to make it clear that the user wants the reverse blame > > > >> >> walk. > > > >> > > > > >> > Sorry, disagree. > > > >> > > > > >> > diff -r 1:5 != diff -r 5:1 > > > >> > log -r 1:5 != log -r 5:1 > > > >> > merge -r 4:5 != merge -r 5:4 > > > >> > > > > >> > With all that in mind, I still think that making 'blame -r 5:4' and > > > >> > 'blame -r 4:5' do different things is the correct course of action. > > > >> > > > > >> > > > >> Okay, I don't feel strongly about this. My only "argument" was that > > > >> people are not used to thinking about the order of rev args when using > > > >> blame. But that doesn't mean they can't get used to it ... > > > > > > > > Implemented in r1493027. No API changes are involved --- this simply > > > > makes 'blame -r 5:4' do something instead of raising an error > > > > immediately --- so I wonder if we should backport it. > > > > > > > > I'll go ahead and put it in STATUS towards 1.8.1, if people prefer a > > > > backport not to happen they can go ahead and cast -0 votes and continue > > > > discussion here. > > > > > > There are still two problems with the implementation you committed in > > > r1493027: > > > > > > With 'svn blame M:N' where M>N > > > > > > 1) It's using the N as peg revision, while it should use M (but you're > > > already working on that). > > > > > > > Should be fixed by r1493106. I'd welcome further review of that, I > > am unsure that the "open ra session to the other svn_opt_revision_t" > > part is idiomatic. > > > > > 2) If N is before the item existed, I get: > > > svn: E195012: Unable to find repository location for > > > 'svn://localhost/path/to/file.txt' in revision 1 > > > > > > It would be nice if you could just blame up to the oldest revision > > > close to 'end' where the item still existed. > > > > Yeah, it would be nice if 'svn blame -r HEAD:1' just worked even for > > files added later. I can look into that, but not today :) It would > > be helpful if someone could point me to another place in the codebase > > that solves the same problem. > > > > Cheers, > > > > Daniel
RE: Kidney blame's behaviour and edge cases
That function and the get revs function can only go from new to old. That deleted rev is the only one for the other way From: Daniel Shahaf Sent: 14/06/2013 17:41 To: Bert Huijben Cc: Johan Corveleyn; Subversion Development Subject: Re: Kidney blame's behaviour and edge cases No typo, but that function doesn't help, since we don't have a revision in which the file existed; the use-case is to do 'svn blame -r HEAD:0 file' and have the 0 become the first revision in which the file existed. It seem the fix is to use svn_client__repos_location_segments(), like 'log' does. I am still not happy with the svn_client_blame5() patch I committed --- specifically, with the way it opens at 'end' and then tries to move to 'start' by re-doing the last part of svn_client__ra_session_from_path2(). I think that part needs to be redone. I'm not sure how yet. Bert Huijben wrote on Fri, Jun 14, 2013 at 08:24:58 -0700: > Svn_ra_get_deleted_rev() ? > (could have a typo) > > Bert From: Daniel Shahaf > Sent: 14/06/2013 17:11 > To: Bert Huijben > Cc: Johan Corveleyn; Subversion Development > Subject: Re: Kidney blame's behaviour and edge cases > Bert Huijben wrote on Fri, Jun 14, 2013 at 08:06:25 -0700: > > I would guess 1 and twi are actually the same problem: no node found > > via peg revision. > > > > Bert From: Johan Corveleyn > > Sent: 14/06/2013 16:51 > > To: Daniel Shahaf > > Cc: Subversion Development > > Subject: Re: Kidney blame's behaviour and edge cases > > On Fri, Jun 14, 2013 at 1:33 PM, Daniel Shahaf wrote: > > > Johan Corveleyn wrote on Fri, Jun 14, 2013 at 11:16:06 +0200: > > >> On Fri, Jun 14, 2013 at 11:00 AM, Daniel Shahaf > > >> wrote: > > >> > Doug Robinson wrote on Thu, Jun 13, 2013 at 12:10:49 -0400: > > >> >> Daniel: > > >> >> > > >> >> I think that simply enabling M > >> >> create the > > >> >> situation where the user makes a mistake, gets something they don't > > >> >> expect > > >> >> and tries to interpret it based on their desire - leading to > > >> >> confusion. I > > >> >> believe M > >> >> should be > > >> >> required to make it clear that the user wants the reverse blame walk. > > >> > > > >> > Sorry, disagree. > > >> > > > >> > diff -r 1:5 != diff -r 5:1 > > >> > log -r 1:5 != log -r 5:1 > > >> > merge -r 4:5 != merge -r 5:4 > > >> > > > >> > With all that in mind, I still think that making 'blame -r 5:4' and > > >> > 'blame -r 4:5' do different things is the correct course of action. > > >> > > > >> > > >> Okay, I don't feel strongly about this. My only "argument" was that > > >> people are not used to thinking about the order of rev args when using > > >> blame. But that doesn't mean they can't get used to it ... > > > > > > Implemented in r1493027. No API changes are involved --- this simply > > > makes 'blame -r 5:4' do something instead of raising an error > > > immediately --- so I wonder if we should backport it. > > > > > > I'll go ahead and put it in STATUS towards 1.8.1, if people prefer a > > > backport not to happen they can go ahead and cast -0 votes and continue > > > discussion here. > > > > There are still two problems with the implementation you committed in > > r1493027: > > > > With 'svn blame M:N' where M>N > > > > 1) It's using the N as peg revision, while it should use M (but you're > > already working on that). > > > > Should be fixed by r1493106. I'd welcome further review of that, I > am unsure that the "open ra session to the other svn_opt_revision_t" > part is idiomatic. > > > 2) If N is before the item existed, I get: > > svn: E195012: Unable to find repository location for > > 'svn://localhost/path/to/file.txt' in revision 1 > > > > It would be nice if you could just blame up to the oldest revision > > close to 'end' where the item still existed. > > Yeah, it would be nice if 'svn blame -r HEAD:1' just worked even for > files added later. I can look into that, but not today :) It would > be helpful if someone could point me to another place in the codebase > that solves the same problem. > > Cheers, > > Daniel
Re: Kidney blame's behaviour and edge cases
Daniel Shahaf wrote on Fri, Jun 14, 2013 at 17:41:18 +0200: > No typo, but that function doesn't help, since we don't have a revision > in which the file existed; the use-case is to do 'svn blame -r HEAD:0 > file' and have the 0 become the first revision in which the file > existed. > > It seem the fix is to use svn_client__repos_location_segments(), like > 'log' does. > > I am still not happy with the svn_client_blame5() patch I committed --- > specifically, with the way it opens at 'end' and then tries to move to > 'start' by re-doing the last part of svn_client__ra_session_from_path2(). > I think that part needs to be redone. I'm not sure how yet. > What I'd conceptually like to do is: 1 - Open RA session to repository root 2 - Resolve the two svn_opt_revision_t arguments to numbers; call the larger X 3 - Reparent to 'target@peg_revision -r X' 4 - Run blame But the current code doesn't separate (1) and (2). Daniel > Bert Huijben wrote on Fri, Jun 14, 2013 at 08:24:58 -0700: > > Svn_ra_get_deleted_rev() ? > > (could have a typo) > > > > Bert From: Daniel Shahaf > > Sent: 14/06/2013 17:11 > > To: Bert Huijben > > Cc: Johan Corveleyn; Subversion Development > > Subject: Re: Kidney blame's behaviour and edge cases > > Bert Huijben wrote on Fri, Jun 14, 2013 at 08:06:25 -0700: > > > I would guess 1 and twi are actually the same problem: no node found > > > via peg revision. > > > > > > Bert From: Johan Corveleyn > > > Sent: 14/06/2013 16:51 > > > To: Daniel Shahaf > > > Cc: Subversion Development > > > Subject: Re: Kidney blame's behaviour and edge cases > > > On Fri, Jun 14, 2013 at 1:33 PM, Daniel Shahaf wrote: > > > > Johan Corveleyn wrote on Fri, Jun 14, 2013 at 11:16:06 +0200: > > > >> On Fri, Jun 14, 2013 at 11:00 AM, Daniel Shahaf > > > >> wrote: > > > >> > Doug Robinson wrote on Thu, Jun 13, 2013 at 12:10:49 -0400: > > > >> >> Daniel: > > > >> >> > > > >> >> I think that simply enabling M > > >> >> create the > > > >> >> situation where the user makes a mistake, gets something they don't > > > >> >> expect > > > >> >> and tries to interpret it based on their desire - leading to > > > >> >> confusion. I > > > >> >> believe M > > >> >> should be > > > >> >> required to make it clear that the user wants the reverse blame > > > >> >> walk. > > > >> > > > > >> > Sorry, disagree. > > > >> > > > > >> > diff -r 1:5 != diff -r 5:1 > > > >> > log -r 1:5 != log -r 5:1 > > > >> > merge -r 4:5 != merge -r 5:4 > > > >> > > > > >> > With all that in mind, I still think that making 'blame -r 5:4' and > > > >> > 'blame -r 4:5' do different things is the correct course of action. > > > >> > > > > >> > > > >> Okay, I don't feel strongly about this. My only "argument" was that > > > >> people are not used to thinking about the order of rev args when using > > > >> blame. But that doesn't mean they can't get used to it ... > > > > > > > > Implemented in r1493027. No API changes are involved --- this simply > > > > makes 'blame -r 5:4' do something instead of raising an error > > > > immediately --- so I wonder if we should backport it. > > > > > > > > I'll go ahead and put it in STATUS towards 1.8.1, if people prefer a > > > > backport not to happen they can go ahead and cast -0 votes and continue > > > > discussion here. > > > > > > There are still two problems with the implementation you committed in > > > r1493027: > > > > > > With 'svn blame M:N' where M>N > > > > > > 1) It's using the N as peg revision, while it should use M (but you're > > > already working on that). > > > > > > > Should be fixed by r1493106. I'd welcome further review of that, I > > am unsure that the "open ra session to the other svn_opt_revision_t" > > part is idiomatic. > > > > > 2) If N is before the item existed, I get: > > > svn: E195012: Unable to find repository location for > > > 'svn://localhost/path/to/file.txt' in revision 1 > > > > > > It would be nice if you could just blame up to the oldest revision > > > close to 'end' where the item still existed. > > > > Yeah, it would be nice if 'svn blame -r HEAD:1' just worked even for > > files added later. I can look into that, but not today :) It would > > be helpful if someone could point me to another place in the codebase > > that solves the same problem. > > > > Cheers, > > > > Daniel
Re: Kidney blame's behaviour and edge cases
No typo, but that function doesn't help, since we don't have a revision in which the file existed; the use-case is to do 'svn blame -r HEAD:0 file' and have the 0 become the first revision in which the file existed. It seem the fix is to use svn_client__repos_location_segments(), like 'log' does. I am still not happy with the svn_client_blame5() patch I committed --- specifically, with the way it opens at 'end' and then tries to move to 'start' by re-doing the last part of svn_client__ra_session_from_path2(). I think that part needs to be redone. I'm not sure how yet. Bert Huijben wrote on Fri, Jun 14, 2013 at 08:24:58 -0700: > Svn_ra_get_deleted_rev() ? > (could have a typo) > > Bert From: Daniel Shahaf > Sent: 14/06/2013 17:11 > To: Bert Huijben > Cc: Johan Corveleyn; Subversion Development > Subject: Re: Kidney blame's behaviour and edge cases > Bert Huijben wrote on Fri, Jun 14, 2013 at 08:06:25 -0700: > > I would guess 1 and twi are actually the same problem: no node found > > via peg revision. > > > > Bert From: Johan Corveleyn > > Sent: 14/06/2013 16:51 > > To: Daniel Shahaf > > Cc: Subversion Development > > Subject: Re: Kidney blame's behaviour and edge cases > > On Fri, Jun 14, 2013 at 1:33 PM, Daniel Shahaf wrote: > > > Johan Corveleyn wrote on Fri, Jun 14, 2013 at 11:16:06 +0200: > > >> On Fri, Jun 14, 2013 at 11:00 AM, Daniel Shahaf > > >> wrote: > > >> > Doug Robinson wrote on Thu, Jun 13, 2013 at 12:10:49 -0400: > > >> >> Daniel: > > >> >> > > >> >> I think that simply enabling M > >> >> create the > > >> >> situation where the user makes a mistake, gets something they don't > > >> >> expect > > >> >> and tries to interpret it based on their desire - leading to > > >> >> confusion. I > > >> >> believe M > >> >> should be > > >> >> required to make it clear that the user wants the reverse blame walk. > > >> > > > >> > Sorry, disagree. > > >> > > > >> > diff -r 1:5 != diff -r 5:1 > > >> > log -r 1:5 != log -r 5:1 > > >> > merge -r 4:5 != merge -r 5:4 > > >> > > > >> > With all that in mind, I still think that making 'blame -r 5:4' and > > >> > 'blame -r 4:5' do different things is the correct course of action. > > >> > > > >> > > >> Okay, I don't feel strongly about this. My only "argument" was that > > >> people are not used to thinking about the order of rev args when using > > >> blame. But that doesn't mean they can't get used to it ... > > > > > > Implemented in r1493027. No API changes are involved --- this simply > > > makes 'blame -r 5:4' do something instead of raising an error > > > immediately --- so I wonder if we should backport it. > > > > > > I'll go ahead and put it in STATUS towards 1.8.1, if people prefer a > > > backport not to happen they can go ahead and cast -0 votes and continue > > > discussion here. > > > > There are still two problems with the implementation you committed in > > r1493027: > > > > With 'svn blame M:N' where M>N > > > > 1) It's using the N as peg revision, while it should use M (but you're > > already working on that). > > > > Should be fixed by r1493106. I'd welcome further review of that, I > am unsure that the "open ra session to the other svn_opt_revision_t" > part is idiomatic. > > > 2) If N is before the item existed, I get: > > svn: E195012: Unable to find repository location for > > 'svn://localhost/path/to/file.txt' in revision 1 > > > > It would be nice if you could just blame up to the oldest revision > > close to 'end' where the item still existed. > > Yeah, it would be nice if 'svn blame -r HEAD:1' just worked even for > files added later. I can look into that, but not today :) It would > be helpful if someone could point me to another place in the codebase > that solves the same problem. > > Cheers, > > Daniel
RE: Kidney blame's behaviour and edge cases
Svn_ra_get_deleted_rev() ? (could have a typo) Bert From: Daniel Shahaf Sent: 14/06/2013 17:11 To: Bert Huijben Cc: Johan Corveleyn; Subversion Development Subject: Re: Kidney blame's behaviour and edge cases Bert Huijben wrote on Fri, Jun 14, 2013 at 08:06:25 -0700: > I would guess 1 and twi are actually the same problem: no node found > via peg revision. > > Bert From: Johan Corveleyn > Sent: 14/06/2013 16:51 > To: Daniel Shahaf > Cc: Subversion Development > Subject: Re: Kidney blame's behaviour and edge cases > On Fri, Jun 14, 2013 at 1:33 PM, Daniel Shahaf wrote: > > Johan Corveleyn wrote on Fri, Jun 14, 2013 at 11:16:06 +0200: > >> On Fri, Jun 14, 2013 at 11:00 AM, Daniel Shahaf wrote: > >> > Doug Robinson wrote on Thu, Jun 13, 2013 at 12:10:49 -0400: > >> >> Daniel: > >> >> > >> >> I think that simply enabling M >> >> the > >> >> situation where the user makes a mistake, gets something they don't > >> >> expect > >> >> and tries to interpret it based on their desire - leading to confusion. > >> >> I > >> >> believe M >> >> should be > >> >> required to make it clear that the user wants the reverse blame walk. > >> > > >> > Sorry, disagree. > >> > > >> > diff -r 1:5 != diff -r 5:1 > >> > log -r 1:5 != log -r 5:1 > >> > merge -r 4:5 != merge -r 5:4 > >> > > >> > With all that in mind, I still think that making 'blame -r 5:4' and > >> > 'blame -r 4:5' do different things is the correct course of action. > >> > > >> > >> Okay, I don't feel strongly about this. My only "argument" was that > >> people are not used to thinking about the order of rev args when using > >> blame. But that doesn't mean they can't get used to it ... > > > > Implemented in r1493027. No API changes are involved --- this simply > > makes 'blame -r 5:4' do something instead of raising an error > > immediately --- so I wonder if we should backport it. > > > > I'll go ahead and put it in STATUS towards 1.8.1, if people prefer a > > backport not to happen they can go ahead and cast -0 votes and continue > > discussion here. > > There are still two problems with the implementation you committed in > r1493027: > > With 'svn blame M:N' where M>N > > 1) It's using the N as peg revision, while it should use M (but you're > already working on that). > Should be fixed by r1493106. I'd welcome further review of that, I am unsure that the "open ra session to the other svn_opt_revision_t" part is idiomatic. > 2) If N is before the item existed, I get: > svn: E195012: Unable to find repository location for > 'svn://localhost/path/to/file.txt' in revision 1 > > It would be nice if you could just blame up to the oldest revision > close to 'end' where the item still existed. Yeah, it would be nice if 'svn blame -r HEAD:1' just worked even for files added later. I can look into that, but not today :) It would be helpful if someone could point me to another place in the codebase that solves the same problem. Cheers, Daniel
Re: Kidney blame's behaviour and edge cases
Bert Huijben wrote on Fri, Jun 14, 2013 at 08:06:25 -0700: > I would guess 1 and twi are actually the same problem: no node found > via peg revision. > > Bert From: Johan Corveleyn > Sent: 14/06/2013 16:51 > To: Daniel Shahaf > Cc: Subversion Development > Subject: Re: Kidney blame's behaviour and edge cases > On Fri, Jun 14, 2013 at 1:33 PM, Daniel Shahaf wrote: > > Johan Corveleyn wrote on Fri, Jun 14, 2013 at 11:16:06 +0200: > >> On Fri, Jun 14, 2013 at 11:00 AM, Daniel Shahaf wrote: > >> > Doug Robinson wrote on Thu, Jun 13, 2013 at 12:10:49 -0400: > >> >> Daniel: > >> >> > >> >> I think that simply enabling M >> >> the > >> >> situation where the user makes a mistake, gets something they don't > >> >> expect > >> >> and tries to interpret it based on their desire - leading to confusion. > >> >> I > >> >> believe M >> >> should be > >> >> required to make it clear that the user wants the reverse blame walk. > >> > > >> > Sorry, disagree. > >> > > >> > diff -r 1:5 != diff -r 5:1 > >> > log -r 1:5 != log -r 5:1 > >> > merge -r 4:5 != merge -r 5:4 > >> > > >> > With all that in mind, I still think that making 'blame -r 5:4' and > >> > 'blame -r 4:5' do different things is the correct course of action. > >> > > >> > >> Okay, I don't feel strongly about this. My only "argument" was that > >> people are not used to thinking about the order of rev args when using > >> blame. But that doesn't mean they can't get used to it ... > > > > Implemented in r1493027. No API changes are involved --- this simply > > makes 'blame -r 5:4' do something instead of raising an error > > immediately --- so I wonder if we should backport it. > > > > I'll go ahead and put it in STATUS towards 1.8.1, if people prefer a > > backport not to happen they can go ahead and cast -0 votes and continue > > discussion here. > > There are still two problems with the implementation you committed in > r1493027: > > With 'svn blame M:N' where M>N > > 1) It's using the N as peg revision, while it should use M (but you're > already working on that). > Should be fixed by r1493106. I'd welcome further review of that, I am unsure that the "open ra session to the other svn_opt_revision_t" part is idiomatic. > 2) If N is before the item existed, I get: > svn: E195012: Unable to find repository location for > 'svn://localhost/path/to/file.txt' in revision 1 > > It would be nice if you could just blame up to the oldest revision > close to 'end' where the item still existed. Yeah, it would be nice if 'svn blame -r HEAD:1' just worked even for files added later. I can look into that, but not today :) It would be helpful if someone could point me to another place in the codebase that solves the same problem. Cheers, Daniel
RE: Kidney blame's behaviour and edge cases
I would guess 1 and twi are actually the same problem: no node found via peg revision. Bert From: Johan Corveleyn Sent: 14/06/2013 16:51 To: Daniel Shahaf Cc: Subversion Development Subject: Re: Kidney blame's behaviour and edge cases On Fri, Jun 14, 2013 at 1:33 PM, Daniel Shahaf wrote: > Johan Corveleyn wrote on Fri, Jun 14, 2013 at 11:16:06 +0200: >> On Fri, Jun 14, 2013 at 11:00 AM, Daniel Shahaf wrote: >> > Doug Robinson wrote on Thu, Jun 13, 2013 at 12:10:49 -0400: >> >> Daniel: >> >> >> >> I think that simply enabling M> >> the >> >> situation where the user makes a mistake, gets something they don't expect >> >> and tries to interpret it based on their desire - leading to confusion. I >> >> believe M> >> be >> >> required to make it clear that the user wants the reverse blame walk. >> > >> > Sorry, disagree. >> > >> > diff -r 1:5 != diff -r 5:1 >> > log -r 1:5 != log -r 5:1 >> > merge -r 4:5 != merge -r 5:4 >> > >> > With all that in mind, I still think that making 'blame -r 5:4' and >> > 'blame -r 4:5' do different things is the correct course of action. >> > >> >> Okay, I don't feel strongly about this. My only "argument" was that >> people are not used to thinking about the order of rev args when using >> blame. But that doesn't mean they can't get used to it ... > > Implemented in r1493027. No API changes are involved --- this simply > makes 'blame -r 5:4' do something instead of raising an error > immediately --- so I wonder if we should backport it. > > I'll go ahead and put it in STATUS towards 1.8.1, if people prefer a > backport not to happen they can go ahead and cast -0 votes and continue > discussion here. There are still two problems with the implementation you committed in r1493027: With 'svn blame M:N' where M>N 1) It's using the N as peg revision, while it should use M (but you're already working on that). 2) If N is before the item existed, I get: svn: E195012: Unable to find repository location for 'svn://localhost/path/to/file.txt' in revision 1 It would be nice if you could just blame up to the oldest revision close to 'end' where the item still existed. -- Johan
Re: Kidney blame's behaviour and edge cases
On Fri, Jun 14, 2013 at 1:33 PM, Daniel Shahaf wrote: > Johan Corveleyn wrote on Fri, Jun 14, 2013 at 11:16:06 +0200: >> On Fri, Jun 14, 2013 at 11:00 AM, Daniel Shahaf wrote: >> > Doug Robinson wrote on Thu, Jun 13, 2013 at 12:10:49 -0400: >> >> Daniel: >> >> >> >> I think that simply enabling M> >> the >> >> situation where the user makes a mistake, gets something they don't expect >> >> and tries to interpret it based on their desire - leading to confusion. I >> >> believe M> >> be >> >> required to make it clear that the user wants the reverse blame walk. >> > >> > Sorry, disagree. >> > >> > diff -r 1:5 != diff -r 5:1 >> > log -r 1:5 != log -r 5:1 >> > merge -r 4:5 != merge -r 5:4 >> > >> > With all that in mind, I still think that making 'blame -r 5:4' and >> > 'blame -r 4:5' do different things is the correct course of action. >> > >> >> Okay, I don't feel strongly about this. My only "argument" was that >> people are not used to thinking about the order of rev args when using >> blame. But that doesn't mean they can't get used to it ... > > Implemented in r1493027. No API changes are involved --- this simply > makes 'blame -r 5:4' do something instead of raising an error > immediately --- so I wonder if we should backport it. > > I'll go ahead and put it in STATUS towards 1.8.1, if people prefer a > backport not to happen they can go ahead and cast -0 votes and continue > discussion here. There are still two problems with the implementation you committed in r1493027: With 'svn blame M:N' where M>N 1) It's using the N as peg revision, while it should use M (but you're already working on that). 2) If N is before the item existed, I get: svn: E195012: Unable to find repository location for 'svn://localhost/path/to/file.txt' in revision 1 It would be nice if you could just blame up to the oldest revision close to 'end' where the item still existed. -- Johan
Re: Kidney blame's behaviour and edge cases
Johan Corveleyn wrote on Fri, Jun 14, 2013 at 11:16:06 +0200: > On Fri, Jun 14, 2013 at 11:00 AM, Daniel Shahaf wrote: > > Doug Robinson wrote on Thu, Jun 13, 2013 at 12:10:49 -0400: > >> Daniel: > >> > >> I think that simply enabling M >> situation where the user makes a mistake, gets something they don't expect > >> and tries to interpret it based on their desire - leading to confusion. I > >> believe M >> required to make it clear that the user wants the reverse blame walk. > > > > Sorry, disagree. > > > > diff -r 1:5 != diff -r 5:1 > > log -r 1:5 != log -r 5:1 > > merge -r 4:5 != merge -r 5:4 > > > > With all that in mind, I still think that making 'blame -r 5:4' and > > 'blame -r 4:5' do different things is the correct course of action. > > > > Okay, I don't feel strongly about this. My only "argument" was that > people are not used to thinking about the order of rev args when using > blame. But that doesn't mean they can't get used to it ... Implemented in r1493027. No API changes are involved --- this simply makes 'blame -r 5:4' do something instead of raising an error immediately --- so I wonder if we should backport it. I'll go ahead and put it in STATUS towards 1.8.1, if people prefer a backport not to happen they can go ahead and cast -0 votes and continue discussion here.
Re: Kidney blame's behaviour and edge cases
On Fri, Jun 14, 2013 at 11:36 AM, Daniel Shahaf wrote: > Johan Corveleyn wrote on Fri, Jun 14, 2013 at 11:16:06 +0200: >> On Fri, Jun 14, 2013 at 11:00 AM, Daniel Shahaf wrote: >> > Doug Robinson wrote on Thu, Jun 13, 2013 at 12:10:49 -0400: >> >> Daniel: >> >> >> >> I think that simply enabling M> >> the >> >> situation where the user makes a mistake, gets something they don't expect >> >> and tries to interpret it based on their desire - leading to confusion. I >> >> believe M> >> be >> >> required to make it clear that the user wants the reverse blame walk. >> > >> > Sorry, disagree. >> > >> > diff -r 1:5 != diff -r 5:1 >> > log -r 1:5 != log -r 5:1 >> > merge -r 4:5 != merge -r 5:4 >> > >> > With all that in mind, I still think that making 'blame -r 5:4' and >> > 'blame -r 4:5' do different things is the correct course of action. >> > >> >> Okay, I don't feel strongly about this. My only "argument" was that >> people are not used to thinking about the order of rev args when using >> blame. But that doesn't mean they can't get used to it ... > > Do people use blame -r M:N at all? I would expect that 'svn blame file' > and 'svn blame -r N file' / 'svn blame file@rN' are more popular > invocations. Good point. Okay, +1 on just letting -r5:4 do the reverse thing without anything more. GUI's can obviously do other things in their dialog windows to make things clear to users, if they want to. But for the commandline I think your proposal is fine. -- Johan
Re: Kidney blame's behaviour and edge cases
Johan Corveleyn wrote on Fri, Jun 14, 2013 at 11:16:06 +0200: > On Fri, Jun 14, 2013 at 11:00 AM, Daniel Shahaf wrote: > > Doug Robinson wrote on Thu, Jun 13, 2013 at 12:10:49 -0400: > >> Daniel: > >> > >> I think that simply enabling M >> situation where the user makes a mistake, gets something they don't expect > >> and tries to interpret it based on their desire - leading to confusion. I > >> believe M >> required to make it clear that the user wants the reverse blame walk. > > > > Sorry, disagree. > > > > diff -r 1:5 != diff -r 5:1 > > log -r 1:5 != log -r 5:1 > > merge -r 4:5 != merge -r 5:4 > > > > With all that in mind, I still think that making 'blame -r 5:4' and > > 'blame -r 4:5' do different things is the correct course of action. > > > > Okay, I don't feel strongly about this. My only "argument" was that > people are not used to thinking about the order of rev args when using > blame. But that doesn't mean they can't get used to it ... Do people use blame -r M:N at all? I would expect that 'svn blame file' and 'svn blame -r N file' / 'svn blame file@rN' are more popular invocations.
Re: Kidney blame's behaviour and edge cases
Daniel Shahaf wrote on Fri, Jun 14, 2013 at 11:18:35 +0200: > Prabhu wrote on Fri, Jun 14, 2013 at 14:33:57 +0530: > > On 06/14/2013 02:30 PM, Daniel Shahaf wrote: > > >Doug Robinson wrote on Thu, Jun 13, 2013 at 12:10:49 -0400: > > >>Daniel: > > >> > > >>I think that simply enabling M > >>the > > >>situation where the user makes a mistake, gets something they don't expect > > >>and tries to interpret it based on their desire - leading to confusion. I > > >>believe M > >>be > > >>required to make it clear that the user wants the reverse blame walk. > > >Sorry, disagree. > > > > > >diff -r 1:5 != diff -r 5:1 > > >log -r 1:5 != log -r 5:1 > > >merge -r 4:5 != merge -r 5:4 > > > > > >With all that in mind, I still think that making 'blame -r 5:4' and > > >'blame -r 4:5' do different things is the correct course of action. > > > > > > Yeah, perhaps 'blame -r 5:4' and 'blame -r4:5 --reverse' should do > > the same ? > > If you do that, why not allow 'svn merge -c 5 --reverse', 'svn diff -c 5 > --reverse', etc as well? And, of course, 'svn merge -c-5 --reverse', which is going to be confusing to someone regardless of whether you define it as -r4:5 or as -r5:4 ...
Re: Kidney blame's behaviour and edge cases
Prabhu wrote on Fri, Jun 14, 2013 at 14:33:57 +0530: > On 06/14/2013 02:30 PM, Daniel Shahaf wrote: > >Doug Robinson wrote on Thu, Jun 13, 2013 at 12:10:49 -0400: > >>Daniel: > >> > >>I think that simply enabling M >>situation where the user makes a mistake, gets something they don't expect > >>and tries to interpret it based on their desire - leading to confusion. I > >>believe M >>required to make it clear that the user wants the reverse blame walk. > >Sorry, disagree. > > > >diff -r 1:5 != diff -r 5:1 > >log -r 1:5 != log -r 5:1 > >merge -r 4:5 != merge -r 5:4 > > > >With all that in mind, I still think that making 'blame -r 5:4' and > >'blame -r 4:5' do different things is the correct course of action. > > > Yeah, perhaps 'blame -r 5:4' and 'blame -r4:5 --reverse' should do > the same ? If you do that, why not allow 'svn merge -c 5 --reverse', 'svn diff -c 5 --reverse', etc as well? Does "There should be one-- and preferably only one --obvious way to do it." not apply here?
Re: Kidney blame's behaviour and edge cases
On Fri, Jun 14, 2013 at 11:00 AM, Daniel Shahaf wrote: > Doug Robinson wrote on Thu, Jun 13, 2013 at 12:10:49 -0400: >> Daniel: >> >> I think that simply enabling M> situation where the user makes a mistake, gets something they don't expect >> and tries to interpret it based on their desire - leading to confusion. I >> believe M> required to make it clear that the user wants the reverse blame walk. > > Sorry, disagree. > > diff -r 1:5 != diff -r 5:1 > log -r 1:5 != log -r 5:1 > merge -r 4:5 != merge -r 5:4 > > With all that in mind, I still think that making 'blame -r 5:4' and > 'blame -r 4:5' do different things is the correct course of action. > Okay, I don't feel strongly about this. My only "argument" was that people are not used to thinking about the order of rev args when using blame. But that doesn't mean they can't get used to it ... -- Johan
Re: Kidney blame's behaviour and edge cases
On 06/14/2013 02:30 PM, Daniel Shahaf wrote: Doug Robinson wrote on Thu, Jun 13, 2013 at 12:10:49 -0400: Daniel: I think that simply enabling M Sorry, disagree. diff -r 1:5 != diff -r 5:1 log -r 1:5 != log -r 5:1 merge -r 4:5 != merge -r 5:4 With all that in mind, I still think that making 'blame -r 5:4' and 'blame -r 4:5' do different things is the correct course of action. Yeah, perhaps 'blame -r 5:4' and 'blame -r4:5 --reverse' should do the same ? -- Prabhu
Re: Kidney blame's behaviour and edge cases
Doug Robinson wrote on Thu, Jun 13, 2013 at 12:10:49 -0400: > Daniel: > > I think that simply enabling M situation where the user makes a mistake, gets something they don't expect > and tries to interpret it based on their desire - leading to confusion. I > believe M required to make it clear that the user wants the reverse blame walk. Sorry, disagree. diff -r 1:5 != diff -r 5:1 log -r 1:5 != log -r 5:1 merge -r 4:5 != merge -r 5:4 With all that in mind, I still think that making 'blame -r 5:4' and 'blame -r 4:5' do different things is the correct course of action. Cheers, Daniel
Re: Kidney blame's behaviour and edge cases
Johan Corveleyn wrote: >On Thu, Jun 13, 2013 at 6:10 PM, Doug Robinson > wrote: >> Daniel: >> >> I think that simply enabling Mcreate the >> situation where the user makes a mistake, gets something they don't >expect >> and tries to interpret it based on their desire - leading to >confusion. I >> believe Mshould be >> required to make it clear that the user wants the reverse blame walk. > >I agree. I think it would be better to make the reverseness explicit >in the UI. When running blame, users are not used to think about the >order of their -r arguments. > >But then I'm wondering: should 'svn blame --reverse -r1:5' give an >error, just like 'svn blame -r5:1' does? Or should it silently swap >the revnum args? Hmmm I would say, silently swap and blame because to me that seems more sensible... -- prabhu -- Sent from my Android phone with K-9 Mail. Please excuse my brevity.
Re: Kidney blame's behaviour and edge cases
On Thu, Jun 13, 2013 at 6:10 PM, Doug Robinson wrote: > Daniel: > > I think that simply enabling M situation where the user makes a mistake, gets something they don't expect > and tries to interpret it based on their desire - leading to confusion. I > believe M required to make it clear that the user wants the reverse blame walk. I agree. I think it would be better to make the reverseness explicit in the UI. When running blame, users are not used to think about the order of their -r arguments. But then I'm wondering: should 'svn blame --reverse -r1:5' give an error, just like 'svn blame -r5:1' does? Or should it silently swap the revnum args? Hmmm (BTW, I think the reverse (kidney) blame will always remain a rarely used usecase. It's really kind of advanced, just thinking about it ...) -- Johan
Re: Kidney blame's behaviour and edge cases
Daniel:
I think that simply enabling M wrote:
> Definition: "kidney blame" == "blame -r N:M" with M an error (raised by svn_client_blame5()).
>
> By and large, it should do exactly what 'blame -r M:N' does: walk the
> revisions from "before-the-colon revision" to "after-the-colon revision"
> and then print the contents of the "after the colon" revision, with each
> line annotated by the revnum of the "rightmost" (that is: nearest to the
> revnum on the right of the colon) revision within the range that added
> that line. In other words, if
> (iota@4, kappa@1), (iota@3, kappa@2), (iota@2, kappa@3), (iota@1, kappa@4)
> are pairs of byte-for-byte-identical files, then 'blame -r 1:4 kappa'
> and 'blame -r 4:1 iota' will output the same thing.
>
> Currently, the non-kidney blame checks one revision before the start ---
> that is, 'blame -r M:N' actually calls svn_ra_get_file_revs2(start=M-1,
> end=N). The purpose of this is to differentiate "lines added in rN"
> from "lines present since before rN". That makes 'blame -r M:N'
> correspond to 'diff -r M-1:N' or 'diff -c M:N', in that it also shows
> changes made _by_ rM, not only _since_ rM.
>
> That behaviour cannot easily replicated for the -r N:M case since it's
> not possible to cheaply find the "next interesting revision" after rN,
> but in any case I think it shouldn't be: 'blame -r M:N' should not have
> automagically decremented M --- instead, if the user had wanted to see
> what lines were added in rM, as opposed to before it, the user should
> have specified M-1 as the start revision. That way, 'blame -r M:N'
> would be consistent with 'diff -r M:N'.
>
> So I suggest that blame -r N:M not try to find the "next change after
> rN" (that's the analogous thing to what "decrement M" is in the '-r M:N'
> case). That means -r M:N and -r N:M are assymetrical, and I propose we
> fix that by making -r M:N not decrement M --- which we can probably only
> do in 2.0.
>
> ---
>
> Another issue: what should 'blame -r 3:3' do? Currently it is allowed,
> and prints '-' for lines added before r3 and '3' for lines added in r3.
> I am not sure whether that is intentional / by design, or just an
> accident of the fact that whoever added the 'end_rev < start_rev' check
> should have used the broader condition 'end_rev <= start_rev' instead.
> (See r7438 <-> http://svn.apache.org/r847512).
>
> It seems to me it should ideally print '3' for every line, and the user
> should pass '-r 2:3' if he wants to distinguish "added in r3" from
> "added before r3". It would be easy to preserve the current behaviour,
> though, of printing '-' rather than '2' (where '2' here is the youngest
> change to that line, for lines added before r3).
>
> ---
>
> Cheers,
>
> Daniel
>
--
Douglas B. Robinson | *Senior Product Manager*
WANdisco // *Non-Stop Data*
t. 925-396-1125
e. doug.robin...@wandisco.com
--
THIS MESSAGE AND ANY ATTACHMENTS ARE CONFIDENTIAL, PROPRIETARY, AND MAY BE
PRIVILEGED. If this message was misdirected, WANdisco, Inc. and its
subsidiaries, ("WANdisco") does not waive any confidentiality or privilege.
If you are not the intended recipient, please notify us immediately and
destroy the message without disclosing its contents to anyone. Any
distribution, use or copying of this e-mail or the information it contains
by other than an intended recipient is unauthorized. The views and
opinions expressed in this e-mail message are the author's own and may not
reflect the views and opinions of WANdisco, unless the author is authorized
by WANdisco to express such views or opinions on its behalf. All email
sent to or from this address is subject to electronic storage and review by
WANdisco. Although WANdisco operates anti-virus programs, it does not
accept responsibility for any damage whatsoever caused by viruses being
passed.
RE: Kidney blame's behaviour and edge cases
> -Original Message- > From: Daniel Shahaf [mailto:danie...@elego.de] > Sent: donderdag 13 juni 2013 10:36 > To: dev@subversion.apache.org > Subject: Kidney blame's behaviour and edge cases > > Another issue: what should 'blame -r 3:3' do? Currently it is allowed, > and prints '-' for lines added before r3 and '3' for lines added in r3. > I am not sure whether that is intentional / by design, or just an > accident of the fact that whoever added the 'end_rev < start_rev' check > should have used the broader condition 'end_rev <= start_rev' instead. > (See r7438 <-> http://svn.apache.org/r847512). > > It seems to me it should ideally print '3' for every line, and the user > should pass '-r 2:3' if he wants to distinguish "added in r3" from > "added before r3". It would be easy to preserve the current behaviour, > though, of printing '-' rather than '2' (where '2' here is the youngest > change to that line, for lines added before r3). I would call this intentional, and would really like to keep this behavior. I certainly don't call it a bug, as this is what makes it possible to know that you need a larger range for blaming to obtain more info. Bert
Re: Kidney blame's behaviour and edge cases
Branko Čibej wrote on Thu, Jun 13, 2013 at 10:54:47 +0200: > On 13.06.2013 10:35, Daniel Shahaf wrote: > > It seems to me it should ideally print '3' for every line, and the user > > should pass '-r 2:3' if he wants to distinguish "added in r3" from > > "added before r3". It would be easy to preserve the current behaviour, > > though, of printing '-' rather than '2' (where '2' here is the youngest > > change to that line, for lines added before r3). > > It strikes me that what you're really looking for is either > (I assume you meant M -rN:M -> [N, M-1], > > which would make the two blame variants symmetrical, or > It would make them symmetrical in that they both extend the range one revision in the "towards older" direction. I think that's not the symmetry we need. My kappa/iota example outlines the kind of symmetry I'm expecting. > -rN:M -> [N+1,M] > > as you don't really have to find "the next interesting change" -- you > only have to make sure the first bound is inclusive, as with -c, rather > than exclusive, as with -r. If you want to make the first bound inclusive, you need to extend the range in the opposite direction, into [next-younger-interesting-revision, M]. (And then you run into the problem that in M:N you can give M=0, but there's no equivalent for that in the N:M case.) > That would also imply > > -cN:M -> [N+1,M] > > and I'm not sure we currently do it that way. 'svn blame' does not take the '-c' option, only '-r'.
Re: Kidney blame's behaviour and edge cases
On 13.06.2013 10:35, Daniel Shahaf wrote: > It seems to me it should ideally print '3' for every line, and the user > should pass '-r 2:3' if he wants to distinguish "added in r3" from > "added before r3". It would be easy to preserve the current behaviour, > though, of printing '-' rather than '2' (where '2' here is the youngest > change to that line, for lines added before r3). It strikes me that what you're really looking for is either -rN:M -> [N, M-1], which would make the two blame variants symmetrical, or -rN:M -> [N+1,M] as you don't really have to find "the next interesting change" -- you only have to make sure the first bound is inclusive, as with -c, rather than exclusive, as with -r. That would also imply -cN:M -> [N+1,M] and I'm not sure we currently do it that way. -- Brane -- Branko Čibej | Director of Subversion WANdisco // Non-Stop Data e. br...@wandisco.com
Kidney blame's behaviour and edge cases
Definition: "kidney blame" == "blame -r N:M" with M http://svn.apache.org/r847512). It seems to me it should ideally print '3' for every line, and the user should pass '-r 2:3' if he wants to distinguish "added in r3" from "added before r3". It would be easy to preserve the current behaviour, though, of printing '-' rather than '2' (where '2' here is the youngest change to that line, for lines added before r3). --- Cheers, Daniel
