From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 14 May 2025 15:19:25 GMT
Subject: Re: [PATCH v2 04/11] contrib: remove "thunderbird-patch-inline"
Message-ID: <ed18e1e5-2941-4f00-9139-66bf95f0faa7@gmail.com>
In-Reply-To: <xmqqfrh9vlxr.fsf@gitster.g>

On 12/05/2025 17:22, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> Due to 'text/plan; format=flowed', the attached patch was damaged so
> the version I may be commenting on may be slightly different from
> what you wanted to show, but there is one thing I noticed.

Oh dear, I need to find out how to turn that off.
>> +SEP="$(printf '^\r\\{0,1\\}$')"
> 
> Here we assign to the variable SEP
> 
>>   SUBJECT=$(sed -n -e '/^Subject: /p' "${PATCH}")
>> -HEADERS=$(sed -e '/^'"${SEP}"'$/,$d' $1)
>> +HEADERS=$(sed -e "/${SEP}/"',$d' $1)
> 
> The old reference must be expecting that the variable SEP should be
> already set up.  I understand that a different separator is used in
> the new version, so it is perfectly fine that HEADERS need to be
> changed, but shouldn't the previous hunk that assign to SEP be
> removing an old assignment to SEP that gave the separator wanted by
> the older version?
> 
> It turns out that after the post-context of the first hunk there is
> assignment that the old separator value is assigned to SEP.  I think
> that should go.
Good point - for some reason I forgot to delete the original definition 
- I'll send a re-roll.

Thanks

Phillip>
>>   BODY=$(sed -e "1,/${SEP}/d" $1)
>>   CMT_MSG=$(sed -e '1,/^$/d' -e '/^---$/,$d' "${PATCH}")
>>   DIFF=$(sed -e '1,/^---$/d' "${PATCH}")
>> @@ -37,7 +43,7 @@ CCS=$(printf '%s\n%s\n' "$CMT_MSG" "$HEADERS" | sed -n -e 's/^Cc: \(.*\)$/\1,/gp
>>   echo "$SUBJECT" > $1
>>   echo "Cc: $CCS" >> $1
>>   echo "$HEADERS" | sed -e '/^Subject: /d' -e '/^Cc: /d' >> $1
>> -echo "$SEP" >> $1
>> +echo >> $1
>>     echo "$CMT_MSG" >> $1
>>   echo "---" >> $1