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

From: Robin Jarry <robin@jarry.cc>
Date: Apr 11, 2023, 17:13 UTC
Message-ID: <CRU3FHOZIRVM.3N8I4FAZ2RGO5@ringo>
In-Reply-To: <xmqqbkjubcyc.fsf@gitster.g>

Hi Junio,

Junio C Hamano, Apr 11, 2023 at 18:28:

Show 30 quoted lines

> The above mentions "cover letter" and naturally the readers would
> wonder how it is treated.  When we have 5-patch series with a
> separate cover letter, do we get TOTAL=6, COUNTER=1 for the cover,
> COUNTER=2 for [PATCH 1/5], and so on, or do we see TOTAL=5,
> COUNTER=0 for the cover, counter=1 for [PATCH 1/5], and so on?
>
> The latter is certainly richer (with the former, the validator that
> wants to act differently on the cover has to somehow figure out if
> the invocation with COUNTER=1 is seeing the cover or the first
> patch).  The usual and recommended workflow being "git format-patch
> -o outdir --cover-letter <range>" followed by "edit outdir/*" to
> proofread and edit the cover and the patches, followed by "git
> send-email outdir/*.patch", git-send-email has to guess before
> invoking the hook.
>
> But it may be better than forcing the hook to guess, I dunno?
>
> Whichever way we choose, we should
>
>  * explain the choice in the proposed log message.  If we choose the
>    "TOTAL is the number of patches and COUNTER=0 is used for the
>    optional cover letter" interpretation, we should also explain
>    that we cannot reliably do so and sometimes can guess wrong.  If
>    we choose the "TOTAL is the number of input files and COUNTER
>    just counts, regardless of the payload" interpretation, we should
>    also explain that even though we hinted that a series with cover
>    letter can be validated, it is a slight lie, because the hook has
>    to guess if the series has cover and it can guess wrong.
>
>  * document what TOTAL and COUNTER means.

It is easy enough to differentiate a cover letter from an actual patch with a simple shell test:

    if grep -q "^diff --git " "$1"; then
        # patch file
    else
        # cover letter
    fi

It is probably best to let git-send-email out of the picture. Since nothing prevents from sending multiple patch series at once, it may not be possible to determine the proper ordering of all these files. A dumb 1-based counter will be perfectly suitable.

I will add more details about these two variables, what they mean and how they should be used.

> This may be sufficient documentation to imply we are not treating
> cover letter any differently, by not saying "patch" or "cover
> letter" but just saying "file".  It may be more helpful to be a bit
> more explicit, though (e.g. "files" -> "input files", perhaps).

It makes sense to use the "files" terminology instead of "patches". I will update for v2.

Show 6 quoted lines

> > Do we really need to clear these? Certainly not in each iteration of
> > the loop I would think.
>
> If we set TOTAL outside, we should clear it outside.  We have to set
> COUNTER inside, and we could clear it outside, but it probably is
> easier to see the correspondence of set/clear if it is done inside.

Given the small cost of setting these variables in a perl script, it was my intention to have a clear correspondence between the set/clear operations.

Show 24 quoted lines

> When you have 3 files to send, and if the last one satisfies "-p",
> the hook will be told "You are called for 1/3" and then "2/3", and
> will never hear about "3/3", so in practice it will spool the first
> two and finish without getting a chance to flush what has been
> spooled.  When you have 3 files to send, and if the first one
> satisfies "-p', the hook will be told "You are called for 2/3", but
> it is understandable if anybody is tempted to write a hook this way:
>
> 	if COUNTER==1:
> 		initialize the spool area
> 		record TOTAL there
> 	else:
> 		read TOTAL recorded in the spool area
> 		make sure TOTAL matches
>
> 	process [PATCH COUNTER/TOTAL] individually
> 	if COUNTER==TOTAL:
> 		process the series as a whole
>
> and for such an invocation of "git send-email", the hook will try to
> process the second file without having its state fully initialzied
> because it never saw the first.
>
> Would these be problems?  I dunno.

I had thought of this. From perl docs:

    -p  File is a named pipe (FIFO), or Filehandle is a pipe.
    https://perldoc.perl.org/functions/-p

While there is very little chance that users will run git send-email on FIFOs, it is a possibility. Reference commit is:

    300913bd448de ("git-send-email: Accept fifos as well as files")
    https://github.com/git/git/commit/300913bd448de

I can run the loop twice to determine the count of non-FIFOs and adjust GIT_SENDEMAIL_FILE_TOTAL accordingly.

Thanks for the review.

PS: What would you think if I also added a sendemail-validate.sample
script in the templates folder? Should I add it in the same commit?