Re: [PATCH] mailer.py: truncate_diff_lines feature

I came into this thread late, sorry.

It's pointless to include anything less than 100% of a diff. There is
no way to know whether the most complicated, review-needing part of
the change is visible or not -- it would just be a roll of the dice
each time. Instead, just omit diffs that exceed the maximum size,
replacing them with a "[diff omitted because too large]" message, and
provide a ViewCVS link (or whatever) for all diffs, both those
included inline and those omitted.

See http://subversion.tigris.org/issues/show_bug.cgi?id=2213 and the
mail threads linked to from it. In particular, see
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=92636 and
the proposed "--diff-alternative" option. I haven't had time to apply
that patch, unfortunately, but I think it's the right way to go...

Best,
-Karl

-- 
www.collab.net  <>  CollabNet  |  Distributed Development On Demand
Alan Barrett <apb@cequrux.com> writes:
> On Wed, 02 Nov 2005, C. Michael Pilato wrote:
> > "truncate_diff_lines" sounds like you're chopping characters off the
> > ends of long lines, not chopping lines off the end of a long diff.
> > Suggest "truncate_diff".
> 
> I went with "truncate_diffs".  A plural word seemed better to me.
> 
> > I'm somewhat ambivalent about this feature addition, but do have one
> > concern: I'd prefer it if the act of truncating a diff would cause an
> > application of that diff data as a patch to fail in some noticable
> > way.
> 
> Done.  I append a new patch.
> 
> [[[
> Add truncate_diffs feature to mailer.py hook script.  If this
> is set to a non-zero value, then diffs longer than the specified
> number of lines will be truncated in email messages.
> 
> * mailer.py (TextCommitRenderer.generate_content): Obtain
>   truncate_diffs value from cfg, and pass it through in the data sent
>   to renderer.render().
>   (TextCommitRenderer.render): Pass data.truncate_diffs through to
>   self._render_diffs().
>   (TextCommitRenderer.render._render_diffs): New truncate_diffs
>   arg, and new logic to implement the truncation.
> * mailer.conf.example: Add commented-out example for truncate_diffs.
> ]]]
> === mailer.py
> ==================================================================
> --- mailer.py	(revision 18413)
> +++ mailer.py	(revision 18535)
> @@ -589,6 +589,12 @@
>    show_nonmatching_paths = cfg.get('show_nonmatching_paths', group, params) \
>        or 'yes'
>  
> +  try:
> +    truncate_diffs = int(
> +        cfg.get('truncate_diffs', group, params))
> +  except ValueError:
> +    truncate_diffs = 0
> +
>    # figure out the lists of changes outside the selected path-space
>    other_added_data = other_removed_data = other_modified_data = [ ]
>    if len(paths) != len(changelist) and show_nonmatching_paths != 'no':
> @@ -617,6 +623,7 @@
>      diffs=DiffGenerator(changelist, paths, True, cfg, repos, date, group,
>                          params, diffsels, diffurls, pool),
>      other_diffs=other_diffs,
> +    truncate_diffs=truncate_diffs,
>      )
>    renderer.render(data)
>  
> @@ -900,10 +907,10 @@
>  
>      w('\nLog:\n%s\n' % data.log)
>  
> -    self._render_diffs(data.diffs)
> +    self._render_diffs(data.diffs, data.truncate_diffs)
>      if data.other_diffs:
>        w('\nDiffs of changes in other areas also in this revision:\n')
> -      self._render_diffs(data.other_diffs)
> +      self._render_diffs(data.other_diffs, data.truncate_diffs)
>  
>    def _render_list(self, header, data_list):
>      if not data_list:
> @@ -934,7 +941,7 @@
>          w('      - copied%s from r%d, %s%s\n'
>            % (text, d.base_rev, d.base_path, is_dir))
>  
> -  def _render_diffs(self, diffs):
> +  def _render_diffs(self, diffs, truncate_diffs):
>      w = self.output.write
>  
>      for diff in diffs:
> @@ -971,8 +978,29 @@
>            w('Binary files. No diff available.\n')
>          continue
>  
> +      nlines = 0
>        for line in diff.content:
> -        w(line.raw)
> +        if truncate_diffs > 0 and nlines >= truncate_diffs:
> +          #
> +          # Truncate the diff, inform the reader, and ensure that
> +          # "patch" will see the truncated diff as an error.
> +          #
> +          # If somebody accidentally feeds the truncated diff to the
> +          # "patch" program, then either the "@@" line or the "***"
> +          # line will be seen by "patch" as a fatal error.  (Which line
> +          # gets reported as an error will depend on exactly where the
> +          # truncation occurs, and it doesn't seem possible to construct
> +          # a single line that will always be reported as an error
> +          # regardless of where it appears.)
> +          #
> +          w('@@ -0,0 +0,1 @@ Diff truncated after %d lines.\n' \
> +            % (truncate_diffs))
> +          w('*** Diff truncated after %d lines.\n' \
> +            % (truncate_diffs))
> +          break
> +        else:
> +          w(line.raw)
> +          nlines = nlines + 1
>  
>  
>  class Repository:
> === mailer.conf.example
> ==================================================================
> --- mailer.conf.example	(revision 18413)
> +++ mailer.conf.example	(revision 18535)
> @@ -224,6 +224,12 @@
>  # Set to 0 to turn off.
>  #truncate_subject = 200
>  
> +# Diff length limit.  The generated diff will be truncated after this many
> +# lines.  The limit applies independently to each file for which a diff
> +# is generated.
> +# Set to 0 to turn off.
> +#truncate_diffs = 200
> +
>  # --------------------------------------------------------------------------
>  
>  [maps]
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
> 
-- 
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org