Re: [PATCH v6 0/6] fast-export, fast-import: add support for signed-commits

[Elijah Newren <newren@gmail.com>]

Hi Christian,

On Mon, Mar 10, 2025 at 8:58 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> Luke Shumaker sent the first 4 versions of this series in April 2021,
> but it looks like he stopped before it got merged. Let's finish
> polishing it.
>
> Goal of this series
> ~~~~~~~~~~~~~~~~~~~
>
> fast-export has an existing --signed-tags= option that controls how to
> handle tag signatures.  However, there is no equivalent for commit
> signatures; it just silently strips the signature out of the commit
> (analogously to --signed-tags=strip).
>
> So implement a --signed-commits= flag in fast-export, and implement
> the receiving side of it in fast-import.
>
> Big picture goal
> ~~~~~~~~~~~~~~~~
>
> Independent from these --signed-tags/--signed-commits options
> addressed in this series, we want to have an option, that allows the
> person who performed the import to attest to the result by adding
> their own signature on tags and commits, whether these tags and
> commits were originally signed or not.
>
> This series lays the groundwork for that future option by upstreaming
> the earlier effort started by Luke Shumaker and stops there. Future
> follow-up work will build on it towards the big picture goal.
>
> Overview of the changes since v5
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> There is no real code change since v5, only a commit message, the
> documentation and some code comments are improved.
>
> Details of the changes since v5
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>   - Rebased on top of current 'master' branch at a36e024e98 (Merge
>     branch 'js/win-2.49-build-fixes', 2025-03-06). This is to get a
>     base as close as possible to v2.49.0 final.
>
>   - In patch 4/6 the commit message subject started with
>     "git-fast-export.txt:" instead of "git-fast-export.adoc" which has
>     been fixed.
>
>   - In patch 4/6 the documentation for `--signed-tags` in
>     "Documentation/git-fast-export.adoc" is improved to better explain
>     when it makes sense to use 'verbatim' and 'warn-verbatim', thanks
>     to Elijah.
>
>   - In patch 6/6 the documentation for `--signed-commits` in
>     "Documentation/git-fast-export.adoc" now spells out that its
>     default is 'abort', thanks to Elijah.
>
>   - In patch 6/6 a code comment in front of
>     find_commit_multiline_header() in "builtin/fast-export.c" has been
>     improved:
>
>       - a "rathar" vs "rather" typo has been fixed, thanks to Elijah,
>
>       - what should be done to the memory returned by the function has
>         been corrected, thanks to Phillip Wood.
>
> CI tests
> ~~~~~~~~
>
> All the CI tests passed, except perhaps the osx-gcc one which isn't
> finished yet, see:
>
> https://github.com/chriscool/git/actions/runs/13767984505
>
> Range diff compared to version 5
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> 1:  f97247e17d = 1:  395dc9b1d9 git-fast-import.adoc: add missing LF in the BNF
> 2:  b71588563d = 2:  6265fd51aa fast-export: fix missing whitespace after switch
> 3:  947bc267e6 = 3:  9e290bab22 fast-export: rename --signed-tags='warn' to 'warn-verbatim'
> 4:  45087db345 ! 4:  923885134f git-fast-export.txt: clarify why 'verbatim' may not be a good idea
>     @@ Metadata
>      Author: Luke Shumaker <lukeshu@datawire.io>
>
>       ## Commit message ##
>     -    git-fast-export.txt: clarify why 'verbatim' may not be a good idea
>     +    git-fast-export.adoc: clarify why 'verbatim' may not be a good idea
>
>          Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
>          Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>     @@ Documentation/git-fast-export.adoc: OPTIONS
>       exported and with 'warn-verbatim' (or 'warn', a deprecated synonym),
>      -they will be exported, but you will see a warning.
>      +they will be exported, but you will see a warning.  'verbatim' and
>     -+'warn-verbatim' should only be used if you know that no
>     -+transformation affecting tags will be performed, or if you do not
>     -+care that the resulting tag will have an invalid signature.
>     ++'warn-verbatim' should only be used if you know that no transformation
>     ++affecting tags or any commit in their history will be performed by you
>     ++or by fast-export or fast-import, or if you do not care that the
>     ++resulting tag will have an invalid signature.
>
>       --tag-of-filtered-object=(abort|drop|rewrite)::
>         Specify how to handle tags whose tagged object is filtered out.
> 5:  20f085a790 = 5:  49f73ee6ef fast-export: do not modify memory from get_commit_buffer
> 6:  48e0d4203c ! 6:  542c692e67 fast-export, fast-import: add support for signed-commits
>     @@ Commit message
>          Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>
>       ## Documentation/git-fast-export.adoc ##
>     -@@ Documentation/git-fast-export.adoc: they will be exported, but you will see a warning.  'verbatim' and
>     - transformation affecting tags will be performed, or if you do not
>     - care that the resulting tag will have an invalid signature.
>     +@@ Documentation/git-fast-export.adoc: affecting tags or any commit in their history will be performed by you
>     + or by fast-export or fast-import, or if you do not care that the
>     + resulting tag will have an invalid signature.
>
>      +--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
>      +  Specify how to handle signed commits.  Behaves exactly as
>     -+  '--signed-tags', but for commits.
>     ++  '--signed-tags', but for commits.  Default is 'abort'.
>      ++
>      +Earlier versions this command that did not have '--signed-commits'
>      +behaved as if '--signed-commits=strip'.  As an escape hatch for users
>     @@ builtin/fast-export.c: static void anonymize_ident_line(const char **beg, const
>
>      +/*
>      + * find_commit_multiline_header is similar to find_commit_header,
>     -+ * except that it handles multi-line headers, rathar than simply
>     ++ * except that it handles multi-line headers, rather than simply
>      + * returning the first line of the header.
>      + *
>      + * The returned string has had the ' ' line continuation markers
>     -+ * removed, and points to statically allocated memory (not to memory
>     -+ * within 'msg'), so it is only valid until the next call to
>     -+ * find_commit_multiline_header.
>     ++ * removed, and points to allocated memory that must be free()d (not
>     ++ * to memory within 'msg').
>      + *
>      + * If the header is found, then *end is set to point at the '\n' in
>      + * msg that immediately follows the header value.

I didn't look closely at Phillip's comments or your changes related to
those, but the other changes in the range-diff address my comments
from v5, so this version looks good to me.

Thanks!