[PATCH] git-add: allow --ignore-missing always, not just in dry run

[PATCH] git-add: allow --ignore-missing always, not just in dry run

[Dieter Plaetinck]

There is no need to restrict use of --ignore-missing to dry runs,
it can be useful to ignore missing files during normal operation as
well.

Signed-off-by: Dieter Plaetinck <dieter@plaetinck.be>
---
 Documentation/git-add.txt |    9 +++++----
 builtin/add.c             |    4 +---
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 9c1d395..c6fae9f 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -138,10 +138,11 @@ subdirectories.
 	true to make this the default behaviour.
 
 --ignore-missing::
-	This option can only be used together with --dry-run. By using
-	this option the user can check if any of the given files would
-	be ignored, no matter if they are already present in the work
-	tree or not.
+	If some files could not be added because they are missing,
+	do not raise any error but continue adding the others.
+	By using this option with --dry-run the user can check if
+	any of the given files would be ignored,
+	no matter if they are already present in the work tree or not.
 
 \--::
 	This option can be used to separate command-line options from
diff --git a/builtin/add.c b/builtin/add.c
index 1c42900..e702714 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -326,7 +326,7 @@ static struct option builtin_add_options[] = {
 	OPT_BOOLEAN('A', "all", &addremove, "add changes from all tracked and untracked files"),
 	OPT_BOOLEAN( 0 , "refresh", &refresh_only, "don't add, only refresh the index"),
 	OPT_BOOLEAN( 0 , "ignore-errors", &ignore_add_errors, "just skip files which cannot be added because of errors"),
-	OPT_BOOLEAN( 0 , "ignore-missing", &ignore_missing, "check if - even missing - files are ignored in dry run"),
+	OPT_BOOLEAN( 0 , "ignore-missing", &ignore_missing, "just skip files which do not exist"),
 	OPT_END(),
 };
 
@@ -388,8 +388,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (addremove && take_worktree_changes)
 		die(_("-A and -u are mutually incompatible"));
