Firmin Martin [off-list ref] writes:
Particulary, this behaviour could be awkward in the following
hypothetical situations:
* The user can easily erase a cover letter coming from prior versions or
another patch series by reusing an old command line (e.g. autocompleted
from the shell history).
"prior versions" implies that the user is better off using -v$n
where $n is the number greater than the one used for the prior
iteration by one, and there won't be any overwriting, so this is not
a very compelling use case.
But the next one is real.
* Assuming that the user is writing a cover letter and realizes that
small changes should be made. They make the change, amend and
format-patch again to regenerate patches. If it happens that they use
the same command again (e.g. with --cover-letter), the cover letter
being written is gone.
Yes, after preparing, say, -v2, but before sending them out, it is
very plausible that proofreading of your own patches may make you
realize more issues in the series, which may make you go back to your
commits, "rebase -i" to improve them and re-run "format-patch -v2".
We do want to encourage such careful preparation of your patch
series before sending it out, and we want to support it well with
our tools. Preventing overwriting of the cover (which will have the
same filename, with the same v2- prefix) is very valuable here.
There is another thing that I suspect people may find irritating in
the same workflow. If you fix the commit title while "rebase -i" to
polish your v2 patch, it would result in a different filename from
the original v2, so you'd end up with something like
v2-0000-cover-letter.patch
v2-0001-thes-forny-change.patch
v2-0001-this-phoney-change.patch
v2-0002-another-sample-change.patch
while redoing a two-patch series. The "thes-forny" thing is a
leftover from the first "format-patch -v2" run, you fixed typoes
with "rebase -i" after a self-review and other three files have the
result of the second "format-patch -v2" run. You need a way to
somehow exclude that stale file when driving send-email; in other
words, before running
git send-email v2-*.patch
you would want to move away v2-0001-thes-forny-change.patch that no
longer is part of the series. I wonder if format-patch can help by
looking at the output directory before writing its output and move
the old files away, say, to "old-v2-*.patch" or something? That
incidentally would solve your "files getting overwritten is
irritating" issue at the same time.
Coming back to the topic of cover letter, even when there is no risk
of ovetwriting, there is another thing we may want to improve to
help our users. Suppose you are preparing your v2 patch after
sending out the v1. The cover letter we generate for v2 will have
the same **BOILERPLATE** placeholders that need to be filled from
scratch. As many things you wrote for the cover letter in the
previous round should be reusable, even though the list of titles of
the patch should be generated afresh, it would be nice if
format-patch can carry forward what you wrote in the cover letter
for the v1 iteration to the cover letter for this v2 iteration.
And when we have such a "reuse description in the existing cover
letter" support, the value of "don't overwrite" knob will mostly go
away. Instead of failing the command by refusing to overwrite, you
can read what is in the existing cover-letter that is about to be
overwritten, combine the hand-written description with the material
automatically generated, and ovewrite the existing file, and as long
as you do a good job of preserving, nobody would complain that you
overwrote the file.
Junio C Hamano [off-list ref] writes:
Firmin Martin [off-list ref] writes:
quoted
Particulary, this behaviour could be awkward in the following
hypothetical situations:
* The user can easily erase a cover letter coming from prior versions or
another patch series by reusing an old command line (e.g. autocompleted
from the shell history).
"prior versions" implies that the user is better off using -v$n
where $n is the number greater than the one used for the prior
iteration by one, and there won't be any overwriting, so this is not
a very compelling use case.
True, silly of me.
But the next one is real.
quoted
* Assuming that the user is writing a cover letter and realizes that
small changes should be made. They make the change, amend and
format-patch again to regenerate patches. If it happens that they use
the same command again (e.g. with --cover-letter), the cover letter
being written is gone.
Yes, after preparing, say, -v2, but before sending them out, it is
very plausible that proofreading of your own patches may make you
realize more issues in the series, which may make you go back to your
commits, "rebase -i" to improve them and re-run "format-patch -v2".
We do want to encourage such careful preparation of your patch
series before sending it out, and we want to support it well with
our tools. Preventing overwriting of the cover (which will have the
same filename, with the same v2- prefix) is very valuable here.
There is another thing that I suspect people may find irritating in
the same workflow. If you fix the commit title while "rebase -i" to
polish your v2 patch, it would result in a different filename from
the original v2, so you'd end up with something like
v2-0000-cover-letter.patch
v2-0001-thes-forny-change.patch
v2-0001-this-phoney-change.patch
v2-0002-another-sample-change.patch
while redoing a two-patch series. The "thes-forny" thing is a
leftover from the first "format-patch -v2" run, you fixed typoes
with "rebase -i" after a self-review and other three files have the
result of the second "format-patch -v2" run. You need a way to
somehow exclude that stale file when driving send-email; in other
words, before running
git send-email v2-*.patch
you would want to move away v2-0001-thes-forny-change.patch that no
longer is part of the series. I wonder if format-patch can help by
looking at the output directory before writing its output and move
the old files away, say, to "old-v2-*.patch" or something? That
I would go with v2-*.patch.old as it won't be matched by *.patch
(something tempting to do when only one version is present). Or, we can
even keep their filenames unchanged and move them into a subdirectory
old_<timestamp>/.
The case of --numbered-files should also be taking into account. In the
same fashion, the files that will be renamed/moved would be those which
are constituted with numbers.
incidentally would solve your "files getting overwritten is
irritating" issue at the same time.
I will work towards this.
Coming back to the topic of cover letter, even when there is no risk
of ovetwriting, there is another thing we may want to improve to
help our users. Suppose you are preparing your v2 patch after
sending out the v1. The cover letter we generate for v2 will have
the same **BOILERPLATE** placeholders that need to be filled from
scratch. As many things you wrote for the cover letter in the
previous round should be reusable, even though the list of titles of
the patch should be generated afresh, it would be nice if
format-patch can carry forward what you wrote in the cover letter
for the v1 iteration to the cover letter for this v2 iteration.
And when we have such a "reuse description in the existing cover
letter" support, the value of "don't overwrite" knob will mostly go
away. Instead of failing the command by refusing to overwrite, you
can read what is in the existing cover-letter that is about to be
overwritten, combine the hand-written description with the material
automatically generated, and ovewrite the existing file, and as long
as you do a good job of preserving, nobody would complain that you
overwrote the file.
I have thought about that before this series and agree that this is the
way to go, but I found it a bit challenging: how should I distinguish
the cover letter body and the shortlog (i.e. any content we don't want
to keep to the next cover letter), given that the user could do anything
they want (e.g. remove the shortlog, write after the shortlog, etc.) ?
To easier this task, should we introduce a delimiter between the letter
body and the shortlog as we already do with patches right after the
commit message and before the diffstat (e.g. "---") ? I ignore whether
there are functions we can directly reuse in trailer.c for this purpose.
Thanks,
Firmin