Re: [PATCH] diff-files: treat "i-t-a" files as "not-in-index"
- From
Srinidhi Kaushik <shrinidhi.kaushik@gmail.com>
- Date
- Jun 19, 2020, 09:31 UTC
- Message-ID
- <20200619093127.GA47528@mail.clickyotomy.dev>
- In-Reply-To
- <xmqqbllgknfd.fsf@gitster.c.googlers.com>
Thank you for reviewing this; I appreciate it!
Show 7 quoted lines
> > + 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.
[...]
Show 17 quoted lines
> > > + 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".
[...]
Show 22 quoted lines
> > + 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.