Skip to content

Support --pathspec-from-file in rm, stash #530

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed

Support --pathspec-from-file in rm, stash #530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cf065e9
doc: rm: synchronize <pathspec> description
SyntevoAlex Jan 13, 2020
7c657de
rm: support the --pathspec-from-file option
SyntevoAlex Jan 13, 2020
bb30021
doc: stash: split options from description (1)
SyntevoAlex Jan 16, 2020
fdaf453
doc: stash: split options from description (2)
SyntevoAlex Jan 14, 2020
764b866
doc: stash: document more options
SyntevoAlex Jan 14, 2020
7353b06
doc: stash: synchronize <pathspec> description
SyntevoAlex Jan 14, 2020
d34eaf4
stash: eliminate crude option parsing
SyntevoAlex Jan 15, 2020
6465a29
stash push: support the --pathspec-from-file option
SyntevoAlex Jan 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 34 additions & 27 deletions Documentation/git-rm.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,18 @@ git-rm - Remove files from the working tree and from the index
SYNOPSIS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>
writes:

> From: Alexandr Miloslavskiy <[email protected]>
>
> 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 `<patchspec>`.

Thanks.  Will queue.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>
writes:

> From: Alexandr Miloslavskiy <[email protected]>
>
> 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.

That is not an explanation nor justification.

> In case of empty pathspec, there is now a clear error message instead
> of showing usage.

Hmph, "git rm --pathspec-from-file=/dev/null" would say "nothing
specified, nothing removed" and it makes perfect sense, but I am not
sure "git rm" that gives the same message is better than the output
by usage_with_options(builtin_rm_usage, builtin_rm_options).

> -'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch] [--quiet] [--] <pathspec>...
> +'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch]
> +	  [--quiet] [--pathspec-from-file=<file> [--pathspec-file-nul]]
> +	  [--] [<pathspec>...]

OK.

> +--pathspec-from-file=<file>::
> +	Pathspec is passed in `<file>` instead of commandline args. If
> +	`<file>` 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).
> +

OK.

> diff --git a/builtin/rm.c b/builtin/rm.c
> index 19ce95a901..8e40795751 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 = 0;
> +static char *pathspec_from_file = NULL;

We may want to clean these "explicitly initialize to 0/NULL" up at
some point.  The clean-up itself would not be in the scope of this
patch, of course, but not making it worse is something this patch
can do to help.

> @@ -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(_("Nothing specified, nothing removed"));

I wonder if doing these in this order instead would make more sense
without making unnecessary behaviour change.

    - parse the options (which would make pathspec_f_f available to
      us)

    - if pathspec_f_f is given, call parse_pathspec_file()

    - otherwise complain if pathspec_file_nul is set

    - otherwise check argc and give the usage_with_options()

I dunno.

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Alexandr Miloslavskiy wrote (reply to this):

Sorry for late reply, I was on vacation. Now I'm back and ready to 
continue :)

Thanks for your review!

On 21.01.2020 20:36, Junio C Hamano wrote:
>> 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.
> 
> That is not an explanation nor justification.

I'm not exactly sure what are you suggesting. My best guess is that you 
want to remove "`if (!argc)` block was adapted" paragraph from commit 
message? I thought about it and it feels wrong to leave this change 
unexplained. Or are you suggesting to reword it? If so, please give a hint.

>> In case of empty pathspec, there is now a clear error message instead
>> of showing usage.
> 
> Hmph, "git rm --pathspec-from-file=/dev/null" would say "nothing
> specified, nothing removed" and it makes perfect sense, but I am not
> sure "git rm" that gives the same message is better than the output
> by usage_with_options(builtin_rm_usage, builtin_rm_options).

What feels wrong to me is when I make a mistake and git just slams me 
with usage, and then it's up to me to figure what could be wrong. I 
myself struggled to find a mistake a couple times (in similar cases, not 
in this specific one) and didn't like the experience.

