Re: svn commit: r25038 - trunk/subversion/libsvn_client

On Fri, 18 May 2007, Paul Burba wrote:

> > -----Original Message-----
> > From: Daniel Rall [mailto:dlr@collab.net]
> > Sent: Thursday, May 17, 2007 6:13 PM
> > To: dev@subversion.tigris.org
> > Subject: Re: svn commit: r25038 - trunk/subversion/libsvn_client
> >
> > This is definitely better than a segfault. However, won't
> > this still prevent the 'merge' operation from completing? Do
> > we want to handle this specific error in the calling code
> > (with notification back to the user that merge info couldn't
> > be determined, perhaps)?
>
> Hi Dan,
>
> Let me answer this in two parts, first the case where r25038 returns
> SVN_ERR_WC_PATH_NOT_FOUND and then where it returns
> SVN_ERR_ENTRY_MISSING_URL.
>
> ========================================
>
> SVN_ERR_WC_PATH_NOT_FOUND
>
> Yes, it will prevent the merge operation from completing, but AFAICT
> this happens *only* when the merge command targets a missing directory,
> that is, given:
>
> svn merge URL DIR_PATH -rX:Y
>
> DIR_PATH itself, not one of it's children, must be missing, and in that
> case what else could we possibly do other than error out?

Okay, makes sense.

...
> Now if DIR_PATH exists, but one of it's children affected by the merge
> is missing, then the merge does complete, albeit with skipped missing
> notifications:
>
> >svn merge %URL%/A merge_tests-1\A_COPY -r2:6
> Skipped missing target: 'merge_tests-1\A_COPY\D\H\psi'
> Skipped missing target: 'merge_tests-1\A_COPY\D\H'
> Skipped missing target: 'merge_tests-1\A_COPY\D\H\omega'
> Skipped missing target: 'merge_tests-1\A_COPY\D\H'
> U merge_tests-1\A_COPY\B\E\beta
>
> Here's why:
>
> get_wc_or_repos_merge_info() is currently called from three places:
>
> DO_SINGLE_FILE_MERGE() and DO_MERGE() - During a merge these are called
> directly by svn_client_merge_peg3()/svn_client_merge3() or from within
> discover_and_merge_children(). But for discover_and_merge_children()
> to "discover" a WC path with merge info, that path obviously can't be
> missing, so the only time do_single_file_merge() and do_merge() operate
> on a missing TARGET_WCPATH is when called from
> svn_client_merge_peg3()/svn_client_merge3() directly.
>
> SVN_CLIENT_GET_MERGEINFO() - Perhaps callers of *this* function might
> want to handle the error, but right now it's only caller is
> log.c:svn_client__suggest_merge_sources(). Which, as I understand code,
> can only be called when we specifiy no merge source path or url and must
> derive the source from the cwd. But to do that we must be in the cwd
> (so it can't be missing) and the cwd must be versioned or we would have
> already errored out with SVN_ERR_WC_NOT_DIRECTORY.

Okay.

>
> ========================================
>
> SVN_ERR_ENTRY_MISSING_URL
>
> Looking at this again:
>
> SVN_ERR(svn_wc__entry_versioned(entry, target_wcpath, adm_access,
> FALSE,
> pool));
>
> /* If TARGET_WCPATH is a directory we may get an entry with
> abrieviated
> information from TARGET_WCPATH's parent. This limited entry does
> not
> have a URL and probably means TARGET_WCPATH is missing or that the
> access for TARGET_WCPATH is not in ADM_ACCESS's baton set (i.e.
> ADM_ACCESS was opened for TARGET_WCPATH's parent with lock depth ==
> 0).
> Either way we can't get accurate merge info for TARGET_WCPATH. */
> if (! (*entry)->url)
> {
> svn_node_kind_t wckind;
>
> SVN_ERR(svn_io_check_path(target_wcpath, &wckind, pool));
> if (wckind == svn_node_none)
> {
> return svn_error_createf(SVN_ERR_WC_PATH_NOT_FOUND, NULL,
> _("Path '%s' is missing"),
> svn_path_local_style(target_wcpath,
> pool));
> }
> else
> {
> return svn_error_createf(SVN_ERR_ENTRY_MISSING_URL, NULL,
> _("Cannot find a URL for '%s'."),
> svn_path_local_style(target_wcpath,
> pool));
> }
> }
>
> I'm less sure now about returning the SVN_ERR_ENTRY_MISSING_URL error.
> I can't come up with a scenario that would exercise the else block. But
> I'm hesitant to change it, since it is a valid reason for
> svn_wc__entry_versioned() to succeed but entry-> URL to be NULL. Any
> thoughts on this?

If entry->url is NULL, the WC entry is bogus, effectively meaning a
corrupt WC entries file. libsvn_wc/entries.c:read_entry() looks like
it'll either fill in the appropriate information, or puke up an error.
So, I think we could remove the "else" block.

- Dan