[v3,0/8] Support --pathspec-from-file in rm, stash

Message

Linus Arver via GitGitGadget Feb. 17, 2020, 5:25 p.m. UTC

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/[e-mail]/
[2] https://lore.kernel.org/git/[e-mail]/

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