This could be a lot worse when there's no mistake, just the file was 
empty - but you already agreed that showing a new error message is 
reasonable with '--pathspec-from-file'.

Still, without '--pathspec-from-file', it should still be better to 
point to a specific error rather then "here's usage and try to find a 
difference". I have reworded the error message in V2 in hopes that it 
will be less controversial.

If you still don't like it, I will change it to only show the new error 
with '--pathspec-from-file'.

>> +static int ignore_unmatch = 0, pathspec_file_nul = 0;
>> +static char *pathspec_from_file = NULL;
> 
> We may want to clean these "explicitly initialize to 0/NULL" up at
> some point.  The clean-up itself would not be in the scope of this
> patch, of course, but not making it worse is something this patch
> can do to help.

Changed in V2.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Alexandr Miloslavskiy <[email protected]> writes:

> Sorry for late reply, I was on vacation. Now I'm back and ready to
> continue :)
>
> Thanks for your review!
>
> On 21.01.2020 20:36, Junio C Hamano wrote:
>>> 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.
>>
>> That is not an explanation nor justification.
>
> I'm not exactly sure what are you suggesting.

I expected that the proposed log message to explain and justify why
a change (in behaviour, in design, etc.) is made.  "There is this
limitation" is not a justification---"because of such and such
reasons, there is this limitation, otherwise such and such bad
things happen" is.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Alexandr Miloslavskiy wrote (reply to this):

On 10.02.2020 19:48, Junio C Hamano wrote:
> I expected that the proposed log message to explain and justify why
> a change (in behaviour, in design, etc.) is made.  "There is this
> limitation" is not a justification---"because of such and such
> reasons, there is this limitation, otherwise such and such bad
> things happen" is.

I have rewritten the commit message in V3. Is that better?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>
writes:

> diff --git a/t/t3601-rm-pathspec-file.sh b/t/t3601-rm-pathspec-file.sh
> new file mode 100755
> index 0000000000..4542a0f02f
> --- /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" &&
> +	

Trailing whitespace on this line.

> +	git tag checkpoint
> +'
> + ...
> +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 &&
> +	

And here too.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Alexandr Miloslavskiy wrote (reply to this):

On 10.02.2020 21:39, Junio C Hamano wrote:
> Trailing whitespace on this line.

Whitespaces fixed in V3; I have also activated pre-commit hook. Sorry!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Alexandr Miloslavskiy <[email protected]> writes:

> On 10.02.2020 21:39, Junio C Hamano wrote:
>> Trailing whitespace on this line.
>
> Whitespaces fixed in V3; I have also activated pre-commit hook. Sorry!

I may not have time to read it over at least in a few days, but lack
if v3 in the title will make it cumbersome to come back to the
thread later X-<.  Thanks for a heads-up, anyway, thoguh.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <[email protected]> writes:

> I may not have time to read it over at least in a few days, but lack
> if v3 in the title will make it cumbersome to come back ...

Oops, please disregard.  I must have been looking at some wrong
thread.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Alexandr Miloslavskiy wrote (reply to this):

On 17.02.2020 18:59, Junio C Hamano wrote:
 > I may not have time to read it over at least in a few days, but lack
 > if v3 in the title will make it cumbersome to come back to the
 > thread later X-<.  Thanks for a heads-up, anyway, thoguh.

Here's the V3 in title :)

--------
[verse]
'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch] [--quiet] [--] <file>...
'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch]
[--quiet] [--pathspec-from-file=<file> [--pathspec-file-nul]]
[--] [<pathspec>...]

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,
Expand All @@ -26,15 +28,20 @@ allowing the file to be removed from just the index.

OPTIONS
-------
<file>...::
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.
<pathspec>...::
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::
Expand Down Expand Up @@ -68,19 +75,19 @@ OPTIONS
`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=<file>::
Pathspec is passed in `<file>` instead of commandline args. If
`<file>` 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`.

DISCUSSION
----------

