Re: [OS-BUILD PATCHv5 2/3] process_configs.sh: Reduce multiple quotes

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

 



On Wed, 16 Sep 2020 19:25:26 -0000, GitLab Bridge on behalf of bcrocker wrote:
> -	for cfg in "$SCRIPT_DIR"/"${PACKAGE_NAME}""${KVERREL}""${SUBARCH}"*.config
> +	for cfg in "$SCRIPT_DIR/${PACKAGE_NAME}${KVERREL}${SUBARCH}"*.config

It doesn't make sense to introduce undesirable quoting in one patch and
immediately fixing it in a follow up. This should be just a single patch.

Nacked-by: Jiri Benc <jbenc@xxxxxxxxxx>

And when you update this, please be sure you run all the checks etc.
and that you are satisfied with the result before force pushing to
gitlab. There's really no reason for having 4 different versions of the
patches in one hour. That just generates noise on mailing lists and
makes review harder.

> -			rm "${cfgtmp}"
> +			rm -f "$cfgtmp"

This change certainly does not do what you write in the patch
description. This should be a separate patch.

The rule is simple: do one thing in one patch and do it well. You have
incomplete changes in patch 1 (as they need a follow up in patch 2) and
you have two different things mixed in patch 2 (fixing up coding style
and adding -f to rm).

Thanks,

 Jiri
_______________________________________________
kernel mailing list -- kernel@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to kernel-leave@xxxxxxxxxxxxxxxxxxxxxxx
Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: https://lists.fedoraproject.org/archives/list/kernel@xxxxxxxxxxxxxxxxxxxxxxx




[Index of Archives]     [Fedora General Discussion]     [Older Fedora Users Archive]     [Fedora Advisory Board]     [Fedora Security]     [Fedora Devel Java]     [Fedora Legacy]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Mentors]     [Fedora Package Announce]     [Fedora Package Review]     [Fedora Music]     [Fedora Packaging]     [Centos]     [Fedora SELinux]     [Coolkey]     [Yum Users]     [Tux]     [Yosemite News]     [KDE Users]     [Fedora Art]     [Fedora Docs]     [USB]     [Asterisk PBX]

  Powered by Linux