Patchwork [2,of,2,v2,sparse-ext,maybe-for-stable] sparse: treat paths as cwd-relative

login
register
mail settings
Submitter via Mercurial-devel
Date July 29, 2017, 9:36 p.m.
Message ID <CAESOdVApkgry+aDnaRWsrNe6Y+bO+VAnZ-E6K-b0DuFLtdwmpw@mail.gmail.com>
Download mbox | patch
Permalink /patch/22583/
State Not Applicable
Headers show

Comments

via Mercurial-devel - July 29, 2017, 9:36 p.m.
On Jul 29, 2017 1:32 PM, "Kostia Balytskyi" <ikostia@fb.com> wrote:

# HG changeset patch
# User Kostia Balytskyi <ikostia@fb.com>
# Date 1501360312 25200
#      Sat Jul 29 13:31:52 2017 -0700
# Branch stable
# Node ID d596185be467a35f5d915d63f94c3fead982c1c5
# Parent  c9a99abd647235c4dfd167ffcb9fccd66f0e0e4b
sparse: treat paths as cwd-relative

This commit makes it so sparse treats passed paths as CWD-relative,
not repo-root-realive. This is a more intuitive behavior in my (and some
other FB people's) opinion.

This is breaking change however. My hope here is that since sparse is
experimental, it's ok to introduce BCs.


I'd rather wait until after the freeze.

That is also the reason why I am
sending this during a freeze: the experimental status of the feature
probably
means that these two patches can just land on stable.


Interesting thought. I think I'd prefer to not make big changes even to
experimental features during the freeze. At least not generally. Maybe we
could consider it on a case-by-case basis.


The reason (glob)s are needed in the test is this: in these two cases we
do not supply path together with slashes, but `os.path.join` adds them,
which
means that under Windows they can be backslashes. To demonstrate this
behavior,
one could remove the (glob)s and run `./run-tests.py test-sparse.t` from
MinGW's terminal on Windows.

+
 Verify commiting while sparse includes other files

   $ echo z > hide
Gregory Szorc - July 30, 2017, 5:50 p.m.
On Sat, Jul 29, 2017 at 2:36 PM, Martin von Zweigbergk via Mercurial-devel <
mercurial-devel@mercurial-scm.org> wrote:

>
>
> On Jul 29, 2017 1:32 PM, "Kostia Balytskyi" <ikostia@fb.com> wrote:
>
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1501360312 25200
> #      Sat Jul 29 13:31:52 2017 -0700
> # Branch stable
> # Node ID d596185be467a35f5d915d63f94c3fead982c1c5
> # Parent  c9a99abd647235c4dfd167ffcb9fccd66f0e0e4b
> sparse: treat paths as cwd-relative
>
> This commit makes it so sparse treats passed paths as CWD-relative,
> not repo-root-realive. This is a more intuitive behavior in my (and some
> other FB people's) opinion.
>
> This is breaking change however. My hope here is that since sparse is
> experimental, it's ok to introduce BCs.
>
>
> I'd rather wait until after the freeze.
>
> That is also the reason why I am
> sending this during a freeze: the experimental status of the feature
> probably
> means that these two patches can just land on stable.
>
>
> Interesting thought. I think I'd prefer to not make big changes even to
> experimental features during the freeze. At least not generally. Maybe we
> could consider it on a case-by-case basis.
>

This affects the user-visible interface to sparse, which I consider to be
the most experimental aspect of the sparse feature from the perspective of
core Mercurial. On one hand, I could be convinced to queue. On the other,
the user-facing bits will continue to undergo many BC changes before sparse
is non-experimental. So it's not like beating the freeze accomplishes much
since it's all going to change anyway.


>
>
> The reason (glob)s are needed in the test is this: in these two cases we
> do not supply path together with slashes, but `os.path.join` adds them,
> which
> means that under Windows they can be backslashes. To demonstrate this
> behavior,
> one could remove the (glob)s and run `./run-tests.py test-sparse.t` from
> MinGW's terminal on Windows.
>
> diff --git a/hgext/sparse.py b/hgext/sparse.py
> --- a/hgext/sparse.py
> +++ b/hgext/sparse.py
> @@ -155,7 +155,8 @@ def _clonesparsecmd(orig, ui, repo, *arg
>      if include or exclude or enableprofile:
>          def clonesparse(orig, self, node, overwrite, *args, **kwargs):
>              sparse.updateconfig(self.unfiltered(), pat, {},
> include=include,
> -                                exclude=exclude,
> enableprofile=enableprofile)
> +                                exclude=exclude,
> enableprofile=enableprofile,
> +                                usereporootpaths=True)
>              return orig(self, node, overwrite, *args, **kwargs)
>          extensions.wrapfunction(hg, 'updaterepo', clonesparse)
>      return orig(ui, repo, *args, **opts)
> diff --git a/mercurial/sparse.py b/mercurial/sparse.py
> --- a/mercurial/sparse.py
> +++ b/mercurial/sparse.py
> @@ -616,7 +616,7 @@ def importfromfiles(repo, opts, paths, f
>
>  def updateconfig(repo, pats, opts, include=False, exclude=False,
> reset=False,
>                   delete=False, enableprofile=False, disableprofile=False,
> -                 force=False):
> +                 force=False, usereporootpaths=False):
>      """Perform a sparse config update.
>
>      Only one of the actions may be performed.
> @@ -639,6 +639,11 @@ def updateconfig(repo, pats, opts, inclu
>          if any(os.path.isabs(pat) for pat in pats):
>              raise error.Abort(_('paths cannot be absolute'))
>
> +        if not usereporootpaths:
> +            # let's treat paths as relative to cwd
> +            cwd = repo.getcwd()
> +            pats = [os.path.join(cwd, pat) for pat in pats]
>
>
> I still think this is too naive. Add a test for "glob:*.ext" and another
> one for "path:.". I don't think those will do what most people would expect.
>
> +
>          if include:
>              newinclude.update(pats)
>          elif exclude:
> diff --git a/tests/test-sparse.t b/tests/test-sparse.t
> --- a/tests/test-sparse.t
> +++ b/tests/test-sparse.t
> @@ -48,6 +48,27 @@ TODO: See if this can be made to fail th
>    [255]
>  #endif
>
> +Paths should be treated as cwd-relative, not repo-root-relative
> +  $ mkdir subdir && cd subdir
> +  $ hg debugsparse --include path
> +  $ hg debugsparse
> +  [include]
> +  $TESTTMP/myrepo/hide
> +  hide
> +  subdir/path (glob)
> +
> +  $ cd ..
> +  $ echo hello > subdir/file2.ext
> +  $ hg debugsparse --include subdir/*.ext  # let us test globs
>
>
> Wasn't this supposed to test "--include '*.ext'" from within subdir?
>
> +  $ hg debugsparse
> +  [include]
> +  $TESTTMP/myrepo/hide
> +  hide
> +  subdir/file2.ext
> +  subdir/path (glob)
> +
> +  $ rm -rf subdir
> +
>  Verify commiting while sparse includes other files
>
>    $ echo z > hide
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>

Patch

diff --git a/hgext/sparse.py b/hgext/sparse.py
--- a/hgext/sparse.py
+++ b/hgext/sparse.py
@@ -155,7 +155,8 @@  def _clonesparsecmd(orig, ui, repo, *arg
     if include or exclude or enableprofile:
         def clonesparse(orig, self, node, overwrite, *args, **kwargs):
             sparse.updateconfig(self.unfiltered(), pat, {},
include=include,
-                                exclude=exclude,
enableprofile=enableprofile)
+                                exclude=exclude,
enableprofile=enableprofile,
+                                usereporootpaths=True)
             return orig(self, node, overwrite, *args, **kwargs)
         extensions.wrapfunction(hg, 'updaterepo', clonesparse)
     return orig(ui, repo, *args, **opts)
diff --git a/mercurial/sparse.py b/mercurial/sparse.py
--- a/mercurial/sparse.py
+++ b/mercurial/sparse.py
@@ -616,7 +616,7 @@  def importfromfiles(repo, opts, paths, f

 def updateconfig(repo, pats, opts, include=False, exclude=False,
reset=False,
                  delete=False, enableprofile=False, disableprofile=False,
-                 force=False):
+                 force=False, usereporootpaths=False):
     """Perform a sparse config update.

     Only one of the actions may be performed.
@@ -639,6 +639,11 @@  def updateconfig(repo, pats, opts, inclu
         if any(os.path.isabs(pat) for pat in pats):
             raise error.Abort(_('paths cannot be absolute'))

+        if not usereporootpaths:
+            # let's treat paths as relative to cwd
+            cwd = repo.getcwd()
+            pats = [os.path.join(cwd, pat) for pat in pats]


I still think this is too naive. Add a test for "glob:*.ext" and another
one for "path:.". I don't think those will do what most people would expect.

+
         if include:
             newinclude.update(pats)
         elif exclude:
diff --git a/tests/test-sparse.t b/tests/test-sparse.t
--- a/tests/test-sparse.t
+++ b/tests/test-sparse.t
@@ -48,6 +48,27 @@  TODO: See if this can be made to fail th
   [255]
 #endif

+Paths should be treated as cwd-relative, not repo-root-relative
+  $ mkdir subdir && cd subdir
+  $ hg debugsparse --include path
+  $ hg debugsparse
+  [include]
+  $TESTTMP/myrepo/hide
+  hide
+  subdir/path (glob)
+
+  $ cd ..
+  $ echo hello > subdir/file2.ext
+  $ hg debugsparse --include subdir/*.ext  # let us test globs


Wasn't this supposed to test "--include '*.ext'" from within subdir?

+  $ hg debugsparse
+  [include]
+  $TESTTMP/myrepo/hide
+  hide
+  subdir/file2.ext
+  subdir/path (glob)
+
+  $ rm -rf subdir