The <file> 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.
--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).

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
--------------------------------------------------------
Expand Down
144 changes: 100 additions & 44 deletions Documentation/git-stash.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ SYNOPSIS
'git stash' branch <branchname> [<stash>]
'git stash' [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
[-u|--include-untracked] [-a|--all] [-m|--message <message>]
[--pathspec-from-file=<file> [--pathspec-file-nul]]
[--] [<pathspec>...]]
'git stash' clear
'git stash' create [<message>]
Expand Down Expand Up @@ -43,10 +44,10 @@ 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 2dedc21997..f75b80a720 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -43,9 +43,6 @@ 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
> --------
> -
>  push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>] [--] [<pathspec>...]::
>  
>  	Save your local modifications to a new 'stash entry' and roll them
> @@ -152,40 +149,51 @@ 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.
> +OPTIONS
> +-------
> +-a::
> +--all::
> +	All ignored and untracked files are also stashed and then cleaned
> +	up with `git clean`.
> +
> +-u::
> +--include-untracked::
> +	All untracked files are also stashed and then cleaned up with
> +	`git clean`.
> +
> +--index::
> +	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::
> +	All changes already added to the index are left intact.
> +
> +-p::
> +--patch::
> +	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.

I have a mixed feelings about this approach.  While I am sympathetic
to the "have a single place to describe all" approach this patch
takes, the approach needs to be executed with care when subcommands
do not share much of the options at all.  Those readers who jump to
the "OPTIONS" section and try to ignore anything outside the section
may not easily notice that --keep-index only applies to subcommands
that creates a new stash, and meaningless to subcommands that lets
you inspect existing stashes, or apply one to the working tree (and
optionally to the index), for example.  If the orinal documentation
did not use "OPTIONS" as the section header and instead said perhaps
"SUBCOMMANDS", it would have been even better, but otherwise I would
suspect that the original "the options understood by 'push' are all
described under the part that begins with 'push [-p] [-k] ...'
command line" arrangement was much easier to understand when reading
them through for the first time to learn and also to find what the
user is looking for after learning the "concept" (e.g. "with
'stash', there is a way to stash-away the changes made to the
working tree") but before becoming familiar with exact set of
options for each subcommand (e.g. "and there was an option that let
me stash only partial changes piecemeal, but what was it spelled?").

If we were to make the result of "a single place to describe all"
approach anything useful, I think at least

 (1) the list itself should make it clear that it does not talk
     about options related to listing and showing at all,
     before enumerating dashed options.

 (2) each item in the enumeration should identify which
     subcommand(s) accept(s) it.

So, I dunno.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Alexandr Miloslavskiy wrote (reply to this):

On 21.01.2020 21:21, Junio C Hamano wrote:
> I have a mixed feelings about this approach.  While I am sympathetic
> to the "have a single place to describe all" approach this patch
> takes, the approach needs to be executed with care when subcommands
> do not share much of the options at all.  Those readers who jump to
> the "OPTIONS" section and try to ignore anything outside the section
> may not easily notice that --keep-index only applies to subcommands
> that creates a new stash, and meaningless to subcommands that lets
> you inspect existing stashes, or apply one to the working tree (and
> optionally to the index), for example.  If the orinal documentation
> did not use "OPTIONS" as the section header and instead said perhaps
> "SUBCOMMANDS", it would have been even better, but otherwise I would
> suspect that the original "the options understood by 'push' are all
> described under the part that begins with 'push [-p] [-k] ...'
> command line" arrangement was much easier to understand when reading
> them through for the first time to learn and also to find what the
> user is looking for after learning the "concept" (e.g. "with
> 'stash', there is a way to stash-away the changes made to the
> working tree") but before becoming familiar with exact set of
> options for each subcommand (e.g. "and there was an option that let
> me stash only partial changes piecemeal, but what was it spelled?").
> 
> If we were to make the result of "a single place to describe all"
> approach anything useful, I think at least
> 
>   (1) the list itself should make it clear that it does not talk
>       about options related to listing and showing at all,
>       before enumerating dashed options.
> 
>   (2) each item in the enumeration should identify which
>       subcommand(s) accept(s) it.
> 
> So, I dunno.

I have updated the patch with (2). Sorry, I didn't understand what you 
mean in (1).

I have included my reasoning in commit message. If you feel against this 
change, I guess I'll just revert it. Afterall, my only goal was to 
describe new options. Tried to change things because I didn't like how 
this doc goes against the layout I have seen in all previous docs I edited.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>
writes:

> +-q::
> +--quiet::
> +	Quiet, suppress feedback messages.
> +
> +\--::
> +	Separates pathspec from options for disambiguation purposes.
> +
>  <pathspec>...::
>  	The new stash entry records the modified states only for the files
>  	that match the pathspec.  The index entries and working tree files

OK.  Describing these in the documentation is a good thing.  How
they should be added depends on in what shape the patch 4/8 should
settle, though.

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Alexandr Miloslavskiy via GitGitGadget" <[email protected]>
writes:

> From: Alexandr Miloslavskiy <[email protected]>
>
> This patch continues the effort that is already applied to
> `git commit`, `git reset`, `git checkout` etc.

Makes sense.

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 <message>] [--] [<pathspec>...]::
push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>] [--pathspec-from-file=<file> [--pathspec-file-nul]] [--] [<pathspec>...]::

Save your local modifications to a new 'stash entry' and roll them
back to HEAD (in the working tree and in the index).
Expand All @@ -56,38 +57,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.
+
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] [<message>]::

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.

Expand All @@ -111,7 +87,7 @@ show [<options>] [<stash>]::

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 `<stash>` 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).
Expand All @@ -128,14 +104,6 @@ pop [--index] [-q|--quiet] [<stash>]::
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 `<stash>` is given, `stash@{0}` is assumed, otherwise `<stash>` must
be a reference of the form `stash@{<revision>}`.

