Re: [PATCH] commit --include-externals (v2)

On 11/04/2011 12:22 PM, Julian Foad wrote:
> Hi Neels. Brief response.

...yet a first class review, thanks a lot Julian!

> Looks like a good improvement.
>
> svn_wc__committable_external_info_t: Use the new 'svn_kind_t' instead of svn_node_kind_t.

doh!

> svn_client_commit6(): Could you extract the main chunk of added code as a separate function? That would help me (the reader) quickly understand how much of the local state it does *not* touch.

Good idea, done that.

> harvest_committables(): Would the 'is_explicit_target' parameter be better named something like 'include_file_externals'? I'm not sure if that would better reflect its purpose in that function; I haven't got my head around it well enough, so just asking.

Not sure -- while that's the only use for it currently, it does reflect a
more general fact about how LOCAL_ABSPATH was reached. Tweaked the doc. Good?

> svn_client_commit6(): In the doc string, of instead of "If A and B, all file respectively dir externals as defined ...", I suggest "If A and/or B, all file and/or dir externals (respectively) as defined by ...". Rationale: "respectively" isn't used as a conjunction word in English: <http://www.transblawg.eu/index.php?/archives/870-Resp.-and-other-non-existent-English-wordsNicht-existente-englische-Woerter.html>.

heh interesting link; I didn't mean the German "Beziehungsweise", I meant
the English "respectively" with its ordering meaning. I fixed its ill
placement, hope it's intelligible now.

> In the same doc string: mention the TODO about --depth=immediates skipping dir externals, that you documented inside the function.

Done.

> svn_wc__committable_externals_below(): Document the 'immediates_only' parameter.

Refactored 'immediates_only' to a 'depth' parameter, foreshadowing future
use and possibly clarifying things. 'immediates_only' is now only an arg on
the __db_ function, where it is documented.

...I also caught a wrongly placed '{' and a missing 'static'. And a "New
function" missing from the log.

I'm still not committing because I have just suggested to flip default
behavior and provide an --exclude-externals option instead, in the other mail...

Have attached the current patch state FYI.

Thanks again!
~Neels