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

[<rsbecker@nexbridge.com>]

On May 10, 2022 5:48 PM, Junio C Hamano wrote:
>"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
>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