git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Joel Teichroeb <joel@teichroeb.net>
Date
Jun 20, 2017, 02:37 UTC
Message-ID
<CA+CzEk9i8H2BAUrL854WJELCTa-O1ONMWa0uOcTsW=WxnB_22Q@mail.gmail.com>
In-Reply-To
<20170611212739.GA7737@hank>
On Sun, Jun 11, 2017 at 2:27 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
Show 64 quoted lines
>> +
>> +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'm not sure this is the best possible error message, but it's just as useful as the message from the old version.

Show 8 quoted lines
>
>> +                     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.

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

Previous: Thomas GummererNext: Thomas Gummerer
Message 15 of 26 in “Implement git stash as a builtin command”
  1. 0/5 Implement git stash as a builtin commandJoel Teichroeb, Jun 8, 2017
  2. 1/5 stash: add test for stash create with no filesJoel Teichroeb, Jun 8, 2017
  3. Junio C HamanoJun 13, 2017
  4. 2/5 stash: Add a test for when apply fails during stash branchJoel Teichroeb, Jun 8, 2017
  5. Junio C HamanoJun 13, 2017
  6. Joel TeichroebJun 13, 2017
  7. 3/5 stash: add test for stashing in a detached stateJoel Teichroeb, Jun 8, 2017
  8. Junio C HamanoJun 13, 2017
  9. Joel TeichroebJun 13, 2017
  10. Junio C HamanoJun 13, 2017
  11. 4/5 merge: close the index lock when not writing the new indexJoel Teichroeb, Jun 8, 2017
  12. Junio C HamanoJun 13, 2017
  13. 5/5 stash: implement builtin stashJoel Teichroeb, Jun 8, 2017
  14. Thomas GummererJun 11, 2017
  15. Joel TeichroebJun 20, 2017
  16. Thomas GummererJun 25, 2017
  17. Matthieu MoyJun 26, 2017
  18. Thomas GummererJun 27, 2017
  19. Junio C HamanoJun 16, 2017
  20. Junio C HamanoJun 16, 2017
  21. Johannes SchindelinJun 19, 2017
  22. Jeff KingJun 19, 2017
  23. Joel TeichroebJun 20, 2017
  24. Junio C HamanoJun 22, 2017
  25. Junio C HamanoJun 22, 2017
  26. Joel TeichroebJun 11, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.