git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
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
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

  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 " 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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ http://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git