On May 10, 2022 5:48 PM, Junio C Hamano wrote:
>"Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
>writes:
>
>> @@ -514,14 +522,38 @@ static int add_file_cb(const struct option *opt, const
>char *arg, int unset)
>> if (!arg)
>> return -1;
>>
>> - path = prefix_filename(args->prefix, arg);
>> - item = string_list_append_nodup(&args->extra_files, path);
>> - item->util = info = xmalloc(sizeof(*info));
>> + info = xmalloc(sizeof(*info));
>> info->base = xstrdup_or_null(base);
>> - if (stat(path, &info->stat))
>> - die(_("File not found: %s"), path);
>> - if (!S_ISREG(info->stat.st_mode))
>> - die(_("Not a regular file: %s"), path);
>> +
>> + if (!strcmp(opt->long_name, "add-file")) {
>> + path = prefix_filename(args->prefix, arg);
>> + if (stat(path, &info->stat))
>> + die(_("File not found: %s"), path);
>> + if (!S_ISREG(info->stat.st_mode))
>> + die(_("Not a regular file: %s"), path);
>> + info->content = NULL; /* read the file later */
>> + } else {
>
>This pretends that this new one will stay to be the only other option that uses the
>same callback in the future. To be more defensive, it should do
>
> } else if (!strcmp(opt->long_name, "...")) {
>
>and end the if/else if/else cascade with
>
> } else {
> BUG("add_file_cb called for unknown option");
> }
>
>> + const char *colon = strchr(arg, ':');
>> + char *p;
>> +
>> + if (!colon)
>> + die(_("missing colon: '%s'"), arg);
>> +
>> + p = xstrndup(arg, colon - arg);
>> + if (!args->prefix)
>> + path = p;
>> + else {
>> + path = prefix_filename(args->prefix, p);
>> + free(p);
>> + }
>> + memset(&info->stat, 0, sizeof(info->stat));
>> + info->stat.st_mode = S_IFREG | 0644;
>
>I can sympathize with the desire to omit the mode bits because it may not be
>useful for the immediate purpose of "scalar diagnose"
>where the extracting end won't care what the file's permission bits are, but by
>letting this "mode is hardcoded" thing squat here would later make it more work
>when other people want to add an option that truely lets the caller add a "vitual"
>file, in response to end-user complaints that they cannot use the existing one to
>add an exectuable file, for example. I do not care too much about the pathname
>limitation that does not allow a colon in it, simply because it is unusual enough, but
>I am not sure about hardcoded permission bits.
>
>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.
Would not core.filemode=false somewhat simulate this? The consumer-client would not care/do anything with the mode anyway. Or am I missing something?
--Randall
