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

From: Kirill Smelkov <kirr@landau.phys.spbu.ru>
Date: Sep 21, 2010, 18:39 UTC
Message-ID: <20100921183950.GA4390@landau.phys.spbu.ru>
In-Reply-To: <vpq1v8o9lg8.fsf@bauges.imag.fr>

On Mon, Sep 20, 2010 at 11:03:35PM +0200, Matthieu Moy wrote:

Show 11 quoted lines

> Kirill Smelkov <kirr@landau.phys.spbu.ru> writes:
> 
> > Matthieu, Jeff, Junio, thanks for your review and input.
> >
> > Comments below inline...
> 
> You've replied to several messages in one. That's OK here since the
> content of your message is essentially uncontroversial, but I'd
> suggest keeping separate threads for separate patches for next time.
> (for example, my reply below looks strange since I'm commenting on
> PATCH 2/3 in a message titled PATCH 1/3 ...). No big deal.

I knew someone will say this :)

But let's try to think about it from a bit different point of view - we had 3 patches, and description for at least two of them were intermixed. In this situation the series is better considered as a whole, and if we consider each patch git branch, what would you do if you need to get cumulative result of all the branches?

Yes, merge them - so did I. And the reply actually had all the references to parent messages in it's In-Reply-To header. It's just that usually MUA's don't show merges graphically (please correct me if I'm wrong), but otherwise isn't it ok?

I agree, subject should be better corrected somehow, but main idea still stays valid!

Clearly I did lots of giting recently, so I project it's concepts on the world :)

Show 17 quoted lines

> >> > 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
> >
> > I'd like to shorten it, but "add failing tests" is not as descriptive as
> > "Demonstrate --textconv misbehaves in such-and-such way",
> 
> Right, but your "such-and-such way" is not really precise either. One
> cannot know whether it's running, but the wrong way, whether it's
> running and should not, etc. The subject line is here to be a summary,
> you don't need details. And I actually find "add failing tests" more
> precise than "demonstrate that ... is behaving wrongly".

Hmm, maybe my wording is not that good, but to me "Demonstrate --textconv misbehaves ..." is closer to being more descriptive than "add failing tests".

This is maybe just a matter of preferences, so I'm ok with either way.

Show 13 quoted lines

> > and I can't come up with a shorter subject without making it more
> > cryptic. And btw, I've looked at log --oneline output, and
> > (surprise, surprise)
> >
> > 479a56 4fccc04 etc ...
> 
> I don't understand what you mean. After applying your patches, I get:
> 
> 6998f9a RFC: blame,cat-file --textconv: Don't assume mode is ``S_IFREF | 0664''
> 025aaac blame,cat-file: Demonstrate --textconv is wrongly running converter on symlinks
> 3646401 tests: Prepare --textconv tests for correctly-failing conversion program
> 
> which do not fit on a 80-columns terminal.

I meant do git log on those sha's, but late at night I've ommitted one letter. Here you go:

$ git log -1 --oneline a479a56 a479a56 Documentation/Makefile: allow man.base.url.for.relative.link to be set from Make $ git log -1 --oneline 4fccc04 4fccc04 Documentation: remove stray backslashes from "Fighting regressions" article

That's 88 & 83 characters. I mean to say maybe it's not that big deal to sacrifice understandability for 80 chars, especially when git log output is paged, usually through less -S.

That said I also try to keep subjects short when possible.

> > So if that's not a major obstacle, I'd leave it as is.
> 
> I don't consider it "major" (but other may have different opinions)
> though.

So what would be conclusion here?

Show 17 quoted lines

> >> These days, it's recommanded to put this kind of code within the
> >> test_expect_success/test_expect_failure.
> >
> > I see, thanks.
> >
> > I've moved other >expected preparation inside test_expect_*, but this
> > expect is special in that it is used in subsequent two tests. So I'd
> > leave this one outside.
> >
> > And btw, I've originally copied in-style what was already there in t8006
> > and t8007 which date to Jun 2010.
> 
> Yes, the guideline is new, and not widely applied (yet).
> 
> > Ah, yes, thanks. I forgot ':/text' means `commit which log is text' -
> 
> A patch to update the comment above follows.

thanks

> > Thanks again to everyone.
> 
> That "everyone" includes you ;-). Thanks for the patch, good job both
> for testing and for the actual fix!

Come on, Matthieu, it's just one small correction :)

Thanks, Kirill