Bug 127765

Created attachment 53807 [details]
Proposed patch to the quilt package that implements recognition of patch options -i, --input= and --strip=

The patch has also been sent to quilt-dev@nongnu.org.

Looks good, except I would prefer using getopt(1) for extracting the patch 
options.  This would really catch all cases, and avoid the mess. I would also 
prefer to keep -i and --input options in the patch command line. 
 
(When taking a look at inspect.in I also noticed that path_search() can be 
replaced by setting PATH to ${PATH#*:} before invoking the real command.) 

(Today I seem to have my own opinions. I have reasons as well. Am I
right/good/useful? Hmmm... (Olaf, doubting to see the whole picture):)

> Looks good, except I would prefer using getopt(1) for extracting the patch
> options.  This would really catch all cases, and avoid the mess.

I thought about this and looked through the options of patch. Here are the
reasons why I did not use getopt:

 - it adds maintenance: every change of options in "patch" (and any tool that
   needs a wrapper) needs to be tracked manually in the getopt line in the
   wrapper; without getopt, "patch" can be changed without the wrapper needing
   a change
 - some options, esp. in some tools (like tar) can not be parsed with getopt,
   or not in the same way the tool does it
 - there are many options to track (currently about 61 in patch, including
   short-/long-variants)
 - AFAIC tell, most of the options of patch do not influence the behaviour of
   "quilt setup", and do not seem to influence other quilt functions; this may
   not be correct for some options, but for the reasons above I would suggest
   to parse them without getopt

> I would also
> prefer to keep -i and --input options in the patch command line.

This can be done of course. ATM I do not understand the benefit and redirecting
STDIN seemed to be simpler.

> (When taking a look at inspect.in I also noticed that path_search() can be
> replaced by setting PATH to ${PATH#*:} before invoking the real command.)

path_search() seemed to be safer, because other layers (macros in RPM?) can add
their layer of wrappers to PATH. But maybe I have not thought this through and
you are right...

> >  - it adds maintenance: every change of options in "patch" (and any tool
> > that needs a wrapper) needs to be tracked manually in the getopt line in
> > the wrapper; without getopt, "patch" can be changed without the wrapper
> > needing a change
> 
> Good point.

(Blush.) ;)

> Not an issue with patch I believe.

Right.

> Options can be any of this:
>   -i foo
>   -ifoo
>   --input foo
>   --input=foo
>   -Rifoo
> 
> It may make sense to ignore the last case, it's quite weird.

Indeed, combinations of options like "-Rifoo" are not easily parsed
without getopt and a complete option specification.

getopt(1) (even with "-q") will and can not parse these correctly. To do
this a tool must at least know which options take arguments (or,
alternatively, which options do not take arguments) in order to parse
-fRBxyz correctly into -f -R -B "xyz". (For a wrapper, even if -f and -R
were not in the option specification, the tool/getopt can assume that
they are simple short options and let the wrapped tool take care of
error handling.) But it is not very tempting to add this capability to
getopt, because specifying only the argument-taking options in the
wrapper is similar in maintenance effort to specifying all options.

I have changed the patch now so that all of the above combinations with
"-i" and "--input" are parsed. Actually, any combination of one or two
non-argument-taking options in combination with -i or -p are parsed. The
code now knows about the current non-argument-taking "patch"-options.

In this way, all kinds of changes to "patch"-options can be made without
requiring maintenance in the wrapper, as long as no non-argument-taking
short option is turned into an argument-taking short option and no new
non-argument-taking short option is created and actually used in a spec
file in combination with -i or -p.

Of course, this does not solve all problems, but it may be a reasonable
tradeoff. And IMO it is better than before.

(BTW, I have long believed it would be best to have a central repository
for all syntax specifications which are then used by wrappers, config
file parsers (e.g. YaST agents) AND the original tools themselves. A
common parser would be needed, which to my knowledge did not materialize
yet. Admittedly, it is not clear yet what a quite generic parser would
look like. Nowadays, a bit more is known about the language classes that
it would need to be able to parse (and, e.g. that some efficient sort of
multi-pass often makes parsing simpler for programming languages), but a
generic specification of the language types used and useful in computing
is still missing. I have done some work (incl. code) on this though, and
OTOH so far I did not encounter a reason (including performance,
maintainability and debugability) why it should not be possible.)

> > > I would also
> > > prefer to keep -i and --input options in the patch command line.
> I would like to avoid changing the command line of the tool invoked in the 
> wrapper.

This is in the patch now.

STDIN is still redirected into "patch", even if -i|--input are used. I
do not see a problem there though.

Created attachment 53825 [details]
Proposed patch to quilt

Fixed now. Thanks, Olaf.