Re: [PATCH] Reject checkouts to existing directory

[Ivan Zhakov]

On 12 October 2016 at 16:28, Patrick Steinhardt
 wrote:
> Hi,
>
> attached is a patch to reject checkouts to already existing
> directories when `--force` is not given. This is according to
> `svn co --help`.
>
> [[
> Reject checkout to existing paths without force
>
> * subversion/svn/checkout-cmd.c:
>   - (svn_cl__checkout): Reject checkout to existing directory
> without --force
> ]]
>
As far I see we have four different cases of checking out to existing directory:
a. Checkout of non-empty directory to non-empty local directory
b. Checkout of non-empty directory to empty local directory
c. Checkout of empty directory to non-empty local directory
d. Checkout of empty directory to empty local directory

I think that cases (b),(c),(d) should not require '--force'. It seems
to be pretty safe operations. Also (c) is common way to 'takeover'
(version) existing repository. I meant:
1. Create empty directory in repository
2. Check it to existing directory with source code
3. Add all requested files/directories
4. Review all changes and commit.

But it makes sense for to require '--force' flag for case (a): it's
very easy mess up local changes in this situation.

-- 
Ivan Zhakov

Re: [PATCH] Reject checkouts to existing directory

[Stefan]

On 10/13/2016 2:54 PM, Stefan wrote:
> On 10/12/2016 5:00 PM, 'Stefan Sperling' wrote:
>> I'm actually surprised that we made this change in 1.7.
>>
>> If 'svn checkout' sees an existing directory the most likely situation is
>> that the user has made a mistake. Leaving behind a working copy full of
>> tree conflicts is hardly useful in this situation. I would expect this to
>> happen only with a flag such as --force but the default behaviour being
>> not touch an existing directory.
> Was looking through the mailing list but couldn't find any older
> discussion leading to this change of behavior in 1.7.
>
> From my point of view, it would be fine to change the behavior, if
> checking out into an empty directory would still work without the
> force-parameter.
> Rational: New users of svn (especially Windows users) usually create a
> folder first and then want to co into that directory (don't ask me why
> it's different for the Linux user base). If an obvious "svn co [URL]
> C:\mywc" fails without specifying the --force parameter it would
> introduce a new unnecessary hurdle for new users, IMO.

Just wanted to check out whether the help string could be improved for
1.9 (and possibly 1.8). Doing a test checkout of the subversion trunk
(svn co [url] .) into an unversioned copy of the subversion trunk I
ended up with several files having been marked as deleted (tested with
1.9.4).

Regards,
Stefan




smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH] Reject checkouts to existing directory

[Stefan]

On 10/12/2016 5:00 PM, 'Stefan Sperling' wrote:
> I'm actually surprised that we made this change in 1.7.
>
> If 'svn checkout' sees an existing directory the most likely situation is
> that the user has made a mistake. Leaving behind a working copy full of
> tree conflicts is hardly useful in this situation. I would expect this to
> happen only with a flag such as --force but the default behaviour being
> not touch an existing directory.

Was looking through the mailing list but couldn't find any older
discussion leading to this change of behavior in 1.7.

From my point of view, it would be fine to change the behavior, if
checking out into an empty directory would still work without the
force-parameter.
Rational: New users of svn (especially Windows users) usually create a
folder first and then want to co into that directory (don't ask me why
it's different for the Linux user base). If an obvious "svn co [URL]
C:\mywc" fails without specifying the --force parameter it would
introduce a new unnecessary hurdle for new users, IMO.

Regards,
Stefan




smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH] Reject checkouts to existing directory

[Patrick Steinhardt]

