Re: [PATCH v4 1/7] archive: optionally add "virtual" files

From: Junio C Hamano <gitster@pobox.com>
Date: May 11, 2022, 19:27 UTC
Message-ID: <xmqqzgjnkgy0.fsf@gitster.g>
In-Reply-To: <3cf6e4f8-9151-6d68-21ca-b94d6a7557e6@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 18 quoted lines

> Am 11.05.22 um 01:21 schrieb Junio C Hamano:
>> <rsbecker@nexbridge.com> writes:
>>
>>>> If we did "--add-virtual-file=<path>:0644:<contents>" instead from day one, it
>>>> certainly adds a few more lines of logic to this patch, and the calling "scalar
>>>> diagnose" may have to pass a few more bytes, but I suspect that such a change
>>>> would help the project in the longer run.
>
>> I did say "scalar diagnose" may not care.  But a patch to "git
>> archive" will affect other people, and among them there would be
>> people who say "gee, now I can add a handful of files from the
>> command line with their contents, without actually having them in
>> throw-away untracked files, when running 'git archive'.  That's
>> handy!", try it out and get disappointed by their inability to
>> create executable files that way.
>
> Which might motivate them to contribute a patch to add that feature.
> Give them a chance! :)

Yes, but there is no way to reuse the same option in a backward compatible way to later add the mode information, and that is why we want to be careful before a half-feature squats on an option.

> FWIW, I'd already be satisfied by a convincing outline of a way towards
> a complete solution to accept the partial feature, just to be sure we
> don't paint ourselves into a corner.

Exactly. As you say, an extra and separate option can be used. I do not know if that is a workaround because we didn't design the first option to take an additional option, or a welcome feature.

> Regarding file modes: We only effectively support the executable bit,
> so an additional option --add-virtual-executable-file=<path>:<contents>
> would suffice.

While I do not think we want to support more than one "is it executable or not?" bit, I am not so sure about what the current code does, though, for these "not from a tree, but added as extra files" entries.

If you add an extra file from an on-disk untracked file, the add_file_cb() callback picks up the full st.st_mode for the file, and write_archive_entries() in its loop over args->extra_files pass the full info->stat.st_mode down to write_entry(), which is used by archive-tar.c::write_tar_entry() to obtain mode bits pretty much as-is. For tracked paths, we probably are normalizing the blobs between 0644 and 0755 way before the values are passed as "mode" parameter to the write_entry() functions, but for these extra files, there is no such massaging.

So, I am OK with --add-virtual-executable=<path>:<contents> (but the point still stands that the way the code in the patch squats in the codepath makes it necessary to first refator it before it can happen) as a separate option. We may want to massage the mode bit we grab from these extra files, if we were to go that route, though.

Thanks.