Add option to specify similarity level for git diff operations on commandline

Add option to specify similarity level for git diff operations on commandline


BUG=125983
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159732

[cmp, 2012-09-18 01:44:15]

lgtm with nits

https://codereview.chromium.org/10945002/diff/2001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/10945002/diff/2001/git_cl.py#newcode1684
git_cl.py:1684: '-C', '-M', '--git_similarity', '--find-copies-harder',
type='int',
let's call this just --similarity to keep it simpler

https://codereview.chromium.org/10945002/diff/2001/git_cl.py#newcode1687
git_cl.py:1687: ' be considered copies (default 50)')
please note here that this is only used for Git (not for SVN)

https://codereview.chromium.org/10945002/diff/2001/third_party/upload.py
File third_party/upload.py (right):

https://codereview.chromium.org/10945002/diff/2001/third_party/upload.py#newc...
third_party/upload.py:591: group.add_option("--git_similarity", action="store",
dest="git_similarity",
let's also call this --similarity

[M-A Ruel, 2012-09-18 10:22:59]

I prefer it to be a:
1. A branch-specific variable, set and read with git config.
2. Add a stand alone command to change the value.

The reason is that when you start uploading often, it is inevitable that the dev
will forget the flag one time and it will not be necessarily obvious for him.

[szager, 2012-09-18 17:05:00]

The CL lgtm, but how comfortable are we all with 50% as the default?  It seems
at least plausible that someone might cut-and-paste more than half of an
existing file, but might not want to record that as a copy.

[iannucci, 2012-09-18 17:16:05]

On 2012/09/18 01:44:15, cmp wrote:
> lgtm with nits
> 
> https://codereview.chromium.org/10945002/diff/2001/git_cl.py
> File git_cl.py (right):
> 
> https://codereview.chromium.org/10945002/diff/2001/git_cl.py#newcode1684
> git_cl.py:1684: '-C', '-M', '--git_similarity', '--find-copies-harder',
> type='int',
> let's call this just --similarity to keep it simpler

I went with git_similarity to be in-sync with the other VCS-specific commands
(like the p4 ones). still want --similarity?
> 
> https://codereview.chromium.org/10945002/diff/2001/git_cl.py#newcode1687
> git_cl.py:1687: ' be considered copies (default 50)')
> please note here that this is only used for Git (not for SVN)
> 

This is in the 'git-specific commands' section in the help. Does it need a
second note?

> https://codereview.chromium.org/10945002/diff/2001/third_party/upload.py
> File third_party/upload.py (right):
> 
>
https://codereview.chromium.org/10945002/diff/2001/third_party/upload.py#newc...
> third_party/upload.py:591: group.add_option("--git_similarity",
action="store",
> dest="git_similarity",
> let's also call this --similarity

[iannucci, 2012-09-18 17:16:42]

On 2012/09/18 17:05:00, szager wrote:
> The CL lgtm, but how comfortable are we all with 50% as the default?  It seems
> at least plausible that someone might cut-and-paste more than half of an
> existing file, but might not want to record that as a copy.

50% is the git default already :)

[iannucci, 2012-09-18 17:18:22]

On 2012/09/18 10:22:59, Marc-Antoine Ruel wrote:
> I prefer it to be a:
> 1. A branch-specific variable, set and read with git config.
> 2. Add a stand alone command to change the value.
> 
> The reason is that when you start uploading often, it is inevitable that the
dev
> will forget the flag one time and it will not be necessarily obvious for him.

