Re: [PATCH v3] send-email: export patch counters in validate environment

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Robin

On 13/04/2023 15:01, Robin Jarry wrote:
> Hi Phillip,
> 
> Phillip Wood, Apr 13, 2023 at 15:52:
>> I think the documentation and implementation look good, I've left a
>> comment about the example hook below. As Junio has previously mentioned,
>> it would be nice to have a test with this patch.
> 
> Yes, I only got Junio's email after sending v3 :)
> 
> The test case is ready. I was waiting for more comments before sending
> a v4.

That's great, thank you for doing that.

>>> +	git worktree remove -f "$worktree" 2>/dev/null || :
>>
>> Now that you've got rid of "set -e" I don't think we need "|| :".
> 
> Right.
> 
>> I had expected that we'd always create a new worktree on the first
>> patch in a series and remove it after processing the the last patch in
>> the series, but this seems to leave it in place until the next time
>> send-email is run or /tmp gets cleaned up. Also if I've understood it
>> correctly the name is set the first time this hook is run, rather than
>> generating a new name for each set of files that is validated.
> 
> I had thought that it would be useful to keep it in case the user wants
> to inspect and resolve issues. I you think it is a problem to leave it,
> I can deleted it after the last patch. In any case, if the user
> interrupts send-email before it has time to validate all patches, the
> worktree will be left in place.

I think leaving it in place if there is an error is fine, but it ought 
to clean up after itself if there isn't an error. More serious is that 
we use mktemp to create the worktree path the first time the hook is run 
and then just keep using that same path forever. It should be creating a 
new temporary directory with mktemp for each series to avoid clashes 
with existing entries in /tmp.

Best Wishes

Phillip

> Thanks for the review!