-	if (!show_only && ignore_missing)
-		die(_("Option --ignore-missing can only be used together with --dry-run"));
 	if ((addremove || take_worktree_changes) && !argc) {
 		static const char *here[2] = { ".", NULL };
 		argc = 1;
-- 
1.7.8.3

Re: [PATCH] git-add: allow --ignore-missing always, not just in dry run

[Junio C Hamano]

Dieter Plaetinck <dieter@plaetinck.be> writes:

> There is no need to restrict use of --ignore-missing to dry runs,
> it can be useful to ignore missing files during normal operation as
> well.
>
> Signed-off-by: Dieter Plaetinck <dieter@plaetinck.be>

Sorry, but for this kind of change, we would want to see a justification
that is much better than that. The default around here is not to change an
established behaviour without a good reason.

Have you dug into the list archive to see _why_ we decided not to allow
this option in the real run in the first place? You would need to find "By
letting the command ignore missing paths, the user can get into X and Y
situations and we would want to avoid it. We however need to give users a
way to see if there is something missing, hence we add it when we are
under dry-run option." and refute that previous justification, arguing why
X and Y is something we should _not_ be worrying about, to make a good
case for this change.

In this particular case, my gut feeling is that this might a change in the
good direction (but I strongly suspect that I am not recalling the real
reason why we didn't allow it when we introduced this option).

If somebody is writing a script using "git add" (which is not recommended
to begin with), it is tempting to say 'git add $list_of_possible_files' in
such a script when the script _knows_ that the list it is giving to "git
add" may contain a path that does not exist, and wants to ignore missing
ones.

But then the script could easily filter what does not exist before
compiling such a list, so that is not a very strong reason to advocate
it.

Re: [PATCH] git-add: allow --ignore-missing always, not just in dry run

[Dieter Plaetinck]

On Wed, 18 Jan 2012 14:56:12 -0800
Junio C Hamano <gitster@pobox.com> wrote:

> Dieter Plaetinck <dieter@plaetinck.be> writes:
> 
> > There is no need to restrict use of --ignore-missing to dry runs,
> > it can be useful to ignore missing files during normal operation as
> > well.
> >
> > Signed-off-by: Dieter Plaetinck <dieter@plaetinck.be>
> 
> Sorry, but for this kind of change, we would want to see a
> justification that is much better than that. The default around here
> is not to change an established behaviour without a good reason.

Hello Junio, thanks for your quick and elaborate response.
 
> Have you dug into the list archive to see _why_ we decided not to
> allow this option in the real run in the first place?

Actually I did, before submitting this patch.
From what I could find, the original patch [1] only cared about having this
feature available in dry run, and came with this "only in dry run" restriction,
merely because it was the only use case considered.
I couldn't find any evidence of this restriction actually being needed nor
any discussion of this matter. I added Jens to this mail, as he wrote
the original patch.

> You would need
> to find "By letting the command ignore missing paths, the user can
> get into X and Y situations and we would want to avoid it. We however
> need to give users a way to see if there is something missing, hence
> we add it when we are under dry-run option." and refute that previous
> justification, arguing why X and Y is something we should _not_ be
> worrying about, to make a good case for this change.

Yes, ignoring missing files can lead to files not actually being added, if they are missing.
But that's why this is an optional flag to needs to explicitly passed.
The flag is clear about what it does, so if users enable it, I don't see the problem?

> If somebody is writing a script using "git add" (which is not
> recommended to begin with), it is tempting to say 'git add
> $list_of_possible_files' in such a script when the script _knows_
> that the list it is giving to "git add" may contain a path that does
> not exist, and wants to ignore missing ones.
> 
> But then the script could easily filter what does not exist before
> compiling such a list, so that is not a very strong reason to advocate
> it.

The use case is as follows:
I'm working on a tool [2] which runs in the background, and automatically synchronizes file/directory trees,
by using inotify events, and synchronizing changes in the working tree to the
index automatically, committing automatically and push/pulling automatically.
Basically for when you want to version a tree with git, but without needing to manually
commit all the time. (useful for a directory with notes as text files, for example)
the problem is, you can have a constant stream of inotify events (if files are being
edited/deleted/(re)created all the time), and at the point where my tool decides to automatically commit the changes it just saw,
more changes (deletes, renames, readding a file that was just deleted, ..) can happen around the same time.
So basically, if this tool needs to check which files still/no-longer exist before calling git-add,
that's vulnerable to race conditions.
The only real solution against race conditions is to deal with (ignore) missing files right at the point where git
adds them to the index.

But if "git add" is not the right way, please let me know the alternative.
From what I can find "git add --all --ignore-missing <file>" (with my patch for allowing ignore-missing without dry run) would be the most appropriate,
this would never accidentially modify the working tree (as "git rm" for a deleted file could, if the file gets readded at the same point
where we call git), and it would gracefully handle changes we didn't see yet (such as new/modified files disappearing again)

thanks again for the feedback,
Dieter

[1] http://kerneltrap.org/mailarchive/git/2010/7/9/34077
[2] https://gitorious.org/search?q=dvcs-autosync

Re: [PATCH] git-add: allow --ignore-missing always, not just in dry run

[Junio C Hamano]

Dieter Plaetinck <dieter@plaetinck.be> writes:

> So basically, if this tool needs to check which files still/no-longer
> exist before calling git-add, that's vulnerable to race conditions.

I do not think you are solving the real problem in your script even if you
allowed "add --ignore-missing".

I suspect you are making things even worse by using "--ignore-missing" in
your script. If a user is actively updating the files in the filesystem,
at least "git add" without "--ignore-missing" would catch the case where
you _thought_ the user modified but still has the file, but in reality the
further updates in the working tree removed the file, which is a clear
indication that the rate you are processing the notify stream is slower
than the activity generated by the user and allows you to notice that you
may be better off waiting a bit until things calm down before running your
automated commit.

Also, with or without "--ignore-missing", I think we have safety valves to
cause "git add" fail if the file being added is updated while git is
working on it (i.e. we read and compute the object name, and then store it
compressed, and check the hash of what is stored matches the object name
we computed earlier, which would fail if the file is updated in the middle
at the right time).

This means that the "--ignore-missing" option will _not_ eliminate all
cases where "git add" may detect an error and fails. In other words, your
script needs to deal with error return from "git add" anyway even if we
applied your patch and you used "--ignore-missing" in your script.

I have to say that the basic premise of your script is simply broken, and
I am afraid that it is unfixable without an atomic snapshot support from
the underlying filesystem (i.e. take a snapshot, run 'git add' on it, and
then release the snapshot).

Having said all that, I do agree to the view that it is OK to let it
happen if the user explicitly asks a typo'ed pathspec on the command line
to be ignored for interactive use cases, and for that reason alone, I am
not fundamentally opposed to allowing the use of --ignore-missing outside
the --dry-run context.

Thanks.

Re: [PATCH] git-add: allow --ignore-missing always, not just in dry run

[Dieter Plaetinck]

On Thu, 19 Jan 2012 13:26:40 -0800
Junio C Hamano <gitster@pobox.com> wrote:

> Dieter Plaetinck <dieter@plaetinck.be> writes:
> 
> > So basically, if this tool needs to check which files
> > still/no-longer exist before calling git-add, that's vulnerable to
> > race conditions.
> 
> I do not think you are solving the real problem in your script even
> if you allowed "add --ignore-missing".
> 
> I suspect you are making things even worse by using
> "--ignore-missing" in your script. If a user is actively updating the
> files in the filesystem, at least "git add" without
> "--ignore-missing" would catch the case where you _thought_ the user
> modified but still has the file, but in reality the further updates
> in the working tree removed the file, which is a clear indication
> that the rate you are processing the notify stream is slower than the
> activity generated by the user and allows you to notice that you may
> be better off waiting a bit until things calm down before running
> your automated commit.

I don't understand what you mean. if this happened:
1) modify file
2) modify file
3) remove file
but my script tries to git add --ignore-missing, when 3) already happened
but the script only sees event 2)
then it will not fail, then catch the 3rd event, then do a git rm.
if however, i don't enable ignore-missing, there's a failure on the git add.
I guess what you're saying is "better to get an error, interpret it as `this may not be a "real" error`, just continue"

