Re: [PATCH 1/3] tests: Prepare --textconv tests for correctly-failing conversion program

> Kirill Smelkov <kirr@landau.phys.spbu.ru> writes:
> 
> > Recently I've spot a bug
> 
> We usually avoid the first person in commit messages. The cover letter
> is a good place to tell about your personal story, but the commit
> message is what will remain, what people will read after a blame or
> bisect. They won't care whether you've "recently" found a bug, or in
> which circumstances you've found it.
> 
> I'd write stg like (which would probably go to PATCH 2/3 instead of
> here):
> 
> -----8<----
> git blame --textconv is wrongly calling the textconv filter on
> symlinks: symlinks are stored as blobs whose content is the target of
> the link, and blame calls the textconv filter on a temporary file
> filled-in with the content of this blob.
> 
> For example:
> 
> >     $ git blame -C -C regular-file.pdf
> >     Error: May not be a PDF file (continuing anyway)
> >     Error: PDF file is damaged - attempting to reconstruct xref table...
> >     Error: Couldn't find trailer dictionary
> >     Error: Couldn't read xref table
> >     Warning: program returned non-zero exit code #1
> >     fatal: unable to read files to diff
> -----8<----

> > So git-blame is wrong here, and I'm going to write problem-demonstration
> > tests + try to fix it, but first we have to convert existing textconv
> > converter, so it will mimic pdftext behaviour -- if there is no '^bin:'
> > in input -- it's not a "binary" file and helper exits with error.
> 
> What's interesting here is not that you mimick pdftext behavior, but
> that you allow to easily distinguish file content and symlink target.
> 
> Here's my try:
> 
> -----8<----
> The textconv filter is sometimes incorrectly ran on a temporary file
> whose content is the target of a symbolic link, instead of actual file
> content. Prepare to test this by marking the content of the file to
> convert with "bin:", and let the helper die if "bin:" is not found in
> the file content.
> -----8<----

> Kirill Smelkov <kirr@landau.phys.spbu.ru> writes:
> 
> > Subject: Re: [PATCH 2/3] blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks
> 
> We try to keep the subject lines short (<80 chars, and as much as
> possible less so that "git log --oneline" be pretty).
> 
> How about
> 
> blame,cat-file: add failing tests for --textconv on symlinks

> > +test_expect_success 'make another new commit' '
> > +	echo "bin: test number 2" >three.bin &&
> > +	echo "bin: test number 2 version 2" >>three.bin &&
> > +	echo "bin: test number 2 version 3" >>three.bin &&
> > +	echo "bin: test number 3" >>three.bin &&
> 
> cat >three.bin <<EOF
> bin: test number 2
> bin: test number 2 version 2
> bin: test number 2 version 3
> bin: test number 3
> EOF
> 
> ?

> > +cat >expected <<EOF
> > +(Number1 2010-01-01 18:00:00 +0000 1) converted: test number 2
> > +(Number2 2010-01-01 20:00:00 +0000 2) converted: test number 2 version 2
> > +(Number3 2010-01-01 22:00:00 +0000 3) converted: test number 2 version 3
> > +(Number4 2010-01-01 23:00:00 +0000 4) converted: test number 3
> > +EOF
> 
> These days, it's recommanded to put this kind of code within the
> test_expect_success/test_expect_failure.

> > +
> > +echo -n "one.bin" >expected
> 
> echo -n is not very portable (and doesn't seem to be used in git's t/
> directory). Better use
> 
> printf "%s" "one.bin" >expected
> 
> (again, within text_expect_failure if possible)

> Kirill Smelkov <kirr@landau.phys.spbu.ru> writes:
> 
> > I don't know blame code well, and I'm not sure I'm doing it right or
> > otherwise without mistakes. Thus an RFC.
> 
> I don't know the code well either, but I didn't see any obvious
> problem in your code.
> 
> > --- a/sha1_name.c
> > +++ b/sha1_name.c
> > @@ -1068,7 +1068,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
> >  		struct cache_entry *ce;
> >  		int pos;
> >  		if (namelen > 2 && name[1] == '/')
> > -			return get_sha1_oneline(name + 2, sha1);
> > +			return get_sha1_oneline(name + 2, sha1);    /* XXX also mode? */
> 
> (This is the case where we're parsing ":/foo")
> 
> Currently, the mode is set a few lines above:
> 
> 	oc->mode = S_IFINVALID;
> 
> I guess this is OK since :/foo will return a commit sha1, not a file
> sha1.

> > @@ -1095,6 +1095,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
> >  				break;
> >  			if (ce_stage(ce) == stage) {
> >  				hashcpy(sha1, ce->sha1);
> > +				oc->mode = ce->ce_mode; /* XXX ok? */
> 
> I'd say this is OK, you're setting the mode from the index. What was
> the reason for your question mark?