Junio C Hamano <gitster@xxxxxxxxx> writes: > Without reading much of the code, my knee jerk reactions are: > > * This probably can (and from the longer term perspective, should) be > done inside a pre-push hook that can decline pushing; > > * I do not think it should use two separate push_refs call into transport > (first with dry-run and second with real). > > Immediately after match_refs() call in transport_push(), you know if > the push is a non-fast-forward (in which case you do not know what you > will be losing anyway because you haven't seen what you are missing > from the other end) or exactly what your fast-forward push will be > sending, so between that call and the actual transport->push_refs() > would be the ideal place to call the hook, with a list of "ref old > new", without running a dry-run. > > for a few reasons. > > (1) When push.confirm is set, you do not want to interact with the user > when the standard input is not a terminal. But an automated script > that runs git-push can still use an appropriate pre-push hook to make > the decision to intervene without human presense. > > (2) As your --show-subjects patch shows, the likes and dislikes of the > output format for confirmation would be highly personal. A separate > hook that is fed list of <ref, old, new> would make it easier to > customize this to suite people's tastes. > > (3) I do not trust the use of the fmt_merge_message() code in this > codepath. That code, like all the major parts of git, relies on > being able to use the object flag bits for its own purpose, and there > is a chance that the way transports (present and future) optimizes > (or may want to optimize in the future) the object transfer by > implementing clever common ancestry discovery, similar to what is > done for the fetch-pack side. Please add ", would be interfered with fmt_merge_message() code contaminating the object flag bits." at the end of this sentence. > > If we force the actual confirmation process out to a separate process > that runs a hook, I do not have to worry about that, which is a huge > relief for maintainability of the system. > > (4) The same objects flag bits contamination issue makes me worried about > your approach of running one transport_push() with dry-run and then > another without. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html