Re: Patches for git-push --confirm and --show-subjects

Junio C Hamano [off-list ref] 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.