> 
> Also, with or without "--ignore-missing", I think we have safety
> valves to cause "git add" fail if the file being added is updated
> while git is working on it (i.e. we read and compute the object name,
> and then store it compressed, and check the hash of what is stored
> matches the object name we computed earlier, which would fail if the
> file is updated in the middle at the right time).
> 
> This means that the "--ignore-missing" option will _not_ eliminate all
> cases where "git add" may detect an error and fails. In other words,
> your script needs to deal with error return from "git add" anyway
> even if we applied your patch and you used "--ignore-missing" in your
> script.
> 
> I have to say that the basic premise of your script is simply broken,
> and I am afraid that it is unfixable without an atomic snapshot
> support from the underlying filesystem (i.e. take a snapshot, run
> 'git add' on it, and then release the snapshot).

I assumed that `git add` works atomically. (as in: if you git add a file and
modify that file at the same time, either the old or the new version will be added
to the index, but always successfully).
If I understand this correctly, `git add` can only be successful if at least the file
remains untouched while the git command runs.
This means I should change my entire approach and be aware that git may return failures when the user is changing files while we are adding to the index. 
Maybe I should do something like:
once >0 inotify events happened,
* run git status, for each file in git status, if we've seen events, try to add file to the index.
if the above fails, wait a few seconds, then try again. if failures persist a few times in a row, only then we can be reasonbly sure something is really wrong.

but there will always be the case where a file gets deleted and (re)added, there is always the risk for races:
1) my script runs "git rm" after seeing a "delete" event but the file has been added in the meanwhile... git removes the file => bad
2) my script runs "git add" after seeing a new/changed file but the file has been removed in the meanwhile... git gives an error.
and in the latter case you can't just apply the "just retry" trick I mentioned above because the next event is delete, which would trigger a "git rm",
potentially causing unrecoverable data loss as described in 1).

