Re: [PATCH v4 5/5] stash: implement builtin stash

[Thomas Gummerer <t.gummerer@gmail.com>]

On 06/19, Joel Teichroeb wrote:
> On Sun, Jun 11, 2017 at 2:27 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >> +
> >> +int cmd_stash(int argc, const char **argv, const char *prefix)
> >> +{
> >> +     int result = 0;
> >> +     pid_t pid = getpid();
> >> +
> >> +     struct option options[] = {
> >> +             OPT_END()
> >> +     };
> >> +
> >> +     git_config(git_default_config, NULL);
> >> +
> >> +     xsnprintf(stash_index_path, 64, ".git/index.stash.%d", pid);
> >> +
> >> +     argc = parse_options(argc, argv, prefix, options, git_stash_usage,
> >> +             PARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH);
> >> +
> >> +     if (argc < 1) {
> >> +             result = do_push_stash(NULL, prefix, 0, 0, 0, 0, NULL);
> >> +     } else if (!strcmp(argv[0], "list"))
> >> +             result = list_stash(argc, argv, prefix);
> >> +     else if (!strcmp(argv[0], "show"))
> >> +             result = show_stash(argc, argv, prefix);
> >> +     else if (!strcmp(argv[0], "save"))
> >> +             result = save_stash(argc, argv, prefix);
> >> +     else if (!strcmp(argv[0], "push"))
> >> +             result = push_stash(argc, argv, prefix);
> >> +     else if (!strcmp(argv[0], "apply"))
> >> +             result = apply_stash(argc, argv, prefix);
> >> +     else if (!strcmp(argv[0], "clear"))
> >> +             result = clear_stash(argc, argv, prefix);
> >> +     else if (!strcmp(argv[0], "create"))
> >> +             result = create_stash(argc, argv, prefix);
> >> +     else if (!strcmp(argv[0], "store"))
> >> +             result = store_stash(argc, argv, prefix);
> >> +     else if (!strcmp(argv[0], "drop"))
> >> +             result = drop_stash(argc, argv, prefix);
> >> +     else if (!strcmp(argv[0], "pop"))
> >> +             result = pop_stash(argc, argv, prefix);
> >> +     else if (!strcmp(argv[0], "branch"))
> >> +             result = branch_stash(argc, argv, prefix);
> >> +     else {
> >> +             if (argv[0][0] == '-') {
> >> +                     struct argv_array args = ARGV_ARRAY_INIT;
> >> +                     argv_array_push(&args, "push");
> >> +                     argv_array_pushv(&args, argv);
> >> +                     result = push_stash(args.argc, args.argv, prefix);
> >
> > This is a bit of a change in behaviour to what we currently have.
> >
> > The rules we decided on are as follows:
> >
> >  - "git stash -p" is an alias for "git stash push -p".
> >  - "git stash" with only option arguments is an alias for "git stash
> >    push" with those same arguments.  non-option arguments can be
> >    specified after a "--" for disambiguation.
> >
> > The above makes "git stash -*" a alias for "git stash push -*".  This
> > would result in a change of behaviour, for example in the case where
> > someone would use "git stash -this is a test-".  In that case the
> > current behaviour is to create a stash with the message "-this is a
> > test-", while the above would end up making git stash error out.  The
> > discussion on how we came up with those rules can be found at
> > http://public-inbox.org/git/20170206161432.zvpsqegjspaa2l5l@sigill.intra.peff.net/.
> 
> I don't really like the "argv[0][0] == '-'" logic, but it doesn't seem
> to have the flaw you pointed out:
> $ ./git stash -this is a test-
> error: unknown switch `t'
> usage: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
> [...]

I just went through the thread again, to remind myself why we did it
this way.  The example I had above was the wrong example, sorry.  The
message at [1] explains it better.  Essentially by implementing the
rules I mentioned we wanted to avoid the potential confusion of what
does 'git stash -q drop' mean.  Before the rewrite this fails and
shows the usage.  After the rewrite this would try to stash everything
matching the pathspec drop, which might be confusing for users.  

[1]: http://public-inbox.org/git/20170213214521.pkjesijdlus36tnp@sigill.intra.peff.net/

> I'm not sure this is the best possible error message, but it's just as
> useful as the message from the old version.
> 
> >
> >> +                     if (!result)
> >> +                             printf_ln(_("To restore them type \"git stash apply\""));
> >
> > In the shell script this is only displayed when the stash_push in the
> > case where git stash is invoked with no arguments, not in the push
> > case if I read this correctly.  So the two lines above should go in
> > the (argc < 1) case I think.
> 
> I think it's correct as is. One of the tests checks for this string to
> be output, and if I move the line, the test fails.

Right, that test that fails only when the "To restore..." string is
printed to stdout.  So moving the "printf_ln()" to the line you did
only makes sure it's not printed there.  Reading the code again and
trying to trigger this print in the shell script stash makes me think
this is not even possible to trigger there anymore.

After the line

test -n "$seen_non_option" || set "push" "$@"

it's not possible that $# is 0 anymore, so this will never be
printed.  From a quick look at the history it seems like it wasn't
possible to trigger that codepath for a while.  If I'm reading things
correctly 3c2eb80fe3 ("stash: simplify defaulting to "save" and reject
unknown options", 2009-08-18) seems to have introduced the small
change in behaviour.   As I don't think anyone has complained since
then, I'd just leave it as is, which makes git stash with no options a
little less verbose.  [Adding Matthieu to cc as author of the above
mentioned commit]

> I agreed with all the other points you raised, and they will be fixed
> in my next revision.

Thanks!