apply [--index] [-q|--quiet] [<stash>]::

Expand All @@ -149,8 +117,7 @@ branch <branchname> [<stash>]::
the commit at which the `<stash>` was originally created, applies the
changes recorded in `<stash>` to the new working tree and index.
If that succeeds, and `<stash>` is a reference of the form
`stash@{<revision>}`, it then drops the `<stash>`. When no `<stash>`
is given, applies the latest one.
`stash@{<revision>}`, it then drops the `<stash>`.
+
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
Expand All @@ -166,9 +133,6 @@ clear::
drop [-q|--quiet] [<stash>]::

Remove a single stash entry from the list of stash entries.
When no `<stash>` is given, it removes the latest one.
i.e. `stash@{0}`, otherwise `<stash>` must be a valid stash
log reference of the form `stash@{<revision>}`.

create::

Expand All @@ -185,6 +149,98 @@ store::
reflog. This is intended to be useful for scripts. It is
probably not the command you want to use; see "push" above.

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`.

-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`.

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

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

--pathspec-from-file=<file>::
This option is only valid for `push` command.
+
Pathspec is passed in `<file>` instead of commandline args. If
`<file>` 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`,
`save`, `store` commands.
+
Quiet, suppress feedback messages.

\--::
This option is only valid for `push` command.
+
Separates pathspec from options for disambiguation purposes.

<pathspec>...::
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.
+
For more details, see the 'pathspec' entry in linkgit:gitglossary[7].

<stash>::
This option is only valid for `apply`, `branch`, `drop`, `pop`,
`show` commands.
+
A reference of the form `stash@{<revision>}`. When no `<stash>` is
given, the latest stash is assumed (that is, `stash@{0}`).

DISCUSSION
----------

Expand Down
28 changes: 22 additions & 6 deletions builtin/rm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")),
Expand All @@ -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(),
};

Expand All @@ -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();
Expand All @@ -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);
Expand Down
Loading