Good point. Would prompting when they specify it here ("Do you want to save this
similarity index for next time? (Y/N)") and saving it into the git config be
good enough?

[M-A Ruel, 2012-09-18 17:25:43]

On 2012/09/18 17:18:22, iannucci wrote:
> On 2012/09/18 10:22:59, Marc-Antoine Ruel wrote:
> > I prefer it to be a:
> > 1. A branch-specific variable, set and read with git config.
> > 2. Add a stand alone command to change the value.
> > 
> > The reason is that when you start uploading often, it is inevitable that the
> dev
> > will forget the flag one time and it will not be necessarily obvious for
him.
> 
> Good point. Would prompting when they specify it here ("Do you want to save
this
> similarity index for next time? (Y/N)") and saving it into the git config be
> good enough?

No need for prompt, make it sticky with a warning when you save it and a warning
when you load a value so it is not silent. Examples:

When saving;
Note: Saving similarity autodetection at %d%% in git config.

When loading;
Note: Using %d%% of similarity for rename/copy detection. Override with --foo

[iannucci, 2012-09-18 17:35:13]

On 2012/09/18 17:25:43, Marc-Antoine Ruel wrote:
> On 2012/09/18 17:18:22, iannucci wrote:
> > On 2012/09/18 10:22:59, Marc-Antoine Ruel wrote:
> > > I prefer it to be a:
> > > 1. A branch-specific variable, set and read with git config.
> > > 2. Add a stand alone command to change the value.
> > > 
> > > The reason is that when you start uploading often, it is inevitable that
the
> > dev
> > > will forget the flag one time and it will not be necessarily obvious for
> him.
> > 
> > Good point. Would prompting when they specify it here ("Do you want to save
> this
> > similarity index for next time? (Y/N)") and saving it into the git config be
> > good enough?
> 
> No need for prompt, make it sticky with a warning when you save it and a
warning
> when you load a value so it is not silent. Examples:
> 
> When saving;
> Note: Saving similarity autodetection at %d%% in git config.
> 
> When loading;
> Note: Using %d%% of similarity for rename/copy detection. Override with --foo

sgtm. Will do.

[iannucci, 2012-09-18 23:51:16]

On 2012/09/18 17:16:05, iannucci wrote:
> On 2012/09/18 01:44:15, cmp wrote:
> > lgtm with nits
> > 
> > https://codereview.chromium.org/10945002/diff/2001/git_cl.py
> > File git_cl.py (right):
> > 
> > https://codereview.chromium.org/10945002/diff/2001/git_cl.py#newcode1684
> > git_cl.py:1684: '-C', '-M', '--git_similarity', '--find-copies-harder',
> > type='int',
> > let's call this just --similarity to keep it simpler
> 
> I went with git_similarity to be in-sync with the other VCS-specific commands
> (like the p4 ones). still want --similarity?
> > 
> > https://codereview.chromium.org/10945002/diff/2001/git_cl.py#newcode1687
> > git_cl.py:1687: ' be considered copies (default 50)')
> > please note here that this is only used for Git (not for SVN)
> > 
> 
> This is in the 'git-specific commands' section in the help. Does it need a
> second note?
> 
> > https://codereview.chromium.org/10945002/diff/2001/third_party/upload.py
> > File third_party/upload.py (right):
> > 
> >
>
https://codereview.chromium.org/10945002/diff/2001/third_party/upload.py#newc...
> > third_party/upload.py:591: group.add_option("--git_similarity",
> action="store",
> > dest="git_similarity",
> > let's also call this --similarity

Actually, this needs to be clarified.

It's git_similarity in upload.py b/c of the other args nearby. I used the same
name in git_cl.py for consistency, but --similarity *is* unambiguous in git_cl
(so I'll change it there).  I'm thinking that the upload.py one should stay
though.

RE: the help text in git_cl: do you think it still needs to mention that it's
git-only (perhaps to indicate that it's not a feature of the underlying svn)? It
IS in a git-only command though.

In upload.py, the help text is in a 'git-only' section.

R

[M-A Ruel, 2012-09-19 00:04:40]

On 2012/09/18 23:51:16, iannucci wrote:
> It's git_similarity in upload.py b/c of the other args nearby. I used the same
> name in git_cl.py for consistency, but --similarity *is* unambiguous in git_cl
> (so I'll change it there).  I'm thinking that the upload.py one should stay
> though.

No need to have the same argument names in upload.py and git-cl. Nobody should
use upload.py directly in the Chromium team.


> RE: the help text in git_cl: do you think it still needs to mention that it's
> git-only (perhaps to indicate that it's not a feature of the underlying svn)?
It
> IS in a git-only command though.

No, it's obvious.

[iannucci, 2012-09-19 02:53:46]

New logic, PTAL. The sticky thing is actually pretty convenient :).

[iannucci, 2012-09-24 22:36:10]

@maruel PTAL at the new git similarity stashing logic. I'm adding a new positive
test case too.

[iannucci, 2012-09-24 23:15:55]

Ok, added a positive test cast for setting similarity... It feels a little
kludgey, but I know that there is no bigger trap than spending huge amounts of
time making the tests pretty. If there's an obviously better way to test this,
I'm all ears!!

[M-A Ruel, 2012-09-25 01:25:02]

technically, lgtm.

I'd prefer the upload.py change to be upstreamed first then update back here.

https://codereview.chromium.org/10945002/diff/17001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/10945002/diff/17001/git_cl.py#newcode111
git_cl.py:111: options.similarity = int(stdout.strip())
Put only this line inside the try/except block.

https://codereview.chromium.org/10945002/diff/17001/git_cl.py#newcode124
git_cl.py:124: 'Override with --similarity.' % options.similarity)
I'm not sure this works? IIRC the operator % has an higher priority than string
concatenation. I don't have access to a real *cough* computer at the moment to
verify.

https://codereview.chromium.org/10945002/diff/17001/git_cl.py#newcode127
git_cl.py:127: parser.parse_args = Parse
A reason I removed all the parse_args monkey patching is that it confuses the
heck out of pylint. If you don't mind, I'd prefer to not do it and use a file
visible function, even if this is the normal python way.

[iannucci, 2012-09-25 02:01:47]

https://codereview.chromium.org/10945002/diff/17001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/10945002/diff/17001/git_cl.py#newcode111
git_cl.py:111: options.similarity = int(stdout.strip())
On 2012/09/25 01:25:02, Marc-Antoine Ruel wrote:
> Put only this line inside the try/except block.

Done

https://codereview.chromium.org/10945002/diff/17001/git_cl.py#newcode124
git_cl.py:124: 'Override with --similarity.' % options.similarity)
On 2012/09/25 01:25:02, Marc-Antoine Ruel wrote:
> I'm not sure this works? IIRC the operator % has an higher priority than
string
> concatenation. I don't have access to a real *cough* computer at the moment to
> verify.

Yeah it does work :). Good eye though

https://codereview.chromium.org/10945002/diff/17001/git_cl.py#newcode127
git_cl.py:127: parser.parse_args = Parse
On 2012/09/25 01:25:02, Marc-Antoine Ruel wrote:
> A reason I removed all the parse_args monkey patching is that it confuses the
> heck out of pylint. If you don't mind, I'd prefer to not do it and use a file
> visible function, even if this is the normal python way.

You mean to use functools.partial on a module-level function instead of a
closure? Or something else? My pylint seems to be ok with it, and we're doing
the same trick in main already.

[M-A Ruel, 2012-09-27 14:29:01]

Do you want me to push upload.py upstream or you want to do it?

https://codereview.chromium.org/10945002/diff/19002/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/10945002/diff/19002/git_cl.py#newcode121
git_cl.py:121: options.similarity = max(1, min(options.similarity, 100))
Maybe max(0, ...) ?

[iannucci, 2012-09-27 17:52:29]

On 2012/09/27 14:29:01, Marc-Antoine Ruel wrote:
> Do you want me to push upload.py upstream or you want to do it?

I'll make a cl for upload.py

> https://codereview.chromium.org/10945002/diff/19002/git_cl.py
> File git_cl.py (right):
> 
> https://codereview.chromium.org/10945002/diff/19002/git_cl.py#newcode121
> git_cl.py:121: options.similarity = max(1, min(options.similarity, 100))
> Maybe max(0, ...) ?

I'm not sure a similarity of 0 makes sense/is allowed. Git could decide that a
new source file is actually a copy of any other file in the repo. Honestly
anything below 25% doesn't really make sense either. I am just trying to mimic
the range offered by git.

[iannucci, 2012-09-27 19:21:31]

On 2012/09/27 17:52:29, iannucci wrote:
> On 2012/09/27 14:29:01, Marc-Antoine Ruel wrote:
> > Do you want me to push upload.py upstream or you want to do it?
> 
> I'll make a cl for upload.py
> 
> > https://codereview.chromium.org/10945002/diff/19002/git_cl.py
> > File git_cl.py (right):
> > 
> > https://codereview.chromium.org/10945002/diff/19002/git_cl.py#newcode121
> > git_cl.py:121: options.similarity = max(1, min(options.similarity, 100))
> > Maybe max(0, ...) ?
> 
> I'm not sure a similarity of 0 makes sense/is allowed. Git could decide that a
> new source file is actually a copy of any other file in the repo. Honestly
> anything below 25% doesn't really make sense either. I am just trying to mimic
> the range offered by git.

Upload.py is here: http://codereview.appspot.com/6574057

[commit-bot: I haz the power, 2012-10-02 18:17:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/iannucci@chromium.org/10945002/25001

[commit-bot: I haz the power, 2012-10-02 18:20:52]

Change committed as 159732