Re: [PATCH] pathspec: prevent empty strings as pathspecs

Emily Xie [off-list ref] writes:

For any command that takes a pathspec, passing an empty string will
execute the command on all files in the current directory. This
results in unexpected behavior. For example, git add "" adds all
files to staging, while git rm -rf "" recursively removes all files
from the working tree and index. This patch prevents such cases by
throwing an error message whenever an empty string is detected as a
pathspec.

This change is likely to break people's existing scripts.

For example, a script may want to inspect some state of the whole
working tree and then after the inspection was satisfactory do
something that affects paths that are inside the directory you were
originally in.  You would write something like this:

	#!/bin/sh
	original=$(git rev-parse --show-prefix)

        cd "$(git rev-parse --show-toplevel)"
        ... do the whole-tree pre-inspection
        ... using various git commands here

	# Then finally do the "action"
        git add "$original"

This works even if you happen to be at the top-level of the working
tree, but with your patch it suddenly breaks.  It may be trivial to
update the script to use "." when $original is empty, but that is
not an excuse.  The important thing is that they need to be told
about having to change their script before this change actually
happens.  Suddenly breaking other people without telling is simply
rude and unacceptable.

At least you would need two-step process to introduce a change like
this to warn the people whose tools and workflows you are breaking.
That is, (1) in one release, you add code to only detect the case
you will be changing the behaviour in a later version and give
warning messages, and (2) in another release that is several release
cycles later, stop warning and actually change the behaviour.

That is, if we assume that passing an empty string by mistake is a
problem in the real world that we want to save users from.  After
all, even though there is _no_ good reason why we _need_ to take
'git add ""' as the same as 'git add .', there is no need not to,
either.  When a user says 'git add sub/sub/' we would add everything
in that directory, 'git add sub/' would add even more, and it is
logically consistent for 'git add ""' to add everything.  This is a
border-line "doctor, if I pass an empty string, it is taken the same
way as a dot. -- don't do that then."

A kind of change that satisifies the following criteria:

 (1) I can tell that the author of the change really means well.

 (2) The problem the change is fixing does not seem to be a huge
     issue in real life.

 (3) The code with the patch would behave is not wrong per-se; it
     probably is how we would have designed the feature if we knew
     better.

 (4) However, we are not designing things from scratch without
     existing users, and the change potentially breaks uncountable
     number of existing users.

is something I really hate having to evaluate.  This is one of them.

So, I am of two minds.

I do not mind a two-step breaking of compatibility to address this
issue; I would also understand if the author thinks it is not worth
the hassle to do so.  The sudden behaviour change with this patch
alone is however not acceptable, I would think.

On 06/18/2016 11:10 PM, Junio C Hamano wrote:

Emily Xie [off-list ref] writes:

quoted

For any command that takes a pathspec, passing an empty string will
execute the command on all files in the current directory. This
results in unexpected behavior. For example, git add "" adds all
files to staging, while git rm -rf "" recursively removes all files
from the working tree and index. This patch prevents such cases by
throwing an error message whenever an empty string is detected as a
pathspec.

This change is likely to break people's existing scripts.

For example, a script may want to inspect some state of the whole
working tree and then after the inspection was satisfactory do
something that affects paths that are inside the directory you were
originally in.  You would write something like this:

	#!/bin/sh
	original=$(git rev-parse --show-prefix)

         cd "$(git rev-parse --show-toplevel)"
         ... do the whole-tree pre-inspection
         ... using various git commands here

	# Then finally do the "action"
         git add "$original"

This works even if you happen to be at the top-level of the working
tree, but with your patch it suddenly breaks.  It may be trivial to
update the script to use "." when $original is empty, but that is
not an excuse.  The important thing is that they need to be told
about having to change their script before this change actually
happens.  Suddenly breaking other people without telling is simply
rude and unacceptable.

The help for rev-parse --show-prefix says:
"When the command is invoked from a subdirectory, show the path of
the current directory relative to the top-level directory."

This doesn't say what it does when invoked at the top-level.  Maybe we 
should just change it to output "./", which would make such scripts 
work?  It would not necessarily fix all possible scripts, but it would 
fix these.  It seem odd to insist on preserving this undocumented 
behavior of git add/rm.

At least you would need two-step process to introduce a change like
this to warn the people whose tools and workflows you are breaking.
That is, (1) in one release, you add code to only detect the case
you will be changing the behaviour in a later version and give
warning messages, and (2) in another release that is several release
cycles later, stop warning and actually change the behaviour.

That is, if we assume that passing an empty string by mistake is a
problem in the real world that we want to save users from.

I discovered this when one of our shell-scripts had a variable was 
accidentally empty.  The user I was supporting was very confused.  So I 
think it is a problem in the real world.