You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2010/03/05 16:57:24 UTC
svn commit: r919460 - in /subversion/trunk/subversion: include/svn_client.h
libsvn_client/patch.c svn/cl.h svn/main.c svn/patch-cmd.c
tests/cmdline/patch_tests.py
Author: stsp
Date: Fri Mar 5 15:57:23 2010
New Revision: 919460
URL: http://svn.apache.org/viewvc?rev=919460&view=rev
Log:
Fix issue #3434, "svn patch API should have a patch target filter"
In addition to the requirements listed in issue #3434, allow the
filter list to contain glob patterns, and make the 'svn patch' command
capable of filtering patch targets.
* subversion/include/svn_client.h
(svn_client_patch): Add FILTER_GLOBS parameter.
* subversion/libsvn_client/patch.c
(): Include apr_fnmatch.h.
(patch_target_t): New field FILTERED.
(resolve_target_path, apply_one_patch): New paramter FILTER_GLOBS.
If a target matches a glob in FILTER_GLOBS, mark it as filtered
and return immediately.
(apply_patches_baton_t): New field FILTER_GLOBS.
(apply_patches): Ignore filtered targets.
(svn_client_patch): Add FILTER_GLOBS parameter, stuff it into the
apply_patches baton.
* subversion/svn/cl.h
(svn_cl__opt_state_t): New field EXCLUDE_PATTERNS.
* subversion/svn/main.c
(svn_cl__longopt_t): New field EXCLUDE_PATTERN.
(svn_cl__options): Add --exclude-pattern option.
(svn_cl__cmd_table): Make 'svn patch' accept the --exclude-pattern option.
(main): Handle --exclude-pattern option.
* subversion/svn/patch-cmd.c
(svn_cl__patch): Pass OPT_STATE->exclude_patterns on to svn_client_patch().
* subversion/tests/cmdline/patch_tests.py
(patch_with_exclude_patterns): New test.
(test_list): Add new test.
Modified:
subversion/trunk/subversion/include/svn_client.h
subversion/trunk/subversion/libsvn_client/patch.c
subversion/trunk/subversion/svn/cl.h
subversion/trunk/subversion/svn/main.c
subversion/trunk/subversion/svn/patch-cmd.c
subversion/trunk/subversion/tests/cmdline/patch_tests.py
Modified: subversion/trunk/subversion/include/svn_client.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_client.h?rev=919460&r1=919459&r2=919460&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_client.h (original)
+++ subversion/trunk/subversion/include/svn_client.h Fri Mar 5 15:57:23 2010
@@ -4848,6 +4848,10 @@
* This is useful when applying a unidiff which was created with the
* original and modified files swapped due to human error.
*
+ * If @a filter_globs is not NULL, patch targets matching any glob
+ * pattern in @a filter_globs will not be patched. The match is performed
+ * on the target path as parsed from the patch file, after canonicalization.
+ *
* If @a ctx->notify_func2 is non-NULL, invoke @a ctx->notify_func2 with
* @a ctx->notify_baton2 as patching progresses.
*
@@ -4862,6 +4866,7 @@
svn_boolean_t dry_run,
int strip_count,
svn_boolean_t reverse,
+ apr_array_header_t *filter_globs,
svn_client_ctx_t *ctx,
apr_pool_t *pool);
Modified: subversion/trunk/subversion/libsvn_client/patch.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/patch.c?rev=919460&r1=919459&r2=919460&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/patch.c (original)
+++ subversion/trunk/subversion/libsvn_client/patch.c Fri Mar 5 15:57:23 2010
@@ -28,6 +28,7 @@
/*** Includes. ***/
#include <apr_hash.h>
+#include <apr_fnmatch.h>
#include "svn_client.h"
#include "svn_dirent_uri.h"
#include "svn_io.h"
@@ -133,6 +134,9 @@
/* True if the target had to be skipped for some reason. */
svn_boolean_t skipped;
+ /* True if the target matches a filter glob pattern. */
+ svn_boolean_t filtered;
+
/* True if at least one hunk was rejected. */
svn_boolean_t had_rejects;
@@ -321,15 +325,18 @@
* which should be stripped from target paths in the patch.
* Upon success, allocate the patch target structure in RESULT_POOL.
* Else, set *target to NULL.
+ * If a target matches a glob in FILTER_GLOBS, mark it as filtered.
* Use SCRATCH_POOL for all other allocations. */
static svn_error_t *
init_patch_target(patch_target_t **patch_target,
const svn_patch_t *patch,
const char *base_dir,
svn_wc_context_t *wc_ctx, int strip_count,
+ apr_array_header_t *filter_globs,
apr_pool_t *result_pool, apr_pool_t *scratch_pool)
{
patch_target_t *target;
+ int i;
target = apr_pcalloc(result_pool, sizeof(*target));
@@ -337,6 +344,25 @@
base_dir, strip_count, wc_ctx,
result_pool, scratch_pool));
+ target->filtered = FALSE;
+ if (filter_globs)
+ {
+ for (i = 0; i < filter_globs->nelts; i++)
+ {
+ const char *glob;
+
+ glob = APR_ARRAY_IDX(filter_globs, i, const char *);
+ target->filtered = (apr_fnmatch(glob,
+ target->canon_path_from_patchfile,
+ APR_FNM_CASE_BLIND) == APR_SUCCESS);
+ if (target->filtered)
+ {
+ *patch_target = target;
+ return SVN_NO_ERROR;
+ }
+ }
+ }
+
target->local_mods = FALSE;
target->executable = FALSE;
@@ -1029,12 +1055,13 @@
* in RESULT_POOL. Use WC_CTX as the working copy context.
* STRIP_COUNT specifies the number of leading path components
* which should be stripped from target paths in the patch.
+ * If a target matches a glob in FILTER_GLOBS, mark it as filtered.
* Do temporary allocations in SCRATCH_POOL. */
static svn_error_t *
apply_one_patch(patch_target_t **patch_target, svn_patch_t *patch,
const char *abs_wc_path, svn_wc_context_t *wc_ctx,
- int strip_count, apr_pool_t *result_pool,
- apr_pool_t *scratch_pool)
+ int strip_count, apr_array_header_t *filter_globs,
+ apr_pool_t *result_pool, apr_pool_t *scratch_pool)
{
patch_target_t *target;
apr_pool_t *iterpool;
@@ -1042,9 +1069,9 @@
static const int MAX_FUZZ = 2;
SVN_ERR(init_patch_target(&target, patch, abs_wc_path, wc_ctx, strip_count,
- result_pool, scratch_pool));
+ filter_globs, result_pool, scratch_pool));
- if (target->skipped)
+ if (target->skipped || target->filtered)
{
*patch_target = target;
return SVN_NO_ERROR;
@@ -1351,6 +1378,9 @@
/* Whether to apply the patch in reverse. */
svn_boolean_t reverse;
+ /* Glob patterns. Files matching any of these patterns won't be patched. */
+ apr_array_header_t *filter_globs;
+
/* The client context. */
svn_client_ctx_t *ctx;
} apply_patches_baton_t;
@@ -1403,8 +1433,11 @@
SVN_ERR(apply_one_patch(&target, patch, btn->abs_wc_path,
btn->ctx->wc_ctx, btn->strip_count,
- scratch_pool, iterpool));
- APR_ARRAY_PUSH(targets, patch_target_t *) = target;
+ btn->filter_globs, scratch_pool, iterpool));
+ if (target->filtered)
+ SVN_ERR(svn_diff__close_patch(patch));
+ else
+ APR_ARRAY_PUSH(targets, patch_target_t *) = target;
}
}
while (patch);
@@ -1440,6 +1473,7 @@
svn_boolean_t dry_run,
int strip_count,
svn_boolean_t reverse,
+ apr_array_header_t *filter_globs,
svn_client_ctx_t *ctx,
apr_pool_t *pool)
{
@@ -1455,6 +1489,7 @@
baton.ctx = ctx;
baton.strip_count = strip_count;
baton.reverse = reverse;
+ baton.filter_globs = filter_globs;
SVN_ERR(svn_wc__call_with_write_lock(apply_patches, &baton,
ctx->wc_ctx, local_abspath,
Modified: subversion/trunk/subversion/svn/cl.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/cl.h?rev=919460&r1=919459&r2=919460&view=diff
==============================================================================
--- subversion/trunk/subversion/svn/cl.h (original)
+++ subversion/trunk/subversion/svn/cl.h Fri Mar 5 15:57:23 2010
@@ -224,6 +224,7 @@
int strip_count; /* number of leading path components to strip */
svn_boolean_t ignore_keywords; /* do not expand keywords */
svn_boolean_t reverse_diff; /* reverse a diff (e.g. when patching) */
+ apr_array_header_t *exclude_patterns; /* targets to exclude from operation */
} svn_cl__opt_state_t;
Modified: subversion/trunk/subversion/svn/main.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/main.c?rev=919460&r1=919459&r2=919460&view=diff
==============================================================================
--- subversion/trunk/subversion/svn/main.c (original)
+++ subversion/trunk/subversion/svn/main.c Fri Mar 5 15:57:23 2010
@@ -117,6 +117,7 @@
opt_show_copies_as_adds,
opt_ignore_keywords,
opt_reverse_diff,
+ opt_exclude_pattern,
} svn_cl__longopt_t;
/* Option codes and descriptions for the command line client.
@@ -347,6 +348,16 @@
N_("apply the unidiff in reverse\n"
" "
"[alias: --rd]")},
+ {"exclude-pattern", opt_exclude_pattern, 1,
+ N_("do not operate on targets matching ARG,\n"
+ " "
+ "which may be a glob pattern such as '*.txt'.\n"
+ " "
+ "If this option is specified multiple times,\n"
+ " "
+ "all patterns are matched in turn.\n"
+ " "
+ "[alias: --ep]")},
/* Long-opt Aliases
*
* These have NULL desriptions, but an option code that matches some
@@ -371,6 +382,7 @@
{"ri", opt_reintegrate, 0, NULL},
{"sca", opt_show_copies_as_adds, 0, NULL},
{"ik", opt_ignore_keywords, 0, NULL},
+ {"ep", opt_exclude_pattern, 1, NULL},
{0, 0, 0, 0},
};
@@ -812,7 +824,7 @@
" for addition. Use 'svn revert' to undo deletions and additions you\n"
" do not agree with.\n"
),
- {'q', opt_dry_run, 'p', opt_reverse_diff} },
+ {'q', opt_dry_run, 'p', opt_reverse_diff, opt_exclude_pattern} },
{ "propdel", svn_cl__propdel, {"pdel", "pd"}, N_
("Remove a property from files, dirs, or revisions.\n"
@@ -1727,6 +1739,12 @@
case opt_reverse_diff:
opt_state.reverse_diff = TRUE;
break;
+ case opt_exclude_pattern:
+ if (opt_state.exclude_patterns == NULL)
+ opt_state.exclude_patterns = apr_array_make(pool, 1,
+ sizeof (const char *));
+ APR_ARRAY_PUSH(opt_state.exclude_patterns, const char *) = opt_arg;
+ break;
default:
/* Hmmm. Perhaps this would be a good place to squirrel away
opts that commands like svn diff might need. Hmmm indeed. */
Modified: subversion/trunk/subversion/svn/patch-cmd.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/patch-cmd.c?rev=919460&r1=919459&r2=919460&view=diff
==============================================================================
--- subversion/trunk/subversion/svn/patch-cmd.c (original)
+++ subversion/trunk/subversion/svn/patch-cmd.c Fri Mar 5 15:57:23 2010
@@ -78,7 +78,8 @@
SVN_ERR(svn_client_patch(abs_patch_path, abs_target_path,
opt_state->dry_run, opt_state->strip_count,
- opt_state->reverse_diff, ctx, pool));
+ opt_state->reverse_diff,
+ opt_state->exclude_patterns, ctx, pool));
if (! opt_state->quiet)
SVN_ERR(svn_cl__print_conflict_stats(ctx->notify_baton2, pool));
Modified: subversion/trunk/subversion/tests/cmdline/patch_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/patch_tests.py?rev=919460&r1=919459&r2=919460&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/patch_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/patch_tests.py Fri Mar 5 15:57:23 2010
@@ -1677,6 +1677,165 @@
expected_output = ["Reverted '" + mu_path + "'\n"]
svntest.actions.run_and_verify_svn(None, expected_output, [], 'revert', '-R', wc_dir)
+def patch_with_exclude_patterns(sbox):
+ "patch with --exclude-patterns"
+
+ sbox.build()
+ wc_dir = sbox.wc_dir
+
+ patch_file_path = tempfile.mkstemp(dir=os.path.abspath(svntest.main.temp_dir))[1]
+ mu_path = os.path.join(wc_dir, 'A', 'mu')
+
+ mu_contents = [
+ "Dear internet user,\n",
+ "\n",
+ "We wish to congratulate you over your email success in our computer\n",
+ "Balloting. This is a Millennium Scientific Electronic Computer Draw\n",
+ "in which email addresses were used. All participants were selected\n",
+ "through a computer ballot system drawn from over 100,000 company\n",
+ "and 50,000,000 individual email addresses from all over the world.\n",
+ "\n",
+ "Your email address drew and have won the sum of 750,000 Euros\n",
+ "( Seven Hundred and Fifty Thousand Euros) in cash credited to\n",
+ "file with\n",
+ " REFERENCE NUMBER: ESP/WIN/008/05/10/MA;\n",
+ " WINNING NUMBER : 14-17-24-34-37-45-16\n",
+ " BATCH NUMBERS :\n",
+ " EULO/1007/444/606/08;\n",
+ " SERIAL NUMBER: 45327\n",
+ "and PROMOTION DATE: 13th June. 2009\n",
+ "\n",
+ "To claim your winning prize, you are to contact the appointed\n",
+ "agent below as soon as possible for the immediate release of your\n",
+ "winnings with the below details.\n",
+ "\n",
+ "Again, we wish to congratulate you over your email success in our\n"
+ "computer Balloting.\n"
+ ]
+
+ # Set mu contents
+ svntest.main.file_write(mu_path, ''.join(mu_contents))
+ expected_output = svntest.wc.State(wc_dir, {
+ 'A/mu' : Item(verb='Sending'),
+ })
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
+ expected_status.tweak('A/mu', wc_rev=2)
+ svntest.actions.run_and_verify_commit(wc_dir, expected_output,
+ expected_status, None, wc_dir)
+
+ # Apply patch
+
+ unidiff_patch = [
+ "Index: A/D/gamma\n",
+ "===================================================================\n",
+ "--- A/D/gamma\t(revision 1)\n",
+ "+++ A/D/gamma\t(working copy)\n",
+ "@@ -1 +1 @@\n",
+ "-This is the file 'gamma'.\n",
+ "+It is the file 'gamma'.\n",
+ "Index: iota\n",
+ "===================================================================\n",
+ "--- iota\t(revision 1)\n",
+ "+++ iota\t(working copy)\n",
+ "@@ -1 +1,2 @@\n",
+ " This is the file 'iota'.\n",
+ "+Some more bytes\n",
+ "\n",
+ "Index: new\n",
+ "===================================================================\n",
+ "--- new (revision 0)\n",
+ "+++ new (revision 0)\n",
+ "@@ -0,0 +1 @@\n",
+ "+new\n",
+ "\n",
+ "--- A/mu.orig 2009-06-24 15:23:55.000000000 +0100\n",
+ "+++ A/mu 2009-06-24 15:21:23.000000000 +0100\n",
+ "@@ -6,6 +6,9 @@\n",
+ " through a computer ballot system drawn from over 100,000 company\n",
+ " and 50,000,000 individual email addresses from all over the world.\n",
+ " \n",
+ "+It is a promotional program aimed at encouraging internet users;\n",
+ "+therefore you do not need to buy ticket to enter for it.\n",
+ "+\n",
+ " Your email address drew and have won the sum of 750,000 Euros\n",
+ " ( Seven Hundred and Fifty Thousand Euros) in cash credited to\n",
+ " file with\n",
+ "@@ -14,11 +17,8 @@\n",
+ " BATCH NUMBERS :\n",
+ " EULO/1007/444/606/08;\n",
+ " SERIAL NUMBER: 45327\n",
+ "-and PROMOTION DATE: 13th June. 2009\n",
+ "+and PROMOTION DATE: 14th June. 2009\n",
+ " \n",
+ " To claim your winning prize, you are to contact the appointed\n",
+ " agent below as soon as possible for the immediate release of your\n",
+ " winnings with the below details.\n",
+ "-\n",
+ "-Again, we wish to congratulate you over your email success in our\n",
+ "-computer Balloting.\n",
+ "Index: A/B/E/beta\n",
+ "===================================================================\n",
+ "--- A/B/E/beta (revision 1)\n",
+ "+++ A/B/E/beta (working copy)\n",
+ "@@ -1 +0,0 @@\n",
+ "-This is the file 'beta'.\n",
+ ]
+
+ svntest.main.file_write(patch_file_path, ''.join(unidiff_patch))
+
+ gamma_contents = "It is the file 'gamma'.\n"
+ iota_contents = "This is the file 'iota'.\nSome more bytes\n"
+ new_contents = "new\n"
+ mu_contents = [
+ "Dear internet user,\n",
+ "\n",
+ "We wish to congratulate you over your email success in our computer\n",
+ "Balloting. This is a Millennium Scientific Electronic Computer Draw\n",
+ "in which email addresses were used. All participants were selected\n",
+ "through a computer ballot system drawn from over 100,000 company\n",
+ "and 50,000,000 individual email addresses from all over the world.\n",
+ "\n",
+ "It is a promotional program aimed at encouraging internet users;\n",
+ "therefore you do not need to buy ticket to enter for it.\n",
+ "\n",
+ "Your email address drew and have won the sum of 750,000 Euros\n",
+ "( Seven Hundred and Fifty Thousand Euros) in cash credited to\n",
+ "file with\n",
+ " REFERENCE NUMBER: ESP/WIN/008/05/10/MA;\n",
+ " WINNING NUMBER : 14-17-24-34-37-45-16\n",
+ " BATCH NUMBERS :\n",
+ " EULO/1007/444/606/08;\n",
+ " SERIAL NUMBER: 45327\n",
+ "and PROMOTION DATE: 14th June. 2009\n",
+ "\n",
+ "To claim your winning prize, you are to contact the appointed\n",
+ "agent below as soon as possible for the immediate release of your\n",
+ "winnings with the below details.\n",
+ ]
+
+ expected_output = [
+ 'U %s\n' % os.path.join(wc_dir, 'A', 'mu'),
+ ]
+
+ expected_disk = svntest.main.greek_state.copy()
+ expected_disk.tweak('A/mu', contents=''.join(mu_contents))
+
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
+ expected_status.tweak('A/mu', status='M ', wc_rev=2)
+
+ expected_skip = wc.State('', { })
+
+ svntest.actions.run_and_verify_patch(wc_dir, os.path.abspath(patch_file_path),
+ expected_output,
+ expected_disk,
+ expected_status,
+ expected_skip,
+ None, # expected err
+ 1, # check-props
+ 1, # dry-run
+ "--exclude-pattern", "A/*/gamma",
+ "--exclude-pattern", "new",
+ "--exclude-pattern", "*a")
########################################################################
#Run the tests
@@ -1696,6 +1855,7 @@
patch_no_svn_eol_style,
patch_with_svn_eol_style,
patch_with_svn_eol_style_uncommitted,
+ patch_with_exclude_patterns,
]
if __name__ == '__main__':
Re: svn commit: r919460 - filtering svn patch targets
Posted by Neels J Hofmeyr <ne...@elego.de>.
Julian Foad wrote:
> To be frank, I am heartily looking forward to the day when we can have
> your expertise focused back on tree conflicts, WC-NG, merging or such
> like.
Yeah, stsp, what's up with that! ;)
~Neels
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Mar 13, 2010 at 09:41:06AM +0100, Stefan Sperling wrote:
> On Fri, Mar 12, 2010 at 11:12:38PM -0800, Joe Swatosh wrote:
> > On Fri, Mar 12, 2010 at 1:45 AM, Stefan Sperling <st...@elego.de> wrote:
> > > On Fri, Mar 12, 2010 at 03:11:06AM +0100, Neels J Hofmeyr wrote:
> > >> Stefan Sperling wrote:
> > >> >>> What is not possible (without adding the --include-pattern option)
> >
> > >
> > > It does. You can manually revert the edits 'svn patch' made to the file.
> > > Whether this is feasible or not really depends on the kind of modifications
> > > the user and the patch are making. But it seems to me like the tortoise
> > > GUI was designed around the assumption that users will often want to
> > > apply only parts of patches at file-level granularity. I don't believe
> > > that this is what people do in reality, so I'm questioning this GUI
> > > design because it has repercussions on the svn patch API.
> > > (I didn't really think about this before working on fixing #3434
> > > unfortunately.)
> > >
> >
> > Speaking up as a user of TortoiseSVN here, I use this several times a
> > week at work.
>
> OK, great then. So people need this.
>
> That means that I'll eventually add an --include-patterns option to the
> CLI client. I don't see the point of having a libsvn_client API that
> tortoiseSVN uses to provide a useful feature, but have the CLI client
> not expose this useful feature.
The --include-pattern option now exists as of r924737.
We may still decide to rename the --include-pattern/--exclude-pattern
options of the svn command line client before 1.7.0 release, but the
API for including/excluding patch targets will most likely stay as is.
Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Joe Swatosh <jo...@gmail.com>.
On Sat, Mar 13, 2010 at 12:41 AM, Stefan Sperling <st...@elego.de> wrote:
> On Fri, Mar 12, 2010 at 11:12:38PM -0800, Joe Swatosh wrote:
>
> I also highly appreciate Stefan K's feedback. Even though in this thread
> it may have seemed like I was primarily arguing against his points,
> the point of doing so is not to "win" the argument, but to make sure
> we're really considering things from different angles, and have people
> present the reasoning behind their opinions, so we have a higher
> chance of ending up with something everyone is happy with.
>
I never for a moment thought otherwise.
I just spoke up because to try to give his statements a little more
weight. I have nothing to offer WRT APIs, but from user-land wanted
to emphasize that there were others with his "partial application" use
case.
--
Joe
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Mar 12, 2010 at 11:12:38PM -0800, Joe Swatosh wrote:
> On Fri, Mar 12, 2010 at 1:45 AM, Stefan Sperling <st...@elego.de> wrote:
> > On Fri, Mar 12, 2010 at 03:11:06AM +0100, Neels J Hofmeyr wrote:
> >> Stefan Sperling wrote:
> >> >>> What is not possible (without adding the --include-pattern option)
>
> >
> > It does. You can manually revert the edits 'svn patch' made to the file.
> > Whether this is feasible or not really depends on the kind of modifications
> > the user and the patch are making. But it seems to me like the tortoise
> > GUI was designed around the assumption that users will often want to
> > apply only parts of patches at file-level granularity. I don't believe
> > that this is what people do in reality, so I'm questioning this GUI
> > design because it has repercussions on the svn patch API.
> > (I didn't really think about this before working on fixing #3434
> > unfortunately.)
> >
>
> Speaking up as a user of TortoiseSVN here, I use this several times a
> week at work.
OK, great then. So people need this.
That means that I'll eventually add an --include-patterns option to the
CLI client. I don't see the point of having a libsvn_client API that
tortoiseSVN uses to provide a useful feature, but have the CLI client
not expose this useful feature.
Julian, are you fine with that? Or would you like the option names
to be more specific to 'svn patch'? I've kept the option's name
general on purpose to encourage re-use of this option by other
subcommands (if they have a use for this option). Which I hope
will help keep the CLI interface as consistent as possible.
> Despite Stefan K's protestations, I encouraged him some
> time back to get involved in this because without dinking with line
> endings and other nonsense TortoiseSVN is by far the best way I've
> found to apply patches on Windows.
I also highly appreciate Stefan K's feedback. Even though in this thread
it may have seemed like I was primarily arguing against his points,
the point of doing so is not to "win" the argument, but to make sure
we're really considering things from different angles, and have people
present the reasoning behind their opinions, so we have a higher
chance of ending up with something everyone is happy with.
> While I have no skin in the development game, "svn patch" is a feature
> that I'm very excited about using in the future.
Cool, thanks!
Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Joe Swatosh <jo...@gmail.com>.
On Fri, Mar 12, 2010 at 1:45 AM, Stefan Sperling <st...@elego.de> wrote:
> On Fri, Mar 12, 2010 at 03:11:06AM +0100, Neels J Hofmeyr wrote:
>> Stefan Sperling wrote:
>> >>> What is not possible (without adding the --include-pattern option)
>
> It does. You can manually revert the edits 'svn patch' made to the file.
> Whether this is feasible or not really depends on the kind of modifications
> the user and the patch are making. But it seems to me like the tortoise
> GUI was designed around the assumption that users will often want to
> apply only parts of patches at file-level granularity. I don't believe
> that this is what people do in reality, so I'm questioning this GUI
> design because it has repercussions on the svn patch API.
> (I didn't really think about this before working on fixing #3434
> unfortunately.)
>
Speaking up as a user of TortoiseSVN here, I use this several times a
week at work. Despite Stefan K's protestations, I encouraged him some
time back to get involved in this because without dinking with line
endings and other nonsense TortoiseSVN is by far the best way I've
found to apply patches on Windows.
While I have no skin in the development game, "svn patch" is a feature
that I'm very excited about using in the future.
--
Joe
Re: svn commit: r919460 - filtering svn patch targets
Posted by Julian Foad <ju...@wandisco.com>.
Stefan Sperling wrote:
> On Fri, Mar 12, 2010 at 03:11:06AM +0100, Neels J Hofmeyr wrote:
[...]
> > So anything it does with patches should be done by
> > svn. In effect, whatever Tortoise wants to do with a patch has to be
> > implemented in svn, taking away dev time from wc-ng, pristine store and <add
> > cool features>.
>
> So svn patch is not a <cool feature>?
Neither of us meant that. Patch is a good thing to be doing, too.
- Julian
> I want to see the basics of the 'svn patch' feature through to completion
> for 1.7. We have enough features that were released in a half-baked state.
>
> > The real question is: how much effort shall we invest at this point into
> > selectively applying patches? Can it wait for a later release? Can someone
> > who really needs it implement it?
>
> Nobody can implement anything before discussing what needs to be
> implemented (which is what this thread is about).
>
> Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Küng <to...@gmail.com>.
On 12.03.2010 10:45, Stefan Sperling wrote:
> On Fri, Mar 12, 2010 at 03:11:06AM +0100, Neels J Hofmeyr wrote:
>> Stefan Sperling wrote:
>>>>> What is not possible (without adding the --include-pattern option)
>>>>> is selecting which files to patch. Is selecting individual patch
>>>>> targets really that important?
>>>> Yes, that's very important. I often find that when I get a patch, I
>>>> only want to use part of it because I found that when reviewing the
>>>> changes I have to reject some of those changes.
>>>
>>> I'm not sure if this use case is worth optimising for.
>>> You could easily apply the patch, and then selectively revert
>>> some of the patched files. What you are describing is a special
>>> case of "I want to apply this patch and also make custom modifications
>>> to the patched result." Why not just apply the patch and then make
>>> the necessary modifications? Or request a revised patch from the patch
>>> submitter?
>>
>> I do see what Stefan K is getting at. Stsp, your revert workaround does not
>> work when there already are modifications on the files prior to patching.
>
> It does. You can manually revert the edits 'svn patch' made to the file.
But that requires that the user knows for *sure* which changes are made
by the patch file and which ones were already there. And believe me,
many users won't know that, especially if they made a lot of changes to
that file.
> Whether this is feasible or not really depends on the kind of modifications
> the user and the patch are making. But it seems to me like the tortoise
> GUI was designed around the assumption that users will often want to
> apply only parts of patches at file-level granularity. I don't believe
> that this is what people do in reality, so I'm questioning this GUI
> design because it has repercussions on the svn patch API.
Well, *I* usually do that, and so do many others I know.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Mar 12, 2010 at 03:11:06AM +0100, Neels J Hofmeyr wrote:
> Stefan Sperling wrote:
> >>> What is not possible (without adding the --include-pattern option)
> >>> is selecting which files to patch. Is selecting individual patch
> >>> targets really that important?
> >> Yes, that's very important. I often find that when I get a patch, I
> >> only want to use part of it because I found that when reviewing the
> >> changes I have to reject some of those changes.
> >
> > I'm not sure if this use case is worth optimising for.
> > You could easily apply the patch, and then selectively revert
> > some of the patched files. What you are describing is a special
> > case of "I want to apply this patch and also make custom modifications
> > to the patched result." Why not just apply the patch and then make
> > the necessary modifications? Or request a revised patch from the patch
> > submitter?
>
> I do see what Stefan K is getting at. Stsp, your revert workaround does not
> work when there already are modifications on the files prior to patching.
It does. You can manually revert the edits 'svn patch' made to the file.
Whether this is feasible or not really depends on the kind of modifications
the user and the patch are making. But it seems to me like the tortoise
GUI was designed around the assumption that users will often want to
apply only parts of patches at file-level granularity. I don't believe
that this is what people do in reality, so I'm questioning this GUI
design because it has repercussions on the svn patch API.
(I didn't really think about this before working on fixing #3434
unfortunately.)
> I guess 'svn patch' should enable Tortoise to be ignorant about what a patch
> file format looks like.
That's what the diff parsing API does. We just need to make it public API.
> So anything it does with patches should be done by
> svn. In effect, whatever Tortoise wants to do with a patch has to be
> implemented in svn, taking away dev time from wc-ng, pristine store and <add
> cool features>.
So svn patch is not a <cool feature>?
I want to see the basics of the 'svn patch' feature through to completion
for 1.7. We have enough features that were released in a half-baked state.
> The real question is: how much effort shall we invest at this point into
> selectively applying patches? Can it wait for a later release? Can someone
> who really needs it implement it?
Nobody can implement anything before discussing what needs to be
implemented (which is what this thread is about).
Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Küng <to...@gmail.com>.
On 12.03.2010 10:34, Stefan Sperling wrote:
> Fine. Either way would work for me.
> But who is responsible for cleaning up the temporary files?
> The caller of svn_client_patch() or a pool cleanup handler installed
> by svn_client_patch() at the result_pool provided by the caller?
In that case, the caller would be responsible for deleting those temp
files. Only the caller knows how long it needs those files. It might
even be required to leave those temp files around after the process
finishes, depending on what the caller uses them for.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Mar 12, 2010 at 12:05:48AM +0000, Julian Foad wrote:
> Stefan Sperling wrote:
> > Right now, the tempfiles will be closed on pool cleanup.
>
> You mean "will be deleted on pool cleanup"?
Yes.
>
> > My idea was to specify svn_io_file_del_on_close instead in case
> > tempfiles is not NULL. Which would mean that we should leave them
> > open when returning to the caller.
>
> The caller can't (properly) close an open file unless you give the
> caller the (open) file handle, not just the file name.
Oh, right.
> > But we can pick either or svn_io_file_del_on_pool_cleanup (depending
> > on the result_pool) or svn_io_file_del_on_close. I don't really mind
> > either way.
> >
> > Can you explain why you would want the files already closed? Just curious.
>
> If you're going to hand someone an "open file", that either means an
> (open) handle to the file, or it means a filename of a file that some
> OTHER code path has an open handle to. If the former, where is the file
> pointer (beginning, end, undefined)? If the latter, what is the sharing
> mode (can the caller also open the file)?
>
> In other words, it's just wierd. Either give the path of a (closed)
> file, or give a stream (positioned at the beginning, ready to read
> from).
Fine. Either way would work for me.
But who is responsible for cleaning up the temporary files?
The caller of svn_client_patch() or a pool cleanup handler installed
by svn_client_patch() at the result_pool provided by the caller?
Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Küng <to...@gmail.com>.
On 11.03.2010 23:44, Stefan Sperling wrote:
> On Thu, Mar 11, 2010 at 07:07:04PM +0100, Stefan Küng wrote:
>> On 10.03.2010 23:22, Stefan Sperling wrote:
>>> svn_error_t *
>>> svn_client_patch(const char *abs_patch_path,
>>> const char *local_abspath,
>>> svn_boolean_t dry_run,
>>> int strip_count,
>>> svn_boolean_t reverse,
>>> apr_hash_t **tempfiles,
>>> svn_client_ctx_t *ctx,
>>> apr_pool_t *result_pool,
>>> apr_pool_t *scratch_pool);
>>>
>>> If you pass non-NULL for tempfiles, and TRUE for dry-run, you'd get back
>>> in *tempfiles a mapping {path as in patch file => tempfile containing patched
>>> result}, the tempfiles are left open by svn_client_patch() so you need to
>>> close them, and the working copy is not modified.
>>
>> Why are the files left open? If there's a good reason for leaving
>> them open, then of course I can close them in TSVN first before
>> using them. But it would be nice if the files were already closed.
>
> The tempfiles will eventually need to be deleted.
>
> Right now, the tempfiles will be closed on pool cleanup.
> My idea was to specify svn_io_file_del_on_close instead in case
> tempfiles is not NULL. Which would mean that we should leave them
> open when returning to the caller.
>
> But we can pick either or svn_io_file_del_on_pool_cleanup (depending
> on the result_pool) or svn_io_file_del_on_close. I don't really mind
> either way.
>
> Can you explain why you would want the files already closed? Just curious.
Because I need to open those files in TortoiseMerge again. Most likely
using a different API.
Also, the temp files shouldn't get deleted by the svn library but the
deletion should be left to the client. You don't know in the library
what a client will use the files for, so you should not delete them.
>
>>> What is not possible (without adding the --include-pattern option)
>>> is selecting which files to patch. Is selecting individual patch
>>> targets really that important?
>>
>> Yes, that's very important. I often find that when I get a patch, I
>> only want to use part of it because I found that when reviewing the
>> changes I have to reject some of those changes.
>
> I'm not sure if this use case is worth optimising for.
> You could easily apply the patch, and then selectively revert
> some of the patched files. What you are describing is a special
> case of "I want to apply this patch and also make custom modifications
> to the patched result." Why not just apply the patch and then make
> the necessary modifications? Or request a revised patch from the patch
> submitter?
The include pattern only has to filter by target path, not by individual
change chunks in the patch file.
Imagine you use a library, but you made some modifications to that
library for yourself. Now, the library gets an update and you receive a
patch for that update.
Do you really want to just apply the full patch without first knowing
whether it conflicts with your own modifications? And I'm not talking
here about conflicts that are detectable by svn and would lead to a
conflicted state, but subtle conflicts you would only notice later.
I often have this situation. So I apply the patch only to those files
that I haven't modified myself. Then I review carefully the result of
patching the files I've modified, by first create a temp file that is
the same as when the patch was applied directly to the real file (but
since it's a temp file, the real file isn't changed yet).
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Küng <to...@gmail.com>.
On 11.03.2010 23:44, Stefan Sperling wrote:
> On Thu, Mar 11, 2010 at 07:07:04PM +0100, Stefan Küng wrote:
>> On 10.03.2010 23:22, Stefan Sperling wrote:
>>> svn_error_t *
>>> svn_client_patch(const char *abs_patch_path,
>>> const char *local_abspath,
>>> svn_boolean_t dry_run,
>>> int strip_count,
>>> svn_boolean_t reverse,
>>> apr_hash_t **tempfiles,
>>> svn_client_ctx_t *ctx,
>>> apr_pool_t *result_pool,
>>> apr_pool_t *scratch_pool);
>>>
>>> If you pass non-NULL for tempfiles, and TRUE for dry-run, you'd get back
>>> in *tempfiles a mapping {path as in patch file => tempfile containing patched
>>> result}, the tempfiles are left open by svn_client_patch() so you need to
>>> close them, and the working copy is not modified.
>>
>> Why are the files left open? If there's a good reason for leaving
>> them open, then of course I can close them in TSVN first before
>> using them. But it would be nice if the files were already closed.
>
> The tempfiles will eventually need to be deleted.
>
> Right now, the tempfiles will be closed on pool cleanup.
> My idea was to specify svn_io_file_del_on_close instead in case
> tempfiles is not NULL. Which would mean that we should leave them
> open when returning to the caller.
>
> But we can pick either or svn_io_file_del_on_pool_cleanup (depending
> on the result_pool) or svn_io_file_del_on_close. I don't really mind
> either way.
>
> Can you explain why you would want the files already closed? Just curious.
Because I need to open those files in TortoiseMerge again. Most likely
using a different API.
Also, the temp files shouldn't get deleted by the svn library but the
deletion should be left to the client. You don't know in the library
what a client will use the files for, so you should not delete them.
>
>>> What is not possible (without adding the --include-pattern option)
>>> is selecting which files to patch. Is selecting individual patch
>>> targets really that important?
>>
>> Yes, that's very important. I often find that when I get a patch, I
>> only want to use part of it because I found that when reviewing the
>> changes I have to reject some of those changes.
>
> I'm not sure if this use case is worth optimising for.
> You could easily apply the patch, and then selectively revert
> some of the patched files. What you are describing is a special
> case of "I want to apply this patch and also make custom modifications
> to the patched result." Why not just apply the patch and then make
> the necessary modifications? Or request a revised patch from the patch
> submitter?
The include pattern only has to filter by target path, not by individual
change chunks in the patch file.
Imagine you use a library, but you made some modifications to that
library for yourself. Now, the library gets an update and you receive a
patch for that update.
Do you really want to just apply the full patch without first knowing
whether it conflicts with your own modifications? And I'm not talking
here about conflicts that are detectable by svn and would lead to a
conflicted state, but subtle conflicts you would only notice later.
I often have this situation. So I apply the patch only to those files
that I haven't modified myself. Then I review carefully the result of
patching the files I've modified, by first create a temp file that is
the same as when the patch was applied directly to the real file (but
since it's a temp file, the real file isn't changed yet).
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r919460 - filtering svn patch targets
Posted by Neels J Hofmeyr <ne...@elego.de>.
Stefan Sperling wrote:
>>> What is not possible (without adding the --include-pattern option)
>>> is selecting which files to patch. Is selecting individual patch
>>> targets really that important?
>> Yes, that's very important. I often find that when I get a patch, I
>> only want to use part of it because I found that when reviewing the
>> changes I have to reject some of those changes.
>
> I'm not sure if this use case is worth optimising for.
> You could easily apply the patch, and then selectively revert
> some of the patched files. What you are describing is a special
> case of "I want to apply this patch and also make custom modifications
> to the patched result." Why not just apply the patch and then make
> the necessary modifications? Or request a revised patch from the patch
> submitter?
I do see what Stefan K is getting at. Stsp, your revert workaround does not
work when there already are modifications on the files prior to patching.
I guess 'svn patch' should enable Tortoise to be ignorant about what a patch
file format looks like. So anything it does with patches should be done by
svn. In effect, whatever Tortoise wants to do with a patch has to be
implemented in svn, taking away dev time from wc-ng, pristine store and <add
cool features>.
The real question is: how much effort shall we invest at this point into
selectively applying patches? Can it wait for a later release? Can someone
who really needs it implement it?
You're all free to choose, especially stsp ;)
~Neels
Re: svn commit: r919460 - filtering svn patch targets
Posted by Julian Foad <ju...@wandisco.com>.
Stefan Sperling wrote:
> On Thu, Mar 11, 2010 at 07:07:04PM +0100, Stefan Küng wrote:
> > On 10.03.2010 23:22, Stefan Sperling wrote:
> > >svn_error_t *
> > >svn_client_patch(const char *abs_patch_path,
> > > const char *local_abspath,
> > > svn_boolean_t dry_run,
> > > int strip_count,
> > > svn_boolean_t reverse,
> > > apr_hash_t **tempfiles,
> > > svn_client_ctx_t *ctx,
> > > apr_pool_t *result_pool,
> > > apr_pool_t *scratch_pool);
> > >
> > >If you pass non-NULL for tempfiles, and TRUE for dry-run, you'd get back
> > >in *tempfiles a mapping {path as in patch file => tempfile containing patched
> > >result}, the tempfiles are left open by svn_client_patch() so you need to
> > >close them, and the working copy is not modified.
> >
> > Why are the files left open? If there's a good reason for leaving
> > them open, then of course I can close them in TSVN first before
> > using them. But it would be nice if the files were already closed.
>
> The tempfiles will eventually need to be deleted.
>
> Right now, the tempfiles will be closed on pool cleanup.
You mean "will be deleted on pool cleanup"?
> My idea was to specify svn_io_file_del_on_close instead in case
> tempfiles is not NULL. Which would mean that we should leave them
> open when returning to the caller.
The caller can't (properly) close an open file unless you give the
caller the (open) file handle, not just the file name.
> But we can pick either or svn_io_file_del_on_pool_cleanup (depending
> on the result_pool) or svn_io_file_del_on_close. I don't really mind
> either way.
>
> Can you explain why you would want the files already closed? Just curious.
If you're going to hand someone an "open file", that either means an
(open) handle to the file, or it means a filename of a file that some
OTHER code path has an open handle to. If the former, where is the file
pointer (beginning, end, undefined)? If the latter, what is the sharing
mode (can the caller also open the file)?
In other words, it's just wierd. Either give the path of a (closed)
file, or give a stream (positioned at the beginning, ready to read
from).
- Julian
> > >What is not possible (without adding the --include-pattern option)
> > >is selecting which files to patch. Is selecting individual patch
> > >targets really that important?
> >
> > Yes, that's very important. I often find that when I get a patch, I
> > only want to use part of it because I found that when reviewing the
> > changes I have to reject some of those changes.
>
> I'm not sure if this use case is worth optimising for.
> You could easily apply the patch, and then selectively revert
> some of the patched files. What you are describing is a special
> case of "I want to apply this patch and also make custom modifications
> to the patched result." Why not just apply the patch and then make
> the necessary modifications? Or request a revised patch from the patch
> submitter?
>
> Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Mar 11, 2010 at 07:07:04PM +0100, Stefan Küng wrote:
> On 10.03.2010 23:22, Stefan Sperling wrote:
> >svn_error_t *
> >svn_client_patch(const char *abs_patch_path,
> > const char *local_abspath,
> > svn_boolean_t dry_run,
> > int strip_count,
> > svn_boolean_t reverse,
> > apr_hash_t **tempfiles,
> > svn_client_ctx_t *ctx,
> > apr_pool_t *result_pool,
> > apr_pool_t *scratch_pool);
> >
> >If you pass non-NULL for tempfiles, and TRUE for dry-run, you'd get back
> >in *tempfiles a mapping {path as in patch file => tempfile containing patched
> >result}, the tempfiles are left open by svn_client_patch() so you need to
> >close them, and the working copy is not modified.
>
> Why are the files left open? If there's a good reason for leaving
> them open, then of course I can close them in TSVN first before
> using them. But it would be nice if the files were already closed.
The tempfiles will eventually need to be deleted.
Right now, the tempfiles will be closed on pool cleanup.
My idea was to specify svn_io_file_del_on_close instead in case
tempfiles is not NULL. Which would mean that we should leave them
open when returning to the caller.
But we can pick either or svn_io_file_del_on_pool_cleanup (depending
on the result_pool) or svn_io_file_del_on_close. I don't really mind
either way.
Can you explain why you would want the files already closed? Just curious.
> >What is not possible (without adding the --include-pattern option)
> >is selecting which files to patch. Is selecting individual patch
> >targets really that important?
>
> Yes, that's very important. I often find that when I get a patch, I
> only want to use part of it because I found that when reviewing the
> changes I have to reject some of those changes.
I'm not sure if this use case is worth optimising for.
You could easily apply the patch, and then selectively revert
some of the patched files. What you are describing is a special
case of "I want to apply this patch and also make custom modifications
to the patched result." Why not just apply the patch and then make
the necessary modifications? Or request a revised patch from the patch
submitter?
Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Mar 11, 2010 at 07:07:04PM +0100, Stefan Küng wrote:
> On 10.03.2010 23:22, Stefan Sperling wrote:
> >svn_error_t *
> >svn_client_patch(const char *abs_patch_path,
> > const char *local_abspath,
> > svn_boolean_t dry_run,
> > int strip_count,
> > svn_boolean_t reverse,
> > apr_hash_t **tempfiles,
> > svn_client_ctx_t *ctx,
> > apr_pool_t *result_pool,
> > apr_pool_t *scratch_pool);
> >
> >If you pass non-NULL for tempfiles, and TRUE for dry-run, you'd get back
> >in *tempfiles a mapping {path as in patch file => tempfile containing patched
> >result}, the tempfiles are left open by svn_client_patch() so you need to
> >close them, and the working copy is not modified.
>
> Why are the files left open? If there's a good reason for leaving
> them open, then of course I can close them in TSVN first before
> using them. But it would be nice if the files were already closed.
The tempfiles will eventually need to be deleted.
Right now, the tempfiles will be closed on pool cleanup.
My idea was to specify svn_io_file_del_on_close instead in case
tempfiles is not NULL. Which would mean that we should leave them
open when returning to the caller.
But we can pick either or svn_io_file_del_on_pool_cleanup (depending
on the result_pool) or svn_io_file_del_on_close. I don't really mind
either way.
Can you explain why you would want the files already closed? Just curious.
> >What is not possible (without adding the --include-pattern option)
> >is selecting which files to patch. Is selecting individual patch
> >targets really that important?
>
> Yes, that's very important. I often find that when I get a patch, I
> only want to use part of it because I found that when reviewing the
> changes I have to reject some of those changes.
I'm not sure if this use case is worth optimising for.
You could easily apply the patch, and then selectively revert
some of the patched files. What you are describing is a special
case of "I want to apply this patch and also make custom modifications
to the patched result." Why not just apply the patch and then make
the necessary modifications? Or request a revised patch from the patch
submitter?
Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Küng <to...@gmail.com>.
On 10.03.2010 23:22, Stefan Sperling wrote:
>> Reviewing a patch file is not what most users expect and are
>> familiar with. Especially if there are many changes spread through a
>> big file, it's not enough to just see the patch file with the three
>> context lines to really get what the changes do - you have to see
>> the full file with all the changes.
>>
>> Applying the patch to a temp file allows me to show the changes in
>> TortoiseMerge, the old and new file side-by-side. That's what users
>> are used to and where they can really see the result.
>>
>> Also, if you just review the patch file itself, there's no guarantee
>> that when the patch is applied that that result is what you expect:
>> depending on the algorithm to find the position of where to apply a
>> hunk, such a hunk can get applied somewhere you didn't expect. So a
>> 'real' preview is necessary.
>
> This is really your second request -- you want to get at the
> tempfiles svn patch generates during patching. Have you seen my
> proposal in http://subversion.tigris.org/issues/show_bug.cgi?id=3598 ?
> Basically, we could have svn_client_patch() look like this:
>
> svn_error_t *
> svn_client_patch(const char *abs_patch_path,
> const char *local_abspath,
> svn_boolean_t dry_run,
> int strip_count,
> svn_boolean_t reverse,
> apr_hash_t **tempfiles,
> svn_client_ctx_t *ctx,
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool);
>
> If you pass non-NULL for tempfiles, and TRUE for dry-run, you'd get back
> in *tempfiles a mapping {path as in patch file => tempfile containing patched
> result}, the tempfiles are left open by svn_client_patch() so you need to
> close them, and the working copy is not modified.
Why are the files left open? If there's a good reason for leaving them
open, then of course I can close them in TSVN first before using them.
But it would be nice if the files were already closed.
> Would that suffice to cover base 2?
Yes, that would work.
> What is not possible (without adding the --include-pattern option)
> is selecting which files to patch. Is selecting individual patch
> targets really that important?
Yes, that's very important. I often find that when I get a patch, I only
want to use part of it because I found that when reviewing the changes I
have to reject some of those changes.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Mar 10, 2010 at 10:50:13PM +0100, Stefan Küng wrote:
> On 10.03.2010 22:43, Stefan Sperling wrote:
> >On Wed, Mar 10, 2010 at 08:58:24PM +0100, Stefan Küng wrote:
> >>On 10.03.2010 19:14, Julian Foad wrote:
> >>
> >>>>Yes, I would also need to know which paths the patch wants to modify,
> >>>>and then later I want to tell the API which of those paths it should
> >>>>actually modify.
> >>>>This is to let the user choose which paths to modify and which ones not.
> >>>
> >>>OK, well the issue #3434 didn't mention that. Since you already have a
> >>>patch implementation which does what you want, could you show us the API
> >>>definition (at least) for this part of it? That would help a lot, since
> >>>then we wouldn't have to guess about what you want :-)
> >>
> >>It's basically a class with methods like:
> >>* ParsePatchfile
> >>* GetCount //returns the number of paths affected
> >>* GetPath(int index) // returns the affected path
> >>* PatchPath(int index, TCHAR * target) // applies the patch for the
> >>path and saves the result in 'target'
> >>The names of the class methods are actually a little bit different,
> >>but that's pretty much what we have right now and what we need.
> >
> >What about if you just parse the patch using the diff parser
> >we have in libsvn_diff? Would that be enough?
> >See this file for details:
> >http://svn.apache.org/repos/asf/subversion/trunk/subversion/include/private/svn_diff_private.h
> >We could make those functions public if you want.
>
> Yes, those functions and structs would suffice to get all the target
> paths of the patch file.
OK, so we have base 1 covered.
> >Another thing: Why do you need to apply the patch to some tempfile?
> >Why is the preview of the patched result really necessary?
> >Why not just show the user the patch? When reviewing what a patch
> >does, it's much easier to look at the patch itself rather than the
> >patched result -- at least from my point of view, I don't know if your
> >users are different.
> >
> >If you agree to these points, we could drop the svn_client_patch() API
> >change again.
>
> Reviewing a patch file is not what most users expect and are
> familiar with. Especially if there are many changes spread through a
> big file, it's not enough to just see the patch file with the three
> context lines to really get what the changes do - you have to see
> the full file with all the changes.
>
> Applying the patch to a temp file allows me to show the changes in
> TortoiseMerge, the old and new file side-by-side. That's what users
> are used to and where they can really see the result.
>
> Also, if you just review the patch file itself, there's no guarantee
> that when the patch is applied that that result is what you expect:
> depending on the algorithm to find the position of where to apply a
> hunk, such a hunk can get applied somewhere you didn't expect. So a
> 'real' preview is necessary.
This is really your second request -- you want to get at the
tempfiles svn patch generates during patching. Have you seen my
proposal in http://subversion.tigris.org/issues/show_bug.cgi?id=3598 ?
Basically, we could have svn_client_patch() look like this:
svn_error_t *
svn_client_patch(const char *abs_patch_path,
const char *local_abspath,
svn_boolean_t dry_run,
int strip_count,
svn_boolean_t reverse,
apr_hash_t **tempfiles,
svn_client_ctx_t *ctx,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);
If you pass non-NULL for tempfiles, and TRUE for dry-run, you'd get back
in *tempfiles a mapping {path as in patch file => tempfile containing patched
result}, the tempfiles are left open by svn_client_patch() so you need to
close them, and the working copy is not modified.
Would that suffice to cover base 2?
What is not possible (without adding the --include-pattern option)
is selecting which files to patch. Is selecting individual patch
targets really that important?
Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Mar 10, 2010 at 10:50:13PM +0100, Stefan Küng wrote:
> On 10.03.2010 22:43, Stefan Sperling wrote:
> >On Wed, Mar 10, 2010 at 08:58:24PM +0100, Stefan Küng wrote:
> >>On 10.03.2010 19:14, Julian Foad wrote:
> >>
> >>>>Yes, I would also need to know which paths the patch wants to modify,
> >>>>and then later I want to tell the API which of those paths it should
> >>>>actually modify.
> >>>>This is to let the user choose which paths to modify and which ones not.
> >>>
> >>>OK, well the issue #3434 didn't mention that. Since you already have a
> >>>patch implementation which does what you want, could you show us the API
> >>>definition (at least) for this part of it? That would help a lot, since
> >>>then we wouldn't have to guess about what you want :-)
> >>
> >>It's basically a class with methods like:
> >>* ParsePatchfile
> >>* GetCount //returns the number of paths affected
> >>* GetPath(int index) // returns the affected path
> >>* PatchPath(int index, TCHAR * target) // applies the patch for the
> >>path and saves the result in 'target'
> >>The names of the class methods are actually a little bit different,
> >>but that's pretty much what we have right now and what we need.
> >
> >What about if you just parse the patch using the diff parser
> >we have in libsvn_diff? Would that be enough?
> >See this file for details:
> >http://svn.apache.org/repos/asf/subversion/trunk/subversion/include/private/svn_diff_private.h
> >We could make those functions public if you want.
>
> Yes, those functions and structs would suffice to get all the target
> paths of the patch file.
OK, so we have base 1 covered.
> >Another thing: Why do you need to apply the patch to some tempfile?
> >Why is the preview of the patched result really necessary?
> >Why not just show the user the patch? When reviewing what a patch
> >does, it's much easier to look at the patch itself rather than the
> >patched result -- at least from my point of view, I don't know if your
> >users are different.
> >
> >If you agree to these points, we could drop the svn_client_patch() API
> >change again.
>
> Reviewing a patch file is not what most users expect and are
> familiar with. Especially if there are many changes spread through a
> big file, it's not enough to just see the patch file with the three
> context lines to really get what the changes do - you have to see
> the full file with all the changes.
>
> Applying the patch to a temp file allows me to show the changes in
> TortoiseMerge, the old and new file side-by-side. That's what users
> are used to and where they can really see the result.
>
> Also, if you just review the patch file itself, there's no guarantee
> that when the patch is applied that that result is what you expect:
> depending on the algorithm to find the position of where to apply a
> hunk, such a hunk can get applied somewhere you didn't expect. So a
> 'real' preview is necessary.
This is really your second request -- you want to get at the
tempfiles svn patch generates during patching. Have you seen my
proposal in http://subversion.tigris.org/issues/show_bug.cgi?id=3598 ?
Basically, we could have svn_client_patch() look like this:
svn_error_t *
svn_client_patch(const char *abs_patch_path,
const char *local_abspath,
svn_boolean_t dry_run,
int strip_count,
svn_boolean_t reverse,
apr_hash_t **tempfiles,
svn_client_ctx_t *ctx,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);
If you pass non-NULL for tempfiles, and TRUE for dry-run, you'd get back
in *tempfiles a mapping {path as in patch file => tempfile containing patched
result}, the tempfiles are left open by svn_client_patch() so you need to
close them, and the working copy is not modified.
Would that suffice to cover base 2?
What is not possible (without adding the --include-pattern option)
is selecting which files to patch. Is selecting individual patch
targets really that important?
Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Küng <to...@gmail.com>.
On 10.03.2010 22:43, Stefan Sperling wrote:
> On Wed, Mar 10, 2010 at 08:58:24PM +0100, Stefan Küng wrote:
>> On 10.03.2010 19:14, Julian Foad wrote:
>>
>>>> Yes, I would also need to know which paths the patch wants to modify,
>>>> and then later I want to tell the API which of those paths it should
>>>> actually modify.
>>>> This is to let the user choose which paths to modify and which ones not.
>>>
>>> OK, well the issue #3434 didn't mention that. Since you already have a
>>> patch implementation which does what you want, could you show us the API
>>> definition (at least) for this part of it? That would help a lot, since
>>> then we wouldn't have to guess about what you want :-)
>>
>> It's basically a class with methods like:
>> * ParsePatchfile
>> * GetCount //returns the number of paths affected
>> * GetPath(int index) // returns the affected path
>> * PatchPath(int index, TCHAR * target) // applies the patch for the
>> path and saves the result in 'target'
>> The names of the class methods are actually a little bit different,
>> but that's pretty much what we have right now and what we need.
>
> What about if you just parse the patch using the diff parser
> we have in libsvn_diff? Would that be enough?
> See this file for details:
> http://svn.apache.org/repos/asf/subversion/trunk/subversion/include/private/svn_diff_private.h
> We could make those functions public if you want.
Yes, those functions and structs would suffice to get all the target
paths of the patch file.
> Another thing: Why do you need to apply the patch to some tempfile?
> Why is the preview of the patched result really necessary?
> Why not just show the user the patch? When reviewing what a patch
> does, it's much easier to look at the patch itself rather than the
> patched result -- at least from my point of view, I don't know if your
> users are different.
>
> If you agree to these points, we could drop the svn_client_patch() API
> change again.
Reviewing a patch file is not what most users expect and are familiar
with. Especially if there are many changes spread through a big file,
it's not enough to just see the patch file with the three context lines
to really get what the changes do - you have to see the full file with
all the changes.
Applying the patch to a temp file allows me to show the changes in
TortoiseMerge, the old and new file side-by-side. That's what users are
used to and where they can really see the result.
Also, if you just review the patch file itself, there's no guarantee
that when the patch is applied that that result is what you expect:
depending on the algorithm to find the position of where to apply a
hunk, such a hunk can get applied somewhere you didn't expect. So a
'real' preview is necessary.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Küng <to...@gmail.com>.
On 10.03.2010 22:43, Stefan Sperling wrote:
> On Wed, Mar 10, 2010 at 08:58:24PM +0100, Stefan Küng wrote:
>> On 10.03.2010 19:14, Julian Foad wrote:
>>
>>>> Yes, I would also need to know which paths the patch wants to modify,
>>>> and then later I want to tell the API which of those paths it should
>>>> actually modify.
>>>> This is to let the user choose which paths to modify and which ones not.
>>>
>>> OK, well the issue #3434 didn't mention that. Since you already have a
>>> patch implementation which does what you want, could you show us the API
>>> definition (at least) for this part of it? That would help a lot, since
>>> then we wouldn't have to guess about what you want :-)
>>
>> It's basically a class with methods like:
>> * ParsePatchfile
>> * GetCount //returns the number of paths affected
>> * GetPath(int index) // returns the affected path
>> * PatchPath(int index, TCHAR * target) // applies the patch for the
>> path and saves the result in 'target'
>> The names of the class methods are actually a little bit different,
>> but that's pretty much what we have right now and what we need.
>
> What about if you just parse the patch using the diff parser
> we have in libsvn_diff? Would that be enough?
> See this file for details:
> http://svn.apache.org/repos/asf/subversion/trunk/subversion/include/private/svn_diff_private.h
> We could make those functions public if you want.
Yes, those functions and structs would suffice to get all the target
paths of the patch file.
> Another thing: Why do you need to apply the patch to some tempfile?
> Why is the preview of the patched result really necessary?
> Why not just show the user the patch? When reviewing what a patch
> does, it's much easier to look at the patch itself rather than the
> patched result -- at least from my point of view, I don't know if your
> users are different.
>
> If you agree to these points, we could drop the svn_client_patch() API
> change again.
Reviewing a patch file is not what most users expect and are familiar
with. Especially if there are many changes spread through a big file,
it's not enough to just see the patch file with the three context lines
to really get what the changes do - you have to see the full file with
all the changes.
Applying the patch to a temp file allows me to show the changes in
TortoiseMerge, the old and new file side-by-side. That's what users are
used to and where they can really see the result.
Also, if you just review the patch file itself, there's no guarantee
that when the patch is applied that that result is what you expect:
depending on the algorithm to find the position of where to apply a
hunk, such a hunk can get applied somewhere you didn't expect. So a
'real' preview is necessary.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Mar 10, 2010 at 08:58:24PM +0100, Stefan Küng wrote:
> On 10.03.2010 19:14, Julian Foad wrote:
>
> >>Yes, I would also need to know which paths the patch wants to modify,
> >>and then later I want to tell the API which of those paths it should
> >>actually modify.
> >>This is to let the user choose which paths to modify and which ones not.
> >
> >OK, well the issue #3434 didn't mention that. Since you already have a
> >patch implementation which does what you want, could you show us the API
> >definition (at least) for this part of it? That would help a lot, since
> >then we wouldn't have to guess about what you want :-)
>
> It's basically a class with methods like:
> * ParsePatchfile
> * GetCount //returns the number of paths affected
> * GetPath(int index) // returns the affected path
> * PatchPath(int index, TCHAR * target) // applies the patch for the
> path and saves the result in 'target'
> The names of the class methods are actually a little bit different,
> but that's pretty much what we have right now and what we need.
What about if you just parse the patch using the diff parser
we have in libsvn_diff? Would that be enough?
See this file for details:
http://svn.apache.org/repos/asf/subversion/trunk/subversion/include/private/svn_diff_private.h
We could make those functions public if you want.
Another thing: Why do you need to apply the patch to some tempfile?
Why is the preview of the patched result really necessary?
Why not just show the user the patch? When reviewing what a patch
does, it's much easier to look at the patch itself rather than the
patched result -- at least from my point of view, I don't know if your
users are different.
If you agree to these points, we could drop the svn_client_patch() API
change again.
Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Mar 10, 2010 at 08:58:24PM +0100, Stefan Küng wrote:
> On 10.03.2010 19:14, Julian Foad wrote:
>
> >>Yes, I would also need to know which paths the patch wants to modify,
> >>and then later I want to tell the API which of those paths it should
> >>actually modify.
> >>This is to let the user choose which paths to modify and which ones not.
> >
> >OK, well the issue #3434 didn't mention that. Since you already have a
> >patch implementation which does what you want, could you show us the API
> >definition (at least) for this part of it? That would help a lot, since
> >then we wouldn't have to guess about what you want :-)
>
> It's basically a class with methods like:
> * ParsePatchfile
> * GetCount //returns the number of paths affected
> * GetPath(int index) // returns the affected path
> * PatchPath(int index, TCHAR * target) // applies the patch for the
> path and saves the result in 'target'
> The names of the class methods are actually a little bit different,
> but that's pretty much what we have right now and what we need.
What about if you just parse the patch using the diff parser
we have in libsvn_diff? Would that be enough?
See this file for details:
http://svn.apache.org/repos/asf/subversion/trunk/subversion/include/private/svn_diff_private.h
We could make those functions public if you want.
Another thing: Why do you need to apply the patch to some tempfile?
Why is the preview of the patched result really necessary?
Why not just show the user the patch? When reviewing what a patch
does, it's much easier to look at the patch itself rather than the
patched result -- at least from my point of view, I don't know if your
users are different.
If you agree to these points, we could drop the svn_client_patch() API
change again.
Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Küng <to...@gmail.com>.
On 10.03.2010 19:14, Julian Foad wrote:
>> Yes, I would also need to know which paths the patch wants to modify,
>> and then later I want to tell the API which of those paths it should
>> actually modify.
>> This is to let the user choose which paths to modify and which ones not.
>
> OK, well the issue #3434 didn't mention that. Since you already have a
> patch implementation which does what you want, could you show us the API
> definition (at least) for this part of it? That would help a lot, since
> then we wouldn't have to guess about what you want :-)
It's basically a class with methods like:
* ParsePatchfile
* GetCount //returns the number of paths affected
* GetPath(int index) // returns the affected path
* PatchPath(int index, TCHAR * target) // applies the patch for the path
and saves the result in 'target'
The names of the class methods are actually a little bit different, but
that's pretty much what we have right now and what we need.
Of course, it might not be possible for the svn patch to use indexes to
specify the affected path, but passing in the path would work too (if
there's a way to get a list of those paths).
>
>> And it would be great if I could tell where to save the result, i.e. not
>> having the patch applied to the target file but to a copy of that file
>> instead so I can show the user a preview of the result without actually
>> modifying the real file yet.
>
> Again, please suggest an API for this. I know we could design it
> ourselves, but it'll be great if you can save us some time!
See above the "PatchPath(int index, TCHAR * target)" method.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r919460 - filtering svn patch targets
Posted by Julian Foad <ju...@wandisco.com>.
Stefan Küng wrote:
> On 10.03.2010 17:23, Julian Foad wrote:
> > Hi, Stefan and Stefan.
> >
> > Stefan Sperling wrote:
> >> On Wed, Mar 10, 2010 at 01:19:37PM +0000, Julian Foad wrote:
> >>> Stefan Sperling wrote:
> >>>> On Wed, Mar 10, 2010 at 10:36:13AM +0000, Julian Foad wrote:
> >>>>> This patch appears to have the filter reversed: it lets the user specify
> >>>>> paths that the filter should REMOVE, which IMO is not so useful.
> >>>>
> >>>> Why isn't that useful? I think either way (include and exclude) is useful.
> >
> > (Re. issue #3434
> > <http://subversion.tigris.org/issues/show_bug.cgi?id=3434>)...
> >>> I understand that TortoiseSVN wants the include/exclude functionality
> >>> available at the API. I wonder if "supply a list of patterns to
> >>> include" is really the best API. If I were writing a GUI that wanted to
> >>> show a list of what the patch is about to patch, I would want an API
> >>> that told me, first of all, what are the paths that this patch file is
> >>> going to patch. Maybe a callback that says: "I'm about to patch the
> >>> target file TARGET_ABSPATH. If you want, you can set TARGET_ABSPATH to a
> >>> different target, or set it to NULL if you don't want to apply this
> >>> file-patch at all." Using that, I could either get a list of all
> >>> targets in the patch (and make it a dry-run by returning TARGET_ABSPATH
> >>> = NULL on each callback), or control which targets are applied, by any
> >>> pattern- or list-matching scheme I like, or apply to different paths, or
> >>> just apply the patch normally by not providing a callback function at
> >>> all, all with the same simple API.
> >>
> >> The requirements as given by the TortoiseSVN developers were "I want
> >> to pass a list of paths the patch is going to modify, the rest of
> >> the paths should be ignored by the patching process".
> >
> > Yup.
> >
> > (Stefan K, is that really what you want? You don't need to know what
> > paths are in the patch, for example?)
Stefan, thanks for your feedback. This is really useful information.
> Yes, I would also need to know which paths the patch wants to modify,
> and then later I want to tell the API which of those paths it should
> actually modify.
> This is to let the user choose which paths to modify and which ones not.
OK, well the issue #3434 didn't mention that. Since you already have a
patch implementation which does what you want, could you show us the API
definition (at least) for this part of it? That would help a lot, since
then we wouldn't have to guess about what you want :-)
> And it would be great if I could tell where to save the result, i.e. not
> having the patch applied to the target file but to a copy of that file
> instead so I can show the user a preview of the result without actually
> modifying the real file yet.
Again, please suggest an API for this. I know we could design it
ourselves, but it'll be great if you can save us some time!
> >>> Is there a real practical need to have the include/exclude functionality
> >>> in "svn" as well? I can't think of a use that's important enough to
> >>> justify it. Can you?
>
> Yes: there are a lot of svn clients out there which could benefit from this.
In this sentence I'm asking about the "svn" CLI specifically. It doesn't
offer a preview checklist.
> >> First and foremost, it allows us to test this feature independently
> >> of TortoiseSVN. That alone of course does not justify exposing the
> >> feature at the CLI level. However, since TortoiseSVN users obviously
> >> see the need for this feature (assuming the TortoiseSVN devs didn't
> >> just make it up and nobody is actually using this feature of TortoiseSVN),
> >> why should we not provide it in the CLI client as well?
> >
> > The usual reasons. Because lots of GUI features are inappropriate in a
> > CLI, and this might well be one such. Because well designed software
> > doesn't provide features just because somebody wanted such features.
>
> As one of the TSVN devs I can assure you that we didn't just make this
> requirement up: TSVN currently does exactly this, but of course with its
> own implementation of 'patch' which isn't that good and only works if
> the patch applies cleanly.
>
> >>> GNU patch doesn't have it (and it doesn't have an
> >>> "include-only" filter either).
> >>
> >> Good point. But following that rationale I'd rather drop the feature
> >> entirely, even at the API level.
> >
> > I wouldn't discourage dropping it if Stefan K would be happy with that.
>
> Just because GNU patch doesn't have a feature doesn't mean the feature
> isn't useful.
Correct, but it does indicate that it's quite likely not. (Again, I'm
talking CLI only.)
> And there's a reason why GNU patch is rarely used on Windows: it's
> unusable for most situations.
>
>
> >> Well, for me the question boils down to:
> >>
> >> Do we want the --exclude-patterns and --include-patterns options,
> >> and if not, what do we tell the TortoiseSVN (and AnkhSVN) devs about
> >> their requirement that svn patch should be able to patch targets
> >> selectively?
> >
> > All I know about what they want is what Stefan K wrote in issue #3434:
> > "svn_client_patch() should have a 'filter' parameter, specifiying one
> > (or more?) paths that will get patched." Nothing more, nothing less.
> >
> > We seem to be on a roll of adding new command-line options, which are
> > undoubtedly useful but I feel it is contrary to one of Subversion's
> > original design principles (small command set), and that's making me
> > slightly edgy.
>
> Well, all I can say is that without this feature, I can't use the patch
> command from the svn library in TortoiseMerge. I would have to keep the
> sub-par patch TMerge already has instead, because it has that feature
> that is required for previews. And previews are important. Users are
> very uncomfortable to apply changes without first knowing what those
> changes are and where they go to (i.e., which files are affected).
>
> Sure, if you do an update you also won't know beforehand what changes
> are applied to your working copy (unless you run 'svn st -u').
> But there you know the people who have commit access. In case of patch
> files you don't really know the people who sent you a patch for
> something. So reviewing the patch before you apply it to your working
> copy (which most likely has local modifications, so reverting isn't
> really an option either).
Thanks.
- Julian
Re: svn commit: r919460 - filtering svn patch targets
Posted by Julian Foad <ju...@wandisco.com>.
On Mon, 2010-03-29 at 19:42 +0200, Stefan Küng wrote:
> On 29.03.2010 15:08, Julian Foad wrote:
> > On Sat, 2010-03-27, Stefan Küng wrote:
> >> On 27.03.2010 15:23, Stefan Sperling wrote:
> >>> On Fri, Mar 26, 2010 at 07:57:09PM +0100, Stefan Küng wrote:
> >>>> On 26.03.2010 18:17, Stefan Sperling wrote:
> >>>>> Can you check if the current API matches your requirements?
> >>>>> See subversion/tests/libsvn_client/client-test.c for a trivial example.
> >>>>
> >>>> Looks very good! Thanks a lot for implementing this.
> >>>
> >>> Great! You're welcome.
> >>>
> >>>> Another thought, not sure if it would make sense or not:
> >>>> would an option to ignore whitespace changes make sense?
> >>>
> >>> Yes, I guess we could easily make the line-matching ignore whitespace.
> >>> Matching is currently done using a simple strcmp(), so it's fairly naive.
> >>>
> >>> Please file an enhancement issue, with undefined schedule (if someone
> >>> wants to pick this up before 1.7.0, that's fine -- but it's not a
> >>> critical feature).
> >>>
> >>> Ignoring whitespace would help dealing with diffs messed up by e.g. gmail.
> >>> Though I've seen gmail use some magic UTF-8 sequences for whitespace,
> >>> which is really annoying and which a naive approach like ignoring
> >>> single-byte characters like ' ', '\t', etc. may not cope with.
> >>
> >> Filed as issue 3610:
> >> http://subversion.tigris.org/issues/show_bug.cgi?id=3610
> >
> > Can I recommend you add more detail to the request. "Ignore whitespace
> > changes" could be interpreted in many ways, and defining what exactly
> > you mean is a big part of the issue.
> >
> > For example, one change that some mail clients make is to remove a space
> > from the beginning of every line that starts with a space (or starts
> > with two spaces?) so that (using "_" to represent a space):
> >
> > __context-line
> > __context-line
> > __context-line
> > +_plus-line
> > -_minus-line
> > __context-line
> > __context-line
> > __context-line
> >
> > has turned into
> >
> > _context-line
> > _context-line
> > _context-line
> > +_plus-line
> > -_minus-line
> > _context-line
> > _context-line
> > _context-line
> >
> > when the mail is received.
> >
> > If the extension we want is just for that, then
> >
> > "Allow a context line to match a real line that has an extra space at
> > the beginning"
> >
> > is a sufficient definition, whereas
> >
> > "Allow any sequence of whitespace (including an empty line) in the
> > context lines and minus lines to match any sequence of whitespace
> > (including an empty line) in the target file"
> >
> > is a very different requirement.
> >
> > And/or do you want
> >
> > "When a hunk performs only whitespace changes, ignore that hunk"
> >
> > ?
>
> I've updated the issue with more information about what I was thinking
> how it should work.
Thanks.
Can I further recommend that the purpose of the feature should be to try
to match context lines and 'minus' lines against the target file,
ignoring those kinds of white space differences, and to insert 'plus'
lines exactly as they are received in the patch, and *not* try to guess
what combination of white space the 'plus' lines should have. (I am
convinced that any scheme for trying to guess what the 'plus' lines
should look like could only work in a very limited set of cases, and
would make changes that are not wanted in other cases.)
- Julian
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Küng <to...@gmail.com>.
On 29.03.2010 15:08, Julian Foad wrote:
> On Sat, 2010-03-27, Stefan Küng wrote:
>> On 27.03.2010 15:23, Stefan Sperling wrote:
>>> On Fri, Mar 26, 2010 at 07:57:09PM +0100, Stefan Küng wrote:
>>>> On 26.03.2010 18:17, Stefan Sperling wrote:
>>>>> Can you check if the current API matches your requirements?
>>>>> See subversion/tests/libsvn_client/client-test.c for a trivial example.
>>>>
>>>> Looks very good! Thanks a lot for implementing this.
>>>
>>> Great! You're welcome.
>>>
>>>> Another thought, not sure if it would make sense or not:
>>>> would an option to ignore whitespace changes make sense?
>>>
>>> Yes, I guess we could easily make the line-matching ignore whitespace.
>>> Matching is currently done using a simple strcmp(), so it's fairly naive.
>>>
>>> Please file an enhancement issue, with undefined schedule (if someone
>>> wants to pick this up before 1.7.0, that's fine -- but it's not a
>>> critical feature).
>>>
>>> Ignoring whitespace would help dealing with diffs messed up by e.g. gmail.
>>> Though I've seen gmail use some magic UTF-8 sequences for whitespace,
>>> which is really annoying and which a naive approach like ignoring
>>> single-byte characters like ' ', '\t', etc. may not cope with.
>>
>> Filed as issue 3610:
>> http://subversion.tigris.org/issues/show_bug.cgi?id=3610
>
> Can I recommend you add more detail to the request. "Ignore whitespace
> changes" could be interpreted in many ways, and defining what exactly
> you mean is a big part of the issue.
>
> For example, one change that some mail clients make is to remove a space
> from the beginning of every line that starts with a space (or starts
> with two spaces?) so that (using "_" to represent a space):
>
> __context-line
> __context-line
> __context-line
> +_plus-line
> -_minus-line
> __context-line
> __context-line
> __context-line
>
> has turned into
>
> _context-line
> _context-line
> _context-line
> +_plus-line
> -_minus-line
> _context-line
> _context-line
> _context-line
>
> when the mail is received.
>
> If the extension we want is just for that, then
>
> "Allow a context line to match a real line that has an extra space at
> the beginning"
>
> is a sufficient definition, whereas
>
> "Allow any sequence of whitespace (including an empty line) in the
> context lines and minus lines to match any sequence of whitespace
> (including an empty line) in the target file"
>
> is a very different requirement.
>
> And/or do you want
>
> "When a hunk performs only whitespace changes, ignore that hunk"
>
> ?
I've updated the issue with more information about what I was thinking
how it should work.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r919460 - filtering svn patch targets
Posted by Julian Foad <ju...@wandisco.com>.
On Sat, 2010-03-27, Stefan Küng wrote:
> On 27.03.2010 15:23, Stefan Sperling wrote:
> > On Fri, Mar 26, 2010 at 07:57:09PM +0100, Stefan Küng wrote:
> >> On 26.03.2010 18:17, Stefan Sperling wrote:
> >>> Can you check if the current API matches your requirements?
> >>> See subversion/tests/libsvn_client/client-test.c for a trivial example.
> >>
> >> Looks very good! Thanks a lot for implementing this.
> >
> > Great! You're welcome.
> >
> >> Another thought, not sure if it would make sense or not:
> >> would an option to ignore whitespace changes make sense?
> >
> > Yes, I guess we could easily make the line-matching ignore whitespace.
> > Matching is currently done using a simple strcmp(), so it's fairly naive.
> >
> > Please file an enhancement issue, with undefined schedule (if someone
> > wants to pick this up before 1.7.0, that's fine -- but it's not a
> > critical feature).
> >
> > Ignoring whitespace would help dealing with diffs messed up by e.g. gmail.
> > Though I've seen gmail use some magic UTF-8 sequences for whitespace,
> > which is really annoying and which a naive approach like ignoring
> > single-byte characters like ' ', '\t', etc. may not cope with.
>
> Filed as issue 3610:
> http://subversion.tigris.org/issues/show_bug.cgi?id=3610
Can I recommend you add more detail to the request. "Ignore whitespace
changes" could be interpreted in many ways, and defining what exactly
you mean is a big part of the issue.
For example, one change that some mail clients make is to remove a space
from the beginning of every line that starts with a space (or starts
with two spaces?) so that (using "_" to represent a space):
__context-line
__context-line
__context-line
+_plus-line
-_minus-line
__context-line
__context-line
__context-line
has turned into
_context-line
_context-line
_context-line
+_plus-line
-_minus-line
_context-line
_context-line
_context-line
when the mail is received.
If the extension we want is just for that, then
"Allow a context line to match a real line that has an extra space at
the beginning"
is a sufficient definition, whereas
"Allow any sequence of whitespace (including an empty line) in the
context lines and minus lines to match any sequence of whitespace
(including an empty line) in the target file"
is a very different requirement.
And/or do you want
"When a hunk performs only whitespace changes, ignore that hunk"
?
- Julian
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Küng <to...@gmail.com>.
On 27.03.2010 15:23, Stefan Sperling wrote:
> On Fri, Mar 26, 2010 at 07:57:09PM +0100, Stefan Küng wrote:
>> On 26.03.2010 18:17, Stefan Sperling wrote:
>>> Can you check if the current API matches your requirements?
>>> See subversion/tests/libsvn_client/client-test.c for a trivial example.
>>
>> Looks very good! Thanks a lot for implementing this.
>
> Great! You're welcome.
>
>> Another thought, not sure if it would make sense or not:
>> would an option to ignore whitespace changes make sense?
>
> Yes, I guess we could easily make the line-matching ignore whitespace.
> Matching is currently done using a simple strcmp(), so it's fairly naive.
>
> Please file an enhancement issue, with undefined schedule (if someone
> wants to pick this up before 1.7.0, that's fine -- but it's not a
> critical feature).
>
> Ignoring whitespace would help dealing with diffs messed up by e.g. gmail.
> Though I've seen gmail use some magic UTF-8 sequences for whitespace,
> which is really annoying and which a naive approach like ignoring
> single-byte characters like ' ', '\t', etc. may not cope with.
Filed as issue 3610:
http://subversion.tigris.org/issues/show_bug.cgi?id=3610
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Küng <to...@gmail.com>.
On 27.03.2010 15:23, Stefan Sperling wrote:
> On Fri, Mar 26, 2010 at 07:57:09PM +0100, Stefan Küng wrote:
>> On 26.03.2010 18:17, Stefan Sperling wrote:
>>> Can you check if the current API matches your requirements?
>>> See subversion/tests/libsvn_client/client-test.c for a trivial example.
>>
>> Looks very good! Thanks a lot for implementing this.
>
> Great! You're welcome.
>
>> Another thought, not sure if it would make sense or not:
>> would an option to ignore whitespace changes make sense?
>
> Yes, I guess we could easily make the line-matching ignore whitespace.
> Matching is currently done using a simple strcmp(), so it's fairly naive.
>
> Please file an enhancement issue, with undefined schedule (if someone
> wants to pick this up before 1.7.0, that's fine -- but it's not a
> critical feature).
>
> Ignoring whitespace would help dealing with diffs messed up by e.g. gmail.
> Though I've seen gmail use some magic UTF-8 sequences for whitespace,
> which is really annoying and which a naive approach like ignoring
> single-byte characters like ' ', '\t', etc. may not cope with.
Filed as issue 3610:
http://subversion.tigris.org/issues/show_bug.cgi?id=3610
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Mar 26, 2010 at 07:57:09PM +0100, Stefan Küng wrote:
> On 26.03.2010 18:17, Stefan Sperling wrote:
> >Can you check if the current API matches your requirements?
> >See subversion/tests/libsvn_client/client-test.c for a trivial example.
>
> Looks very good! Thanks a lot for implementing this.
Great! You're welcome.
> Another thought, not sure if it would make sense or not:
> would an option to ignore whitespace changes make sense?
Yes, I guess we could easily make the line-matching ignore whitespace.
Matching is currently done using a simple strcmp(), so it's fairly naive.
Please file an enhancement issue, with undefined schedule (if someone
wants to pick this up before 1.7.0, that's fine -- but it's not a
critical feature).
Ignoring whitespace would help dealing with diffs messed up by e.g. gmail.
Though I've seen gmail use some magic UTF-8 sequences for whitespace,
which is really annoying and which a naive approach like ignoring
single-byte characters like ' ', '\t', etc. may not cope with.
Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Mar 26, 2010 at 07:57:09PM +0100, Stefan Küng wrote:
> On 26.03.2010 18:17, Stefan Sperling wrote:
> >Can you check if the current API matches your requirements?
> >See subversion/tests/libsvn_client/client-test.c for a trivial example.
>
> Looks very good! Thanks a lot for implementing this.
Great! You're welcome.
> Another thought, not sure if it would make sense or not:
> would an option to ignore whitespace changes make sense?
Yes, I guess we could easily make the line-matching ignore whitespace.
Matching is currently done using a simple strcmp(), so it's fairly naive.
Please file an enhancement issue, with undefined schedule (if someone
wants to pick this up before 1.7.0, that's fine -- but it's not a
critical feature).
Ignoring whitespace would help dealing with diffs messed up by e.g. gmail.
Though I've seen gmail use some magic UTF-8 sequences for whitespace,
which is really annoying and which a naive approach like ignoring
single-byte characters like ' ', '\t', etc. may not cope with.
Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Küng <to...@gmail.com>.
On 26.03.2010 18:17, Stefan Sperling wrote:
> On Wed, Mar 10, 2010 at 06:56:12PM +0100, Stefan Küng wrote:
>> And it would be great if I could tell where to save the result, i.e.
>> not having the patch applied to the target file but to a copy of
>> that file instead so I can show the user a preview of the result
>> without actually modifying the real file yet.
>
> This should be possible in trunk now, by passing the new patched_tempfiles
> and reject_tempfiles parameters and setting dry_run to TRUE.
>
> Can you check if the current API matches your requirements?
> See subversion/tests/libsvn_client/client-test.c for a trivial example.
Looks very good! Thanks a lot for implementing this.
Another thought, not sure if it would make sense or not:
would an option to ignore whitespace changes make sense?
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Küng <to...@gmail.com>.
On 26.03.2010 18:17, Stefan Sperling wrote:
> On Wed, Mar 10, 2010 at 06:56:12PM +0100, Stefan Küng wrote:
>> And it would be great if I could tell where to save the result, i.e.
>> not having the patch applied to the target file but to a copy of
>> that file instead so I can show the user a preview of the result
>> without actually modifying the real file yet.
>
> This should be possible in trunk now, by passing the new patched_tempfiles
> and reject_tempfiles parameters and setting dry_run to TRUE.
>
> Can you check if the current API matches your requirements?
> See subversion/tests/libsvn_client/client-test.c for a trivial example.
Looks very good! Thanks a lot for implementing this.
Another thought, not sure if it would make sense or not:
would an option to ignore whitespace changes make sense?
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Mar 10, 2010 at 06:56:12PM +0100, Stefan Küng wrote:
> And it would be great if I could tell where to save the result, i.e.
> not having the patch applied to the target file but to a copy of
> that file instead so I can show the user a preview of the result
> without actually modifying the real file yet.
This should be possible in trunk now, by passing the new patched_tempfiles
and reject_tempfiles parameters and setting dry_run to TRUE.
Can you check if the current API matches your requirements?
See subversion/tests/libsvn_client/client-test.c for a trivial example.
Thanks,
Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Mar 10, 2010 at 06:56:12PM +0100, Stefan Küng wrote:
> And it would be great if I could tell where to save the result, i.e.
> not having the patch applied to the target file but to a copy of
> that file instead so I can show the user a preview of the result
> without actually modifying the real file yet.
This should be possible in trunk now, by passing the new patched_tempfiles
and reject_tempfiles parameters and setting dry_run to TRUE.
Can you check if the current API matches your requirements?
See subversion/tests/libsvn_client/client-test.c for a trivial example.
Thanks,
Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Küng <to...@gmail.com>.
On 10.03.2010 17:23, Julian Foad wrote:
> Hi, Stefan and Stefan.
>
> Stefan Sperling wrote:
>> On Wed, Mar 10, 2010 at 01:19:37PM +0000, Julian Foad wrote:
>>> Stefan Sperling wrote:
>>>> On Wed, Mar 10, 2010 at 10:36:13AM +0000, Julian Foad wrote:
>>>>> This patch appears to have the filter reversed: it lets the user specify
>>>>> paths that the filter should REMOVE, which IMO is not so useful.
>>>>
>>>> Why isn't that useful? I think either way (include and exclude) is useful.
>
> (Re. issue #3434
> <http://subversion.tigris.org/issues/show_bug.cgi?id=3434>)...
>>> I understand that TortoiseSVN wants the include/exclude functionality
>>> available at the API. I wonder if "supply a list of patterns to
>>> include" is really the best API. If I were writing a GUI that wanted to
>>> show a list of what the patch is about to patch, I would want an API
>>> that told me, first of all, what are the paths that this patch file is
>>> going to patch. Maybe a callback that says: "I'm about to patch the
>>> target file TARGET_ABSPATH. If you want, you can set TARGET_ABSPATH to a
>>> different target, or set it to NULL if you don't want to apply this
>>> file-patch at all." Using that, I could either get a list of all
>>> targets in the patch (and make it a dry-run by returning TARGET_ABSPATH
>>> = NULL on each callback), or control which targets are applied, by any
>>> pattern- or list-matching scheme I like, or apply to different paths, or
>>> just apply the patch normally by not providing a callback function at
>>> all, all with the same simple API.
>>
>> The requirements as given by the TortoiseSVN developers were "I want
>> to pass a list of paths the patch is going to modify, the rest of
>> the paths should be ignored by the patching process".
>
> Yup.
>
> (Stefan K, is that really what you want? You don't need to know what
> paths are in the patch, for example?)
Yes, I would also need to know which paths the patch wants to modify,
and then later I want to tell the API which of those paths it should
actually modify.
This is to let the user choose which paths to modify and which ones not.
And it would be great if I could tell where to save the result, i.e. not
having the patch applied to the target file but to a copy of that file
instead so I can show the user a preview of the result without actually
modifying the real file yet.
>>> Is there a real practical need to have the include/exclude functionality
>>> in "svn" as well? I can't think of a use that's important enough to
>>> justify it. Can you?
Yes: there are a lot of svn clients out there which could benefit from this.
>> First and foremost, it allows us to test this feature independently
>> of TortoiseSVN. That alone of course does not justify exposing the
>> feature at the CLI level. However, since TortoiseSVN users obviously
>> see the need for this feature (assuming the TortoiseSVN devs didn't
>> just make it up and nobody is actually using this feature of TortoiseSVN),
>> why should we not provide it in the CLI client as well?
>
> The usual reasons. Because lots of GUI features are inappropriate in a
> CLI, and this might well be one such. Because well designed software
> doesn't provide features just because somebody wanted such features.
As one of the TSVN devs I can assure you that we didn't just make this
requirement up: TSVN currently does exactly this, but of course with its
own implementation of 'patch' which isn't that good and only works if
the patch applies cleanly.
>>> GNU patch doesn't have it (and it doesn't have an
>>> "include-only" filter either).
>>
>> Good point. But following that rationale I'd rather drop the feature
>> entirely, even at the API level.
>
> I wouldn't discourage dropping it if Stefan K would be happy with that.
Just because GNU patch doesn't have a feature doesn't mean the feature
isn't useful.
And there's a reason why GNU patch is rarely used on Windows: it's
unusable for most situations.
>> Well, for me the question boils down to:
>>
>> Do we want the --exclude-patterns and --include-patterns options,
>> and if not, what do we tell the TortoiseSVN (and AnkhSVN) devs about
>> their requirement that svn patch should be able to patch targets
>> selectively?
>
> All I know about what they want is what Stefan K wrote in issue #3434:
> "svn_client_patch() should have a 'filter' parameter, specifiying one
> (or more?) paths that will get patched." Nothing more, nothing less.
>
> We seem to be on a roll of adding new command-line options, which are
> undoubtedly useful but I feel it is contrary to one of Subversion's
> original design principles (small command set), and that's making me
> slightly edgy.
Well, all I can say is that without this feature, I can't use the patch
command from the svn library in TortoiseMerge. I would have to keep the
sub-par patch TMerge already has instead, because it has that feature
that is required for previews. And previews are important. Users are
very uncomfortable to apply changes without first knowing what those
changes are and where they go to (i.e., which files are affected).
Sure, if you do an update you also won't know beforehand what changes
are applied to your working copy (unless you run 'svn st -u').
But there you know the people who have commit access. In case of patch
files you don't really know the people who sent you a patch for
something. So reviewing the patch before you apply it to your working
copy (which most likely has local modifications, so reverting isn't
really an option either).
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r919460 - filtering svn patch targets
Posted by Julian Foad <ju...@wandisco.com>.
Hi, Stefan and Stefan.
Stefan Sperling wrote:
> On Wed, Mar 10, 2010 at 01:19:37PM +0000, Julian Foad wrote:
> > Stefan Sperling wrote:
> > > On Wed, Mar 10, 2010 at 10:36:13AM +0000, Julian Foad wrote:
> > > > This patch appears to have the filter reversed: it lets the user specify
> > > > paths that the filter should REMOVE, which IMO is not so useful.
> > >
> > > Why isn't that useful? I think either way (include and exclude) is useful.
(Re. issue #3434
<http://subversion.tigris.org/issues/show_bug.cgi?id=3434>)...
> > I understand that TortoiseSVN wants the include/exclude functionality
> > available at the API. I wonder if "supply a list of patterns to
> > include" is really the best API. If I were writing a GUI that wanted to
> > show a list of what the patch is about to patch, I would want an API
> > that told me, first of all, what are the paths that this patch file is
> > going to patch. Maybe a callback that says: "I'm about to patch the
> > target file TARGET_ABSPATH. If you want, you can set TARGET_ABSPATH to a
> > different target, or set it to NULL if you don't want to apply this
> > file-patch at all." Using that, I could either get a list of all
> > targets in the patch (and make it a dry-run by returning TARGET_ABSPATH
> > = NULL on each callback), or control which targets are applied, by any
> > pattern- or list-matching scheme I like, or apply to different paths, or
> > just apply the patch normally by not providing a callback function at
> > all, all with the same simple API.
>
> The requirements as given by the TortoiseSVN developers were "I want
> to pass a list of paths the patch is going to modify, the rest of
> the paths should be ignored by the patching process".
Yup.
(Stefan K, is that really what you want? You don't need to know what
paths are in the patch, for example?)
> Because of the convoluted wording of the issue (they used the word
> "filter") I implemented filtering functionality rather than the
> include list, but adding the include list is trivial.
>
> I extended their requierements so that the list also supports globbing,
> such that it's easier to use from the svn CLI client. Note that the globbing
> is of course optional so passing literal paths (like the TortoiseSVN
> devs want to do) works fine.
If this application were a patch program, then such extensions would be
right on track. Now, it's definitely useful for a version control
program to be able to apply patches, and will be especially useful one
day when it can apply tree-change and property-change patches. But
we're having such a hard job just maintaining the version-control parts
of Subversion that I get uncomfortable when we start to extend non-core
features beyond the essentials.
To be frank, I am heartily looking forward to the day when we can have
your expertise focused back on tree conflicts, WC-NG, merging or such
like.
> > Is there a real practical need to have the include/exclude functionality
> > in "svn" as well? I can't think of a use that's important enough to
> > justify it. Can you?
>
> First and foremost, it allows us to test this feature independently
> of TortoiseSVN. That alone of course does not justify exposing the
> feature at the CLI level. However, since TortoiseSVN users obviously
> see the need for this feature (assuming the TortoiseSVN devs didn't
> just make it up and nobody is actually using this feature of TortoiseSVN),
> why should we not provide it in the CLI client as well?
The usual reasons. Because lots of GUI features are inappropriate in a
CLI, and this might well be one such. Because well designed software
doesn't provide features just because somebody wanted such features.
> > GNU patch doesn't have it (and it doesn't have an
> > "include-only" filter either).
>
> Good point. But following that rationale I'd rather drop the feature
> entirely, even at the API level.
I wouldn't discourage dropping it if Stefan K would be happy with that.
> > > My plan is to add an --include-pattern option (at the CLI level as well
> > > as the API level), and if both --include-pattern and --exclude-pattern
> > > options have been specified, have the --exclude-pattern option operate on
> > > any files left after one or more --include-pattern options were used to
> > > limit the patch target list to a given list of files.
> >
> > I just don't want us to make it more flexible than it needs to be.
> >
> > Have you thought about consistency with other svn commands? How does
> > this relate to "--targets="?
>
> That's irrelevant right now. 'svn patch' does not support --targets
> and no subcommand but svn patch supports --exclude-patterns. But there
> is no reason why --targets and --exclude-patterns could not co-exist
> within the same subcommand. The exclude patterns would simply act as
> a filter of the lines contained in the file passed to --targets.
>
> > To the "ignore patterns"?
> > Are the glob patterns using the same syntax as auto-props?
>
> The --exclude-patterns, auto-props and svn:ignore all use apr_fnmatch()
> to do pattern matching. They work the same.
Great.
> > > I'm also planning on renaming the 'glob_filter' parameter of the API
> > > to 'exclude_patterns' so it matches the terminology used by the CLI
> > > and does not say 'filter' anymore.
> >
> > That sounds good.
>
> Well, for me the question boils down to:
>
> Do we want the --exclude-patterns and --include-patterns options,
> and if not, what do we tell the TortoiseSVN (and AnkhSVN) devs about
> their requirement that svn patch should be able to patch targets
> selectively?
All I know about what they want is what Stefan K wrote in issue #3434:
"svn_client_patch() should have a 'filter' parameter, specifiying one
(or more?) paths that will get patched." Nothing more, nothing less.
We seem to be on a roll of adding new command-line options, which are
undoubtedly useful but I feel it is contrary to one of Subversion's
original design principles (small command set), and that's making me
slightly edgy.
> We could also implement your proposal, but it seems that TortoiseSVN
> and AnkhSVN devs are happy with where we're currently heading, and
> they are the ones who brought up this requirement originally.
> I certainly would not have added this feature if they hadn't requested it.
> If you want to convince them to adapt their requirements to some
> callback-driven way of doing this, that's fine by me. But I'm happy
> with --exclude-patterns and --include-patterns, so I'm not gonna
> push for a different way of doing this.
Fair point. I suggested it as a request for comments, not as a request
for re-doing what you've done.
- Julian
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Mar 10, 2010 at 01:19:37PM +0000, Julian Foad wrote:
> Stefan Sperling wrote:
> > On Wed, Mar 10, 2010 at 10:36:13AM +0000, Julian Foad wrote:
> > > This patch appears to have the filter reversed: it lets the user specify
> > > paths that the filter should REMOVE, which IMO is not so useful.
> >
> > Why isn't that useful? I think either way (include and exclude) is useful.
>
> I understand that TortoiseSVN wants the include/exclude functionality
> available at the API. I wonder if "supply a list of patterns to
> include" is really the best API. If I were writing a GUI that wanted to
> show a list of what the patch is about to patch, I would want an API
> that told me, first of all, what are the paths that this patch file is
> going to patch. Maybe a callback that says: "I'm about to patch the
> target file TARGET_ABSPATH. If you want, you can set TARGET_ABSPATH to a
> different target, or set it to NULL if you don't want to apply this
> file-patch at all." Using that, I could either get a list of all
> targets in the patch (and make it a dry-run by returning TARGET_ABSPATH
> = NULL on each callback), or control which targets are applied, by any
> pattern- or list-matching scheme I like, or apply to different paths, or
> just apply the patch normally by not providing a callback function at
> all, all with the same simple API.
The requirements as given by the TortoiseSVN developers were "I want
to pass a list of paths the patch is going to modify, the rest of
the paths should be ignored by the patching process".
Because of the convoluted wording of the issue (they used the word
"filter") I implemented filtering functionality rather than the
include list, but adding the include list is trivial.
I extended their requierements so that the list also supports globbing,
such that it's easier to use from the svn CLI client. Note that the globbing
is of course optional so passing literal paths (like the TortoiseSVN
devs want to do) works fine.
> Is there a real practical need to have the include/exclude functionality
> in "svn" as well? I can't think of a use that's important enough to
> justify it. Can you?
First and foremost, it allows us to test this feature independently
of TortoiseSVN. That alone of course does not justify exposing the
feature at the CLI level. However, since TortoiseSVN users obviously
see the need for this feature (assuming the TortoiseSVN devs didn't
just make it up and nobody is actually using this feature of TortoiseSVN),
why should we not provide it in the CLI client as well?
> GNU patch doesn't have it (and it doesn't have an
> "include-only" filter either).
Good point. But following that rationale I'd rather drop the feature
entirely, even at the API level.
> > My plan is to add an --include-pattern option (at the CLI level as well
> > as the API level), and if both --include-pattern and --exclude-pattern
> > options have been specified, have the --exclude-pattern option operate on
> > any files left after one or more --include-pattern options were used to
> > limit the patch target list to a given list of files.
>
> I just don't want us to make it more flexible than it needs to be.
>
> Have you thought about consistency with other svn commands? How does
> this relate to "--targets="?
That's irrelevant right now. 'svn patch' does not support --targets
and no subcommand but svn patch supports --exclude-patterns. But there
is no reason why --targets and --exclude-patterns could not co-exist
within the same subcommand. The exclude patterns would simply act as
a filter of the lines contained in the file passed to --targets.
> To the "ignore patterns"?
> Are the glob patterns using the same syntax as auto-props?
The --exclude-patterns, auto-props and svn:ignore all use apr_fnmatch()
to do pattern matching. They work the same.
> > I'm also planning on renaming the 'glob_filter' parameter of the API
> > to 'exclude_patterns' so it matches the terminology used by the CLI
> > and does not say 'filter' anymore.
>
> That sounds good.
Well, for me the question boils down to:
Do we want the --exclude-patterns and --include-patterns options,
and if not, what do we tell the TortoiseSVN (and AnkhSVN) devs about
their requirement that svn patch should be able to patch targets
selectively?
We could also implement your proposal, but it seems that TortoiseSVN
and AnkhSVN devs are happy with where we're currently heading, and
they are the ones who brought up this requirement originally.
I certainly would not have added this feature if they hadn't requested it.
If you want to convince them to adapt their requirements to some
callback-driven way of doing this, that's fine by me. But I'm happy
with --exclude-patterns and --include-patterns, so I'm not gonna
push for a different way of doing this.
Stefan
Re: svn commit: r919460 - filtering svn patch targets
Posted by Julian Foad <ju...@wandisco.com>.
Stefan Sperling wrote:
> On Wed, Mar 10, 2010 at 10:36:13AM +0000, Julian Foad wrote:
> > This patch appears to have the filter reversed: it lets the user specify
> > paths that the filter should REMOVE, which IMO is not so useful.
>
> Why isn't that useful? I think either way (include and exclude) is useful.
I understand that TortoiseSVN wants the include/exclude functionality
available at the API. I wonder if "supply a list of patterns to
include" is really the best API. If I were writing a GUI that wanted to
show a list of what the patch is about to patch, I would want an API
that told me, first of all, what are the paths that this patch file is
going to patch. Maybe a callback that says: "I'm about to patch the
target file TARGET_ABSPATH. If you want, you can set TARGET_ABSPATH to a
different target, or set it to NULL if you don't want to apply this
file-patch at all." Using that, I could either get a list of all
targets in the patch (and make it a dry-run by returning TARGET_ABSPATH
= NULL on each callback), or control which targets are applied, by any
pattern- or list-matching scheme I like, or apply to different paths, or
just apply the patch normally by not providing a callback function at
all, all with the same simple API.
Is there a real practical need to have the include/exclude functionality
in "svn" as well? I can't think of a use that's important enough to
justify it. Can you? GNU patch doesn't have it (and it doesn't have an
"include-only" filter either).
> > (Using the word "filter" as a verb is ambiguous: it's never clear
> > whether you want to keep the big particles that the filter catches or
> > the liquid that passes through - or both, in two separate bowls.)
>
> Yes, we're aware of all this.
> See http://subversion.tigris.org/issues/show_bug.cgi?id=3599
I hadn't seen that.
> My plan is to add an --include-pattern option (at the CLI level as well
> as the API level), and if both --include-pattern and --exclude-pattern
> options have been specified, have the --exclude-pattern option operate on
> any files left after one or more --include-pattern options were used to
> limit the patch target list to a given list of files.
I just don't want us to make it more flexible than it needs to be.
Have you thought about consistency with other svn commands? How does
this relate to "--targets="? To the "ignore patterns"? Are the glob
patterns using the same syntax as auto-props?
> I'm also planning on renaming the 'glob_filter' parameter of the API
> to 'exclude_patterns' so it matches the terminology used by the CLI
> and does not say 'filter' anymore.
That sounds good.
Thanks.
- Julian
Re: svn commit: r919460 - filtering svn patch targets
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Mar 10, 2010 at 10:36:13AM +0000, Julian Foad wrote:
> On Fri, 2010-03-05, stsp@apache.org wrote:
> > Author: stsp
> > Date: Fri Mar 5 15:57:23 2010
> > New Revision: 919460
> >
> > URL: http://svn.apache.org/viewvc?rev=919460&view=rev
> > Log:
> > Fix issue #3434, "svn patch API should have a patch target filter"
>
> Hi Stefan.
>
> Issue #3434 <http://subversion.tigris.org/issues/show_bug.cgi?id=3434> "
> svn patch API should have a patch target filter" says:
>
> [[[
> svn_client_patch() should have a 'filter' parameter, specifiying one (or
> more?) paths that will get patched. Other paths that are in the svnpatch
> file would not get used if those paths don't match that filter.
>
> This allows the user to select which parts of the svnpatch file to apply
> and which ones not.
> ]]]
>
> This patch appears to have the filter reversed: it lets the user specify
> paths that the filter should REMOVE, which IMO is not so useful.
Why isn't that useful? I think either way (include and exclude) is useful.
> (Using the word "filter" as a verb is ambiguous: it's never clear
> whether you want to keep the big particles that the filter catches or
> the liquid that passes through - or both, in two separate bowls.)
Yes, we're aware of all this.
See http://subversion.tigris.org/issues/show_bug.cgi?id=3599
My plan is to add an --include-pattern option (at the CLI level as well
as the API level), and if both --include-pattern and --exclude-pattern
options have been specified, have the --exclude-pattern option operate on
any files left after one or more --include-pattern options were used to
limit the patch target list to a given list of files.
I'm also planning on renaming the 'glob_filter' parameter of the API
to 'exclude_patterns' so it matches the terminology used by the CLI
and does not say 'filter' anymore.
Stefan