[pacman-dev] [PATCH] remove isatty check before reading from stdin
The old behavior is undocumented and we already require the user to explicitly request reading from stdin so we should oblige them whether stdin is a tty or not. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- src/pacman/pacman.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index e86b5c7..d9de556 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -809,7 +809,7 @@ int main(int argc, char *argv[]) } /* we support reading targets from stdin if a cmdline parameter is '-' */ - if(!isatty(fileno(stdin)) && alpm_list_find_str(pm_targets, "-")) { + if(alpm_list_find_str(pm_targets, "-")) { size_t current_size = PATH_MAX; char *vdata, *line = malloc(current_size); -- 1.8.0
On Sat, Nov 24, 2012 at 01:15:17PM -0500, Andrew Gregory wrote:
The old behavior is undocumented and we already require the user to explicitly request reading from stdin so we should oblige them whether stdin is a tty or not.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- src/pacman/pacman.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index e86b5c7..d9de556 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -809,7 +809,7 @@ int main(int argc, char *argv[]) }
/* we support reading targets from stdin if a cmdline parameter is '-' */ - if(!isatty(fileno(stdin)) && alpm_list_find_str(pm_targets, "-")) { + if(alpm_list_find_str(pm_targets, "-")) {
Just for fun, this would make a package by the name of '-' (which is a valid name) only accessible via something like: pacman -Si - <<<- Alternatively, I think it'd be a little weird to see pacman just "hang" if you had a random '-' as an argument and pacman just hung. Is there anything this patch actually fixes?
size_t current_size = PATH_MAX; char *vdata, *line = malloc(current_size);
-- 1.8.0
On Sat, Nov 24, 2012 at 1:29 PM, Dave Reisner <d@falconindy.com> wrote:
On Sat, Nov 24, 2012 at 01:15:17PM -0500, Andrew Gregory wrote:
The old behavior is undocumented and we already require the user to explicitly request reading from stdin so we should oblige them whether stdin is a tty or not.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- src/pacman/pacman.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index e86b5c7..d9de556 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -809,7 +809,7 @@ int main(int argc, char *argv[]) }
/* we support reading targets from stdin if a cmdline parameter is '-' */ - if(!isatty(fileno(stdin)) && alpm_list_find_str(pm_targets, "-")) { + if(alpm_list_find_str(pm_targets, "-")) {
Just for fun, this would make a package by the name of '-' (which is a valid name) only accessible via something like:
pacman -Si - <<<-
Alternatively, I think it'd be a little weird to see pacman just "hang" if you had a random '-' as an argument and pacman just hung.
Is there anything this patch actually fixes?
I also worry about how easy it is to use '-' instead of '--' if you are trying to prevent interpretation as an arg; e.g. in a command like `pacman -Ss -- -pytz`. The user would be wondering why nothing seems to be happening. -Dan
On Sat, 24 Nov 2012 13:37:25 -0500 Dan McGee <dpmcgee@gmail.com> wrote:
On Sat, Nov 24, 2012 at 1:29 PM, Dave Reisner <d@falconindy.com> wrote:
On Sat, Nov 24, 2012 at 01:15:17PM -0500, Andrew Gregory wrote:
The old behavior is undocumented and we already require the user to explicitly request reading from stdin so we should oblige them whether stdin is a tty or not.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- src/pacman/pacman.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index e86b5c7..d9de556 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -809,7 +809,7 @@ int main(int argc, char *argv[]) }
/* we support reading targets from stdin if a cmdline parameter is '-' */ - if(!isatty(fileno(stdin)) && alpm_list_find_str(pm_targets, "-")) { + if(alpm_list_find_str(pm_targets, "-")) {
Just for fun, this would make a package by the name of '-' (which is a valid name) only accessible via something like:
pacman -Si - <<<-
Alternatively, I think it'd be a little weird to see pacman just "hang" if you had a random '-' as an argument and pacman just hung.
Is there anything this patch actually fixes?
I also worry about how easy it is to use '-' instead of '--' if you are trying to prevent interpretation as an arg; e.g. in a command like `pacman -Ss -- -pytz`. The user would be wondering why nothing seems to be happening.
-Dan
If the primary concern is user confusion, we could print a message to stderr to the effect that we're reading from stdin. Really though, I'm less bothered by the fact that pacman treats '-' very differently based on context than I am that the behavior isn't documented. I would also be happy just tweaking the man page; perhaps something like "Additionally, if stdin is not connected to a terminal and a single dash (-) is passed as an argument..."
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- I think I'm alone in wanting to remove the isatty check, but we can at least document it. doc/pacman.8.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/pacman.8.txt b/doc/pacman.8.txt index de28b9c..358d506 100644 --- a/doc/pacman.8.txt +++ b/doc/pacman.8.txt @@ -27,8 +27,8 @@ front ends to be written (for instance, a GUI front end). Invoking pacman involves specifying an operation with any potential options and targets to operate on. A 'target' is usually a package name, filename, URL, or a search string. Targets can be provided as command line arguments. -Additionally, if a single dash (-) is passed as an argument, targets will be -read from stdin. +Additionally, if stdin is not from a terminal and a single dash (-) is passed +as an argument, targets will be read from stdin. Operations -- 1.8.1.2
On 30/01/13 12:38, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> ---
I think I'm alone in wanting to remove the isatty check, but we can at least document it.
For the record, this is the old discussion, which I am not entirely convinced came to a conclusion. Is there some standard - not necessarily a real standard but perhaps in the GNU coding conventions - that we can use to justify either behaviour. [1] https://patchwork.archlinux.org/patch/720/
doc/pacman.8.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/doc/pacman.8.txt b/doc/pacman.8.txt index de28b9c..358d506 100644 --- a/doc/pacman.8.txt +++ b/doc/pacman.8.txt @@ -27,8 +27,8 @@ front ends to be written (for instance, a GUI front end). Invoking pacman involves specifying an operation with any potential options and targets to operate on. A 'target' is usually a package name, filename, URL, or a search string. Targets can be provided as command line arguments. -Additionally, if a single dash (-) is passed as an argument, targets will be -read from stdin. +Additionally, if stdin is not from a terminal and a single dash (-) is passed +as an argument, targets will be read from stdin.
Operations
On 30/01/13 12:56, Allan McRae wrote:
On 30/01/13 12:38, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> ---
I think I'm alone in wanting to remove the isatty check, but we can at least document it.
For the record, this is the old discussion, which I am not entirely convinced came to a conclusion. Is there some standard - not necessarily a real standard but perhaps in the GNU coding conventions - that we can use to justify either behaviour.
OK... I have done some reading and here is my decision! Keep the !isatty check. It makes little sense to read packages from a terminal to me. But there is two things that need done: 1) Documentation as below. 2) Fix the error message:
pacman -S - error: target not found: -
"-" is not a valid pkgname so we can do better in this situation. How about something like: error: "-" specified - stdin can not be read from a terminal Better suggestions welcome!
doc/pacman.8.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/doc/pacman.8.txt b/doc/pacman.8.txt index de28b9c..358d506 100644 --- a/doc/pacman.8.txt +++ b/doc/pacman.8.txt @@ -27,8 +27,8 @@ front ends to be written (for instance, a GUI front end). Invoking pacman involves specifying an operation with any potential options and targets to operate on. A 'target' is usually a package name, filename, URL, or a search string. Targets can be provided as command line arguments. -Additionally, if a single dash (-) is passed as an argument, targets will be -read from stdin. +Additionally, if stdin is not from a terminal and a single dash (-) is passed +as an argument, targets will be read from stdin.
Operations
On 25/11/12 04:29, Dave Reisner wrote:
On Sat, Nov 24, 2012 at 01:15:17PM -0500, Andrew Gregory wrote:
The old behavior is undocumented and we already require the user to explicitly request reading from stdin so we should oblige them whether stdin is a tty or not.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- src/pacman/pacman.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index e86b5c7..d9de556 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -809,7 +809,7 @@ int main(int argc, char *argv[]) }
/* we support reading targets from stdin if a cmdline parameter is '-' */ - if(!isatty(fileno(stdin)) && alpm_list_find_str(pm_targets, "-")) { + if(alpm_list_find_str(pm_targets, "-")) {
Just for fun, this would make a package by the name of '-' (which is a valid name) only accessible via something like:
Not that valid.... error "$(gettext "%s is not allowed to start with a hyphen.")" "pkgname"
pacman -Si - <<<-
Alternatively, I think it'd be a little weird to see pacman just "hang" if you had a random '-' as an argument and pacman just hung.
Is there anything this patch actually fixes?
size_t current_size = PATH_MAX; char *vdata, *line = malloc(current_size);
-- 1.8.0
participants (4)
-
Allan McRae
-
Andrew Gregory
-
Dan McGee
-
Dave Reisner