Re: [PATCHv3 2/2] Warnings before amending published history

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 12, 2012, 15:22 UTC
Message-ID: <7vzk88367g.fsf@alter.siamese.dyndns.org>
In-Reply-To: <vpqvcixyoed.fsf@bauges.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Show 13 quoted lines

> Lucien Kong <Lucien.Kong@ensimag.imag.fr> writes:
>
>>  builtin/commit.c              |   82 ++++++++++++++++++++++++++
>>  sha1_name.c                   |   95 +++++++-----------------------
>>  sha1_name.h                   |  130 +++++++++++++++++++++++++++++++++++++++++
>
> I'm surprised that you need such a big patch. Basically, you're making
> all static functions in sha1_name.c public. If you really need such
> intrusive change, then you should at least explain why in the commit
> message, and most preferably split the patch into one refactoring patch
> to expose the functions and one to use them.
>
> But I suspect what you're looking for is already in cache.h.

I do not think I am going to take this nor the rebase patch that bases its decision on the hardcoded "Does the commit appear in the history of refs/remotes/*anything*?" logic.

At least, there should be "Here is a list of the branches I promised others that I am not to going to rewind." even if you are going to make its default value to be "for-each-ref refs/remotes/". It is too inflexible to be useful otherwise. Not only in the contributor and integrator workflow, but a simple "Alice asks Bob to pull from her Github repository" will be hurt on the "I fixed up the issues you raised. Could you please take another look" round. Besides, I won't be able to amend things outside 'next' but are in 'pu' ;-).

The logic in the patch in this thread to check each ref~$n is not even worth commenting on, but as to the logic in the other "rebase" one, I think it is wasteful to ask "what are the refs that can reach this commit?" when what you really want to know is "is there any ref among this set that can reach this commit?" (the former needs to keep a lot more state). It should be something like looking at the output of:

	git rev-list <list commits you are going to touch here> \
		--not <list tips of refs you have published>

and make sure all the commits you are going to touch appear in the result. Any missing one is reachable from the refs you have published and you may not want to rebase.

It may be an interesting thought experiment to see if you can take advantage of the inherent ancestry relationship among the list of commits you are going to touch. The later commits that will be replayed in a rebase are very likely to be children of earlier one, so in theory, if you can identify the set of topologically earliest commits that will be replayed, you only need to check them, and if you can cheaply come up with that set of earliest commits, the above rev-list may become cheaper.