You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2010/02/02 16:16:06 UTC
two questions (and a proposed patch) regarding svn:ignore
Hi,
the book says:
Subversion uses the ignore patterns —- both the global
and the per-directory lists --
^^^^^^^^^^^^^^^^^^^^^^^^^^^
to determine which files should not be swept into the version
control system as part of a larger recursive addition or import operation.
http://svnbook.red-bean.com/en/1.5/svn.advanced.props.special.ignore.html
When adding a recursive directory tree, we only heed the global ignores
list. Recall that the svn:ignore property is not inherited.
Subversion is adding the directory to version control, and immediately
proceeds to read the global ignores list, and the (obviously non-existent)
svn:ignore property of the new directory
(see subversion/libsvn_client/add.c:add_dir_recursive()).
How can a user specify a pattern using an svn:ignore property deep
inside a directory tree which is to be added? Isn't this a chicken-
and-egg problem? Are people supposed to use auto-props trickery for this?
I think the book is wrong there. It should make clear that only the
global ignores list is heeded.
My second question is due to a question in #svn.
A user noticed that svn add adds files and directories which are
explicitly mentioned on the command line, even if they match an svn:ignore
pattern (I've tested with 1.5.x, 1.6.x, and trunk). Why do we have an
--no-ignores option for svn add, documented as:
--no-ignore : disregard default and svn:ignore property ignores
but the default behaviour of 'svn add path' is to add the path even
if it is listed in the svn:ignore property of its parent directory?
I am leaning towards applying the patch below to fix this problem.
But I wanted to ask before committing this, in case I'm missing
something. This bug just seems too obvious :) Maybe there is a good
reason for the current behaviour?
Thanks,
Stefan
[[[
* subversion/libsvn_client/add.c
(add): Don't add a path if its basename matches its dirname's
svn:ignore list, unless the --no-ignore option was passed.
]]]
Index: subversion/libsvn_client/add.c
===================================================================
--- subversion/libsvn_client/add.c (revision 905565)
+++ subversion/libsvn_client/add.c (working copy)
@@ -501,6 +501,20 @@ add(const char *local_abspath,
svn_node_kind_t kind;
svn_error_t *err;
+ if (! no_ignore)
+ {
+ apr_array_header_t *ignores;
+ const char *dirname;
+ const char *basename;
+
+ dirname = svn_dirent_dirname(local_abspath, pool);
+ basename = svn_dirent_basename(local_abspath, pool);
+ SVN_ERR(svn_wc_get_ignores2(&ignores, ctx->wc_ctx, dirname,
+ ctx->config, pool, pool));
+ if (svn_wc_match_ignore_list(basename, ignores, pool))
+ return SVN_NO_ERROR;
+ }
+
SVN_ERR(svn_io_check_path(local_abspath, &kind, pool));
if (kind == svn_node_dir)
{
Re: two questions (and a proposed patch) regarding svn:ignore
Posted by 'Stefan Sperling' <st...@elego.de>.
On Tue, Feb 02, 2010 at 06:52:46PM +0100, Bert Huijben wrote:
> -1 on applying this change without revving the apis and making the
> compatibility wrappers 100% compatible with the original behavior.
> (Maybe the change can be in an 1.7 version of the api if the api was already converted).
>
The original behaviour isn't documented.
We can keep it for backward compat if you like, but we've
fixed API promises which aren't being help before without
revving the function.
> This patch by itself will break many third party subversion clients
> and because the only way to see the change is the notification... it
> will most likely not be caught by many unit tests.
Right now, I'm trying to verify the rationale for the patch,
not whether the tests pass with it. Before committing it I will
run and adjust tests of course.
> Changes of 'svn' specific client behavior should be coded in 'svn'...
> not in libsvn_client. (or the apis must be revved following our
> versioning guarantees)
This is not specific to the 'svn' client.
The docco for svn_client_add4() says:
* If @a no_ignore is FALSE, don't add files or directories that match
* ignore patterns.
That promise isn't being held.
Stefan
RE: two questions (and a proposed patch) regarding svn:ignore
Posted by Bert Huijben <be...@vmoo.com>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@elego.de]
> Sent: dinsdag 2 februari 2010 17:16
> To: dev@subversion.apache.org
> Subject: two questions (and a proposed patch) regarding svn:ignore
>
> Hi,
>
> the book says:
>
> Subversion uses the ignore patterns —- both the global
> and the per-directory lists --
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> to determine which files should not be swept into the version
> control system as part of a larger recursive addition or import operation.
> http://svnbook.red-
> bean.com/en/1.5/svn.advanced.props.special.ignore.html
>
> When adding a recursive directory tree, we only heed the global ignores list.
> Recall that the svn:ignore property is not inherited.
> Subversion is adding the directory to version control, and immediately
> proceeds to read the global ignores list, and the (obviously non-existent)
> svn:ignore property of the new directory (see
> subversion/libsvn_client/add.c:add_dir_recursive()).
> How can a user specify a pattern using an svn:ignore property deep inside a
> directory tree which is to be added? Isn't this a chicken- and-egg problem?
> Are people supposed to use auto-props trickery for this?
> I think the book is wrong there. It should make clear that only the global
> ignores list is heeded.
>
> My second question is due to a question in #svn.
> A user noticed that svn add adds files and directories which are explicitly
> mentioned on the command line, even if they match an svn:ignore pattern
> (I've tested with 1.5.x, 1.6.x, and trunk). Why do we have an --no-ignores
> option for svn add, documented as:
>
> --no-ignore : disregard default and svn:ignore property ignores
>
> but the default behaviour of 'svn add path' is to add the path even if it is
> listed in the svn:ignore property of its parent directory?
> I am leaning towards applying the patch below to fix this problem.
>
> But I wanted to ask before committing this, in case I'm missing something.
> This bug just seems too obvious :) Maybe there is a good reason for the
> current behaviour?
>
> Thanks,
> Stefan
>
> [[[
> * subversion/libsvn_client/add.c
> (add): Don't add a path if its basename matches its dirname's
> svn:ignore list, unless the --no-ignore option was passed.
> ]]]
>
> Index: subversion/libsvn_client/add.c
> ==========================================================
> =========
> --- subversion/libsvn_client/add.c (revision 905565)
> +++ subversion/libsvn_client/add.c (working copy)
> @@ -501,6 +501,20 @@ add(const char *local_abspath,
> svn_node_kind_t kind;
> svn_error_t *err;
>
> + if (! no_ignore)
> + {
> + apr_array_header_t *ignores;
> + const char *dirname;
> + const char *basename;
> +
> + dirname = svn_dirent_dirname(local_abspath, pool);
> + basename = svn_dirent_basename(local_abspath, pool);
> + SVN_ERR(svn_wc_get_ignores2(&ignores, ctx->wc_ctx, dirname,
> + ctx->config, pool, pool));
> + if (svn_wc_match_ignore_list(basename, ignores, pool))
> + return SVN_NO_ERROR;
> + }
> +
> SVN_ERR(svn_io_check_path(local_abspath, &kind, pool));
> if (kind == svn_node_dir)
> {
See other mail for details:
-1 on applying this change without revving the apis and making the compatibility wrappers 100% compatible with the original behavior.
(Maybe the change can be in an 1.7 version of the api if the api was already converted).
This patch by itself will break many third party subversion clients and because the only way to see the change is the notification... it will most likely not be caught by many unit tests.
Changes of 'svn' specific client behavior should be coded in 'svn'... not in libsvn_client. (or the apis must be revved following our versioning guarantees)
Bert
Re: two questions (and a proposed patch) regarding svn:ignore
Posted by "C. Michael Pilato" <cm...@collab.net>.
Peter Samuelson wrote:
> [C. Michael Pilato]
>> Peter Samuelson wrote:
>>> (Come to think of it, I bet a way to specify "all new directories" in
>>> auto-props would be welcome too.)
>> How about "any pattern ending a slash character" meaning "applies to
>> directories"?
>
> Sure. Unix shell globs behave exactly like this (try it: 'echo */'),
> and I guess that's what auto-props were patterned on.
I've recorded this suggestion in issue #1989:
http://subversion.tigris.org/issues/show_bug.cgi?id=1989
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: two questions (and a proposed patch) regarding svn:ignore
Posted by Peter Samuelson <pe...@p12n.org>.
[C. Michael Pilato]
> Peter Samuelson wrote:
> > (Come to think of it, I bet a way to specify "all new directories" in
> > auto-props would be welcome too.)
>
> How about "any pattern ending a slash character" meaning "applies to
> directories"?
Sure. Unix shell globs behave exactly like this (try it: 'echo */'),
and I guess that's what auto-props were patterned on.
--
Peter Samuelson | org-tld!p12n!peter | http://p12n.org/
Re: two questions (and a proposed patch) regarding svn:ignore
Posted by "C. Michael Pilato" <cm...@collab.net>.
Peter Samuelson wrote:
> (Come to think of it, I bet a way to specify "all new directories" in
> auto-props would be welcome too.)
How about "any pattern ending a slash character" meaning "applies to
directories"?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: two questions (and a proposed patch) regarding svn:ignore
Posted by Peter Samuelson <pe...@p12n.org>.
Sorry to revive a thread from 3 weeks ago, but I had a thought.
These things do not happen to me every day, you know!
[Stefan Sperling]
> When adding a recursive directory tree, we only heed the global ignores
> list. Recall that the svn:ignore property is not inherited.
> Subversion is adding the directory to version control, and immediately
> proceeds to read the global ignores list, and the (obviously non-existent)
> svn:ignore property of the new directory
> (see subversion/libsvn_client/add.c:add_dir_recursive()).
This makes me wonder if 'svn add' / 'svn mkdir' should explicitly copy
svn:ignore to the child directory - by default, or with a flag or
config option. I believe this honors the spirit of what svn:ignore is
supposed to accomplish.
Pro: It is what a lot of people would expect.
Con: It is what a lot of people wouldn't expect.
I think a lot of users would welcome this functionality, though. They
do complain that svn:ignore isn't inherited. I hate to suggest yet
another config option, as relying on config options to settle design
conflicts is a sure path to madness, but it may actually be reasonable
here, given the potential utility.
(Come to think of it, I bet a way to specify "all new directories" in
auto-props would be welcome too.)
--
Peter Samuelson | org-tld!p12n!peter | http://p12n.org/
Re: two questions (and a proposed patch) regarding svn:ignore
Posted by Julian Foad <ju...@wandisco.com>.
Stefan Sperling wrote:
> On Wed, Feb 03, 2010 at 12:52:25AM +0000, Julian Foad wrote:
> > I think the rule is (not just for 'svn add', but for all svn commands
> > that do this kind of ignoring):
> >
> > a file is ignored iff
> > (it matches the svn:ignore property or the global-ignores setting)
>
> Which svn:ignore property? The one placed at the parent directory
> of the path we are adding, right?
Yup.
> > and
> > (it is unversioned)
> > and
> > (it is encountered in recursion, rather than specified explicitly)
>
> Yes, which means we have to be running 'svn add path' where path is
> a directory. Else we never recurse.
>
> > and
> > (--no-ignores is not specified)
>
> Yes, that's how I understand the current behaviour, too, and is what
> I meant to say (but didn't). Thanks for phrasing it more clearly.
>
> > > So it looks like your new "no_svn_ignores" parameter is not needed.
> >
> > Or rather, I think the original change you were considering was
> > controlling whether to ignore files that are specified explicitly as
> > well as files that are encountered in recursion.
>
> Not just files, but directories also. Let's use 'path' as a collective
> term for either.
>
> > That extension could be
> > introduced in svn_client_addN(), but as that is a single-target API the
> > caller could implement the behaviour instead by choosing whether or not
> > to call it.
>
> Good point.
>
> > The svn subcommand, which takes multiple targets, could be extended to
> > control this behaviour. However, this behaviour (of not ignoring an
> > explicit target) was designed that way on purpose. It's not necessarily
> > the perfect UI, but it's fairly sensible and easy to describe. Are you
> > sure it is important to change the UI?
>
> The current behaviour may be intended, but if so it was not documented
> properly. We need to change either the UI or the documentation.
Yup - see below.
> I find the docstring of the API severly lacking.
> It does not explain the no_ignore parameter as well as you did above.
> At the very least, we need to update the docstring. As written, it
> implies that local_abspath will never be added if it matches any
> ignores (global or from the svn:ignore property).
In r906091 I have documented (thoroughly, I think) the current behaviour
of the client add, import and status APIs.
> However, I also think that the current behaviour is harder to explain
> than the suggested new behaviour. The currently documented (but not
> implemented) behaviour is pretty straightforward -- there are no exceptions.
> If it is unversioned and matches either the global or svn:ignore lists,
> and --no-ignores is not specified, it will not be added. Full stop.
>
> We have users who have read the relevant sections of the book, but still
> don't understand why svn is adding files they don't want to be added.
> In the case I saw being reported, the user was listing them on the command
> line driven by a script or something.
>
> This is because the output of 'svn help add', as well as the book, fail to
> properly explain the current behaviour.
The output of "svn help add" doesn't say anything about ignoring files,
and I should think it ought to.
The book documents it in the "Tip" at the end of this section:
<http://svnbook.red-bean.com/nightly/en/svn.advanced.props.special.ignore.html>, and in the "global-ignores" section of <http://svnbook.red-bean.com/nightly/en/svn.advanced.confarea.html#svn.advanced.confarea.opts.config>.
The information ought to be more accessible from the reference section,
which currently says little more than the "svn help ..." output.
> They do not mention the special case where a path is listed as an explicit
> argument. Like the svn_client_add4() docco, they claim that 'svn add'
> would ignore any files which are listed in global ignores or the
> svn:ignores property (I've quoted the relevant bits at the start of this
> thread).
The bit of the book you quoted from is this, which appears just before
the "Tip" I mentioned above:
[[[
As mentioned earlier, the list of file patterns to ignore is also used
by svn add and svn import. Both of these operations involve asking
Subversion to begin managing some set of files and directories. Rather
than force the user to pick and choose which files in a tree she wishes
to start versioning, Subversion uses the ignore patterns--both the
global and the per-directory lists--to determine which files should not
be swept into the version control system as part of a larger recursive
addition or import operation. And here again, you can use the
--no-ignore option to tell Subversion ignore its ignores list and
operate on all the files and directories present.
]]]
Yes, I agree that's misleading by mentioning the per-directory lists, as
that's inapplicable for import and not usually applicable for add.
> Personally, I'm in favour of adjusting the code to match the documentation,
> keeping an old API around that emulates the current behaviour.
> Doing so would keep the feature easy to explain and understand.
I'm ambivalent about changing the behaviour. I see why that behaviour
would be useful as an option, and it would be fine if that was how it
had always been, but do not feel that making it optional would be better
than sticking with the current behaviour.
+1 to improving the help and the book.
- Julian
Re: two questions (and a proposed patch) regarding svn:ignore
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Feb 03, 2010 at 12:52:25AM +0000, Julian Foad wrote:
> > >
> > > Right now, the no_ignore parameter only disables global ignores,
> > > and svn:ignore ignores are always ignored.
> >
> > No, that's not right. I just tried it with a trunk build. I set
> > 'svn:ignore' to ignore the file named 'F', and then I try an 'svn add
> > --force' that encounters a file named 'F', without and then with
> > '--no-ignores'.
> >
> [...]
> > $ svn add --force a
> >
> > $ svn add --force --no-ignore a
> > A a/F
> > ]]]
>
> I think the rule is (not just for 'svn add', but for all svn commands
> that do this kind of ignoring):
>
> a file is ignored iff
> (it matches the svn:ignore property or the global-ignores setting)
Which svn:ignore property? The one placed at the parent directory
of the path we are adding, right?
> and
> (it is unversioned)
> and
> (it is encountered in recursion, rather than specified explicitly)
Yes, which means we have to be running 'svn add path' where path is
a directory. Else we never recurse.
> and
> (--no-ignores is not specified)
Yes, that's how I understand the current behaviour, too, and is what
I meant to say (but didn't). Thanks for phrasing it more clearly.
> > So it looks like your new "no_svn_ignores" parameter is not needed.
>
> Or rather, I think the original change you were considering was
> controlling whether to ignore files that are specified explicitly as
> well as files that are encountered in recursion.
Not just files, but directories also. Let's use 'path' as a collective
term for either.
> That extension could be
> introduced in svn_client_addN(), but as that is a single-target API the
> caller could implement the behaviour instead by choosing whether or not
> to call it.
Good point.
> The svn subcommand, which takes multiple targets, could be extended to
> control this behaviour. However, this behaviour (of not ignoring an
> explicit target) was designed that way on purpose. It's not necessarily
> the perfect UI, but it's fairly sensible and easy to describe. Are you
> sure it is important to change the UI?
The current behaviour may be intended, but if so it was not documented
properly. We need to change either the UI or the documentation.
I find the docstring of the API severly lacking.
It does not explain the no_ignore parameter as well as you did above.
At the very least, we need to update the docstring. As written, it
implies that local_abspath will never be added if it matches any
ignores (global or from the svn:ignore property).
However, I also think that the current behaviour is harder to explain
than the suggested new behaviour. The currently documented (but not
implemented) behaviour is pretty straightforward -- there are no exceptions.
If it is unversioned and matches either the global or svn:ignore lists,
and --no-ignores is not specified, it will not be added. Full stop.
We have users who have read the relevant sections of the book, but still
don't understand why svn is adding files they don't want to be added.
In the case I saw being reported, the user was listing them on the command
line driven by a script or something.
This is because the output of 'svn help add', as well as the book, fail to
properly explain the current behaviour.
They do not mention the special case where a path is listed as an explicit
argument. Like the svn_client_add4() docco, they claim that 'svn add'
would ignore any files which are listed in global ignores or the
svn:ignores property (I've quoted the relevant bits at the start of this
thread).
Personally, I'm in favour of adjusting the code to match the documentation,
keeping an old API around that emulates the current behaviour.
Doing so would keep the feature easy to explain and understand.
Stefan
Re: two questions (and a proposed patch) regarding svn:ignore
Posted by Julian Foad <ju...@wandisco.com>.
I (Julian Foad) wrote:
> On Tue, 2010-02-02 at 19:20 +0100, Stefan Sperling wrote:
> > On Tue, Feb 02, 2010 at 06:46:46PM +0100, Bert Huijben wrote:
> > > I like this change for my uses of 'svn' as '*' is handled by the
> > > shell, but I don't think the new functionality is logical at the
> > > svn_client_add() layer. In most gui clients I use, I see some svn
> > > status output containing the not added files and then these clients
> > > allow adding all or some of the files via the explicit variant.
> > >
> > > Just notifying that you skipped a few files that would have been added
> > > before, would make it very hard for api users to replicate the old
> > > behavior via the new api. (Using the notify handler to check for error
> > > conditions is not so easy for most api users)
> >
> > Right now, the no_ignore parameter only disables global ignores,
> > and svn:ignore ignores are always ignored.
>
> No, that's not right. I just tried it with a trunk build. I set
> 'svn:ignore' to ignore the file named 'F', and then I try an 'svn add
> --force' that encounters a file named 'F', without and then with
> '--no-ignores'.
>
[...]
> $ svn add --force a
>
> $ svn add --force --no-ignore a
> A a/F
> ]]]
I think the rule is (not just for 'svn add', but for all svn commands
that do this kind of ignoring):
a file is ignored iff
(it matches the svn:ignore property or the global-ignores setting)
and
(it is unversioned)
and
(it is encountered in recursion, rather than specified explicitly)
and
(--no-ignores is not specified)
> So it looks like your new "no_svn_ignores" parameter is not needed.
Or rather, I think the original change you were considering was
controlling whether to ignore files that are specified explicitly as
well as files that are encountered in recursion. That extension could be
introduced in svn_client_addN(), but as that is a single-target API the
caller could implement the behaviour instead by choosing whether or not
to call it.
The svn subcommand, which takes multiple targets, could be extended to
control this behaviour. However, this behaviour (of not ignoring an
explicit target) was designed that way on purpose. It's not necessarily
the perfect UI, but it's fairly sensible and easy to describe. Are you
sure it is important to change the UI?
- Julian
> If you find it does what you said in a different scenario (e.g. if the
> svn:ignore property is committed versus uncommitted) then clearly that
> would be a bug.
>
> - Julian
Re: two questions (and a proposed patch) regarding svn:ignore
Posted by Julian Foad <ju...@wandisco.com>.
On Tue, 2010-02-02 at 19:20 +0100, Stefan Sperling wrote:
> On Tue, Feb 02, 2010 at 06:46:46PM +0100, Bert Huijben wrote:
> > I like this change for my uses of 'svn' as '*' is handled by the
> > shell, but I don't think the new functionality is logical at the
> > svn_client_add() layer. In most gui clients I use, I see some svn
> > status output containing the not added files and then these clients
> > allow adding all or some of the files via the explicit variant.
> >
> > Just notifying that you skipped a few files that would have been added
> > before, would make it very hard for api users to replicate the old
> > behavior via the new api. (Using the notify handler to check for error
> > conditions is not so easy for most api users)
>
> Right now, the no_ignore parameter only disables global ignores,
> and svn:ignore ignores are always ignored.
No, that's not right. I just tried it with a trunk build. I set
'svn:ignore' to ignore the file named 'F', and then I try an 'svn add
--force' that encounters a file named 'F', without and then with
'--no-ignores'.
[[[
$ svn mkdir --parents a/b/c
A a
A a/b
A a/b/c
$ echo hello > a/F
$ echo hello > a/b/G
$ svn ps svn:ignore F a a/b a/b/c
property 'svn:ignore' set on 'a'
property 'svn:ignore' set on 'a/b'
property 'svn:ignore' set on 'a/b/c'
$ svn add --force a
A a/b/G
$ svn add --force a
$ svn add --force --no-ignore a
A a/F
]]]
So it looks like your new "no_svn_ignores" parameter is not needed.
If you find it does what you said in a different scenario (e.g. if the
svn:ignore property is committed versus uncommitted) then clearly that
would be a bug.
- Julian
RE: two questions (and a proposed patch) regarding svn:ignore
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Julian Foad [mailto:julian.foad@wandisco.com]
> Sent: woensdag 3 februari 2010 12:30
> To: Bert Huijben
> Cc: 'Stefan Sperling'; 'Bert Huijben'; 'Subversion Development'
> Subject: RE: two questions (and a proposed patch) regarding svn:ignore
>
> Bert Huijben wrote:
> > > -----Original Message-----
> > > From: Julian Foad [mailto:julianfoad@btopenworld.com]
> > > While we're revving it, consider this API design point. We pass the list
> > > of global ignores in to the function through the CTX parameter. It seems
> > > stupid to have an additional parameter that says whether to ignore the
> > > global ignores. Instead, the caller should set
> > > CTX->config['config:miscellany:global-ignores'] to empty before calling
> > > this function.
> > >
> > > Makes sense?
> >
> > >From 'svn' it makes sense to move the setting to the configuration as
> > 'svn' use a new 'svn_client_ctx_t' for every command.
> >
> > But, in several other clients the svn_client_ctx_t is a long living
> > object. (This is becoming more useful in WC-NG as it will keep the
> > handle to the wc.db instance open for you over multiple commands).
> >
> > This would require setting the setting before executing the command
> > and then resetting it later in the compatibility wrapper (and in other
> > libraries that don't want to change their api incompatible).
>
> Yes, it would. I nearly wrote that out in my original email. Do you
> think that's so awkward that the API should continue to have a special
> flag for overriding that part of the config?
We duplicate a lot of these settings in more and more places.
I don't have a huge issue with it as I will probably add it to my wrapper library and be done with it. (All functions in my wrapper have the required arguments as direct function argument and all the rest on a command specific args object that can be extended without breaking the ABI).
But it also makes us update a hashtable in a +- global pool for setting and resetting and I don't know how that influences memory usage of the hashtable and the data stored inside.
(Another minor issue with this approach is that we usually only test our compatibility wrappers only once... and we won't know that we broke them until a third party client starts complaining. Keeping the wrappers simple reduces the number of possible errors)
Bert
RE: two questions (and a proposed patch) regarding svn:ignore
Posted by Julian Foad <ju...@wandisco.com>.
Bert Huijben wrote:
> > -----Original Message-----
> > From: Julian Foad [mailto:julianfoad@btopenworld.com]
> > While we're revving it, consider this API design point. We pass the list
> > of global ignores in to the function through the CTX parameter. It seems
> > stupid to have an additional parameter that says whether to ignore the
> > global ignores. Instead, the caller should set
> > CTX->config['config:miscellany:global-ignores'] to empty before calling
> > this function.
> >
> > Makes sense?
>
> >From 'svn' it makes sense to move the setting to the configuration as
> 'svn' use a new 'svn_client_ctx_t' for every command.
>
> But, in several other clients the svn_client_ctx_t is a long living
> object. (This is becoming more useful in WC-NG as it will keep the
> handle to the wc.db instance open for you over multiple commands).
>
> This would require setting the setting before executing the command
> and then resetting it later in the compatibility wrapper (and in other
> libraries that don't want to change their api incompatible).
Yes, it would. I nearly wrote that out in my original email. Do you
think that's so awkward that the API should continue to have a special
flag for overriding that part of the config?
- Julian
RE: two questions (and a proposed patch) regarding svn:ignore
Posted by Julian Foad <ju...@wandisco.com>.
Bert Huijben wrote:
> > -----Original Message-----
> > From: Julian Foad [mailto:julianfoad@btopenworld.com]
> > While we're revving it, consider this API design point. We pass the list
> > of global ignores in to the function through the CTX parameter. It seems
> > stupid to have an additional parameter that says whether to ignore the
> > global ignores. Instead, the caller should set
> > CTX->config['config:miscellany:global-ignores'] to empty before calling
> > this function.
> >
> > Makes sense?
>
> >From 'svn' it makes sense to move the setting to the configuration as
> 'svn' use a new 'svn_client_ctx_t' for every command.
>
> But, in several other clients the svn_client_ctx_t is a long living
> object. (This is becoming more useful in WC-NG as it will keep the
> handle to the wc.db instance open for you over multiple commands).
>
> This would require setting the setting before executing the command
> and then resetting it later in the compatibility wrapper (and in other
> libraries that don't want to change their api incompatible).
Yes, it would. I nearly wrote that out in my original email. Do you
think that's so awkward that the API should continue to have a special
flag for overriding that part of the config?
- Julian
RE: two questions (and a proposed patch) regarding svn:ignore
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
> Sent: woensdag 3 februari 2010 1:11
> To: 'Stefan Sperling'
> Cc: Bert Huijben; 'Subversion Development'
> Subject: Re: two questions (and a proposed patch) regarding svn:ignore
>
> On Tue, 2010-02-02, 'Stefan Sperling' wrote:
> > On Tue, Feb 02, 2010 at 09:12:42PM +0100, Bert Huijben wrote:
> > > In this libsvn_client_addX() case it would be adding an extra boolean to
> > > pass to the new svn_client_addX() function... And passing the right value
> > > from the wrapper in deprecated.c.
> >
> > Right, no problem. So we add svn_client_add5, with boolean
> > 'no_global_ignores' and 'no_svn_ignores' (better name anyone?).
>
> While we're revving it, consider this API design point. We pass the list
> of global ignores in to the function through the CTX parameter. It seems
> stupid to have an additional parameter that says whether to ignore the
> global ignores. Instead, the caller should set
> CTX->config['config:miscellany:global-ignores'] to empty before calling
> this function.
>
> Makes sense?
Re: two questions (and a proposed patch) regarding svn:ignore
Posted by 'Stefan Sperling' <st...@elego.de>.
On Wed, Feb 03, 2010 at 12:11:09AM +0000, Julian Foad wrote:
> On Tue, 2010-02-02, 'Stefan Sperling' wrote:
> > On Tue, Feb 02, 2010 at 09:12:42PM +0100, Bert Huijben wrote:
> > > In this libsvn_client_addX() case it would be adding an extra boolean to
> > > pass to the new svn_client_addX() function... And passing the right value
> > > from the wrapper in deprecated.c.
> >
> > Right, no problem. So we add svn_client_add5, with boolean
> > 'no_global_ignores' and 'no_svn_ignores' (better name anyone?).
>
> While we're revving it, consider this API design point. We pass the list
> of global ignores in to the function through the CTX parameter. It seems
> stupid to have an additional parameter that says whether to ignore the
> global ignores. Instead, the caller should set
> CTX->config['config:miscellany:global-ignores'] to empty before calling
> this function.
>
> Makes sense?
Yes, it does, thanks for the suggestion.
Stefan
RE: two questions (and a proposed patch) regarding svn:ignore
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
> Sent: woensdag 3 februari 2010 1:11
> To: 'Stefan Sperling'
> Cc: Bert Huijben; 'Subversion Development'
> Subject: Re: two questions (and a proposed patch) regarding svn:ignore
>
> On Tue, 2010-02-02, 'Stefan Sperling' wrote:
> > On Tue, Feb 02, 2010 at 09:12:42PM +0100, Bert Huijben wrote:
> > > In this libsvn_client_addX() case it would be adding an extra boolean to
> > > pass to the new svn_client_addX() function... And passing the right value
> > > from the wrapper in deprecated.c.
> >
> > Right, no problem. So we add svn_client_add5, with boolean
> > 'no_global_ignores' and 'no_svn_ignores' (better name anyone?).
>
> While we're revving it, consider this API design point. We pass the list
> of global ignores in to the function through the CTX parameter. It seems
> stupid to have an additional parameter that says whether to ignore the
> global ignores. Instead, the caller should set
> CTX->config['config:miscellany:global-ignores'] to empty before calling
> this function.
>
> Makes sense?
>From 'svn' it makes sense to move the setting to the configuration as 'svn' use a new 'svn_client_ctx_t' for every command.
But, in several other clients the svn_client_ctx_t is a long living object. (This is becoming more useful in WC-NG as it will keep the handle to the wc.db instance open for you over multiple commands).
This would require setting the setting before executing the command and then resetting it later in the compatibility wrapper (and in other libraries that don't want to change their api incompatible).
Bert
Re: two questions (and a proposed patch) regarding svn:ignore
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2010-02-02, 'Stefan Sperling' wrote:
> On Tue, Feb 02, 2010 at 09:12:42PM +0100, Bert Huijben wrote:
> > In this libsvn_client_addX() case it would be adding an extra boolean to
> > pass to the new svn_client_addX() function... And passing the right value
> > from the wrapper in deprecated.c.
>
> Right, no problem. So we add svn_client_add5, with boolean
> 'no_global_ignores' and 'no_svn_ignores' (better name anyone?).
While we're revving it, consider this API design point. We pass the list
of global ignores in to the function through the CTX parameter. It seems
stupid to have an additional parameter that says whether to ignore the
global ignores. Instead, the caller should set
CTX->config['config:miscellany:global-ignores'] to empty before calling
this function.
Makes sense?
The caller would look something like this (untested):
[[[
Index: subversion/svn/add-cmd.c
===================================================================
--- subversion/svn/add-cmd.c (revision 905836)
+++ subversion/svn/add-cmd.c (working copy)
@@ -66,6 +66,15 @@
SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+ if (opt_state->no_ignore)
+ {
+ svn_config_t *config = apr_hash_get(ctx->config, "config",
+ APR_HASH_KEY_STRING);
+
+ if (config)
+ svn_config_set(config, "miscellany", "global-ignores", NULL);
+ }
+
subpool = svn_pool_create(pool);
for (i = 0; i < targets->nelts; i++)
{
]]]
(The call to svn_client_add5() would then pass just the new
"no_svn_ignores" parameter that you suggest.)
It could be argued that it's "easier" for a caller to pass a
"no-ignores" flag that overrides part of the config, but the same could
be said about all parts of the config and that would be silly. If we
want to use separate parameters, let's just use separate parameters. If
we want to pass a config structure, let's use it.
- Julian
> The old API would be interpreted as always passing no_svn_ignores = FALSE,
> and setting no_global_ignores based on svn_client_add4's no_ignore
> parameter. Would that be OK?
>
> Stefan
Re: two questions (and a proposed patch) regarding svn:ignore
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2010-02-02, 'Stefan Sperling' wrote:
> On Tue, Feb 02, 2010 at 09:12:42PM +0100, Bert Huijben wrote:
> > In this libsvn_client_addX() case it would be adding an extra boolean to
> > pass to the new svn_client_addX() function... And passing the right value
> > from the wrapper in deprecated.c.
>
> Right, no problem. So we add svn_client_add5, with boolean
> 'no_global_ignores' and 'no_svn_ignores' (better name anyone?).
While we're revving it, consider this API design point. We pass the list
of global ignores in to the function through the CTX parameter. It seems
stupid to have an additional parameter that says whether to ignore the
global ignores. Instead, the caller should set
CTX->config['config:miscellany:global-ignores'] to empty before calling
this function.
Makes sense?
The caller would look something like this (untested):
[[[
Index: subversion/svn/add-cmd.c
===================================================================
--- subversion/svn/add-cmd.c (revision 905836)
+++ subversion/svn/add-cmd.c (working copy)
@@ -66,6 +66,15 @@
SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
+ if (opt_state->no_ignore)
+ {
+ svn_config_t *config = apr_hash_get(ctx->config, "config",
+ APR_HASH_KEY_STRING);
+
+ if (config)
+ svn_config_set(config, "miscellany", "global-ignores", NULL);
+ }
+
subpool = svn_pool_create(pool);
for (i = 0; i < targets->nelts; i++)
{
]]]
(The call to svn_client_add5() would then pass just the new
"no_svn_ignores" parameter that you suggest.)
It could be argued that it's "easier" for a caller to pass a
"no-ignores" flag that overrides part of the config, but the same could
be said about all parts of the config and that would be silly. If we
want to use separate parameters, let's just use separate parameters. If
we want to pass a config structure, let's use it.
- Julian
> The old API would be interpreted as always passing no_svn_ignores = FALSE,
> and setting no_global_ignores based on svn_client_add4's no_ignore
> parameter. Would that be OK?
>
> Stefan
RE: two questions (and a proposed patch) regarding svn:ignore
Posted by Bert Huijben <be...@vmoo.com>.
> -----Original Message-----
> From: 'Stefan Sperling' [mailto:stsp@elego.de]
> Sent: dinsdag 2 februari 2010 21:21
> To: Bert Huijben
> Cc: 'Subversion Development'
> Subject: Re: two questions (and a proposed patch) regarding svn:ignore
>
> On Tue, Feb 02, 2010 at 09:12:42PM +0100, Bert Huijben wrote:
> > Yes..
> >
> > It can also skip libsvn_client and use libsvn_wc or (for what it's
> > worth) edit the entries file directly, but that is not the point here..
>
> Heh.
>
> > We have a
> > versioning policy on our api and hundreds of existing libsvn_client
> > api users.
> >
> > An application compiled against Subversion 1.0 should still work when
> > the library it is linked to is upgraded to 1.7.0... We expect the same
> > for apr and all the libraries we use.
>
> OK, fair point. Even if the behaviour wasn't intended, we have to live
with it
> now, and keep providing it.
>
> > If you want to break these users we have to go to Subversion 2.0.
> >
> > I know about 3 documented cases where we had to break compatibility
> > for specific corner cases with libsvn_wc, but if possible we try to
> > avoid breaking existing public apis at all cost.
> >
> > In this libsvn_client_addX() case it would be adding an extra boolean
> > to pass to the new svn_client_addX() function... And passing the right
> > value from the wrapper in deprecated.c.
>
> Right, no problem. So we add svn_client_add5, with boolean
> 'no_global_ignores' and 'no_svn_ignores' (better name anyone?).
>
> The old API would be interpreted as always passing no_svn_ignores = FALSE,
> and setting no_global_ignores based on svn_client_add4's no_ignore
> parameter. Would that be OK?
+1
Not sure if it is better but something like 'skip_ignored_files',
'skip_ignores'? (Better names welcome ;-)
Bert
Re: two questions (and a proposed patch) regarding svn:ignore
Posted by 'Stefan Sperling' <st...@elego.de>.
On Tue, Feb 02, 2010 at 09:12:42PM +0100, Bert Huijben wrote:
> Yes..
>
> It can also skip libsvn_client and use libsvn_wc or (for what it's worth)
> edit the entries file directly, but that is not the point here..
Heh.
> We have a
> versioning policy on our api and hundreds of existing libsvn_client api
> users.
>
> An application compiled against Subversion 1.0 should still work when the
> library it is linked to is upgraded to 1.7.0... We expect the same for apr
> and all the libraries we use.
OK, fair point. Even if the behaviour wasn't intended, we have to
live with it now, and keep providing it.
> If you want to break these users we have to go to Subversion 2.0.
>
> I know about 3 documented cases where we had to break compatibility for
> specific corner cases with libsvn_wc, but if possible we try to avoid
> breaking existing public apis at all cost.
>
> In this libsvn_client_addX() case it would be adding an extra boolean to
> pass to the new svn_client_addX() function... And passing the right value
> from the wrapper in deprecated.c.
Right, no problem. So we add svn_client_add5, with boolean
'no_global_ignores' and 'no_svn_ignores' (better name anyone?).
The old API would be interpreted as always passing no_svn_ignores = FALSE,
and setting no_global_ignores based on svn_client_add4's no_ignore
parameter. Would that be OK?
Stefan
RE: two questions (and a proposed patch) regarding svn:ignore
Posted by Bert Huijben <be...@vmoo.com>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@elego.de]
> Sent: dinsdag 2 februari 2010 19:21
> To: Bert Huijben
> Cc: 'C. Michael Pilato'; dev@subversion.apache.org
> Subject: Re: two questions (and a proposed patch) regarding svn:ignore
>
> On Tue, Feb 02, 2010 at 06:46:46PM +0100, Bert Huijben wrote:
> > I like this change for my uses of 'svn' as '*' is handled by the
> > shell, but I don't think the new functionality is logical at the
> > svn_client_add() layer. In most gui clients I use, I see some svn
> > status output containing the not added files and then these clients
> > allow adding all or some of the files via the explicit variant.
> >
> > Just notifying that you skipped a few files that would have been added
> > before, would make it very hard for api users to replicate the old
> > behavior via the new api. (Using the notify handler to check for error
> > conditions is not so easy for most api users)
>
> Right now, the no_ignore parameter only disables global ignores, and
> svn:ignore ignores are always ignored.
>
> Could a gui client not get the ignore list for the directory, and let the
user
> toggle no_ignore for any particular file in the list, and then call
> svn_client_add4() with the appropriate value for the no_ignore parameter?
Yes..
It can also skip libsvn_client and use libsvn_wc or (for what it's worth)
edit the entries file directly, but that is not the point here.. We have a
versioning policy on our api and hundreds of existing libsvn_client api
users.
An application compiled against Subversion 1.0 should still work when the
library it is linked to is upgraded to 1.7.0... We expect the same for apr
and all the libraries we use.
If you want to break these users we have to go to Subversion 2.0.
I know about 3 documented cases where we had to break compatibility for
specific corner cases with libsvn_wc, but if possible we try to avoid
breaking existing public apis at all cost.
In this libsvn_client_addX() case it would be adding an extra boolean to
pass to the new svn_client_addX() function... And passing the right value
from the wrapper in deprecated.c.
Bert
Re: two questions (and a proposed patch) regarding svn:ignore
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 02, 2010 at 06:46:46PM +0100, Bert Huijben wrote:
> I like this change for my uses of 'svn' as '*' is handled by the
> shell, but I don't think the new functionality is logical at the
> svn_client_add() layer. In most gui clients I use, I see some svn
> status output containing the not added files and then these clients
> allow adding all or some of the files via the explicit variant.
>
> Just notifying that you skipped a few files that would have been added
> before, would make it very hard for api users to replicate the old
> behavior via the new api. (Using the notify handler to check for error
> conditions is not so easy for most api users)
Right now, the no_ignore parameter only disables global ignores,
and svn:ignore ignores are always ignored.
Could a gui client not get the ignore list for the directory,
and let the user toggle no_ignore for any particular file in the list,
and then call svn_client_add4() with the appropriate value for the
no_ignore parameter?
> Not sure if the same applies for scripts that use svn internally...
> but I would guess most batch files have a hard time making sense of
> the changing svn status output for warning conditions. (Would this
> break svnmerge.py like wrapper scripts?)
Changing output always break badly written scripts.
IMO that should not make us hesitate to improve the output.
Stefan
RE: two questions (and a proposed patch) regarding svn:ignore
Posted by Bert Huijben <be...@vmoo.com>.
> -----Original Message-----
> From: C. Michael Pilato [mailto:cmpilato@collab.net]
> Sent: dinsdag 2 februari 2010 17:37
> To: Stefan Sperling
> Cc: dev@subversion.apache.org
> Subject: Re: two questions (and a proposed patch) regarding svn:ignore
>
> Stefan Sperling wrote:
> > Hi,
> >
> > the book says:
> >
> > Subversion uses the ignore patterns —- both the global
> > and the per-directory lists --
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > to determine which files should not be swept into the version
> > control system as part of a larger recursive addition or import operation.
> > http://svnbook.red-
> bean.com/en/1.5/svn.advanced.props.special.ignore.h
> > tml
> >
> > When adding a recursive directory tree, we only heed the global
> > ignores list.
>
> I wonder if, when documenting that bit, I was thinking about the scenario
> where you are running 'svn add -R --force' atop an already versioned
> directory tree, just trying to pick up any unversioned stuff in that tree.
>
> > My second question is due to a question in #svn.
> > A user noticed that svn add adds files and directories which are
> > explicitly mentioned on the command line, even if they match an
> > svn:ignore pattern (I've tested with 1.5.x, 1.6.x, and trunk). Why do
> > we have an --no-ignores option for svn add, documented as:
> >
> > --no-ignore : disregard default and svn:ignore property ignores
> >
> > but the default behaviour of 'svn add path' is to add the path even if
> > it is listed in the svn:ignore property of its parent directory?
> > I am leaning towards applying the patch below to fix this problem.
> >
> > But I wanted to ask before committing this, in case I'm missing
> > something. This bug just seems too obvious :) Maybe there is a good
> > reason for the current behaviour?
>
> The --no-ignore option aside, I'd argue that the current behavior is more
> obvious than silently not honoring a user's request to 'svn add' something.
>
> $ svn add foo.o
> $
>
> Of course, I realize that most folks wouldn't do the above. They'd instead do
> something like:
>
> $ svn add *
>
> and would reasonably expect that stuff configured to be ignored would be
> ignored (and it isn't, right, because the wildcard is expanded by the shell).
>
> So maybe we could benefit from a pair of changes here: the one you
> included, but also some notification for 'svn add' (and 'svn import') that
> reports either explicitly-named or reached-through-recursion ignored items.
>
> $ svn add foo.o
> Ignoring 'foo.o'. ### or maybe just: "I foo.c"
> $ svn add foo.c --no-ignore
> A (bin) foo.c
> $ svn add some-dir
> A some-dir
> A some-dir/foo.c
> A some-dir/foo.h
> Ignoring 'some-dir/foo.o'
> A some-dir/lib.c
> ...
>
> Whaddaya say?
I like this change for my uses of 'svn' as '*' is handled by the shell, but I don't think the new functionality is logical at the svn_client_add() layer. In most gui clients I use, I see some svn status output containing the not added files and then these clients allow adding all or some of the files via the explicit variant.
Just notifying that you skipped a few files that would have been added before, would make it very hard for api users to replicate the old behavior via the new api. (Using the notify handler to check for error conditions is not so easy for most api users)
Not sure if the same applies for scripts that use svn internally... but I would guess most batch files have a hard time making sense of the changing svn status output for warning conditions. (Would this break svnmerge.py like wrapper scripts?)
I always thought the old behavior was taken from older/similar systems like CVS.
Bert
>
> --
> C. Michael Pilato <cm...@collab.net>
> CollabNet <> www.collab.net <> Distributed Development On Demand
Re: two questions (and a proposed patch) regarding svn:ignore
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 02, 2010 at 11:36:44AM -0500, C. Michael Pilato wrote:
> So maybe we could benefit from a pair of changes here: the one you
> included, but also some notification for 'svn add' (and 'svn import') that
> reports either explicitly-named or reached-through-recursion ignored items.
>
> $ svn add foo.o
> Ignoring 'foo.o'. ### or maybe just: "I foo.c"
> $ svn add foo.c --no-ignore
> A (bin) foo.c
> $ svn add some-dir
> A some-dir
> A some-dir/foo.c
> A some-dir/foo.h
> Ignoring 'some-dir/foo.o'
> A some-dir/lib.c
> ...
>
> Whaddaya say?
Sounds good. I'll try to hack up the notification bits.
Stefan
Re: two questions (and a proposed patch) regarding svn:ignore
Posted by "C. Michael Pilato" <cm...@collab.net>.
Stefan Sperling wrote:
> Hi,
>
> the book says:
>
> Subversion uses the ignore patterns —- both the global
> and the per-directory lists --
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> to determine which files should not be swept into the version
> control system as part of a larger recursive addition or import operation.
> http://svnbook.red-bean.com/en/1.5/svn.advanced.props.special.ignore.html
>
> When adding a recursive directory tree, we only heed the global ignores
> list.
I wonder if, when documenting that bit, I was thinking about the scenario
where you are running 'svn add -R --force' atop an already versioned
directory tree, just trying to pick up any unversioned stuff in that tree.
> My second question is due to a question in #svn.
> A user noticed that svn add adds files and directories which are
> explicitly mentioned on the command line, even if they match an svn:ignore
> pattern (I've tested with 1.5.x, 1.6.x, and trunk). Why do we have an
> --no-ignores option for svn add, documented as:
>
> --no-ignore : disregard default and svn:ignore property ignores
>
> but the default behaviour of 'svn add path' is to add the path even
> if it is listed in the svn:ignore property of its parent directory?
> I am leaning towards applying the patch below to fix this problem.
>
> But I wanted to ask before committing this, in case I'm missing
> something. This bug just seems too obvious :) Maybe there is a good
> reason for the current behaviour?
The --no-ignore option aside, I'd argue that the current behavior is more
obvious than silently not honoring a user's request to 'svn add' something.
$ svn add foo.o
$
Of course, I realize that most folks wouldn't do the above. They'd instead
do something like:
$ svn add *
and would reasonably expect that stuff configured to be ignored would be
ignored (and it isn't, right, because the wildcard is expanded by the shell).
So maybe we could benefit from a pair of changes here: the one you
included, but also some notification for 'svn add' (and 'svn import') that
reports either explicitly-named or reached-through-recursion ignored items.
$ svn add foo.o
Ignoring 'foo.o'. ### or maybe just: "I foo.c"
$ svn add foo.c --no-ignore
A (bin) foo.c
$ svn add some-dir
A some-dir
A some-dir/foo.c
A some-dir/foo.h
Ignoring 'some-dir/foo.o'
A some-dir/lib.c
...
Whaddaya say?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand