Re: [GUILT v2 12/29] "guilt header": more robust header selection.

[Jeff Sipek <jeffpc@josefsipek.net>]

On Fri, May 16, 2014 at 11:51:37AM +0200, Per Cederqvist wrote:
> On Fri, May 16, 2014 at 12:46 AM, Jeff Sipek <jeffpc@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 <cederp@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*