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

Dieter Plaetinck [off-list ref] 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 <redacted>

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.

On Wed, 18 Jan 2012 14:56:12 -0800
Junio C Hamano [off-list ref] wrote:

Dieter Plaetinck [off-list ref] writes:

quoted

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

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

Dieter Plaetinck [off-list ref] 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.

On Thu, 19 Jan 2012 13:26:40 -0800
Junio C Hamano [off-list ref] wrote:

Dieter Plaetinck [off-list ref] writes:

quoted

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