Re: [PATCH] Fix issue #4049 - SEGV on patch that deletes and skips
On Wed, Nov 9, 2011 at 10:46 AM, Philip Martin
philip.mar...@wandisco.com wrote:
Daniel Näslund dan...@dannas.name writes:
Index: subversion/libsvn_client/patch.c
===
--- subversion/libsvn_client/patch.c (revision 1199347)
+++ subversion/libsvn_client/patch.c (arbetskopia)
@@ -255,6 +255,7 @@ typedef struct patch_target_t {
typedef struct patch_target_info_t {
const char *local_abspath;
svn_boolean_t deleted;
+ svn_boolean_t skipped;
} patch_target_info_t;
@@ -2730,6 +2731,11 @@ delete_empty_dirs(apr_array_header_t *targets_info
SVN_ERR(ctx-cancel_func(ctx-cancel_baton));
target_info = APR_ARRAY_IDX(targets_info, i, patch_target_info_t *);
+
+ /* Skipped dirs will not have a local_abspath set. */
+ if (target_info-skipped)
+ continue;
+
parent = svn_dirent_dirname(target_info-local_abspath, iterpool);
if (apr_hash_get(non_empty_dirs, parent, APR_HASH_KEY_STRING))
@@ -2915,6 +2921,7 @@ apply_patches(void *baton,
target_info-local_abspath = apr_pstrdup(scratch_pool,
target-local_abspath);
target_info-deleted = target-deleted;
+ target_info-skipped = target-skipped;
APR_ARRAY_PUSH(targets_info,
patch_target_info_t *) = target_info;
What is not clear to me is why skipped targets get added to targets_info
in the first place. How about moving the APR_ARRAY_PUSH inside the
!target-skipped section:
apply_patches(...)
{
...
if (! target-filtered)
{
...
APR_ARRAY_PUSH(targets_info, ...)
if (! target-skipped)
{
}
}
}
Doh, looking at it from perspective that appears to be the right fix. No
changes has been made to skipped targets so no need to check if they've
been deleted.
I'll test to see that the patch works with your suggested change and
commit it later tonight (UTC+1) if no one has any other suggestions.
A side note; when I tested the patch I needed to use abspath's in the
patch file or else the target wouldn't get skipped. For prop targets
that didn't have existing parent-dirs, an error was returned. I would
have expected that such path would get skipped or that we would create
the parent dirs. Will investigate and come back with a test case.
--
Daniel
Re: [PATCH] Fix issue #4049 - SEGV on patch that deletes and skips
On Wed, Nov 9, 2011 at 12:41 PM, Daniel Näslund dan...@dannas.name wrote:
On Wed, Nov 9, 2011 at 10:46 AM, Philip Martin
philip.mar...@wandisco.com wrote:
Daniel Näslund dan...@dannas.name writes:
What is not clear to me is why skipped targets get added to targets_info
in the first place. How about moving the APR_ARRAY_PUSH inside the
!target-skipped section:
apply_patches(...)
{
...
if (! target-filtered)
{
...
APR_ARRAY_PUSH(targets_info, ...)
if (! target-skipped)
{
}
}
}
Doh, looking at it from perspective that appears to be the right fix. No
changes has been made to skipped targets so no need to check if they've
been deleted.
I'll test to see that the patch works with your suggested change and
commit it later tonight (UTC+1) if no one has any other suggestions.
Committed revision 1199950.
--
Daniel
[PATCH] Fix issue #4049 - SEGV on patch that deletes and skips
Hi,
It's been a year since my last commit and I've forgotten about the
internals of the patch code. Sending this patch for review so that I
don't mess something up.
[[[
Fix issue #4049 - SEGV on patch that deletes and skips.
When we do the sanity check of the supplied patch file we set
skipped=TRUE for the files that we're not going to process.
* subversion/libsvn_client/patch.c
(patch_target_info_t): Add SKIPPED field.
(delete_empty_dirs): Don't process targets that are marked skipped.
* subversion/tests/cmdline/patch_tests.py
(patch_delete_and_skip): New.
(test_list): Add new test.
]]]
I could have checked if local_abspath was NULL but I figured it might be
easy to forget to set local_abspath = NULL in one of the many places
were we set the skipped flag.
--
Daniel (dannas)
Index: subversion/tests/cmdline/patch_tests.py
===
--- subversion/tests/cmdline/patch_tests.py (revision 1199347)
+++ subversion/tests/cmdline/patch_tests.py (arbetskopia)
@@ -3913,6 +3913,84 @@ def patch_dev_null(sbox):
1, # check-props
1) # dry-run
+@Issue(4049)
+def patch_delete_and_skip(sbox):
+ patch that deletes and skips
+
+ sbox.build()
+ wc_dir = sbox.wc_dir
+
+ patch_file_path = make_patch_path(sbox)
+
+ os.chdir(wc_dir)
+
+ # We need to use abspaths to trigger the segmentation fault.
+ abs = os.path.abspath('.')
+ if sys.platform == 'win32':
+ abs = abs.replace(\\, /)
+
+ outside_wc = os.path.join(os.pardir, 'X')
+ if sys.platform == 'win32':
+ outside_wc = outside_wc.replace(\\, /)
+
+ unidiff_patch = [
+Index: %s/A/B/E/alpha\n % abs,
+===\n,
+--- %s/A/B/E/alpha\t(revision 1)\n % abs,
++++ %s/A/B/E/alpha\t(working copy)\n % abs,
+@@ -1 +0,0 @@\n,
+-This is the file 'alpha'.\n,
+Index: %s/A/B/E/beta\n % abs,
+===\n,
+--- %s/A/B/E/beta\t(revision 1)\n % abs,
++++ %s/A/B/E/beta\t(working copy)\n % abs,
+@@ -1 +0,0 @@\n,
+-This is the file 'beta'.\n,
+Index: %s/A/B/E/out-of-reach\n % abs,
+===\n,
+--- %s/iota\t(revision 1)\n % outside_wc,
++++ %s/iota\t(working copy)\n % outside_wc,
+\n,
+Property changes on: iota\n,
+___\n,
+Added: propname\n,
+## -0,0 +1 ##\n,
++propvalue\n,
+ ]
+
+ svntest.main.file_write(patch_file_path, ''.join(unidiff_patch))
+
+ skipped_path = os.path.join(os.pardir, 'X', 'iota')
+ expected_output = [
+'D %s\n' % os.path.join('A', 'B', 'E', 'alpha'),
+'D %s\n' % os.path.join('A', 'B', 'E', 'beta'),
+'Skipped missing target: \'%s\'\n' % skipped_path,
+'D %s\n' % os.path.join('A', 'B', 'E'),
+'Summary of conflicts:\n',
+' Skipped paths: 1\n'
+ ]
+
+ expected_disk = svntest.main.greek_state.copy()
+ expected_disk.remove('A/B/E/alpha')
+ expected_disk.remove('A/B/E/beta')
+ expected_disk.remove('A/B/E')
+
+ expected_status = svntest.actions.get_virginal_state('.', 1)
+ expected_status.tweak('A/B/E', status='D ')
+ expected_status.tweak('A/B/E/alpha', status='D ')
+ expected_status.tweak('A/B/E/beta', status='D ')
+
+ expected_skip = wc.State('', {skipped_path: Item()})
+
+ svntest.actions.run_and_verify_patch('.', os.path.abspath(patch_file_path),
+ expected_output,
+ expected_disk,
+ expected_status,
+ expected_skip,
+ None, # expected err
+ 1, # check-props
+ 1) # dry-run
+
#Run the tests
@@ -3954,6 +4032,7 @@ test_list = [ None,
patch_reversed_add_with_props,
patch_reversed_add_with_props2,
patch_dev_null,
+ patch_delete_and_skip,
]
if __name__ == '__main__':
Index: subversion/libsvn_client/patch.c
===
--- subversion/libsvn_client/patch.c(revision 1199347)
+++ subversion/libsvn_client/patch.c(arbetskopia)
@@ -255,6 +255,7 @@ typedef struct patch_target_t {
typedef struct patch_target_info_t {
const char *local_abspath;
svn_boolean_t deleted;
+ svn_boolean_t skipped;
} patch_target_info_t;
@@ -2730,6 +2731,11 @@ delete_empty_dirs(apr_array_header_t *targets_info
SVN_ERR(ctx-cancel_func(ctx-cancel_baton));
target_info = APR_ARRAY_IDX(targets_info, i, patch_target_info_t *);
+
+ /* Skipped dirs will not
Re: svn commit: r1084138 - in /subversion/trunk/subversion: libsvn_diff/parse-diff.c tests/libsvn_diff/parse-diff-test.c
On Sun, Mar 26, 2000 at 10:27:38AM +0200, Daniel Shahaf wrote:
You can declare VALID_HEADER_FILE in the do{} loop's scope. (I spotted
this because the double initialization stood out.)
dan...@apache.org wrote on Tue, Mar 22, 2011 at 11:19:44 -:
Author: dannas
Date: Tue Mar 22 11:19:44 2011
New Revision: 1084138
URL: http://svn.apache.org/viewvc?rev=1084138view=rev
Log:
Fix issue #3809 - 'svn patch' accepts invalid git diff headers.
Fixed in r1085788. Thanks.
Daniel (dannas)
Re: svn commit: r1034139 - in /subversion/trunk/subversion: libsvn_client/patch.c tests/cmdline/patch_tests.py
On Fri, Nov 12, 2010 at 03:14:13AM +0200, Daniel Shahaf wrote: Stefan Sperling wrote on Fri, Nov 12, 2010 at 01:07:53 +0100: On Thu, Nov 11, 2010 at 09:21:26PM -, dan...@apache.org wrote: Add support for handling symlinks in 'svn patch'. If you haven't done so, can you close the corresponding issue, too? Issue #3697. You beat me too it, sorry about beeing so slow. :) Daniel (dannas)
Re: [WIP] Gracefully handle svn_wc_prop_set4() errors in svn patch
On Tue, Sep 28, 2010 at 03:45:33PM +0200, Daniel Shahaf wrote:
Daniel Näslund wrote on Mon, Sep 27, 2010 at 22:09:26 +0200:
* What is wrong with the way I handle the error? I hit err_abort() when
the program terminates. (I'm expecting the answer to hurt a bit - this
is surely something that I should have understood by now). I thought
that since the error is allocated on the heap I could just store the
pointer to it and free it later, e.g. call svn_error_clear().
err_abort() is called when an error object hadn't been svn_error_clear()'d.
(The error creation installed err_abort() as a pool cleanup callback,
and clearing the error unregisters the callback.)
Yes, that was how I understood it.
So, yeah, you can do whatever you want with the error (they get
allocated in a global pool) as long as you svn_error_clear() them
eventually.
Ok.
Index: subversion/svn/notify.c
===
--- subversion/svn/notify.c (revision 1001829)
+++ subversion/svn/notify.c (arbetskopia)
@@ -464,6 +464,16 @@ notify(void *baton, const svn_wc_notify_t *n, apr_
goto print_error;
break;
+case svn_wc_notify_failed_prop_patch:
+ nb-received_some_change = TRUE;
+ err = svn_cmdline_printf(pool,
+ _(property '%s' rejected from '%s'.\n),
+ n-prop_name, path_local);
+ svn_handle_warning2(stderr, n-err, svn: );
+ if (err)
+goto print_error;
+ break;
+
That's fine, print_error: clears the error.
Index: subversion/libsvn_client/patch.c
===
--- subversion/libsvn_client/patch.c(revision 1001829)
+++ subversion/libsvn_client/patch.c(arbetskopia)
@@ -130,6 +130,12 @@ typedef struct prop_patch_target_t {
/* ### Here we'll add flags telling if the prop was added, deleted,
* ### had_rejects, had_local_mods prior to patching and so on. */
+
+ /* TRUE if the property could not be set on the path. */
+ svn_boolean_t was_rejected;
+
+ /* Set if was_rejected is TRUE. */
+ svn_error_t *err;
} prop_patch_target_t;
typedef struct patch_target_t {
@@ -1573,6 +1579,22 @@ send_patch_notification(const patch_target_t *targ
prop_target = svn__apr_hash_index_val(hash_index);
+ if (prop_target-was_rejected)
+{
+ svn_wc_notify_t *notify;
+ svn_wc_notify_action_t action =
svn_wc_notify_failed_prop_patch;
+
+ notify = svn_wc_create_notify(target-local_abspath
+? target-local_abspath
+: target-local_relpath,
+action, pool);
+ notify-prop_name = prop_target-name;
+ notify-err = prop_target-err;
+
+ (*ctx-notify_func2)(ctx-notify_baton2, notify, pool);
+ svn_error_clear(prop_target-err);
Here I'm clearing prop_target-err. Since prop_target-was_rejected is
only set if and error exists (e.g. ! prop_target-err) I was expecting
that err would always be cleared.
[...]
@@ -2260,14 +2283,23 @@ install_patched_prop_targets(patch_target_t *targe
+ err = svn_wc_prop_set4(ctx-wc_ctx, target-local_abspath,
+ prop_target-name,
+ svn_string_create_from_buf(prop_content,
+iterpool),
+ TRUE /* skip_checks */,
+ NULL, NULL,
+ iterpool);
+ if (err)
+{
+ prop_target-was_rejected = TRUE;
+ prop_target-err = err;
Does prop_target-err always get cleared?
(The answer is probably No.)
As I said above, my intention was to clear it in
send_patch_notification().
I'll check again and see if I can spot why the err isn't always cleared.
Thanks for your feedback,
Daniel (dannas)
Re: [WIP] Gracefully handle svn_wc_prop_set4() errors in svn patch
On Tue, Sep 28, 2010 at 07:18:39PM +0100, Philip Martin wrote: Daniel Näslund dan...@longitudo.com writes: On Tue, Sep 28, 2010 at 03:45:33PM +0200, Daniel Shahaf wrote: Daniel Näslund wrote on Mon, Sep 27, 2010 at 22:09:26 +0200: err_abort() is called when an error object hadn't been svn_error_clear()'d. (The error creation installed err_abort() as a pool cleanup callback, and clearing the error unregisters the callback.) Yes, that was how I understood it. If you run the program gdb, it will catch the abort. If you step up the stack to err_abort and print err[0] then you will see the file and line where the error was created. (You may well have worked this out already). Turned out that it was caused by prop_target-was_deleted (the flag that was set when an error was detected) not beeing initialized to FALSE. Thanks for the suggestion on checking err. Didn't think of that (but I really should have!). Thanks, Daniel
[WIP] Gracefully handle svn_wc_prop_set4() errors in svn patch
Hi!
Questions
-
* Is it sane to add a svn_wc_notify_failed_prop_patch action for this
special case? We're starting to have a lot of actions.
* What is wrong with the way I handle the error? I hit err_abort() when
the program terminates. (I'm expecting the answer to hurt a bit - this
is surely something that I should have understood by now). I thought
that since the error is allocated on the heap I could just store the
pointer to it and free it later, e.g. call svn_error_clear().
[[[
Print a warning instead of error out if a property could not be set on a
path with 'svn patch'.
* subversion/svn/notify.c
(notify): Add svn_wc_notify_failed_prop_patch case.
* subversion/include/svn_wc.h
(svn_wc_notify_action_t): Add svn_wc_notify_failed_prop_patch field.
(svn_wc_notify_t): Update doc string for 'err' field to mention that
it is set for svn_wc_notify_failed_prop_patch.
* subversion/libsvn_client/patch.c
(prop_patch_target_t): Add 'was_rejected' and 'err' field to record
failed property patches.
(send_patch_notification): Send svn_wc_notify_failed_prop_patch
notifications.
(install_patched_prop_targets): Record failed propsets.
]]]
Daniel
Index: subversion/svn/notify.c
===
--- subversion/svn/notify.c (revision 1001829)
+++ subversion/svn/notify.c (arbetskopia)
@@ -464,6 +464,16 @@ notify(void *baton, const svn_wc_notify_t *n, apr_
goto print_error;
break;
+case svn_wc_notify_failed_prop_patch:
+ nb-received_some_change = TRUE;
+ err = svn_cmdline_printf(pool,
+ _(property '%s' rejected from '%s'.\n),
+ n-prop_name, path_local);
+ svn_handle_warning2(stderr, n-err, svn: );
+ if (err)
+goto print_error;
+ break;
+
case svn_wc_notify_update_update:
case svn_wc_notify_merge_record_info:
{
Index: subversion/include/svn_wc.h
===
--- subversion/include/svn_wc.h (revision 1001829)
+++ subversion/include/svn_wc.h (arbetskopia)
@@ -1089,8 +1089,11 @@ typedef enum svn_wc_notify_action_t
/** The server has instructed the client to follow a URL
* redirection.
* @since New in 1.7. */
- svn_wc_notify_url_redirect
+ svn_wc_notify_url_redirect,
+ /** A hunk from a patch could not be applied. */
+ svn_wc_notify_failed_prop_patch
+
} svn_wc_notify_action_t;
@@ -1198,7 +1201,8 @@ typedef struct svn_wc_notify_t {
/** Points to an error describing the reason for the failure when @c
* action is one of the following: #svn_wc_notify_failed_lock,
- * #svn_wc_notify_failed_unlock, #svn_wc_notify_failed_external.
+ * #svn_wc_notify_failed_unlock, #svn_wc_notify_failed_external,
+ * #svn_wc_notify_failed_prop_patch.
* Is @c NULL otherwise. */
svn_error_t *err;
Index: subversion/libsvn_client/patch.c
===
--- subversion/libsvn_client/patch.c(revision 1001829)
+++ subversion/libsvn_client/patch.c(arbetskopia)
@@ -130,6 +130,12 @@ typedef struct prop_patch_target_t {
/* ### Here we'll add flags telling if the prop was added, deleted,
* ### had_rejects, had_local_mods prior to patching and so on. */
+
+ /* TRUE if the property could not be set on the path. */
+ svn_boolean_t was_rejected;
+
+ /* Set if was_rejected is TRUE. */
+ svn_error_t *err;
} prop_patch_target_t;
typedef struct patch_target_t {
@@ -1573,6 +1579,22 @@ send_patch_notification(const patch_target_t *targ
prop_target = svn__apr_hash_index_val(hash_index);
+ if (prop_target-was_rejected)
+{
+ svn_wc_notify_t *notify;
+ svn_wc_notify_action_t action = svn_wc_notify_failed_prop_patch;
+
+ notify = svn_wc_create_notify(target-local_abspath
+? target-local_abspath
+: target-local_relpath,
+action, pool);
+ notify-prop_name = prop_target-name;
+ notify-err = prop_target-err;
+
+ (*ctx-notify_func2)(ctx-notify_baton2, notify, pool);
+ svn_error_clear(prop_target-err);
+}
+
for (i = 0; i prop_target-content_info-hunks-nelts; i++)
{
const hunk_info_t *hi;
@@ -2189,6 +2211,7 @@ install_patched_prop_targets(patch_target_t *targe
svn_stringbuf_t *prop_content;
const char *eol_str;
svn_boolean_t eof;
+ svn_error_t *err;
svn_pool_clear(iterpool);
@@ -2260,14 +2283,23 @@ install_patched_prop_targets(patch_target_t *targe
* ### stsp: I'd say reject the property hunk.
* ### We should verify all modified prop hunk texts using
* ###
Re: Do we want 'svn patch' to be able to add empty files?
On Wed, Sep 01, 2010 at 06:37:08PM +0100, Julian Foad wrote: Daniel Näslund wrote: The question is: Do we want 'svn patch to be able to add empty files. If add an empty file is a well defined operation in Git-diff syntax, then yes, it would be good to support it. As well as delete this empty file and any other valid Subversion operations that aren't yet supported. Support for deleting empty files was added in r1001493. There's a lot of other interresting stuff in this thread (adding support for dirs in the git format and properly applying symlinks). Will take a stab at those in the close future. Daniel
Re: Do we want 'svn patch' to be able to add empty files?
On Fri, Sep 03, 2010 at 12:18:37PM +0200, Branko Čibej wrote: On 02.09.2010 10:50, Branko Čibej wrote: Hmm, this is interesting. :) Git faithfully (blindly?) interprets Unix permission bits, whiles SVN faithfully (blindly?) interprets the contents of special files ... I wonder if svn patch does the right thing here? Anyway, for the sake of interoperability, we'd have to emit and parse the git format for symlinks. Not that I'm too amused by the idea that git probably just does a chmod on the new file without thinking about it, but hey, All the World is Linux, right? :) Did some testing ... apparently git apply completely ignores the permission bits new file mode ... line, at least I haven't been able to force it to do anything with them. From builtin/apply::try_create_file() in the git source code: fd = open(path, O_CREAT | O_EXCL | O_WRONLY, (mode 0100) ? 0777 : 0666); if (fd 0) return -1; Git only checks for the executable bit, AFAIK. Daniel
Re: Do we want 'svn patch' to be able to add empty files?
On Thu, Sep 02, 2010 at 11:27:07AM +0300, Daniel Shahaf wrote: Daniel Näslund wrote on Thu, Sep 02, 2010 at 07:13:00 +0200: On Wed, Sep 01, 2010 at 06:37:08PM +0100, Julian Foad wrote: This may be off topic, but I'm wondering whether Git has defined such operations on directories fully or at all, since it doesn't version them explicitly. I mean, can you tell the difference between add empty file A and add empty dir A? I could go and look, but haven't time today. If yes, great; if it doesn't, we'll have to invent syntax extensions to do it. (I'm recalling that the goal of this work is we want Subversion diffs to be able to support all valid Subversion changes, and we chose Git format as a basis for supporting that. We don't want to constrain ourselves to only the operations that Git supports.) Not supported at the moment: $ svn mkdir X A X $ svn status AX $ svn diff $ svn diff --git $ Suggestion: $ svn diff --git Index: empty === diff --git a/trunk/empty b/trunk/empty new directory mode 10644 IIRC trailing slashes on empty/ were suggested on IRC, what was the conclusion about that? I didn't follow that one (atleast I don't recall following it) but will stay tuned for the resolution of the discussion. E.g., just changing the 'new file mode 10644' line to mention directory instead. Haven't investigate what changes would be needed in the diff editor. By the way, are we just influenced by Git's format, or are we looking for some degree of interoperability? Consider adding a symlink: We have two goals I think: i) Implement a format that is interoperable with git and mercurial and possible other vcs tools that choose to implement the git extensions to the unidiff format. ii) Adjust the format dictated by the git unidiff extensions to allow us to describe svn-specific features such as properties, versioned dirs and copies from a specific revision. Those are the ones that immediately comes to mind. Please fill in if you have more suggestions on what the format should include! As for symlinks, svn has the (in my opinion) ugly solution of storing the information in an svn:special property. So we have a specific svn way of specifying symlinks. Originally it was my intention to only use the svn:special property. For me, the goal of interoperability is clearly not as important as us having a format that works for svn. If I were to choose between introducing additional complexity for providing interoperability or drop that feature (i.e. git symlinks) I would choose the second by instinct. But maybe the community values i) more than I do? Note that we don't yet perform any symlink operation when applying a path with svn:special set. [[[ ### with git (in $wcroot/trunk/): diff --git a/trunk/bar b/trunk/bar new file mode 12 index 000..1910281 --- /dev/null +++ b/trunk/bar @@ -0,0 +1 @@ +foo \ No newline at end of file ### with svn (in $wcroot/trunk/): Index: bar === diff --git a/trunk/bar b/trunk/bar new file mode 10644 --- /dev/null (revision 0) +++ b/trunk/bar (working copy) @@ -0,0 +1 @@ +link foo \ No newline at end of file Property changes on: trunk/bar ___ Added: svn:special ## -0,0 +1 ## +* ]]] Daniel (Näslund)
Re: Do we want 'svn patch' to be able to add empty files?
On Thu, Sep 02, 2010 at 11:14:27AM +0300, Daniel Shahaf wrote: (Thanks for the examples. I suppose next time I should try to run 'touch foo; $svn add foo; $svn diff --git' myself...) Daniel Näslund wrote on Thu, Sep 02, 2010 at 07:05:41 +0200: On Wed, Sep 01, 2010 at 10:54:08PM +0300, Daniel Shahaf wrote: Daniel Näslund wrote on Wed, Sep 01, 2010 at 11:28:51 +0200: (This started out as me trying to apply added paths using the information from a patch file in the git diff format. The only that I could come up with where an add could not be detected by just looking at the regular unidiff headers was adding an empty file (it has no unidiff headers). If anyone has any other cases, please let me know.) How does a diff adding an empty file look? Like this: Index: empty === diff --git a/trunk/empty b/trunk/empty new file mode 10644 So, it boils down to having to recognize /^new file mode/ (even though there are no following /^(---|+++)/ lines), right? I've never been inside the patch code, I don't know how easy/tricky it would be to add this. See svn_diff_parse_next_patch() for the gory details. Note that we allow empty files to be created for regular diffs too if they have property changes. This patch will create an empty file with property 'foo' set on it: Index: empty === Property changes on: empty ___ Added: foo ## -0,0 +1 ## +value So this implicitly creates the file if it doesn't exist already; in other words, we do not distinguish setting a property on an existing file (without content changes) from adding a file with properties. Would it be better to make a distinction --- for example, by generating a /^new file/ line in the latter case? (that would be explicit and more friendly to non-property-aware tools) My personal opinion is that it would be better to add a --git option to svn patch and only apply tree changes and property changes if we have a git diff and a the --git diff option given. Then we wouln't have these kind of overlapping cases. But for the user it's more convinient to just call 'svn patch PATCH_FILE' on whatever file we have at hand. It's a personal opinion and I'm not really advocating it... Just making noise I guess. :) Daniel (Näslund)
Do we want 'svn patch' to be able to add empty files?
Hi! (This started out as me trying to apply added paths using the information from a patch file in the git diff format. The only that I could come up with where an add could not be detected by just looking at the regular unidiff headers was adding an empty file (it has no unidiff headers). If anyone has any other cases, please let me know.) The question is: Do we want 'svn patch to be able to add empty files. + It's consistent with the idea of a git diff dealing with tree changes. - It adds an extra special case to the code. I've seen GNU patch with its gigantic if-spaghetti's. Just don't want to obscure the code with something that's not needed. - There might not be a use case for it, though I think I've heard of the use of empty files as markers. Just can't come up with an example right now. Daniel
Re: Do we want 'svn patch' to be able to add empty files?
On Wed, Sep 01, 2010 at 10:54:08PM +0300, Daniel Shahaf wrote: Daniel Näslund wrote on Wed, Sep 01, 2010 at 11:28:51 +0200: (This started out as me trying to apply added paths using the information from a patch file in the git diff format. The only that I could come up with where an add could not be detected by just looking at the regular unidiff headers was adding an empty file (it has no unidiff headers). If anyone has any other cases, please let me know.) How does a diff adding an empty file look? Like this: Index: empty === diff --git a/trunk/empty b/trunk/empty new file mode 10644 Note that we allow empty files to be created for regular diffs too if they have property changes. This patch will create an empty file with property 'foo' set on it: Index: empty === Property changes on: empty ___ Added: foo ## -0,0 +1 ## +value The question is: Do we want 'svn patch to be able to add empty files. + It's consistent with the idea of a git diff dealing with tree changes. - It adds an extra special case to the code. I've seen GNU patch with its gigantic if-spaghetti's. Just don't want to obscure the code with something that's not needed. - There might not be a use case for it, though I think I've heard of the use of empty files as markers. Just can't come up with an example right now. Two examples: * vimrc. (Vim behaves differently with no vimrc than with an empty vimrc) * pseudo-targets for makefiles foo: $commands touch $@ Good examples. I think configuration files and build files are the typical use cases for empty files. Daniel (Näslund)
Re: [PATCH] was: [WIP] Enable passing copyfrom information for the diff code when dealing with repository diffs.
Daniel, Sorry for the delayed response. I've committed the patch with your suggestions in r990790. Below is just ACK's for those changes. On Mon, Aug 16, 2010 at 10:59:03PM +0300, Daniel Shahaf wrote: Daniel Näslund wrote on Mon, Aug 16, 2010 at 15:16:33 +0200: And this time with a patch attached. On Mon, Aug 16, 2010 at 03:09:49PM +0200, Daniel Näslund wrote: I'm touching a lot of code here. Reviews would be much apprecieated. [[[ Make the diff editor able to receive copyfrom information. Involves passing down a 'send_copyfrom_args' to all RA implemtations. Note that this commit merely allows the copyfrom args to be passed to the client. They copyfrom information is not yet stored and used. This commit changes the ra_svn protocols, but not the other protocols. Okay. ]]] Thanks, Daniel Index: subversion/libsvn_ra/deprecated.c === --- subversion/libsvn_ra/deprecated.c (revision 985813) +++ subversion/libsvn_ra/deprecated.c (arbetskopia) @@ -239,6 +239,30 @@ svn_error_t *svn_ra_get_commit_editor(svn_ra_sessi keep_locks, pool); } +svn_error_t * svn_ra_do_diff3(svn_ra_session_t *session, Style nit: no space after '*'. Fixed. + apr_pool_t *pool) Index: subversion/libsvn_ra_svn/protocol === --- subversion/libsvn_ra_svn/protocol (revision 985813) +++ subversion/libsvn_ra_svn/protocol (arbetskopia) @@ -345,7 +345,8 @@ second place for auth-request point as noted below diff params: ( [ rev:number ] target:string recurse:bool ignore-ancestry:bool -url:string ? text-deltas:bool ? depth:word ) +url:string ? text-deltas:bool ? depth:word +send_copyfrom_param:bool ) Err, shouldn't this be +url:string ? text-deltas:bool ? depth:word +? send_copyfrom_param:bool ) since clients before-your-change don't send the send_copyfrom_args param? Yes it should. Fixed. Client switches to report command set. Upon finish-report, server sends auth-request. After auth exchange completes, server switches to editor command set. Index: subversion/include/svn_ra.h === --- subversion/include/svn_ra.h (revision 985813) +++ subversion/include/svn_ra.h (arbetskopia) @@ -1291,9 +1291,30 @@ svn_ra_do_status(svn_ra_session_t *session, * needed, and sending too much data back, a pre-1.5 'recurse' * directive may be sent to the server, based on @a depth. * - * @since New in 1.5. + * @since New in 1.7. */ svn_error_t * +svn_ra_do_diff4(svn_ra_session_t *session, +const svn_ra_reporter3_t **reporter, +void **report_baton, +svn_revnum_t revision, +const char *diff_target, +svn_depth_t depth, +svn_boolean_t send_copyfrom_args, +svn_boolean_t ignore_ancestry, +svn_boolean_t text_deltas, +const char *versus_url, +const svn_delta_editor_t *diff_editor, +void *diff_baton, +apr_pool_t *pool); Need an empty line right here (as I said in a previous mail). Fixed. +/** + * Similar to svn_ra_do_diff4(), but with @c send_copyfrom_args set to + * FALSE. + * + * @deprecated Provided for compatibility with the 1.5 API. + */ +SVN_DEPRECATED +svn_error_t * svn_ra_do_diff3(svn_ra_session_t *session, const svn_ra_reporter3_t **reporter, void **report_baton, @@ -1306,7 +1327,6 @@ svn_ra_do_diff3(svn_ra_session_t *session, const svn_delta_editor_t *diff_editor, void *diff_baton, apr_pool_t *pool); - And here you removed an empty line? Can we have it back please? (that way one can use paragraph-motion commands (e.g., '}') when reading the source.) Fixed. /** * Similar to svn_ra_do_diff3(), but taking @c svn_ra_reporter2_t * instead of @c svn_ra_reporter3_t, and therefore only able to report Index: subversion/libsvn_client/repos_diff.c === --- subversion/libsvn_client/repos_diff.c (revision 985813) +++ subversion/libsvn_client/repos_diff.c (arbetskopia) @@ -552,6 +552,8 @@ delete_entry(const char *path, svn_wc_notify_action_t action = svn_wc_notify_skip; svn_boolean_t tree_conflicted = FALSE; + SVN_DBG((delete_entry: %s\n, path)); + Shouldn't be committed. (There are a few others in the diff.) The debug statements are now removed. Index: subversion/svnserve
[PATCH] was: [WIP] Enable passing copyfrom information for the diff code when dealing with repository diffs.
I'm touching a lot of code here. Reviews would be much apprecieated. [[[ Make the diff editor able to receive copyfrom information. Involves passing down a 'send_copyfrom_args' to all RA implemtations. Note that this commit merely allows the copyfrom args to be passed to the client. They copyfrom information is not yet stored and used. * subversion/libsvn_ra/ra_loader.c (svn_ra_do_diff4): New. (svn_ra_do_diff3): Move from here .. * subversion/libsvn_ra/deprecated.c (svn_ra_do_diff3): .. To here. (svn_ra_do_diff2): Call svn_ra_do_diff3() instead of the vtable callback since the signature has changed. * subversion/libsvn_ra/wrapper_template.h (compat_do_diff): Track the new 'send_copyfrom_args' parameter. * subversion/libsvn_ra/ra_loader.h (svn_ra__vtable_t): Add 'send_copyfrom_args' parameter. * subversion/libsvn_ra_local/ra_plugin.c (svn_ra_local__do_diff): Add 'send_copyfrom_args' parameter. * subversion/tests/cmdline/diff_tests.py (test_list): Mark diff_backward_repos_wc_copy() as XFailing. The tested code currently does not handle copied paths with no text changes. Will be fixed in a follow-up. * subversion/libsvn_ra_svn/protocol (...) Update the diff command description. * subversion/libsvn_ra_svn/client.c (ra_svn_diff): Add 'send_copyfrom_args' to the command to be written. * subversion/include/svn_ra.h (svn_ra_do_diff4): New. (svn_ra_do_diff3): Deprecate. * subversion/libsvn_wc/diff.c (add_file): Add TODO about recording the copyfrom info and checking that the copyfrom revision is within the span of the diff operation. * subversion/libsvn_client/repos_diff.c (add_file): Add TODO about recording the copyfrom info and checking that the copyfrom revision is within the span of the diff operation. * subversion/libsvn_client/diff.c (diff_repos_repos, diff_repos_wc, diff_summarize_repos_repos): Replace svn_ra_do_diff3() with svn_ra_do_diff4(). * subversion/libsvn_ra_neon/ra_neon.h (svn_ra_neon__do_diff): Add 'send_copyfrom_args' parameter. * subversion/libsvn_ra_neon/fetch.c (svn_ra_neon__do_diff): Add 'send_copyfrom_args' parameter. * subversion/libsvn_ra_serf/update.c (svn_ra_serf__do_diff): Add 'send_copyfrom_args' parameter. * subversion/libsvn_ra_serf/ra_serf.h (svn_ra_serf__do_diff): Add 'send_copyfrom_args' parameter. * subversion/svnserve/serve.c (diff): Parse the parameters for send_copyfrom_param. ]]] Thanks, Daniel
Re: [PATCH] was: [WIP] Enable passing copyfrom information for the diff code when dealing with repository diffs.
And this time with a patch attached.
On Mon, Aug 16, 2010 at 03:09:49PM +0200, Daniel Näslund wrote:
I'm touching a lot of code here. Reviews would be much apprecieated.
[[[
Make the diff editor able to receive copyfrom information. Involves
passing down a 'send_copyfrom_args' to all RA implemtations.
Note that this commit merely allows the copyfrom args to be passed to
the client. They copyfrom information is not yet stored and used.
* subversion/libsvn_ra/ra_loader.c
(svn_ra_do_diff4): New.
(svn_ra_do_diff3): Move from here ..
* subversion/libsvn_ra/deprecated.c
(svn_ra_do_diff3): .. To here.
(svn_ra_do_diff2): Call svn_ra_do_diff3() instead of the vtable
callback since the signature has changed.
* subversion/libsvn_ra/wrapper_template.h
(compat_do_diff): Track the new 'send_copyfrom_args' parameter.
* subversion/libsvn_ra/ra_loader.h
(svn_ra__vtable_t): Add 'send_copyfrom_args' parameter.
* subversion/libsvn_ra_local/ra_plugin.c
(svn_ra_local__do_diff): Add 'send_copyfrom_args' parameter.
* subversion/tests/cmdline/diff_tests.py
(test_list): Mark diff_backward_repos_wc_copy() as XFailing.
The tested code currently does not handle copied paths
with no text changes. Will be fixed in a follow-up.
* subversion/libsvn_ra_svn/protocol
(...) Update the diff command description.
* subversion/libsvn_ra_svn/client.c
(ra_svn_diff): Add 'send_copyfrom_args' to the command to be written.
* subversion/include/svn_ra.h
(svn_ra_do_diff4): New.
(svn_ra_do_diff3): Deprecate.
* subversion/libsvn_wc/diff.c
(add_file): Add TODO about recording the copyfrom info and checking
that the copyfrom revision is within the span of the diff operation.
* subversion/libsvn_client/repos_diff.c
(add_file): Add TODO about recording the copyfrom info and checking
that the copyfrom revision is within the span of the diff operation.
* subversion/libsvn_client/diff.c
(diff_repos_repos,
diff_repos_wc,
diff_summarize_repos_repos): Replace svn_ra_do_diff3() with
svn_ra_do_diff4().
* subversion/libsvn_ra_neon/ra_neon.h
(svn_ra_neon__do_diff): Add 'send_copyfrom_args' parameter.
* subversion/libsvn_ra_neon/fetch.c
(svn_ra_neon__do_diff): Add 'send_copyfrom_args' parameter.
* subversion/libsvn_ra_serf/update.c
(svn_ra_serf__do_diff): Add 'send_copyfrom_args' parameter.
* subversion/libsvn_ra_serf/ra_serf.h
(svn_ra_serf__do_diff): Add 'send_copyfrom_args' parameter.
* subversion/svnserve/serve.c
(diff): Parse the parameters for send_copyfrom_param.
]]]
Thanks,
Daniel
Index: subversion/libsvn_ra/deprecated.c
===
--- subversion/libsvn_ra/deprecated.c (revision 985813)
+++ subversion/libsvn_ra/deprecated.c (arbetskopia)
@@ -239,6 +239,30 @@ svn_error_t *svn_ra_get_commit_editor(svn_ra_sessi
keep_locks, pool);
}
+svn_error_t * svn_ra_do_diff3(svn_ra_session_t *session,
+ const svn_ra_reporter3_t **reporter,
+ void **report_baton,
+ svn_revnum_t revision,
+ const char *diff_target,
+ svn_depth_t depth,
+ svn_boolean_t ignore_ancestry,
+ svn_boolean_t text_deltas,
+ const char *versus_url,
+ const svn_delta_editor_t *diff_editor,
+ void *diff_baton,
+ apr_pool_t *pool)
+{
+ SVN_ERR_ASSERT(svn_path_is_empty(diff_target)
+ || svn_path_is_single_path_component(diff_target));
+ return session-vtable-do_diff(session,
+ reporter, report_baton,
+ revision, diff_target,
+ depth, FALSE /* send_copyfrom_args */,
+ ignore_ancestry, text_deltas, versus_url,
+ diff_editor, diff_baton, pool);
+}
+
+
svn_error_t *svn_ra_do_diff2(svn_ra_session_t *session,
const svn_ra_reporter2_t **reporter,
void **report_baton,
@@ -257,8 +281,7 @@ svn_error_t *svn_ra_do_diff2(svn_ra_session_t *ses
|| svn_path_is_single_path_component(diff_target));
*reporter = reporter_3in2_wrapper;
*report_baton = b;
- return session-vtable-do_diff(session,
- (b-reporter3), (b-reporter3_baton),
+ return svn_ra_do_diff3(session, (b-reporter3), (b-reporter3_baton),
revision, diff_target,
SVN_DEPTH_INFINITY_OR_FILES(recurse),
ignore_ancestry, text_deltas, versus_url,
Index: subversion/libsvn_ra/wrapper_template.h
[WIP] Enable passing copyfrom information for the diff code when dealing with repository diffs.
Hi!
Why doesn't svn_ra_do_diff3() request that the repository sends copyfrom
args? The best explanation I've found is this commit message:
r866577 | sussman | 2007-09-10 06:56:55 +0200 (mån, 10 sep 2007) | 87 lines
Send copyfrom-args on updates *only* if the client explicitly requests them.
This keeps the 1.5 servers backward-compatible. Because incoming
copyfrom args cause old clients to error, they should only be
requested by new clients that understand them. This means adding a
new boolean 'send_copyfrom_args' to the main libsvn_repos
editor-driver, svn_ra_do_update2(), and all four RA implementations.
Eesh!
[.. Rest of log message omitted for brevity ..]
I'm assuming noone saw a use case for having diff display copies back then and
since it involved passing down a lot of parameters, only svn_ra_do_updateX()
was revved. But with git diffs we have a use case for copyfrom info. How about
enable it? There's a WIP patch attached to this mail. I haven't done any
wrapper
for svn_ra_do_diff3() though. A log message:
[[[
Make the diff code able to supply copyfrom information even when one of
the targets is an URL. Involves passing down a 'send_copyfrom_args' to
all RA implemtations.
* subversion/libsvn_ra/ra_loader.h
(svn_ra__vtable_t): Add 'send_copyfrom_args' parameter.
* subversion/libsvn_ra_local/ra_plugin.c
(svn_ra_local__do_diff): Add 'send_copyfrom_args' parameter.
* subversion/include/svn_ra.h
(svn_ra_do_diff4): New.
(svn_ra_do_diff3): Deprecate.
* subversion/libsvn_ra_neon/fetch.c
(svn_ra_neon__do_diff): Add 'send_copyfrom_args' parameter.
* subversion/libsvn_ra_serf/update.c
(svn_ra_serf__do_diff): Add 'send_copyfrom_args' parameter.
* subversion/libsvn_ra_serf/ra_serf.h
(svn_ra_serf__do_diff): Add 'send_copyfrom_args' parameter.
* subversion/svnserve/serve.c
(diff): Parse the parameters for send_copyfrom_param.
]]]
Does anyone have any objections to enable copyfrom info for URL-URL
URL-WC diffs?
Thanks,
Daniel
Index: subversion/libsvn_ra/ra_loader.h
===
--- subversion/libsvn_ra/ra_loader.h(revision 985618)
+++ subversion/libsvn_ra/ra_loader.h(arbetskopia)
@@ -155,6 +155,7 @@ typedef struct svn_ra__vtable_t {
svn_revnum_t revision,
const char *diff_target,
svn_depth_t depth,
+ svn_boolean_t send_copyfrom_args,
svn_boolean_t ignore_ancestry,
svn_boolean_t text_deltas,
const char *versus_url,
Index: subversion/libsvn_ra_local/ra_plugin.c
===
--- subversion/libsvn_ra_local/ra_plugin.c (revision 985618)
+++ subversion/libsvn_ra_local/ra_plugin.c (arbetskopia)
@@ -810,6 +810,7 @@ svn_ra_local__do_diff(svn_ra_session_t *session,
svn_revnum_t update_revision,
const char *update_target,
svn_depth_t depth,
+ svn_boolean_t send_copyfrom_args,
svn_boolean_t ignore_ancestry,
svn_boolean_t text_deltas,
const char *switch_url,
@@ -825,7 +826,7 @@ svn_ra_local__do_diff(svn_ra_session_t *session,
switch_url,
text_deltas,
depth,
- FALSE,
+ send_copyfrom_args,
ignore_ancestry,
update_editor,
update_baton,
Index: subversion/include/svn_ra.h
===
--- subversion/include/svn_ra.h (revision 985618)
+++ subversion/include/svn_ra.h (arbetskopia)
@@ -1291,9 +1291,29 @@ svn_ra_do_status(svn_ra_session_t *session,
* needed, and sending too much data back, a pre-1.5 'recurse'
* directive may be sent to the server, based on @a depth.
*
- * @since New in 1.5.
+ * @since New in 1.7.
*/
svn_error_t *
+svn_ra_do_diff4(svn_ra_session_t *session,
+const svn_ra_reporter3_t **reporter,
+void **report_baton,
+svn_revnum_t revision,
+const char *diff_target,
+svn_depth_t depth,
+svn_boolean_t send_copyfrom_args,
+svn_boolean_t ignore_ancestry,
+svn_boolean_t text_deltas,
+const char *versus_url,
+const svn_delta_editor_t *diff_editor,
+void *diff_baton,
+apr_pool_t *pool);
+/**
+ * Similar to svn_ra_do_diff4(), but with @c send_copyfrom_args set to
+ * FALSE.
+ *
+ * @deprecated Provided for compatibility with the 1.5 API.
+ */
+svn_error_t *
svn_ra_do_diff3(svn_ra_session_t *session,
Re: svn commit: r985514 - in /subversion/branches/performance/subversion: include/svn_io.h libsvn_subr/stream.c libsvn_subr/subst.c
On Sat, Aug 14, 2010 at 03:46:13PM -, stef...@apache.org wrote: Author: stefan2 Date: Sat Aug 14 15:46:13 2010 New Revision: 985514 URL: http://svn.apache.org/viewvc?rev=985514view=rev Log: Extend the stream API by three functions: svn_stream_move_mark() to move an existing mark by some delta svn_stream_supports_mark() tells whether getting, setting and moving marks is supported by this stream * subversion/include/svn_io.h (svn_io_move_mark_fn_t, svn_io_buffered_fn_t): declare new vtable function pointers (svn_stream_set_move_mark, svn_stream_set_buffered): declare functions to set these vtable pointers (svn_stream_supports_mark, svn_stream_move_mark, svn_stream_buffered): declare new stream API functions * subversion/libsvn_subr/stream.c (svn_stream_t): extend the vtable part by the new functions (svn_stream_create): add initialization code for the new vtable entries (svn_stream_set_move_mark, svn_stream_set_buffered): implement new vtable modifiers (svn_stream_supports_mark, svn_stream_buffered, svn_stream_buffered): implement new stream generic API functions (move_mark_handler_empty, buffered_handler_empty, svn_stream_empty): implement support for the new stream API in empty streams (move_mark_handler_disown, buffered_handler_disown, svn_stream_disown): implement support for the new stream API in disowned streams (move_mark_handler_apr, buffered_handler_apr, stream_from_aprfile, svn_stream_from_aprfile_range_readonly): implement support for the new stream API in APR file based streams (move_mark_handler_stringbuf, buffered_handler_stringbuf, svn_stream_from_stringbuf): implement support for the new stream API in stringbuf streams * subversion/libsvn_subr/subst.c (translated_stream_move_mark, translated_stream_buffered, svn_subst_stream_translated): implement support for the new stream API in translated streams Out of curiosity, why are you touching the svn_stream_mark() code on the performance branch? AFAIK, svn_stream_mark() is only used in the patch code. What performance benefits are you hoping to achieve? Daniel
GSoC coming to an end
Hi! The soft deadline for GSoC 2010 is today. Not much time left then... What has been accomplished: * A parseable diff format for properties. * svn patch can apply properties * We have Python tests for applying properties. * svn diff can create 'git diffs when passed a special flag. * C-unit tests for parsing properties and git diffs. * Python-tests for svn diff to create git diffs. What is left: * Extensive test coverage for the properties code. * Make the git unidiff format the standard format for diffs. * Record copies for URL-WC and URL-URL. * Record moves for WC-WC, URL-WC, URL-URL. * Actually apply the git diffs with 'svn patch'. Thanks, Daniel
Re: svn commit: r982582 - /subversion/trunk/subversion/tests/libsvn_diff/parse-diff-test.c
On Thu, Aug 05, 2010 at 11:52:14AM -0400, Paul Burba wrote: On Thu, Aug 5, 2010 at 8:41 AM, dan...@apache.org wrote: Author: dannas Date: Thu Aug 5 12:41:51 2010 New Revision: 982582 Modified: subversion/trunk/subversion/tests/libsvn_diff/parse-diff-test.c @@ -834,8 +834,9 @@ test_git_diffs_with_spaces_diff(apr_pool FALSE, /* ignore_whitespace */ pool, pool)); SVN_TEST_ASSERT(patch); - SVN_TEST_ASSERT(! strcmp(patch-old_filename, dir/b/path)); - SVN_TEST_ASSERT(! strcmp(patch-new_filename, dir/b/path)); + SVN_DBG((%s\n, patch-old_filename)); Was this intentional? If so, we need to conditionally exclude it so we can build release configurations: Nope, it was just a scalpel left in the patient. Fixed in r982706. Thanks, Daniel
Why doesn't svn_ra_do_diff3() provide a reporter that supplies copyfrom info?
Hi! Why doesn't svn_ra_do_diff3() provide a reporter that supplies copyfrom info? svn_ra_do_update2() does, so why doesn't the diff func? Is it based on a notion of diffs as beeing text compares and thus not interrested in tree changes such as copies/moves? Is there something in the FS layer that doesn't allow us to fetch copyfrom info? Thanks, Daniel
Re: svn commit: r980784 - in /subversion/trunk/subversion/libsvn_wc: adm_files.c node.c wc_db.h
On Fri, Jul 30, 2010 at 01:28:39PM -, phi...@apache.org wrote:
Author: philip
Date: Fri Jul 30 13:28:39 2010
New Revision: 980784
Modified: subversion/trunk/subversion/libsvn_wc/adm_files.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/adm_files.c?rev=980784r1=980783r2=980784view=diff
==
--- subversion/trunk/subversion/libsvn_wc/adm_files.c (original)
+++ subversion/trunk/subversion/libsvn_wc/adm_files.c Fri Jul 30 13:28:39 2010
@@ -622,40 +624,59 @@ svn_wc__internal_ensure_adm(svn_wc__db_t
repos_relpath, repos_root_url,
repos_uuid,
revision, depth, scratch_pool));
- /* Now, get the existing url and repos for PATH. */
- SVN_ERR(svn_wc__get_entry(entry, db, local_abspath, FALSE,
svn_node_unknown,
-FALSE, scratch_pool, scratch_pool));
+ SVN_ERR(svn_wc__db_read_info(status, NULL,
+ db_revision, db_repos_relpath,
+ db_repos_root_url, db_repos_uuid,
+ NULL, NULL, NULL, NULL, NULL, NULL, NULL,
+ NULL, NULL, NULL, NULL, NULL, NULL, NULL,
+ NULL, NULL, NULL, NULL,
+ db, local_abspath, scratch_pool,
scratch_pool));
- /* When the directory exists and is scheduled for deletion do not
- * check the revision or the URL. The revision can be any
+ /* When the directory exists and is scheduled for deletion or is
not-present
+ * do not check the revision or the URL. The revision can be any
* arbitrary revision and the URL may differ if the add is
* being driven from a merge which will have a different URL. */
- if (entry-schedule != svn_wc_schedule_delete)
+ if (status != svn_wc__db_status_deleted
+ status != svn_wc__db_status_obstructed_delete)
{
- if (entry-revision != revision)
+ /* ### Should we match copyfrom_revision? */
+ if (db_revision != revision)
return
svn_error_createf(SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
_(Revision %ld doesn't match existing
revision %ld in '%s'),
-revision, entry-revision, local_abspath);
+revision, db_revision, local_abspath);
/* The caller gives us a URL which should match the entry. However,
some callers compensate for an old problem in entry-url and pass
the copyfrom_url instead. See ^/notes/api-errata/wc002.txt. As
a result, we allow the passed URL to match copyfrom_url if it
- does match the entry's primary URL. */
- /** ### comparing URLs, should they be canonicalized first? */
- if (strcmp(entry-url, url) != 0
- (entry-copyfrom_url == NULL
- || strcmp(entry-copyfrom_url, url) != 0)
- (!svn_uri_is_ancestor(repos_root_url, entry-url)
- || strcmp(entry-uuid, repos_uuid) != 0))
+ does not match the entry's primary URL. */
+ /* ### comparing URLs, should they be canonicalized first? */
Previously urls were compared but now, we're comparing uuid, root_url
and repos_relpath. Those are all already canonicalized, right? Appears
not, since a bit higher up we're doing:
repos_relpath = svn_uri_is_child(repos_root_url, url, scratch_pool);
repos_relpath = svn_path_uri_decode(repos_relpath, scratch_pool);
We probably should add a svn_relpath_canonicalize() after those and
maybe the documentation on the top of svn_dirent_uri.h should mention
that a relpath is supposed to not be encoded as an uri. And shouldn't
all relpaths, uris and dirents be canonicalized when they reach
libsvn_wc?
+ if (strcmp(db_repos_uuid, repos_uuid)
+ || strcmp(db_repos_root_url, repos_root_url)
+ || !svn_relpath_is_ancestor(db_repos_relpath, repos_relpath))
{
- return
-svn_error_createf(SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
- _(URL '%s' doesn't match existing
-URL '%s' in '%s'),
- url, entry-url, local_abspath);
+ const char *copyfrom_root_url, *copyfrom_repos_relpath;
+
+ SVN_ERR(svn_wc__internal_get_copyfrom_info(copyfrom_root_url,
+ copyfrom_repos_relpath,
+ NULL, NULL, NULL,
+ db, local_abspath,
+ scratch_pool,
+ scratch_pool));
+
+ if (copyfrom_root_url == NULL
+ || strcmp(copyfrom_root_url, repos_root_url)
+
[PATCH] Clarify that 'svn patch' can only apply changes to targets that are dirs
Hi!
resolve_target_path() makes a few assumptions about the target for a
patch file beeing a wc_dir. Currently we can only invoke svn patch like
this:
svn patch PATCH_FILE WC_DIR_PATH
Is this the intended behavoiour?
[[[
Make it clearer that the patch code can only use base paths that are
dirs.
* subversion/svn/main.c
(svn_opt_subcommand_desc2_t): Clarify help text.
* subversion/include/svn_client.h
(svn_client_patch): Replace parameter 'local_abspath' with
'base_dir_abspath'.
* subversion/libsvn_client/patch.c
(resolve_target_path,
svn_client_patch): Replace parameter 'local_abspath' with
'base_dir_abspath'
]]]
Cheers,
Daniel
Index: subversion/svn/main.c
===
--- subversion/svn/main.c (revision 981230)
+++ subversion/svn/main.c (arbetskopia)
@@ -812,10 +812,10 @@ const svn_opt_subcommand_desc2_t svn_cl__cmd_table
{ patch, svn_cl__patch, {0}, N_
(Apply a patch to a working copy.\n
- usage: patch PATCHFILE [WCPATH]\n
+ usage: patch PATCHFILE [WC_DIR_PATH]\n
\n
- Apply a unidiff patch in PATCHFILE to the working copy WCPATH.\n
- If WCPATH is omitted, '.' is assumed.\n
+ Apply a unidiff patch in PATCHFILE to the working copy WC_DIR_PATH.\n
+ If WC_DIR_PATH is omitted, '.' is assumed.\n
\n
A unidiff patch suitable for application to a working copy can be\n
produced with the 'svn diff' command or third-party diffing tools.\n
Index: subversion/include/svn_client.h
===
--- subversion/include/svn_client.h (revision 981230)
+++ subversion/include/svn_client.h (arbetskopia)
@@ -5204,7 +5204,7 @@ typedef svn_error_t *(*svn_client_patch_func_t)(
/**
* Apply a unidiff patch that's located at absolute path
- * @a patch_abspath to the working copy at @a local_abspath.
+ * @a patch_abspath to the working copy at @a base_dir_abspath.
*
* This function makes a best-effort attempt at applying the patch.
* It might skip patch targets which cannot be patched (e.g. targets
@@ -5247,7 +5247,7 @@ typedef svn_error_t *(*svn_client_patch_func_t)(
*/
svn_error_t *
svn_client_patch(const char *patch_abspath,
- const char *local_abspath,
+ const char *base_dir_abspath,
svn_boolean_t dry_run,
int strip_count,
svn_boolean_t reverse,
Index: subversion/libsvn_client/patch.c
===
--- subversion/libsvn_client/patch.c(revision 981230)
+++ subversion/libsvn_client/patch.c(arbetskopia)
@@ -329,7 +329,7 @@ obtain_eol_and_keywords_for_file(apr_hash_t **keyw
static svn_error_t *
resolve_target_path(patch_target_t *target,
const char *path_from_patchfile,
-const char *local_abspath,
+const char *base_dir_abspath,
int strip_count,
svn_boolean_t prop_changes_only,
svn_wc_context_t *wc_ctx,
@@ -359,7 +359,7 @@ resolve_target_path(patch_target_t *target,
if (svn_dirent_is_absolute(stripped_path))
{
- target-local_relpath = svn_dirent_is_child(local_abspath, stripped_path,
+ target-local_relpath = svn_dirent_is_child(base_dir_abspath,
stripped_path,
result_pool);
/* ### We need to allow setting props on the wc root dir */
@@ -380,7 +380,7 @@ resolve_target_path(patch_target_t *target,
/* Make sure the path is secure to use. We want the target to be inside
* of the working copy and not be fooled by symlinks it might contain. */
- if (! svn_dirent_is_under_root(target-local_abspath, local_abspath,
+ if (! svn_dirent_is_under_root(target-local_abspath, base_dir_abspath,
target-local_relpath, result_pool))
{
/* The target path is outside of the working copy. Skip it. */
@@ -2582,7 +2582,7 @@ apply_patches(void *baton,
svn_error_t *
svn_client_patch(const char *patch_abspath,
- const char *local_abspath,
+ const char *base_dir_abspath,
svn_boolean_t dry_run,
int strip_count,
svn_boolean_t reverse,
Re: svn commit: r980427 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_client/patch.c svn/notify.c tests/cmdline/patch_tests.py
On Thu, Jul 29, 2010 at 02:54:29PM +0200, Stefan Sperling wrote: On Thu, Jul 29, 2010 at 12:43:56PM -, dan...@apache.org wrote: Author: dannas Date: Thu Jul 29 12:43:56 2010 New Revision: 980427 URL: http://svn.apache.org/viewvc?rev=980427view=rev Log: Make 'svn patch' do notifications for property hunks. We use the same notification actions as for text hunks but replace the headers to look like: '## -1,6 +1,7 ##' instead of: '@@ -1,6 +1,7 @@' Note that we don't specify what property a hunk belongs to (that information is available in the reject file). Why not provide the propery name to the notification callback, and print it e.g. like this? ## -1,6 +1,7 ## (svn:externals) Oh... So obvious. And the precense of n-prop_name tells us if we have a property hunk or not. I'll do that. Thanks, Daniel
GSoC Progress report
Hi! Property diffs === * We can create parseable prop patches * We can parse property patches * We can match property patches * We can write patched properties to tmp files * We can install those prop changes to files and dirs (not the wc-root dir yet) Todo -- * Decide on how we want notification for property patches. * Decide on how to handle SVN_ERR_ILLEGAL_TARGET (i.e. returned from svn_wc_prop_set() when trying to set svn:executable on a dir or svn:ignore on a file. * Decide on how to handle SVN_ERR_BAD_MIMETYPE (if svn:mime-type is not valid). * Allow wc-root dirs to be patched for props * Fix a bug in how 'svn diff' displays context lines for overlapping hunks when dealing with property diffs. (diff_test 54) * Make 'svn patch' notify if a property to be modified does not exist. * Make 'svn patch' notify if a property to be deleted does not exist. * Tests with different eol. * Tests with many different kinds of props on a target (add/del/mod). * Decide on how to handle binary props. Git diff format == * We can create add/del/mod/copy patches for WC-WC diffs (if using --git-diff flag and --show-copies-as-adds for copies) * We can create add/del/mod patches for URL-WC and URL-URL diffs * We can parse git diffs. Todo --- * Remove the --git-diff flag and make the git diff format standard. The flag was introduced to avoid excessive use of #ifdefs and since I wrongly interpreted the git unidiff format to not include hunks for deleted paths. If someone has objections to use the git diff format as standard, do speak up! * Record copyfrom for URL-WC * Record copyfrom for URL-URL * Rearrange a bit in the patch code to allow us to keep the current behaviour for unidiff where we delete files where all lines are deleted and add files that did not previously exist. But we should also be able to handle explicit add/del/copy/move operations that were recorded from a git unidiff patch. * base85 encode binary content Note that we won't be able to track moves unless we start matching copyfrom with deleted paths and that may get a bit involved. We might just seattle for tracking copies at the moment. Cheers, Daniel
[WIP] Make the patch code accept dirs as targets for prop only patches
Hi Stefan!
A few questions and a WIP patch for making the patch code deal with
property targets being dirs.
* How should we deal with SVN_ERR_ILLEGAL_TARGET? svn_wc_prop_set()
throws one of those if we for instance try to set svn:executable on a
dir or svn:ignore on a file. Should we do some kind of skipped prop
notification?
* Are you ok with the change I made in resolve_target_path()? If we only
have property changes to a file than it's ok to have a dir as a
target.
* What should we do if the target dir for a property is locally_deleted?
I think we should skip it (that's what the code does right now).
I said to you earlier on IRC that I didn't quite understand how we
determined if we had a valid patch target. Do you agree with my doc
change for resolve_target_path() and the two sections below?
* Do you agree on this definition:
! target-skipped = We have an add/del/mod for target
target-node_kind_on_disk = We have a mod for target. (special case:
if we're adding to an existing file we might have an add if the target
already is the expected result after patching).
* For properties I have:
! target-skipped is_prop_hunk = We have an add/del/mod for
prop_target.
prop_target-content_info-stream = We have a mod for prop_target.
My recent work has been an attempt to make the matching/applying part as
generic as possible, e.g. work for both properties and texts. My limited
testing suggests I've succeeded with that. But I'll have to introduce a
couple of if statements and flags to distinguish between props and text
changes.
[[[
Enable the patch code to use dirs as targets if we only have property
changes.
* subversion/libsvn_client/patch.c
(patch_target_t): Add fields 'has_text_changes' and 'has_prop_changes'
to be able to decide if we should install tmp files for text and/or
props. It's needed since we may have a dir as a target for a
property. If we'd try to copy the tmp file for the text changes onto
a dir we get an error.
(resolve_target_path): Add new parameter 'only_prop_changes' to help
us determine the case when a dir is a valid target.
(init_patch_target): Update caller of resolve_target_path().
(apply_hunk): Record if we've done any changes to props or text
content.
(apply_patches): Only call install_patched_target() if we have changed
the text contents.
]]]
Thanks,
Daniel
Index: subversion/libsvn_client/patch.c
===
--- subversion/libsvn_client/patch.c (revision 979179)
+++ subversion/libsvn_client/patch.c (arbetskopia)
@@ -186,6 +186,12 @@ typedef struct patch_target_t {
/* True if the target has the executable bit set. */
svn_boolean_t executable;
+ /* True if the patch changes the text of the target */
+ svn_boolean_t has_text_changes;
+
+ /* True if the patch changes any of the properties of the target */
+ svn_boolean_t has_prop_changes;
+
/* All the information that is specifict to the content of the target. */
target_content_info_t *content_info;
@@ -303,6 +309,9 @@ obtain_eol_and_keywords_for_file(apr_hash_t **keyw
* If possible, determine TARGET-WC_PATH, TARGET-ABS_PATH, TARGET-KIND,
* TARGET-ADDED, and TARGET-PARENT_DIR_EXISTS.
* Indicate in TARGET-SKIPPED whether the target should be skipped.
+ * The target should be skipped if the versioned path does not exist or does
+ * not match our expected type, e.g. it should be a file unless we're
+ * dealing with a patch that has ONLY_PROP_CHANGES.
* STRIP_COUNT specifies the number of leading path components
* which should be stripped from target paths in the patch.
* Use RESULT_POOL for allocations of fields in TARGET.
@@ -312,6 +321,7 @@ resolve_target_path(patch_target_t *target,
const char *path_from_patchfile,
const char *local_abspath,
int strip_count,
+svn_boolean_t only_prop_changes,
svn_wc_context_t *wc_ctx,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
@@ -341,6 +351,8 @@ resolve_target_path(patch_target_t *target,
{
target-local_relpath = svn_dirent_is_child(local_abspath, stripped_path,
result_pool);
+
+ /* ### We need to allow setting props on the wc root dir */
if (! target-local_relpath)
{
/* The target path is either outside of the working copy
@@ -403,7 +415,9 @@ resolve_target_path(patch_target_t *target,
}
SVN_ERR(svn_wc_read_kind(target-db_kind, wc_ctx, target-local_abspath,
FALSE, scratch_pool));
- if (target-db_kind == svn_node_dir)
+
+ /* We allow properties to have dirs as targets */
+ if (target-db_kind == svn_node_dir ! only_prop_changes)
{
/* ### We cannot yet replace a locally deleted dir with a file,
* ### but some day we might want to allow it. */
Problems in svn patch when adding a file and the parent dir is scheduled for deletion
Hi! Posting this one here so I don't forget about it. The patch code should not change the wc when run with the --dry-run option but it will if we're adding a file with a parent scheduled for deletion. The patch code will add a file with a parent scheduled for deletion to the filesystem but not fail when trying to add it to version control. It would be better if the parent_dir and path would not be created or atleast that 'svn patch new.diff' and 'svn patch new.diff --dry-run' caused the same output. Run these commands on a greek-tree wc: [[[ echo This is the file 'new'. A/C/new $SVN add A/C/new $SVN di new.diff $SVN revert A/C/new rm A/C/new $SVN rm A/C LC_ALL=C $SVN patch new.diff # Here we get a message about 'Can't add 'new' to a parent dir scheduled for deletion. # But 'svn patch' still creates A/C/new. # If I had used 'svn patch new.diff --dry-run' I would have gotten: # 'AA/C/new' and the missing parent would have been deleted. rm -r A/C #LC_ALL=C gdb $SVN patch new.diff # Here we get a message about 'Can't create temporary file from template A/C/svn-' # The patch code copies the tmp file to the parent dir of the target file before renaming it. # Another error message would probably be a good idea. ]]] Cheers, Daniel
Re: Problems in svn patch when adding a file and the parent dir is scheduled for deletion
On Mon, Jul 26, 2010 at 01:04:26PM +0200, Daniel Näslund wrote: Hi! Posting this one here so I don't forget about it. The patch code should not change the wc when run with the --dry-run option but it will if we're adding a file with a parent scheduled for deletion. The patch code will add a file with a parent scheduled for deletion to the filesystem but not fail when trying to add it to version control. It would be better if the parent_dir and path would not be created or atleast that 'svn patch new.diff' and 'svn patch new.diff --dry-run' caused the same output. Run these commands on a greek-tree wc: [[[ echo This is the file 'new'. A/C/new $SVN add A/C/new $SVN di new.diff $SVN revert A/C/new rm A/C/new $SVN rm A/C LC_ALL=C $SVN patch new.diff # Here we get a message about 'Can't add 'new' to a parent dir scheduled for deletion. # But 'svn patch' still creates A/C/new. # If I had used 'svn patch new.diff --dry-run' I would have gotten: # 'AA/C/new' and the missing parent would have been deleted. Err, should be '... and the missing parent would have been created.' rm -r A/C #LC_ALL=C gdb $SVN patch new.diff # Here we get a message about 'Can't create temporary file from template A/C/svn-' # The patch code copies the tmp file to the parent dir of the target file before renaming it. # Another error message would probably be a good idea. ]]]
Re: [WIP] Make the patch code accept dirs as targets for prop only patches
On Mon, Jul 26, 2010 at 01:07:14PM +0200, Stefan Sperling wrote:
On Mon, Jul 26, 2010 at 09:59:01AM +0200, Daniel Näslund wrote:
Index: subversion/libsvn_client/patch.c
===
--- subversion/libsvn_client/patch.c(revision 979179)
+++ subversion/libsvn_client/patch.c(arbetskopia)
@@ -303,6 +309,9 @@ obtain_eol_and_keywords_for_file(apr_hash_t **keyw
* If possible, determine TARGET-WC_PATH, TARGET-ABS_PATH, TARGET-KIND,
* TARGET-ADDED, and TARGET-PARENT_DIR_EXISTS.
* Indicate in TARGET-SKIPPED whether the target should be skipped.
+ * The target should be skipped if the versioned path does not exist or
does
+ * not match our expected type, e.g. it should be a file unless we're
+ * dealing with a patch that has ONLY_PROP_CHANGES.
I'd say don't put the e.g. it should ... part in the docstring of the
function. It has nothing to do with the interface the caller uses.
The caller does not care how this is done internally.
In general, providing an example in this way doesn't really help,
because it makes the reader ask ok, fine, so you've told me one
of the rules... what are the other rules? So it's better to just
put explicit explanations of internal workings of the function into
comments inside the function's body.
Ok, I agree with you on the badness of using examples, but I do think it
would benefit the reader to have a clear definition of when a target
should be skipped.
* STRIP_COUNT specifies the number of leading path components
* which should be stripped from target paths in the patch.
* Use RESULT_POOL for allocations of fields in TARGET.
@@ -312,6 +321,7 @@ resolve_target_path(patch_target_t *target,
const char *path_from_patchfile,
const char *local_abspath,
int strip_count,
+svn_boolean_t only_prop_changes,
I'd prefer call this prop_changes_only everywhere,
but that's a matter of taste.
Either way is fine by me.
svn_wc_context_t *wc_ctx,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
@@ -341,6 +351,8 @@ resolve_target_path(patch_target_t *target,
{
target-local_relpath = svn_dirent_is_child(local_abspath,
stripped_path,
result_pool);
+
+ /* ### We need to allow setting props on the wc root dir */
if (! target-local_relpath)
{
/* The target path is either outside of the working copy
@@ -403,7 +415,9 @@ resolve_target_path(patch_target_t *target,
}
SVN_ERR(svn_wc_read_kind(target-db_kind, wc_ctx, target-local_abspath,
FALSE, scratch_pool));
- if (target-db_kind == svn_node_dir)
+
+ /* We allow properties to have dirs as targets */
I'd suggest the following comment instead:
/* If the target is a version directory not missing from disk,
* and there are only property changes in the patch, we accept
* a directory target. Else, we skip directories. */
Yeah, that's better.
+ if (target-db_kind == svn_node_dir ! only_prop_changes)
{
/* ### We cannot yet replace a locally deleted dir with a file,
* ### but some day we might want to allow it. */
@@ -502,6 +516,7 @@ init_patch_target(patch_target_t **patch_target,
{
patch_target_t *target;
target_content_info_t *content_info;
+ svn_boolean_t only_prop_changes = patch-hunks-nelts == 0 ? TRUE :
FALSE;
Don't use ? TRUE : FALSE. It's redundant, and AFAIK e.g. Julian has
been trying to get rid of these everywhere. This is enough:
svn_boolean_t only_prop_changes = patch-hunks-nelts == 0;
Oups.
Is it a given that if text changes are empty, we only have property changes?
Might be better to also make it check for the existence of property hunks.
In case we add something else later. Dunno. It just seems more robust to
write if (!A B) instead of just (!A) if you also rely on B to be TRUE.
Ouch, good catch. I was sure that svn_diff_parse_next_patch() set the
patch to NULL if it didn't contain a patch. That wasn't the case so I
need to check for (!A B).
content_info = apr_pcalloc(result_pool, sizeof(*content_info));
@@ -525,8 +540,8 @@ init_patch_target(patch_target_t **patch_target,
target-pool = result_pool;
SVN_ERR(resolve_target_path(target, patch-new_filename,
- base_dir, strip_count, wc_ctx,
- result_pool, scratch_pool));
+ base_dir, strip_count, only_prop_changes,
+ wc_ctx, result_pool, scratch_pool));
if (! target-skipped)
{
const char *diff_header;
@@ -1350,6 +1365,11 @@ apply_hunk(patch_target_t *target, target_content_
while (! eof
[PATCH] Create text_info_t for storing text related parts of patch_target_t
Hi Stefan!
Do you think the attached patch looks reasonable, e.g. it does not
interfer with the overall design of 'svn patch'? It's not fun to review
a patch that touches on so many parts. Sorry 'bout that. Find below some
specific questions.
The patch code consist of four steps:
1) match text in streams
2) apply text to target stream (or reject stream)
3) install tmp files by copying them to the target path
4) send notifications on what we've done.
Question 1
text_info_t is the datatype that will be passed around in step 1 and 2.
Is the name ok? I tried target_info_t first but it sounded too vague. On
the other hand, the doc strings for scan_for_match() and friends will
sound strange:
Scan lines of TEXT_INFO for a match of the original text of HUNK...
Question 2
--
In step 3, we're passing both patch_target_t and text_info_t as
parameters to apply_hunk() and reject_hunk() since we might need
target-path and target-had_rejects. Is it ok to pass both?
[[[
Create text_info_t for storing text related parts of patch_target_t.
This is a step towards handling properties in the patch code. The idea
is that text_info_t should be able to describe any text, be it from a
file or a property. But for now we're only dealing with texts from a
file.
* subversion/libsvn_client/patch.c
(text_info_t): New.
(patch_target_t): Fields related to the text have been moved to the
field 'text_info'
(init_patch_target): Initialize 'text_info'.
(resolve_target_wc_file_info,
(read_line,
(seek_to_line,
(match_hunk,
(scan_for_match): Replace 'target-' with 'text_info-' for fields
that have moved.
(match_existing_file): Use 'target-text_info' for accessing the
streams. We still need other fields from the target and text_info
can only refer to text_info of the file in this case.
(get_hunk_info): Add 'text_info' parameter. We still need target for
match_existing_file().
(copy_lines_to_target): Add 'text_info' parameter.
(reject_hunk,
(apply_hunk): Add 'text_info' parameter. We still need target for
marking a target as having rejects.
(send_patch_notification): Uses text_info-hunks
(apply_one_patch): Pass around target-text_info to the match and
apply/reject functions.
]]]
Thanks,
Daniel
Index: subversion/libsvn_client/patch.c
===
--- subversion/libsvn_client/patch.c(revision 964865)
+++ subversion/libsvn_client/patch.c(arbetskopia)
@@ -65,28 +65,7 @@ typedef struct hunk_info_t {
int fuzz;
} hunk_info_t;
-typedef struct patch_target_t {
- /* The patch being applied. */
- const svn_patch_t *patch;
-
- /* The target path as it appeared in the patch file,
- * but in canonicalised form. */
- const char *canon_path_from_patchfile;
-
- /* The target path, relative to the working copy directory the
- * patch is being applied to. A patch strip count applies to this
- * and only this path. This is never NULL. */
- const char *local_relpath;
-
- /* The absolute path of the target on the filesystem.
- * Any symlinks the path from the patch file may contain are resolved.
- * Is not always known, so it may be NULL. */
- char *local_abspath;
-
- /* The target file, read-only, seekable. This is NULL in case the target
- * file did not exist prior to patch application. */
- apr_file_t *file;
-
+typedef struct text_info_t {
/* A stream to read lines form the target file. This is NULL in case
* the target file did not exist prior to patch application. */
svn_stream_t *stream;
@@ -98,16 +77,10 @@ typedef struct hunk_info_t {
* so EOL transformation and keyword contraction is done transparently. */
svn_stream_t *patched;
- /* Path to the temporary file underlying the result stream. */
- const char *patched_path;
-
/* The reject stream, write-only, not seekable.
* Hunks that are rejected will be written to this stream. */
svn_stream_t *reject;
- /* Path to the temporary file underlying the reject stream. */
- const char *reject_path;
-
/* The line last read from the target file. */
svn_linenum_t current_line;
@@ -127,6 +100,44 @@ typedef struct hunk_info_t {
/* An array containing hunk_info_t structures for hunks already matched. */
apr_array_header_t *hunks;
+ /* True if end-of-file was reached while reading from the target. */
+ svn_boolean_t eof;
+
+ /* The keywords of the target. */
+ apr_hash_t *keywords;
+
+ /* The pool the target_info is allocated in. */
+ apr_pool_t *pool;
+} text_info_t;
+
+typedef struct patch_target_t {
+ /* The patch being applied. */
+ const svn_patch_t *patch;
+
+ /* The target path as it appeared in the patch file,
+ * but in canonicalised form. */
+ const char *canon_path_from_patchfile;
+
+ /* The target path, relative to the working copy directory the
+ * patch is being applied to. A patch strip count applies to this
+ * and only this path. This is never NULL. */
+
Re: Non-matching revision arguments in the diff code
On Fri, Jul 09, 2010 at 12:36:35PM +0100, Julian Foad wrote:
On Fri, 2010-07-09 at 09:52 +0200, Daniel Näslund wrote:
Hi!
The diff callbacks for handling text have revision arguments. The prop
callbacks don't.
I'm creating diff headers for the cases when we only have property
changes. When creating those diff headers I need revisions. Since the
prop diff callback doesn't have revision arguments I'm using the
revision fields in struct diff_cmd_baton. Here's what the doc string says
about those fields.
[[[
/* These are the numeric representations of the revisions passed to
svn_client_diff5, either may be SVN_INVALID_REVNUM. We need these
because some of the svn_wc_diff_callbacks4_t don't get revision
arguments.
### Perhaps we should change the callback signatures and eliminate
### these?
*/
svn_revnum_t revnum1;
svn_revnum_t revnum2;
]]]
But they don't match the revisions returned by the diff callbacks.
Here's the output from diff_content_changed() comparing what the baton
fields says and what the revision argument says (from diff_tests 32):
$ svn di -r 1 | grep DBG
DBG: diff.c: 692: btn-rev1 1, rev1 0, btn-revn2 -1, rev2 3
DBG: diff.c: 692: btn-rev1 1, rev1 0, btn-revn2 -1, rev2 3
What this output actually refers to is two added files, thus the start
revision is set to 0. That kinda makes sense. But for some reason the
diff callback gets called with rev2 set to the actual revision of the
path instead of SVN_INVALID_REVNUM (meaning we're referring to the
working copy revision).
An excerpt from libsvn_wc/diff.c::file_diff():
[[[
/* A wc-wc diff of replaced files actually shows a diff against the
* revert-base, showing all previous lines as removed and adding all new
* lines. This does not happen for copied/moved-here files, not even
* with show_copies_as_adds == TRUE (in which case copy/move is really
* shown as an add, diffing against the empty file). So show the
* revert-base revision for plain replaces. */
if (replaced
! (status == svn_wc__db_status_copied
|| status == svn_wc__db_status_moved_here))
revision = revert_base_revnum;
]]]
Appearantly we're adjusting the revision for the case of replaced files.
Still we could return SVN_INVALID_REVNUM for all the rest since this
part of the code only deals with diffs against the WC.
For rev2, it looks like the caller is providing SVN_INVALID_REVNUM to
mean HEAD, and then something is discovering the current value of HEAD
is 3, but is not updating btn-revn2 with that information. I think,
if possible, the code that discovers the actual HEAD revision number
should store it in the baton so that everything else can refer to it.
If this discovery happens on the client side, that should be possible.
I thought SVN_INVALID_REVNUM was used for marking WORKING, see
libsvn_client/diff.c::diff_label():
[[[
static const char *
diff_label(const char *path,
svn_revnum_t revnum,
apr_pool_t *pool)
{
const char *label;
if (revnum != SVN_INVALID_REVNUM)
label = apr_psprintf(pool, _(%s\t(revision %ld)), path, revnum);
else
label = apr_psprintf(pool, _(%s\t(working copy)), path);
return label;
}
]]]
Removed the XFail markers around diff_tests 32 in r962788.
Thanks,
Daniel
Non-matching revision arguments in the diff code
Hi! The diff callbacks for handling text have revision arguments. The prop callbacks don't. I'm creating diff headers for the cases when we only have property changes. When creating those diff headers I need revisions. Since the prop diff callback doesn't have revision arguments I'm using the revision fields in struct diff_cmd_baton. Here's what the doc string says about those fields. [[[ /* These are the numeric representations of the revisions passed to svn_client_diff5, either may be SVN_INVALID_REVNUM. We need these because some of the svn_wc_diff_callbacks4_t don't get revision arguments. ### Perhaps we should change the callback signatures and eliminate ### these? */ svn_revnum_t revnum1; svn_revnum_t revnum2; ]]] But they don't match the revisions returned by the diff callbacks. Here's the output from diff_content_changed() comparing what the baton fields says and what the revision argument says (from diff_tests 32): $ svn di -r 1 | grep DBG DBG: diff.c: 692: btn-rev1 1, rev1 0, btn-revn2 -1, rev2 3 DBG: diff.c: 692: btn-rev1 1, rev1 0, btn-revn2 -1, rev2 3 I was under the impression that a revision is something fixed (after we've done the peg revision dance). Any suggestion to why the revisions may differ? Sidenote: svn diff -r PREV:BASE shows 'working copy' as revision in the diff label. I'm thinking 'working copy' must involve the working changes. Thanks, Daniel
Things left to do for 'svn patch --git-diff'
Hi!
Just trying to sort out what work lays ahead.
Things todo for property diffs
---
1) Make svn diff print diff headers even if we have just property
changes.
2) Make svn patch able to match (compare streams) property hunks
3) Make svn patch able to apply (write to stream) property hunks
4) Make svn patch able to install (copy contents of stream to target
location) property hunks. Observe that it means that we should be
able to deal with directories too since a property can be set on either
a dir or a file.
5) Make parse_next_hunk() record the operation {del/add/mod} on
properties.
6) Make svn patch able to install property hunks with the help of
the operation kind.
7) Make svn patch do proper notifications for property hunks.
8) Write tests that applies both text and property changes.
9) Find a way to split/extend/make new new encapsulating type for
patch_target_t. It needs to both handle props and texts. Much of the
code expects a patch_target_t argument.
10) Abort if we find binary diffs for properties?
Things todo for git diffs
---
1) Make svn diff create git diffs for copied paths without
--show-copies-as-adds.
2) Move the writing of git diff headers from libsvn_client to
libsvn_diff. If we do that, we'd have to move the creation of diff
labels too.
3) Make the git diff parser more strict.
4) Be able to install added/deleted/copied paths. Moved is too hard at
the moment. Note that we don't want to abandon the current way of
detecting adds/dels for patches in non-git-format.
5) Disallow the --git-diff flag for URL-URL diffs until we support
copyfrom detection. Maybe disallow it for URL-WC too. We can only
detect the copyfrom for the WC changes.
6) Do base85 encoding of binary contents.
Cheers,
Daniel
Re: svn commit: r958260 - /subversion/trunk/subversion/libsvn_client/diff.c
On Sat, Jun 26, 2010 at 04:47:46PM -0400, Greg Stein wrote: On Sat, Jun 26, 2010 at 13:56, dan...@apache.org wrote: Author: dannas Date: Sat Jun 26 17:56:06 2010 New Revision: 958260 URL: http://svn.apache.org/viewvc?rev=958260view=rev Log: With SVN_EXPERIMENTAL_PATCH defined, only print the git diff header I still don't understand why we have SVN_EXPERIMENTAL_PATCH. The whole patch feature is in-progress. What funny business needs to be protected? There is no legacy right way where changes need to be excluded. The original thought was that we didn't need a commandline switch since the extra header lines would be considered noise by non-git aware patch applications. I thought that I would use the ifdef until the diff code had reached a stable state and then I could remove them and alter the test output of all the diff tests. If you're altering the *diff* output, to enable some Git diff headers, then please add a (cmdline) option to do that. The default can't change for compat reasons, so the option will be required. If we use the git format, then deletes, copies and moves may not contain more than the header lines. In that case, patch applications that could process svn diff output for 1.6 would not be able to do it for svn version with git diffs. If this #define is protecting something else, then ... what? and why would PATCH be in the name? SVN_EXPERIMENTAL_PATCH since I expected to do some changes in the svn patch code too using ifdefs. I could have used SVN_EXPERIMENTAL_DIFF instead. I justed wanted one define to turn on to enable the feature. Since r958537, there is a diff extension switch to enable git diffs: svn diff -x --git-diff [-g] In hindsight, I'm not sure that was the best thing to do. I read this part from svn help diff: [[[ -x --extensions ARG : Default: '-u'. When Subversion is invoking an external diff program, ARG is simply passed along to the program. But when Subversion is using its default internal diff implementation, or when Subversion is displaying blame annotations, ARG could be any of the following: ]]] I thought that --extensions would just extend the diff format and could be usable only from within the internal diff application (we don't pass on the information needed for creating git diffs to the external diff tool) but the svn book says: [[[ --extensions (-x) ARGS Specifies an argument or arguments that Subversion should pass to an external diff command when providing differences between files. If you wish to pass multiple arguments, you must enclose all of them in quotes (for example, svn diff --diff-cmd /usr/bin/diff -x -b -E). This switch can only be used if you also pass the --diff-cmd switch. ]]] Thanks, Daniel
Re: svn commit: r958260 - /subversion/trunk/subversion/libsvn_client/diff.c
On Mon, Jun 28, 2010 at 02:38:26PM +0200, Daniel Näslund wrote: Since r958537, there is a diff extension switch to enable git diffs: svn diff -x --git-diff [-g] In hindsight, I'm not sure that was the best thing to do. I read this part from svn help diff: [[[ -x --extensions ARG : Default: '-u'. When Subversion is invoking an external diff program, ARG is simply passed along to the program. But when Subversion is using its default internal diff implementation, or when Subversion is displaying blame annotations, ARG could be any of the following: ]]] I thought that --extensions would just extend the diff format and could be usable only from within the internal diff application (we don't pass on the information needed for creating git diffs to the external diff tool) but the svn book says: [[[ --extensions (-x) ARGS Specifies an argument or arguments that Subversion should pass to an external diff command when providing differences between files. If you wish to pass multiple arguments, you must enclose all of them in quotes (for example, svn diff --diff-cmd /usr/bin/diff -x -b -E). This switch can only be used if you also pass the --diff-cmd switch. ]]] Reverted r958537. I will use a regular command line switch instead. Daniel
How does the workqueues provide atomic updates, or do they?
Hi! Some questions about how the workqueues... work. Have I got it right? How are files installed during an update? We acquire a write lock We crawl the wc We tell the repos about our state The repo runs an editor open_root(): We set status to _status_incomplete open_directory(): We set status t _status_incomplete. close_file(): We run the wq close_directory(): We run the wq close_edit(): Remove status_incomplete release write_lock What if we get interrupted? We have this incoming change: M A/B/lambda M A/B/beta But we only process lambda and get cancelled before we start processing beta. Then there's no wq started for beta. How does the wq know that the update isn't complete? Or is the status that tells us that the update is not complete? E.g. the workqueues provide write atomicity but it's the status that tells us if the whole operation has succeded? I think it works like this: -- * If the next operation tries to acquire a write lock it will fail and tell the user to run 'svn cleanup' * svn_wc_cleanup() will run all remaining work items in the different workqueues (will be one queue when we have one db). * Something detects that the update is incomplete and resumes it *or* something detects that the update is incomplete and reverts what has been done so far. I assume that the best thing would have been if all items could have been placed in one workqueue and then be run but that we don't do that since we don't want to increase the memory usage or slow down the system by writing a lot of tempfiles holding the incoming changes. My problem is that I haven't found the code parts that deals with status_incomplete. I only see: If write locks already held - run the items left in the workqueues. Thanks, Daniel
[WIP] #1957 'svn switch does not update keywords'
Hi!
Is 'svn switch does not update keywords' a problem that needs solving?
Is the performance of this solution acceptable (traverse the entire wc
one more time after the editor drive)?
Alternatives?
* Custom db-query that only returns the paths with matching property set
on them. Could be used by 'svn pg -R' later on.
* Do the recording during the crawling of the wc. Saves us the extra
traversal.
And why is it not working? :)
I'm translating the file in the WC back into Normal Form and then I
install it through the workqueue mecanism. It does get translated but
not to the current URL but to the previous.
[[[
Fix issue #1975 - 'svn switch does not update keywords'.
Only files affected by the switch gets keyword translation, possibly
making some $URL$ keywords incorrect. We do a complete traversal of
the WC to check for the files that have svn:keywords pristine props
set and retranslate those files. */
* subversion/libsvn_wc/update_editor.c
(reinstall_target_baton): New
(reinstall_target_with_keywords): New.
(close_edit): walk all children and call
reinstall_target_with_keywords().
]]]
Daniel
Index: subversion/libsvn_wc/update_editor.c
===
--- subversion/libsvn_wc/update_editor.c(revision 957424)
+++ subversion/libsvn_wc/update_editor.c(arbetskopia)
@@ -5080,13 +5080,83 @@ close_file(void *file_baton,
return SVN_NO_ERROR;
}
+struct reinstall_target_baton
+{
+ svn_wc__db_t *db;
+ svn_cancel_func_t cancel_func;
+ void *cancel_baton;
+ /* ### Do we need a result_pool here? How is the apr pool cleanup handler
+ * configured? We need a pool with the handler activated in case we get
+ * cancelled. ### */
+};
+
+/* Reinstall target to assure svn:keywords expand correctly. */
+static svn_error_t *
+reinstall_target_with_keywords(const char *local_abspath,
+ void *walk_baton,
+ apr_pool_t *scratch_pool)
+{
+ struct reinstall_target_baton *rtb = walk_baton;
+ apr_hash_t *props;
+ const char *tmptext;
+
+ SVN_DBG((reinstall_target() %s\n, local_abspath));
+
+ SVN_ERR(svn_wc__get_pristine_props(props, rtb-db, local_abspath,
+ scratch_pool, scratch_pool));
+
+ /* ### There must be a constant for svn:keywords somewhere, but where? */
+ if (props apr_hash_get(props, svn:keywords, APR_HASH_KEY_STRING))
+{
+ svn_skel_t *all_work_items;
+ svn_skel_t *work_item;
+
+ SVN_DBG((svn:keywords found\n));
+ SVN_ERR(svn_wc__internal_translated_file(tmptext, local_abspath,
+ rtb-db, local_abspath,
+ SVN_WC_TRANSLATE_TO_NF
+ |
SVN_WC_TRANSLATE_NO_OUTPUT_CLEANUP,
+ rtb-cancel_func,
+ rtb-cancel_baton,
+ scratch_pool, scratch_pool));
+
+ SVN_ERR(svn_wc__wq_build_file_install(all_work_items, rtb-db,
+local_abspath,
+tmptext, /* install_from */
+TRUE, /* use_commit_times */
+TRUE, /* record_fileinfo */
+scratch_pool, scratch_pool));
+ SVN_ERR(svn_wc__wq_build_file_remove(work_item, rtb-db, tmptext,
+ scratch_pool, scratch_pool));
+ all_work_items = svn_wc__wq_merge(all_work_items, work_item,
+scratch_pool);
+
+ /* ### Should we really run the wq here? close_file() and
+ * close_directory() runs wq's so I assume that we want them performed
+ * on each item. I had this notion about wq's as something containing
+ * all items in a WC, e.g. we check what needs to be done and then do
+ * it all in one batch. What happens if we have three files done but
+ * three more should have been installed when we get an interrupt?
+ * Does the editor have some way of marking what has been done and
+ * what has not? E.g. if we run svn_wc_cleanup() on a WC, we're
+ * supposed to finish the remaining work items but the update_editor
+ * may not have recorded all work items, right? Do we check that
+ * revisions match or what? ### */
+ SVN_ERR(svn_wc__wq_run(rtb-db, local_abspath, rtb-cancel_func,
+ rtb-cancel_baton, scratch_pool));
+}
+
+ return SVN_NO_ERROR;
+}
+
/* An svn_delta_editor_t function. */
static svn_error_t *
close_edit(void *edit_baton,
apr_pool_t *pool)
{
struct edit_baton *eb = edit_baton;
+ struct reinstall_target_baton rtb;
/* If there is a target and that target is missing, then it
Re: How does the workqueues provide atomic updates, or do they?
On Thu, Jun 24, 2010 at 09:20:22AM +0100, Philip Martin wrote: Daniel Näslund dan...@longitudo.com writes: How are files installed during an update? We acquire a write lock We crawl the wc We tell the repos about our state The repo runs an editor open_root(): We set status to _status_incomplete open_directory(): We set status t _status_incomplete. close_file(): We run the wq close_directory(): We run the wq close_directory will remove status_incomplete for the directory, see maybe_bump_dir_info. At present it's a direct database operation but it may become part of the wq for the directory. close_edit(): Remove status_incomplete release write_lock What if we get interrupted? We have this incoming change: M A/B/lambda M A/B/beta But we only process lambda and get cancelled before we start processing beta. Then there's no wq started for beta. How does the wq know that the update isn't complete? The wq doesn't know or care. All the wq knows is whether there are outstanding items. Or is the status that tells us that the update is not complete? E.g. the workqueues provide write atomicity but it's the status that tells us if the whole operation has succeded? The status_incomplete remains on the directory. It will remain until the user runs update successfully. I think it works like this: -- * If the next operation tries to acquire a write lock it will fail and tell the user to run 'svn cleanup' * svn_wc_cleanup() will run all remaining work items in the different workqueues (will be one queue when we have one db). Yes. Although note that pool cleanups attempt to run any outstanding wq so controlled cancel will often run any wq and remove locks, just leaving the incomplete status. Ok. For update that would be update_editor.c::cleanup_dir_baton() that gets registered in cleanup_dir_baton_child() with a call to apr_pool_cleanup_kill(). (Just putting it here for reference). * Something detects that the update is incomplete and resumes it *or* something detects that the update is incomplete and reverts what has been done so far. No. In future it may change but at present the user needs to invoke update to finish the operation. Thanks for your answers! Daniel
Re: [Patch] - Fix paths on OS/2 - svn-paths.diff [1/1]
On Fri, Jun 25, 2010 at 02:03:17AM +, Paul Smedley wrote: [[[ Fix paths command prompts for OS/2 by re-using WIN32 #ifdef's * subversion/libsvn_subr/dirent_uri.c * subversion/libsvn_subr/io.c * subversion/libsvn_subr/prompt.c * subversion/libsvn_ra_local/split_url.c * subversion/libsvn_fs_fs/fs_fs.c (various): Use WIN32 code for paths with drive letters and command prompts ]]] I (and probably other people too) have a hard time reading your patch [1]. :) Why does it look like it's base64 encoded? Cheers, Daniel [1] http://svn.haxx.se/dev/archive-2010-06/0394.shtml
Re: How use patch_target_t for both files and (possibly multiple) properties
On Mon, Jun 21, 2010 at 11:12:43AM +0200, Stefan Sperling wrote:
On Sun, Jun 20, 2010 at 09:53:29PM +0200, Daniel Näslund wrote:
Hi stsp!
Attaching a patch showing a suggestion on how to split up
patch_target_t. Maybe you have some insights on this, before I rush out
and start grooking around and making changes to your design.
What 'svn patch' does
--
1) Match hunks
2) Apply hunks to a stream backed up by a tmp file
3) Install. copy the tmp file to the target location
4) Notify the user on what has happened, e.g. fuzz, offset and rejects
The problem
-
Each property is essentially like it's own text file. 'svn patch' must
be generic enough to be able to perform the above actions on both
properties and texts.
Alternatives
--
* Simplify. Don't use the unidiff format but just copy the content
straight off and apply it.
I don't think I understand the above. What do you mean?
I was just thinking: We usually have properties that are limited in
size. We could just put the whole contents of the property after a header
in the diff file, e.g. not use the unidiff format. Then we could ignore
step 1) and 2) and just install the property and do the notification
according to what the diff header said, e.g added, deleted or modified.
I'm not proposing that we do it like that, but it's just that using
diffs for something that is typically one line or one word is a lot of
overhead. *But*, the properties can be longer; Tortoisesvn has a
tsvn:logtemplate property for instance.
* Duplicate. Create special code for handling props, e.g. have both
match_hunk() and match_property_hunk() and so on.
What you're proposing below is to duplicate the matching and applying of
properties. My thought was that we've put in a lot of thoughts to the
matching and applying and have encountered quite a few edge cases
involving newlines, fuzz and offsets. I was just hoping that we could
reuse our existing code.
Proposed solution
---
Use patch_target_t as a container for sub-patch_targets that can be either
file-text or a property. Store tree change info needed for step 3)
installing and step 4) notifying.
Your description is quite low-level (detailing new fields in existing
structs etc.) which makes it a bit hard to follow.
I agree on that when rereading my mail, what did I really mean? :)
Let's take a step back. What is the general idea of your approach?
To be able to use properties and text diffs in the same way when
matching and applying hunks.
I'll try making some high-levelI suggestions about how to approach the
problem. Please comment on them (I am most likely not seeing the whole
picture) and we'll work something out.
parse_next_hunk() currently puts both normal and property hunks
in the same list (patch-hunks). Have you considered splitting this into
two separate lists, e.g. patch-hunks and patch-property_hunks?
Since r955844, we put property hunks in patch-property_hunks and text
hunks in patch-hunks. :)
property_hunks can be a hashtable with prop_name as keys and
arrays of hunk_t as value: {prop_name, [hunk1, hunk2, hunk3, ...]}.
Since r955844, that is the approach.
Or it can be a list of structs like { prop_name; array of hunks; }
if that is easier to work with in the caller.
This should provide much cleaner separation, because now you can
leave the original hunk handling code alone, and add extra steps
that handle the property cases.
Regarding parse_next_hunk(), consider splitting this up into two
functions: parse_hunk() and parse_property_hunk().
The first one attempts to find a normal @@ hunk at the current file
offset, and returns a NULL *hunk on parsing failure. If it fails
(the hunk returned is NULL), rewind the file to the seek position
where parsing a normal hunk failed, and parse_property_hunk() to
attempt parsing a ## property hunk.
These functions could probably share some code, but you should keep
them independent for now so we can see how much they really differ.
I considered using that approach but thought that seeking around would
be messy. I didn't have to add too many if statements [1]. I reused the
parse code by allowing atat to be either '@@' or '##'. Maybe I was a bit
too enthusiastic and committed my changes too quickly. I'm able to use
parse_next_hunk() for both properties and hunks but maybe it just
obfuscates the code.
In the patch code, you can then add property patching as a separate step.
Currently, we do something like the following in apply_one_patch():
for hunk in hunks:
match(hunk)
# gather some additional meta data on hunk such as actual line
# offset we'll be using
hunk_info = get_info(hunk)
target.hunks.append(hunk_info)
for hunk_info in target.hunks:
if hunk.rejected:
reject(hunk)
else
apply(hunk)
Rejection and application happens on temporary files, we haven't
modified
How use patch_target_t for both files and (possibly multiple) properties
Hi stsp!
Attaching a patch showing a suggestion on how to split up
patch_target_t. Maybe you have some insights on this, before I rush out
and start grooking around and making changes to your design.
What 'svn patch' does
--
1) Match hunks
2) Apply hunks to a stream backed up by a tmp file
3) Install. copy the tmp file to the target location
4) Notify the user on what has happened, e.g. fuzz, offset and rejects
The problem
-
Each property is essentially like it's own text file. 'svn patch' must
be generic enough to be able to perform the above actions on both
properties and texts.
Alternatives
--
* Simplify. Don't use the unidiff format but just copy the content
straight off and apply it.
* Duplicate. Create special code for handling props, e.g. have both
match_hunk() and match_property_hunk() and so on.
Proposed solution
---
Use patch_target_t as a container for sub-patch_targets that can be either
file-text or a property. Store tree change info needed for step 3)
installing and step 4) notifying.
Use patch_target_stream_t for step 1) matching and step 2) applying. It
should contain streams to the property or text target and the hunks
associated with the specific file/property.
A scratch log message (the attached patch only contains changes to the
structs):
* subversion/libsvn_client/patch.c
(patch_target_stream_t): New. A struct used for handling the match and
apply step of 'svn patch'. It can be either associated with the
content of a file or a property.
(patch_target_t): Move some fields to patch_target_stream_t and
introduce it as a new field.
(get_hunk_info,
scan_for_match,
match_hunk): These funcs makes up step 1) matching. Use
patch_target_stream_t instead of patch_target_t.
(apply_hunk,
reject_hunk): These funcs makes up step 2) applying. Use
patch_target_stream_t instead of patch_target_t.
(init_patch_target): Create the patch_target_stream_t objects.
(apply_one_patch): Loop through the available property hunks in the
patch and do step 1) matching. Loop through the property hunks in
the patch and do step 2) applying.
(...): More things to do.
Cheers,
Daniel
Index: subversion/libsvn_client/patch.c
===
--- subversion/libsvn_client/patch.c(revision 956349)
+++ subversion/libsvn_client/patch.c(arbetskopia)
@@ -61,28 +61,14 @@ typedef struct {
int fuzz;
} hunk_info_t;
+/* ### Better naming. This struct contains streams and hunks associated with
+ * ### a target, be it a file or a property of a file. sub_patch_target_t?`*/
typedef struct {
- /* The patch being applied. */
- const svn_patch_t *patch;
- /* The target path as it appeared in the patch file,
- * but in canonicalised form. */
- const char *canon_path_from_patchfile;
+ /* The name of the property associated with this 'stream' or NULL if it's
+ * a text 'stream'. */
+ const char *prop_name;
- /* The target path, relative to the working copy directory the
- * patch is being applied to. A patch strip count applies to this
- * and only this path. This is never NULL. */
- const char *local_relpath;
-
- /* The absolute path of the target on the filesystem.
- * Any symlinks the path from the patch file may contain are resolved.
- * Is not always known, so it may be NULL. */
- char *local_abspath;
-
- /* The target file, read-only, seekable. This is NULL in case the target
- * file did not exist prior to patch application. */
- apr_file_t *file;
-
/* A stream to read lines form the target file. This is NULL in case
* the target file did not exist prior to patch application. */
svn_stream_t *stream;
@@ -100,13 +86,6 @@ typedef struct {
/* Path to the temporary file underlying the result stream. */
const char *patched_path;
- /* The reject stream, write-only, not seekable.
- * Hunks that are rejected will be written to this stream. */
- svn_stream_t *reject;
-
- /* Path to the temporary file underlying the reject stream. */
- const char *reject_path;
-
/* The line last read from the target file. */
svn_linenum_t current_line;
@@ -119,13 +98,48 @@ typedef struct {
* last line read from the target file was using. */
const char *eol_str;
+ /* True if end-of-file was reached while reading from the target. */
+ svn_boolean_t eof;
+
+ /* An array containing hunk_info_t structures for hunks already matched. */
+ apr_array_header_t *hunks;
+
+} patch_target_stream_t;
+
+
+typedef struct {
+ /* The patch being applied. */
+ const svn_patch_t *patch;
+
+ /* The target path as it appeared in the patch file,
+ * but in canonicalised form. */
+ const char *canon_path_from_patchfile;
+
+ /* The target path, relative to the working copy directory the
+ * patch is being applied to. A patch strip count applies to this
+ * and only this path. This is never NULL. */
+ const char
Progress report for GSoC 'svn patch' project
Hi! The goal is to be able to create, parse and apply diffs in the 'git unidiff' format. The original thought was to add support for property diffs when the basic parts of the 'git unidiff' part was in place, but Stefan suggested that we could easily add property diffs even without the git parts. At the moment I'm working in that direction. What we have for 'property diffs' == * A fixed format using '##' as hunk header delimiters instead of '@@'. Added: prop ## -0,0 +1 ## - value + value Modified: prop ## -0,0 +1 ## - value + value Deleted: prop ## -0,0 +1 ## - value + value * The ability to parse those property hunks since r955844. What needs to be done for 'property diffs' == * A diff header is only added if we have text changes, we need one even if the patch has only property changes. * We need to be able to distinguish between patches that deletes a property and those that sets the property to empty. * The property changes needs to be applied to the target. * More C-tests involving different combinations of text and prop hunks. * C-tests with context lines starting with 'Added: ', 'Modified: ' and 'Deleted'. * C-tests with reverse diffs involving properties. * A clear documentation of the property diff format. * Proper handling of properties with binary content. What we have for 'git unidiffs' * With SVN_EXPERIMENTAL_PATCH we can create git headers for added and deleted paths * We have fields in 'svn_patch_t' for recording what tree operation the patch performs. * C-test for parsing simple 'git diffs', e.g. diffs with either text modifications or tree changes but not both. * Three XFailing unittests (passes with SVN_EXPERIMENTAL_PATCH defined) for wc-wc, url-wc, url-url. What needs to be done for 'git unidiffs' = * The parsing of the git headers. We still need to be able to handle unidiffs. There's a *lot* of if statements in the patch I have. I'm thinking about using something table-driven instead. * C-tests for git diffs that combines tree changes and text mods. * Applying the tree changes. * Create git headers for copied paths. Needs some small rearrangement of diff callbacks but nothing big. * Create git headers for moved paths. We need to keep the deleted paths in a baton and match them against copyfrom information. Can be a bit messy. * Encode and decode binary files. Needs to be done but can't be done at the moment == * Be able to handle copies and renames that's in revisions previous to WORKING. We need editor-v2 for that. With SVN_EXPERIMENTAL_PATCH defined, we create git diff headers for all possible combinatinos of sources, e.g. wc-wc, url-wc and url-url. But we can only track renames and copies for wc-wc and the changes in wc for url-wc. I mean that that since we won't have git diffs available for 1.7 anyway, we can just continue with the goal of releasing the git diff feature when we have proper rename/copy tracking available. Daniel
[WIP] How create diff headers even if we only have property diffs?
Hi!
I need to have a diff header even if there's only property changes for
the sake of parsing.
And I want only one diff header per file. Makes it easier to apply the
patch. For each found diff header we create one svn_patch_t object.
Simple and clean.
What to do? I could add a flag to the diff_cmd_baton and reset it
between each call but, as Greg pointed out, that's like using a global
variable.
I could collect all the paths with affected files and create the diff in
something like close_edit() but there's no such call for
svn_wc_diff_callbacks4_t.
One other approach:
[[[
Make 'svn diff' always write out a diff header if there are changes to a
path, no matter if it's property or text changes.
* subversion/libsvn_client/diff.c
(diff_cmd_baton): New field 'visited_paths' to hold all the paths for
which a diff header has already been written.
(display_prop_diff): Add parameter 'show_diff_header'.
(diff_props_changed): Call display_prop_diffs() with show_diff_header
set to TRUE if the path has not yet been processed.
(diff_content_changed): Add the path to 'visited_paths'.
(svn_client_diff5,
svn_client_diff_peg5): Initialize diff_cmd_baton-visited_paths.
]]]
Feels kinda ugly. Some obvious way to do it that I'm missing? The
alternatives I can come up with would involve revamping the callback
chain to make property changes and text changes end up in one function.
[[[
Index: subversion/libsvn_client/diff.c
===
--- subversion/libsvn_client/diff.c (revision 955844)
+++ subversion/libsvn_client/diff.c (arbetskopia)
@@ -200,6 +200,7 @@ display_prop_diffs(const apr_array_header_t *propc
const char *encoding,
apr_file_t *file,
const char *relative_to_dir,
+ svn_boolean_t show_diff_header,
apr_pool_t *pool)
{
int i;
@@ -294,7 +295,8 @@ display_prop_diffs(const apr_array_header_t *propc
* UNIX patch could apply the property diff to, so we use ##
* instead of @@ as the default hunk delimiter for property diffs.
* We also supress the diff header. */
-SVN_ERR(svn_diff_mem_string_output_unified2(os, diff, FALSE, ##,
+SVN_ERR(svn_diff_mem_string_output_unified2(os, diff,
show_diff_header,
+ ##,
svn_dirent_local_style(path, pool),
svn_dirent_local_style(path, pool),
encoding, orig, val, pool));
@@ -451,6 +453,8 @@ struct diff_cmd_baton {
/* The directory that diff target paths should be considered as
relative to for output generation (see issue #2723). */
const char *relative_to_dir;
+
+ apr_hash_t *visited_paths;
};
/* Generate a label for the diff output for file PATH at revision REVNUM.
@@ -485,15 +489,22 @@ diff_props_changed(const char *local_dir_abspath,
{
struct diff_cmd_baton *diff_cmd_baton = diff_baton;
apr_array_header_t *props;
+ svn_boolean_t show_diff_header;
apr_pool_t *subpool = svn_pool_create(diff_cmd_baton-pool);
SVN_ERR(svn_categorize_props(propchanges, NULL, NULL, props, subpool));
+ if (apr_hash_get(diff_cmd_baton-visited_paths, path, APR_HASH_KEY_STRING))
+show_diff_header = FALSE;
+ else
+show_diff_header = TRUE;
+
if (props-nelts 0)
SVN_ERR(display_prop_diffs(props, original_props, path,
diff_cmd_baton-header_encoding,
diff_cmd_baton-outfile,
diff_cmd_baton-relative_to_dir,
+ show_diff_header,
subpool));
if (state)
@@ -750,6 +761,8 @@ diff_content_changed(const char *path,
/* Destroy the subpool. */
svn_pool_destroy(subpool);
+ apr_hash_set(diff_cmd_baton-visited_paths, path, APR_HASH_KEY_STRING, path);
+
return SVN_NO_ERROR;
}
@@ -1908,6 +1921,7 @@ svn_client_diff5(const apr_array_header_t *options
diff_cmd_baton.force_empty = FALSE;
diff_cmd_baton.force_binary = ignore_content_type;
diff_cmd_baton.relative_to_dir = relative_to_dir;
+ diff_cmd_baton.visited_paths = apr_hash_make(pool);
return do_diff(diff_params, diff_callbacks, diff_cmd_baton, ctx, pool);
}
@@ -1974,6 +1988,7 @@ svn_client_diff_peg5(const apr_array_header_t *opt
diff_cmd_baton.force_empty = FALSE;
diff_cmd_baton.force_binary = ignore_content_type;
diff_cmd_baton.relative_to_dir = relative_to_dir;
+ diff_cmd_baton.visited_paths = apr_hash_make(pool);
return do_diff(diff_params, diff_callbacks, diff_cmd_baton, ctx, pool);
}
]]]
Daniel
Re: svn commit: r952205 - /subversion/trunk/subversion/libsvn_client/diff.c
On Mon, Jun 07, 2010 at 03:33:16PM +0200, Stefan Sperling wrote:
On Mon, Jun 07, 2010 at 12:43:19PM -, dan...@apache.org wrote:
+static svn_error_t *
+print_git_diff_header_copied(svn_stream_t *os, const char
*header_encoding,
+ const char *path, const char *copyfrom_path,
+ apr_pool_t *result_pool)
+{
+ SVN_ERR(svn_stream_printf_from_utf8(os, header_encoding, result_pool,
+ diff --git a/%s b/%s%s,
+ copyfrom_path, path, APR_EOL_STR));
+ SVN_ERR(svn_stream_printf_from_utf8(os, header_encoding, result_pool,
+ copy from %s%s, path,
APR_EOL_STR));
^
+ SVN_ERR(svn_stream_printf_from_utf8(os, header_encoding, result_pool,
+ copy to %s%s, copyfrom_path,
^^^
+ APR_EOL_STR));
Shouldn't the copyfrom_path be printed for copy from, and the path for
copy to?
+static svn_error_t *
+print_git_diff_header_moved(svn_stream_t *os, const char *header_encoding,
+const char *path, const char *copyfrom_path,
+apr_pool_t *result_pool)
+{
+ SVN_ERR(svn_stream_printf_from_utf8(os, header_encoding, result_pool,
+ diff --git a/%s b/%s%s,
+ copyfrom_path, path, APR_EOL_STR));
+ SVN_ERR(svn_stream_printf_from_utf8(os, header_encoding, result_pool,
+ rename from %s%s, path,
+ APR_EOL_STR));
+ SVN_ERR(svn_stream_printf_from_utf8(os, header_encoding, result_pool,
+ rename to %s%s, copyfrom_path,
+ APR_EOL_STR));
Same here.
Doh, fixed in r952344.
Thanks,
Daniel
Re: svn commit: r952205 - /subversion/trunk/subversion/libsvn_client/diff.c
On Mon, Jun 07, 2010 at 03:41:27PM +0200, Stefan Sperling wrote: On Mon, Jun 07, 2010 at 12:43:19PM -, dan...@apache.org wrote: Author: dannas Date: Mon Jun 7 12:43:19 2010 New Revision: 952205 URL: http://svn.apache.org/viewvc?rev=952205view=rev Log: Use several smaller functions for printing git diff headers instead of one. * subversion/libsvn_client/diff.c (print_git_diff_header): Split this one into ... (print_git_diff_header_added): ..this ... (print_git_diff_header_deleted): .. and this ... (print_git_diff_header_copied): .. and this ... (print_git_diff_header_moved): .. and this ... (print_git_diff_header_modified): .. and this. (diff_content_changed): Adjust caller. @@ -681,42 +705,43 @@ diff_content_changed(const char *path, + SVN_ERR(print_git_diff_header_deleted( +os, + diff_cmd_baton-header_encoding, +path, subpool)); + label1 = diff_label(apr_psprintf(subpool, a/%s, path1), rev1, subpool); label2 = diff_label(/dev/null, rev2, subpool); You could make the print_git_diff_headet_* functions return the appropriate labels as output parameters. E.g: SVN_ERR(print_git_diff_header_deleted( label1, label2, os, diff_cmd_baton-header_encoding, path, subpool)); To create labels we need two paths (haven't fully understood why we have two different paths. How can we invoke 'svn diff' to compare two different paths to each other?) and two revisions. That's a lot of parameter passing for only executing two lines. I do want to put the label creation inside a function but doing it in the print_git_diff_header_*() seems too bulky. Hopefully I can change diff_label() once we use 'git diffs' everywhere and just call it once. Thanks, Daniel
Re: svn commit: r951871 - /subversion/trunk/subversion/libsvn_client/diff.c
On Sun, Jun 06, 2010 at 02:49:16PM -, dan...@apache.org wrote: Author: dannas Date: Sun Jun 6 14:49:15 2010 New Revision: 951871 URL: http://svn.apache.org/viewvc?rev=951871view=rev Log: First small step towards using the 'git unidiff' format for 'svn diff'. The parts that writes to the output stream are ifdef'd with SVN_EXPERIMENTAL_PATCH since five diff-tests needs to be adjusted and I don't want to change the testsuite before we're 100 % certain that we want to use the git diff format as our standard format and not as an optional one. * subversion/libsvn_client/diff.c (print_git_diff_header): New. (diff_cmd_baton): Add field 'deleted'. Dunno what I was thinking here. I'm using the diff_cmd_baton as if it is a per path baton when in reality it, of course, is used for all paths involved in the diff operation. Working on a way to pass the needed information to diff_content_changed() without using the baton. (diff_content_changed): Call print_git_diff_header() and adjust the labels before asking libsvn_diff to produce the actual diff. (diff_file_deleted_with_diff): Note in the diff_cmd_baton that we have a deleted path. (svn_client_diff5 svn_client_diff_peg5): Initialize diff_cmd_baton.deleted to FALSE. Modified: subversion/trunk/subversion/libsvn_client/diff.c Modified: subversion/trunk/subversion/libsvn_client/diff.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/diff.c?rev=951871r1=951870r2=951871view=diff Daniel
Re: Ideas for handling copies/moves in 'svn diff'
On Sun, May 30, 2010 at 11:16:55AM +0200, Stefan Sperling wrote: On Sat, May 29, 2010 at 02:34:06PM +0200, Daniel Näslund wrote: Problem 1 == We're processing the delete-half instead of the add-half for a copy/move. At present, copy/move add-half is not shown by 'svn diff' unless invoked with '--show-copies-as-adds'. But with the 'git unidiff' format we would only show a diff for the add-part, e.g. we would change the behaviour from reporting the delete-half to reporting the add-half. Why do we need to change the behaviour? My main concern was that we didn't use file_added() for all the cases involving copies and moves. That was unfortunately far from obvious from my last mail. :( Right now, svn diff always shows deleted paths as deleted in the diff, unless you pass --no-diff-deleted. svn patch already knows how to deal with those files (delete them). For copies, we don't show the add unless you pass --show-copies-as-adds. If the copy has local mods, you see a diff between the copy source and the modified copy. With --show-copies-as-adds, you see every copy as added. Note that --show-copies-as-adds was added so that svn patch can see copied files (without knowing that they are copies, it treats them just like any other added file). Once svn patch understand copy/move information in the diff header this option will be needed much less. I think it's fine for now to make copy/move info in the diff header behave according to the above: - Print just a copy header but no content diff for copied without mods: Index: somefile === copy from copyfrom path copy to somefile Index: nextfile - Print a diff as well if the file was modified after copying: Index: somefile === copy from copyfrom path copy to somefile --- [...] +++ [...] @@ [...] @@ [...] (I've used [...] to denote an omission). - With --show-copies-as-adds, print copy/move info and also show the copy as a diff that only adds lines: Index: somefile === copy from copyfrom path copy to somefile --- [...] +++ [...] @@ [...] @@ + [...] + [...] + [...] Ok, we keep the current behaviour but add 'git diff' headers. I think you can handle all the above cases in the file_added() callback in the diff editor. Nah, I will need some way to determine if a file was modified after copying. I probably will introduce a copy_from field to file_changed(). Suggestion on solutions: 1)Call file_added() for all paths with db_status copied/moved instead of using file_changed(). + Allows us to easily detect what files are adds and the svn_wc_diff_callback already has a copyfrom arg. - We still have to distinguish between copied and moved paths in the callback. We could chose to only show copies in the git headers for now. And defer moves till wc-ng can handle them properly. svn patch will need to make sure to copy files before deleting the source. This will probably require a two-pass approach when patching... But again, once wc-ng can tell us whether a file was moved or copied, this will get much easier. _scan_addition() can give us the copyfrom relpath and tell us if the path was added or copied. 'svn mv' is currently implemented using a lock and doing a svn_wc_copy3() and then svn_wc_delete4(). Thus, we can detect copies/moves and we can perform them using wc-ng mechanisms. Or am I missing something? 2)Introduce file_copied() and file_moved() callbacks. Those could have a 'delete_half' arg or similar, e.g. we would pass both the add-half and del-half to that function and separate them with an parameter. + Might enhance readability. - We're introducing a concept that may be viewed as inconsistent with the rest of the code. svn handles 'add+del', is it acceptable to introduce copy and move at this level of the code? Further, the diff_file_added() and diff_file_deleted() does very little as is, wouldn't it be better to introduce some extra complexity there instead? Aren't you changing public diff-editor API by adding those functions? If so, I don't think it's a huge problem to rev the API. But if we can avoid doing so I'd prefer not to. A sidetrack: I thought that all editors must implement all svn_delta_editor_t funcs, but svn_wc_diff_callbacks4_t only does it for a subset, i.e. there's no open_root() or close_edit(). We're calling it a diff editor but is it really equal to something like the status editor? To me, it feels like a bunch of callbacks created in the same spirit as the delta_editor callbacks but not strictly following it's conventions. I agree
Ideas for handling copies/moves in 'svn diff'
Hi! The Google Summer of Code period has started. Posting about what I'm currently working on as part of implementing the 'git unidiff' format for wc-wc diffs. Perhaps someone sees an obvious solution to any of the three problems I present below. The callchain: === do_diff() svn_wc_diff_wc6() /* Calls svn_wc__db_read_children() and recurses into child dirs but * calls file_diff() on files. */ directory_elements_diff() file_diff() /* Will be called for copies and moves add-part. */ eb-file_changed() /* If called with '--show-copies-as-adds' the add-part will be * reported with this func. */ eb-file_added() /* Will eventually be called for both added, changed and * deleted paths. From here we call into the diff library to * create headers and actual diff content. */ diff_content_changed() Problem 1 == We're processing the delete-half instead of the add-half for a copy/move. At present, copy/move add-half is not shown by 'svn diff' unless invoked with '--show-copies-as-adds'. But with the 'git unidiff' format we would only show a diff for the add-part, e.g. we would change the behaviour from reporting the delete-half to reporting the add-half. Suggestion on solutions: 1)Call file_added() for all paths with db_status copied/moved instead of using file_changed(). + Allows us to easily detect what files are adds and the svn_wc_diff_callback already has a copyfrom arg. - We still have to distinguish between copied and moved paths in the callback. 2)Introduce file_copied() and file_moved() callbacks. Those could have a 'delete_half' arg or similar, e.g. we would pass both the add-half and del-half to that function and separate them with an parameter. + Might enhance readability. - We're introducing a concept that may be viewed as inconsistent with the rest of the code. svn handles 'add+del', is it acceptable to introduce copy and move at this level of the code? Further, the diff_file_added() and diff_file_deleted() does very little as is, wouldn't it be better to introduce some extra complexity there instead? 3)... Problem 2 = We need to process the add-half to know the delete-half. The add-half knows about the delete-half through the copyfrom information, but the other way around does not work - there's no 'copyto' information. Since we don't want to present a diff for the delete-half, we need to be able to know that a deleted file actually is part of a delete. Suggestion on solutions: 1) Create a custom svn_wc__db_read_children() func that can has an out parameter callled 'copyto', e.g. the func would traverse the entire wc looking if there is any other paths that has the current path as copyfrom. + We can make the detecting operation less expensive than a file traversal since we have SQL and the db file will already be in memory. (Will involve more reads before we move to one db). - It feels a bit awkvard to fetch 'copyto'. There must have been a design decision to only only make the copyfrom relation unidirected. How does other code parts detect the copyto? 2) Store added and deleted paths as keys in two hashs with the diff contents inside diff_content_changed(). + We don't have to change directory_elements_diff(). - The memory consumption would grow with the number of deleted and added paths. If we choose to save them to tmp-files, we could avoid that but would suffer in performance instead. 3) ... Problem 3 == If we do changes in the callchain presented earlier, we would affect the 'url-wc' use case too. directory_elements_diff() is called from close_directory() and close_edit(). We would have git diffs for -r BASE:WORKING but no git diff headers for the revisions preceeding BASE. I'm assuming that it's not a problem since the current 'svn diff' command shows copies for the changes in the wc but not the ones before. Cheers, Daniel
Re: svn commit: r948304 - /subversion/trunk/subversion/libsvn_client/checkout.c
On Wed, May 26, 2010 at 03:57:52AM -, hwri...@apache.org wrote: Author: hwright Date: Wed May 26 03:57:52 2010 New Revision: 948304 URL: http://svn.apache.org/viewvc?rev=948304view=rev Log: Remove a couple of goto statements in favor of some refactoring and a helper function. Suggested by: Edsger Dijkstra :)
Re: svn_client_patch not applying patch correctly
Hi Stefan Sperling Just posting to tell I'm looking into this. On Sat, May 22, 2010 at 12:58:42AM +0200, Stefan Sperling wrote: On Fri, May 21, 2010 at 10:27:42PM +0200, Stefan Küng wrote: * ShellCache.h - this file results in three applied and one rejected hunk the result as it *should* be is shown in image1, the result from svn_client_patch is shown in image2 Where are the images you are referring to? This seems to be a no newline at end of file handling problem. We should look at it in detail. Please file an issue. Yup. I simplified the failing hunk to only have a context line at the end and could still get a failure: [[[ Index: ShellCache.h === --- ShellCache.h (revision 906) +++ ShellCache.h (working copy) @@ -193,6 +193,7 @@ DWORD layoutticker; DWORD langticker; DWORD blockstatusticker; + DWORD debug; UINT drivetypecache[27]; TCHAR drivetypepathcache[MAX_PATH]; }; ]]] What does the code do to handle missing eols? == parse_next_hunk(): Since r906588, we're including lines with missing newlines at end of hunk. match_hunk(): If the target has no eol at end-of-file, we check if the next line of the hunk is the eof and adjust accordingly. The code says that we have a match. What fails? = apply_hunk(): When seeking in the target to target-current_line + hunk-original_text we get the wrong line and the hunk is marked as rejected. Cheers, Daniel
[WIP] Use repos_root_url and repos_relpath in the status code.
Hi!
There's a lot of parameteter tracking in this patch. Basically it's all
about passing down the url arguments to assemble_status().
The goal is that we should be able to remove the entry and parent_entry
fields in a follow-up and be able to use the parent_relpath when we want
to detect switches but also for determining the toplevel op_root of a
copy (we may have a copy below another copy with mixed revs).
Sending it in to see if anyone has any objections. I don't feel
comfortable committing it without someone more experienced giving it a
look.
[[[
First step in the move to using repos_root_url and repos_relpaths for
describing urls in the status code.
The idea is to reuse the parents repos_relpath when detecting if a node is
switched instead of doing an extra scan for each node. Since we're doing a
depth first traversal of the wc, the parent has already been visited.
Hopefully we will save some read calls.
We still depend on the url field but the plan is to remove it.
* subversion/include/svn_wc.h
(svn_wc_status3_t): Add new fields 'repos_root_url' and 'repos_relpath'.
* subversion/libsvn_wc/status.c
(assemble_status): Add new parameters. Go back to using the
previous way of detecting a switched path; simply comparing the
repos_relpath with the parent_repos_relpath + basename(path).
(send_status_structure): Add parameter 'parent_repos_root_url' and
'parent_repos_relpath.
(handle_dir_entry): Add parameter 'dir_repos_root_url' and
'dir_repos_relpath'.
(internal_status): Fetch the parent_repos_root_url and
parent_repos_relpath from the db. This function
is called on the anchor paths.
(get_dir_status): Add parameter 'parent_repos_root_url' and
'parent_repos_relpath.
(svn_wc_dup_status3): Duplicate 'repos_root_url' and 'repos_relpath'.
]]]
Daniel
Index: subversion/include/svn_wc.h
===
--- subversion/include/svn_wc.h (revision 944886)
+++ subversion/include/svn_wc.h (working copy)
@@ -3647,6 +3647,13 @@ typedef struct svn_wc_status3_t
/** Which changelist this item is part of, or NULL if not part of any. */
const char *changelist;
+ /** The leading part of the url, not including the wc root and subsequent
+ * paths. */
+ const char *repos_root_url;
+
+ /** The path relative to the wc root. */
+ const char *repos_relpath;
+
/* NOTE! Please update svn_wc_dup_status3() when adding new fields here. */
} svn_wc_status3_t;
Index: subversion/libsvn_wc/status.c
===
--- subversion/libsvn_wc/status.c (revision 944886)
+++ subversion/libsvn_wc/status.c (working copy)
@@ -273,6 +273,8 @@ assemble_status(svn_wc_status3_t **status,
const char *local_abspath,
const svn_wc_entry_t *entry,
const svn_wc_entry_t *parent_entry,
+const char *parent_repos_root_url,
+const char *parent_repos_relpath,
svn_node_kind_t path_kind,
svn_boolean_t path_special,
svn_boolean_t get_all,
@@ -284,6 +286,8 @@ assemble_status(svn_wc_status3_t **status,
svn_wc_status3_t *stat;
svn_wc__db_status_t db_status;
svn_wc__db_kind_t db_kind;
+ const char *repos_relpath;
+ const char *repos_root_url;
const char *url;
svn_boolean_t locked_p = FALSE;
svn_boolean_t switched_p = FALSE;
@@ -313,16 +317,20 @@ assemble_status(svn_wc_status3_t **status,
SVN_ERR(svn_wc__db_op_read_tree_conflict(tree_conflict, db, local_abspath,
scratch_pool, scratch_pool));
- SVN_ERR(svn_wc__db_read_info(db_status, db_kind, revision, NULL, NULL,
- NULL, changed_rev, changed_date,
+ SVN_ERR(svn_wc__db_read_info(db_status, db_kind, revision,
+ repos_relpath, repos_root_url, NULL,
+ changed_rev, changed_date,
changed_author, NULL, NULL, NULL, NULL,
NULL, changelist, NULL, NULL, NULL, NULL,
NULL, NULL, base_shadowed, conflicted,
lock, db, local_abspath, result_pool,
scratch_pool));
+ /* ### Temporary until we've revved svn_wc_status3_t to use
+ * ### repos_{root_url,relpath} */
SVN_ERR(svn_wc__internal_node_get_url(url, db, local_abspath,
result_pool, scratch_pool));
+
SVN_ERR(svn_wc__internal_is_file_external(file_external_p, db,
local_abspath, scratch_pool));
@@ -331,10 +339,23 @@ assemble_status(svn_wc_status3_t **status,
an URL, at the very least. */
if (! file_external_p)
{
- svn_boolean_t is_wc_root; /* Not used. */
-
- SVN_ERR(svn_wc__check_wc_root(is_wc_root, NULL, switched_p, db,
-
Re: svn commit: r943986 - /subversion/trunk/subversion/libsvn_wc/status.c
On Thu, May 13, 2010 at 10:35:36PM +0200, Bert Huijben wrote:
Log:
Don't use svn_wc_entry for fetching an url.
* subversion/libsvn_wc/status.c
(assemble_status): Fetch the url with a node func. Use
svn_wc__check_wc_root() for determining if a path is switched.
[...]
+ SVN_ERR(svn_wc__internal_node_get_url(url, db, local_abspath,
+result_pool, scratch_pool));
+
/** File externals are switched files, but they are not shown as
such. To be switched it must have both an URL and a parent with
- an URL, at the very least. If this is the root folder on the
- (virtual) disk, entry and parent_entry will be equal. */
+ an URL, at the very least. */
if (entry-file_external_path)
{
file_external_p = TRUE;
}
- else if (entry-url parent_entry parent_entry-url
- entry != parent_entry)
+ else
{
- /* An item is switched if:
- parent-url + basename(path) != entry-url */
- switched_p = (strcmp(
- svn_uri_join(parent_entry-url,
- svn_path_uri_encode(svn_dirent_basename(
-local_abspath, NULL),
- scratch_pool),
- scratch_pool), entry-url) != 0);
+ svn_boolean_t is_wc_root; /* Not used. */
+
+ SVN_ERR(svn_wc__check_wc_root(is_wc_root, NULL, switched_p,
db,
+local_abspath, scratch_pool));
You replace a few in memory operations with several database
operations on the node and its parent(s) here. Was this intended? (I
don't think this will make status faster). While walking the tree for
status you know the url of the parent and of the node itself.
This check_wc_root function starts looking in the database for the url
of the node.. and its parent (probably inherited, so looking further
up)... and then does this same check for you.
Pasting my answer to the same question from #svn-dev:
09:17 @dannas Bert: Yeah, the url-fetching and switch detection is
sub-optimal at the moment. I just set out to replace the entry calls,
thinking I could optimize for performance later but I should have
atleast written a TODO about it,
09:17 @dannas If we call assemble_status() from
send_status_structure() with my next patch for removing entry-url usage
there we may end up fetching the url for the same path three times! :)
09:18 @dannas I'll loook into you suggestion later today, it makes a
lot of sense.
09:20 @dannas The only thing I need to add is a way to detect if our
path is wc_root.
09:21 @dannas It was the problem to detect a wc-root that lead me to
use the bulky svn_wc__check_wc_root()
Daniel
WC-NG: Perhaps it's time to write a new summary of where we stand at the moment?
Hi! Greg [1] identified the following items in the beginning of February. * remove entry_t usage within libsvn_wc/client * remove access_t usage withing libsvn_wc/client * move props into wc.db * move to single/root wc.db * switch to new pristine file mgmt I have the feeling, we've made some progress. Are there any new identifiable items for wc-ng completion? The WC-NG meta issue [2] contains only one dependency, #3586. Perhaps we could add some more to make it easier to track the progress? Cheers, Daniel (Hope I'm not too forward asking these questions) [1] http://svn.haxx.se/dev/archive-2010-02/0286.shtml [2] http://subversion.tigris.org/issues/show_bug.cgi?id=3357
Re: svn commit: r941793 - /subversion/trunk/subversion/libsvn_wc/util.c
On Thu, May 06, 2010 at 04:27:27PM -, hwri...@apache.org wrote: Author: hwright Date: Thu May 6 16:27:27 2010 New Revision: 941793 URL: http://svn.apache.org/viewvc?rev=941793view=rev Log: * subversion/libsvn_wc/util.c (svn_wc__status2_from_3): Fix a pointer error, and one of the failures in the python tests. Modified: subversion/trunk/subversion/libsvn_wc/util.c Modified: subversion/trunk/subversion/libsvn_wc/util.c URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/util.c?rev=941793r1=941792r2=941793view=diff == --- subversion/trunk/subversion/libsvn_wc/util.c (original) +++ subversion/trunk/subversion/libsvn_wc/util.c Thu May 6 16:27:27 2010 @@ -562,7 +562,7 @@ svn_wc__status2_from_3(svn_wc_status2_t return SVN_NO_ERROR; } - status = apr_pcalloc(result_pool, sizeof(*status)); + *status = apr_pcalloc(result_pool, sizeof(**status)); SVN_ERR(svn_wc__get_entry(entry, wc_ctx-db, local_abspath, TRUE, svn_node_unknown, FALSE, result_pool, Thanks for fixing this! Daniel
Re: svn commit: r940651 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_wc/status.c libsvn_wc/util.c
On Mon, May 03, 2010 at 08:41:14PM -0400, Greg Stein wrote: On Mon, May 3, 2010 at 18:01, dan...@apache.org wrote: ... +++ subversion/trunk/subversion/libsvn_wc/util.c Mon May 3 22:01:53 2010 ... @@ -565,6 +567,12 @@ svn_wc__status2_from_3(svn_wc_status2_t SVN_ERR(svn_wc__get_entry(entry, wc_ctx-db, local_abspath, TRUE, svn_node_unknown, FALSE, result_pool, scratch_pool)); + + SVN_ERR(svn_wc__db_op_read_tree_conflict(tree_conflict, wc_ctx-db, + local_abspath, result_pool, + scratch_pool)); You don't need 'tree_conflict' to persist, so both pools can be scratch_pool. + old_tree_conflict = svn_wc__cd2_to_cd(tree_conflict, scratch_pool); But you're returning this one, so that should be result_pool. Committed in r940982. Thanks, Daniel
Future direction for the diff editor
Hi! First, I've been accepted as a GSoC student for the summer of 2010. I'm really excited and look forward to a summer of coding. I'm supposed to implement the git unidiff format for 'svn diff' and 'svn patch' and I'll start with the diff side. The git unidiff format can represent tree changes but unfortunately the diff code in it's current state makes it hard to detect those tree changes. What to do? 1) Just allow wc-wc diffs to create diffs with the git format. Use the available wc functions to retrieve info on tree changes. 2) Allow diffs involving the repos too by creating special ra functions for retrieving the missing information. Something like svn_ra_get_copyfrom_info(). 3) Start revamping the diff code to not use an editor but instead return text-modified and props-modified nodes as detected by the server. [1]. In the mail, Greg makes a case for not using an editor in the diff code since nothing is modified. As I've understood it, an editor is used for almost all repos communication. I see the complexity involved in using an editor and I understand that sharing the same code for merge and diff has drawbacks. But I'm not seeing how we will decrease complexity by not using an editor. We'll still have to detect all those tree changes and we'll have to create additional code for doing it. If we would just have have to check for text-modified and props-modified things would have been different. 4) Wait by the roadside for editor-v2 to be finished. It is supposed to automatically detect tree changes. Has anyone given any more thoughts to how the diff code could be improved? Unless someone has a clear idea about how to move forward, I'll go with option 1) and just allow the git format for wc-wc diffs. Cheers, Daniel [1] http://svn.haxx.se/dev/archive-2009-09/0341.shtml
[PATCH] Converter for svn_wc_status3_t to svn_wc_status2_t
Hi!
Since I can't test it, and know too little of libsvn_wc, I ask if
any of you guys think it looks ok?
[[[
Implmenent converter function for converting svn_wc_status3_t to
svn_wc_status2_t.
* subversion/include/private/svn_wc_private.h
(svn_wc__status2_from_3): Return svn_error_t * instead of
svn_wc_status2_t *. Return the new status in an out parameter
instead. I needed to be able to return errors from the entry
fetching function. Add wc_ctx and local_abspath parameters to be
used when fetching the entry.
* subversion/libsvn_wc/deprecated.c
(status_4_wrapper_baton): Add wc_ctx field to be used when calling
svn_wc__status2_from_3().
(status4_wrapper_func): Track parameter and return value changes of
svn_wc__status2_from_3().
(svn_wc_get_status_editor4): Initialize wc_ctx field of the
wrapper_baton.
(svn_wc_status2): Track parameter and return value changes of
svn_wc__status2_from_3().
* subversion/libsvn_wc/util.c
(svn_wc__status2_from_3): Initialize the svn_wc_status2_t struct with
values from svn_wc_status3_t.
* subversion/libsvn_client/deprecated.c
(status4_wrapper_baton): Add ctx field.
(status4_wrapper_func): Track parameter and return value changes of
svn_wc__status2_from_3().
(svn_client_status4): Initialize ctx field of wrapper_baton.
]]]
Index: subversion/include/private/svn_wc_private.h
===
--- subversion/include/private/svn_wc_private.h (revision 938200)
+++ subversion/include/private/svn_wc_private.h (working copy)
@@ -248,9 +248,13 @@
* Convert from svn_wc_status3_t to svn_wc_status2_t.
* Allocate the result in RESULT_POOL.
*/
-svn_wc_status2_t *
-svn_wc__status2_from_3(const svn_wc_status3_t *status,
- apr_pool_t *result_pool);
+svn_error_t *
+svn_wc__status2_from_3(svn_wc_status2_t **status,
+ const svn_wc_status3_t *old_status,
+ svn_wc_context_t *wc_ctx,
+ const char *local_abspath,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
/**
Index: subversion/libsvn_wc/deprecated.c
===
--- subversion/libsvn_wc/deprecated.c (revision 938200)
+++ subversion/libsvn_wc/deprecated.c (working copy)
@@ -2230,6 +2230,7 @@
void *old_baton;
const char *anchor_abspath;
const char *anchor_relpath;
+ svn_wc_context_t *wc_ctx;
};
/* */
@@ -2243,7 +2244,8 @@
svn_wc_status2_t *dup;
const char *path = local_abspath;
- dup = svn_wc__status2_from_3(status, scratch_pool);
+ SVN_ERR(svn_wc__status2_from_3(dup, status, swb-wc_ctx, local_abspath,
+ scratch_pool, scratch_pool));
if (swb-anchor_abspath != NULL)
{
@@ -2289,6 +2291,8 @@
SVN_ERR(svn_wc__context_create_with_db(wc_ctx, NULL /* config */,
wc_db, pool));
+ swb-wc_ctx = wc_ctx;
+
anchor_abspath = svn_wc__adm_access_abspath(anchor);
if (!svn_dirent_is_absolute(svn_wc_adm_access_path(anchor)))
@@ -2562,7 +2566,8 @@
pool));
SVN_ERR(svn_wc_status3(stat3, wc_ctx, local_abspath, pool, pool));
- *status = svn_wc__status2_from_3(stat3, pool);
+ SVN_ERR(svn_wc__status2_from_3(status, stat3, wc_ctx, local_abspath,
+ pool, pool));
return svn_error_return(svn_wc_context_destroy(wc_ctx));
}
Index: subversion/libsvn_wc/util.c
===
--- subversion/libsvn_wc/util.c (revision 938200)
+++ subversion/libsvn_wc/util.c (working copy)
@@ -544,11 +544,43 @@
return new_conflict;
}
-svn_wc_status2_t *
-svn_wc__status2_from_3(const svn_wc_status3_t *status,
- apr_pool_t *result_pool)
+svn_error_t *
+svn_wc__status2_from_3(svn_wc_status2_t **status,
+ const svn_wc_status3_t *old_status,
+ svn_wc_context_t *wc_ctx,
+ const char *local_abspath,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
{
- /* ### As of r937468, status2_t and status3_t are no longer identical. We
- * ### need to extend this function to properly do the conversion. */
- return (svn_wc_status2_t *) svn_wc_dup_status3(status, result_pool);
+ const svn_wc_entry_t *entry;
+
+ if (old_status == NULL)
+*status = NULL;
+
+ status = apr_pcalloc(result_pool, sizeof(*status));
+
+ SVN_ERR(svn_wc__get_entry(entry, wc_ctx-db, local_abspath, TRUE,
+svn_node_unknown, FALSE, result_pool,
+scratch_pool));
+ (*status)-entry = entry;
+ (*status)-text_status = old_status-text_status;
+ (*status)-prop_status = old_status-prop_status;
+ (*status)-locked = old_status-locked;
+ (*status)-copied = old_status-copied;
Re: [PATCH] Don't use entries for checking status in svn_wc_status3_t. Was: Re: [WIP] Some quirky parts of libsvn_wc/status.c:assemble_status() for retrieving revisions
On Thu, Apr 22, 2010 at 05:07:56PM -0400, Greg Stein wrote: On Thu, Apr 22, 2010 at 15:48, Daniel Näslund dan...@longitudo.com wrote: read_info() returns changed_* information for copied nodes. A copied node has a corresponding node in the repository, and the changed_* information reflects that. (thus, your assemble_status should not be wiping the values out... if the caller wants to interpret added nodes that way, then it can) Personally, I would suggest that assemble_status() does NOTHING more than returning read_info's REVISION result. That is the ideal value for the revision of the node LOCAL_ABSPATH. If that revision is NOT what you want (ie. you have some other rules like well... if it is added, then ... or if it is deleted, then ...), then the caller should perform those calculations explicitly. The status code should not pre-suppose what rules the caller may want for revision handling. It should simply report the ideal revision for that node and be done with it. WHAT? You're saying that all the scanning should take place in the client callback? No. I said don't create strange semantics. The code you had in there had the following definition for status-revision: The revision of the BASE node, unless that has been deleted, then: if nothing was added, then still the same BASE revision, but if something was added, then SVN_INVALID_REVNUM. BUT! If you delete a child of that addition, then the BASE version MAY be returned again... we say may because if a BASE doesn't correspond to the deleted child of the add, then you'll get an error instead. The definition that read_info uses is: The revision of the unmodified (BASE) node, or SVN_INVALID_REVNUM if any (structural) changes have been made to that node. (where structural means an add/copy/move/delete; we don't mean content edits like propchanges or editing the text) That's a major change from what I've done so far. That I should just return SVN_INVALID_REVNUM for cases where I've scanned for deletion? Is that a behaviour that everyone agrees on? You're saying personally so I'm assuming there hasn't been a discussion about it. No, I don't think it should do a scan_add, nor a scan_deletion. Report the add/delete, and leave it at that. repos_relpath, repos_root_url, and repos_uuid are all inherited values, so I believe a scan_base_repos makes sense: you're simply returning those values. You're not looking for further details about operations (which the caller may not be interested in anyways). (and note that scan_base_repos doesn't even have to be used cuz assemble_status could be passed the parent repos_* values; if the BASE node hasn't been switched and (therefore) returns NULL for repos_*, then its repos_* values are easily derived within wc/status.c; but repos_* values will always be NULL for modified nodes (ie. if a WORKING node exists)) Scanning additional rows is not free, so we shouldn't be looking up information from the ancestors unless/until we find that most/all of the callers need that information. And in some cases, we can even make status.c fill in the info, since it has seen the ancestors already. In the future, we may return added/copied/moved_here distinctly so that a scan won't be needed to resolve that (tho you'd still have to scan to locate the root of the operation, tho we may be able to optimize that inside wc_db). I set out to replace the entry field in svn_wc_statusX_t. I thought that ideally I could just keep the semantics from the entry but move out the fields to svn_wc_status2_t. I wanted to keep the current behaviour since the status code is involved in so _many_ tests and rewriting all of those would cause me to loose momentum. Guess, what? I've already lost that momentum. :) I guess I should do what you suggested: Make assemble_status() simpler to insure we that we can easily describe what it's returning. On the other hand, I don't see the scanning process as something expensive comparing to 'text compares'. And I question if the API we're exposing really should demand that the callers do any extra work for fetching something as fundamental as a revision. AFAIK, the only thing that is not clear is how we handle working changes below a deleted node. If we get that one settled, then why not return the _right_ revision at once? I mean, for a VCS where the notion of globally recognized identifiers for change sets is a central part, there should be little vagueness as to which one we really mean for a path! The users of our API will hardly want to use another behaviour than what the CLI exposes. Since I'm easily persuaded, below is my idea of what to do to get to the point where assemble_status() only returns the most basic info about the revision of a node. (I really should skip step (i) and rewrite it to be part (ii) at once but I've put so much effort into that patch. I just want to commit it! Hardly a good argument
Re: [PATCH] Don't use entries for checking status in svn_wc_status3_t. Was: Re: [WIP] Some quirky parts of libsvn_wc/status.c:assemble_status() for retrieving revisions
On Thu, Apr 22, 2010 at 02:45:08PM -0400, Greg Stein wrote: On Thu, Apr 22, 2010 at 10:05, Daniel Näslund dan...@longitudo.com wrote: [[[ As part of WC-NG, replace entry calls for revisions from svn_wc_status2_t related code. The entries code set the revision of newly added nodes to 0 but the db sets them to -1. Since too many tests needs to be changed and 'svn info' also uses 0, I'll change those values in a follow-up patch instead. * subversion/tests/cmdline/copy_tests.py (repos_to_wc): Change revision to 0 since the node is added. Don't check against entries since the behavior differs. * subversion/tests/cmdline/stat_tests.py (status_with_tree_conflicts): An copied node should not have a changed_rev or author. Don't the changed_rev and changed_author correspond to the repository node that was copied into the tree? Thus, when I'm looking at a diff, it is effectively relative to what I copied in. I had my doubts about this one, although I didn't express them here. Since a copied or moved node has a previous line of history it makes sense to use that history for changed_rev and changed_author. My motivation for not setting those for copied/moved nodes was that I thought that changed_rev was defined as the closest rev less than or equal to the current revision. Since changed_author is closely related to changed_rev I set that one to it's nil value too. * subversion/tests/cmdline/svntest/actions.py (run_and_verify_status): Add new parameter check_entries. We only check the entries if check_entries is set to True. No way. For the *one* item which needs a different revision... maybe okay. But by eliminating the check_entries, you're also eliminating the checks on all the *other* items in the status output. Instead, I recommend changing svntest/wc.StateItem. Add an entry_rev attribute defaulting to None. Where we expect the entry's revision to differ from that reported in a typical status output, then we can set this extra attribute and specifically track the change in behavior. Ok, I of course prefer this suggestion since it offers the same test coverage but allows me to change the behaviour. But I would have _never_ come up with it on my own. And that's what we're talking about here: a change in behavior from 1.6 to 1.7. The entries testing is in there to ensure that we don't (arbitrarily) change the revision information that we display. I understand and totally agree on that we should be able to easily see in what way our new code differs from the entries-based. * subversion/tests/cmdline/merge_tests.py (merge_into_missing): Add revision to status output for missing dir. Previously, missing dirs did not have a revision, only files had. Don't check entries since the behavior differs. * subversion/svn/status.c (print_status): Replace checks for revisions using entries with the direct fields in svn_wc_status3_t. Set the revision for added and replaced nodes to 0. WC-NG sets those revisions to -1, but changing all the involved tests is for a follow-up. * subversion/include/svn_wc.h (svn_wc_status2_t): Add fields revision, changed_rev and changed_author. If you have changed_rev and changed_author, then please include changed_date. It is weird/inconsistent to have two of the three. Ok, the reason it was left out was due to me looking in svn/status.c to determine what fields were used. I didn't see it so it was not included. I took the idea of the status code just returning the most cruciual parts too literally. What I should have done was look at what performance impact there was to including different fields. If I include the others, there won't be much overhead (virtually zero) to including the changed_date as well. Will do. Also: your log message says svn_wc_status2_t, but you changed 3. Oups. I thought I changed it but, well, no I didn't. :( And now that you've changed status3, you should alter the svn_wc__status2_from_3() function. Since the old code uses the fields from the entry and the entry is still there, nothing needs to be changed in status2_from_3(), *yet*. And you don't need @since tags in this part of the change, since the whole structure is new. * subversion/libsvn_wc/status.c (assemble_status): Fill in the new fields with data from the wc db. (svn_wc_dup_status3): Copy the changed_author field. ]]] Quirks Checking entries --- I need to tweak the status output when doing a compare against the entries in run_and_verify_status(). The code changes the behaviour for revisions in three ways: i) Missing dirs, now have a revision This seems fine. ii) Copies from foreign repos to wc has the rev set to -1 Hmm? foreign repos only makes sense for merges, I believe. I think you mean copy from (our) repository to wc. Nope, check out copy_tests.py:978. The original status code would show
Re: [RFC] Proposal for GSoC project - extend 'svn {patch,diff}' with git unidiff format
On Mon, Apr 05, 2010 at 01:41:03PM +0200, Stefan Sperling wrote: On Mon, Apr 05, 2010 at 10:02:56AM +0200, Daniel Näslund wrote: Git unidiff format extension to 'svn patch' and 'svn diff' === [..] 2) Add the ability to track renames and copies in libsvn_diff. Probably by using some wc funcs for getting the status. My first assumption was that the svn_wc_diff_callbacks4_t vtable would be revved to allow for copied and moved scenarios once we have editor-v2. But Neels was talking about some bigger rewrite where the diff editor would be dropped. Anyway, as goes for the 'git unidiff format' work, I need some way to detect copies and moves. When I have detection, add the git headers for copies and renames and write tests to confirm the right behavior. It's a pity that diff is implemented with an editor because it makes your task much harder than necessary (you don't have all information you need available while the diff is produced piece-by-piece). Can you think of a way to do this in a simpler fashion? I think this could be done simpler by checking all files which appear in the diff for 'copyfrom' info. The question of how to get that information is not easily answered, given that we allow wc-wc, repos-wc and repos-repos diffs. neels said on IRC: [[[ If you're in a wc-wc diff (i.e. base - working), you could get each node's wc-ng status. easy., but in a repos-repos diff or repos-wc diff, you can't find that stuff unless you a) actively query the repos for more info or b) use editor v2 to tell that stuff right away. a) sucks because it duplicates server code on the client side. ]]] We could decide to only allow git diffs for wc-wc diffs. A decision that will be hard to motivate for users. But maybe we can get it working for wc-wc and by the time we're finished, editor-v2 will be too. Though The 'Vision and Roadmap' mail says that editor-v2 will not be scheduled for release until 1.9. Too bad! Let's assume your diff contains files A, B and C: If you find a single relation A-B, if A was deleted: A was moved to B if A was not deleted: A was copied to B If you find multiple relations A-B, A-C: A was copied to B and to C maybe A was also deleted But maybe because of the editorness the receiver (i.e. diff text producer) doesn't know about A when dealing with B and vice-versa, because it only gets to deal with each of them in isolated function calls? Bah. That is really ugly. Maybe you can fix this by tweaking the way the diff editor is driven? Note though that the same editor is used for merge! Or maybe you should rewrite diff this year and then do the git-diff-extensions project next year? :) I like the proposal. The only thing that worries me is that we may need to tackle the diff-is-implemented-with-the-merge-editor hack. Note that this hack is already biting us elsewhere: http://subversion.tigris.org/issues/show_bug.cgi?id=2333#desc12 Maybe we should just bite the bullet and try to re-implement (or fix) diff together before or during gsoc? It certainly would not hurt to have this problem solved before adding the git extensions. Cheers, Daniel
Re: svn commit: r935095 - in /subversion/trunk/subversion: include/private/svn_wc_private.h libsvn_client/diff.c libsvn_client/prop_commands.c libsvn_client/revisions.c libsvn_wc/node.c
On Thu, Apr 22, 2010 at 01:28:11AM +0200, Neels J Hofmeyr wrote:
Daniel Näslund wrote:
On Fri, Apr 16, 2010 at 11:09:06PM -, ne...@apache.org wrote:
==
--- subversion/trunk/subversion/libsvn_wc/node.c (original)
+++ subversion/trunk/subversion/libsvn_wc/node.c Fri Apr 16 23:09:06 2010
@@ -626,6 +626,75 @@ svn_wc__node_get_base_rev(svn_revnum_t *
[...]
+ else if (status == svn_wc__db_status_deleted)
+{
+ /* This node is deleted, but we didn't get a revnum above. So this
must
+ be a delete inside a locally copied tree. Return the revision
number
+ of the parent of the delete, presumably the copy-from revision.
+ ### This is legacy behaviour, and it sucks. */
I thought that we would only get a rev from read_info() when we have a
delete if the op_root was the path itself, e.g. that we would need to
scan upwards if we had deleted A/D and was checking the rev of A/D/G/pi.
My code was wrong. I fixed that in above-mentioned revision. Thanks!
Not sure if read_info() even returns the revision for the op_root. I think
it did otherwise during my recent testing.
Yeah, read_info() gives us SVN_INVALID_REVNUM if there is a WORKING node. For a
plain delete op_root (e.g. base_del_abspath) we have a WORKING node and should
use base_get_info(). For some reason I had a strange idea of the op_root
not beeing a WORKING node. That was _wrong_.
Daniel
Re: svn commit: r935095 - in /subversion/trunk/subversion: include/private/svn_wc_private.h libsvn_client/diff.c libsvn_client/prop_commands.c libsvn_client/revisions.c libsvn_wc/node.c
On Fri, Apr 16, 2010 at 11:09:06PM -, ne...@apache.org wrote:
[...]
==
--- subversion/trunk/subversion/libsvn_wc/node.c (original)
+++ subversion/trunk/subversion/libsvn_wc/node.c Fri Apr 16 23:09:06 2010
@@ -626,6 +626,75 @@ svn_wc__node_get_base_rev(svn_revnum_t *
}
svn_error_t *
+svn_wc__node_get_commit_base_rev(svn_revnum_t *commit_base_revision,
+ svn_wc_context_t *wc_ctx,
+ const char *local_abspath,
+ apr_pool_t *scratch_pool)
+{
+ svn_wc__db_status_t status;
+
+ SVN_ERR(svn_wc__db_read_info(status, NULL,
+ commit_base_revision,
+ NULL, NULL, NULL, NULL, NULL, NULL, NULL,
NULL,
+ NULL, NULL, NULL, NULL, NULL, NULL, NULL,
NULL,
+ NULL, NULL, NULL, NULL, NULL,
+ wc_ctx-db, local_abspath, scratch_pool,
+ scratch_pool));
What about if a path has been replaced? svn_wc__node_get_commit_base_rev
is used when diffing wc to wc. Consider this:
[[[
$ svn rm A/D/G/pi
$ svn di
Index: A/D/G/pi
===
--- A/D/G/pi (revision 1)
+++ A/D/G/pi (working copy)
@@ -1 +0,0 @@
-This is the file 'pi'.
]]]
[[[
$ svn rm A/D/G/pi
$ touch A/D/G/pi
$ svn add A/D/G/pi
Index: A/D/G/pi
===
--- A/D/G/pi (working copy)
+++ A/D/G/pi (working copy)
@@ -1 +0,0 @@
-This is the file 'pi'.
]]]
When we have a replaced node we get an incorrect base revision!
+
+ /* If this returned a valid revnum, there is no WORKING node. The node is
+ cleanly checked out, no modifications, copies or replaces. */
+ if (SVN_IS_VALID_REVNUM(*commit_base_revision))
+return SVN_NO_ERROR;
+
+ if (status == svn_wc__db_status_added)
+{
+ /* If the node was copied/moved-here, return the copy/move source
+ revision (not this node's base revision). If it's just added,
+ return SVN_INVALID_REVNUM. */
+ SVN_ERR(svn_wc__db_scan_addition(NULL, NULL, NULL, NULL, NULL, NULL,
+ NULL, NULL, commit_base_revision,
+ wc_ctx-db, local_abspath,
+ scratch_pool, scratch_pool));
+}
I assume you're doing this to not change the behaviour of the callers.
But that we in the future, always should set the revision to -1 for
add, copy-here and move-here.
+ else if (status == svn_wc__db_status_deleted)
+{
+ /* This node is deleted, but we didn't get a revnum above. So this must
+ be a delete inside a locally copied tree. Return the revision number
+ of the parent of the delete, presumably the copy-from revision.
+ ### This is legacy behaviour, and it sucks. */
I thought that we would only get a rev from read_info() when we have a
delete if the op_root was the path itself, e.g. that we would need to
scan upwards if we had deleted A/D and was checking the rev of A/D/G/pi.
+ const char *work_del_abspath;
+ const char *parent_abspath;
+ svn_wc__db_status_t parent_status;
+
+ SVN_ERR(svn_wc__db_scan_deletion(NULL, NULL, NULL,
+ work_del_abspath,
+ wc_ctx-db, local_abspath,
+ scratch_pool, scratch_pool));
+ SVN_ERR_ASSERT(work_del_abspath != NULL);
+ parent_abspath = svn_dirent_dirname(work_del_abspath, scratch_pool);
+
+ SVN_ERR(svn_wc__db_read_info(parent_status,
+ NULL, NULL, NULL, NULL, NULL, NULL, NULL,
+ NULL, NULL, NULL, NULL, NULL, NULL, NULL,
+ NULL, NULL, NULL, NULL, NULL, NULL, NULL,
+ NULL, NULL,
+ wc_ctx-db, parent_abspath,
+ scratch_pool, scratch_pool));
+
+ SVN_ERR_ASSERT(parent_status == svn_wc__db_status_added
+ || parent_status == svn_wc__db_status_obstructed_add);
+
+ SVN_ERR(svn_wc__db_scan_addition(NULL, NULL, NULL, NULL, NULL, NULL,
+ NULL, NULL,
+ commit_base_revision,
+ wc_ctx-db, parent_abspath,
+ scratch_pool, scratch_pool));
+}
+
+ return SVN_NO_ERROR;
+}
In a couple of places we're going to have similar behaviour as this one.
Consider libsvn_wc/adm_crawler.c:find_base_rev() and my WIP for
libsvn_wc/status.c:assemble_status(). Maybe some parts could be put
together to avoid code
#3610, How make 'svn patch' able to use the targets lines for intermediate context?
Hi! #3610 [1] is about ignoring white space changes to be able to apply patches that have been mangled, i.e. a mailing software converting tabs to spaces, removing trailing white spaces or leading white spaces. It means that if a certain option is given, we will match and apply hunks if the only thing differing is white spaces. The problem - The '+' lines will be applied with the white spaces changes in the patch. That's the intended behaviour. But it would be preferable if the context lines would be read from the target. For leading and trailing context, I can do that. But in the case when there are context lines in the middle of a hunk I can't since svn_hunk_t uses a stream called modified_text that makes no distinction between '+' lines and context lines, e.g. the leading ' ' or '+' are removed. The options 1) Just say that the whole hunk will be applied and we make no guarantees on what the white spaces will be. 2) Change svn_hunk_t to make it possible to differ between context and '+' lines. I really hope I don't have to do that since using modified_text and original_text is a clean solution and all the code in libsvn_client/patch.c depends on the current representation. 3) Introduce some sort of list of offsets into the hunk for where intermediate context starts and ends. Yeah, that sucks! Any suggestions? I'd go with option 1, since the other two are complicated and ugly to implement. cheers, Daniel [1] http://subversion.tigris.org/issues/show_bug.cgi?id=3610
Re: #3610, How make 'svn patch' able to use the targets lines for intermediate context?
On Wed, Apr 14, 2010 at 05:35:36PM +0200, Stefan Sperling wrote:
On Wed, Apr 14, 2010 at 04:21:56PM +0200, Daniel Näslund wrote:
Hi!
#3610 [1] is about ignoring white space changes to be able to apply
patches that have been mangled, i.e. a mailing software converting tabs
to spaces, removing trailing white spaces or leading white spaces.
It means that if a certain option is given, we will match and apply
hunks if the only thing differing is white spaces.
I'd say it means that if a certain option is given, we will match
and apply hunks even if amount of whitespace doesn't match (everything
else must match though).
I.e. we'll add some optional magic in match_hunk(), making it skip over
whitespace on either side (as determined by isspace()), but comparing
any other characters.
Note that a whitespace may be tabs beeing translated to spaces, e.g.
it's not enough to just look at the trailing or leading whitespaces.
I've used this approach (comparing the the lines with all whitespaces
removed):
[[[
if (ignore_whitespaces)
{
char *stripped_hunk_line = apr_pstrdup(pool,
hunk_line_translated);
char *stripped_target_line = apr_pstrdup(pool, target_line);
apr_collapse_spaces(stripped_hunk_line,
hunk_line_translated);
apr_collapse_spaces(stripped_target_line, target_line);
lines_matched = ! strcmp(stripped_hunk_line,
stripped_target_line);
}
]]]
The problem
-
The '+' lines will be applied with the white spaces changes
in the patch. That's the intended behaviour. But it would be preferable if
the context lines would be read from the target. For leading and
trailing context, I can do that. But in the case when there are context
lines in the middle of a hunk I can't since svn_hunk_t uses a stream
called modified_text that makes no distinction between '+' lines and context
lines, e.g. the leading ' ' or '+' are removed.
The options
1) Just say that the whole hunk will be applied and we make no
guarantees on what the white spaces will be.
Yes. Just use whatever comes from the patch, including context lines.
This is consistent with the current behaviour. I think we should avoid
special cases where this rule is currently not true anymore.
I'll do that.
(I'm not sure how UNIX patch behaves wrt context lines, actually.
Might be worth checking.)
If people don't like the result of patching, they can edit the patch
to their liking and re-apply it (or ask the submitter to resubmit
the patch -- mangled patches are usually the submitter's fault).
my thoughts exactly.
Thanks,
Daniel
Re: #3610, How make 'svn patch' able to use the targets lines for intermediate context?
On Wed, Apr 14, 2010 at 08:18:06PM +0200, Alan Barrett wrote:
On Wed, 14 Apr 2010, Stefan Sperling wrote:
Yes. Just use whatever comes from the patch, including context lines.
This is consistent with the current behaviour. I think we should avoid
special cases where this rule is currently not true anymore.
(I'm not sure how UNIX patch behaves wrt context lines, actually.
Might be worth checking.)
If applying a patch ever causes changes to lines that are marked in the
patch as context lines (not as lines to be removed or added), then I
expect people to be unhappy. The context lines in the patch file are
known or suspected to be damaged (or else the user would not have asked
to ignore whitespace changes), so copying the context lines from the
patch is not wanted.
The GNU patch implementation that I just tried does the right thing
here: when passed the --ignore-whitespace option, it ignores
whitespace changes in context lines and lines marked as being removed,
and it copies context lines from the input file, not from the patch.
Darn GNU patch! :) I guess there's a reason for the maze of if
statements that makes up it's source code! It does more than one
expects!
I don't buy the argument that it's necessary, for consistency with
current svn behaviour, to copy context lines from the patch to
the output file. Current behaviour does not include any kind of
ignore whitespace option, so the context lines in the input file
are guaranteed not to differ from the context lines in the patch,
so an outside observer (without knowledge of the internals of the
implementation) cannot tell whether the current implementation copies
context lines from the patch or from the input file. Changing the
implementation to copy context lines from the input file would therefore
not be an incompatible change.
Nope, it wouldn't be an incompatible change. It's just that it would
involve rewriting how we construct the streams from the patch and how we
apply the hunks. The current behaviour is (writing it down to remind
myself):
When parsing
-
Lines beginning with ' ' and '-' are read to original_stream
Lines beinning with ' ' and '+' are read to modified_stream
When applying
---
for each hunk in patch
determine the line the hunk should be applied at by matching
original_stream to target
for each hunk in patch
copy lines from target until we reach match-point
copy lines from modified_stream
If we would implement --ignore-whitespaces to have the behaviour you
suggested we would have to rewrite the parts dealing with
modified_stream to make us able to distinguish between context lines and
added lines. At the moment, we filter out the leading characters (' ',
'+', '-') before writing to the streams.
In the end it boils down to: From my standpoint as a newbie
programmer I'm very dense when it comes to any major changes. I do agree
that the behaviour you describe is the preferred but I'm a bit annoyed
about having to do all those changes for something that is a very small
part of the typical use case set. In my world, I would probably just ask
the submitter to retransmit the patch using some other mail software.
I hope I'm not too grumpy. I just don't want to rush into some bigger
change without weighing the options.
cheers,
Daniel
[PATCH] Follow-up to r932571 - missed one caller was: Re: svn commit: r932571 - in /subversion/trunk/subversion: include/ include/private/ libsvn_client/ libsvn_wc/ svn/
On Mon, Apr 12, 2010 at 08:58:49AM -0400, Greg Stein wrote:
On Fri, Apr 9, 2010 at 16:16, Greg Stein gst...@gmail.com wrote:
On Fri, Apr 9, 2010 at 16:07, dan...@apache.org wrote:
...
...
+++ subversion/trunk/subversion/libsvn_wc/deprecated.c Fri Apr 9 20:07:51
2010
...
@@ -2515,13 +2551,15 @@ svn_wc_status2(svn_wc_status2_t **status
{
const char *local_abspath;
svn_wc_context_t *wc_ctx;
+ svn_wc_status3_t *stat3;
SVN_ERR(svn_dirent_get_absolute(local_abspath, path, pool));
SVN_ERR(svn_wc__context_create_with_db(wc_ctx, NULL /* config */,
svn_wc__adm_get_db(adm_access),
pool));
- SVN_ERR(svn_wc_status3(status, wc_ctx, local_abspath, pool, pool));
+ SVN_ERR(svn_wc_status3(stat3, wc_ctx, local_abspath, pool, pool));
+ *status = (svn_wc_status2_t *) stat3;
Another funky cast. Make sure this gets handled with your new
converter function.
Told ya that you'd miss this if you didn't leave yourself a reminder marker...
I have no excuses. I didn't look enough. Now, I have grepped
libsvn_{wc,client} for other uses of svn_wc_status2 and every else looks
in order.
[[[
Follow-up to r923571. Update one caller I missed in the previous
commit.
* subversion/libsvn_wc/deprecated.c
(svn_wc_status2): Use svn_wc__status2_from_3().
Suggested by: gstein
]]]
Index: subversion/libsvn_wc/deprecated.c
===
--- subversion/libsvn_wc/deprecated.c (revision 933374)
+++ subversion/libsvn_wc/deprecated.c (working copy)
@@ -2556,7 +2556,7 @@ svn_wc_status2(svn_wc_status2_t **status,
pool));
SVN_ERR(svn_wc_status3(stat3, wc_ctx, local_abspath, pool, pool));
- *status = (svn_wc_status2_t *) stat3;
+ *status = svn_wc__status2_from_3(stat3, pool);
return svn_error_return(svn_wc_context_destroy(wc_ctx));
}
[PATCH] rev svn_wc_status2_t to svn_wc_status3_t
Hi!
This is a _big_ patch but it does very little. I've just replaced every
occurence of svn_wc_status2_t with the new svn_wc_status3_t. It's
needed, in the cause of removing the entry field which I intend to do in
a follow-up. There's a WIP on the list, that handles most of the chores of the
status-entry.
Hopefully someone will not be put of by this gigantic blob. If we hadn't
already revved svn_wc_status3() for 1.7 I could have done the compat
wrappers and declarations in one patch and update the callers in a
follow-up. But since it was already revved, here you go! :)
make check passed.
[[[
As part of replacing the entries field in svn_wc_status2_t for WC-NG, we
rev it to svn_wc_status3_t. No semantic changes, except for adjusting some
compat wrappers, else just revving.
As for now, the compat wrappers just casts to the previous type. Some more
magic will be needed when we're ready to throw away that pescious entry field.
* subversion/include/svn_wc.h
(svn_wc_status3_t): New. An exact copy svn_wc_status2_t at the moment.
(svn_wc_status2_t): Mark as deprecated.
(svn_wc_dup_status3): New.
(svn_wc_dup_status2): Mark as deprecated.
(svn_wc_status3,
svn_wc_status_func4_t): Replace svn_wc_status2_t parameter.
* subversion/include/private/svn_wc_private.h
(svn_wc__is_sendable_status): Replace svn_wc_status2_t.
* subversion/libsvn_wc/status.c
(edit_baton): Replace svn_wc_status2_t.
(dir_baton): Update comment referring to status2_t.
(internal_status,
assemble_status,
send_status_structure,
send_unversioned_item,
handle_externals,
hash_stash,
tweak_statushash,
find_dir_url,
make_dir_baton,
svn_wc__is_sendable_status,
mark_deleted,
handle_statii,
close_directory): Replace svn_wc_status2_t.
(svn_wc_dup_status3): New.
(svn_wc_dup_status2): Moved from here ..
* subversion/libsvn_wc/deprecated.c
(svn_wc_dup_status2): .. to here.
(status4_wrapper_func): Replace svn_wc_status2_t parameter and use
svn_wc_dup_status3().
(svn_wc_status2): Use svn_wc_status3() for fetching the status struct.
* subversion/libsvn_client/deprecated.c
(status4_wrapper_func): Use the new status3_t type.
* subversion/libsvn_client/blame.c,
subversion/libsvn_client/cat.c,
subversion/libsvn_client/commit.c,
subversion/libsvn_client/delete.c,
subversion/libsvn_client/export.c,
subversion/libsvn_client/merge.c,
subversion/libsvn_client/patch.c,
subversion/libsvn_client/status.c:
subversion/svn/status-cmd.c
subversion/svn/status.c
subversion/svn/cl.h
Use the new svn_wc_status3_t and in some cases svn_wc_dup_status3().
]]]
cheers,
Daniel
Index: subversion/svn/cl.h
===
--- subversion/svn/cl.h (revision 932402)
+++ subversion/svn/cl.h (working copy)
@@ -388,7 +388,7 @@
*/
svn_error_t *
svn_cl__print_status(const char *path,
- const svn_wc_status2_t *status,
+ const svn_wc_status3_t *status,
svn_boolean_t detailed,
svn_boolean_t show_last_committed,
svn_boolean_t skip_unrecognized,
@@ -403,7 +403,7 @@
allocations. */
svn_error_t *
svn_cl__print_status_xml(const char *path,
- const svn_wc_status2_t *status,
+ const svn_wc_status3_t *status,
apr_pool_t *pool);
Index: subversion/svn/status.c
===
--- subversion/svn/status.c (revision 932402)
+++ subversion/svn/status.c (working copy)
@@ -62,7 +62,7 @@
/* Return the single character representation of the switched column
status. */
static char
-generate_switch_column_code(const svn_wc_status2_t *status)
+generate_switch_column_code(const svn_wc_status3_t *status)
{
if (status-switched)
return 'S';
@@ -104,7 +104,7 @@
svn_boolean_t detailed,
svn_boolean_t show_last_committed,
svn_boolean_t repos_locks,
- const svn_wc_status2_t *status,
+ const svn_wc_status3_t *status,
unsigned int *text_conflicts,
unsigned int *prop_conflicts,
unsigned int *tree_conflicts,
@@ -249,7 +249,7 @@
svn_error_t *
svn_cl__print_status_xml(const char *path,
- const svn_wc_status2_t *status,
+ const svn_wc_status3_t *status,
apr_pool_t *pool)
{
svn_stringbuf_t *sb = svn_stringbuf_create(, pool);
@@ -369,7 +369,7 @@
/* Called by status-cmd.c */
svn_error_t *
svn_cl__print_status(const char *path,
- const svn_wc_status2_t *status,
+ const svn_wc_status3_t *status,
svn_boolean_t detailed,
svn_boolean_t show_last_committed,
svn_boolean_t skip_unrecognized,
Index: subversion/svn/status-cmd.c
[RFC] Proposal for GSoC project - extend 'svn {patch,diff}' with git unidiff format
Hi! I'm supposed to send this proposal to the Google Summer of Code machinery and let it be forwarded to the interrested mentor of the Subversion community, in this case Stefan. In the interrest of openess I'm posting it here before sending it off to Google later today. Maybe someone has something they'd like to add? === Git unidiff format extension to 'svn patch' and 'svn diff' === Contents == Suggested workflow What is the git unidiff format? Parsing the git headers Applying tree changes Applying mode changes Applying binary patches Applying property changes Suggested workflow -- Here's my project proposal for GSoC 2010. The purpose of the project is pretty self-explanatory; make 'svn patch' and 'svn diff' able to deal with git unidiff extensions. I've tried to point out some of the API changes that are neccessary to show that I have an understanding of what to do. If I'd get accepted I would do things in this order: 1) Rev funcs to allow a use_git_format flag to be passed down to libsvn_diff and create git diff format patches for adds and deletes. Write a copule of tests to verify that we get the intended format. 2) Add the ability to track renames and copies in libsvn_diff. Probably by using some wc funcs for getting the status. My first assumption was that the svn_wc_diff_callbacks4_t vtable would be revved to allow for copied and moved scenarios once we have editor-v2. But Neels was talking about some bigger rewrite where the diff editor would be dropped. Anyway, as goes for the 'git unidiff format' work, I need some way to detect copies and moves. When I have detection, add the git headers for copies and renames and write tests to confirm the right behavior. 3) Determine how the base85 format works and write C-tests to confirm the behavior. Git does it like this: [4] 4) Pass down a flag for allowing or disallowing binary diffs to libsvn_diff. Detect binary files and write the patches. Write tests to confirm the behavior. 5) Allow 'svn patch' to apply git diff formats for adds and deletes. Write tests to confirm the behavior. 6) Allow 'svn patch' to apply git diff formats for moves and copies. 7) Allow 'svn patch' to apply git diff formats for binary patches. I propably need to do some thinking about what state the wc can be in as for obstructed, missing, replaced, unversioned, ignored nodes and so on. 8) Make libsvn_diff able to record modes. Probably we're only interrested in the executable bit and that one can we get from svn:executable. Write tests to confirm the behavior. 9) Allow 'svn patch' to apply mode changes (if we agree that we want that behavior): 10) Decide on a header for dealing with props? Do we need to stay compatible with git and diff? Probably, so we need a header that will be ignored by applications not interrested in svn:properties. 11) Decide on the header format for properties. Implement it in the diff code and write tests for it. 12) Extend the diff parser to deal with property diffs. Write tests. 13) Done. What is the git unidiff format? The format is thoroughly described in [1] so I'll just recapitulate the use cases for it: 1) Track copies and renames 2) File mode changes 3) Binary patches Creating the git headers - A couple of funcs needs to be revved to pass down the neccessary parameters telling libsvn_diff to create a git diff. And we need a way to detect copies and renames. subversion/libsvn_client/diff.c (svn_client_diff5): We need a parameter to tell the diff machinery we want a git diff. (svn_wc_diff_callbacks4_t): We have callbacks for changed, added and deleted nodes but none for copied or moved nodes. Since we don't have editor-v2 we can't get that info from the server so git diffs should only be possible for wc-wc diffs at the moment. At the moment I'll probably check the status of the path that we get in file_added() and record copied-from or moved-from. subversion/libsvn_diff/diff_file.c (svn_diff_file_diff2): Parsing the git headers We have examples of how the parsing should be done from the mercurial source code [2]. (This link was found in the notes document referred above. A big thank you to Augie Fackler for taking the time to write down all the information). subversion/libsvn_diff/parse-diff.c (parse_git_hunk_header): Create this func to be invoked before parse_hunk_header(). Captures oldname, newname, operation and mode. Applying tree changes --- We already have many different scenarios to handle with nodes beeing obstructed, missing, ignored, unversioned and so on. If we'll track tree changes the number of scenarios will increase. I probably should make some kind of graph to map out
Re: [PATCH v2] 'svn patch' should remove empty dir
Committed in r930662 on my branch svn-patch-improvements.
On Wed, Mar 03, 2010 at 04:25:07PM +0100, Daniel Näslund wrote:
Hi!
Here's my revised patch. I've commented on the changes inline.
One strange thing: If you compile this you'll get a compiler warning
about this variable not beeing used:
1354 apr_array_header_t *deleted_targets;
But if I remove it, no targets will be condensed. It's really silly. Can
someone explain why a variable that isn't used must be declared in order
for my code to work. What is the simple logical explanation?
On Mon, Mar 01, 2010 at 04:10:09PM +0100, Stefan Sperling wrote:
On Sat, Feb 27, 2010 at 09:09:52PM +0100, Daniel Näslund wrote:
Index: subversion/libsvn_client/patch.c
===
--- subversion/libsvn_client/patch.c (revision 916918)
+++ subversion/libsvn_client/patch.c (arbetskopia)
[...]
+/* Check if PARENT_DIR_ABSPATH has any versioned or unversioned children
if
+ * we ignore the ones in TARGETS_TO_BE_DELETED. Return the answer in
+ * EMPTY. */
+static svn_error_t *
+is_dir_empty(svn_boolean_t *empty, const char *parent_dir_abspath,
+ svn_client_ctx_t *ctx,
+ apr_array_header_t *targets_to_be_deleted,
+ apr_pool_t *result_pool, apr_pool_t *scratch_pool)
+{
+ struct status_baton btn;
+ svn_opt_revision_t revision;
+ int i;
+
+ btn.existing_targets = apr_array_make(scratch_pool, 0,
+sizeof(patch_target_t *));
+ btn.parent_path = parent_dir_abspath;
+ btn.result_pool = scratch_pool;
+ revision.kind = svn_opt_revision_unspecified;
+
+ SVN_ERR(svn_client_status5(NULL, parent_dir_abspath, revision,
+ find_existing_children, btn,
+ svn_depth_immediates, TRUE, FALSE, TRUE,
+ FALSE, NULL, ctx, scratch_pool));
Not sure if svn_client_status5() is really what we want here.
Is there a public or semi-private libsvn_wc function we could
use instead? One that doesn't need a client context?
I used svn_wc_walk_status() instead. It does not handle externals but
treats them as unversioned. No problemos for us, we just want to find
out if anything exists in a dir.
+ /* Do we delete all targets? */
+ for (i = 0; i btn.existing_targets-nelts; i++)
+{
+ int j;
+ const char *found = APR_ARRAY_IDX(btn.existing_targets, i, const
char *);
+ svn_boolean_t deleted = FALSE;
+
+ for (j = 0; j targets_to_be_deleted-nelts; j++)
+{
+ patch_target_t *to_be_del;
+ to_be_del = APR_ARRAY_IDX(targets_to_be_deleted, j,
patch_target_t *);
+ if (! strcmp(found, to_be_del-abs_path))
+deleted = TRUE;
Are you missing a 'break' here?
Added it. Saves us some iterations.
+}
+ if (! deleted)
+{
+ *empty = FALSE;
+ break;
+}
+}
+
+ return SVN_NO_ERROR;
+}
+
+/* Add TARGET to the array of targets keyed by their parent dir in
+ * TARGETS_TO_BE_DELETED. If there is no array for the parent dir a new
one
+ * is created. All allocations are done in RESULT_POOL. */
+static svn_error_t *
+add_target_to_hash_keyed_by_parent_dir(apr_hash_t
*targets_to_be_deleted,
+ patch_target_t *target,
+ apr_pool_t *result_pool)
+{
+ apr_array_header_t * deleted_targets_in_dir;
+
+ /* We're using the abs_path of the target. The abs_path is not
+ * present if the path is a symlink pointing outside the wc but we
+ * know, that's not the case. */
The comma should either be removed, or moved: ... wc, but we know
Fixed.
+ const char *dirname = svn_dirent_dirname(target-abs_path,
+ result_pool);
+
+ deleted_targets_in_dir = apr_hash_get(targets_to_be_deleted,
+dirname, APR_HASH_KEY_STRING);
+
+ if (deleted_targets_in_dir)
+{
+ APR_ARRAY_PUSH(deleted_targets_in_dir, patch_target_t *) = target;
+
+ apr_hash_set(targets_to_be_deleted, dirname,
+ APR_HASH_KEY_STRING, deleted_targets_in_dir);
+}
+ else
+{
+ apr_array_header_t *new_array;
+
+ new_array = apr_array_make(result_pool, 0, sizeof(patch_target_t
*));
+ APR_ARRAY_PUSH(new_array, patch_target_t *) = target;
+ apr_hash_set(targets_to_be_deleted,
+ dirname, APR_HASH_KEY_STRING, new_array);
+}
+ return SVN_NO_ERROR;
+}
+
+/* Compare A and B and return an integer greater than, equal to, or less
+ * than 0, according to whether
[PATCH] v2. Follow-up to r922176
Hi Julian!
On Wed, Mar 31, 2010 at 10:19:10AM +0100, Julian Foad wrote:
On Tue, 2010-03-30, Daniel Näslund wrote:
Ping! This patch has not been revieved
[...]
[[[
Follow-up to r922176. Fix that tree changes were not considered when
determining if the wc has modifications.
* subversion/libsvn_wc/revision_status.c
(analyze_status): Determine from status if a path has been added or
deleted. Do some optimizations to avoid having to do a text
comparison for determining if a wc has modifications.
Suggested by: gstein
rhuijben
Approved by: ?
]]]
Index: subversion/libsvn_wc/revision_status.c
===
--- subversion/libsvn_wc/revision_status.c(revision 922398)
+++ subversion/libsvn_wc/revision_status.c(arbetskopia)
@@ -41,7 +41,8 @@
};
/* An svn_wc__node_found_func_t callback function for analyzing the
status
- * of nodes */
+ * of nodes. Optimized to avoid text compares and unneccessary checks of
+ * already set values. */
Be more specific: Which nodes does it analyze? How does it return the
result? What kinds of status can it report? (A reference to somewhere
else is fine, if the details are already explained somewhere else.)
Then the Optimised ... sentence should make more sense.
I've clarified exactly how the function is optimized and what parameters
it takes. A doc comment should say what a func does, rather than how it
does it. In that sence, my comment is a bit off. Still, someone reading
a static func is surely going to look at the implementation as well.
static svn_error_t *
analyze_status(const char *local_abspath,
void *baton,
@@ -50,10 +51,9 @@
struct walk_baton *wb = baton;
svn_revnum_t changed_rev;
svn_revnum_t revision;
+ svn_revnum_t item_rev;
svn_depth_t depth;
svn_wc__db_status_t status;
- svn_boolean_t wc_root;
- svn_boolean_t switched;
SVN_ERR(svn_wc__db_read_info(status, NULL, revision, NULL,
NULL, NULL, changed_rev,
@@ -71,24 +71,36 @@
wb-result-sparse_checkout = TRUE;
return SVN_NO_ERROR;
}
+ else if (status == svn_wc__db_status_not_present)
+{
+ return SVN_NO_ERROR;
+}
+ else if (status == svn_wc__db_status_added
+ || status == svn_wc__db_status_obstructed_add
+ || status == svn_wc__db_status_deleted
+ || status == svn_wc__db_status_obstructed_delete)
+{
+ wb-result-modified = TRUE;
+}
(Minor nit: else is redundant after a return. I don't particularly
mind, but somebody objected to them the other day.)
I'm insisting on my if-else construction. Just a matter of personal
preferences. I think the if-else construction ties the function that
part together and makes it more readable.
Can I get your +1 for this?
Daniel
Index: subversion/libsvn_wc/revision_status.c
===
--- subversion/libsvn_wc/revision_status.c (revision 930178)
+++ subversion/libsvn_wc/revision_status.c (working copy)
@@ -40,8 +40,13 @@ struct walk_baton
svn_wc__db_t *db;
};
-/* An svn_wc__node_found_func_t callback function for analyzing the status
- * of nodes */
+/* An svn_wc__node_found_func_t callback function for analyzing the wc
+ * status of LOCAL_ABSPATH. Since it can be invoked for a lot of paths in
+ * a wc but some data , i.e. if the wc is switched or has modifications, is
+ * expensive to calculate, we optimize by checking if those values are
+ * already set before runnning the db operations. The found status
+ * information is stored in BATON. Temporary allocations are made in
+ * SCRATCH_POOL. */
static svn_error_t *
analyze_status(const char *local_abspath,
void *baton,
@@ -50,10 +55,9 @@ analyze_status(const char *local_abspath,
struct walk_baton *wb = baton;
svn_revnum_t changed_rev;
svn_revnum_t revision;
+ svn_revnum_t item_rev;
svn_depth_t depth;
svn_wc__db_status_t status;
- svn_boolean_t wc_root;
- svn_boolean_t switched;
SVN_ERR(svn_wc__db_read_info(status, NULL, revision, NULL,
NULL, NULL, changed_rev,
@@ -71,24 +75,36 @@ analyze_status(const char *local_abspath,
wb-result-sparse_checkout = TRUE;
return SVN_NO_ERROR;
}
+ else if (status == svn_wc__db_status_not_present)
+{
+ return SVN_NO_ERROR;
+}
+ else if (status == svn_wc__db_status_added
+ || status == svn_wc__db_status_obstructed_add
+ || status == svn_wc__db_status_deleted
+ || status == svn_wc__db_status_obstructed_delete)
+{
+ wb-result-modified = TRUE;
+}
- if (status == svn_wc__db_status_not_present)
-return SVN_NO_ERROR;
-
if (! wb-result-switched
[PATCH] v2 Fix svnversion message as follow-up to r922176
On Tue, Mar 30, 2010 at 11:04:13PM +0200, Stefan Sperling wrote:
On Tue, Mar 30, 2010 at 10:14:42PM +0200, Daniel Näslund wrote:
Ping! This patch has not been reviewed!
On Sun, Mar 14, 2010 at 09:38:15PM +0100, Daniel Näslund wrote:
Hi!
The 1.6 svnversion message was 'path' not versioned, and not exported.
But on trunk more than one message has been changed. My first thought
was that we should be backward compat in our output but if changes of
those messages are ok I'm supplying one.
In case we will use new messages, the help text must be updated. It
talks of 'exported' but those are not used in the new messages.
[[[
After the changes in r922176, versioned but not yet committed files were
not properly detected. Fixed now!
* subversion/svnversion/main.c
(main): Check for invalid rev nr for files and dirs.
* subversion/tests/cmdline/svnversion_tests.py
(structural_changes): New.
(tests_list): Add new test.
]]]
Index: subversion/svnversion/main.c
===
--- subversion/svnversion/main.c (revision 922931)
+++ subversion/svnversion/main.c (arbetskopia)
@@ -290,6 +290,17 @@
return EXIT_FAILURE;
}
+ if (res-min_rev == -1)
s/-1/SVN_INVALID_REVNUM/
Fixed!
+{
+ /* Local uncommited modifications, no revision info was found. */
+ SVN_INT_ERR(svn_cmdline_printf(pool, _(Local uncommitted
+ modifications, no revision
+ information found%s),
+ no_newline ? : \n));
No revision information found sounds a bit like something went
wrong, as in, Subversion was looking for information that was
expected to be present but it didn't find the information.
What about Uncommitted local addition, copy, or move%s instead?
I agree with you (user messages is my weak spot). The only cases when we
can have no revision number but the path is under version control is for
locally added, copied or moved paths. So we tell the user about it.
Can I have your +1 for commiting?
Daniel
Index: subversion/tests/cmdline/svnversion_tests.py
===
--- subversion/tests/cmdline/svnversion_tests.py(revision 930178)
+++ subversion/tests/cmdline/svnversion_tests.py(working copy)
@@ -236,7 +236,36 @@ def svnversion_with_excluded_subtrees(sbox):
svntest.actions.run_and_verify_svnversion(working copy with excluded file,
D_path, repo_url + '/A/D',
[ 1P\n ], [])
+def structural_changes(sbox):
+ test 'svnversion' with structural changes
+ sbox.build()
+ wc_dir = sbox.wc_dir
+ repo_url = sbox.repo_url
+ iota_path = os.path.join(wc_dir, 'iota')
+ iota_copy_path = os.path.join(wc_dir, 'iota_copy')
+
+ svntest.actions.run_and_verify_svn(None, None, [],
+ 'cp', iota_path, iota_copy_path)
+
+ svntest.actions.run_and_verify_svnversion(Copied file,
+iota_copy_path, repo_url +
+'/iota_copy',
+[ Local uncommitted
+modifications, no revision
+information found\n ], [])
+ C_path = os.path.join(wc_dir, 'A', 'C')
+ C_copy_path = os.path.join(wc_dir, 'C_copy')
+ svntest.actions.run_and_verify_svn(None, None, [],
+ 'cp', C_path, C_copy_path)
+
+ svntest.actions.run_and_verify_svnversion(Copied dir,
+C_copy_path, repo_url +
+'/C_copy',
+[ Local uncommitted
+modifications, no revision
+information found\n ], [])
+
# Run the tests
@@ -246,6 +275,7 @@ test_list = [ None,
svnversion_test,
ignore_externals,
svnversion_with_excluded_subtrees,
+ structural_changes,
]
if __name__ == '__main__':
Index: subversion/svnversion/main.c
===
--- subversion/svnversion/main.c(revision 930178)
+++ subversion/svnversion/main.c(working copy)
@@ -290,6 +290,16 @@ main(int argc, const char *argv[])
return EXIT_FAILURE;
}
+ if (res-min_rev == SVN_INVALID_REVNUM)
+{
+ /* Local uncommited modifications, no revision info was found. */
+ SVN_INT_ERR(svn_cmdline_printf(pool, _(Uncommited local addition
Re: [PATCH] v2 Fix svnversion message as follow-up to r922176
Mispelled Uncommitted with one 't'. This patch corrects that.
On Fri, Apr 02, 2010 at 10:36:53AM +0200, Daniel Näslund wrote:
On Tue, Mar 30, 2010 at 11:04:13PM +0200, Stefan Sperling wrote:
On Tue, Mar 30, 2010 at 10:14:42PM +0200, Daniel Näslund wrote:
Ping! This patch has not been reviewed!
On Sun, Mar 14, 2010 at 09:38:15PM +0100, Daniel Näslund wrote:
Hi!
The 1.6 svnversion message was 'path' not versioned, and not exported.
But on trunk more than one message has been changed. My first thought
was that we should be backward compat in our output but if changes of
those messages are ok I'm supplying one.
In case we will use new messages, the help text must be updated. It
talks of 'exported' but those are not used in the new messages.
[[[
After the changes in r922176, versioned but not yet committed files were
not properly detected. Fixed now!
* subversion/svnversion/main.c
(main): Check for invalid rev nr for files and dirs.
* subversion/tests/cmdline/svnversion_tests.py
(structural_changes): New.
(tests_list): Add new test.
]]]
Index: subversion/svnversion/main.c
===
--- subversion/svnversion/main.c(revision 922931)
+++ subversion/svnversion/main.c(arbetskopia)
@@ -290,6 +290,17 @@
return EXIT_FAILURE;
}
+ if (res-min_rev == -1)
s/-1/SVN_INVALID_REVNUM/
Fixed!
+{
+ /* Local uncommited modifications, no revision info was found. */
+ SVN_INT_ERR(svn_cmdline_printf(pool, _(Local uncommitted
+ modifications, no
revision
+ information found%s),
+ no_newline ? : \n));
No revision information found sounds a bit like something went
wrong, as in, Subversion was looking for information that was
expected to be present but it didn't find the information.
What about Uncommitted local addition, copy, or move%s instead?
I agree with you (user messages is my weak spot). The only cases when we
can have no revision number but the path is under version control is for
locally added, copied or moved paths. So we tell the user about it.
Can I have your +1 for commiting?
Index: subversion/tests/cmdline/svnversion_tests.py
===
--- subversion/tests/cmdline/svnversion_tests.py(revision 930183)
+++ subversion/tests/cmdline/svnversion_tests.py(working copy)
@@ -236,7 +236,36 @@ def svnversion_with_excluded_subtrees(sbox):
svntest.actions.run_and_verify_svnversion(working copy with excluded file,
D_path, repo_url + '/A/D',
[ 1P\n ], [])
+def structural_changes(sbox):
+ test 'svnversion' with structural changes
+ sbox.build()
+ wc_dir = sbox.wc_dir
+ repo_url = sbox.repo_url
+ iota_path = os.path.join(wc_dir, 'iota')
+ iota_copy_path = os.path.join(wc_dir, 'iota_copy')
+
+ svntest.actions.run_and_verify_svn(None, None, [],
+ 'cp', iota_path, iota_copy_path)
+
+ svntest.actions.run_and_verify_svnversion(Copied file,
+iota_copy_path, repo_url +
+'/iota_copy',
+[ Local uncommitted
+modifications, no revision
+information found\n ], [])
+ C_path = os.path.join(wc_dir, 'A', 'C')
+ C_copy_path = os.path.join(wc_dir, 'C_copy')
+ svntest.actions.run_and_verify_svn(None, None, [],
+ 'cp', C_path, C_copy_path)
+
+ svntest.actions.run_and_verify_svnversion(Copied dir,
+C_copy_path, repo_url +
+'/C_copy',
+[ Local uncommitted
+modifications, no revision
+information found\n ], [])
+
# Run the tests
@@ -246,6 +275,7 @@ test_list = [ None,
svnversion_test,
ignore_externals,
svnversion_with_excluded_subtrees,
+ structural_changes,
]
if __name__ == '__main__':
Index: subversion/svnversion/main.c
===
--- subversion/svnversion/main.c(revision 930183)
+++ subversion/svnversion/main.c(working copy)
@@ -290,6 +290,16 @@ main(int argc, const char *argv[])
return EXIT_FAILURE
Re: [PATCH] v4 Fix svnversion message as follow-up to r922176
We're running out of patch versions here ...
gstein pointed out that I should use SVN_IS_VALID_REVNUM(). Fixed!
On Fri, Apr 02, 2010 at 10:59:15AM +0200, Daniel Näslund wrote:
Mispelled Uncommitted with one 't'. This patch corrects that.
On Fri, Apr 02, 2010 at 10:36:53AM +0200, Daniel Näslund wrote:
On Tue, Mar 30, 2010 at 11:04:13PM +0200, Stefan Sperling wrote:
On Tue, Mar 30, 2010 at 10:14:42PM +0200, Daniel Näslund wrote:
Ping! This patch has not been reviewed!
On Sun, Mar 14, 2010 at 09:38:15PM +0100, Daniel Näslund wrote:
Hi!
The 1.6 svnversion message was 'path' not versioned, and not
exported.
But on trunk more than one message has been changed. My first thought
was that we should be backward compat in our output but if changes of
those messages are ok I'm supplying one.
In case we will use new messages, the help text must be updated. It
talks of 'exported' but those are not used in the new messages.
[[[
After the changes in r922176, versioned but not yet committed files
were
not properly detected. Fixed now!
* subversion/svnversion/main.c
(main): Check for invalid rev nr for files and dirs.
* subversion/tests/cmdline/svnversion_tests.py
(structural_changes): New.
(tests_list): Add new test.
]]]
Index: subversion/tests/cmdline/svnversion_tests.py
===
--- subversion/tests/cmdline/svnversion_tests.py(revision 930183)
+++ subversion/tests/cmdline/svnversion_tests.py(working copy)
@@ -236,7 +236,36 @@ def svnversion_with_excluded_subtrees(sbox):
svntest.actions.run_and_verify_svnversion(working copy with excluded file,
D_path, repo_url + '/A/D',
[ 1P\n ], [])
+def structural_changes(sbox):
+ test 'svnversion' with structural changes
+ sbox.build()
+ wc_dir = sbox.wc_dir
+ repo_url = sbox.repo_url
+ iota_path = os.path.join(wc_dir, 'iota')
+ iota_copy_path = os.path.join(wc_dir, 'iota_copy')
+
+ svntest.actions.run_and_verify_svn(None, None, [],
+ 'cp', iota_path, iota_copy_path)
+
+ svntest.actions.run_and_verify_svnversion(Copied file,
+iota_copy_path, repo_url +
+'/iota_copy',
+[ Local uncommitted
+modifications, no revision
+information found\n ], [])
+ C_path = os.path.join(wc_dir, 'A', 'C')
+ C_copy_path = os.path.join(wc_dir, 'C_copy')
+ svntest.actions.run_and_verify_svn(None, None, [],
+ 'cp', C_path, C_copy_path)
+
+ svntest.actions.run_and_verify_svnversion(Copied dir,
+C_copy_path, repo_url +
+'/C_copy',
+[ Local uncommitted
+modifications, no revision
+information found\n ], [])
+
# Run the tests
@@ -246,6 +275,7 @@ test_list = [ None,
svnversion_test,
ignore_externals,
svnversion_with_excluded_subtrees,
+ structural_changes,
]
if __name__ == '__main__':
Index: subversion/svnversion/main.c
===
--- subversion/svnversion/main.c(revision 930183)
+++ subversion/svnversion/main.c(working copy)
@@ -290,6 +290,16 @@ main(int argc, const char *argv[])
return EXIT_FAILURE;
}
+ if (SVN_IS_VALID_REVNUM(res-min_rev))
+{
+ /* Local uncommited modifications, no revision info was found. */
+ SVN_INT_ERR(svn_cmdline_printf(pool, _(Uncommitted local addition
+ copy, or move%s),
+ no_newline ? : \n));
+ svn_pool_destroy(pool);
+ return EXIT_SUCCESS;
+}
+
/* Build compact '123[:456]M?S?' string. */
SVN_INT_ERR(svn_cmdline_printf(pool, %ld, res-min_rev));
if (res-min_rev != res-max_rev)
Re: logging #svn-dev
On Tue, Mar 30, 2010 at 02:15:32PM +0200, neels wrote: Hi all, every now and then I would find it useful if there was a public log of #svn-dev (on FreeNode IRC). I believe it was previously decided not to log #svn-dev because discussions about potential new committers took place there. Since I joined the project, I have only seen new-committer discussions on the committers@ mailing list. Anyway, #svn-dev is as public as can be. Anyone could join and log. I'm writing this mail to get official agreement that we want #svn-dev to be logged. All that needs to be done is that we get a bunch of +1s here and then tell the guy on #irclogger to please log #svn-dev, including a reference to this mail thread. The channel will then be logged exactly like #svn is logged right now. +1 Daniel
[WIP] Replace entry calls for revisions in svn_wc_status2_t
Hi!
This patch passes make check with modifications to some tests to take
into account some changed behaviors. I've tried to justificate those
changes in the log message below.
Is this really right?
==
When scanning for a deletion I never use the base_del_abspath parameter
of scan_deletion(). Shouldn't there be some tests that would fail since
we're not checking that? My idea was that the op root of a 'svn delete'
operation could only be detected with the base_del_abspath parameter.
I've set the revision, changed_rev and changed_author to their nil
values if the read_info() says we have an addition. changed_rev and
changed_author could only be values from previous revisions so if
revision is -1 they can only have their nil values.
Things I need to fix
=
I've disabled the entries check for some tests below. I should do this
in a more fine-grained manner. gstein suggested what I should do on
#svn-dev. I listened mindfully to what he had to say and then forgot all
about it. :-(
The update test sets the revision to '?' but it should be '0' I need to
change the logic of print_status() in svn/status.c to allow it to be
'0'. There should be some way to say that it is part of a copy (it's
parent is copied but itself is deleted).
[[[
As part of WC-NG, replace entry calls for revisions from
svn_wc_status2_t related code.
The entries code set the revision of newly added nodes to 0 but the db
sets them to -1. Since too many tests needs to be changed and 'svn info'
also uses 0, I'll change those values in a follow-up patch instead.
* subversion/tests/cmdline/copy_tests.py
(repos_to_wc): Change revision to 0 since the node is added. Don't
check against entries since the behavior differs.
* subversion/tests/cmdline/stat_tests.py
(status_with_tree_conflicts): An copied node should not have a
changed_rev or author.
* subversion/tests/cmdline/update_tests.py
(tree_conflict_uc2_schedule_re_add): A scheduled delete in a copied
tree. The parent operation decides what the revision should be.
Don't check against entries since the behavior differs.
* subversion/tests/cmdline/svntest/actions.py
(run_and_verify_status): Add new parameter check_entries. We only
check the entries if check_entries is set to True.
* subversion/tests/cmdline/merge_tests.py
(merge_into_missing): Add revision to status output for missing dir.
Previously, missing dirs did not have a revision, only files had.
Don't check entries since the behavior differs.
* subversion/svn/status.c
(print_status): Replace checks for revisions using entries with the
direct fields in svn_wc_status2_t. Set the revision for added and
replaced nodes to 0. WC-NG sets those revisions to -1, but changing
all the involved tests is for a follow-up.
* subversion/include/svn_wc.h
(svn_wc_status2_t): Add fields revision, changed_rev and
changed_author.
* subversion/libsvn_wc/status.c
(assemble_status): Fill in the new fields with data from the wc db.
(svn_wc_dup_status2): Copy the changed_author field.
]]]
cheers,
Daniel
Index: subversion/tests/cmdline/copy_tests.py
===
--- subversion/tests/cmdline/copy_tests.py (revision 929211)
+++ subversion/tests/cmdline/copy_tests.py (working copy)
@@ -980,9 +980,10 @@ def repos_to_wc(sbox):
expected_output = svntest.actions.get_virginal_state(wc_dir, 1)
expected_output.add({
-'pi' : Item(status='A ', wc_rev='1'),
+'pi' : Item(status='A ', wc_rev='0'),
})
- svntest.actions.run_and_verify_status(wc_dir, expected_output)
+ svntest.actions.run_and_verify_status(wc_dir, expected_output,
+check_entries=False)
# Revert everything and verify.
svntest.actions.run_and_verify_svn(None, None, [], 'revert', '-R', wc_dir)
Index: subversion/tests/cmdline/stat_tests.py
===
--- subversion/tests/cmdline/stat_tests.py (revision 929211)
+++ subversion/tests/cmdline/stat_tests.py (working copy)
@@ -1593,7 +1593,7 @@ def status_with_tree_conflicts(sbox):
[ 22 jrandom %s\n % G,
D C 22 jrandom %s\n % pi,
local delete, incoming edit upon update\n,
- A + C -1 jrandom %s\n % rho,
+ A + C - ? ? %s\n % rho,
local edit, incoming delete upon update\n,
! C %s\n % tau,
local delete, incoming delete upon update\n,
Index: subversion/tests/cmdline/update_tests.py
===
--- subversion/tests/cmdline/update_tests.py (revision 929211)
+++ subversion/tests/cmdline/update_tests.py (working copy)
@@ -5128,7 +5128,7 @@ def tree_conflict_uc2_schedule_re_add(sbox):
Re: [PATCH] Follow-up to r922176 was:Re: svn commit: r922176 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revision_status.c svnversion/main.c
Ping! This patch has not been revieved
On Fri, Mar 12, 2010 at 10:52:55PM +0100, Daniel Näslund wrote:
On Fri, Mar 12, 2010 at 05:43:18AM -0500, Greg Stein wrote:
On Fri, Mar 12, 2010 at 03:21, dan...@apache.org wrote:
...
+++ subversion/trunk/subversion/libsvn_wc/revision_status.c Fri Mar 12
08:21:45 2010
...
{
- struct status_baton *sb = baton;
+ struct walk_baton *wb = baton;
+ svn_revnum_t changed_rev;
+ svn_revnum_t revision;
+ svn_depth_t depth;
+ svn_wc__db_status_t status;
+ svn_boolean_t wc_root;
+ svn_boolean_t switched;
wc_root and switched can be moved into a tighter scope.
Fixed!
- if (status-entry-depth != svn_depth_exclude)
+ /* Added files have a revision of no interest */
+ if (revision != SVN_INVALID_REVNUM)
{
- sb-result-switched |= status-switched;
- sb-result-modified |= (status-text_status !=
svn_wc_status_normal);
- sb-result-modified |= (status-prop_status !=
svn_wc_status_normal
- status-prop_status !=
svn_wc_status_none);
+ svn_revnum_t item_rev = (wb-committed
+ ? changed_rev
+ : revision);
I think this may introduce a bug. Depending on wb-committed, we look
at different revision values. And it may be that REVISION is valid,
but CHANGED_REV is not. I would suggest moving the assignment of
ITEM_REV one block out, and using that in the primary if() test.
Fixed, although I must admit that I don't truly understand how
changed_rev and revision differs!
My change of the caller svnversion/main.c in r922176 caused a problem
when svnversion was invoked on a newly added path. It is under version
control but has no revision number. At the moment '-1' is returned for
such a path. I intend to fix that in a separate patch.
[[[
Follow-up to r922176. Fix that tree changes were not considered when
determining if the wc has modifications.
* subversion/libsvn_wc/revision_status.c
(analyze_status): Determine from status if a path has been added or
deleted. Do some optimizations to avoid having to do a text
comparison for determining if a wc has modifications.
Suggested by: gstein
rhuijben
Approved by: ?
]]]
Index: subversion/libsvn_wc/revision_status.c
===
--- subversion/libsvn_wc/revision_status.c(revision 922398)
+++ subversion/libsvn_wc/revision_status.c(arbetskopia)
@@ -41,7 +41,8 @@
};
/* An svn_wc__node_found_func_t callback function for analyzing the status
- * of nodes */
+ * of nodes. Optimized to avoid text compares and unneccessary checks of
+ * already set values. */
static svn_error_t *
analyze_status(const char *local_abspath,
void *baton,
@@ -50,10 +51,9 @@
struct walk_baton *wb = baton;
svn_revnum_t changed_rev;
svn_revnum_t revision;
+ svn_revnum_t item_rev;
svn_depth_t depth;
svn_wc__db_status_t status;
- svn_boolean_t wc_root;
- svn_boolean_t switched;
SVN_ERR(svn_wc__db_read_info(status, NULL, revision, NULL,
NULL, NULL, changed_rev,
@@ -71,24 +71,36 @@
wb-result-sparse_checkout = TRUE;
return SVN_NO_ERROR;
}
+ else if (status == svn_wc__db_status_not_present)
+{
+ return SVN_NO_ERROR;
+}
+ else if (status == svn_wc__db_status_added
+ || status == svn_wc__db_status_obstructed_add
+ || status == svn_wc__db_status_deleted
+ || status == svn_wc__db_status_obstructed_delete)
+{
+ wb-result-modified = TRUE;
+}
- if (status == svn_wc__db_status_not_present)
-return SVN_NO_ERROR;
-
if (! wb-result-switched)
{
+ svn_boolean_t wc_root;
+ svn_boolean_t switched;
+
SVN_ERR(svn_wc__check_wc_root(wc_root, NULL, switched, wb-db,
local_abspath, scratch_pool));
wb-result-switched |= switched;
}
+ item_rev = (wb-committed
+ ? changed_rev
+ : revision);
+
/* Added files have a revision of no interest */
- if (revision != SVN_INVALID_REVNUM)
+ if (item_rev != SVN_INVALID_REVNUM)
{
- svn_revnum_t item_rev = (wb-committed
- ? changed_rev
- : revision);
if (wb-result-min_rev == SVN_INVALID_REVNUM
|| item_rev wb-result-min_rev)
@@ -101,22 +113,27 @@
if (! wb-result-modified)
{
- svn_boolean_t text_mod;
svn_boolean_t props_mod;
SVN_ERR(svn_wc__props_modified(props_mod, wb-db, local_abspath,
scratch_pool));
+ wb-result-modified |= props_mod;
+}
+ if (! wb-result-modified)
+{
+ svn_boolean_t text_mod;
+
SVN_ERR
Re: [PATCH] Fix svnversion message as follow-up to r922176
Ping! This patch has not been reviewed!
On Sun, Mar 14, 2010 at 09:38:15PM +0100, Daniel Näslund wrote:
Hi!
The 1.6 svnversion message was 'path' not versioned, and not exported.
But on trunk more than one message has been changed. My first thought
was that we should be backward compat in our output but if changes of
those messages are ok I'm supplying one.
In case we will use new messages, the help text must be updated. It
talks of 'exported' but those are not used in the new messages.
[[[
After the changes in r922176, versioned but not yet committed files were
not properly detected. Fixed now!
* subversion/svnversion/main.c
(main): Check for invalid rev nr for files and dirs.
* subversion/tests/cmdline/svnversion_tests.py
(structural_changes): New.
(tests_list): Add new test.
]]]
Index: subversion/tests/cmdline/svnversion_tests.py
===
--- subversion/tests/cmdline/svnversion_tests.py (revision 922931)
+++ subversion/tests/cmdline/svnversion_tests.py (arbetskopia)
@@ -236,7 +236,36 @@
svntest.actions.run_and_verify_svnversion(working copy with excluded
file,
D_path, repo_url + '/A/D',
[ 1P\n ], [])
+def structural_changes(sbox):
+ test 'svnversion' with structural changes
+ sbox.build()
+ wc_dir = sbox.wc_dir
+ repo_url = sbox.repo_url
+ iota_path = os.path.join(wc_dir, 'iota')
+ iota_copy_path = os.path.join(wc_dir, 'iota_copy')
+
+ svntest.actions.run_and_verify_svn(None, None, [],
+ 'cp', iota_path, iota_copy_path)
+
+ svntest.actions.run_and_verify_svnversion(Copied file,
+iota_copy_path, repo_url +
+'/iota_copy',
+[ Local uncommitted
+modifications, no revision
+information found\n ], [])
+ C_path = os.path.join(wc_dir, 'A', 'C')
+ C_copy_path = os.path.join(wc_dir, 'C_copy')
+ svntest.actions.run_and_verify_svn(None, None, [],
+ 'cp', C_path, C_copy_path)
+
+ svntest.actions.run_and_verify_svnversion(Copied dir,
+C_copy_path, repo_url +
+'/C_copy',
+[ Local uncommitted
+modifications, no revision
+information found\n ], [])
+
# Run the tests
@@ -246,6 +275,7 @@
svnversion_test,
ignore_externals,
svnversion_with_excluded_subtrees,
+ structural_changes,
]
if __name__ == '__main__':
Index: subversion/svnversion/main.c
===
--- subversion/svnversion/main.c (revision 922931)
+++ subversion/svnversion/main.c (arbetskopia)
@@ -290,6 +290,17 @@
return EXIT_FAILURE;
}
+ if (res-min_rev == -1)
+{
+ /* Local uncommited modifications, no revision info was found. */
+ SVN_INT_ERR(svn_cmdline_printf(pool, _(Local uncommitted
+ modifications, no revision
+ information found%s),
+ no_newline ? : \n));
+ svn_pool_destroy(pool);
+ return EXIT_SUCCESS;
+}
+
/* Build compact '123[:456]M?S?' string. */
SVN_INT_ERR(svn_cmdline_printf(pool, %ld, res-min_rev));
if (res-min_rev != res-max_rev)
Re: What revision should an added not yet commited node have?
On Fri, Mar 19, 2010 at 02:33:33AM +0100, Neels J Hofmeyr wrote:
C. Michael Pilato wrote:
Stefan Sperling wrote:
On Tue, Mar 16, 2010 at 09:01:28PM +0100, Daniel Näslund wrote:
Hi!
When trying to replace entries in the status code I got a couple of test
failures saying that the revision should be 0 for newly added nodes.
Greg pointed out that the entries code set the revision to 0 for those
cases while the revision returned from _read_info() sets it to -1.
Should we continue to use the 0 value? Is it well established as the
revision number of version controlled, not yet committed files or should
we tell 'svn info' and 'svn status' to not output any rev nr at all for
these nodes?
I think -1 (invalid revnum) is more appropriate than 0.
Nice, I hit that same question like two weeks ago, with
svn_client__get_revision_number() upon svn_opt_revision_base for added
nodes. I found the same conclusion: it should have always returned -1.
I am at the point where I would trial to see how callers deal with a -1
revision number (would because I need to study for an exam next week, bah).
Ideally, we will change the behaviour of this private function when
switching it to wc-ng. I hope we don't have to mock up current behaviour for
compat, especially because that depends on parent nodes sometimes.
I've decided to postpone the change of revisions set to zero to a
follow-up. Two reasons: (i) I'm _replacing_ revisions fetched from the
entries file with revisions fetched from the db, changing the behaviour
is a separate task that involves fiddling with a lot of test, causing me
to loose momentum with the svn_wc_status2_t work. (ii) The info code
uses zeroes too for newly added nodes. It would be better to change the
behavior of 'svn {info,status}' at the same time.
When it's time to replace the zeroes in the CLI, I opt for leaving those
fields blank. We have no revision, we show no revision. Plain and
simple!
Btw, other diffs I've found between entries and db handling of
revisions: (i) A missing dir has no revision value in the entries code,
but it has in the db. (ii) A file copied from foreign url to a wc has
the revision number set to its value in the foreign repo, although it is
locally added w/o history in this wc and thus should have 0. That's for
the entries. The db sets it to -1.
I have some quirks with switching locally added files but that's for
another post.
Daniel
Re: Will subversion participate in Google Summer of Code 2010?
On Fri, Mar 12, 2010 at 10:00:48AM +0100, Stefan Sperling wrote:
On Tue, Mar 09, 2010 at 03:01:36PM +, Justin Erenkrantz wrote:
On Tue, Mar 9, 2010 at 9:59 AM, Stefan Sperling s...@elego.de wrote:
Does this mean that we don't have to do anything special to apply
for Gsoc, but just apply for slots within ASF when the time comes?
Yup - you just apply internally.
I believe someone from SVN should subscribe to
d...@community.apache.org [1] and poke the people over there about what
SVN should do. (I haven't been tracking the process, but the GSoC
admins on Apache's end are over there and talking about this whole
thing.)
I am subscribed to that list now. Today I also got instructions
via pmcs@ about what to do apply.
These pages pretty much say it all:
http://community.apache.org/gsoc.html
http://community.apache.org/guide-to-being-a-mentor.html
Now, I'd like to post to d...@community.a.o and present Subversion's
list of project ideas. Can people who are interested in Gsoc (students
as well as potential mentors as well as people who are good at coming
up with project ideas) help write down a definite list (including
suitable project idea descriptions) of Gsoc project ideas for Subversion?
In this thread?
Ok, I've already mentioned it but here's my main suggestion:
Git diff format
-
svn {diff,patch} should be able to use the git diff format. That would
allow us to use patches for tree and mode changes. It should be
(According to a statement by Augie Fackler in
notes/svnpatch/svnpatch-git.txt) possible to our own hunk types to this
format and thereby allowing us to transfer property modifications.
After that, it gets harder. I don't have the neccessary experience to
come up with more things...
Editor-v2
---
Something with authz
-
Something with externals
---
Something with HTTP performance
-
Daniel
Re: [PATCH] Follow-up to r922176 was:Re: svn commit: r922176 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revision_status.c svnversion/main.c
On Fri, Mar 12, 2010 at 07:42:53PM -0600, Hyrum K. Wright wrote:
On Mar 12, 2010, at 3:52 PM, Daniel Näslund wrote:
- if (status-entry-depth != svn_depth_exclude)
+ /* Added files have a revision of no interest */
+ if (revision != SVN_INVALID_REVNUM)
{
- sb-result-switched |= status-switched;
- sb-result-modified |= (status-text_status !=
svn_wc_status_normal);
- sb-result-modified |= (status-prop_status !=
svn_wc_status_normal
-status-prop_status !=
svn_wc_status_none);
+ svn_revnum_t item_rev = (wb-committed
+ ? changed_rev
+ : revision);
I think this may introduce a bug. Depending on wb-committed, we look
at different revision values. And it may be that REVISION is valid,
but CHANGED_REV is not. I would suggest moving the assignment of
ITEM_REV one block out, and using that in the primary if() test.
Fixed, although I must admit that I don't truly understand how
changed_rev and revision differs!
'Changed rev' is the last time the object was modified in the repo.
'Revision' refers to which revision the working copy thinks this
object is up-to-date to.
For instance, if I change foo.c in r10, and then later update to r12,
if foo.c wasn't modified by the update, the last changed revision
would still be r10, but revision would be r12. You've every right to
be confused, btw, since in wc-1 entry-revision is overloaded about 4
different ways.
Ah, it was that simple. I got a bit twisted up when reading
wc-metadata.sql and forgot the big picture. I was sure there was
something not-obvious to it. Glad it wasn't.
Thanks,
Daniel
Re: [PATCH v3] Replace entries in revisision_status.c
On Thu, Mar 11, 2010 at 05:44:18PM -0500, Greg Stein wrote: [[[ As part of WC-NG, remove some uses of svn_wc_entry_t. * subversion/include/svn_wc.h (svn_wc_revision_status2): Add note about us returning SVN_ERR_WC_PATH_NOT_FOUND. * subversion/libsvn_wc/revision_status.c (status_baton): Rename this... (walk_baton): .. to this. (analyze_status): Change this to implement svn_wc__node_func_t and use WC-NG funcs instead of information in svn_wc_entry_t. (svn_wc_revision_status2): Use svn_wc__node_walk_children() instead of svn_wc_walk_status() to spare us from the overhead of invoking an editor. ]]] Per IRC, you've got my +1 to commit. There are some other minor improvements, but let's get this in first. I can respond to the commit message. Committed in r922176.
[PATCH] Follow-up to r920118. was:Re: svn commit: r920118 - in /subversion/trunk/subversion: libsvn_client/cleanup.c tests/cmdline/upgrade_tests.py tests/cmdline/upgrade_tests_data/upgrade_with_extern
On Tue, Mar 09, 2010 at 02:19:52PM +, Julian Foad wrote:
On Sun, 2010-03-07, dan...@apache.org wrote:
Author: dannas
Date: Sun Mar 7 21:21:58 2010
New Revision: 920118
URL: http://svn.apache.org/viewvc?rev=920118view=rev
Log:
Fix issue #3483 - extend svn_client_upgrade() to include externals. I've
done the externals upgrading after wc upgrade is finished. In that way
no errors in the externals will affect the wc.
Hi Daniel. A few post-commit review comments.
--- subversion/trunk/subversion/libsvn_client/cleanup.c (original)
+++ subversion/trunk/subversion/libsvn_client/cleanup.c Sun Mar 7 21:21:58
2010
@@ -34,8 +34,11 @@
#include svn_dirent_uri.h
#include svn_pools.h
#include client.h
+#include svn_pools.h
Included twice now. Bert already included it, just above, a few days
before :-)
Fixed!
@@ -123,5 +130,79 @@
ctx-notify_func2, ctx-notify_baton2,
scratch_pool));
+ /* Now it's time to upgrade the externals too. We do it after the wc
+ upgrade to avoid that errors in the externals causes the wc upgrade to
+ fail. Thanks to caching the performance penalty of walking the wc a
+ second time shouldn't be too severe */
+ SVN_ERR(svn_client_propget3(externals, SVN_PROP_EXTERNALS, path, rev,
+ rev, NULL, svn_depth_infinity, NULL, ctx,
+ scratch_pool));
+
+ iterpool = svn_pool_create(scratch_pool);
+
+ for (hi = apr_hash_first(scratch_pool, externals); hi;
+ hi = apr_hash_next(hi))
+{
+ const char *key;
+ int i;
+ apr_ssize_t klen;
+ svn_string_t *external_desc;
+ apr_array_header_t *externals_p;
+
+ svn_pool_clear(iterpool);
+ externals_p = apr_array_make(iterpool, 1,
+ sizeof(svn_wc_external_item2_t*));
+
+ apr_hash_this(hi, (void*)key, klen, NULL);
+
+ external_desc = apr_hash_get(externals, key, klen);
apr_hash_this() can give you the current item's key and value, so you
don't need to look up the value separately: you can just
apr_hash_this(hi, key, NULL, value). However, I discourage use of
apr_hash_this() as it requires (void *) pointers, as you have seen.
A neater solution enables the key and value to be assigned straight in
to variables of the appropriate type:
for (hi = ...)
{
const char *external_path = svn_apr_hash_index_key(hi);
svn_string_t *external_desc = svn_apr_hash_index_value(hi);
...
}
And by calling variable 'external_path' rather than 'key' you can
describe the data it holds. (I see you have an 'external_path' in the
inner loop too, so you might want to choose a different name or
eliminate the inner one.)
Fixed!
Can this be committed?
[[[
Follow up to r920118.
* subversion/libsvn_client/cleanup.c
(svn_client_upgrade): Use svn__apr_hash_index_{key,val} to avoid
casts. Use a more descriptive variable name for the path holding the
svn:externals declaration.
Suggested by: philipm
julianfoad
]]]
Index: subversion/libsvn_client/cleanup.c
===
--- subversion/libsvn_client/cleanup.c (revision 922358)
+++ subversion/libsvn_client/cleanup.c (arbetskopia)
@@ -34,7 +34,6 @@
#include svn_dirent_uri.h
#include svn_pools.h
#include client.h
-#include svn_pools.h
#include svn_props.h
#include svn_private_config.h
@@ -143,20 +142,15 @@
for (hi = apr_hash_first(scratch_pool, externals); hi;
hi = apr_hash_next(hi))
{
- const char *key;
int i;
- apr_ssize_t klen;
- svn_string_t *external_desc;
+ const char *externals_parent = svn__apr_hash_index_key(hi);
+ svn_string_t *external_desc = svn__apr_hash_index_val(hi);
apr_array_header_t *externals_p;
svn_pool_clear(iterpool);
externals_p = apr_array_make(iterpool, 1,
sizeof(svn_wc_external_item2_t*));
- apr_hash_this(hi, (void*)key, klen, NULL);
-
- external_desc = apr_hash_get(externals, key, klen);
-
SVN_ERR(svn_wc_parse_externals_description3(externals_p,
svn_dirent_dirname(path,
iterpool),
@@ -172,8 +166,8 @@
item = APR_ARRAY_IDX(externals_p, i, svn_wc_external_item2_t*);
- /* The key is the path to the dir the svn:externals was set on */
- external_path = svn_dirent_join(key, item-target_dir, iterpool);
+ external_path = svn_dirent_join(externals_parent, item-target_dir,
+ iterpool);
SVN_ERR(svn_dirent_get_absolute(external_abspath, external_path,
iterpool));
Re: [PATCH] Follow-up to r920118. was:Re: svn commit: r920118 - in /subversion/trunk/subversion: libsvn_client/cleanup.c tests/cmdline/upgrade_tests.py tests/cmdline/upgrade_tests_data/upgrade_with_ex
On Fri, Mar 12, 2010 at 06:15:27PM +, Philip Martin wrote:
Daniel Näslund dan...@longitudo.com writes:
Can this be committed?
[[[
Follow up to r920118.
* subversion/libsvn_client/cleanup.c
(svn_client_upgrade): Use svn__apr_hash_index_{key,val} to avoid
casts. Use a more descriptive variable name for the path holding the
svn:externals declaration.
Suggested by: philipm
julianfoad
]]]
Yes.
Commited in r922387.
[PATCH] Follow-up to r922176 was:Re: svn commit: r922176 - in /subversion/trunk/subversion: include/svn_wc.h libsvn_wc/revision_status.c svnversion/main.c
On Fri, Mar 12, 2010 at 05:43:18AM -0500, Greg Stein wrote:
On Fri, Mar 12, 2010 at 03:21, dan...@apache.org wrote:
...
+++ subversion/trunk/subversion/libsvn_wc/revision_status.c Fri Mar 12
08:21:45 2010
...
{
- struct status_baton *sb = baton;
+ struct walk_baton *wb = baton;
+ svn_revnum_t changed_rev;
+ svn_revnum_t revision;
+ svn_depth_t depth;
+ svn_wc__db_status_t status;
+ svn_boolean_t wc_root;
+ svn_boolean_t switched;
wc_root and switched can be moved into a tighter scope.
Fixed!
- if (status-entry-depth != svn_depth_exclude)
+ /* Added files have a revision of no interest */
+ if (revision != SVN_INVALID_REVNUM)
{
- sb-result-switched |= status-switched;
- sb-result-modified |= (status-text_status !=
svn_wc_status_normal);
- sb-result-modified |= (status-prop_status != svn_wc_status_normal
- status-prop_status !=
svn_wc_status_none);
+ svn_revnum_t item_rev = (wb-committed
+ ? changed_rev
+ : revision);
I think this may introduce a bug. Depending on wb-committed, we look
at different revision values. And it may be that REVISION is valid,
but CHANGED_REV is not. I would suggest moving the assignment of
ITEM_REV one block out, and using that in the primary if() test.
Fixed, although I must admit that I don't truly understand how
changed_rev and revision differs!
My change of the caller svnversion/main.c in r922176 caused a problem
when svnversion was invoked on a newly added path. It is under version
control but has no revision number. At the moment '-1' is returned for
such a path. I intend to fix that in a separate patch.
[[[
Follow-up to r922176. Fix that tree changes were not considered when
determining if the wc has modifications.
* subversion/libsvn_wc/revision_status.c
(analyze_status): Determine from status if a path has been added or
deleted. Do some optimizations to avoid having to do a text
comparison for determining if a wc has modifications.
Suggested by: gstein
rhuijben
Approved by: ?
]]]
Index: subversion/libsvn_wc/revision_status.c
===
--- subversion/libsvn_wc/revision_status.c (revision 922398)
+++ subversion/libsvn_wc/revision_status.c (arbetskopia)
@@ -41,7 +41,8 @@
};
/* An svn_wc__node_found_func_t callback function for analyzing the status
- * of nodes */
+ * of nodes. Optimized to avoid text compares and unneccessary checks of
+ * already set values. */
static svn_error_t *
analyze_status(const char *local_abspath,
void *baton,
@@ -50,10 +51,9 @@
struct walk_baton *wb = baton;
svn_revnum_t changed_rev;
svn_revnum_t revision;
+ svn_revnum_t item_rev;
svn_depth_t depth;
svn_wc__db_status_t status;
- svn_boolean_t wc_root;
- svn_boolean_t switched;
SVN_ERR(svn_wc__db_read_info(status, NULL, revision, NULL,
NULL, NULL, changed_rev,
@@ -71,24 +71,36 @@
wb-result-sparse_checkout = TRUE;
return SVN_NO_ERROR;
}
+ else if (status == svn_wc__db_status_not_present)
+{
+ return SVN_NO_ERROR;
+}
+ else if (status == svn_wc__db_status_added
+ || status == svn_wc__db_status_obstructed_add
+ || status == svn_wc__db_status_deleted
+ || status == svn_wc__db_status_obstructed_delete)
+{
+ wb-result-modified = TRUE;
+}
- if (status == svn_wc__db_status_not_present)
-return SVN_NO_ERROR;
-
if (! wb-result-switched)
{
+ svn_boolean_t wc_root;
+ svn_boolean_t switched;
+
SVN_ERR(svn_wc__check_wc_root(wc_root, NULL, switched, wb-db,
local_abspath, scratch_pool));
wb-result-switched |= switched;
}
+ item_rev = (wb-committed
+ ? changed_rev
+ : revision);
+
/* Added files have a revision of no interest */
- if (revision != SVN_INVALID_REVNUM)
+ if (item_rev != SVN_INVALID_REVNUM)
{
- svn_revnum_t item_rev = (wb-committed
- ? changed_rev
- : revision);
if (wb-result-min_rev == SVN_INVALID_REVNUM
|| item_rev wb-result-min_rev)
@@ -101,22 +113,27 @@
if (! wb-result-modified)
{
- svn_boolean_t text_mod;
svn_boolean_t props_mod;
SVN_ERR(svn_wc__props_modified(props_mod, wb-db, local_abspath,
scratch_pool));
+ wb-result-modified |= props_mod;
+}
+ if (! wb-result-modified)
+{
+ svn_boolean_t text_mod;
+
SVN_ERR(svn_wc__internal_text_modified_p(text_mod, wb-db,
local_abspath,
FALSE,
TRUE,
Re: [PATCH v3] Replace entries in revisision_status.c
On Thu, Mar 11, 2010 at 03:05:23AM -0500, Greg Stein wrote:
On Wed, Mar 10, 2010 at 05:51, Daniel Näslund dan...@longitudo.com wrote:
[...]
I've removed the comments in the end about sparse_checkout not detecting
all cases. The walker reaches all nodes beneath local_abspath and if we
check for absent too I can't come up with any more cases where we won't
detect sparse_checkouts. Is that assumption correct?
I'm catching the _WC_PATH_NOT_FOUND error. Before my changes the code
didn't return that error. BUT, is that the _right_ thing to do? Are we
counting on the svn_wc_.* functions to always return _WC_PATH_NOT_FOUND
error for unversioned resources and such? Since it's a new revision I
could just as well return it, right?
As long as you alter the docstring to reflect the (new) error
condition, then yes: it would be best to propagate that error to the
caller. It totally sucks where functions silently do nothing when
asked to operate on a non-existent node (where the caller should have
known better!).
Done that!
[[[
As part of WC-NG, remove some uses of svn_wc_entry_t.
* subversion/include/svn_wc.h
(svn_wc_revision_status2): Add note about us returning
SVN_ERR_WC_PATH_NOT_FOUND.
* subversion/libsvn_wc/revision_status.c
(status_baton): Rename this...
(walk_baton): .. to this.
(analyze_status): Change this to implement svn_wc__node_func_t and use
WC-NG funcs instead of information in svn_wc_entry_t.
(svn_wc_revision_status2): Use svn_wc__node_walk_children() instead of
svn_wc_walk_status() to spare us from the overhead of invoking an
editor.
]]]
My client isn't inlining the patch for a specific review, so I'll
bullet-list things again:
* sparse_checkout |= TRUE should just be sparse_checkout = TRUE
Fixed.
* the URL stuff is wonky. you may *never* get repos info during the
walk. just have the outer function recover the URL via node_get_url()
or somesuch. it certainly beats testing that thing for every node
during the walk, and is much more explicit/obvious to the reader. this
also means you can toss wb-pool
I was planning to do that in an follow-up but ok, fixed!
* check_wc_root isn't cheap, so taking a page from Bert's book, you
could do that only when switched is FALSE
Premature optimization is the root of all evil. Done!
* the logic in the outer function about getting the URL and computing
the switched flag... you could do that before the walk in order to set
-switched early on
Premature opti... Done!
I've tested the patch on the tests I know is using the code, but not a
full make check on this version yet. Waiting 'til I know there will be
no more fixes needed.
Daniel
Index: subversion/svnversion/main.c
===
--- subversion/svnversion/main.c(revision 921239)
+++ subversion/svnversion/main.c(arbetskopia)
@@ -253,18 +253,25 @@
return EXIT_SUCCESS;
}
- SVN_INT_ERR(svn_wc_revision_status2(res, wc_ctx, local_abspath,
- trail_url, committed, NULL, NULL,
- pool, pool));
+ err = svn_wc_revision_status2(res, wc_ctx, local_abspath,
+trail_url, committed, NULL, NULL,
+pool, pool);
- /* Unversioned file in versioned directory */
- if (res-min_rev == -1)
+ if (err)
{
- SVN_INT_ERR(svn_cmdline_printf(pool, _(Unversioned file%s),
- no_newline ? : \n));
- svn_pool_destroy(pool);
- return EXIT_SUCCESS;
+ /* Unversioned file in versioned directory */
+ if (err-apr_err == SVN_ERR_WC_PATH_NOT_FOUND)
+{
+ svn_error_clear(err);
+ SVN_INT_ERR(svn_cmdline_printf(pool, _(Unversioned file%s),
+ no_newline ? : \n));
+ svn_pool_destroy(pool);
+ return EXIT_SUCCESS;
+}
+ else
+ SVN_INT_ERR(err);
}
+
}
else if (kind == svn_node_none)
{
Index: subversion/include/svn_wc.h
===
--- subversion/include/svn_wc.h (revision 921239)
+++ subversion/include/svn_wc.h (arbetskopia)
@@ -7171,7 +7171,8 @@
/** Set @a *result_p to point to a new #svn_wc_revision_status_t structure
* containing a summary of the revision range and status of the working copy
* at @a local_abspath (not including externals). @a local_abspath must
- * be absolute.
+ * be absolute. Return SVN_ERR_WC_PATH_NOT_FOUND if @a local_abspath is not
+ * a working copy path.
*
* Set @a (*result_p)-min_rev and @a (*result_p)-max_rev respectively to the
* lowest and highest revision numbers in the working copy. If @a committed
Index: subversion/libsvn_wc/revision_status.c
Re: [PATCH v2] Replace entries in revisision_status.c
Hi!
Here's my new patch.
On Tue, Mar 09, 2010 at 12:52:23AM +0100, Bert Huijben wrote:
Several points:
* use svn_wc__internal_text_modified_p(); then you won't need wc_ctx
(just the db)
Done!
* read_info can return NULL for the repos_* values
I'm checking that the values are null before setting using them for
svn_uri_join().
* typo in analyze_status docstring (the typename)
Fixed!
* status_deleted nodes have no revision info; you may simply want to
test for SVN_INVALID_REVNUM, since read_info() will return that when
it is not interesting. IOW, it does the proper work for you
I'm checking that revision is not SVN_INVALID_REVNUM. As I've understood
it, that happens when we have something in WORKING, it has been deleted
or added and in that case there's no revision info for us to use. Right?
* do you need wc_root? how about just using
svn_wc__internal_path_switched() ?
I first tried to use the child_disjoint.*() func of libsvn_wc/lock.c but
it felt strange to use that code when we already have
svn_wc__check_wc_root() that checks if a node is switched. I can
separate the switch part into a separate func in a follow up and
replace the call to svn_wc__check_wc_root() with that one then. But I'm
so terribly slow at getting my patches finished that I thought it was
best to use _check_wc_root() as is for now.
* You could optimize the checking for modifications to stop checking when
you find the first change. (You are not reporting specific changes)
Done that!
I've removed the comments in the end about sparse_checkout not detecting
all cases. The walker reaches all nodes beneath local_abspath and if we
check for absent too I can't come up with any more cases where we won't
detect sparse_checkouts. Is that assumption correct?
I'm catching the _WC_PATH_NOT_FOUND error. Before my changes the code
didn't return that error. BUT, is that the _right_ thing to do? Are we
counting on the svn_wc_.* functions to always return _WC_PATH_NOT_FOUND
error for unversioned resources and such? Since it's a new revision I
could just as well return it, right?
[[[
As part of WC-NG, remove some uses of svn_wc_entry_t.
* subversion/libsvn_wc/revision_status.c
(status_baton): Rename this...
(walk_baton): .. to this.
(analyze_status): Change this to implement svn_wc__node_func_t and use
WC-NG funcs instead of information in svn_wc_entry_t.
(svn_wc_revision_status2): Use svn_wc__node_walk_children() instead of
svn_wc_walk_status() to spare us from the overhead of invoking an
editor.
]]]
Daniel
Index: subversion/libsvn_wc/revision_status.c
===
--- subversion/libsvn_wc/revision_status.c (revision 921239)
+++ subversion/libsvn_wc/revision_status.c (arbetskopia)
@@ -23,65 +23,109 @@
#include svn_wc.h
#include svn_dirent_uri.h
+#include wc_db.h
+#include wc.h
+#include props.h
#include private/svn_wc_private.h
#include svn_private_config.h
/* A baton for analyze_status(). */
-struct status_baton
+struct walk_baton
{
svn_wc_revision_status_t *result; /* where to put the result */
svn_boolean_t committed; /* examine last committed revisions */
const char *local_abspath; /* path whose URL we're looking for */
const char *wc_url;/* URL for the path whose URL we're looking for */
+ svn_wc__db_t *db;
apr_pool_t *pool; /* pool in which to store alloc-needy things */
};
-/* An svn_wc_status_func4_t callback function for analyzing status
- structures. */
+/* An svn_wc__node_found_func_t callback function for analyzing the status
+ * of nodes */
static svn_error_t *
-analyze_status(void *baton,
- const char *local_abspath,
- const svn_wc_status2_t *status,
- apr_pool_t *pool)
+analyze_status(const char *local_abspath,
+ void *baton,
+ apr_pool_t *scratch_pool)
{
- struct status_baton *sb = baton;
+ struct walk_baton *wb = baton;
+ svn_revnum_t changed_rev;
+ svn_revnum_t revision;
+ svn_depth_t depth;
+ svn_wc__db_status_t status;
+ const char *repos_root;
+ const char *repos_relpath;
+ svn_boolean_t wc_root;
+ svn_boolean_t switched;
- if (! status-entry)
+ SVN_ERR(svn_wc__db_read_info(status, NULL, revision, repos_relpath,
+ repos_root, NULL, changed_rev,
+ NULL, NULL, NULL, depth, NULL, NULL, NULL,
+ NULL, NULL, NULL, NULL, NULL, NULL,
+ NULL, NULL, NULL, NULL, wb-db,
+ local_abspath, scratch_pool, scratch_pool));
+
+ /* We need the excluded and absent paths when checking for sparse
+ * checkouts. But only for that. To collect those we're walking all the
+ * hidden nodes. */
+ if (status == svn_wc__db_status_excluded
+ || status == svn_wc__db_status_absent)
+{
+ wb-result-sparse_checkout |=
[PATCH] Replace entries in revision
Hi!
I have run the merge_tests on this patch. When grepping I only found
this code to be used there. If this looks ok, I'll run a make check on
it.
[[[
As part of WC-NG, remove some uses of svn_wc_entry_t.
* subversion/libsvn_wc/revision_status.c
(status_baton): Rename this...
(walk_baton): .. to this.
(analyze_status): Change this to implement svn_wc__node_func_t and use
WC-NG funcs instead of information in svn_wc_entry_t.
(svn_wc_revision_status2): Use svn_wc__node_walk_children() instead of
svn_wc_walk_status() to spare us from the overhead of invoking an
editor.
]]]
The entries code transformed svn_depth_unknown to svn_depth_infinity. I
check for them both when determing if we have a sparse checkout.
Bert told me that in WC-NG we check for svn_depth_exclude with the
status. Did so, hope it's right.
Bert told me that svnversion (the primary user of the revision_status
functionality as I understood it) didn't take absent or excluded items
into account so I set the show_hidden parameter of
svn_wc__node_walk_children to FALSE.
I could have moved the check for the url out of analyze_status() but I
just wanted to remove some entries and get out of there as fast as I
could. I'm still afraid of libsvn_wc!
Daniel
Index: subversion/libsvn_wc/revision_status.c
===
--- subversion/libsvn_wc/revision_status.c (revision 920439)
+++ subversion/libsvn_wc/revision_status.c (arbetskopia)
@@ -23,64 +23,89 @@
#include svn_wc.h
#include svn_dirent_uri.h
+#include wc_db.h
+#include wc.h
+#include props.h
#include private/svn_wc_private.h
#include svn_private_config.h
/* A baton for analyze_status(). */
-struct status_baton
+struct walk_baton
{
svn_wc_revision_status_t *result; /* where to put the result */
svn_boolean_t committed; /* examine last committed revisions */
const char *local_abspath; /* path whose URL we're looking for */
const char *wc_url;/* URL for the path whose URL we're looking for */
+ svn_wc_context_t *wc_ctx;
apr_pool_t *pool; /* pool in which to store alloc-needy things */
};
-/* An svn_wc_status_func4_t callback function for analyzing status
- structures. */
+/* An svn_wc__node_found_funct_t callback function for analyzing the status
+ * of nodes */
static svn_error_t *
-analyze_status(void *baton,
- const char *local_abspath,
- const svn_wc_status2_t *status,
- apr_pool_t *pool)
+analyze_status(const char *local_abspath,
+ void *baton,
+ apr_pool_t *scratch_pool)
{
- struct status_baton *sb = baton;
+ struct walk_baton *wb = baton;
+ svn_revnum_t changed_rev;
+ svn_revnum_t revision;
+ svn_depth_t depth;
+ svn_wc__db_status_t status;
+ const char *repos_root;
+ const char *repos_relpath;
+ svn_boolean_t text_mod;
+ svn_boolean_t props_mod;
+ svn_boolean_t switched;
+ svn_boolean_t wc_root;
- if (! status-entry)
-return SVN_NO_ERROR;
+ SVN_ERR(svn_wc__db_read_info(status, NULL, revision, repos_relpath,
+ repos_root, NULL, changed_rev,
+ NULL, NULL, NULL, depth, NULL, NULL, NULL,
+ NULL, NULL, NULL, NULL, NULL, NULL,
+ NULL, NULL, NULL, NULL, wb-wc_ctx-db,
+ local_abspath, scratch_pool, scratch_pool));
+ SVN_ERR(svn_wc__check_wc_root(wc_root, NULL, switched, wb-wc_ctx-db,
+local_abspath, scratch_pool));
+
+ SVN_ERR(svn_wc__props_modified(props_mod, wb-wc_ctx-db, local_abspath,
+ scratch_pool));
+
+ SVN_ERR(svn_wc_text_modified_p2(text_mod, wb-wc_ctx, local_abspath,
+ FALSE, /* force_comparison */
+ scratch_pool));
+
/* Added files have a revision of no interest */
- if (status-text_status != svn_wc_status_added)
+ if (status != svn_wc__db_status_added)
{
- svn_revnum_t item_rev = (sb-committed
- ? status-entry-cmt_rev
- : status-entry-revision);
+ svn_revnum_t item_rev = (wb-committed
+ ? changed_rev
+ : revision);
- if (sb-result-min_rev == SVN_INVALID_REVNUM
- || item_rev sb-result-min_rev)
-sb-result-min_rev = item_rev;
+ if (wb-result-min_rev == SVN_INVALID_REVNUM
+ || item_rev wb-result-min_rev)
+wb-result-min_rev = item_rev;
- if (sb-result-max_rev == SVN_INVALID_REVNUM
- || item_rev sb-result-max_rev)
-sb-result-max_rev = item_rev;
+ if (wb-result-max_rev == SVN_INVALID_REVNUM
+ || item_rev wb-result-max_rev)
+wb-result-max_rev = item_rev;
}
- if (status-entry-depth != svn_depth_exclude)
+ if (status !=
[PATCH] #3483 - Extend svn_client_upgrade() to include externals
Hi Bert!
You said on IRC that if I adjusted this patch to apply after your
changes and sent it to dev@ you would give a +1. Will you?
I only resolved a variable declaration and added the new parameters to
svn_wc_upgrade(). You added a callback for retrieving the wc root when
upgrading from 1.0. As a external is just as any other wc, I mean that I
don't have to make any special considerations regarding the wc root
fetching.
make check passed.
[[[
Fix issue #3483 - extend svn_client_upgrade() to include externals. I've
done the externals upgrading after wc upgrade is finished. In that way
no errors in the externals will affect the wc.
* subversion/libsvn_client/cleanup.c
(svn_client_upgrade): Get all svn:externals. We need the target_dir so
we have to parse the description. For each target_dir we call
svn_wc_upgrade() which will recursively upgrade the external.
* subversion/tests/cmdline/upgrade_tests.py
(upgrade_with_externals): New. Checks the format of a 1.6 wc upgraded
to wc-ng.
(test_list): Add upgrade with_externals.
* subversion/tests/cmdline/upgrade_tests_data/upgrade_with_extenals.tar.bz2
(...): New. An 1.6 wc with the same structure as those used in
externals_tests.py.
Patch by: Daniel Näslund daniel{_AT_}longitudo.com
]]]
Index: subversion/tests/cmdline/upgrade_tests.py
===
--- subversion/tests/cmdline/upgrade_tests.py (revision 919571)
+++ subversion/tests/cmdline/upgrade_tests.py (arbetskopia)
@@ -141,7 +141,24 @@
expected_status = svntest.actions.get_virginal_state(sbox.wc_dir, 1)
run_and_verify_status_no_server(sbox.wc_dir, expected_status)
+def upgrade_with_externals(sbox):
+ upgrade with externals
+
+ # Create wc from tarfile, uses the same structure of the wc as the tests
+ # in externals_tests.py.
+ replace_sbox_with_tarfile(sbox, 'upgrade_with_externals.tar.bz2')
+ # Attempt to use the working copy, this should give an error
+ expected_stderr = wc_is_too_old_regex
+ svntest.actions.run_and_verify_svn(None, None, expected_stderr,
+ 'info', sbox.wc_dir)
+ # Now upgrade the working copy
+ svntest.actions.run_and_verify_svn(None, None, [],
+ 'upgrade', sbox.wc_dir)
+
+ # Actually check the format number of the upgraded working copy
+ check_format(sbox, get_current_format())
+
def upgrade_1_5_body(sbox, subcommand):
replace_sbox_with_tarfile(sbox, 'upgrade_1_5.tar.bz2')
@@ -328,6 +345,7 @@
# list all tests here, starting with None:
test_list = [ None,
basic_upgrade,
+ upgrade_with_externals,
upgrade_1_5,
update_1_5,
logs_left_1_5,
Index:
subversion/tests/cmdline/upgrade_tests_data/upgrade_with_externals.tar.bz2
===
Kan inte visa: filen markerad som binär.
svn:mime-type = application/octet-stream
Egenskapsändringar för:
subversion/tests/cmdline/upgrade_tests_data/upgrade_with_externals.tar.bz2
___
Added: svn:mime-type
+ application/octet-stream
Index: subversion/libsvn_client/cleanup.c
===
--- subversion/libsvn_client/cleanup.c (revision 919571)
+++ subversion/libsvn_client/cleanup.c (arbetskopia)
@@ -34,8 +34,11 @@
#include svn_dirent_uri.h
#include svn_pools.h
#include client.h
+#include svn_pools.h
+#include svn_props.h
#include svn_private_config.h
+#include private/svn_wc_private.h
/*** Code. ***/
@@ -110,6 +113,10 @@
apr_pool_t *scratch_pool)
{
const char *local_abspath;
+ apr_hash_t *externals;
+ apr_hash_index_t *hi;
+ apr_pool_t *iterpool;
+ svn_opt_revision_t rev = {svn_opt_revision_unspecified, {0}};
struct repos_info_baton info_baton;
info_baton.pool = scratch_pool;
info_baton.ctx = ctx;
@@ -123,5 +130,79 @@
ctx-notify_func2, ctx-notify_baton2,
scratch_pool));
+ /* Now it's time to upgrade the externals too. We do it after the wc
+ upgrade to avoid that errors in the externals causes the wc upgrade to
+ fail. Thanks to caching the performance penalty of walking the wc a
+ second time shouldn't be too severe */
+ SVN_ERR(svn_client_propget3(externals, SVN_PROP_EXTERNALS, path, rev,
+ rev, NULL, svn_depth_infinity, NULL, ctx,
+ scratch_pool));
+
+ iterpool = svn_pool_create(scratch_pool);
+
+ for (hi = apr_hash_first(scratch_pool, externals); hi;
+ hi = apr_hash_next(hi))
+{
+ const char *key;
+ int i;
+ apr_ssize_t klen;
+ svn_string_t *external_desc;
+ apr_array_header_t *externals_p;
+
+ svn_pool_clear(iterpool);
+ externals_p = apr_array_make(iterpool, 1
[PATCH] For 'svn patch', fix bug when reading patches without index lines
Hi!
I have my own branch, and I was told that I could just commit there. But
it's _scary_. I'll ask for permission first.
[[[
Fix bug with 'svn patch' not recognizing diff headers when parsing
patches without the 'Index' line and the '==' line. The old code
just assumed that the first line after the hunk would not be a '-'. But
it can be so we must handle it.
* subversion/libsvn_diff/parse-diff.c
(parse_next_hunk): Check that we have not read all the lines, that the
hunk header said, the hunk should consist of. We need to check for
both nr of modified and original lines since we can do a reverse
parsing. That means treating '+' as '-' and the other way around.
* subversion/tests/cmdline/patch_tests.py
(patch_no_index_line): New.
(test_list): Add the new test.
]]]
Index: subversion/libsvn_diff/parse-diff.c
===
--- subversion/libsvn_diff/parse-diff.c (revision 919442)
+++ subversion/libsvn_diff/parse-diff.c (arbetskopia)
@@ -283,6 +283,7 @@
svn_stream_t *original_text;
svn_stream_t *modified_text;
svn_linenum_t original_lines;
+ svn_linenum_t modified_lines;
svn_linenum_t leading_context;
svn_linenum_t trailing_context;
svn_boolean_t changed_line_seen;
@@ -360,12 +361,13 @@
{
hunk_seen = TRUE;
original_lines--;
+ modified_lines--;
if (changed_line_seen)
trailing_context++;
else
leading_context++;
}
- else if (c == add || c == del)
+ else if (original_lines 0 c == del)
{
hunk_seen = TRUE;
changed_line_seen = TRUE;
@@ -375,9 +377,20 @@
if (trailing_context 0)
trailing_context = 0;
- if (original_lines 0 c == del)
-original_lines--;
+ original_lines--;
}
+ else if (modified_lines 0 c == add)
+{
+ hunk_seen = TRUE;
+ changed_line_seen = TRUE;
+
+ /* A hunk may have context in the middle. We only want the
+ last lines of context. */
+ if (trailing_context 0)
+trailing_context = 0;
+
+ modified_lines--;
+}
else
{
in_hunk = FALSE;
@@ -397,7 +410,10 @@
in_hunk = parse_hunk_header(line-data, *hunk, reverse,
iterpool);
if (in_hunk)
-original_lines = (*hunk)-original_length;
+{
+ original_lines = (*hunk)-original_length;
+ modified_lines = (*hunk)-modified_length;
+}
}
else if (starts_with(line-data, minus))
/* This could be a header of another patch. Bail out. */
Index: subversion/tests/cmdline/patch_tests.py
===
--- subversion/tests/cmdline/patch_tests.py (revision 919442)
+++ subversion/tests/cmdline/patch_tests.py (arbetskopia)
@@ -836,6 +836,92 @@
1, # dry-run
'-p1')
+def patch_no_index_line(sbox):
+ patch with no index lines
+
+ sbox.build()
+ wc_dir = sbox.wc_dir
+
+ patch_file_path =
tempfile.mkstemp(dir=os.path.abspath(svntest.main.temp_dir))[1]
+ gamma_path = os.path.join(wc_dir, 'A', 'D', 'gamma')
+ iota_path = os.path.join(wc_dir, 'iota')
+
+ gamma_contents = [
+\n,
+Another line before\n,
+A third line before\n,
+This is the file 'gamma'.\n,
+A line after\n,
+Another line after\n,
+A third line after\n,
+ ]
+
+ svntest.main.file_write(gamma_path, ''.join(gamma_contents))
+ expected_output = svntest.wc.State(wc_dir, {
+'A/D/gamma' : Item(verb='Sending'),
+})
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
+ expected_status.tweak('A/D/gamma', wc_rev=2)
+ svntest.actions.run_and_verify_commit(wc_dir, expected_output,
+expected_status, None, wc_dir)
+ unidiff_patch = [
+--- A/D/gamma\t(revision 1)\n,
++++ A/D/gamma\t(working copy)\n,
+@@ -1,7 +1,7 @@\n,
+ \n,
+ Another line before\n,
+ A third line before\n,
+-This is the file 'gamma'.\n,
++It is the file 'gamma'.\n,
+ A line after\n,
+ Another line after\n,
+ A third line after\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,
+ ]
+
+ svntest.main.file_write(patch_file_path, ''.join(unidiff_patch))
+
+ gamma_contents = [
+\n,
+Another line before\n,
+A third line before\n,
+It is the file 'gamma'.\n,
+A line after\n,
+Another line after\n,
+A third line after\n,
+ ]
+ iota_contents
[PATCH v2] 'svn patch' should remove empty dir
Hi!
Here's my revised patch. I've commented on the changes inline.
One strange thing: If you compile this you'll get a compiler warning
about this variable not beeing used:
1354 apr_array_header_t *deleted_targets;
But if I remove it, no targets will be condensed. It's really silly. Can
someone explain why a variable that isn't used must be declared in order
for my code to work. What is the simple logical explanation?
On Mon, Mar 01, 2010 at 04:10:09PM +0100, Stefan Sperling wrote:
On Sat, Feb 27, 2010 at 09:09:52PM +0100, Daniel Näslund wrote:
Index: subversion/libsvn_client/patch.c
===
--- subversion/libsvn_client/patch.c(revision 916918)
+++ subversion/libsvn_client/patch.c(arbetskopia)
[...]
+/* Check if PARENT_DIR_ABSPATH has any versioned or unversioned children if
+ * we ignore the ones in TARGETS_TO_BE_DELETED. Return the answer in
+ * EMPTY. */
+static svn_error_t *
+is_dir_empty(svn_boolean_t *empty, const char *parent_dir_abspath,
+ svn_client_ctx_t *ctx,
+ apr_array_header_t *targets_to_be_deleted,
+ apr_pool_t *result_pool, apr_pool_t *scratch_pool)
+{
+ struct status_baton btn;
+ svn_opt_revision_t revision;
+ int i;
+
+ btn.existing_targets = apr_array_make(scratch_pool, 0,
+sizeof(patch_target_t *));
+ btn.parent_path = parent_dir_abspath;
+ btn.result_pool = scratch_pool;
+ revision.kind = svn_opt_revision_unspecified;
+
+ SVN_ERR(svn_client_status5(NULL, parent_dir_abspath, revision,
+ find_existing_children, btn,
+ svn_depth_immediates, TRUE, FALSE, TRUE,
+ FALSE, NULL, ctx, scratch_pool));
Not sure if svn_client_status5() is really what we want here.
Is there a public or semi-private libsvn_wc function we could
use instead? One that doesn't need a client context?
I used svn_wc_walk_status() instead. It does not handle externals but
treats them as unversioned. No problemos for us, we just want to find
out if anything exists in a dir.
+ /* Do we delete all targets? */
+ for (i = 0; i btn.existing_targets-nelts; i++)
+{
+ int j;
+ const char *found = APR_ARRAY_IDX(btn.existing_targets, i, const
char *);
+ svn_boolean_t deleted = FALSE;
+
+ for (j = 0; j targets_to_be_deleted-nelts; j++)
+{
+ patch_target_t *to_be_del;
+ to_be_del = APR_ARRAY_IDX(targets_to_be_deleted, j,
patch_target_t *);
+ if (! strcmp(found, to_be_del-abs_path))
+deleted = TRUE;
Are you missing a 'break' here?
Added it. Saves us some iterations.
+}
+ if (! deleted)
+{
+ *empty = FALSE;
+ break;
+}
+}
+
+ return SVN_NO_ERROR;
+}
+
+/* Add TARGET to the array of targets keyed by their parent dir in
+ * TARGETS_TO_BE_DELETED. If there is no array for the parent dir a new one
+ * is created. All allocations are done in RESULT_POOL. */
+static svn_error_t *
+add_target_to_hash_keyed_by_parent_dir(apr_hash_t *targets_to_be_deleted,
+ patch_target_t *target,
+ apr_pool_t *result_pool)
+{
+ apr_array_header_t * deleted_targets_in_dir;
+
+ /* We're using the abs_path of the target. The abs_path is not
+ * present if the path is a symlink pointing outside the wc but we
+ * know, that's not the case. */
The comma should either be removed, or moved: ... wc, but we know
Fixed.
+ const char *dirname = svn_dirent_dirname(target-abs_path,
+ result_pool);
+
+ deleted_targets_in_dir = apr_hash_get(targets_to_be_deleted,
+dirname, APR_HASH_KEY_STRING);
+
+ if (deleted_targets_in_dir)
+{
+ APR_ARRAY_PUSH(deleted_targets_in_dir, patch_target_t *) = target;
+
+ apr_hash_set(targets_to_be_deleted, dirname,
+ APR_HASH_KEY_STRING, deleted_targets_in_dir);
+}
+ else
+{
+ apr_array_header_t *new_array;
+
+ new_array = apr_array_make(result_pool, 0, sizeof(patch_target_t *));
+ APR_ARRAY_PUSH(new_array, patch_target_t *) = target;
+ apr_hash_set(targets_to_be_deleted,
+ dirname, APR_HASH_KEY_STRING, new_array);
+}
+ return SVN_NO_ERROR;
+}
+
+/* Compare A and B and return an integer greater than, equal to, or less
+ * than 0, according to whether A has less subdirs, just as many or more
+ * subdir than B. */
+static int
+sort_compare_nr_of_path_elements(const svn_sort__item_t *a,
+const svn_sort__item_t *b)
Indentation is off above.
Fixed.
+ const char *astr
Re: Will subversion participate in Google Summer of Code 2010?
On Tue, Feb 23, 2010 at 08:07:05AM -0600, Hyrum K. Wright wrote: On Sun, Feb 21, 2010 at 5:11 AM, Daniel Näslunddan...@longitudo.com wrote: Google Summer of Code is on for 2010 [1]. Is there any interest in participating this year? If so, I've found that the mentoring application deadline is 12 march. [2] Given the short amount of time to deadline (18 days) and the absence of discussions on fitting tasks for a GSoC project I assume the Subversion community is not interrested in participating this year. Too bad if that's the case! We haven't made a concrete decision one way or another, so don't be disappointed quite yet. It actually helps to know that people are interested in participating, 'cause that will motivate us to participate. Oki. Since I'm interrested in participating I'll try some more suggestions on fitting work for a GSoC student. Here's my ideas: 1) svn patch should use git diff format svn patch is starting to get stable. It would be a nice thing to make it able to handle the git diff format, and of course make 'svn diff' able to produce git diffs. In the first place I'm considering to make it able to handle tree changes but who knows, maybe it's possible to make it able to deal with svn propty diffs too? I'm intending to work on the tree changes part during spring but there may be enough work to last during the summer too. In 'notes/svnpatch/svnpatch-git.txt' we have a summary of how the git diff format works. 2) Subversion should provide merge tools for dealing with xml or latex --- XML and LaTeX both is not purely linebased. One item may span multiple rows and using the usual merge algorithm often (I say often, though I've never tried to merge xml or latex files) gives us the wrong result. Since atleast xml-files are very common I'm suspecting there is a use case for a merge tool. 3) svn patch queues Ok, maybe it needs a stable WC-NG but we're working hard on that and by summer there's a good chance we're there. I would really like to have this one. Bert digged up the old tasks list for me: http://svn.apache.org/repos/asf/subversion/branches/1.6.x/www/tasks.html I checked the issues the tasks refer to and most seems to be open. Both my first and second suggestion touches on things proposed on that page. It's hard to come up with good tasks! All the tasks in the links sounds fun to me. Let's hope we think it's a good idea to participate. 12 days to the deadline. *dannas is poking gently :) Daniel
[PATCH] 'svn patch' should remove empty dir
Hi!
Puh, 363 lines added. That's the biggest patch I've written so far. Sad
to think that once we have implemented git diff format this code will be
obsolute :).
Reviews welcome!
[[[
Make 'svn patch' able to remove dirs beeing empty after patching, taken
into account both versioned and unversioned files and dirs.
* subversion/tests/cmdline/patch_tests.py
(patch_remove_empty_dir): New.
(test_list): Add new test.
* subversion/libsvn_client/patch.c
(status_baton): New.
(find_existing_children, is_dir_empty,
add_target_to_hash_keyed_by_parent_dir, is_dir_empty,
sort_compare_nr_of_path_elements,
maybe_condense_deleted_targets): New functions
(apply_patches): Call maybe_condense_deleted_targets(). Check if a
target has a patch associated with it before calling
svn_diff__close_patch() since we're creating targets to delete dirs
and those have no associated patch.
Patch by: Daniel Näslund daniel{_AT_}longitudo.com
Review by: julianf, stsp, neels
]]]
What target fields must be set for the parent dir target? I've just set
deleted, abs_path and deleted. May be a bit hard to maintain. Would be
better with a central initialize_basic_target() func.
In find_existing_children() I've said that if a path has text_status
svn_wc_status{none,deleted} or is the name of the parent_dir we'll
ignore it, all else states means that we have a file or dir at the path.
I haven't taken file externals into account. If a patch says that a
target should be removed I'm assuming the user knows what they're doing.
Daniel
Index: subversion/tests/cmdline/patch_tests.py
===
--- subversion/tests/cmdline/patch_tests.py (revision 916918)
+++ subversion/tests/cmdline/patch_tests.py (arbetskopia)
@@ -911,7 +911,108 @@
None, # expected err
1, # check-props
1) # dry-run
+def patch_remove_empty_dir(sbox):
+ delete the dir if all children is deleted
+ sbox.build()
+ wc_dir = sbox.wc_dir
+
+ patch_file_path =
tempfile.mkstemp(dir=os.path.abspath(svntest.main.temp_dir))[1]
+
+ # Contents of B:
+ # A/B/lamba
+ # A/B/F
+ # A/B/E/{alpha,beta}
+ # Before patching we've deleted F, which means that B is empty after
patching and
+ # should be removed.
+ #
+ # Contents of H:
+ # A/D/H/{chi,psi,omega}
+ # Before patching, chi has been removed by a non-svn operation which means
it has
+ # status missing. The patch deletes the other two files but should not
delete H.
+
+ unidiff_patch = [
+Index: psi\n,
+===\n,
+--- A/D/H/psi\t(revision 0)\n,
++++ A/D/H/psi\t(revision 0)\n,
+@@ -1 +0,0 @@\n,
+-This is the file 'psi'.\n,
+Index: omega\n,
+===\n,
+--- A/D/H/omega\t(revision 0)\n,
++++ A/D/H/omega\t(revision 0)\n,
+@@ -1 +0,0 @@\n,
+-This is the file 'omega'.\n,
+Index: lambda\n,
+===\n,
+--- A/B/lambda\t(revision 0)\n,
++++ A/B/lambda\t(revision 0)\n,
+@@ -1 +0,0 @@\n,
+-This is the file 'lambda'.\n,
+Index: alpha\n,
+===\n,
+--- A/B/E/alpha\t(revision 0)\n,
++++ A/B/E/alpha\t(revision 0)\n,
+@@ -1 +0,0 @@\n,
+-This is the file 'alpha'.\n,
+Index: beta\n,
+===\n,
+--- A/B/E/beta\t(revision 0)\n,
++++ A/B/E/beta\t(revision 0)\n,
+@@ -1 +0,0 @@\n,
+-This is the file 'beta'.\n,
+ ]
+
+ svntest.main.file_write(patch_file_path, ''.join(unidiff_patch))
+
+ F_path = os.path.join(wc_dir, 'A', 'B', 'F')
+ svntest.actions.run_and_verify_svn(Deleting F failed, None, [],
+ 'rm', F_path)
+ svntest.actions.run_and_verify_svn(Update failed, None, [],
+ 'up', wc_dir)
+
+ # We should be able to handle one path beeing missing.
+ os.remove(os.path.join(wc_dir, 'A', 'D', 'H', 'chi'))
+
+ expected_output = [
+'D %s\n' % os.path.join(wc_dir, 'A', 'B'),
+'D %s\n' % os.path.join(wc_dir, 'A', 'D', 'H', 'psi'),
+'D %s\n' % os.path.join(wc_dir, 'A', 'D', 'H', 'omega'),
+ ]
+
+ expected_disk = svntest.main.greek_state.copy()
+ expected_disk.remove('A/D/H/chi')
+ expected_disk.remove('A/D/H/psi')
+ expected_disk.remove('A/D/H/omega')
+ expected_disk.remove('A/B/lambda')
+ expected_disk.remove('A/B/E/alpha')
+ expected_disk.remove('A/B/E/beta')
+
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
+ expected_status.add({'A/D/H/chi' : Item(status='! ', wc_rev=1)})
+ expected_status.add({'A/D/H/omega' : Item(status='D ', wc_rev=1)})
+ expected_status.add({'A/D/H/psi
Re: Why non matching eols in patch and target. (svn patch related)
On Tue, Feb 02, 2010 at 09:11:52PM +0100, Stefan Sperling wrote:
On Tue, Feb 02, 2010 at 08:48:43PM +0100, Daniel Näslund wrote:
Hi Stefan!
In match_hunk() we try to match lines from the context of the patch with
lines in the target. Earlier, in init_patch_target() we detect the eol
of the target and open streams to read from the target and to write the
patched result. Those streams does translation of keywords and eols.
In match_hunk we read a line from original_text and translate it. But we
don't get any translation of the eols in hunk_line_translated.
svn patch only repairs EOLs if the svn:eol-style enforces a fixed
value (such as CR or CRLF). Try setting svn:eol-style to 'CRLF' on
the patch target. Then you'll see dos-style newlines in the patched result.
Admittedly, we may want to repair EOLs in more scenarios (such as
eol-style = native).
I have a test where the target uses '\r\n' and the patch uses '\r' . The
eols are consistent within each file but we get a failure saying:
[[[
subversion/svn/patch-cmd.c:81: (apr_err=135000)
subversion/libsvn_client/patch.c:1463: (apr_err=135000)
subversion/libsvn_client/patch.c:1410: (apr_err=135000)
subversion/libsvn_client/patch.c:1100: (apr_err=135000)
subversion/libsvn_client/patch.c:830: (apr_err=135000)
subversion/libsvn_client/patch.c:799: (apr_err=135000)
subversion/libsvn_subr/subst.c:876: (apr_err=135000)
subversion/libsvn_subr/subst.c:643: (apr_err=135000)
svn: Inconsistent line ending style
]]]
What to do?
[ ] Write documentation saying we can only repair eols on targets with a
svn:eol-style prop.
[ ] Wrap the error for a better error message perhaps saying 'patch and
target uses different eol' or similar.
[ ] Adjust the code to allow for repairing eols even when there is no
svn:eol-style prop. I would go for this one if it's doable.
Before I throw myself to the mercy of the eol-translating code, I ask
you if there is something you know of hindering us from repairing eols
where there is no prop set?
In init_patch_target() we decide how the eol translation of
target-patched will be done:
[[[
eol_style_val = apr_hash_get(props, SVN_PROP_EOL_STYLE,
APR_HASH_KEY_STRING);
if (eol_style_val)
{
svn_subst_eol_style_from_value(target-eol_style,
target-eol_str,
eol_style_val-data);
}
else
{
/* Just use the first EOL sequence we can find in the file. */
SVN_ERR(svn_eol__detect_file_eol(target-eol_str,
target-file, scratch_pool));
/* But don't enforce any particular EOL-style. */
target-eol_style = svn_subst_eol_style_none;
}
[...]
target-patched = svn_subst_stream_translated(
target-patched_raw,
target-eol_str,
target-eol_style ==
svn_subst_eol_style_fixed,
target-keywords, TRUE, result_pool);
]]]
You decided to not enforce any particular eol-style if we don't have a
prop. The eol_style is used for the cases where there is an eol-style
prop. Fair enough. But couldn't we do some hack where we always say that
the eol-style is fixed and use whatever eol we find in the target? I did
a quick try and got a lot of rejected hunks...
What can go wrong with this approach?
-
* We falsely label targets with inconsistent eols to be of type
eol_style_fixed. But after patching the eols will be consistent and
that's a good thing.
* [Placeholder for more stuff that can go wrong]
Or we could set the style to svn_subst_eol_style_native. We will end up
with native newlines in the target then, right?
What say you?
Daniel
Re: Why non matching eols in patch and target. (svn patch related)
On Tue, Feb 23, 2010 at 09:27:34PM +0100, Stefan Sperling wrote:
On Tue, Feb 23, 2010 at 08:57:26PM +0100, Daniel Näslund wrote:
On Tue, Feb 02, 2010 at 09:11:52PM +0100, Stefan Sperling wrote:
On Tue, Feb 02, 2010 at 08:48:43PM +0100, Daniel Näslund wrote:
Hi Stefan!
In match_hunk() we try to match lines from the context of the patch with
lines in the target. Earlier, in init_patch_target() we detect the eol
of the target and open streams to read from the target and to write the
patched result. Those streams does translation of keywords and eols.
In match_hunk we read a line from original_text and translate it. But we
don't get any translation of the eols in hunk_line_translated.
svn patch only repairs EOLs if the svn:eol-style enforces a fixed
value (such as CR or CRLF). Try setting svn:eol-style to 'CRLF' on
the patch target. Then you'll see dos-style newlines in the patched
result.
Admittedly, we may want to repair EOLs in more scenarios (such as
eol-style = native).
I have a test where the target uses '\r\n' and the patch uses '\r' . The
eols are consistent within each file but we get a failure saying:
[[[
subversion/svn/patch-cmd.c:81: (apr_err=135000)
subversion/libsvn_client/patch.c:1463: (apr_err=135000)
subversion/libsvn_client/patch.c:1410: (apr_err=135000)
subversion/libsvn_client/patch.c:1100: (apr_err=135000)
subversion/libsvn_client/patch.c:830: (apr_err=135000)
subversion/libsvn_client/patch.c:799: (apr_err=135000)
subversion/libsvn_subr/subst.c:876: (apr_err=135000)
subversion/libsvn_subr/subst.c:643: (apr_err=135000)
svn: Inconsistent line ending style
]]]
Where and how?
It happens when we've written all our hunks and we want to copy the
remaining lines. Here's a piece of apply_one_patch(). It's the call to
copy_lines_to_target() that throws the error.
[[[
/* Apply or reject hunks. */
for (i = 0; i target-hunks-nelts; i++)
{
hunk_info_t *hi;
svn_pool_clear(iterpool);
hi = APR_ARRAY_IDX(target-hunks, i, hunk_info_t *);
if (hi-rejected)
SVN_ERR(reject_hunk(target, hi, iterpool));
else
SVN_ERR(apply_hunk(target, hi, iterpool));
}
svn_pool_destroy(iterpool);
if (target-kind == svn_node_file)
{
/* Copy any remaining lines to target. */
SVN_ERR(copy_lines_to_target(target, 0, scratch_pool));
if (! target-eof)
{
/* We could not copy the entire target file to the temporary file,
* and would truncate the target if we copied the temporary file
* on top of it. Skip this target. */
target-skipped = TRUE;
}
}
]]]
The test has one line after the last hunk. If I remove that line, the
test passes.
Daniel
Re: Will subversion participate in Google Summer of Code 2010?
On Sun, Feb 21, 2010 at 07:28:13AM -0800, Justin Erenkrantz wrote: On Sun, Feb 21, 2010 at 5:11 AM, Daniel Näslund dan...@longitudo.com wrote: Hi! Google Summer of Code is on for 2010 [1]. Is there any interest in participating this year? If so, I've found that the mentoring application deadline is 12 march. [2] Since we're part of ASF, perhaps an application should go through them? [3] Yes. There's an ASF committee called comdev (http://community.apache.org/gsoc.html) that manages our participation in GSoC. They will do the application on the foundation's behalf and post info to all PMCs about how to participate on the ASF end if a project is interested. ComDev will also manage the ranking process - when you have 70+ projects interested in participating even when you have 30-40 slots, the review process can get a bit heated internally. (If you're interested in tracking this, you might want to join d...@community.apache.org.) Ok. Given the short amount of time to deadline (18 days) and the absence of discussions on fitting tasks for a GSoC project I assume the Subversion community is not interrested in participating this year. Too bad if that's the case! cheers, Daniel
Will subversion participate in Google Summer of Code 2010?
Hi! Google Summer of Code is on for 2010 [1]. Is there any interest in participating this year? If so, I've found that the mentoring application deadline is 12 march. [2] Since we're part of ASF, perhaps an application should go through them? [3] I've tried locating the GSoC ideas pages for 2009 but it seems to have got lost in the move to apache.org. My own suggestions (given what I know of subversion so far) would be: Implementing a tree conflict resolver --- This task probably should have editor-v2 available. Perhaps the task could be defined as doing some vital parts of editor-v2 and continue with a TC resolver once finished with the editor. And yes, I have no notion whatsoever about the time frame required for the editor-work. Replacing externals with something similar of ClearCases views I overheard a conversation between Senthil and Bert on IRC where Bert mentioned the views concept of ClearCase. [4] I want something like externals but without the edge cases of file externals and with the externals nodes being more like regular nodes. Patch queues -- Some kind of quilt functionality for local storage of work-in-progress. Probably needs to have most the WC-NG parts stable. The views concept and TC resolver was in the GSoC 2009 ideas page IIRC. If the subversion community is interested in participating, I'm looking forward to compete in a friendly fashion for one of the Student Developer positions. cheers, Daniel [1] http://code.google.com/soc/ [2] http://socghop.appspot.com/document/show/gsoc_program/google/gsoc2010/faqs#timeline [3] http://community.apache.org/gsoc.html [4] http://techpubs.sgi.com/library/dynaweb_docs/0620/SGI_Developer/books/ClrC_CG/sgi_html/ch04.html
Re: [PATCH] v2 #3483 - Extend svn_client_upgrade() to include externals
Ping!
This patch has not been reviewed!
On Tue, Dec 29, 2009 at 07:44:49PM +0100, Daniel Näslund wrote:
Hi!
[[[
Fix issue #3483 - extend svn_client_upgrade() to include externals. I've
done the externals upgrading after wc upgrade is finished. In that way
no errors in the externals will affect the wc.
* subversion/libsvn_client/cleanup.c
(svn_client_upgrade): Get all svn:externals. We need the target_dir so
we have to parse the description. For each target_dir we call
svn_wc_upgrade() which will recursively upgrade the external.
* subversion/tests/cmdline/upgrade_tests.py
(upgrade_with_externals): New. Checks the format of a 1.6 wc upgraded
to wc-ng.
(test_list): Add upgrade with_externals.
* subversion/tests/cmdline/upgrade_tests_data/upgrade_with_extenals.tar.bz2
(...): New. An 1.6 wc with the same structure as those used in
externals_tests.py.
Patch by: Daniel Näslund daniel{_AT_}longitudo.com
]]]
A big thank you to Bert for putting up with all my questions on IRC.
My previous patch didn't handle relative externals all that well. And it
failed to upgrade file externals too. I've added a testcase to this
patch to prove that this one does things right.
BUT, the testcase does not check the statustree of the wc after the
upgrade. I didn't understand how to set up a statustree to check
against.
Daniel
Index: subversion/tests/cmdline/upgrade_tests.py
===
--- subversion/tests/cmdline/upgrade_tests.py (revision 894319)
+++ subversion/tests/cmdline/upgrade_tests.py (arbetskopia)
@@ -139,7 +139,24 @@
expected_status = svntest.actions.get_virginal_state(sbox.wc_dir, 1)
run_and_verify_status_no_server(sbox.wc_dir, expected_status)
+def upgrade_with_externals(sbox):
+ upgrade with externals
+
+ # Create wc from tarfile, uses the same structure of the wc as the tests
+ # in externals_tests.py.
+ replace_sbox_with_tarfile(sbox, 'upgrade_with_externals.tar.bz2')
+ # Attempt to use the working copy, this should give an error
+ expected_stderr = wc_is_too_old_regex
+ svntest.actions.run_and_verify_svn(None, None, expected_stderr,
+ 'info', sbox.wc_dir)
+ # Now upgrade the working copy
+ svntest.actions.run_and_verify_svn(None, None, [],
+ 'upgrade', sbox.wc_dir)
+
+ # Actually check the format number of the upgraded working copy
+ check_format(sbox, get_current_format())
+
def upgrade_1_5_body(sbox, subcommand):
replace_sbox_with_tarfile(sbox, 'upgrade_1_5.tar.bz2')
@@ -216,6 +233,7 @@
# list all tests here, starting with None:
test_list = [ None,
basic_upgrade,
+ upgrade_with_externals,
upgrade_1_5,
XFail(update_1_5),
logs_left_1_5,
Index:
subversion/tests/cmdline/upgrade_tests_data/upgrade_with_externals.tar.bz2
===
Kan inte visa: filen markerad som binär.
svn:mime-type = application/octet-stream
Egenskapsändringar för:
subversion/tests/cmdline/upgrade_tests_data/upgrade_with_externals.tar.bz2
___
Added: svn:mime-type
+ application/octet-stream
Index: subversion/libsvn_client/cleanup.c
===
--- subversion/libsvn_client/cleanup.c(revision 894319)
+++ subversion/libsvn_client/cleanup.c(arbetskopia)
@@ -33,8 +33,11 @@
#include svn_config.h
#include svn_dirent_uri.h
#include client.h
+#include svn_pools.h
+#include svn_props.h
#include svn_private_config.h
+#include private/svn_wc_private.h
/*** Code. ***/
@@ -62,6 +65,10 @@
apr_pool_t *scratch_pool)
{
const char *local_abspath;
+ apr_hash_t *externals;
+ apr_hash_index_t *hi;
+ apr_pool_t *iterpool;
+ svn_opt_revision_t rev = {svn_opt_revision_unspecified, {0}};
SVN_ERR(svn_dirent_get_absolute(local_abspath, path, scratch_pool));
SVN_ERR(svn_wc_upgrade(ctx-wc_ctx, local_abspath,
@@ -69,5 +76,78 @@
ctx-notify_func2, ctx-notify_baton2,
scratch_pool));
+ /* Now it's time to upgrade the externals too. We do it after the wc
+ upgrade to avoid that errors in the externals causes the wc upgrade to
+ fail. Thanks to caching the performance penalty of walking the wc a
+ second time shouldn't be too severe */
+ SVN_ERR(svn_client_propget3(externals, SVN_PROP_EXTERNALS, path, rev,
+ rev, NULL, svn_depth_infinity, NULL, ctx,
+ scratch_pool));
+
+ iterpool = svn_pool_create(scratch_pool);
+
+ for (hi = apr_hash_first(scratch_pool, externals); hi;
+ hi = apr_hash_next(hi))
+{
+ const
[WIP] 'svn patch' should remove empty dirs
Hi Stefan!
This patch is growing and growing and I'm worried that I'm heading off
in the wrong direction. Perhaps you can skim through the patch and tell
me if this is the right approach. You've said so earlier but all these
lines of code makes me nervous. I like small fixes, and this is starting
to look bloated.
A preliminary log msg (it's a WIP)
[[[
Make 'svn patch' able to remove empty.
* subversion/tests/cmdline/patch_tests.py
(patch_remove_empty_dir): New.
* subversion/libsvn_client/patch.c
(is_dir_empty): Checks if a dir is empty, taking missing nodes into
account.
(sort_compare_paths_by_depth): Compare func to use with
svn_sort__hash(). Compares nr of '/' separators. Since the paths are
canonicalized, it should work just fine.
(collect_deleted_targets): If there is targets to be deleted, collect
those targets and the targets no involving deletes in two arrays.
TODO: We should condense the list of deleted dirs and be able to
only delete the top-most parent.
(apply_patches): TODO: We should remove the targets in
delete_all_of_this. Or perhaps concatenate the two list into one?
]]]
Just a quick.
[ ] Yes, using APR forces you to a lot of boilerplate.
[ ] Hey there, you don't need to do all that, just ...
Hope I'm not asking to much,
Daniel
Index: subversion/tests/cmdline/patch_tests.py
===
--- subversion/tests/cmdline/patch_tests.py (revision 912338)
+++ subversion/tests/cmdline/patch_tests.py (arbetskopia)
@@ -911,7 +911,63 @@
None, # expected err
1, # check-props
1) # dry-run
+def patch_remove_empty_dir(sbox):
+ delete the dir if all children is deleted
+ sbox.build()
+ wc_dir = sbox.wc_dir
+
+ patch_file_path =
tempfile.mkstemp(dir=os.path.abspath(svntest.main.temp_dir))[1]
+
+ unidiff_patch = [
+Index: psi\n,
+===\n,
+--- A/D/H/psi\t(revision 0)\n,
++++ A/D/H/psi\t(revision 0)\n,
+@@ -1 +0,0 @@\n,
+-This is the file 'psi'.\n,
+Index: omega\n,
+===\n,
+--- A/D/H/omega\t(revision 0)\n,
++++ A/D/H/omega\t(revision 0)\n,
+@@ -1 +0,0 @@\n,
+-This is the file 'omega'.\n,
+ ]
+
+ svntest.main.file_write(patch_file_path, ''.join(unidiff_patch))
+
+ # We should be able to handle one path beeing missing.
+ os.remove(os.path.join(wc_dir, 'A', 'D', 'H', 'chi'))
+
+
+ expected_output = [
+'D %s\n' % os.path.join(wc_dir, 'A', 'D', 'H', 'chi'),
+'D %s\n' % os.path.join(wc_dir, 'A', 'D', 'H', 'psi'),
+'D %s\n' % os.path.join(wc_dir, 'A', 'D', 'H', 'omega'),
+ ]
+
+ expected_disk = svntest.main.greek_state.copy()
+ expected_disk.remove('A/D/H/chi')
+ expected_disk.remove('A/D/H/psi')
+ expected_disk.remove('A/D/H/omega')
+ expected_disk.remove('A/D/H')
+
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
+ expected_status.add({'A/D/H' : Item(status='D ', wc_rev=1)})
+
+ 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
+
+
def patch_reject(sbox):
apply a patch which is rejected
@@ -1363,6 +1419,7 @@
patch_chopped_leading_spaces,
patch_strip1,
patch_add_new_dir,
+ patch_remove_empty_dir,
patch_reject,
patch_keywords,
patch_with_fuzz,
Index: subversion/libsvn_client/patch.c
===
--- subversion/libsvn_client/patch.c(revision 912338)
+++ subversion/libsvn_client/patch.c(arbetskopia)
@@ -34,6 +34,7 @@
#include svn_path.h
#include svn_pools.h
#include svn_props.h
+#include svn_sorts.h
#include svn_subst.h
#include svn_wc.h
#include client.h
@@ -1159,6 +1160,305 @@
return SVN_NO_ERROR;
}
+/* Helper for collect_deleted_targets(). Checks if PARENT_DIR_ABSPATH is
+ * EMPTY when treating the targets in TARGETS_TO_BE_DELETED as already
+ * deleted. */
+static svn_error_t *
+is_dir_empty(svn_boolean_t *empty, const char *parent_dir_abspath,
+ svn_wc_context_t *wc_ctx,
+ apr_array_header_t *targets_to_be_deleted,
+ apr_pool_t *result_pool, apr_pool_t *scratch_pool)
+{
+ apr_dir_t *dir;
+ apr_finfo_t this_entry;
+ apr_int32_t
Re: [RFC] v2 Tree conflict resolver spec.
Hi Neels! Thanks for all your feedback! The use of the libsvn_wc terms BASE, WORKING and ACTUAL will be replaced by your suggested (or was it Julians?) checked-in state and checked-out state in the next version of the RFC. Find further comments inline. On Tue, Feb 09, 2010 at 03:20:57PM +0100, Neels J Hofmeyr wrote: Daniel Näslund wrote: Design spec for tree conflict resolution in the commandline client ~~~ The hard part is figuring out what state the wc is in during the different user cases. Actually, wc-ng should make that part easy. The hard part is making the conflict resolution conform with adjacent WC states. (attention, high degree of meta language. No need to understand me :P ) After a read through of last weeks discussions on pristine and base, the hard part is probably to understand what the terminology actually means! :) [...] Contents = Problem definition Requirements Terminology Use cases update/switch Use cases merge API changes User interface Problem definition = Users are having problems understanding how to resolve tree conflicts. For some operations they may not know how to get back to a previous state. They don't always know how to view the changes causing a conflict. Agreed!! :) The spec has really been about resolving conflicts and defining the states of the wc. We haven't touched on methods for viewing the changes. A diff is good for a text conflict but of limited value for displaying a tree conflict. More on that later. Requirements = It should be easy for the user to understand why the conflict has happened and how to resolv it. +1!! Update, switch and merge should be reversible. That is; going back to the former revision in the wc should restore the contents to the original. Need some finer grain here: going back? 'svn revert' should be able to undo all local changes, be they from merge or manually inflicted. After 'revert', any tree-conflict should be gone, and the node should reflect a state achievable with 'svn checkout'. We could have a desire to revert only the last steps of current modifications (i.e. only revert the last three merges that I did on top of a locally modified file, the last of which caused a tree-conflict) -- but this enters a scope far beyond tree conflict resolution. It would surely be nice nevertheless, and it might be implemented by layers of THEIRS information... in a different RFC. The resolver should not handle moves since we have no way to track those. When I say handle moves I mean do something about the other end not affected by this conflict. We will apply give the option to apply changes elswehere and do renames but we will leave some files behind for the user to clean up. (on the long run, with editor v2, we may be able to track moves satisfactorily. We might want to design this future behavior now so we can prepare for it and don't get ourselves in a big mess later.) Noted. My idea so far has been to substitute detection functionality by editor-v2 with input from the user, e.g. the user has the responsibility to detect moves and copies. When we have editor-v2, those parts should be easily replaced. ### There should be a good way to view what has caused a conflict. ### Perhaps some info from 'svn info'. Currently, 'svn info' on a tree-conflicted node says something like [[[ Tree conflict: local edit, incoming delete upon merge. Source left: (kind) u...@rev Source right: (kind) u...@rev ]]] Are you saying that there should be more info? Which in particular? I'm talking about the options given when running the interactive tree conflict resolver (In case you thought I meant adding more info to svn info). If I got a tree conflict running without the resolver I would want to know : * What is the content of theirs? A dir listing or file content. * what is the content of mine? A dir listing or file content. * Was the target copied [#]? * Was the target moved? [#] * If the target was copied or moved in theirs, where did it get copied/moved from? We can't tell this automatically (until you know what...) but we can help the user make a pretty good guess. * For merges, we probably would want to check the svn:mergeinfo in some cases. [#] I have a weak understanding of the copy-from and moved_here concepts. As I understand it, we can detect locally moved or copied targets. We can't tell what has happened on the server side but we know what has happened locally. To exemplify: For a locally copied, incoming move of the file alpha2 I would do something like this: [[[ svn st - shows me a local add, incoming add upon update TC on alpha2 svn info alpha2 - shows that alpha2 was locally copied from alpha svn info ^/trunk/alpha2 - shows me svn log -v ^/trunk/alpha2 - shows that alpha2 was moved from alpha
