[PATCH 4 of 7] patch: drop internalpatch checks for NoHunks

[Mads Kiilerich]

Matt Mackall wrote, On 04/28/2010 06:33 AM:
...
> 4 looks suspicious, you'll need to tell us
> more about that.
>    

and

Augie Fackler wrote, On 04/26/2010 06:06 PM:
> On Mon, Apr 26, 2010 at 7:37 AM, Mads Kiilerich<mads at kiilerich.com>  wrote:
>    
>> # HG changeset patch
>> # User Mads Kiilerich<mads at kiilerich.com>
>> # Date 1272280862 -7200
>> # Node ID 50c548cc4249b964cb5012307389f57377ea8149
>> # Parent  7e65479fe7b82026f9016fb0bfa4a2a2c4917e57
>> patch: drop internalpatch checks for NoHunks
>>
>> This will change the behavior for some (slightly bogus) patches, but the new
>> behavior is IMHO both better and good.
>>      
> Can you describe what the behavior change will be? That feels like it
> belongs in the commit message.
>    


At some point during development of the other patches I got some 
failures without this patch - but I can't reproduce the case and this 
patch isn't necessary.

I tried to understand the code and the semantics of the "empty" state 
variable and clean it up, but I had to give up. I couldn't see what the 
purpose with NoHunks was.

There is apparently no tests of NoHunks. This patch introduces no errors 
in the test suite.

In my opinion we don't have to recognize all malformed patches as long 
as we handle the well-formed patches correct and don't behave too badly 
on malformed patches.

The "internal patcher failed" message and fallback to external patcher 
seems very odd.

...

Admitted: There is no strong evidence here.

Could someone please explain in test or code comments why NoHunks and 
the state variables are needed and for which cases - and thus prove that 
this is a bad idea?

/Mads


>> diff --git a/mercurial/patch.py b/mercurial/patch.py
>> --- a/mercurial/patch.py
>> +++ b/mercurial/patch.py
>> @@ -18,9 +18,6 @@
>>   class PatchError(Exception):
>>      pass
>>
>> -class NoHunks(PatchError):
>> -    pass
>> -
>>   # helper functions
>>
>>   def copyfile(src, dst, basedir):
>> @@ -1018,11 +1015,6 @@
>>      BFILE = 1
>>      context = None
>>      lr = linereader(fp)
>> -    # gitworkdone is True if a git operation (copy, rename, ...) was
>> -    # performed already for the current file. Useful when the file
>> -    # section may have no hunk.
>> -    gitworkdone = False
>> -    empty = None
>>
>>      while True:
>>          newfile = newgitfile = False
>> @@ -1034,7 +1026,6 @@
>>                  current_hunk.fix_newline()
>>              yield 'hunk', current_hunk
>>              current_hunk = None
>> -            empty = False
>>          if ((sourcefile or state == BFILE) and ((not context and x[0] == '@') or
>>              ((context is not False) and x.startswith('***************')))):
>>              try:
>> @@ -1052,19 +1043,16 @@
>>              if emitfile:
>>                  emitfile = False
>>                  yield 'file', (afile, bfile, current_hunk)
>> -                empty = False
>>          elif state == BFILE and x.startswith('GIT binary patch'):
>>              current_hunk = binhunk(changed[bfile])
>>              hunknum += 1
>>              if emitfile:
>>                  emitfile = False
>>                  yield 'file', ('a/' + afile, 'b/' + bfile, current_hunk)
>> -                empty = False
>>              current_hunk.extract(lr)
>>          elif x.startswith('diff --git'):
>>              # check for git diff, scanning the whole patch file if needed
>>              m = gitre.match(x)
>> -            gitworkdone = False
>>              if m:
>>                  afile, bfile = m.group(1, 2)
>>                  if not git:
>> @@ -1079,7 +1067,6 @@
>>                  if gp and (gp.op in ('COPY', 'DELETE', 'RENAME', 'ADD')
>>                             or gp.mode):
>>                      afile = bfile
>> -                    gitworkdone = True
>>                  newgitfile = True
>>          elif x.startswith('---'):
>>              # check for a unified diff
>> @@ -1107,12 +1094,6 @@
>>              afile = parsefilename(x)
>>              bfile = parsefilename(l2)
>>
>> -        if newfile:
>> -            if empty:
>> -                raise NoHunks
>> -            empty = not gitworkdone
>> -            gitworkdone = False
>> -
>>          if newgitfile or newfile:
>>              emitfile = True
>>              state = BFILE
>> @@ -1120,14 +1101,10 @@
>>      if current_hunk:
>>          if current_hunk.complete():
>>              yield 'hunk', current_hunk
>> -            empty = False
>>          else:
>>              raise PatchError(_("malformed patch %s %s") % (afile,
>>                               current_hunk.desc))
>>
>> -    if (empty is None and not gitworkdone) or empty:
>> -        raise NoHunks
>> -
>>
>>   def applydiff(ui, fp, changed, strip=1, sourcefile=None, eolmode='strict'):
>>      """Reads a patch from fp and tries to apply it.
>> @@ -1332,21 +1309,7 @@
>>              return externalpatch(patcher, args, patchname, ui, strip, cwd,
>>                                   files)
>>          else:
>> -            try:
>> -                return internalpatch(patchname, ui, strip, cwd, files, eolmode)
>> -            except NoHunks:
>> -                ui.warn(_('internal patcher failed\n'
>> -                          'please report details to '
>> -                          'http://mercurial.selenic.com/bts/\n'
>> -                          'or mercurial at selenic.com\n'))
>> -                patcher = (util.find_exe('gpatch') or util.find_exe('patch')
>> -                           or 'patch')
>> -                ui.debug('no valid hunks found; trying with %r instead\n' %
>> -                         patcher)
>> -                if util.needbinarypatch():
>> -                    args.append('--binary')
>> -                return externalpatch(patcher, args, patchname, ui, strip, cwd,
>> -                                     files)
>> +            return internalpatch(patchname, ui, strip, cwd, files, eolmode)
>>      except PatchError, err:
>>          s = str(err)
>>          if s:
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel at selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel
>>
>>