Re: [PATCH v3 1/4] automatically ban strcpy()

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 26, 2018, 17:33 UTC
Message-ID: <xmqqfu05rjfc.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180726072105.GA6057@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> So here's a replacement for just patch 1 (I'm assuming this creates less
> work than re-posting them all, but it may not be if Junio prefers
> dealing with a whole new mbox rather than a "rebase -i", "reset --hard
> HEAD^", "git am" -- let me know if you'd prefer it the other way).

A single patch replacement that is clearly marked which one to replace and which other ones to keep, like you did here, is fine. The amount of work is about the same either way.

0) I would first do these to make sure that I can replace:

 $ git checkout jk/banned-functions
 $ git log --first-parent --oneline master..
 $ git log --first-parent --oneline next..

If 'next' has some patches that are not in 'master' from the topic, I must refrain from replacing them (in this case, there is no such issue).

1-a) With a wholesale replacement,

 $ git checkout master...	;# try to keep the same base
 $ git am -s mbox		;# on detached HEAD
 $ git tbdiff ..@{-1} @{-1}..	;# or range-diff

If the range-/tbdiff tells me that earlier N patches are the same, the above is followed by something like (pardon off-by-one)

 $ git rebase --onto @{-1}~N HEAD~N

to preserve as many original commits as possible.

1-b) With a single patch replacement, it is quite different.

 $ git checkout HEAD~4	        ;# we are replacing 1/4 of the original
 $ git am -s mbox               ;# that single patch
 $ git show-branch HEAD @{-1}

That would give me something like this

    * [HEAD] automatically ban strcpy()
     ! [@{-1}] banned.h: mark strncpy() as banned
    --
    *  [HEAD] automatically ban strcpy()
     + [@{-1}] banned.h: mark strncpy() as banned
     + [@{-1}^] banned.h: mark sprintf() as banned
     + [@{-1}~2] banned.h: mark strcat() as banned
     + [@{-1}~3] automatically ban strcpy()
    -- [HEAD^] Merge branch 'sb/blame-color' into jk/banned-function

The most natural thing to do at this point is

 $ git cherry-pick -3 @{-1}

But we know range-pick is buggy and loses core.rewriteref, so instead I did this, which I know carries the notes forward:

 $ git rebase --onto HEAD @{-1}~3 @{-1}^0

    Side note: The point of last "^0" is that I do not want to lose
    the topic branch jk/banned-functions not just yet.

If I need to re-apply separate pieces of the original history, it becomes very painful to emulate these multiple cherry-picks with multiple "rebase --onto", though. That is where "the amount of work is about the same" comes from. If cherry-pick worked correctly, selective replacement should be less work for me.

Anyway, we already preserved as many original commits, but unlike 1-a, did so manually when we decided to replace selective patches. So there is no further rebasing with this approach.

2) I now have two diverged histories in HEAD and @{-1} that I can
compare with range-/tbdiff in either case:

 $ git tbdiff ..@{-1} @{-1}..

After the usual inspection and testing, replacement is concluded by

 $ git checkout -B @{-1}

which takes me back to (an updated) jk/banned-functions topic.