From: "Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>,
Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Subject: [PATCH v3 0/8] Support --pathspec-from-file in rm, stash
Date: Mon, 17 Feb 2020 17:25:14 +0000 [thread overview]
Message-ID: <pull.530.v3.git.1581960322.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.530.v2.git.1581345948.gitgitgadget@gmail.com>
Changes since V1
------------------
Some polishing based on code review in V1
1) Improved error message for the case where pathspec is not given to `git rm`
2) Removed explicit variable initialization to 0 / NULL
3) Polishing in docs for `git stash`
------------------
This series continues the effort to support `--pathspec-from-file`
in various git commands. Series already in `master`: [1][2]
Cc'ing Paul-Sebastian Ungureanu because I touched his git stash code.
[1] https://public-inbox.org/git/pull.445.git.1572895605.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/pull.490.git.1576161385.gitgitgadget@gmail.com/
Alexandr Miloslavskiy (8):
doc: rm: synchronize <pathspec> description
rm: support the --pathspec-from-file option
doc: stash: split options from description (1)
doc: stash: split options from description (2)
doc: stash: document more options
doc: stash: synchronize <pathspec> description
stash: eliminate crude option parsing
stash push: support the --pathspec-from-file option
Documentation/git-rm.txt | 61 +++++++-------
Documentation/git-stash.txt | 144 +++++++++++++++++++++++----------
builtin/rm.c | 28 +++++--
builtin/stash.c | 79 +++++++++---------
t/t3601-rm-pathspec-file.sh | 79 ++++++++++++++++++
t/t3903-stash.sh | 5 ++
t/t3909-stash-pathspec-file.sh | 100 +++++++++++++++++++++++
7 files changed, 381 insertions(+), 115 deletions(-)
create mode 100755 t/t3601-rm-pathspec-file.sh
create mode 100755 t/t3909-stash-pathspec-file.sh
base-commit: de93cc14ab7e8db7645d8dbe4fd2603f76d5851f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-530%2FSyntevoAlex%2F%230207(git)_pathspec_from_file_3-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-530/SyntevoAlex/#0207(git)_pathspec_from_file_3-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/530
Range-diff vs v2:
1: 2e8c8ad8158 = 1: cf065e905dc doc: rm: synchronize <pathspec> description
2: 7ccbab52e51 ! 2: 7c657dea89e rm: support the --pathspec-from-file option
@@ -5,17 +5,36 @@
Decisions taken for simplicity:
1) It is not allowed to pass pathspec in both args and file.
- `if (!argc)` block was adapted to work with --pathspec-from-file. For
- that, I also had to parse pathspec earlier. Now it happens before
- `read_cache()` / `hold_locked_index()` / `setup_work_tree()`, which
- sounds fine to me.
+ Adjustments were needed for `if (!argc)` block:
- In case of empty pathspec, there is now a clear error message instead
- of showing usage. As a consequence, exit code has also changed. Judging
- from [1] it doesn't seem that showing usage in this case was important
- (according to commit message, it was to avoid segfault), and it doesn't
- fit into how other commands react to empty pathspec. Finally, the new
- error message is easier to understand.
+ 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)
@@ -136,7 +155,7 @@
+ echo D >fileD.t &&
+ git add fileA.t fileB.t fileC.t fileD.t &&
+ git commit -m "files" &&
-+
++
+ git tag checkpoint
+'
+
@@ -193,7 +212,7 @@
+
+ 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
3: 8c212fc0ed4 = 3: bb300215d49 doc: stash: split options from description (1)
4: db3a96720ce = 4: fdaf4532404 doc: stash: split options from description (2)
5: f91ec08b472 = 5: 764b8668d10 doc: stash: document more options
6: 04e2fd5865f = 6: 7353b06e30e doc: stash: synchronize <pathspec> description
7: 0558cbbe38e = 7: d34eaf4a272 stash: eliminate crude option parsing
8: 0c6f28dc68d = 8: 6465a292b55 stash push: support the --pathspec-from-file option
--
gitgitgadget
next prev parent reply other threads:[~2020-02-17 17:25 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-16 16:09 [PATCH 0/8] Support --pathspec-from-file in rm, stash Alexandr Miloslavskiy via GitGitGadget
2020-01-16 16:09 ` [PATCH 1/8] doc: rm: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2020-01-21 19:14 ` Junio C Hamano
2020-01-16 16:09 ` [PATCH 2/8] rm: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2020-01-21 19:36 ` Junio C Hamano
2020-02-10 14:46 ` Alexandr Miloslavskiy
2020-02-10 18:48 ` Junio C Hamano
2020-02-17 17:25 ` Alexandr Miloslavskiy
2020-01-16 16:09 ` [PATCH 3/8] doc: stash: split options from description (1) Alexandr Miloslavskiy via GitGitGadget
2020-01-16 16:09 ` [PATCH 4/8] doc: stash: split options from description (2) Alexandr Miloslavskiy via GitGitGadget
2020-01-21 20:21 ` Junio C Hamano
2020-02-10 14:47 ` Alexandr Miloslavskiy
2020-01-16 16:09 ` [PATCH 5/8] doc: stash: document more options Alexandr Miloslavskiy via GitGitGadget
2020-01-21 20:29 ` Junio C Hamano
2020-01-16 16:09 ` [PATCH 6/8] doc: stash: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2020-01-21 20:29 ` Junio C Hamano
2020-01-16 16:09 ` [PATCH 7/8] stash: eliminate crude option parsing Alexandr Miloslavskiy via GitGitGadget
2020-01-16 16:09 ` [PATCH 8/8] stash push: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2020-02-10 14:45 ` [PATCH v2 0/8] Support --pathspec-from-file in rm, stash Alexandr Miloslavskiy via GitGitGadget
2020-02-10 14:45 ` [PATCH v2 1/8] doc: rm: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2020-02-10 14:45 ` [PATCH v2 2/8] rm: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2020-02-10 20:39 ` Junio C Hamano
2020-02-17 17:27 ` Alexandr Miloslavskiy
2020-02-17 17:59 ` Junio C Hamano
2020-02-17 21:03 ` Junio C Hamano
2020-02-10 14:45 ` [PATCH v2 3/8] doc: stash: split options from description (1) Alexandr Miloslavskiy via GitGitGadget
2020-02-10 14:45 ` [PATCH v2 4/8] doc: stash: split options from description (2) Alexandr Miloslavskiy via GitGitGadget
2020-02-10 14:45 ` [PATCH v2 5/8] doc: stash: document more options Alexandr Miloslavskiy via GitGitGadget
2020-02-10 14:45 ` [PATCH v2 6/8] doc: stash: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2020-02-10 14:45 ` [PATCH v2 7/8] stash: eliminate crude option parsing Alexandr Miloslavskiy via GitGitGadget
2020-02-10 14:45 ` [PATCH v2 8/8] stash push: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2020-02-17 17:25 ` Alexandr Miloslavskiy via GitGitGadget [this message]
2020-02-17 17:25 ` [PATCH v3 1/8] doc: rm: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2020-02-17 17:25 ` [PATCH v3 2/8] rm: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2020-02-17 18:06 ` Alexandr Miloslavskiy
2020-02-17 17:25 ` [PATCH v3 3/8] doc: stash: split options from description (1) Alexandr Miloslavskiy via GitGitGadget
2020-02-17 17:25 ` [PATCH v3 4/8] doc: stash: split options from description (2) Alexandr Miloslavskiy via GitGitGadget
2020-02-17 17:25 ` [PATCH v3 5/8] doc: stash: document more options Alexandr Miloslavskiy via GitGitGadget
2020-02-17 17:25 ` [PATCH v3 6/8] doc: stash: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2020-02-17 17:25 ` [PATCH v3 7/8] stash: eliminate crude option parsing Alexandr Miloslavskiy via GitGitGadget
2020-02-17 17:25 ` [PATCH v3 8/8] stash push: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=pull.530.v3.git.1581960322.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=alexandr.miloslavskiy@syntevo.com \
--cc=git@vger.kernel.org \
--cc=ungureanupaulsebastian@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).