{"thread":{"id":"13300","subject":"entry terminator/separator behavior in show_log()","startedAt":"2008-04-28T04:55:05Z","lastAt":"2008-04-29T08:58:42Z","messageCount":3,"participants":["Adam Simpkins","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"75343","messageId":"20080428045504.GA24981@adamsimpkins.net","threadId":"13300","inReplyTo":null,"subject":"entry terminator/separator behavior in show_log()","fromName":"Adam Simpkins","fromEmail":"adam@adamsimpkins.net","sentAt":"2008-04-28T04:55:05Z","receivedAt":"2008-04-28T04:55:05Z","isPatch":false,"sender":{"key":"adam@adamsimpkins.net","avatar":"https://gravatar.com/avatar/d3fd2c0b3e2d2136b56e95726ee03227bee4eb562f627dcd3ebca0623fa05054?d=mp&s=160"},"body":"This is a write-up describing the current entry terminator/separator\nbehavior of show_log().  I wrote it while updating my \"log --graph\"\ncode to work with Junio's new use_terminator flag.\n\nI was initially hoping to find places where we could simplify the\nbehavior.  However, after examining all of the cases, I'm not sure if\nthere is much we can do without breaking backwards compatibility too\nbadly.  Perhaps we should just check this description into\nDocumentation/technical, for the next person who needs to work with\nthis part of the code.\n\nTowards the end of the document, I've also listed how the new --graph\noption will interact with each of the cases.  I'd love to have\nfeedback to see if my current plans sound reasonable.\n\n\n\nVariables affecting the behavior\n--------------------------------\n\n- The sep argument to show_log()\n\n  As far as I can tell, this is always \"\".  show_log() is sometimes\n  invoked with a sep argument of diffopt->msg_sep.  However,\n  \"git grep -w msg_sep\" shows that msg_sep is never set to anything but\n  the empty string.\n\n  Perhaps we could just remove this argument, to reduce complexity and\n  confusion?\n\n- The line_termination member of struct diff_options\n\n  This is normally '\\n', but is set to '\\0' if the -z option is\n  supplied.\n\n- The use_terminator flag in struct rev_info\n\n  This flag is enabled by --pretty=oneline and --pretty=format.  It is\n  off for all other formats.\n\n  This flag has no effect if opt->verbose_header is not set.  If\n  opt->verbose_header is set, the behavior is as follows:\n\n  When use_terminator is set, a newline is added to the end of each log\n  entry.  diffopt.line_termination is not printed in between entries.\n  When use_terminator is not set, diffopt.line_termination is printed\n  before all entries except the very first one.\n\n- Whether or not the log entry ends in a terminating newline\n\n  The log entry for CMIT_FMT_ONELINE never ends in a newline.\n  The log entry for CMIT_FMT_USERFORMAT only ends in a newline if the\n  user explicitly places a newline at the end of the format.\n\n  For all other formats, the entry always ends in a terminating newline.\n\n- Whether or not the log message buffer is empty\n\n  If use_terminator is set and the message buffer returned by\n  pretty_print_commit() is empty, no newline is printed between entries.\n\n  This can happen, for example, if CMIT_FMT_USERFORMAT is used with an\n  empty format string.  Even though pretty_print_commit() returns an\n  empty buffer, the log entry itself does not necessarily have to be\n  empty.  For example, the log size will be printed if if the --log-size\n  argument is used.\n\n  I'm not sure if there any are other code paths that can also cause\n  this to happen.\n  \n\nCurrent Behavior\n----------------\n\n(1) For all formats except CMIT_FMT_ONELINE and CMIT_FMT_USERFORMAT:\n\n    diffopt->line_termination is printed before each log entry but the\n    first.  This results in diffopt->line_termination in between each\n    entry, and not after the final entry.\n  \n    (1.1)\n      When diffopt->line_termination is '\\0', this prints a NUL\n      character between log entries.\n\n    (1.2)\n      When diffopt->line_termination is '\\n', this essentially adds an\n      extra blank line of padding between log entries, since each log\n      entry already ends in a newline.\n\n(2) For CMIT_FMT_ONELINE and CMIT_FMT_USERFORMAT:\n\n    (2.1)\n      If use_terminator is set (--pretty=oneline and --pretty=tformat):\n\n      (2.1.1)\n        If the message buffer returned by pretty_print_commit() is\n        non-empty:\n\n        A newline is printed at the end of each entry.  Since the log\n        entry does not end in a newline already, the newline ends the\n        last line of the log entry, and there is no extra blank line\n        between entries.\n\n        With --pretty=tformat, if the user's format explicitly ends in a\n        newline, this will create a blank line at the end of each entry.\n        (This looks like a padding line, very similar to case 1 above.\n        However, it shows up even after the very last entry.)\n\n      (2.1.2)\n        If the message buffer returned by pretty_print_commit() is\n        empty:\n\n        A newline is not printed at the end of each entry.  Therefore\n        there are no blank lines between log entries.  If the log\n        entries are completely empty, nothing at all is printed.\n\n    (2.2)\n      If use_terminator is not set (--pretty=format):\n\n      diffopt->line_termination is printed before each log entry but the\n      first.  This results in diffopt->line_termination in between each\n      entry, and not after the final entry.\n\n      (2.2.1)\n        When diffopt->line_termination is '\\0', this is reasonable\n        behavior, and makes sense for consumption by other scripts.\n\n      (2.2.2)\n        When diffopt->line_termination is '\\n':\n\n        (2.2.2.1)\n          If the user's format is non-empty and does not end in an\n          explicit newline, log entries do not end in a newline.\n          However, the line_termination character printed before the\n          next entry appears to terminate the previous entry, so it\n          looks human-readable.  There are no blank lines between\n          entries.\n\n          However, no newline is printed after the very last entry.\n          This seems like a bug to me, since I would expect that the\n          output is normally being read by a human instead of another\n          script when diffopt->line_termination is '\\n'.\n\n          Fortunately, the log output is normally piped through less,\n          and less hides the fact that the output does not end in a\n          newline.  It prints a terminating newline for us if the git\n          output doesn't end in a newline.\n\n        (2.2.2.2)\n          If the user's format does end in an explicit newline, the\n          diffopt->line_termination character between entries creates a\n          blank line between entries.  The output looks exactly like\n          case 1: a blank line show up between entries, but not after\n          the very last entry.\n\n        (2.2.2.3)\n          If the user's format is the empty string, the log entry will\n          either be completely empty, or will already end in a newline.\n          The diffopt->line_termination characters printed between\n          entries appear as blank padding lines between entries.  If the\n          log entries are completely empty the resulting output\n          consists solely of padding lines.\n\n(3) opt->verbose_header is unset\n\n    Only a single line of output is printed, terminated with the\n    diffopt->line_termination character.\n\n    Note that this differs from case 1!  The line_termination character\n    is printed even after the very last entry.\n\n\nComments on the current behavior\n--------------------------------\n\nThe current behavior is fairly complicated.  There are quite a few\ncorner cases, especially around uncommon behavior (for example,\n\"--pretty=format:\" with an empty format string).  It would be nice to\neliminate some of the complexity.  However, doing so would change the\nbehavior and potentially break existing user scripts relying on this\nbehavior.\n\n- Cases 1.1 and 1.2 make perfect sense to me.\n\n- Case 2.1 makes perfect sense to me.\n\n- Case 2.1.2 seems reasonable, but not really necessary.  It might be\n  nicer to simply always print a terminating newline even if the message\n  buffer is empty.  This would appear as an extra blank line after each\n  entry.\n  \n  The --pretty=tformat option is quite new, so I doubt that many people\n  would complain if we changed this behavior.\n\n- Case 2.2.1 makes perfect sense.  There are probably lots of scripts\n  out there relying on this behavior.\n\n- I don't really like the behavior for any of the cases under 2.2.2.\n\n  I especially don't like the fact that the output does not end in a\n  terminating newline for case 2.2.2.1.\n\n  If the output is for human consumption, the user probably really wants\n  case 2.1 (i.e., --pretty=tformat instead of --pretty=format.)\n  Hopefully most scripts that parse the log output use the -z option,\n  but there are probably also many that don't.  Changing this behavior\n  would break these scripts.  Teaching users to use --pretty=tformat\n  instead of --pretty=format will probably cause less pain than\n  breaking backwards compatibility.\n\n\nDesired behavior with the new --graph option\n--------------------------------------------\n\n- Case 1.1\n  (All formats but CMIT_FMT_ONELINE and CMIT_FMT_USERFORMAT,\n  line_termination is '\\0')\n\n  The graph output should be printed before each line in the log entry.\n\n  Graph output should not be printed between the last newline in the log\n  entry and the line_termination character ('\\0') for the next log entry.\n\n- Case 1.2\n  (All formats but CMIT_FMT_ONELINE and CMIT_FMT_USERFORMAT,\n  line_termination is '\\n')\n\n  The graph output should be printed before each line in the log entry.\n\n  For each entry but the first, graph padding should also be printed\n  *before* the line_termination character.  Otherwise the blank padding\n  line between entries will be completely blank, which would leave gaps\n  in the graph output.\n\n- Cases 2.1.1 and 2.1.2\n  (--pretty=oneline and --pretty=tformat)\n\n  The graph output should be printed before each line in the log entry.\n\n  There is no additional separator between entries, so there is nothing\n  else to worry about here.  All log entries always end in a newline,\n  which also helps keep things simple.\n\n- Case 2.2.1\n  (--pretty=format and line_termination is '\\0')\n\n  The graph output should be printed before each line in the log entry.\n  Graph output should not be printed between the last newline in the log\n  entry and the line_termination character ('\\0') for the next log entry.\n  \n  This behavior is identical to that for case 1.1.\n\n- Case 2.2.2.1\n  (--pretty=format, line_termination is '\\n', and with a non-empty\n  format that doesn't end with a newline)\n\n  The graph output should be printed before each line in the log entry.\n\n  Graph output should not be printed before the line_termination\n  character, since this character is merely terminating the last line of\n  output from the previous entry.\n\n- Cases 2.2.2.2 and 2.2.2.3\n  (--pretty=format, line_termination is '\\n', and with a format that\n  ends with an explicit newline, or with an empty format)\n\n  The graph output should be printed before each line in the log entry.\n\n  Graph output *should* be before the line_termination character,\n  since this character is creating a new blank padding line.\n\n- Case 3\n  (opt->verbose_header is unset)\n\n  Each log entry consists of a single line, terminated with the\n  line_termination character.  The graph output should be printed before\n  each line.\n\n  However, this case currently can't happen via the \"git log\" command,\n  since this command always sets the verbose_header flag.\n\nWith the current code, there's not really enough information for\nthe graphing code in show_log() to distinguish between cases cases\n2.2.2.1 and 2.2.2.{2,3}.\n\nIn order to do so, I plan to add a new flag to struct rev_info to\nindicate whether or not the previous log entry ended in a newline.  The\nnext invocation of show_log() can check this flag to see if it should\nprint graph padding before the line_termination character or not.\n\n\n\nThanks for reading, if you made it all the way down here!\n\n-- \nAdam Simpkins\nadam@adamsimpkins.net\n"},{"id":"75393","messageId":"7vtzhmc63w.fsf@gitster.siamese.dyndns.org","threadId":"13300","inReplyTo":"20080428045504.GA24981@adamsimpkins.net","subject":"Re: entry terminator/separator behavior in show_log()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-28T16:10:59Z","receivedAt":"2008-04-28T16:10:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Simpkins <adam@adamsimpkins.net> writes:\n\n> Variables affecting the behavior\n> --------------------------------\n>\n> - The sep argument to show_log()\n>\n>   As far as I can tell, this is always \"\".  show_log() is sometimes\n>   invoked with a sep argument of diffopt->msg_sep.  However,\n>   \"git grep -w msg_sep\" shows that msg_sep is never set to anything but\n>   the empty string.\n>\n>   Perhaps we could just remove this argument, to reduce complexity and\n>   confusion?\n\nYes.  39bc9a6 (Add msg_sep to diff_options, 2006-06-25) and f005df3\n(diff-tree: Use ---\\n as a message separator, 2006-06-27) introduced the\nvariable and used it as the mechanism to add \"---\\n\" at the end of the\nmessage, which was a mistake, because it treated \"---\\n\" as a part of the\nmessage even though it is not --- it is a separator between the message\nand the diff part if and only if the diff exists.\n\nLater, 3969cf7 (Fix some more diff options changes., 2006-06-27) corrected\nthis and made generation of --- as part of the log-tree, where \"the diff\nfor the commit\" is generated.  The variable is not used anymore and we can\nsafely remove it.\n\n> - Whether or not the log entry ends in a terminating newline\n>\n>   The log entry for CMIT_FMT_ONELINE never ends in a newline.\n>   The log entry for CMIT_FMT_USERFORMAT only ends in a newline if the\n>   user explicitly places a newline at the end of the format.\n>\n>   For all other formats, the entry always ends in a terminating newline.\n\nHmm, interesting.  This is true even when the original commit log message\nends with an incomplete line.\n\n> - Whether or not the log message buffer is empty\n>\n>   If use_terminator is set and the message buffer returned by\n>   pretty_print_commit() is empty, no newline is printed between entries.\n>\n>   This can happen, for example, if CMIT_FMT_USERFORMAT is used with an\n>   empty format string.  Even though pretty_print_commit() returns an\n>   empty buffer, the log entry itself does not necessarily have to be\n>   empty.  For example, the log size will be printed if if the --log-size\n>   argument is used.\n\nThis could just be a bug.  I do not think of a good reason to treat an\nempty entry any differently from a single line entry that happens to have\nzero characters on it.\n\n> (2) For CMIT_FMT_ONELINE and CMIT_FMT_USERFORMAT:\n>     (2.2)\n>       If use_terminator is not set (--pretty=format):\n>\n>       diffopt->line_termination is printed before each log entry but the\n>       first.  This results in diffopt->line_termination in between each\n>       entry, and not after the final entry.\n\nIOW, \"use separators between entries.\"\n\n> - Case 2.1.2 seems reasonable, but not really necessary.  It might be\n>   nicer to simply always print a terminating newline even if the message\n>   buffer is empty.  This would appear as an extra blank line after each\n>   entry.\n>\n>   The --pretty=tformat option is quite new, so I doubt that many people\n>   would complain if we changed this behavior.\n\nConcurred.\n\n> - Case 2.2.1 makes perfect sense.  There are probably lots of scripts\n>   out there relying on this behavior.\n\nI would agree with the second sentence, but not necessarily with the\nfirst.  For script consumption, terminator is easier to deal with than\nseparator if the script is streaming; separator is easier if the script\ngobbles up everything and then uses split(/$separator/, $everything).\nThey both have their uses.\n\n> - I don't really like the behavior for any of the cases under 2.2.2.\n>\n>   I especially don't like the fact that the output does not end in a\n>   terminating newline for case 2.2.2.1.\n\nThis is exactly why I did tformat so that we do not have to have a complex\nspecial case (Jeff and I exchanged a few weatherbaloon patches on the list\ntrying out heuristics) to avoid breaking existing scripts that use format.\n"},{"id":"75514","messageId":"20080429085839.GA19705@adamsimpkins.net","threadId":"13300","inReplyTo":"7vtzhmc63w.fsf@gitster.siamese.dyndns.org","subject":"Re: entry terminator/separator behavior in show_log()","fromName":"Adam Simpkins","fromEmail":"adam@adamsimpkins.net","sentAt":"2008-04-29T08:58:42Z","receivedAt":"2008-04-29T08:58:42Z","isPatch":false,"sender":{"key":"adam@adamsimpkins.net","avatar":"https://gravatar.com/avatar/d3fd2c0b3e2d2136b56e95726ee03227bee4eb562f627dcd3ebca0623fa05054?d=mp&s=160"},"body":"On Mon, Apr 28, 2008 at 09:10:59AM -0700, Junio C Hamano wrote:\n> Adam Simpkins <adam@adamsimpkins.net> writes:\n> \n> > - I don't really like the behavior for any of the cases under 2.2.2.\n> >\n> >   I especially don't like the fact that the output does not end in a\n> >   terminating newline for case 2.2.2.1.\n> \n> This is exactly why I did tformat so that we do not have to have a complex\n> special case (Jeff and I exchanged a few weatherbaloon patches on the list\n> trying out heuristics) to avoid breaking existing scripts that use format.\n\nHmm.  Here's another possible option to kick around:\n\nModify the argument parsing code to set use_terminator for\n--pretty=format when -z is not supplied.  In other words:\n\n  - If the -z argument is used (diffopt->line_termination is '\\0'),\n    the code has separator semantics, just like it always has.\n    '\\0' appears between the entries, and not at the end of the last\n    one.\n\n  - If the -z argument is not used (diffopt->line_termination is '\\n'),\n    the code has terminator semantics.  '\\n' appears at the end of each\n    entry.\n\nThe only change to the current behavior is that if the -z option is\nnot used, an extra newline will appear at the end of the output.  This\nmight confuse some scripts that don't use -z.\n\nThe nice thing about this change is that now all of the cases under\n2.2.2 from my initial email behave identically.  These cases are the\nmost annoying to distinguish and handle correctly for the new --graph\ncode, so it would make the graph logic simpler.\n\nDo you think this change would be acceptable, or would it still break\ntoo many scripts?\n\n-- \nAdam Simpkins\nadam@adamsimpkins.net\n"}]}