Re: [PATCH 10/16] mktree: overwrite duplicate entries

From: Victoria Dye <vdye@github.com>
Date: Jun 12, 2024, 18:48 UTC
Message-ID: <dab4b0e3-8000-465e-8f0a-61df3d9168a3@github.com>
In-Reply-To: <ZmltGAPQ2dAfW0kG@tanuki>

Patrick Steinhardt wrote:

Show 41 quoted lines

> On Tue, Jun 11, 2024 at 06:24:42PM +0000, Victoria Dye via GitGitGadget wrote:
>> From: Victoria Dye <vdye@github.com>
>>
>> If multiple tree entries with the same name are provided as input to
>> 'mktree', only write the last one to the tree. Entries are considered
>> duplicates if they have identical names (*not* considering mode); if a blob
>> and a tree with the same name are provided, only the last one will be
>> written to the tree. A tree with duplicate entries is invalid (per 'git
>> fsck'), so that condition should be avoided wherever possible.
>>
>> Signed-off-by: Victoria Dye <vdye@github.com>
>> ---
>>  Documentation/git-mktree.txt |  8 ++++---
>>  builtin/mktree.c             | 45 ++++++++++++++++++++++++++++++++----
>>  t/t1010-mktree.sh            | 36 +++++++++++++++++++++++++++--
>>  3 files changed, 80 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/git-mktree.txt b/Documentation/git-mktree.txt
>> index fb07e40cef0..afbc846d077 100644
>> --- a/Documentation/git-mktree.txt
>> +++ b/Documentation/git-mktree.txt
>> @@ -43,9 +43,11 @@ OPTIONS
>>  INPUT FORMAT
>>  ------------
>>  Tree entries may be specified in any of the formats compatible with the
>> -`--index-info` option to linkgit:git-update-index[1]. The order of the tree
>> -entries is normalized by `mktree` so pre-sorting the input by path is not
>> -required.
>> +`--index-info` option to linkgit:git-update-index[1].
>> +
>> +The order of the tree entries is normalized by `mktree` so pre-sorting the input
>> +by path is not required. Multiple entries provided with the same path are
>> +deduplicated, with only the last one specified added to the tree.
> 
> Hm. I'm not sure whether this is a good idea. With git-mktree(1) being
> part of our plumbing layer, you can expect that it's mostly going to be
> fed input from scripts. And any script that generates duplicate tree
> entries is broken, but we now start to paper over such brokenness
> without giving the user any indicator of this. As user of git-mktree(1)
> in Gitaly I can certainly say that I'd rather want to see it die instead
> of silently fixing my inputs so that I start to notice my own bugs.

'git mktree' already does some cleaning of the inputs by sorting the entries, presumably so that a valid tree is created rather than one with ordering errors. Deduplication is also a cleanup of user inputs to ensure a valid tree is created, so to me it's a consistent extension to existing behavior. Conversely, rejecting the inputs and failing would be introducing an error scenario where none existed previously, which to me would be a bigger deviation.

One potential way to get the kind of functionality you're looking for, though, might be to combine something like '--literally' and a '--strict' that validates the tree before writing. Like I mentioned in the cover letter [1], I do plan to submit a follow-up series with '--strict' (it's just that this series is already pretty long and it would add 4-ish more patches).

[1] https://lore.kernel.org/git/pull.1746.git.1718130288.gitgitgadget@gmail.com/

> So without seeing a strong motivating usecase for this feature I'd think
> that git-mktree(1) should reject such inputs and return an error such
> that the user can fix their tooling.

Practically, there are a couple of reasons that led me to wanting this behavior. One is that it allows using data structures with more rigid integrity checks (like the index & cache tree). The other is that, once the ability to add nested entries is introduced, the concept of a "duplicate" gets fuzzier and blocking them entirely could lead to inconsistencies and/or limited flexibility. If, for example a user wants to create a tree with a directory 'folder1/' with OID '0123456789012345678901234567890123456789', but update a blob 'folder1/file1' in it to OID '0987654321098765432109876543210987654321', the latter is technically a "duplicate" but rejecting it would avoid being able to create the tree without first expanding 'folder1/'with something like 'ls-tree', replacing the appropriate entry, then calling 'mktree'.

> 
> Patrick