one example of such things happening in real life is vim which creates (and removes) tmp files called `4913`.
We could just add such filenames to gitignore,
but that seems like a bit of a burden which shouldn't be needed.

To avoid the risks mentioned above and the "file can be altered during git add command",
what i'm really looking for is a way to, for a given file (file for which I've seen inotify events):
synchronize changes of that file to the index even if the file was unknown to git, or doesn't exist anymore;
and do that in an atomic way (or: i'll just retry a few times until it fails consistently, but removal race conditions can lead to data loss)

I tried `git update-index` as you suggested but couldn't have it behave like that.
And something like `git add --all` can still cause a file removal when it shouldn't, and lead to uncoverable data loss on race conditions.

Sorry for the long mail, I tried to keep it as minimal as possible.

thanks!
Dieter

Re: [PATCH] git-add: allow --ignore-missing always, not just in dry run

[Thomas Rast]

[dropped Dieter as this really goes off on an internal tangent]

Junio C Hamano <gitster@pobox.com> writes:

> If somebody is writing a script using "git add" (which is not recommended
> to begin with)

Can we still stick to that stance?  Our tests are increasingly using
'git add' instead of 'git update-index --add':

  $ git grep 'git[ -]add' t/ | wc -l
  1540
  $ git grep 'git[ -]update-index --add' t/ | wc -l
  269
  $ git grep 'git[ -]update-index --add' v1.6.0 t/ | wc -l
  251
  $ git grep 'git[ -]add' v1.6.0 t/ | wc -l
  705

So while git(1) still says git-add is porcelain (and thus not to be used
for scripting), it has mostly superseded 'git update-index --add' in new
script usage even within git.git.  I suspect the same goes for things
like git-rm, git-commit, etc.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] git-add: allow --ignore-missing always, not just in dry run

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> [dropped Dieter as this really goes off on an internal tangent]
>
> Junio C Hamano <gitster@pobox.com> writes:
>
>> If somebody is writing a script using "git add" (which is not recommended
>> to begin with)
>
> Can we still stick to that stance?  Our tests are increasingly using
> 'git add' instead of 'git update-index --add':
>
>   $ git grep 'git[ -]add' t/ | wc -l
>   1540
>   $ git grep 'git[ -]update-index --add' t/ | wc -l
>   269
>   $ git grep 'git[ -]update-index --add' v1.6.0 t/ | wc -l
>   251
>   $ git grep 'git[ -]add' v1.6.0 t/ | wc -l
>   705

Stop being silly.

Have you actually looked at these usage?  Some of them are genuinely
testing if "git add" works correctly, so it is out of the scope of this
discussion, but others that could be "git update-index" are feeding the
paths known to the script to exist (and we want 'git add' to error out
if that is not the case).

More generally, scripts in t/ directories are "scripts", but it is totally
different from the kind of "user facing script that behaves as if it is a
complete command, taking its own command line arguments, passing them
through to the underlying plumbing commands".

Re: [PATCH] git-add: allow --ignore-missing always, not just in dry run

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

> Thomas Rast <trast@student.ethz.ch> writes:
>
>>   $ git grep 'git[ -]add' t/ | wc -l
>>   1540
>>   $ git grep 'git[ -]update-index --add' t/ | wc -l
>>   269
>>   $ git grep 'git[ -]update-index --add' v1.6.0 t/ | wc -l
>>   251
>>   $ git grep 'git[ -]add' v1.6.0 t/ | wc -l
>>   705
>
> Stop being silly.
>
> Have you actually looked at these usage?  Some of them are genuinely
> testing if "git add" works correctly, so it is out of the scope of this
> discussion, but others that could be "git update-index" are feeding the
> paths known to the script to exist (and we want 'git add' to error out
> if that is not the case).

