7:43 p.m.
On 03/14/2017 07:05 PM, John Ferlan wrote:
On 03/11/2017 09:41 PM, Laine Stump wrote:
> Ah, but wait! I ACKed this too soon! *This* is the patch that's renaming the
functions. It should be changing the arguments in some cases too (and at least one of the
names seems wrong).
>
>
> On 03/10/2017 04:10 PM, John Ferlan wrote:
>> Rename the API's to remove the storage pool source pieces
> Yeah, if it's consistent in all cases, I can agree with that...
>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/conf/storage_adapter_conf.c | 14 +++++++-------
>> src/conf/storage_adapter_conf.h | 14 +++++++-------
>> src/conf/storage_conf.c | 8 ++++----
>> src/libvirt_private.syms | 8 ++++----
>> 4 files changed, 22 insertions(+), 22 deletions(-)
>>
>> diff --git a/src/conf/storage_adapter_conf.c b/src/conf/storage_adapter_conf.c
>> index 3a16bcc..4f5b665 100644
>> --- a/src/conf/storage_adapter_conf.c
>> +++ b/src/conf/storage_adapter_conf.c
>> @@ -38,7 +38,7 @@ VIR_ENUM_IMPL(virStoragePoolSourceAdapter,
>>
>>
>> void
>> -virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapterPtr adapter)
>> +virStorageAdapterClear(virStoragePoolSourceAdapterPtr adapter)
>> {
>> if (adapter->type == VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
>> VIR_FREE(adapter->data.fchost.wwnn);
>> @@ -55,9 +55,9 @@ virStoragePoolSourceAdapterClear(virStoragePoolSourceAdapterPtr
adapter)
>>
>>
>> int
>> -virStoragePoolDefParseSourceAdapter(virStoragePoolSourcePtr source,
>> - xmlNodePtr node,
>> - xmlXPathContextPtr ctxt)
>> +virStorageAdapterParseXML(virStoragePoolSourcePtr source,
>> + xmlNodePtr node,
>> + xmlXPathContextPtr ctxt)
> This function should take a virStoragePoolSourceAdapterPtr rather than a
virStoragePoolSourcePtr.
>
>> {
>> int ret = -1;
>> xmlNodePtr relnode = ctxt->node;
>> @@ -177,7 +177,7 @@ virStoragePoolDefParseSourceAdapter(virStoragePoolSourcePtr
source,
>>
>>
>> int
>> -virStoragePoolSourceAdapterParseValidate(virStoragePoolDefPtr ret)
>> +virStorageAdapterParseValidate(virStoragePoolDefPtr ret)
>
> This function should take a virStoragePoolSourceAdapterPtr rather than
virStoragePoolDefPtr, and the name should just be "virStorageAdapterValidate(), since
the parsing is already finished, and this function just validates.
>
I'd prefer to use virStorageAdapterValidateParse() - as that what it's
doing validating that the parse was correct. So is this is a case where
a verb can turn into an adverb? (it's a grammar question!)
Yeah, that name makes sense once you explain it. Maybe. Is it really validating that the
parse was done correctly? Or is it just validating that the data in the object meets
various criteria? Seems like it's the latter. Would you really want to validate the
object any differently if it had just been parsed from XML vs. if the object was generated
in some other manner? (e.g. some chunk of C code that created the object and filled in
attributes programmatically)
I don't think there's any "order" I could choose other than a really
painful use a long name now only to change to a shorter name approach in
later patches.
Yeah, I didn't look ahead when I started reviewing the series, so I didn't realize
what the end product was going to be. *A lot* of my comments are moot in light of that.
I'd like to reduce the amount of churn though just for
the sake of "name sanity" between the steps that no one will care about
except when they have to backport something... In which case, gitk is
your friend because it really makes the what changed when lookup simple.
I've stared straight at it for multiple years now, but just last week thought to try
out for the first time the "old Version" and "new Version" buttons in
gitk - combine those with a very large number for context lines, and it is truly very
useful.
Another nice way to look at diffs is with "git difftool -d -t meld revA..revB".
I haven't found a simple way to move from one commit to the next, but once it's
displayed it is a very nice format.
(while on the topic of tools, what I would *really* like is a tool like gitk where you
could select a chunk and drag it from one commit to another, or drag entire commits around
to change the order on a branch, or copy a commit or group of commits from one branch to
another (essentially something that would do an entire "git rebase -i master" or
"git cherry-pick" at the drag of a mouse). Any suggestions?