Re: RFH: unexpected reflog behavior with --since=

From: Jeff King <peff@peff.net>
Date: Nov 10, 2011, 08:08 UTC
Message-ID: <20111110080851.GA28342@sigill.intra.peff.net>
In-Reply-To: <4EBB8596.6040507@nextest.com>

On Thu, Nov 10, 2011 at 12:04:38AM -0800, Eric Raible wrote:

Show 10 quoted lines

> > I think I am leaning towards the latter. It seems to me to be the more
> > likely guess for what the user would want. And there is real benefit to
> > doing it in git, since we can stop the traversal early. In the
> > "grep-like" case, doing it inside git is not really any more efficient
> > than filtering in a pipeline, like:
> > 
> >   git log -g --format='%ct %H' |
> >   awk '{ print $2 if $1 < SOME_TIMESTAMP }'
> 
> And then the sha would have to be fed back into git to be useful, eh?

It's just illustrative. You could replace "%H" with the actual information you're interested in.

Show 9 quoted lines

> > Of course we could still offer both (with a "--reflog-since" type of
> > option). We'd also need to turn off the optimization for "--since", and
> > then check whether "--until" has a similar bug (and offer
> > "--reflog-until").
> 
> I don't see the point of --reflog-since.  If the user specifies 'reflog'
> (either directly or with -g), then can't we just use the reflog's timestamp?
> Note: there might be good reasons, as my use of the reflog (and --since, for
> that matter), has been very simplistic so far.

The only point would be to leave "--since" to act on the commit timestamps, so that you don't have to resort to the external grepping I mentioned above. However, I'm not convinced anybody even cares about that use case.

I think the behavior you want is much more sensible.

Show 15 quoted lines

> > diff --git a/reflog-walk.c b/reflog-walk.c
> > index 5d81d39..2e5b270 100644
> > --- a/reflog-walk.c
> > +++ b/reflog-walk.c
> > @@ -231,6 +231,7 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)
> >  	reflog = &commit_reflog->reflogs->items[commit_reflog->recno];
> >  	info->last_commit_reflog = commit_reflog;
> >  	commit_reflog->recno--;
> > +	commit->date = reflog->timestamp;
> >  	commit_info->commit = (struct commit *)parse_object(reflog->osha1);
> >  	if (!commit_info->commit) {
> >  		commit->parents = NULL;
> 
> Is this something you'd be willing to turn into a real patch?
> I'm certainly not qualified.

Yes. We're in release freeze now, so I didn't even bother with sending it to Junio. But also, I'd like to gather more opinions on whether the design is the right thing (hopefully the implementation is Obviously Correct. :) ).

-Peff