| Message ID | 1440417809-21069-4-git-send-email-gwshan@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 24 August 2015 at 13:03, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: > This submits changes with formatted commit log while updating Linux > headers using scripts/update-linux-headers.sh. > > Signed-off-by: Gavin Shan <gwshan@linux.vent.ibm.com> Thanks for writing a patch for this. > --- > scripts/update-linux-headers.sh | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh > index 18daabe..451b739 100755 > --- a/scripts/update-linux-headers.sh > +++ b/scripts/update-linux-headers.sh > @@ -63,6 +63,25 @@ cp_virtio() { > fi > } > > +submit_change() { > + from=$1 > + to=$2 > + if ! [ -e $to/include/qemu-common.h ]; then An error message about why we're bailing out might be nice. Also, it would be better to tell the user the output directory isn't valid before we spend all the time building the kernel headers, rather than afterwards... > + exit 3 > + fi > + > + cd $from > + version=$(make -s kernelversion) > + subject="Sync Linux headers from kernel $version" > + message=$(git log --oneline -1) > + cd - I think 'cd -' is a bashism. Better to use version=$(make -C $from -s kernelversion) message = $(cd $from && git log ...) etc. > + cd $to > + name=$(git config --get user.name) > + email=$(git config --get user.email) > + git commit -a -m "$subject" -m "$message" -m "Signed-off-by: $name <$email>" Is git commit's --signoff option not present on all the git versions we care about? The commit message could be made a bit more verbose. I'd suggest something like: Synchronize Linux headers from kernel $version Synchronize the Linux headers from kernel version $version (commit $commithash). This commit was created automatically by update-linux-headers.sh. thanks -- PMM
On 24 August 2015 at 13:03, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: > This submits changes with formatted commit log while updating Linux > headers using scripts/update-linux-headers.sh. > > Signed-off-by: Gavin Shan <gwshan@linux.vent.ibm.com> Also, you typoed your email address ^^^^ thanks -- PMM
On Mon, Aug 24, 2015 at 03:13:40PM +0100, Peter Maydell wrote: >On 24 August 2015 at 13:03, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: >> This submits changes with formatted commit log while updating Linux >> headers using scripts/update-linux-headers.sh. >> >> Signed-off-by: Gavin Shan <gwshan@linux.vent.ibm.com> > >Also, you typoed your email address ^^^^ > Yes, I noticed that right after sending it out. Thanks for pointing it out. Thanks, Gavin
On Mon, Aug 24, 2015 at 03:08:33PM +0100, Peter Maydell wrote: >On 24 August 2015 at 13:03, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: >> This submits changes with formatted commit log while updating Linux >> headers using scripts/update-linux-headers.sh. >> >> Signed-off-by: Gavin Shan <gwshan@linux.vent.ibm.com> > >Thanks for writing a patch for this. > Thanks for your review. >> --- >> scripts/update-linux-headers.sh | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh >> index 18daabe..451b739 100755 >> --- a/scripts/update-linux-headers.sh >> +++ b/scripts/update-linux-headers.sh >> @@ -63,6 +63,25 @@ cp_virtio() { >> fi >> } >> >> +submit_change() { >> + from=$1 >> + to=$2 >> + if ! [ -e $to/include/qemu-common.h ]; then > >An error message about why we're bailing out might be nice. >Also, it would be better to tell the user the output directory >isn't valid before we spend all the time building the >kernel headers, rather than afterwards... > Yes, I'll add one error message like below: echo "$to is not QEMU source directory, skip submitting changes" Note that the output directory hasn't to be QEMU source directory in original implementation, but I don't know the use cases to have invalid QEMU source directory. If you're sure to change the behaviour, the check here should be done at the beginning of this script as you suggested and the above error message would be something like below then: echo "Invalid QEMU source directory ($to) specified" >> + exit 3 >> + fi >> + >> + cd $from >> + version=$(make -s kernelversion) >> + subject="Sync Linux headers from kernel $version" >> + message=$(git log --oneline -1) >> + cd - > >I think 'cd -' is a bashism. Better to use > version=$(make -C $from -s kernelversion) > message = $(cd $from && git log ...) >etc. > Right. I'll change accordingly. >> + cd $to >> + name=$(git config --get user.name) >> + email=$(git config --get user.email) >> + git commit -a -m "$subject" -m "$message" -m "Signed-off-by: $name <$email>" > >Is git commit's --signoff option not present on all the git >versions we care about? > > >The commit message could be made a bit more verbose. I'd suggest >something like: > > Synchronize Linux headers from kernel $version > > Synchronize the Linux headers from kernel version $version > (commit $commithash). > > This commit was created automatically by update-linux-headers.sh. > We needn't care about if user.name and user.email are existing or not. If they're invalid, the commit log needs to be fixed manually. Or just to give explicit message like below to remind users to fix it? Anyway, the commit log isn't complete without correct name/email in SOB if I'm correct. name=$(git config --get user.name) email=$(git config --get user.email) if ! [ "$name" ]; then name="FIXME" fi if ! [ '$(echo "$email" | grep -v -e '@' > /dev/null)' ]; then email="FIXME" fi Thanks, Gavin
On 25 August 2015 at 00:58, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: > On Mon, Aug 24, 2015 at 03:08:33PM +0100, Peter Maydell wrote: >>On 24 August 2015 at 13:03, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: >>> + cd $to >>> + name=$(git config --get user.name) >>> + email=$(git config --get user.email) >>> + git commit -a -m "$subject" -m "$message" -m "Signed-off-by: $name <$email>" >> >>Is git commit's --signoff option not present on all the git >>versions we care about? > We needn't care about if user.name and user.email are existing or not. > If they're invalid, the commit log needs to be fixed manually. Or just > to give explicit message like below to remind users to fix it? Anyway, > the commit log isn't complete without correct name/email in SOB if > I'm correct. > > name=$(git config --get user.name) > email=$(git config --get user.email) > if ! [ "$name" ]; then > name="FIXME" > fi > if ! [ '$(echo "$email" | grep -v -e '@' > /dev/null)' ]; then > email="FIXME" > fi My point is that you appear to be manually reimplementing the built in git commit function for adding the SOB line, and you haven't explained why you need to do that. thanks -- PMM
On Tue, Aug 25, 2015 at 04:09:22PM +0100, Peter Maydell wrote: >On 25 August 2015 at 00:58, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: >> On Mon, Aug 24, 2015 at 03:08:33PM +0100, Peter Maydell wrote: >>>On 24 August 2015 at 13:03, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: >>>> + cd $to >>>> + name=$(git config --get user.name) >>>> + email=$(git config --get user.email) >>>> + git commit -a -m "$subject" -m "$message" -m "Signed-off-by: $name <$email>" >>> >>>Is git commit's --signoff option not present on all the git >>>versions we care about? > >> We needn't care about if user.name and user.email are existing or not. >> If they're invalid, the commit log needs to be fixed manually. Or just >> to give explicit message like below to remind users to fix it? Anyway, >> the commit log isn't complete without correct name/email in SOB if >> I'm correct. >> >> name=$(git config --get user.name) >> email=$(git config --get user.email) >> if ! [ "$name" ]; then >> name="FIXME" >> fi >> if ! [ '$(echo "$email" | grep -v -e '@' > /dev/null)' ]; then >> email="FIXME" >> fi > >My point is that you appear to be manually reimplementing >the built in git commit function for adding the SOB line, >and you haven't explained why you need to do that. > Ok. I misunderstood your original comment. Yeah, the SOB line can be simply dded by "-s" option to "git commit", which I just got from the manpage. I'll use "-s" option in next revision. If you don't object, "FIXME" for user.name and user.email if they're not existing will be folded to next revision as well. Thanks, Gavin >thanks >-- PMM >
On 26 August 2015 at 00:46, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: > Ok. I misunderstood your original comment. Yeah, the SOB line can be simply > dded by "-s" option to "git commit", which I just got from the manpage. I'll > use "-s" option in next revision. If you don't object, "FIXME" for user.name > and user.email if they're not existing will be folded to next revision as well. I wouldn't bother trying to handle missing name/email beyond whatever "git commit -s" does in that situation, personally. thanks -- PMM
diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh index 18daabe..451b739 100755 --- a/scripts/update-linux-headers.sh +++ b/scripts/update-linux-headers.sh @@ -63,6 +63,25 @@ cp_virtio() { fi } +submit_change() { + from=$1 + to=$2 + if ! [ -e $to/include/qemu-common.h ]; then + exit 3 + fi + + cd $from + version=$(make -s kernelversion) + subject="Sync Linux headers from kernel $version" + message=$(git log --oneline -1) + cd - + cd $to + name=$(git config --get user.name) + email=$(git config --get user.email) + git commit -a -m "$subject" -m "$message" -m "Signed-off-by: $name <$email>" + cd - +} + # This will pick up non-directories too (eg "Kconfig") but we will # ignore them in the next loop. ARCHLIST=$(cd "$linux/arch" && echo *) @@ -132,3 +151,5 @@ cat <<EOF >$output/include/standard-headers/linux/if_ether.h EOF rm -rf "$tmpdir" + +submit_change "$linux" "$output"
This submits changes with formatted commit log while updating Linux headers using scripts/update-linux-headers.sh. Signed-off-by: Gavin Shan <gwshan@linux.vent.ibm.com> --- scripts/update-linux-headers.sh | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)