Hi Junio,
Junio C Hamano, Apr 11, 2023 at 18:28:
> 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.
> > 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.
> 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?
