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

From: Per Cederqvist <cederp@opera.com>
Date: May 16, 2014, 09:51 UTC
Message-ID: <CAP=KgsRmmmUOoasPGpJ12RRBXQxu07FbELHbQw38-=DM6MfW3w@mail.gmail.com>
In-Reply-To: <20140515224658.GA1334@meili.valhalla.31bits.net>

On Fri, May 16, 2014 at 12:46 AM, Jeff Sipek <jeffpc@josefsipek.net> wrote:

Show 43 quoted lines

> 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.)

Show 19 quoted lines

>> +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

Show 9 quoted lines

>>
>>  # FIXME: warn if we're editing an applied patch
>>
>> --
>> 1.8.3.1
>>
>
> --
> OpenIndiana ibdm: 8 cores, 12 GB RAM