[PATCH] detect conflicts between merged remote files and directories (issue29)

[Matt Mackall]

On Tue, 2012-10-23 at 10:09 -0700, Evgeniy Makeev wrote:
> # HG changeset patch
> # User Evgeniy Makeev <evgeniym at fb.com>
> # Date 1350964267 25200
> # Node ID 395b8469087e74e0ed98487217c9259e215ec479
> # Parent  d51364b318eab1871af13f30be099799c04a43d1
> detect conflicts between merged remote files and directories (issue29)
> 
> The fix checks for three kinds of conflicts:
>   1. Remote file conflicts with a local directory with identical name
>     In this case user prompted to abort the merge (default) or to resolve
>     the conflict by deleting the remote file
>   2. Remote directory conflicts with a local file with the same name
>     In this case user prompted to abort (default) or to resolve the conflict
>     by deleting all remote files which have the directory in their paths
>   3. Remote file conflicts with local empty directory
>     This conflict is caught only during applyupdates when the IO already
>     in progress. User prompted to resolve conflict by removing the empty
>     local directory (default) or abort the merge

On the whole, this is a nice piece of work, but not yet in a form we can
accept. Seems like now is a good time to point you at:

http://mercurial.selenic.com/wiki/OneChangePerPatch

So, yes, I did mention there were case issues here. But, no, that was
not a request that you send a very large patch that tried to fixed
everything at once.

You should really structure this as:

patch 1: fix basic directory/file collision issues with a test
patch 2: add groundwork for doing case-insensitive version of the above
patch 3: add case-insensitive support in merge with corresponding test

(and possibly more parts)

Also, you should probably hold off on sending or even working on 2 and 3
further until 1 is accepted, which may take a couple round trips still.
But I will comment a bit on all three parts below anyway.

> diff -r d51364b318ea -r 395b8469087e mercurial/context.py
> --- a/mercurial/context.py	Thu Jul 26 21:29:39 2012 +0200
> +++ b/mercurial/context.py	Mon Oct 22 20:51:07 2012 -0700
> @@ -352,6 +352,23 @@
>      def dirs(self):
>          return self._dirs
>  
> +    @propertycache
> +    def _normdirs(self):

One of these methods wants a docstring.

> +        dirs = set()
> +        for f in self._manifest:
> +            pos = f.rfind('/')
> +            while pos != -1:
> +                f = f[:pos]
> +                fnorm = util.normcase(f)
> +                if fnorm in dirs:
> +                    break # dirs already contains this and above
> +                dirs.add(fnorm)
> +                pos = f.rfind('/')
> +        return dirs
> +
> +    def normdirs(self):
> +        return self._normdirs

..and all of this should go in patch #1 of N.

>  class filectx(object):
>      """A filecontext object makes access to data related to a particular
>         filerevision convenient."""
> diff -r d51364b318ea -r 395b8469087e mercurial/merge.py
> --- a/mercurial/merge.py	Thu Jul 26 21:29:39 2012 +0200
> +++ b/mercurial/merge.py	Mon Oct 22 20:51:07 2012 -0700
> @@ -190,6 +190,31 @@
>      def act(msg, m, f, *args):
>          repo.ui.debug(" %s: %s -> %s\n" % (f, msg, m))
>          action.append((f, m) + args)
> +    
> +    def addremotedir(f):
> +        # find all dirs from f's path
> +        pathpos = f.rfind('/')
> +        while pathpos != -1:
> +            f = f[:pathpos]
> +            if f in newdirset:
> +                break # newdirset already contains this and above
> +            newdirset.add(f)
> +            pathpos = f.rfind('/')

Initialize newdirset immediately above this, please.

> +    if util.checkcase(repo.path):

This is a bit unfortunate on two counts:

a) we already call checkcase and cache its result for future use in
several places
b) we'd like to minimize the the number of places that are directly
exposed to case-awareness.

> +        def findirs(f):
> +            return f in p1.dirs()
> +        def addnormdir(fdir):
> +            addremotedir(fdir)
> +        def finnewdirs(f):
> +            return f in newdirset
> +    else:
> +        def findirs(f):
> +            return util.normcase(f) in p1.normdirs()
> +        def addnormdir(fdir):
> +            addremotedir(util.normcase(fdir))
> +        def finnewdirs(f):
> +            return util.normcase(f) in newdirset

 
>      action, copy = [], {}
>  
> @@ -259,6 +284,7 @@
>              else:
>                  act("other deleted", "r", f)
>  
> +    newdirset = set()
>      for f, n in m2.iteritems():
>          if partial and not partial(f):
>              continue
> @@ -282,7 +308,20 @@
>                  act("remote differs from untracked local",
>                      "m", f, f, f, rflags, False)
>              else:
> -                act("remote created", "g", f, m2.flags(f))
> +                if findirs(f) and os.path.isdir(repo.wjoin(f)):
> +                    # check if the added remote file collides with existing dir
> +                    if 0 == repo.ui.promptchoice(
> +                            _(" remote %s conflicts with local directory %s\n"
> +                              "(a)bort operation or (d)elete remote file? [a]")
> +                                                 % (f, repo.wjoin(f)),

Here I'll refer you back to

http://markmail.org/message/ult635knza7kr45l

where I

a) suggested you not include an abort option
b) asked you to add an explicit choice between a file and a directory
c) gave you suggested wording

If you don't like my suggested wording, fine, but it's highly
inefficient to "argue by large patch". Better to just use normal email.

-- 
Mathematics is the supreme nostalgia of our time.