On 08/08, Paul-Sebastian Ungureanu wrote:
> Currently, `show_stash()` uses `cmd_diff()` to generate
> the output. After this commit, the output will be generated
> using the internal API.
>
> Before this commit, `git stash show --quiet` would act like
> `git diff` and error out if the stash is not empty. Now, the
> `--quiet` option does not error out given an empty stash.
I think this needs a bit more justification. As I mentioned in my
comment to a previous patch, I'm not sure '--quiet' makes much sense
with 'git stash show' (it will show nothing, and will always exit with
an error code, as the stash will always contain something), but if we
are supporting the same flags as 'git diff', and essentially just
forwarding them, shouldn't they keep the same behaviour as well?
> Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@xxxxxxxxx>
> ---
> builtin/stash--helper.c | 73 +++++++++++++++++++++++++----------------
> 1 file changed, 45 insertions(+), 28 deletions(-)
>
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index 0c1efca6b..ec8c38c6f 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -10,6 +10,8 @@
> #include "run-command.h"
> #include "dir.h"
> #include "rerere.h"
> +#include "revision.h"
> +#include "log-tree.h"
>
> static const char * const git_stash_helper_usage[] = {
> N_("git stash--helper list [<options>]"),
> @@ -662,56 +664,71 @@ static int git_stash_config(const char *var, const char *value, void *cb)
>
> static int show_stash(int argc, const char **argv, const char *prefix)
> {
> - int i, ret = 0;
> - struct child_process cp = CHILD_PROCESS_INIT;
> - struct argv_array args_refs = ARGV_ARRAY_INIT;
> + int i;
> + int flags = 0;
> struct stash_info info;
> + struct rev_info rev;
> + struct argv_array stash_args = ARGV_ARRAY_INIT;
> struct option options[] = {
> OPT_END()
> };
>
> - argc = parse_options(argc, argv, prefix, options,
> - git_stash_helper_show_usage,
> - PARSE_OPT_KEEP_UNKNOWN);
> + init_diff_ui_defaults();
> + git_config(git_diff_ui_config, NULL);
>
> - cp.git_cmd = 1;
> - argv_array_push(&cp.args, "diff");
> + init_revisions(&rev, prefix);
>
> - /* Push arguments which are not options into args_refs. */
> - for (i = 0; i < argc; ++i) {
> - if (argv[i][0] == '-')
> - argv_array_push(&cp.args, argv[i]);
> + /* Push arguments which are not options into stash_args. */
> + for (i = 1; i < argc; ++i) {
> + if (argv[i][0] != '-')
> + argv_array_push(&stash_args, argv[i]);
> else
> - argv_array_push(&args_refs, argv[i]);
> - }
> -
> - if (get_stash_info(&info, args_refs.argc, args_refs.argv)) {
> - child_process_clear(&cp);
> - argv_array_clear(&args_refs);
> - return -1;
> + flags++;
> }
>
> /*
> * The config settings are applied only if there are not passed
> * any flags.
> */
> - if (cp.args.argc == 1) {
> + if (!flags) {
> git_config(git_stash_config, NULL);
> if (show_stat)
> - argv_array_push(&cp.args, "--stat");
> + rev.diffopt.output_format |= DIFF_FORMAT_DIFFSTAT;
> + if (show_patch) {
> + rev.diffopt.output_format = ~DIFF_FORMAT_NO_OUTPUT;
> + rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
> + }
I failed to notice this in the previous patch (the problem existed
there as well), but this changes the behaviour of 'git -c
stash.showStat=false stash show <stash>'. Previously doing this would
not show anything, which is the correct behaviour, while now still
shows the diffstat.
I think the show_stat variable is interpreted the wrong way around in
the previous patch.
Something else I noticed now that I was playing around more with the
config options is that the parsing of the config options is not
correctly done in the previous patch. It does a 'strcmp(var,
"stash.showStat"))', but the config API makes all variables lowercase
(config options are case insensitive, and making everything lowercase
is the way to ensure that), so it should be 'strcmp(var, "stash.showstat"))',
and similar for the 'stash.showpatch' config option.
This all sounds like it would be nice to have some tests for these
config options, to make sure we get it right, and won't break them in
the future.
> + }
>
> - if (show_patch)
> - argv_array_push(&cp.args, "-p");
> + if (get_stash_info(&info, stash_args.argc, stash_args.argv)) {
> + argv_array_clear(&stash_args);
> + return -1;
> }
>
> - argv_array_pushl(&cp.args, oid_to_hex(&info.b_commit),
> - oid_to_hex(&info.w_commit), NULL);
> + argc = setup_revisions(argc, argv, &rev, NULL);
> + if (!rev.diffopt.output_format)
> + rev.diffopt.output_format = DIFF_FORMAT_PATCH;
> + diff_setup_done(&rev.diffopt);
> + rev.diffopt.flags.recursive = 1;
> + setup_diff_pager(&rev.diffopt);
>
> - ret = run_command(&cp);
> + /*
> + * We can return early if there was any option not recognised by
> + * `diff_opt_parse()`, besides the word `stash`.
> + */
I'm not sure I follow what you mean with this comment. I don't see
any 'diff_opt_parse' in the code above?
> + if (argc > 1) {
> + free_stash_info(&info);
> + argv_array_clear(&stash_args);
> + usage_with_options(git_stash_helper_show_usage, options);
> + }
> +
> + /* Do the diff thing. */
This comment should be dropped. It's obvious that we are doing the
diff thing below from the code, so the comment is redundant at best,
makes people read more lines, and could possibly get outdated.
I'd also drop the other comments above, as they do not provide a lot
of extra value imho.
> + diff_tree_oid(&info.b_commit, &info.w_commit, "", &rev.diffopt);
> + log_tree_diff_flush(&rev);
>
> free_stash_info(&info);
> - argv_array_clear(&args_refs);
> - return ret;
> + argv_array_clear(&stash_args);
> + return 0;
> }
>
> int cmd_stash__helper(int argc, const char **argv, const char *prefix)
> --
> 2.18.0.573.g56500d98f
>
