Re: [RFC/PATCH] fetch: bigger forced-update warnings

From: Jeff King <peff@peff.net>
Date: Sep 7, 2011, 21:57 UTC
Message-ID: <20110907215700.GA27292@sigill.intra.peff.net>
In-Reply-To: <7vty8o10kj.fsf@alter.siamese.dyndns.org>

On Wed, Sep 07, 2011 at 02:53:32PM -0700, Junio C Hamano wrote:

Show 6 quoted lines

> > Some branches are expected to rewind, so the prominent
> > warning would be annoying. However, git doesn't know what
> > the expectation is for a particular branch. We can have it
> > guess by peeking at the lost couple of reflog entries. If we
> 
> s/lost/last/

Oops, thanks.

Show 6 quoted lines

> This is slightly offtopic, but I have been wondering if this approach do
> the right thing for "git pull". Wouldn't the underlying "git fetch" give a
> warning, and then the calling "git pull" go ahead and make a merge,
> scrolling the warning away with the merge/update summary diffstat? That
> would be a larger change if "git pull" needs to stash away the warning
> message, do its thing and then spit out the warning later.

I think this particular warning has nothing to do with git-pull. But rather, that we should _always_ abort a pull with a forced-update. Because the only sane things to do there are:

  1. Stop and look around, and see if you should be doing a "git reset"
     first.

or

  2. "git pull --rebase"

But proceeding with the pull just seems like a disaster. So it is not about "this usually fast forwards, but isn't now, so let's make the warning bigger". It is more about noticing that it is a forced-update at all. Maybe that's what you meant by "off-topic". :)

Show 10 quoted lines

> > +static int forced_update_is_uncommon(const char *ref)
> > +{
> > +	struct update_counts uc;
> > +	memset(&uc, 0, sizeof(&uc));
> > +	if (for_each_recent_reflog_ent(ref, count_updates, 4096, &uc) < 0)
> > +		for_each_reflog_ent(ref, count_updates, &uc);
> > +	return uc.fastforward && uc.forced <= 1; /* 1 for the one we just did */
> > +}
> 
> Looks sensible.

Now we just need to paint the shed. :)

-Peff