bugfindutils - Bugs: bug #52137, unexpected behaviour when...

 
 

bug #52137: unexpected behaviour when combining -I and -n

Submitter:  Ulrich Sibiller <uli42>
Submitted:  Thu 28 Sep 2017 10:37:17 AM UTC
   
 
Category:  xargs Severity:  3 - Normal
Item Group:  Wrong result Status:  Fixed
Privacy:  Public Assigned to:  berny
Originator Name:  Open/Closed:  Closed
Release:  4.5.11 Fixed Release:  4.8.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 07 Jan 2021 05:35:04 PM UTC, comment #13: 

I got green light to push from Andreas in an off-list email.

I added another test case to verify that the special case '--replace=REPL -n1' doesn't emit the warning.

Pushed here:
https://git.sv.gnu.org/cgit/findutils.git/commit/?id=ee402bdd

Bernhard Voelker <berny>
Group administrator
Thu 07 Jan 2021 12:31:26 AM UTC, comment #12: 

Finally I managed to put it into a nice patch incl. tests+doc.

@Andreas Metzler: okay to push in your name like that?

(file #50672)

Bernhard Voelker <berny>
Group administrator
Sun 01 Sep 2019 02:26:54 PM UTC, comment #11: 

Bernhard, I notice that the shell-based test infrastructure is in place.   Are these patches ready to be applied?   If you're no longer working on this please change the "Assigned to" and "Status" fields back to "None".  Thanks.

James Youngman <jay>
Group administrator
Thu 16 Nov 2017 09:42:01 AM UTC, comment #10: 

James wrote in comment #8:

> In issue 6, -L and -n are specified to interact such that the last-specified takes effect. The -I option is specified, but no interaction with -L or -n is called out.
>
> In issue 7, we have the previously quoted phrase:
>
> "The -I, -L, and -n options are mutually-exclusive. Some implementations use the last one specified if more than one is given on a command line; other implementations treat combinations of the options in different ways."
>
> However, this quote is taken from the Rationale section which is non-normative (per the comment earlier in the document, "The following sections are informative.")
>
> Since the text stating that these options are mutually exclusive is non-normative, and that the text doesn't seem to indicate that the user should expect anything other than a perhaps-unexpected combination of behaviours, I'm left not totally sure that the xargs implementation is allowed to issue a diagnostic.
>
> Geoff, do you have an opinion on whether a diagnostic is permissible?


You noticed the descriptive text changes but you missed the all-important SYNOPSIS change.

Issue 6: [-I replstr][-L number][-n number

Issue 7: [-I replstr|-L number|-n number]

The vertical bars indicate that the options are mutually exclusive.

So yes you can write a diagnostic.

> If we do issue a diagnostic, but all invocations of the named utility return 0, is it still conforming for xargs also to return 0?


The general rule is that if you write a diagnostic, the exit status has to be non-zero.  However, in this particular case there is a get-out courtesy of XBD 12.1 item 8:

"The use of conflicting mutually-exclusive arguments produces undefined results, unless a utility description specifies otherwise."

Since it says the result is "undefined", you can do anything you want.

Geoff Clare <geoffclare>
Wed 15 Nov 2017 08:08:12 PM UTC, comment #9: 

(For those following by email only, the Savannah bug tracker entry for this bug is https://savannah.gnu.org/bugs/index.php?52137)

James Youngman <jay>
Group administrator
Wed 15 Nov 2017 08:05:35 PM UTC, comment #8: 

The approach (issue a warning without changing the current semantics of sequences of these options) seems good to me.   I'd like to apply this patch basically as-is, with perhaps a couple of tweaks to the wording of the NEWS entry and the warning message; details of those tweaks below.   Also, further below, a concern about POSIX conformance.

The NEWS file mentions the options by long form, while I think that since these options are POSIX-specified, it's very likely that most uses of these options will be via the single-letter form.   So IMO we should provide the short-form options in the NEWS - either instead, or as well.

On the other hand, some users may indeed use long options.   However, when we're processing options and find that "exclusive" options have been given, we (with this patch) issue a message which gives only the short form.   This could be confusing if the user actually specified the long form of the option.

I notice that there have also been changes between the 2004 (Issue 6, http://pubs.opengroup.org/onlinepubs/009604599/utilities/xargs.html) and the 2008
(Issue 7, http://pubs.opengroup.org/onlinepubs/9699919799/utilities/xargs.html) POSIX standards.

In issue 6, -L and -n are specified to interact such that the last-specified takes effect.  The -I option is specified, but no interaction with -L or -n is called out.

In issue 7, we have the previously quoted phrase:

"The -I, -L, and -n options are mutually-exclusive. Some implementations use the last one specified if more than one is given on a command line; other implementations treat combinations of the options in different ways."

However, this quote is taken from the Rationale section which is non-normative (per the comment earlier in the document, "The following sections are informative.")

Since the text stating that these options are mutually exclusive is non-normative, and that the text doesn't seem to indicate that  the user should expect anything other than a perhaps-unexpected combination of behaviours, I'm left not totally sure that the xargs implementation is allowed to issue a diagnostic. 

Geoff, do you have an opinion on whether a diagnostic is permissible?   If we do issue a diagnostic, but all invocations of the named utility return 0, is it still conforming for xargs also to return 0?

James Youngman <jay>
Group administrator
Wed 11 Oct 2017 09:27:29 PM UTC, comment #7: 

oops, good catch - missing "{...}" in the else case.

Regarding the tests: I would try to avoid having so many one-line
test files.  Furthermore, I'm not very good in Tcl, so I propose
to build up more shell based tests ... and have helper functions
similar to those in coreutils.
In the end, I think it's sufficient to have 1 file for the
conflicting_* tests.

I'll try to come up with a little shell-based test framework for
xargs soon.

Bernhard Voelker <berny>
Group administrator
Wed 11 Oct 2017 05:19:23 PM UTC, comment #6: 


> I squashed the commits into one, and changed the following:
> - use error() rather than fprintf(),


I tried that first but it broke the testsuite because error() always outputs argv[0] and argv[0] will include the path when run from testsuite. You have fixed that in config/unix.exp. ;-)

[...]

> - change the test cases to always only have 2 conflicting options,


I am not too happy with that, I can understand adding two-option examples but not replacing the three-case ones.

Your patch introduces an unintended behavior change in respect to "-I REPL -n1":

xargs release:
(sid)ametzler@argenau:/tmp/FIND/findutils-4.6.0+git+20170828/xargs$ echo a b c | xargs -I x -n1  echo x z
a b c z

with patch
(sid)ametzler@argenau:/tmp/FIND/findutils-4.6.0+git+20170828/xargs$ echo a b c | ./xargs -I x -n1  echo x z
x z a b c

cu Andreas

Andreas Metzler <ametzler>
Tue 10 Oct 2017 11:01:05 PM UTC, comment #5: 

Thanks for the patches ... I also had already started something similar.

I squashed the commits into one, and changed the following:
- use error() rather than fprintf(),
- factor out the outputting of the warning diagnostic into
  its own function, and mention the actually conflicting options
  in the message,
- improve the warning message:
   - "..., using the last one specified."
   + "..., ignoring previous -I,-i value"
- add a NEWS entry,
- fix some typos,
- improve the documentation (e.g. added an example),
- change the test cases to always only have 2 conflicting options,
- adjust the commit message to GNU standard.

With your permission, I'd push the attached soon.
WDYT?


(file #42117)

Bernhard Voelker <berny>
Group administrator
Sun 08 Oct 2017 04:21:59 PM UTC, comment #4: 

Hello,

find attached a patch series to
- improve documentation for conflicting options
- make xargs display a warning on conflicting options
- increase coverage for these options.

cu Andreas

(file #42093, file #42094, file #42095)

Andreas Metzler <ametzler>
Sun 01 Oct 2017 04:42:08 PM UTC, comment #3: 

Hello,

agreed regarding addition of warning and doc-update.

> "But I cannot see posix requiring that -I and -n must produce
> strange results, so xargs could go beyond posix and produce
> correct results in that case anyway."


Actually POSIX saying that -I and -n are mutually exclusive is not arbitrary. The specification of -I and -n cannot be fullfilled at same time, except for the degenerate case (one line with a single argument):

-I [...] utility is executed for each logical line from standard input.  Arguments in the standard input shall be separated only by unescaped [newline] characters, not by [blank] characters.
-n [...] Invoke utility using as many standard input arguments as possible, up to number arguments maximum.

Therefore because POSIX requires this behavior ...

ametzler@argenau:/tmp$ echo a b | xargs -n1 echo x
x a
x b
ametzler@argenau:/tmp$ echo a b | xargs -I {}  echo x {}
x a b


... mixing -L and -n must produce strange results (or an error).

Andreas Metzler <ametzler>
Thu 28 Sep 2017 09:01:40 PM UTC, comment #2: 

It's neither documented nor does xargs perfom any checks if mutually-exclusive options are provided. So I think both should be fixed.

But I cannot see posix requiring that -I and -n must produce strange results, so xargs could go beyond posix and produce correct results in that case anyway.


Ulrich Sibiller <uli42>
Thu 28 Sep 2017 05:39:29 PM UTC, comment #1: 

Posix says: "The -I, -L, and -n options are mutually-exclusive. Some implementations use the last one specified if more than one is given on a command line; other implementations treat combinations of the options in different ways."

GNU xargs is using the "last one specified" approach.

However afaict this is not documented.

Andreas Metzler <ametzler>
Thu 28 Sep 2017 10:37:17 AM UTC, original submission:  

I see an unexpected behaviour with xargs on Ubuntu 16.04 (and also with 4.5.11 on Centos 7.4) when using -I and -n together.

This is the version of xargs:
$ xargs --version
xargs (GNU findutils) 4.7.0-git
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by Eric B. Decker, James Youngman, and Kevin Dalley.

I try

$ echo a b c d e f | xargs -I_ -n1 echo _xxx
a b c d e fxxx

-> this is expected, as the manpage states "unquoted blanks do not terminate input items" for -I

But increasing -n leads to this surprising result:

$ echo a b c d e f | xargs -I_ -n2 echo _xxx
_xxx a b
_xxx c d
_xxx e f

 From my understanding -n2 means use 2 input items at max, and xargs is only getting one here (because of -I ignoring the spaces). So the result should be identical to the -n1 case. Also -I does not seem to have an effect here, nothing is replaced.

Playing around shows even more unexpected stuff:

$ echo a b c d e f | xargs -n2 -I_ echo _xxx
a b c d e fxxx

Obviously the position of -n does matter here, but I cannot find the manpage mentioning that.

Now, adding spaces as delimiters it gets even weirder:

$ echo a b c d e f | xargs -n1 -d" " -I_ echo _xxx
axxx
bxxx
cxxx
dxxx
exxx
f
xxx

That one looks good at first sight, but the manpage refers to -d as a single character, so it does not include \n anymore. But the line break that echo appends is treated as a delimiter, too.

Now, increasing -n again does not change the output. But it should:

$ echo a b c d e f | xargs -n2 -d" " -I_ echo _xxx
axxx
bxxx
cxxx
dxxx
exxx
f
xxx

So it looks to me that handling -I and -n in the same command line is broken somehow.

Ulrich Sibiller <uli42>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #50672:  0001-xargs-warn-on-conflicting-options-L-I-n.patch added by berny (17KiB - text/x-patch - [PATCH] xargs: warn on conflicting options -L, -I, -n)
file #42117:  0001-xargs-warn-on-conflicting-options-L-I-n.patch added by berny (22KiB - text/x-patch - [PATCH] xargs: warn on conflicting options -L, -I, -n)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by geoffclare (Posted a comment)
  • -email is unavailable- added by jay (Posted a comment)
  • -email is unavailable- added by jay (Geoff Clare)
  • -email is unavailable- added by berny (Updated the item)
  • -email is unavailable- added by uli42 (Submitted the item)
  • -email is unavailable- added by uli42
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-01-09 berny Open/ClosedOpen Closed
        Fixed ReleaseNone 4.8.0
    2021-01-07 berny StatusReady For Test Fixed
    2021-01-07 berny Attached File- Added 0001-xargs-warn-on-conflicting-options-L-I-n.patch, #50672
        StatusIn Progress Ready For Test
    2019-09-01 jay StatusNone In Progress
        Assigned toNone berny
    2017-11-15 jay Carbon-Copy- Added -email is unavailable-
    2017-10-10 berny Attached File- Added 0001-xargs-warn-on-conflicting-options-L-I-n.patch, #42117
    2017-10-08 ametzler Attached File- Added 0001-Improve-documentation-for-conflicting-xargs-options.patch, #42093
        Attached File- Added 0002-Warn-on-conflicting-options.patch, #42094
        Attached File- Added 0003-xargs-test-conflicting-options-I-L-n.patch, #42095
    2017-09-28 uli42 Carbon-Copy- Added uli42

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code