Re: [PATCH] diff-files: treat "i-t-a" files as "not-in-index"
From: Srinidhi Kaushik <hidden>
Date: 2020-06-19 09:34:16
Thank you for reviewing this; I appreciate it!
quoted
+ content="foo" && + echo $content >not-empty &&The quoting decision is backwards in these two lines. It is OK not to quote when the right hand side literal is clearly a single word without $IFS. On the other hand, it is a good practice to always quote when using what is in a "$variable".
Yes, that doesn't look right, I will make changes in v2. [...]
quoted
quoted
+ touch empty &&Use of "touch" gives a wrong impression that you care about the file timestamp; use something like ": >empty &&" instead when you care about the presence of the file and do not care about its timestamp.I just realized that this is even more important in this case not to use "touch". The test that uses this file cares not just the presence, but it deeply cares that its contents is empty. The thing it least cares about is its timestamp. The purpose of using "touch" is to update the timestamp, to keep the current contents if it exists, and to ensure it exists (as a side effect), in the decreasing order of importance. Use of the command here misleads the readers.
Oops, you are right. That makes sense. Will update to ": >empty". [...]
quoted
+ git add -N empty not-empty && + git diff-files -p >actual && + hash_e=$(git hash-object empty) && + hash_n=$(git hash-object not-empty) && + cat >expect <<-EOF && + diff --git a/empty b/empty + new file mode 100644 + index 0000000..$(git rev-parse --short $hash_e) + diff --git a/not-empty b/not-empty + new file mode 100644 + index 0000000..$(git rev-parse --short $hash_n) + --- /dev/null + +++ b/not-empty + @@ -0,0 +1 @@ + +$content + EOF + test_cmp expect actual +'OK. Do we want to show what happens when "diff" and "diff --cached" are run with these two "added but not quite added yet" paths to contrast with this new case?
I'm not sure if we want to repeat an older test. The test (which was renamed in this patch) in t2203-add-intent.sh: "diff/diff-cached shows ita as new/not-new files" is already doing that. Should the "diff" and "diff --cached" steps be appended here again? Thanks.