[Svnmerge] [PATCH] preserving mods to renamed files/directories

[Giovanni Bajo]

On 10/07/2007 5.57, lsuvkne at onemodel.org wrote:

> This relates to preserving file and  revision log comments changes
> across a merge that includes renames.  This one is harder to break up
> into smaller units, I think.
> 
> I have applied almost all of the suggested changes that Dustin kindly
> provided in an earlier review; those not already discussed are,
> below.
> 
> The heart of the changes (and the likely easiest place to start reading
> this, IMO) is in action_merge(), where just before the script calls
> this:
>     svn_command("merge --force -r
> ..I added a call to check_for_changes_to_preserve_across_merge(). This
> function, and the functions it depends on, determine from "merge
> --dry-run" output whether there are any matching "add/delete" pairs of
> statements (representing renames) about to be applied, and for each of
> those renames, determines from logs what file edits (diffs) and comments
> would be lost due to the merge's deleting that file and replacing it. It
> saves the diffs to temporary patch files, accumulates the comments, and
> returns  the information back to action_merge().  Later in
> action_merge(), for each file where  edits were lost by the merge, it
> applies the proper patch, and appends the otherwise "lost" comments to
> the file svnmerge-commit-message.txt.

I'm -1 on this patch, because it adds a *lot* of unnecessary complication in 
svnmerge.py, just to workaround a bug (or missing feature) in svn.

I think there is a right place for every feature, and svnmerge.py isn't just 
the right place for this one. I can't but agree with Daniel Rall that any 
effort in this direction should be aimed at issue #2685, rather than kludging 
svnmerge.py.

Really, it's not that I'm against having the bug fixed: it hurts me many 
times! It's just that to fix the bug, you need to fix issue #2685, not 
svnmerge.py.

I might have considered such a feature if the patch was really small. But the 
patch is utterly complicated and filled with sub-kludges.

For instance, I'm -1 on adding an external dependency like that you proposed 
for GNU patch. svnmerge.py is nice because it's a single file with depends on 
nothing but Python (and a very old version of it). Please, keep this in mind 
as a guidance for your future patches.

Moreover, I find your variables with names longer than 30 variables totally 
unreadable. If you need 30 characters to describe a variable, it's really too 
complicated. Try doing it in a simpler way. Or maybe it doesn't need a 
self-describing name. Just use a single letter and it would do as well with a 
comment describing it -- but at least the actual algorithm would stand out 
from the code instead of being buried between 30-chars identifier. As a 
pseudo-example, try renaming:

   for currentRevisionInThisForLoop in 
revisionsWeNeedToExamineToFindTheCommonAncestor:

into:

   # Look for the common ancestor
   for r in revs:

-- 
Giovanni Bajo