From 6a7aca6f016e397838926250764318b767650bd5 Mon Sep 17 00:00:00 2001 From: Alexandr Miloslavskiy Date: Thu, 16 Jan 2020 16:09:18 +0000 Subject: doc: rm: synchronize description This patch continues the effort that is already applied to `git commit`, `git reset`, `git checkout` etc. 1) Changed outdated descriptions to mention pathspec instead. 2) Added reference to 'linkgit:gitglossary[7]'. 3) Removed content that merely repeated gitglossary. 4) Merged the remainder of "discussion" into ``. Signed-off-by: Alexandr Miloslavskiy Signed-off-by: Junio C Hamano --- Documentation/git-rm.txt | 50 ++++++++++++++++++++---------------------------- 1 file changed, 21 insertions(+), 29 deletions(-) diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt index b5c46223c4..e02a08e5ef 100644 --- a/Documentation/git-rm.txt +++ b/Documentation/git-rm.txt @@ -8,16 +8,16 @@ git-rm - Remove files from the working tree and from the index SYNOPSIS -------- [verse] -'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch] [--quiet] [--] ... +'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch] [--quiet] [--] ... DESCRIPTION ----------- -Remove files from the index, or from the working tree and the index. -`git rm` will not remove a file from just your working directory. -(There is no option to remove a file only from the working tree -and yet keep it in the index; use `/bin/rm` if you want to do that.) -The files being removed have to be identical to the tip of the branch, -and no updates to their contents can be staged in the index, +Remove files matching pathspec from the index, or from the working tree +and the index. `git rm` will not remove a file from just your working +directory. (There is no option to remove a file only from the working +tree and yet keep it in the index; use `/bin/rm` if you want to do +that.) The files being removed have to be identical to the tip of the +branch, and no updates to their contents can be staged in the index, though that default behavior can be overridden with the `-f` option. When `--cached` is given, the staged content has to match either the tip of the branch or the file on disk, @@ -26,15 +26,20 @@ allowing the file to be removed from just the index. OPTIONS ------- -...:: - Files to remove. Fileglobs (e.g. `*.c`) can be given to - remove all matching files. If you want Git to expand - file glob characters, you may need to shell-escape them. - A leading directory name - (e.g. `dir` to remove `dir/file1` and `dir/file2`) can be - given to remove all files in the directory, and recursively - all sub-directories, - but this requires the `-r` option to be explicitly given. +...:: + Files to remove. A leading directory name (e.g. `dir` to remove + `dir/file1` and `dir/file2`) can be given to remove all files in + the directory, and recursively all sub-directories, but this + requires the `-r` option to be explicitly given. ++ +The command removes only the paths that are known to Git. ++ +File globbing matches across directory boundaries. Thus, given two +directories `d` and `d2`, there is a difference between using +`git rm 'd*'` and `git rm 'd/*'`, as the former will also remove all +of directory `d2`. ++ +For more details, see the 'pathspec' entry in linkgit:gitglossary[7]. -f:: --force:: @@ -69,19 +74,6 @@ OPTIONS for each file removed. This option suppresses that output. -DISCUSSION ----------- - -The list given to the command can be exact pathnames, -file glob patterns, or leading directory names. The command -removes only the paths that are known to Git. Giving the name of -a file that you have not told Git about does not remove that file. - -File globbing matches across directory boundaries. Thus, given -two directories `d` and `d2`, there is a difference between -using `git rm 'd*'` and `git rm 'd/*'`, as the former will -also remove all of directory `d2`. - REMOVING FILES THAT HAVE DISAPPEARED FROM THE FILESYSTEM -------------------------------------------------------- There is no option for `git rm` to remove from the index only -- cgit v1.2.3 From 5f393dc3aa62ba3e13a9890af4e769ad84812fbc Mon Sep 17 00:00:00 2001 From: Alexandr Miloslavskiy Date: Mon, 17 Feb 2020 17:25:16 +0000 Subject: rm: support the --pathspec-from-file option Decisions taken for simplicity: 1) It is not allowed to pass pathspec in both args and file. Adjustments were needed for `if (!argc)` block: This code actually means "pathspec is not present". Previously, pathspec could only come from commandline arguments, so testing for `argc` was a valid way of testing for the presence of pathspec. But this is no longer true with `--pathspec-from-file`. During the entire `--pathspec-from-file` story, I tried to keep its behavior very close to giving pathspec on commandline, so that switching from one to another doesn't involve any surprises. However, throwing usage at user in the case of empty `--pathspec-from-file` would puzzle because there's nothing wrong with "usage" (that is, argc/argv array). On the other hand, throwing usage in the old case also feels bad to me. While it's less of a puzzle, I (as user) never liked the experience of comparing my commandline to "usage", trying to spot a difference. Since it's already known what the error is, it feels a lot better to give that specific error to user. Judging from [1] it doesn't seem that showing usage in this case was important (the patch was to avoid segfault), and it doesn't fit into how other commands react to empty pathspec (see for example `git add` with a custom message). Therefore, I decided to show new error text in both cases. In order to continue testing for error early, I moved `parse_pathspec()` higher. Now it happens before `read_cache()` / `hold_locked_index()` / `setup_work_tree()`, which shouldn't cause any issues. [1] Commit 7612a1ef ("git-rm: honor -n flag" 2006-06-09) Signed-off-by: Alexandr Miloslavskiy Signed-off-by: Junio C Hamano --- Documentation/git-rm.txt | 17 +++++++++- builtin/rm.c | 28 ++++++++++++---- t/t3601-rm-pathspec-file.sh | 79 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 117 insertions(+), 7 deletions(-) create mode 100755 t/t3601-rm-pathspec-file.sh diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt index e02a08e5ef..ab750367fd 100644 --- a/Documentation/git-rm.txt +++ b/Documentation/git-rm.txt @@ -8,7 +8,9 @@ git-rm - Remove files from the working tree and from the index SYNOPSIS -------- [verse] -'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch] [--quiet] [--] ... +'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch] + [--quiet] [--pathspec-from-file= [--pathspec-file-nul]] + [--] [...] DESCRIPTION ----------- @@ -73,6 +75,19 @@ For more details, see the 'pathspec' entry in linkgit:gitglossary[7]. `git rm` normally outputs one line (in the form of an `rm` command) for each file removed. This option suppresses that output. +--pathspec-from-file=:: + Pathspec is passed in `` instead of commandline args. If + `` is exactly `-` then standard input is used. Pathspec + elements are separated by LF or CR/LF. Pathspec elements can be + quoted as explained for the configuration variable `core.quotePath` + (see linkgit:git-config[1]). See also `--pathspec-file-nul` and + global `--literal-pathspecs`. + +--pathspec-file-nul:: + Only meaningful with `--pathspec-from-file`. Pathspec elements are + separated with NUL character and all other characters are taken + literally (including newlines and quotes). + REMOVING FILES THAT HAVE DISAPPEARED FROM THE FILESYSTEM -------------------------------------------------------- diff --git a/builtin/rm.c b/builtin/rm.c index 19ce95a901..4858631e0f 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -235,7 +235,8 @@ static int check_local_mod(struct object_id *head, int index_only) } static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0; -static int ignore_unmatch = 0; +static int ignore_unmatch = 0, pathspec_file_nul; +static char *pathspec_from_file; static struct option builtin_rm_options[] = { OPT__DRY_RUN(&show_only, N_("dry run")), @@ -245,6 +246,8 @@ static struct option builtin_rm_options[] = { OPT_BOOL('r', NULL, &recursive, N_("allow recursive removal")), OPT_BOOL( 0 , "ignore-unmatch", &ignore_unmatch, N_("exit with a zero status even if nothing matched")), + OPT_PATHSPEC_FROM_FILE(&pathspec_from_file), + OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul), OPT_END(), }; @@ -259,8 +262,24 @@ int cmd_rm(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, builtin_rm_options, builtin_rm_usage, 0); - if (!argc) - usage_with_options(builtin_rm_usage, builtin_rm_options); + + parse_pathspec(&pathspec, 0, + PATHSPEC_PREFER_CWD, + prefix, argv); + + if (pathspec_from_file) { + if (pathspec.nr) + die(_("--pathspec-from-file is incompatible with pathspec arguments")); + + parse_pathspec_file(&pathspec, 0, + PATHSPEC_PREFER_CWD, + prefix, pathspec_from_file, pathspec_file_nul); + } else if (pathspec_file_nul) { + die(_("--pathspec-file-nul requires --pathspec-from-file")); + } + + if (!pathspec.nr) + die(_("No pathspec was given. Which files should I remove?")); if (!index_only) setup_work_tree(); @@ -270,9 +289,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix) if (read_cache() < 0) die(_("index file corrupt")); - parse_pathspec(&pathspec, 0, - PATHSPEC_PREFER_CWD, - prefix, argv); refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, &pathspec, NULL, NULL); seen = xcalloc(pathspec.nr, 1); diff --git a/t/t3601-rm-pathspec-file.sh b/t/t3601-rm-pathspec-file.sh new file mode 100755 index 0000000000..7de21f8bcf --- /dev/null +++ b/t/t3601-rm-pathspec-file.sh @@ -0,0 +1,79 @@ +#!/bin/sh + +test_description='rm --pathspec-from-file' + +. ./test-lib.sh + +test_tick + +test_expect_success setup ' + echo A >fileA.t && + echo B >fileB.t && + echo C >fileC.t && + echo D >fileD.t && + git add fileA.t fileB.t fileC.t fileD.t && + git commit -m "files" && + + git tag checkpoint +' + +restore_checkpoint () { + git reset --hard checkpoint +} + +verify_expect () { + git status --porcelain --untracked-files=no -- fileA.t fileB.t fileC.t fileD.t >actual && + test_cmp expect actual +} + +test_expect_success 'simplest' ' + restore_checkpoint && + + cat >expect <<-\EOF && + D fileA.t + EOF + + echo fileA.t | git rm --pathspec-from-file=- && + verify_expect +' + +test_expect_success '--pathspec-file-nul' ' + restore_checkpoint && + + cat >expect <<-\EOF && + D fileA.t + D fileB.t + EOF + + printf "fileA.t\0fileB.t\0" | git rm --pathspec-from-file=- --pathspec-file-nul && + verify_expect +' + +test_expect_success 'only touches what was listed' ' + restore_checkpoint && + + cat >expect <<-\EOF && + D fileB.t + D fileC.t + EOF + + printf "fileB.t\nfileC.t\n" | git rm --pathspec-from-file=- && + verify_expect +' + +test_expect_success 'error conditions' ' + restore_checkpoint && + echo fileA.t >list && + + test_must_fail git rm --pathspec-from-file=list -- fileA.t 2>err && + test_i18ngrep -e "--pathspec-from-file is incompatible with pathspec arguments" err && + + test_must_fail git rm --pathspec-file-nul 2>err && + test_i18ngrep -e "--pathspec-file-nul requires --pathspec-from-file" err && + + >empty_list && + test_must_fail git rm --pathspec-from-file=empty_list 2>err && + test_i18ngrep -e "No pathspec was given. Which files should I remove?" err +' + +test_done -- cgit v1.2.3 From 2b7460d16719b751f025094ba9c8d10c81a6c4b1 Mon Sep 17 00:00:00 2001 From: Alexandr Miloslavskiy Date: Mon, 17 Feb 2020 17:25:17 +0000 Subject: doc: stash: split options from description (1) This patch moves blocks of text as-is to make it easier to review the next patch. Signed-off-by: Alexandr Miloslavskiy Signed-off-by: Junio C Hamano --- Documentation/git-stash.txt | 68 +++++++++++++++++++++++---------------------- 1 file changed, 35 insertions(+), 33 deletions(-) diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt index 53e1a1205d..31ab780f5a 100644 --- a/Documentation/git-stash.txt +++ b/Documentation/git-stash.txt @@ -58,31 +58,6 @@ non-option arguments are not allowed to prevent a misspelled subcommand from making an unwanted stash entry. The two exceptions to this are `stash -p` which acts as alias for `stash push -p` and pathspecs, which are allowed after a double hyphen `--` for disambiguation. -+ -When pathspec is given to 'git stash push', the new stash entry records the -modified states only for the files that match the pathspec. The index -entries and working tree files are then rolled back to the state in -HEAD only for these files, too, leaving files that do not match the -pathspec intact. -+ -If the `--keep-index` option is used, all changes already added to the -index are left intact. -+ -If the `--include-untracked` option is used, all untracked files are also -stashed and then cleaned up with `git clean`, leaving the working directory -in a very clean state. If the `--all` option is used instead then the -ignored files are stashed and cleaned in addition to the untracked files. -+ -With `--patch`, you can interactively select hunks from the diff -between HEAD and the working tree to be stashed. The stash entry is -constructed such that its index state is the same as the index state -of your repository, and its worktree contains only the changes you -selected interactively. The selected changes are then rolled back -from your worktree. See the ``Interactive Mode'' section of -linkgit:git-add[1] to learn how to operate the `--patch` mode. -+ -The `--patch` option implies `--keep-index`. You can use -`--no-keep-index` to override this. save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] []:: @@ -128,14 +103,6 @@ pop [--index] [-q|--quiet] []:: Applying the state can fail with conflicts; in this case, it is not removed from the stash list. You need to resolve the conflicts by hand and call `git stash drop` manually afterwards. -+ -If the `--index` option is used, then tries to reinstate not only the working -tree's changes, but also the index's ones. However, this can fail, when you -have conflicts (which are stored in the index, where you therefore can no -longer apply the changes as they were originally). -+ -When no `` is given, `stash@{0}` is assumed, otherwise `` must -be a reference of the form `stash@{}`. apply [--index] [-q|--quiet] []:: @@ -185,6 +152,41 @@ store:: reflog. This is intended to be useful for scripts. It is probably not the command you want to use; see "push" above. +If the `--all` option is used instead then the +ignored files are stashed and cleaned in addition to the untracked files. + +If the `--include-untracked` option is used, all untracked files are also +stashed and then cleaned up with `git clean`, leaving the working directory +in a very clean state. + +If the `--index` option is used, then tries to reinstate not only the working +tree's changes, but also the index's ones. However, this can fail, when you +have conflicts (which are stored in the index, where you therefore can no +longer apply the changes as they were originally). + +If the `--keep-index` option is used, all changes already added to the +index are left intact. + +With `--patch`, you can interactively select hunks from the diff +between HEAD and the working tree to be stashed. The stash entry is +constructed such that its index state is the same as the index state +of your repository, and its worktree contains only the changes you +selected interactively. The selected changes are then rolled back +from your worktree. See the ``Interactive Mode'' section of +linkgit:git-add[1] to learn how to operate the `--patch` mode. ++ +The `--patch` option implies `--keep-index`. You can use +`--no-keep-index` to override this. + +When pathspec is given to 'git stash push', the new stash entry records the +modified states only for the files that match the pathspec. The index +entries and working tree files are then rolled back to the state in +HEAD only for these files, too, leaving files that do not match the +pathspec intact. + +When no `` is given, `stash@{0}` is assumed, otherwise `` must +be a reference of the form `stash@{}`. + DISCUSSION ---------- -- cgit v1.2.3 From 0093abc286fe3f195f702de28c6d9af009dda8e0 Mon Sep 17 00:00:00 2001 From: Alexandr Miloslavskiy Date: Mon, 17 Feb 2020 17:25:18 +0000 Subject: doc: stash: split options from description (2) Together with the previous patch, this brings docs for `git stash` to the common layout used for most other commands (see for example docs for `git add`, `git commit`, `git checkout`, `git reset`) where all options are documented in a separate list. After some thinking and having a look at docs for `git svn` and `git `submodule`, I have arrived at following conclusions: * Options should be described in a list rather then text to facilitate lookup for user. * Single list is better then multiple lists because it avoids copy&pasting descriptions between subcommands (or, without copy&pasting, user will have to look up missing options in other subcommands). * As a consequence, commands section should only give brief info and list possible options. Since options have good enough names, user will only need to look up the "interesting" options. * Every option should list which subcommands support it. I have decided to use alphabetical sorting in the list of options to facilitate lookup for user. There is some text editing done to make old descriptions better fit into the list-style format. Signed-off-by: Alexandr Miloslavskiy Signed-off-by: Junio C Hamano --- Documentation/git-stash.txt | 92 ++++++++++++++++++++++++++++----------------- 1 file changed, 57 insertions(+), 35 deletions(-) diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt index 31ab780f5a..69281fcf43 100644 --- a/Documentation/git-stash.txt +++ b/Documentation/git-stash.txt @@ -43,8 +43,8 @@ created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}` is also possible). Stashes may also be referenced by specifying just the stash index (e.g. the integer `n` is equivalent to `stash@{n}`). -OPTIONS -------- +COMMANDS +-------- push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message ] [--] [...]:: @@ -86,7 +86,7 @@ show [] []:: Show the changes recorded in the stash entry as a diff between the stashed contents and the commit back when the stash entry was first - created. When no `` is given, it shows the latest one. + created. By default, the command shows the diffstat, but it will accept any format known to 'git diff' (e.g., `git stash show -p stash@{1}` to view the second most recent entry in patch form). @@ -116,8 +116,7 @@ branch []:: the commit at which the `` was originally created, applies the changes recorded in `` to the new working tree and index. If that succeeds, and `` is a reference of the form - `stash@{}`, it then drops the ``. When no `` - is given, applies the latest one. + `stash@{}`, it then drops the ``. + This is useful if the branch on which you ran `git stash push` has changed enough that `git stash apply` fails due to conflicts. Since @@ -133,9 +132,6 @@ clear:: drop [-q|--quiet] []:: Remove a single stash entry from the list of stash entries. - When no `` is given, it removes the latest one. - i.e. `stash@{0}`, otherwise `` must be a valid stash - log reference of the form `stash@{}`. create:: @@ -152,40 +148,66 @@ store:: reflog. This is intended to be useful for scripts. It is probably not the command you want to use; see "push" above. -If the `--all` option is used instead then the -ignored files are stashed and cleaned in addition to the untracked files. - -If the `--include-untracked` option is used, all untracked files are also -stashed and then cleaned up with `git clean`, leaving the working directory -in a very clean state. +OPTIONS +------- +-a:: +--all:: + This option is only valid for `push` and `save` commands. ++ +All ignored and untracked files are also stashed and then cleaned +up with `git clean`. -If the `--index` option is used, then tries to reinstate not only the working -tree's changes, but also the index's ones. However, this can fail, when you -have conflicts (which are stored in the index, where you therefore can no -longer apply the changes as they were originally). +-u:: +--include-untracked:: + This option is only valid for `push` and `save` commands. ++ +All untracked files are also stashed and then cleaned up with +`git clean`. -If the `--keep-index` option is used, all changes already added to the -index are left intact. +--index:: + This option is only valid for `pop` and `apply` commands. ++ +Tries to reinstate not only the working tree's changes, but also +the index's ones. However, this can fail, when you have conflicts +(which are stored in the index, where you therefore can no longer +apply the changes as they were originally). + +-k:: +--keep-index:: +--no-keep-index:: + This option is only valid for `push` and `save` commands. ++ +All changes already added to the index are left intact. -With `--patch`, you can interactively select hunks from the diff -between HEAD and the working tree to be stashed. The stash entry is -constructed such that its index state is the same as the index state -of your repository, and its worktree contains only the changes you -selected interactively. The selected changes are then rolled back -from your worktree. See the ``Interactive Mode'' section of -linkgit:git-add[1] to learn how to operate the `--patch` mode. +-p:: +--patch:: + This option is only valid for `push` and `save` commands. ++ +Interactively select hunks from the diff between HEAD and the +working tree to be stashed. The stash entry is constructed such +that its index state is the same as the index state of your +repository, and its worktree contains only the changes you selected +interactively. The selected changes are then rolled back from your +worktree. See the ``Interactive Mode'' section of linkgit:git-add[1] +to learn how to operate the `--patch` mode. + The `--patch` option implies `--keep-index`. You can use `--no-keep-index` to override this. -When pathspec is given to 'git stash push', the new stash entry records the -modified states only for the files that match the pathspec. The index -entries and working tree files are then rolled back to the state in -HEAD only for these files, too, leaving files that do not match the -pathspec intact. - -When no `` is given, `stash@{0}` is assumed, otherwise `` must -be a reference of the form `stash@{}`. +...:: + This option is only valid for `push` command. ++ +The new stash entry records the modified states only for the files +that match the pathspec. The index entries and working tree files +are then rolled back to the state in HEAD only for these files, +too, leaving files that do not match the pathspec intact. + +:: + This option is only valid for `apply`, `branch`, `drop`, `pop`, + `show` commands. ++ +A reference of the form `stash@{}`. When no `` is +given, the latest stash is assumed (that is, `stash@{0}`). DISCUSSION ---------- -- cgit v1.2.3 From b22909144ccb717e6f3c96562451fdfeca592c9c Mon Sep 17 00:00:00 2001 From: Alexandr Miloslavskiy Date: Mon, 17 Feb 2020 17:25:19 +0000 Subject: doc: stash: document more options Signed-off-by: Alexandr Miloslavskiy Signed-off-by: Junio C Hamano --- Documentation/git-stash.txt | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt index 69281fcf43..4303b5abc2 100644 --- a/Documentation/git-stash.txt +++ b/Documentation/git-stash.txt @@ -194,6 +194,18 @@ to learn how to operate the `--patch` mode. The `--patch` option implies `--keep-index`. You can use `--no-keep-index` to override this. +-q:: +--quiet:: + This option is only valid for `apply`, `drop`, `pop`, `push`, + `save`, `store` commands. ++ +Quiet, suppress feedback messages. + +\--:: + This option is only valid for `push` command. ++ +Separates pathspec from options for disambiguation purposes. + ...:: This option is only valid for `push` command. + -- cgit v1.2.3 From 3f3d8068f577ccbf11a9343fbfec3b7c679a6772 Mon Sep 17 00:00:00 2001 From: Alexandr Miloslavskiy Date: Mon, 17 Feb 2020 17:25:20 +0000 Subject: doc: stash: synchronize description This patch continues the effort that is already applied to `git commit`, `git reset`, `git checkout` etc. 1) Added reference to 'linkgit:gitglossary[7]'. 2) Fixed mentions of incorrectly plural "pathspecs". Signed-off-by: Alexandr Miloslavskiy Signed-off-by: Junio C Hamano --- Documentation/git-stash.txt | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt index 4303b5abc2..f10d8a2a5c 100644 --- a/Documentation/git-stash.txt +++ b/Documentation/git-stash.txt @@ -56,13 +56,13 @@ push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q For quickly making a snapshot, you can omit "push". In this mode, non-option arguments are not allowed to prevent a misspelled subcommand from making an unwanted stash entry. The two exceptions to this -are `stash -p` which acts as alias for `stash push -p` and pathspecs, +are `stash -p` which acts as alias for `stash push -p` and pathspec elements, which are allowed after a double hyphen `--` for disambiguation. save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] []:: This option is deprecated in favour of 'git stash push'. It - differs from "stash push" in that it cannot take pathspecs. + differs from "stash push" in that it cannot take pathspec. Instead, all non-option arguments are concatenated to form the stash message. @@ -213,6 +213,8 @@ The new stash entry records the modified states only for the files that match the pathspec. The index entries and working tree files are then rolled back to the state in HEAD only for these files, too, leaving files that do not match the pathspec intact. ++ +For more details, see the 'pathspec' entry in linkgit:gitglossary[7]. :: This option is only valid for `apply`, `branch`, `drop`, `pop`, -- cgit v1.2.3 From 8c3713cede71e4741dac546f787734627329f55b Mon Sep 17 00:00:00 2001 From: Alexandr Miloslavskiy Date: Mon, 17 Feb 2020 17:25:21 +0000 Subject: stash: eliminate crude option parsing Eliminate crude option parsing and rely on real parsing instead, because 1) Crude parsing is crude, for example it's not capable of handling things like `git stash -m Message` 2) Adding options in two places is inconvenient and prone to bugs As a side result, the case of `git stash -m Message` gets fixed. Also give a good error message instead of just throwing usage at user. ---- Some review of what's been happening to this code: Before [1], `git-stash.sh` only verified that all args begin with `-` : # The default command is "push" if nothing but options are given seen_non_option= for opt do case "$opt" in --) break ;; -*) ;; *) seen_non_option=t; break ;; esac done Later, [1] introduced the duplicate code I'm now removing, also making the previous test more strict by white-listing options. ---- [1] Commit 40af1468 ("stash: convert `stash--helper.c` into `stash.c`" 2019-02-26) Signed-off-by: Alexandr Miloslavskiy Signed-off-by: Junio C Hamano --- builtin/stash.c | 59 ++++++++++++++++++++------------------------------------ t/t3903-stash.sh | 5 +++++ 2 files changed, 26 insertions(+), 38 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index 4ad3adf4ba..7bc4c5d696 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1448,8 +1448,10 @@ done: return ret; } -static int push_stash(int argc, const char **argv, const char *prefix) +static int push_stash(int argc, const char **argv, const char *prefix, + int push_assumed) { + int force_assume = 0; int keep_index = -1; int patch_mode = 0; int include_untracked = 0; @@ -1471,10 +1473,22 @@ static int push_stash(int argc, const char **argv, const char *prefix) OPT_END() }; - if (argc) + if (argc) { + force_assume = !strcmp(argv[0], "-p"); argc = parse_options(argc, argv, prefix, options, git_stash_push_usage, - 0); + PARSE_OPT_KEEP_DASHDASH); + } + + if (argc) { + if (!strcmp(argv[0], "--")) { + argc--; + argv++; + } else if (push_assumed && !force_assume) { + die("subcommand wasn't specified; 'push' can't be assumed due to unexpected token '%s'", + argv[0]); + } + } parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN, prefix, argv); @@ -1547,7 +1561,6 @@ static int use_builtin_stash(void) int cmd_stash(int argc, const char **argv, const char *prefix) { - int i = -1; pid_t pid = getpid(); const char *index_file; struct argv_array args = ARGV_ARRAY_INIT; @@ -1580,7 +1593,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix) (uintmax_t)pid); if (!argc) - return !!push_stash(0, NULL, prefix); + return !!push_stash(0, NULL, prefix, 0); else if (!strcmp(argv[0], "apply")) return !!apply_stash(argc, argv, prefix); else if (!strcmp(argv[0], "clear")) @@ -1600,45 +1613,15 @@ int cmd_stash(int argc, const char **argv, const char *prefix) else if (!strcmp(argv[0], "create")) return !!create_stash(argc, argv, prefix); else if (!strcmp(argv[0], "push")) - return !!push_stash(argc, argv, prefix); + return !!push_stash(argc, argv, prefix, 0); else if (!strcmp(argv[0], "save")) return !!save_stash(argc, argv, prefix); else if (*argv[0] != '-') usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]), git_stash_usage, options); - if (strcmp(argv[0], "-p")) { - while (++i < argc && strcmp(argv[i], "--")) { - /* - * `akpqu` is a string which contains all short options, - * except `-m` which is verified separately. - */ - if ((strlen(argv[i]) == 2) && *argv[i] == '-' && - strchr("akpqu", argv[i][1])) - continue; - - if (!strcmp(argv[i], "--all") || - !strcmp(argv[i], "--keep-index") || - !strcmp(argv[i], "--no-keep-index") || - !strcmp(argv[i], "--patch") || - !strcmp(argv[i], "--quiet") || - !strcmp(argv[i], "--include-untracked")) - continue; - - /* - * `-m` and `--message=` are verified separately because - * they need to be immediately followed by a string - * (i.e.`-m"foobar"` or `--message="foobar"`). - */ - if (starts_with(argv[i], "-m") || - starts_with(argv[i], "--message=")) - continue; - - usage_with_options(git_stash_usage, options); - } - } - + /* Assume 'stash push' */ argv_array_push(&args, "push"); argv_array_pushv(&args, argv); - return !!push_stash(args.argc, args.argv, prefix); + return !!push_stash(args.argc, args.argv, prefix, 1); } diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index ea56e85e70..3ad23e2502 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -285,6 +285,11 @@ test_expect_success 'stash --no-keep-index' ' test bar,bar2 = $(cat file),$(cat file2) ' +test_expect_success 'dont assume push with non-option args' ' + test_must_fail git stash -q drop 2>err && + test_i18ngrep -e "subcommand wasn'\''t specified; '\''push'\'' can'\''t be assumed due to unexpected token '\''drop'\''" err +' + test_expect_success 'stash --invalid-option' ' echo bar5 >file && echo bar6 >file2 && -- cgit v1.2.3 From 8a98758a8d9dbd807fc2f1576e9141c1b720c5c1 Mon Sep 17 00:00:00 2001 From: Alexandr Miloslavskiy Date: Mon, 17 Feb 2020 17:25:22 +0000 Subject: stash push: support the --pathspec-from-file option Decisions taken for simplicity: 1) For now, `--pathspec-from-file` is declared incompatible with `--patch`, even when is not `-`. Such use case is not really expected. 2) It is not allowed to pass pathspec in both args and file. Signed-off-by: Alexandr Miloslavskiy Signed-off-by: Junio C Hamano --- Documentation/git-stash.txt | 20 ++++++++- builtin/stash.c | 20 +++++++++ t/t3909-stash-pathspec-file.sh | 100 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 139 insertions(+), 1 deletion(-) create mode 100755 t/t3909-stash-pathspec-file.sh diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt index f10d8a2a5c..31f1beb65b 100644 --- a/Documentation/git-stash.txt +++ b/Documentation/git-stash.txt @@ -15,6 +15,7 @@ SYNOPSIS 'git stash' branch [] 'git stash' [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet] [-u|--include-untracked] [-a|--all] [-m|--message ] + [--pathspec-from-file= [--pathspec-file-nul]] [--] [...]] 'git stash' clear 'git stash' create [] @@ -46,7 +47,7 @@ stash index (e.g. the integer `n` is equivalent to `stash@{n}`). COMMANDS -------- -push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message ] [--] [...]:: +push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message ] [--pathspec-from-file= [--pathspec-file-nul]] [--] [...]:: Save your local modifications to a new 'stash entry' and roll them back to HEAD (in the working tree and in the index). @@ -194,6 +195,23 @@ to learn how to operate the `--patch` mode. The `--patch` option implies `--keep-index`. You can use `--no-keep-index` to override this. +--pathspec-from-file=:: + This option is only valid for `push` command. ++ +Pathspec is passed in `` instead of commandline args. If +`` is exactly `-` then standard input is used. Pathspec +elements are separated by LF or CR/LF. Pathspec elements can be +quoted as explained for the configuration variable `core.quotePath` +(see linkgit:git-config[1]). See also `--pathspec-file-nul` and +global `--literal-pathspecs`. + +--pathspec-file-nul:: + This option is only valid for `push` command. ++ +Only meaningful with `--pathspec-from-file`. Pathspec elements are +separated with NUL character and all other characters are taken +literally (including newlines and quotes). + -q:: --quiet:: This option is only valid for `apply`, `drop`, `pop`, `push`, diff --git a/builtin/stash.c b/builtin/stash.c index 7bc4c5d696..74d92595a2 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -27,6 +27,7 @@ static const char * const git_stash_usage[] = { N_("git stash clear"), N_("git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n" " [-u|--include-untracked] [-a|--all] [-m|--message ]\n" + " [--pathspec-from-file= [--pathspec-file-nul]]\n" " [--] [...]]"), N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n" " [-u|--include-untracked] [-a|--all] []"), @@ -1456,7 +1457,9 @@ static int push_stash(int argc, const char **argv, const char *prefix, int patch_mode = 0; int include_untracked = 0; int quiet = 0; + int pathspec_file_nul = 0; const char *stash_msg = NULL; + const char *pathspec_from_file = NULL; struct pathspec ps; struct option options[] = { OPT_BOOL('k', "keep-index", &keep_index, @@ -1470,6 +1473,8 @@ static int push_stash(int argc, const char **argv, const char *prefix, N_("include ignore files"), 2), OPT_STRING('m', "message", &stash_msg, N_("message"), N_("stash message")), + OPT_PATHSPEC_FROM_FILE(&pathspec_from_file), + OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul), OPT_END() }; @@ -1492,6 +1497,21 @@ static int push_stash(int argc, const char **argv, const char *prefix, parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN, prefix, argv); + + if (pathspec_from_file) { + if (patch_mode) + die(_("--pathspec-from-file is incompatible with --patch")); + + if (ps.nr) + die(_("--pathspec-from-file is incompatible with pathspec arguments")); + + parse_pathspec_file(&ps, 0, + PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN, + prefix, pathspec_from_file, pathspec_file_nul); + } else if (pathspec_file_nul) { + die(_("--pathspec-file-nul requires --pathspec-from-file")); + } + return do_push_stash(&ps, stash_msg, quiet, keep_index, patch_mode, include_untracked); } diff --git a/t/t3909-stash-pathspec-file.sh b/t/t3909-stash-pathspec-file.sh new file mode 100755 index 0000000000..55e050cfd4 --- /dev/null +++ b/t/t3909-stash-pathspec-file.sh @@ -0,0 +1,100 @@ +#!/bin/sh + +test_description='stash --pathspec-from-file' + +. ./test-lib.sh + +test_tick + +test_expect_success setup ' + >fileA.t && + >fileB.t && + >fileC.t && + >fileD.t && + git add fileA.t fileB.t fileC.t fileD.t && + git commit -m "Files" && + + git tag checkpoint +' + +restore_checkpoint () { + git reset --hard checkpoint +} + +verify_expect () { + git stash show --name-status >actual && + test_cmp expect actual +} + +test_expect_success 'simplest' ' + restore_checkpoint && + + # More files are written to make sure that git didnt ignore + # --pathspec-from-file, stashing everything + echo A >fileA.t && + echo B >fileB.t && + echo C >fileC.t && + echo D >fileD.t && + + cat >expect <<-\EOF && + M fileA.t + EOF + + echo fileA.t | git stash push --pathspec-from-file=- && + verify_expect +' + +test_expect_success '--pathspec-file-nul' ' + restore_checkpoint && + + # More files are written to make sure that git didnt ignore + # --pathspec-from-file, stashing everything + echo A >fileA.t && + echo B >fileB.t && + echo C >fileC.t && + echo D >fileD.t && + + cat >expect <<-\EOF && + M fileA.t + M fileB.t + EOF + + printf "fileA.t\0fileB.t\0" | git stash push --pathspec-from-file=- --pathspec-file-nul && + verify_expect +' + +test_expect_success 'only touches what was listed' ' + restore_checkpoint && + + # More files are written to make sure that git didnt ignore + # --pathspec-from-file, stashing everything + echo A >fileA.t && + echo B >fileB.t && + echo C >fileC.t && + echo D >fileD.t && + + cat >expect <<-\EOF && + M fileB.t + M fileC.t + EOF + + printf "fileB.t\nfileC.t\n" | git stash push --pathspec-from-file=- && + verify_expect +' + +test_expect_success 'error conditions' ' + restore_checkpoint && + echo A >fileA.t && + echo fileA.t >list && + + test_must_fail git stash push --pathspec-from-file=list --patch 2>err && + test_i18ngrep -e "--pathspec-from-file is incompatible with --patch" err && + + test_must_fail git stash push --pathspec-from-file=list -- fileA.t 2>err && + test_i18ngrep -e "--pathspec-from-file is incompatible with pathspec arguments" err && + + test_must_fail git stash push --pathspec-file-nul 2>err && + test_i18ngrep -e "--pathspec-file-nul requires --pathspec-from-file" err +' + +test_done -- cgit v1.2.3 From 62e7a6f7a1427f461e72ccd5acebc200d55747e0 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 20 Feb 2020 15:15:15 +0100 Subject: parse-options: add testcases for OPT_CMDMODE() Before modifying the implementation, ensure that general operation of OPT_CMDMODE() and detection of incompatible options are covered. Signed-off-by: Paolo Bonzini Signed-off-by: Junio C Hamano --- t/helper/test-parse-options.c | 2 ++ t/t0040-parse-options.sh | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c index af82db06ac..2051ce57db 100644 --- a/t/helper/test-parse-options.c +++ b/t/helper/test-parse-options.c @@ -121,6 +121,8 @@ int cmd__parse_options(int argc, const char **argv) OPT_INTEGER('j', NULL, &integer, "get a integer, too"), OPT_MAGNITUDE('m', "magnitude", &magnitude, "get a magnitude"), OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23), + OPT_CMDMODE(0, "mode1", &integer, "set integer to 1 (cmdmode option)", 1), + OPT_CMDMODE(0, "mode2", &integer, "set integer to 2 (cmdmode option)", 2), OPT_CALLBACK('L', "length", &integer, "str", "get length of ", length_callback), OPT_FILENAME('F', "file", &file, "set file to "), diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index 705a136ed9..5c91464094 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -23,6 +23,8 @@ usage: test-tool parse-options -j get a integer, too -m, --magnitude get a magnitude --set23 set integer to 23 + --mode1 set integer to 1 (cmdmode option) + --mode2 set integer to 2 (cmdmode option) -L, --length get length of -F, --file set file to @@ -324,6 +326,22 @@ test_expect_success 'OPT_NEGBIT() works' ' test-tool parse-options --expect="boolean: 6" -bb --no-neg-or4 ' +test_expect_success 'OPT_CMDMODE() works' ' + test-tool parse-options --expect="integer: 1" --mode1 +' + +test_expect_success 'OPT_CMDMODE() detects incompatibility' ' + test_must_fail test-tool parse-options --mode1 --mode2 >output 2>output.err && + test_must_be_empty output && + test_i18ngrep "incompatible with --mode" output.err +' + +test_expect_success 'OPT_CMDMODE() detects incompatibility with something else' ' + test_must_fail test-tool parse-options --set23 --mode2 >output 2>output.err && + test_must_be_empty output && + test_i18ngrep "incompatible with something else" output.err +' + test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' ' test-tool parse-options --expect="boolean: 6" + + + + + + ' -- cgit v1.2.3 From bc8620b44094e9fb6fc20d141678f9b845c19193 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 20 Feb 2020 15:15:16 +0100 Subject: parse-options: convert "command mode" to a flag OPTION_CMDMODE is essentially OPTION_SET_INT plus an extra check that the variable had not set before. In order to allow custom processing of the option, for example a "command mode" option that also has an argument, it would be nice to use OPTION_CALLBACK and not have to rewrite the extra check on incompatible options. In other words, making the processing of the option orthogonal to the "only one of these" behavior provided by OPTION_CMDMODE. Add a new flag that takes care of the check, and modify OPT_CMDMODE to use it together with OPTION_SET_INT. The new flag still requires that the option value points to an int, but any OPTION_* value can be specified as long as it does not require a non-int type for opt->value. Signed-off-by: Paolo Bonzini Signed-off-by: Junio C Hamano --- parse-options.c | 20 +++++++++----------- parse-options.h | 8 ++++---- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/parse-options.c b/parse-options.c index 60fae3ad21..c51be1860f 100644 --- a/parse-options.c +++ b/parse-options.c @@ -61,7 +61,7 @@ static enum parse_opt_result opt_command_mode_error( */ for (that = all_opts; that->type != OPTION_END; that++) { if (that == opt || - that->type != OPTION_CMDMODE || + !(that->flags & PARSE_OPT_CMDMODE) || that->value != opt->value || that->defval != *(int *)opt->value) continue; @@ -95,6 +95,14 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p, if (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG)) return error(_("%s takes no value"), optname(opt, flags)); + /* + * Giving the same mode option twice, although unnecessary, + * is not a grave error, so let it pass. + */ + if ((opt->flags & PARSE_OPT_CMDMODE) && + *(int *)opt->value && *(int *)opt->value != opt->defval) + return opt_command_mode_error(opt, all_opts, flags); + switch (opt->type) { case OPTION_LOWLEVEL_CALLBACK: return opt->ll_callback(p, opt, NULL, unset); @@ -130,16 +138,6 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p, *(int *)opt->value = unset ? 0 : opt->defval; return 0; - case OPTION_CMDMODE: - /* - * Giving the same mode option twice, although is unnecessary, - * is not a grave error, so let it pass. - */ - if (*(int *)opt->value && *(int *)opt->value != opt->defval) - return opt_command_mode_error(opt, all_opts, flags); - *(int *)opt->value = opt->defval; - return 0; - case OPTION_STRING: if (unset) *(const char **)opt->value = NULL; diff --git a/parse-options.h b/parse-options.h index fdc0c1cb97..05bf8245de 100644 --- a/parse-options.h +++ b/parse-options.h @@ -18,7 +18,6 @@ enum parse_opt_type { OPTION_BITOP, OPTION_COUNTUP, OPTION_SET_INT, - OPTION_CMDMODE, /* options with arguments (usually) */ OPTION_STRING, OPTION_INTEGER, @@ -47,7 +46,8 @@ enum parse_opt_option_flags { PARSE_OPT_LITERAL_ARGHELP = 64, PARSE_OPT_SHELL_EVAL = 256, PARSE_OPT_NOCOMPLETE = 512, - PARSE_OPT_COMP_ARG = 1024 + PARSE_OPT_COMP_ARG = 1024, + PARSE_OPT_CMDMODE = 2048 }; enum parse_opt_result { @@ -168,8 +168,8 @@ struct option { #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0) #define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \ (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1} -#define OPT_CMDMODE(s, l, v, h, i) { OPTION_CMDMODE, (s), (l), (v), NULL, \ - (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) } +#define OPT_CMDMODE(s, l, v, h, i) { OPTION_SET_INT, (s), (l), (v), NULL, \ + (h), PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) } #define OPT_INTEGER(s, l, v, h) OPT_INTEGER_F(s, l, v, h, 0) #define OPT_MAGNITUDE(s, l, v, h) { OPTION_MAGNITUDE, (s), (l), (v), \ N_("n"), (h), PARSE_OPT_NONEG } -- cgit v1.2.3 From e8ef1e8d6edd835fcea7f88d2740d756a1353190 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 20 Feb 2020 15:15:17 +0100 Subject: am: convert "resume" variable to a struct This will allow stashing the submode of --show-current-patch from a callback function. Using a struct will allow accessing both fields from outside cmd_am (through container_of). Signed-off-by: Paolo Bonzini Signed-off-by: Junio C Hamano --- builtin/am.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 8181c2aef3..bd3cda8bec 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2118,7 +2118,7 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, int return 0; } -enum resume_mode { +enum resume_type { RESUME_FALSE = 0, RESUME_APPLY, RESUME_RESOLVED, @@ -2128,6 +2128,10 @@ enum resume_mode { RESUME_SHOW_PATCH }; +struct resume_mode { + enum resume_type mode; +}; + static int git_am_config(const char *k, const char *v, void *cb) { int status; @@ -2145,7 +2149,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) int binary = -1; int keep_cr = -1; int patch_format = PATCH_FORMAT_UNKNOWN; - enum resume_mode resume = RESUME_FALSE; + struct resume_mode resume = { .mode = RESUME_FALSE }; int in_progress; int ret = 0; @@ -2214,22 +2218,22 @@ int cmd_am(int argc, const char **argv, const char *prefix) PARSE_OPT_NOARG), OPT_STRING(0, "resolvemsg", &state.resolvemsg, NULL, N_("override error message when patch failure occurs")), - OPT_CMDMODE(0, "continue", &resume, + OPT_CMDMODE(0, "continue", &resume.mode, N_("continue applying patches after resolving a conflict"), RESUME_RESOLVED), - OPT_CMDMODE('r', "resolved", &resume, + OPT_CMDMODE('r', "resolved", &resume.mode, N_("synonyms for --continue"), RESUME_RESOLVED), - OPT_CMDMODE(0, "skip", &resume, + OPT_CMDMODE(0, "skip", &resume.mode, N_("skip the current patch"), RESUME_SKIP), - OPT_CMDMODE(0, "abort", &resume, + OPT_CMDMODE(0, "abort", &resume.mode, N_("restore the original branch and abort the patching operation."), RESUME_ABORT), - OPT_CMDMODE(0, "quit", &resume, + OPT_CMDMODE(0, "quit", &resume.mode, N_("abort the patching operation but keep HEAD where it is."), RESUME_QUIT), - OPT_CMDMODE(0, "show-current-patch", &resume, + OPT_CMDMODE(0, "show-current-patch", &resume.mode, N_("show the patch being applied."), RESUME_SHOW_PATCH), OPT_BOOL(0, "committer-date-is-author-date", @@ -2281,12 +2285,12 @@ int cmd_am(int argc, const char **argv, const char *prefix) * intend to feed us a patch but wanted to continue * unattended. */ - if (argc || (resume == RESUME_FALSE && !isatty(0))) + if (argc || (resume.mode == RESUME_FALSE && !isatty(0))) die(_("previous rebase directory %s still exists but mbox given."), state.dir); - if (resume == RESUME_FALSE) - resume = RESUME_APPLY; + if (resume.mode == RESUME_FALSE) + resume.mode = RESUME_APPLY; if (state.signoff == SIGNOFF_EXPLICIT) am_append_signoff(&state); @@ -2300,7 +2304,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) * stray directories. */ if (file_exists(state.dir) && !state.rebasing) { - if (resume == RESUME_ABORT || resume == RESUME_QUIT) { + if (resume.mode == RESUME_ABORT || resume.mode == RESUME_QUIT) { am_destroy(&state); am_state_release(&state); return 0; @@ -2311,7 +2315,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) state.dir); } - if (resume) + if (resume.mode) die(_("Resolve operation not in progress, we are not resuming.")); for (i = 0; i < argc; i++) { @@ -2329,7 +2333,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) argv_array_clear(&paths); } - switch (resume) { + switch (resume.mode) { case RESUME_FALSE: am_run(&state, 0); break; -- cgit v1.2.3 From f3b4822899dc4ddca688de285f01e1d12aeb33b6 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 20 Feb 2020 15:15:18 +0100 Subject: am: support --show-current-patch=raw as a synonym for--show-current-patch When "git am --show-current-patch" was added in commit 984913a210 ("am: add --show-current-patch", 2018-02-12), "git am" started recommending it as a replacement for .git/rebase-merge/patch. Unfortunately the suggestion is somewhat misguided; for example, the output "git am --show-current-patch" cannot be passed to "git apply" if it is encoded as quoted-printable or base64. To simplify worktree operations and to avoid that users poke into .git, it would be better if "git am" also provided a mode that copies .git/rebase-merge/patch to stdout. One possibility could be to have completely separate options, introducing for example --show-current-message (for .git/rebase-apply/NNNN) and --show-current-diff (for .git/rebase-apply/patch), while possibly deprecating --show-current-patch. That would even remove the need for the first two patches in the series. However, the long common prefix would have prevented using an abbreviated option such as "--show". Therefore, I chose instead to add a string argument to --show-current-patch. The new argument is optional, so that "git am --show-current-patch"'s behavior remains backwards-compatible. The next choice to make is how to handle multiple --show-current-patch options. Right now, something like "git am --abort --show-current-patch" is rejected, and the previous suggestion would likewise have naturally rejected a command line like git am --show-current-message --show-current-diff Therefore, I decided to also reject for example git am --show-current-patch=diff --show-current-patch=raw In other words the whole of --show-current-patch=xxx (including the optional argument) is treated as the command mode. I found this to be more consistent and intuitive, even though it differs from the usual "last one wins" semantics of the git command line. Add the code to parse submodes based on the above design, where for now "raw" is the only valid submode. "raw" prints the full e-mail message just like "git am --show-current-patch". Signed-off-by: Paolo Bonzini Signed-off-by: Junio C Hamano --- Documentation/git-am.txt | 9 +++--- builtin/am.c | 59 ++++++++++++++++++++++++++++++---- contrib/completion/git-completion.bash | 5 +++ t/t4150-am.sh | 10 ++++++ 4 files changed, 73 insertions(+), 10 deletions(-) diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt index 11ca61b00b..590b711536 100644 --- a/Documentation/git-am.txt +++ b/Documentation/git-am.txt @@ -16,7 +16,7 @@ SYNOPSIS [--exclude=] [--include=] [--reject] [-q | --quiet] [--[no-]scissors] [-S[]] [--patch-format=] [( | )...] -'git am' (--continue | --skip | --abort | --quit | --show-current-patch) +'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=raw]) DESCRIPTION ----------- @@ -176,9 +176,10 @@ default. You can use `--no-utf8` to override this. Abort the patching operation but keep HEAD and the index untouched. ---show-current-patch:: - Show the entire e-mail message "git am" has stopped at, because - of conflicts. +--show-current-patch[=raw]:: + Show the raw contents of the e-mail message at which `git am` + has stopped due to conflicts. The argument must be omitted or + `raw`. DISCUSSION ---------- diff --git a/builtin/am.c b/builtin/am.c index bd3cda8bec..54b04da86d 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -81,6 +81,10 @@ enum signoff_type { SIGNOFF_EXPLICIT /* --signoff was set on the command-line */ }; +enum show_patch_type { + SHOW_PATCH_RAW = 0, +}; + struct am_state { /* state directory path */ char *dir; @@ -2061,7 +2065,7 @@ static void am_abort(struct am_state *state) am_destroy(state); } -static int show_patch(struct am_state *state) +static int show_patch(struct am_state *state, enum show_patch_type sub_mode) { struct strbuf sb = STRBUF_INIT; const char *patch_path; @@ -2078,7 +2082,14 @@ static int show_patch(struct am_state *state) return ret; } - patch_path = am_path(state, msgnum(state)); + switch (sub_mode) { + case SHOW_PATCH_RAW: + patch_path = am_path(state, msgnum(state)); + break; + default: + BUG("invalid mode for --show-current-patch"); + } + len = strbuf_read_file(&sb, patch_path, 0); if (len < 0) die_errno(_("failed to read '%s'"), patch_path); @@ -2130,8 +2141,42 @@ enum resume_type { struct resume_mode { enum resume_type mode; + enum show_patch_type sub_mode; }; +static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset) +{ + int *opt_value = opt->value; + struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode); + + /* + * Please update $__git_showcurrentpatch in git-completion.bash + * when you add new options + */ + const char *valid_modes[] = { + [SHOW_PATCH_RAW] = "raw" + }; + int new_value = SHOW_PATCH_RAW; + + if (arg) { + for (new_value = 0; new_value < ARRAY_SIZE(valid_modes); new_value++) { + if (!strcmp(arg, valid_modes[new_value])) + break; + } + if (new_value >= ARRAY_SIZE(valid_modes)) + return error(_("Invalid value for --show-current-patch: %s"), arg); + } + + if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode) + return error(_("--show-current-patch=%s is incompatible with " + "--show-current-patch=%s"), + arg, valid_modes[resume->sub_mode]); + + resume->mode = RESUME_SHOW_PATCH; + resume->sub_mode = new_value; + return 0; +} + static int git_am_config(const char *k, const char *v, void *cb) { int status; @@ -2233,9 +2278,11 @@ int cmd_am(int argc, const char **argv, const char *prefix) OPT_CMDMODE(0, "quit", &resume.mode, N_("abort the patching operation but keep HEAD where it is."), RESUME_QUIT), - OPT_CMDMODE(0, "show-current-patch", &resume.mode, - N_("show the patch being applied."), - RESUME_SHOW_PATCH), + { OPTION_CALLBACK, 0, "show-current-patch", &resume.mode, + "raw", + N_("show the patch being applied"), + PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, + parse_opt_show_current_patch, RESUME_SHOW_PATCH }, OPT_BOOL(0, "committer-date-is-author-date", &state.committer_date_is_author_date, N_("lie about committer date")), @@ -2354,7 +2401,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) am_destroy(&state); break; case RESUME_SHOW_PATCH: - ret = show_patch(&state); + ret = show_patch(&state, resume.sub_mode); break; default: BUG("invalid resume value"); diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index e4d9ff4a95..2673fc162c 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1180,6 +1180,7 @@ __git_count_arguments () __git_whitespacelist="nowarn warn error error-all fix" __git_patchformat="mbox stgit stgit-series hg mboxrd" +__git_showcurrentpatch="raw" __git_am_inprogress_options="--skip --continue --resolved --abort --quit --show-current-patch" _git_am () @@ -1198,6 +1199,10 @@ _git_am () __gitcomp "$__git_patchformat" "" "${cur##--patch-format=}" return ;; + --show-current-patch=*) + __gitcomp "$__git_showcurrentpatch" "" "${cur##--show-current-patch=}" + return + ;; --*) __gitcomp_builtin am "" \ "$__git_am_inprogress_options" diff --git a/t/t4150-am.sh b/t/t4150-am.sh index 4f1e24ecbe..afe456e75e 100755 --- a/t/t4150-am.sh +++ b/t/t4150-am.sh @@ -666,6 +666,16 @@ test_expect_success 'am --show-current-patch' ' test_cmp .git/rebase-apply/0001 actual.patch ' +test_expect_success 'am --show-current-patch=raw' ' + git am --show-current-patch=raw >actual.patch && + test_cmp .git/rebase-apply/0001 actual.patch +' + +test_expect_success 'am accepts repeated --show-current-patch' ' + git am --show-current-patch --show-current-patch=raw >actual.patch && + test_cmp .git/rebase-apply/0001 actual.patch +' + test_expect_success 'am --skip works' ' echo goodbye >expected && git am --skip && -- cgit v1.2.3 From aa416b22ea73321aba94c3c96db0491fb25ea7ea Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 20 Feb 2020 15:15:19 +0100 Subject: am: support --show-current-patch=diff to retrieve .git/rebase-apply/patch When "git am --show-current-patch" was added in commit 984913a210 ("am: add --show-current-patch", 2018-02-12), "git am" started recommending it as a replacement for .git/rebase-merge/patch. Unfortunately the suggestion is somewhat misguided; for example, the output of "git am --show-current-patch" cannot be passed to "git apply" if it is encoded as quoted-printable or base64. Add a new mode to "git am --show-current-patch" in order to straighten the suggestion. Reported-by: J. Bruce Fields Signed-off-by: Paolo Bonzini Signed-off-by: Junio C Hamano --- Documentation/git-am.txt | 11 ++++++----- builtin/am.c | 9 +++++++-- contrib/completion/git-completion.bash | 2 +- t/t4150-am.sh | 10 ++++++++++ 4 files changed, 24 insertions(+), 8 deletions(-) diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt index 590b711536..ab5754e05d 100644 --- a/Documentation/git-am.txt +++ b/Documentation/git-am.txt @@ -16,7 +16,7 @@ SYNOPSIS [--exclude=] [--include=] [--reject] [-q | --quiet] [--[no-]scissors] [-S[]] [--patch-format=] [( | )...] -'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=raw]) +'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=(diff|raw)]) DESCRIPTION ----------- @@ -176,10 +176,11 @@ default. You can use `--no-utf8` to override this. Abort the patching operation but keep HEAD and the index untouched. ---show-current-patch[=raw]:: - Show the raw contents of the e-mail message at which `git am` - has stopped due to conflicts. The argument must be omitted or - `raw`. +--show-current-patch[=(diff|raw)]:: + Show the message at which `git am` has stopped due to + conflicts. If `raw` is specified, show the raw contents of + the e-mail message; if `diff`, show the diff portion only. + Defaults to `raw`. DISCUSSION ---------- diff --git a/builtin/am.c b/builtin/am.c index 54b04da86d..e3dfd93c25 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -83,6 +83,7 @@ enum signoff_type { enum show_patch_type { SHOW_PATCH_RAW = 0, + SHOW_PATCH_DIFF = 1, }; struct am_state { @@ -1767,7 +1768,7 @@ static void am_run(struct am_state *state, int resume) linelen(state->msg), state->msg); if (advice_amworkdir) - advise(_("Use 'git am --show-current-patch' to see the failed patch")); + advise(_("Use 'git am --show-current-patch=diff' to see the failed patch")); die_user_resolve(state); } @@ -2086,6 +2087,9 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode) case SHOW_PATCH_RAW: patch_path = am_path(state, msgnum(state)); break; + case SHOW_PATCH_DIFF: + patch_path = am_path(state, "patch"); + break; default: BUG("invalid mode for --show-current-patch"); } @@ -2154,6 +2158,7 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar * when you add new options */ const char *valid_modes[] = { + [SHOW_PATCH_DIFF] = "diff", [SHOW_PATCH_RAW] = "raw" }; int new_value = SHOW_PATCH_RAW; @@ -2279,7 +2284,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) N_("abort the patching operation but keep HEAD where it is."), RESUME_QUIT), { OPTION_CALLBACK, 0, "show-current-patch", &resume.mode, - "raw", + "(diff|raw)", N_("show the patch being applied"), PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, parse_opt_show_current_patch, RESUME_SHOW_PATCH }, diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 2673fc162c..b33c23b717 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1180,7 +1180,7 @@ __git_count_arguments () __git_whitespacelist="nowarn warn error error-all fix" __git_patchformat="mbox stgit stgit-series hg mboxrd" -__git_showcurrentpatch="raw" +__git_showcurrentpatch="diff raw" __git_am_inprogress_options="--skip --continue --resolved --abort --quit --show-current-patch" _git_am () diff --git a/t/t4150-am.sh b/t/t4150-am.sh index afe456e75e..cb45271457 100755 --- a/t/t4150-am.sh +++ b/t/t4150-am.sh @@ -671,11 +671,21 @@ test_expect_success 'am --show-current-patch=raw' ' test_cmp .git/rebase-apply/0001 actual.patch ' +test_expect_success 'am --show-current-patch=diff' ' + git am --show-current-patch=diff >actual.patch && + test_cmp .git/rebase-apply/patch actual.patch +' + test_expect_success 'am accepts repeated --show-current-patch' ' git am --show-current-patch --show-current-patch=raw >actual.patch && test_cmp .git/rebase-apply/0001 actual.patch ' +test_expect_success 'am detects incompatible --show-current-patch' ' + test_must_fail git am --show-current-patch=raw --show-current-patch=diff && + test_must_fail git am --show-current-patch --show-current-patch=diff +' + test_expect_success 'am --skip works' ' echo goodbye >expected && git am --skip && -- cgit v1.2.3 From a51d9e8f07652ded830cb50187be74c1709816d8 Mon Sep 17 00:00:00 2001 From: Rasmus Jonsson Date: Sun, 23 Feb 2020 01:50:49 +0100 Subject: t1050: replace test -f with test_path_is_f