On Wed, Oct 12, 2016 at 04:49:55PM +0200, Bert Huijben wrote:
> 
> 
> > -Original Message-
> > From: Stefan Sperling [mailto:s...@elego.de]
> > Sent: woensdag 12 oktober 2016 16:37
> > To: Patrick Steinhardt <patrick.steinha...@elegosoft.com>
> > Cc: Subversion <dev@subversion.apache.org>
> > Subject: Re: [PATCH] Reject checkouts to existing directory
> > 
> > On Wed, Oct 12, 2016 at 04:28:05PM +0200, Patrick Steinhardt wrote:
> > > Hi,
> > >
> > > attached is a patch to reject checkouts to already existing
> > > directories when `--force` is not given. This is according to
> > > `svn co --help`.
> > >
> > > [[
> > > Reject checkout to existing paths without force
> > >
> > > * subversion/svn/checkout-cmd.c:
> > >   - (svn_cl__checkout): Reject checkout to existing directory
> > > without --force
> > > ]]
> > 
> > Nice catch.
> > It's odd that the actual behaviour and help string don't line up.
> 
> I'm not sure if the behavior you see is actually a bug. The --force flag is
> already used for processing in the update editor and there are cases where I
> would like to turn an existing directory in a checkout, while not invoking
> that behavior. (I prefer conflicts, over just assuming that every file is
> just a modified version of the files already in the directory; the current
> behavior triggered by --force)
> 
> I can see that you might want to have a warning when there is already a
> directory, but I don't think it is the right way to use the --force option
> for this.

Yeah, I'd definitly want a prompt or be required to set a flag
when checking out to an existing repository as I've already
cloned into existing directories multiple times by accident.

> And then there is the feature that svn checkout is restartable when it fails
> after some part of the checkout (just like update)... with this patch
> applied that would also stop being supported.
> 
>   Bert
> 

I guess a new flag would be the most obvious thing to do here. No
idea though what this flag should be called.
`--allow-clone-to-existing-dir` is really long,
`--force-existing` may be mistaken for requiring an existing
directory. Any ideas?

Regards
-- 
Patrick Steinhardt, Entwickler

elego Software Solutions GmbH, http://www.elego.de
Gebäude 12 (BIG), Gustav-Meyer-Allee 25, 13355 Berlin, Germany

Sitz der Gesellschaft: Berlin, USt-IdNr.: DE 163214194
Handelsregister: Amtsgericht Charlottenburg HRB 77719
Geschäftsführer: Olaf Wagner


signature.asc
Description: PGP signature

Re: [PATCH] Reject checkouts to existing directory

['Stefan Sperling']

On Wed, Oct 12, 2016 at 04:49:55PM +0200, Bert Huijben wrote:
> 
> 
> > -Original Message-
> > From: Stefan Sperling [mailto:s...@elego.de]
> > Sent: woensdag 12 oktober 2016 16:37
> > To: Patrick Steinhardt <patrick.steinha...@elegosoft.com>
> > Cc: Subversion <dev@subversion.apache.org>
> > Subject: Re: [PATCH] Reject checkouts to existing directory
> > 
> > On Wed, Oct 12, 2016 at 04:28:05PM +0200, Patrick Steinhardt wrote:
> > > Hi,
> > >
> > > attached is a patch to reject checkouts to already existing
> > > directories when `--force` is not given. This is according to
> > > `svn co --help`.
> > >
> > > [[
> > > Reject checkout to existing paths without force
> > >
> > > * subversion/svn/checkout-cmd.c:
> > >   - (svn_cl__checkout): Reject checkout to existing directory
> > > without --force
> > > ]]
> > 
> > Nice catch.
> > It's odd that the actual behaviour and help string don't line up.
> 
> I'm not sure if the behavior you see is actually a bug. The --force flag is
> already used for processing in the update editor and there are cases where I
> would like to turn an existing directory in a checkout, while not invoking
> that behavior. (I prefer conflicts, over just assuming that every file is
> just a modified version of the files already in the directory; the current
> behavior triggered by --force)
> 
> I can see that you might want to have a warning when there is already a
> directory, but I don't think it is the right way to use the --force option
> for this.
> 
> 
> And then there is the feature that svn checkout is restartable when it fails
> after some part of the checkout (just like update)... with this patch
> applied that would also stop being supported.
> 
>   Bert
> 

