Re: [GUILT v2 12/29] guilt header: more robust header selection.
On Fri, May 16, 2014 at 12:46 AM, Jeff Sipek jef...@josefsipek.net wrote: On Tue, May 13, 2014 at 10:30:48PM +0200, Per Cederqvist wrote: If you run something like guilt header '.*' the command would crash, because the grep comand that tries to ensure that the patch exist would detect a match, but the later code expected the match to be exact. Fixed by comparing exact strings. And as a creeping feature guilt header will now try to use the supplied patch name as an unachored regexp if no exact match was found. If the regexp yields a unique match, it is used; if more than one patch matches, the names of all patches are listed and the command fails. (Exercise left to the reader: generalized this so that guilt push also accepts a unique regular expression.) Signed-off-by: Per Cederqvist ced...@opera.com --- guilt-header | 28 +--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/guilt-header b/guilt-header index 41e00cc..4701b31 100755 --- a/guilt-header +++ b/guilt-header @@ -45,10 +45,32 @@ esac [ -z $patch ] die No patches applied. # check that patch exists in the series -ret=`get_full_series | grep -e ^$patch\$ | wc -l` -if [ $ret -eq 0 ]; then - die Patch $patch is not in the series +full_series=`get_tmp_file series` +get_full_series $full_series +found_patch= +while read x; do + if [ $x = $patch ]; then + found_patch=$patch + break + fi +done $full_series We have to use a temp file instead of a 'get_full_series | while read x; do ...' because that'd create a subshell, correct? Yes. Also (and probably less importantly) we sometimes need to run grep on the same output (see the creation of TMP_MATCHES below) and it would be a bit wasteful to run get_full_series twice. (The assumption is that it is cheaper to create a temp file than to recompute the value. I have not measured this, though.) +if [ -z $found_patch ]; then + TMP_MATCHES=`get_tmp_file series` + grep $patch $full_series $TMP_MATCHES + nr=`wc -l $TMP_MATCHES` + if [ $nr -gt 1 ]; then + echo $patch does not uniquely identify a patch. Did you mean any of these? 2 + sed 's/^/ /' $TMP_MATCHES 2 + rm -f $TMP_MATCHES + exit 1 + elif [ $nr -eq 0 ]; then + rm -f $TMP_MATCHES + die Patch $patch is not in the series + fi + found_patch=`cat $TMP_MATCHES` + rm -f $TMP_MATCHES fi +patch=$found_patch Do we not delete $full_series? Good catch. Will fix in the next version of the series. I'll also rename the variable $TMP_FULL_SERIES to adhere to the apparent coding style. (But I will not fix guilt-patchbomb that uses $dir as a temporary variable.) /ceder # FIXME: warn if we're editing an applied patch -- 1.8.3.1 -- OpenIndiana ibdm: 8 cores, 12 GB RAM -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GUILT v2 12/29] guilt header: more robust header selection.
On Fri, May 16, 2014 at 11:51:37AM +0200, Per Cederqvist wrote: On Fri, May 16, 2014 at 12:46 AM, Jeff Sipek jef...@josefsipek.net wrote: On Tue, May 13, 2014 at 10:30:48PM +0200, Per Cederqvist wrote: If you run something like guilt header '.*' the command would crash, because the grep comand that tries to ensure that the patch exist would detect a match, but the later code expected the match to be exact. Fixed by comparing exact strings. And as a creeping feature guilt header will now try to use the supplied patch name as an unachored regexp if no exact match was found. If the regexp yields a unique match, it is used; if more than one patch matches, the names of all patches are listed and the command fails. (Exercise left to the reader: generalized this so that guilt push also accepts a unique regular expression.) Signed-off-by: Per Cederqvist ced...@opera.com --- guilt-header | 28 +--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/guilt-header b/guilt-header index 41e00cc..4701b31 100755 --- a/guilt-header +++ b/guilt-header @@ -45,10 +45,32 @@ esac [ -z $patch ] die No patches applied. # check that patch exists in the series -ret=`get_full_series | grep -e ^$patch\$ | wc -l` -if [ $ret -eq 0 ]; then - die Patch $patch is not in the series +full_series=`get_tmp_file series` +get_full_series $full_series +found_patch= +while read x; do + if [ $x = $patch ]; then + found_patch=$patch + break + fi +done $full_series We have to use a temp file instead of a 'get_full_series | while read x; do ...' because that'd create a subshell, correct? Yes. Also (and probably less importantly) we sometimes need to run grep on the same output (see the creation of TMP_MATCHES below) and it would be a bit wasteful to run get_full_series twice. (The assumption is that it is cheaper to create a temp file than to recompute the value. I have not measured this, though.) I think this is a fair assumption. +if [ -z $found_patch ]; then + TMP_MATCHES=`get_tmp_file series` + grep $patch $full_series $TMP_MATCHES + nr=`wc -l $TMP_MATCHES` + if [ $nr -gt 1 ]; then + echo $patch does not uniquely identify a patch. Did you mean any of these? 2 + sed 's/^/ /' $TMP_MATCHES 2 + rm -f $TMP_MATCHES + exit 1 + elif [ $nr -eq 0 ]; then + rm -f $TMP_MATCHES + die Patch $patch is not in the series + fi + found_patch=`cat $TMP_MATCHES` + rm -f $TMP_MATCHES fi +patch=$found_patch Do we not delete $full_series? Good catch. Will fix in the next version of the series. I'll also rename the variable $TMP_FULL_SERIES to adhere to the apparent coding style. (But I will not fix guilt-patchbomb that uses $dir as a temporary variable.) Ok. Thanks, Jeff. -- *NOTE: This message is ROT-13 encrypted twice for extra protection* -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GUILT v2 12/29] guilt header: more robust header selection.
On Tue, May 13, 2014 at 10:30:48PM +0200, Per Cederqvist wrote: If you run something like guilt header '.*' the command would crash, because the grep comand that tries to ensure that the patch exist would detect a match, but the later code expected the match to be exact. Fixed by comparing exact strings. And as a creeping feature guilt header will now try to use the supplied patch name as an unachored regexp if no exact match was found. If the regexp yields a unique match, it is used; if more than one patch matches, the names of all patches are listed and the command fails. (Exercise left to the reader: generalized this so that guilt push also accepts a unique regular expression.) Signed-off-by: Per Cederqvist ced...@opera.com --- guilt-header | 28 +--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/guilt-header b/guilt-header index 41e00cc..4701b31 100755 --- a/guilt-header +++ b/guilt-header @@ -45,10 +45,32 @@ esac [ -z $patch ] die No patches applied. # check that patch exists in the series -ret=`get_full_series | grep -e ^$patch\$ | wc -l` -if [ $ret -eq 0 ]; then - die Patch $patch is not in the series +full_series=`get_tmp_file series` +get_full_series $full_series +found_patch= +while read x; do + if [ $x = $patch ]; then + found_patch=$patch + break + fi +done $full_series We have to use a temp file instead of a 'get_full_series | while read x; do ...' because that'd create a subshell, correct? +if [ -z $found_patch ]; then + TMP_MATCHES=`get_tmp_file series` + grep $patch $full_series $TMP_MATCHES + nr=`wc -l $TMP_MATCHES` + if [ $nr -gt 1 ]; then + echo $patch does not uniquely identify a patch. Did you mean any of these? 2 + sed 's/^/ /' $TMP_MATCHES 2 + rm -f $TMP_MATCHES + exit 1 + elif [ $nr -eq 0 ]; then + rm -f $TMP_MATCHES + die Patch $patch is not in the series + fi + found_patch=`cat $TMP_MATCHES` + rm -f $TMP_MATCHES fi +patch=$found_patch Do we not delete $full_series? # FIXME: warn if we're editing an applied patch -- 1.8.3.1 -- OpenIndiana ibdm: 8 cores, 12 GB RAM -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
