git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFD] Rewriting safety - warn before/when rewriting published history

From
Johan Herland <johan@herland.net>
Date
Feb 6, 2012, 15:59 UTC
Message-ID
<CALKQrgcN9Miq7SN_ETvjboRkHRFzcZejrQfy36BQFxYDnVPP6w@mail.gmail.com>
In-Reply-To
<201202061544.14417.jnareb@gmail.com>
On Mon, Feb 6, 2012 at 15:44, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sun, 5 Feb 2012, Johan Herland wrote:
> Relying on (default) hooks to implement this feature has the disadvantage
> that it wouldn't be turned on by default... while this feature would be
> most helpful for users new to git (scared by refuse to push).

True. I too believe that this will be most helpful if it is enabled by default. That said, the easiest way to get there might be through first demonstrating that it works in practice when implemented as hooks.

> I am not sure either if everything (wrt. safety net) can be implemented
> via hooks.  One thing that I forgot about is preventing rewinding of
> branch past the published commit using e.g. "git reset --hard <commit>".
> Unless `pre-rewrite` hook could be used for that safety too...

Hmm. I don't think we'll be able to "plug" all the holes that might leave the user in a rewritten state (e.g. what if the user (possibly with the help of some tool) does an "echo $SHA1 > .git/refs/head/master"?). And trying to plug too many holes might end up annoying more experienced users who "know what they're doing". Instead we might want to add a client-side check at push time. I realize that this check is already done by the remote end, but the client-side might be able to give a more helpful response along the lines of:

  You are trying to push branch X to remote Y, but remote Y already
has a branch X that is N commits in front of you. You may want to
rebase your work on top of the remote branch (see 'git pull
--rebase'), If you instead force this push (with --force), you will
remove those N commits, and replace them with the M last commits from
your branch X.
  (followed by a list of the remote N and local M commits, respectively)
[...]
Show 8 quoted lines
>> If you use 'git notes' to annotate 'public' and 'secret' states, then
>> you can also use the --show-notes=<ref> option to let show/log display
>> the annotations on 'public'/'secret' commits.
>
> First, in my opinion annotating _all_ commits with their phase is I think
> out of question, especially annotating 'public' commits.  I don't think
> git-notes mechanism would scale well to annotating every commit; but
> perhaps this was tested to work, and I am mistaken.

First, we don't need to annotate _all_ commits. For the 'public' state, we only annotate the last/tip commit that was pushed/published. From there, we can defer that all ancestor commits are also 'public'. For the 'secret' state, we do indeed annotate _all_ secret commits, but I believe this will be a somewhat limited number of commits. If your workflow forces you to annotate millions of commits as 'secret', I claim there is something wrong with your workflow.

Second, git-notes were indeed designed scale well to handle a large number of notes, up to the same order of magnitude as the number of commits in your repo. (When git-notes was originally written, I successfully tested it on versions of a linux-kernel repo where every single commit was annotated). In this case, the number of 'public' annotations in your repo would be equal to the number of pushes you do, and the number of 'secret' annotations would be equal to the number of 'secret' commits in your repo. I'd expect both of these numbers to be orders of magnitude smaller than the total number of commits in your repo (given a fairly typical workflow in a fairly typical repo).

> Second, I have doubts if "phase" is really state of an individual commit,
> and not the feature of revision walking.

I believe the 'public' state is a "feature of revision walking" (i.e. one annotated 'public' commit implies that all its ancestors are also 'public'). However, the 'secret' state should be bound to the individual commit, IMHO.

Show 11 quoted lines
> Take for example the situation where given commit is reference by
> remote-tracking branch 'public/foo', and also by two local branches:
> 'foo' with upstream 'public/foo', and local branch 'bar' with no upstream.
>
> Now it is quite obvious that this feature should prevent rewriting 'foo'
> branch, for which commits are published upstream.  But what about branch
> 'bar'?  Should we prevent rewriting (e.g. rebase) here too?  What about
> rewinding 'bar' to point somewhere else.  What if 'bar' is really detached
> HEAD?
>
> These questions need to be answered...

Good point. There are two questions we may need to answer: "Has commit X ever been published?", and "Has commit X ever been published in the context of branch Y?". In the latter case, we do indeed need to take the upstream branch into account.

Basically, there are three different "levels" for this rewrite/publish protection to run at:

1. Do not meddle at all. This is the current behavior, and assumes
that if the user rewrites and pushes something, the user knows what
he/she is doing, and Git should not meddle (obviously unless the
server refuses the push).
2. Warn/refuse rewriting commits in your upstream. This would only
check branch X against its registered upstream. Only if there is a
registered upstream, and you're about to rewrite commits that are
reachable from the upstream remote-tracking branch, should Git
intervene and warn/refuse the rewrite. This level would IMHO provide
most of the benefit, and little or no trouble (i.e. false positives).
3. Warn/refuse rewriting _any_ 'public' commit. Refuse to rewrite any
commit that is reachable from any remote-tracking branch. Some would
say that this is a Good Thing(tm), since it prevents a commit from
being _copied_ (i.e. rebased or cherry-picked) between branches (you'd
be in this camp if you run a tightly-controlled workflow, where you
e.g. mandate upmerging patches from the oldest applicable branch
instead of cherry-picking patches from a newer branch). However, other
people would say that this is too limiting, and imposes unnecessary
rules on the workflow of the project (where e.g. copying (by way of
git-rebase) a topic branch from one place to another would cause an
annoying false positive).
[...]
Have fun! :)
...Johan
-- 
Johan Herland, <johan@herland.net>
www.herland.net
Previous: Jakub NarebskiNext: Jakub Narebski
Message 13 of 34 in “[RFD] Rewriting safety - warn before/when rewriting published history”
  1. Jakub NarebskiFeb 4, 2012
  2. Ben WaltonFeb 5, 2012
  3. Jakub NarebskiFeb 5, 2012
  4. Steven MichalskeFeb 6, 2012
  5. Johan HerlandFeb 6, 2012
  6. Jakub NarebskiFeb 6, 2012
  7. Steven MichalskeApr 7, 2012
  8. Jakub NarebskiFeb 5, 2012
  9. Johan HerlandFeb 5, 2012
  10. Jakub NarebskiFeb 5, 2012
  11. Johan HerlandFeb 5, 2012
  12. Jakub NarebskiFeb 6, 2012
  13. Johan HerlandFeb 6, 2012
  14. Jakub NarebskiFeb 6, 2012
  15. Johan HerlandFeb 6, 2012
  16. Jakub NarebskiFeb 7, 2012
  17. Johan HerlandFeb 7, 2012
  18. Jakub NarebskiFeb 10, 2012
  19. Philip OakleyFeb 10, 2012
  20. Johan HerlandFeb 11, 2012
  21. Jakub NarebskiFeb 11, 2012
  22. [RFC] pre-rebase: Refuse to rewrite commits that are reachable from upstreamJohan Herland, Feb 20, 2012
  23. Johan HerlandFeb 20, 2012
  24. Junio C HamanoFeb 20, 2012
  25. Johan HerlandFeb 21, 2012
  26. Junio C HamanoFeb 21, 2012
  27. Johan HerlandFeb 21, 2012
  28. Junio C HamanoFeb 21, 2012
  29. Dave ZarzyckiFeb 21, 2012
  30. Jeff KingFeb 22, 2012
  31. Dave ZarzyckiFeb 22, 2012
  32. Steven MichalskeApr 7, 2012
  33. Steven MichalskeApr 7, 2012
  34. Ronan KeryellFeb 7, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.