threads / rfc / 7978

RFC patchActivate rename detection by default.

Subject: [StGIT RFC PATCH] Activate rename detection by default.

## tl;dr

4 messages between May 4, 2007 and May 15, 2007. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Yann Dirson· May 4, 2007, 23:54 UTC · lore

This patch uses -M when generating diffs, which then looks fine, just like any other git-generated diff.

While I was at it, I also added -M when calling "stg files" or "stg status". While we need that for completeness, we currently just display the raw git-diff-tree output, and it may break some scripts. We may also want to make it more human-readable (not sure how, esp. if we want to remain machine-parsable).

It also raises the issue of whether to activate this by default or not (mostly in status and files), and how to select this behaviour - something which will possibly be needed to be decided before introducing -C and --find-copies-harder, which would be useful as well, the latter being definitely a candidate to be off by default.

That gives:

$ ./stg status ? contrib/stg-push-force R100 README README2 M stgit/git.py

$ ./stg files sink R056 Documentation/stg-bury.txt Documentation/stg-sink.txt M Documentation/stg.txt M contrib/stgit-completion.bash R093 stgit/commands/bury.py stgit/commands/sink.py M stgit/main.py

Signed-off-by: Yann Dirson <ydirson@altern.org>
---
 README2      |    0 
 stgit/git.py |   12 ++++++------
 2 files changed, 6 insertions(+), 6 deletions(-)
Show changes to 2 files +6 −6

README, stgit/git.py

diff --git a/README b/README2
similarity index 100%
rename from README
rename to README2
diff --git a/stgit/git.py b/stgit/git.py
index d7eb48e..ed09eca 100644
--- a/stgit/git.py
+++ b/stgit/git.py
@@ -255,7 +255,7 @@ def __tree_status(files = None, tree_id = 'HEAD', unknown = False,
     cache_files += [('C', filename) for filename in conflicts]
 
     # the rest
-    for line in _output_lines(['git-diff-index', tree_id, '--'] + files):
+    for line in _output_lines(['git-diff-index', '-M', tree_id, '--'] + files):
         fs = tuple(line.rstrip().split(' ',4)[-1].split('\t',1))
         if fs[1] not in conflicts:
             cache_files.append(fs)
@@ -774,13 +774,13 @@ def diff(files = None, rev1 = 'HEAD', rev2 = None, out_fd = None):
         files = []
 
     if rev1 and rev2:
-        diff_str = _output(['git-diff-tree', '-p', rev1, rev2, '--'] + files)
+        diff_str = _output(['git-diff-tree', '-M', '-p', rev1, rev2, '--'] + files)
     elif rev1 or rev2:
         refresh_index()
         if rev2:
-            diff_str = _output(['git-diff-index', '-p', '-R', rev2, '--'] + files)
+            diff_str = _output(['git-diff-index', '-M', '-p', '-R', rev2, '--'] + files)
         else:
-            diff_str = _output(['git-diff-index', '-p', rev1, '--'] + files)
+            diff_str = _output(['git-diff-index', '-M', '-p', rev1, '--'] + files)
     else:
         diff_str = ''
 
@@ -808,7 +808,7 @@ def files(rev1, rev2):
     """
 
     result = ''
-    for line in _output_lines('git-diff-tree -r %s %s' % (rev1, rev2)):
+    for line in _output_lines('git-diff-tree -M -r %s %s' % (rev1, rev2)):
         result += '%s %s\n' % tuple(line.rstrip().split(' ',4)[-1].split('\t',1))
 
     return result.rstrip()
@@ -826,7 +826,7 @@ def barefiles(rev1, rev2):
 def pretty_commit(commit_id = 'HEAD'):
     """Return a given commit (log + diff)
     """
-    return _output(['git-diff-tree', '--cc', '--always', '--pretty', '-r',
+    return _output(['git-diff-tree', '-M', '--cc', '--always', '--pretty', '-r',
                     commit_id])
 
 def checkout(files = None, tree_id = None, force = False):
Catalin Marinas· May 15, 2007, 15:41 UTC · re: Yann Dirson · lore

Re: [StGIT RFC PATCH] Activate rename detection by default.

On 05/05/07, Yann Dirson <ydirson@altern.org> wrote:
> This patch uses -M when generating diffs, which then looks fine, just
> like any other git-generated diff.

I think we should only use -M for "files", "status", "show", "diff" but not for "export" and "mail", maybe by adding an extra parameter to git.diff() (or we could have this configurable or with an cmd line option). The reason is that I send patches that might not be applied with GIT.

-- 
Catalin
Karl Hasselström· May 15, 2007, 18:47 UTC · re: Catalin Marinas · lore

Re: [StGIT RFC PATCH] Activate rename detection by default.

On 2007-05-15 16:41:47 +0100, Catalin Marinas wrote:
Show 10 quoted lines
> On 05/05/07, Yann Dirson <ydirson@altern.org> wrote:
>
> > This patch uses -M when generating diffs, which then looks fine,
> > just like any other git-generated diff.
>
> I think we should only use -M for "files", "status", "show", "diff"
> but not for "export" and "mail", maybe by adding an extra parameter
> to git.diff() (or we could have this configurable or with an cmd
> line option). The reason is that I send patches that might not be
> applied with GIT.

Seems like a good idea to me: default to -M whenever the consumer is guaranteed to understand it, and have it as an option defaulting to off when we export data.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle
Yann Dirson· May 15, 2007, 21:37 UTC · re: Karl Hasselström · lore

Re: [StGIT RFC PATCH] Activate rename detection by default.

On Tue, May 15, 2007 at 08:47:59PM +0200, Karl Hasselström wrote:
Show 16 quoted lines
> On 2007-05-15 16:41:47 +0100, Catalin Marinas wrote:
> 
> > On 05/05/07, Yann Dirson <ydirson@altern.org> wrote:
> >
> > > This patch uses -M when generating diffs, which then looks fine,
> > > just like any other git-generated diff.
> >
> > I think we should only use -M for "files", "status", "show", "diff"
> > but not for "export" and "mail", maybe by adding an extra parameter
> > to git.diff() (or we could have this configurable or with an cmd
> > line option). The reason is that I send patches that might not be
> > applied with GIT.
> 
> Seems like a good idea to me: default to -M whenever the consumer is
> guaranteed to understand it, and have it as an option defaulting to
> off when we export data.

This option should probably be available through a config setting as well: we want to use it everytime we send a git or StGIT patch to the list :)

OTOH, using different conventions for different commands is guaranted to confuse more than one user. Eg, I customarily use "stg show > ../file.diff", assuming it will be readable by patch(1) - I even issued today more than one "stg diff -r //bottom.old | (cd ../other && patch -p1)" - luckily, no rename there ;)

It makes sense to have it my default on for "files" and "status", but it breaks the output format, and thus probably some user scripts.

Maybe we should add this behaviour as off by default, tunable from config, and overridable from command-line (roughly what I suggested in https://gna.org/task/?4773, with everything off by default). Now the question would be what command-line syntax to use to be usable.

-M/--renames <> --no-renames ? -M would become useles when the default, and --no-renames is cumbersome. Also, adds 2 flags per option, we have 3 of them (-M -C --find-copies-harder), and we may want to allow other useful ones like -w and -b.

--diff-flags='-M -C' ? Can't find a non-brain-damaging way of negating that: --no-diff-flags='-C' to negate part of a stgit.diff.defaultflags='-M -C' is likely to be confusing.

Better ideas anyone ?
-- 
Yann.

← back to recent threads