http://svnbook.red-bean.com/en/1.7/svn.ref.svn.c.checkout.html
[[[
Prior to version 1.7, Subversion would complain by default if you try to
check out a directory atop an existing directory which contains files or
subdirectories that the checkout itself would have created.
Subversion 1.7 handles this situation differently, allowing the checkout
to proceed but marking any obstructing objects as tree conflicts.
Use the --force option to override this safeguard.
]]]

I'm actually surprised that we made this change in 1.7.

If 'svn checkout' sees an existing directory the most likely situation is
that the user has made a mistake. Leaving behind a working copy full of
tree conflicts is hardly useful in this situation. I would expect this to
happen only with a flag such as --force but the default behaviour being
not touch an existing directory.

To allow for the case where 'svn checkout' is restarted we could check if
the existing .svn directory contains a database that matches the repository
location the checkout is reading from. And only then proceed with the update,
but otherwise behave as above.

RE: [PATCH] Reject checkouts to existing directory

[Bert Huijben]

> -Original Message-
> From: Stefan Sperling [mailto:s...@elego.de]
> Sent: woensdag 12 oktober 2016 16:37
> To: Patrick Steinhardt <patrick.steinha...@elegosoft.com>
> Cc: Subversion <dev@subversion.apache.org>
> Subject: Re: [PATCH] Reject checkouts to existing directory
> 
> On Wed, Oct 12, 2016 at 04:28:05PM +0200, Patrick Steinhardt wrote:
> > Hi,
> >
> > attached is a patch to reject checkouts to already existing
> > directories when `--force` is not given. This is according to
> > `svn co --help`.
> >
> > [[
> > Reject checkout to existing paths without force
> >
> > * subversion/svn/checkout-cmd.c:
> >   - (svn_cl__checkout): Reject checkout to existing directory
> > without --force
> > ]]
> 
> Nice catch.
> It's odd that the actual behaviour and help string don't line up.

I'm not sure if the behavior you see is actually a bug. The --force flag is
already used for processing in the update editor and there are cases where I
would like to turn an existing directory in a checkout, while not invoking
that behavior. (I prefer conflicts, over just assuming that every file is
just a modified version of the files already in the directory; the current
behavior triggered by --force)

I can see that you might want to have a warning when there is already a
directory, but I don't think it is the right way to use the --force option
for this.


And then there is the feature that svn checkout is restartable when it fails
after some part of the checkout (just like update)... with this patch
applied that would also stop being supported.

Bert

Re: [PATCH] Reject checkouts to existing directory

[Stefan Sperling]

On Wed, Oct 12, 2016 at 04:28:05PM +0200, Patrick Steinhardt wrote:
> Hi,
> 
> attached is a patch to reject checkouts to already existing
> directories when `--force` is not given. This is according to
> `svn co --help`.
> 
> [[
> Reject checkout to existing paths without force
> 
> * subversion/svn/checkout-cmd.c:
>   - (svn_cl__checkout): Reject checkout to existing directory
> without --force
> ]]

Nice catch.
It's odd that the actual behaviour and help string don't line up.

> diff --git a/subversion/svn/checkout-cmd.c b/subversion/svn/checkout-cmd.c
> index 56fd02b..5fda44a 100644
> --- a/subversion/svn/checkout-cmd.c
> +++ b/subversion/svn/checkout-cmd.c
> @@ -155,6 +155,20 @@ svn_cl__checkout(apr_getopt_t *os,
> subpool);
>  }
>  
> +  if (! opt_state->force)
> +{
> +  svn_node_kind_t kind;
> +
> +  SVN_ERR(svn_io_check_path(target_dir, , subpool));
> +
> +  if (kind != svn_node_none)
> +{
> +  return svn_error_createf
> +(SVN_ERR_ILLEGAL_TARGET, NULL,
> + _("Rejecting checkout to existing directory '%s'"), 
> target_dir);

At this poing the node kind could be svn_node_file as well.
Perhaps make it say something like "%s already exists" instead?

Does this patch pass 'make check'?

> +}
> +}
> +
>/* Checkout doesn't accept an unspecified revision, so default to
>   the peg revision, or to HEAD if there wasn't a peg. */
>if (revision.kind == svn_opt_revision_unspecified)