"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.
Thanks.