I'm sorry if I sound silly, that was totally not the point.  I also
admit that I did not look at the usages at all.  I merely wanted to
point out that the understanding in the git community *itself* has
evolved to use git-add instead of git update-index --add in its own
scripting.  Admittedly the statistics are even more striking than I
could possibly hope for.

So I am challenging the notion that git-add is not recommended for use
in scripts, which is how I understood your parenthetical remark

} If somebody is writing a script using "git add" (which is not recommended
} to begin with)

We're no longer following that advice ourselves, how can we expect users
to adhere to it?

> More generally, scripts in t/ directories are "scripts", but it is totally
> different from the kind of "user facing script that behaves as if it is a
> complete command, taking its own command line arguments, passing them
> through to the underlying plumbing commands".

I don't understand what distinction you are trying to make here.  Maybe
my mental model of the plumbing/porcelain separation (which is mostly
about interface stability) is wrong?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] git-add: allow --ignore-missing always, not just in dry run

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

>> More generally, scripts in t/ directories are "scripts", but it is totally
>> different from the kind of "user facing script that behaves as if it is a
>> complete command, taking its own command line arguments, passing them
>> through to the underlying plumbing commands".
>
> I don't understand what distinction you are trying to make here.  Maybe
> my mental model of the plumbing/porcelain separation (which is mostly
> about interface stability) is wrong?

What's so hard to understand that these tests are very different from
end-user scripts?

We could have shipped you a written instruction to type these commands
from your shell and made you responsible for running them every time we
release a new version. That would have been more true to the intent of
these test scripts. The test being implemented as scripts is merely a
substitute for hiring one Thomas Rast as a test engineer to type them from
the terminal ;-).

User-facing scripts (Porcelain enhancements) people write are in a totally
different boat. They take input and have code to make their own decision
what kind of arguments and inputs to feed to their underlying building
blocks. They may even parse output from the commands they invoke to base
their decision that affects what happens next. Our tests start from a
known state (i.e. empty trash directory), take input from neither command
line, human interaction nor the filesystem content of the day, that affect
the input to the commands they drive.

To put it another way, if you have a cron job that does

    cd $HOME/diary && git add MyDiary.txt

that is perfectly fine. You are letting the machine do the typing for you
every hour, instead of having you type these yourself. It is even OK if
the filename was derived from `date` or something, i.e.

    N=$(date +'%Y-%m-%d').txt &&
    if test -f "$N"
    then
	git add "$N"
    fi

What is not OK is to attempt parsing from Porcelain output to decide what
to do next. "git branch | sed -ne 's/^\* //p'" is a typical example.

Our tests are different for another important reason you seem to be
missing. The tests we ship are tied very closely with the version of Git
they are testing. Even parsing the command output is acceptable for our
tests for this reason (obviously that is the only way to make sure that we
are issuing an appropriate error, warning, or advice message to the end
user). End-user scripts do not have that property.

And the biggest thing you should consider is that 99% of users are too
busy to bother thinking for themselves and instead prefer to be handed
down a concise recipe to follow blindly. You could include "in this, that,
and that other situation, it is OK to use Porcelain command" to the
recipe, but doing so defeats the whole purpose of having a recipe to begin
with, by making the readers responsible for thinking for themselves again.
That is why we just give a concise "Do not use Porcelain commands in your
scripts as their behaviour is subject to change."

Re: [PATCH] git-add: allow --ignore-missing always, not just in dry run

[Mike Gant]

On Wed, Jan 18, 2012 at 10:52:24PM +0100, Dieter Plaetinck wrote:
> There is no need to restrict use of --ignore-missing to dry runs,
> it can be useful to ignore missing files during normal operation as
> well.

FWIW I would be in favor of this change and I was going to submit a
patch, too. My use case is different, though. I create branches that
will never be merged to the mainline because they have files added that
I don't want in master. The files added to these branches can vary. In
my script to create the branch, I want the largest possible set of these
files as the argument to 'git add' but if not all exist it's okay. I
realize I can write my script to only add the files that exist but I'm
lazy ;) and the --ignore-missing option would be easier.

Mike