qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v4 4/5] iotests: Add VMDK tests for blockdev-cre


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v4 4/5] iotests: Add VMDK tests for blockdev-create
Date: Fri, 7 Dec 2018 10:52:58 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1

On 12/7/18 10:42 AM, Kevin Wolf wrote:
Am 07.12.2018 um 16:40 hat Eric Blake geschrieben:
On 12/7/18 8:45 AM, Kevin Wolf wrote:
Am 07.12.2018 um 14:12 hat Markus Armbruster geschrieben:
git-am complains

      Applying: iotests: Add VMDK tests for blockdev-create
      .git/rebase-apply/patch:281: trailing whitespace.
                  format:
      .git/rebase-apply/patch:308: trailing whitespace.
                  format:
      .git/rebase-apply/patch:335: trailing whitespace.
                  format:
      .git/rebase-apply/patch:600: new blank line at EOF.
      +
      warning: 4 lines add whitespace errors.

This is in the reference output, so trailing whitespace/blank lines are
actually correct.

Ah, but doesn't ./check already ignore differences in trailing whitespace
present in the actual running that is not present in the *.out files,
precisely so we don't have to check in trailing whitespace reference
outputs?

It does ignore whitespace changes, so even if we remove that whitespace,
the test won't fail. But I don't think that's a good reason to check in
inaccurate reference output.

There are a few test cases that have a reference output like this and
it's always annoying: When I later add a new subtest, I add the new test
code, review the ./check output and if it looks good, I do something
like 'cp 237.out.bad 237.out'. At that point, I'll have to manually
revert completely unrelated whitespace changes again.

Is it worth teaching iotests.img_info_log() to strip trailing whitespace? Or even to teach qemu-img itself to quit generating trailing whitespace?


Some 'git am' warnings feel like the lesser evil to me.

True, and a comment in the commit message about intentionally triggering a known checkpatch flag goes a long ways to document why you want the trailing whitespace (assuming we don't instead decide to tackle a root cause of having the whitespace in the first place).

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



reply via email to

[Prev in Thread] Current Thread [Next in Thread]