[PATCH 0/4] Fixes for (M)erge mode and --sudo
The first two patches are just mailmap changes for incorrect authorship. The third patch is some documentation update. The final patch are some fixes to a bad interaction between (M)erge mode and --sudo. Denton Liu (4): .mailmap: Map Daniel Parks .mailmap: Map Denton Liu pacdiff: update --sudo usages pacdiff: Don't use $SUDO on temporary files .mailmap | 2 ++ doc/pacdiff.8.txt | 3 +++ src/pacdiff.sh.in | 18 +++++++----------- 3 files changed, 12 insertions(+), 11 deletions(-) -- 2.31.0.208.g409f899ff0
In 19ab4fa (pacdiff: Add option to use sudo/sudoedit to manage files, 2021-03-27), the patch was erroneously applied with incorrect authorship information. Fix this with a new .mailmap entry. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- .mailmap | 1 + 1 file changed, 1 insertion(+) diff --git a/.mailmap b/.mailmap index c8185d5..325cbf9 100644 --- a/.mailmap +++ b/.mailmap @@ -1,2 +1,3 @@ <polyzen@archlinux.org> <polycitizen@gmail.com> <polyzen@archlinux.org> <polyzen@archlinux.info> +Daniel Parks <danielrparks@gmail.com> Daniel Parks via pacman-contrib <pacman-contrib@lists.archlinux.org> -- 2.31.0.208.g409f899ff0
In 94b2a19 (pacdiff: Learn the (M)erge mode, 2021-03-04), 348a296 (pacdiff: Reduce repetition in input loop, 2021-03-04), and b675c92 (pacdiff: Implement die(), 2021-03-04), the patches were erroneously applied with incorrect authorship information. Fix this with a new .mailmap entry. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- .mailmap | 1 + 1 file changed, 1 insertion(+) diff --git a/.mailmap b/.mailmap index 325cbf9..60ed742 100644 --- a/.mailmap +++ b/.mailmap @@ -1,3 +1,4 @@ <polyzen@archlinux.org> <polycitizen@gmail.com> <polyzen@archlinux.org> <polyzen@archlinux.info> Daniel Parks <danielrparks@gmail.com> Daniel Parks via pacman-contrib <pacman-contrib@lists.archlinux.org> +Denton Liu <liu.denton@gmail.com> Denton Liu via pacman-contrib <pacman-contrib@lists.archlinux.org> -- 2.31.0.208.g409f899ff0
In 19ab4fa (pacdiff: Add option to use sudo/sudoedit to manage files, 2021-03-27), the --sudo option was introduced. However, a corresponding entry was not included in the man page. Document it. Also, align the usage text in the program with its friends above. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- doc/pacdiff.8.txt | 3 +++ src/pacdiff.sh.in | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/doc/pacdiff.8.txt b/doc/pacdiff.8.txt index 592da72..12f786b 100644 --- a/doc/pacdiff.8.txt +++ b/doc/pacdiff.8.txt @@ -53,6 +53,9 @@ Options *-c, \--cachedir <dir>*:: Scan 'dir' instead as the pacman cache for 3-way merge base candidates. +*-s, \--sudo*:: + Use sudo to merge/remove files. + See Also -------- linkman:pacman[8], linkman:pacman.conf[5] diff --git a/src/pacdiff.sh.in b/src/pacdiff.sh.in index 674892b..fe66a6e 100644 --- a/src/pacdiff.sh.in +++ b/src/pacdiff.sh.in @@ -60,7 +60,7 @@ General Options: --nocolor remove colors from output -c/--cachedir <dir> scan "dir" for 3-way merge base candidates. (default: read from @sysconfdir@/pacman.conf) - -s/--sudo use sudo to merge/remove files + -s/--sudo use sudo to merge/remove files Environment Variables: DIFFPROG override the merge program: (default: 'vim -d') -- 2.31.0.208.g409f899ff0
From: Denton Liu via pacman-contrib <pacman-contrib@lists.archlinux.org> In 19ab4fa (pacdiff: Add option to use sudo/sudoedit to manage files, 2021-03-27), pacdiff was taught to accept -s to run various commands with $SUDO. This introduced many instances of $SUDO in merge_file() where most of them are unnecessary. In particular, it is not necessary to $SUDO to write the temporary files as /tmp should be writable by all. Also, remove the usage of sudoedit when comparing the original file with the merge result. This is because the merged file is placed in a writable directory. Attempting to run sudoedit on this file results in the following error: sudoedit: <file>: editing files in a writable directory is not permitted but root permissions are not really required since users should not write to the original file anyway. The merged file will be used to overwrite the original file at the end of the function anyway. Remove these unnecessary usages of $SUDO. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- src/pacdiff.sh.in | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/pacdiff.sh.in b/src/pacdiff.sh.in index fe66a6e..3c34fdb 100644 --- a/src/pacdiff.sh.in +++ b/src/pacdiff.sh.in @@ -117,20 +117,16 @@ merge_file() { fi basename="$(basename "$file")" - tempdir="$($SUDO mktemp -d --tmpdir "pacdiff-merge-$basename.XXX")" - base="$($SUDO mktemp "$tempdir"/"$basename.base.XXX")" - merged="$($SUDO mktemp "$tempdir"/"$basename.merged.XXX")" + tempdir="$(mktemp -d --tmpdir "pacdiff-merge-$basename.XXX")" + base="$(mktemp "$tempdir"/"$basename.base.XXX")" + merged="$(mktemp "$tempdir"/"$basename.merged.XXX")" - tar -xOf "$base_tar" "${file#/}" | $SUDO tee "$base" > /dev/null - if $SUDO $mergeprog "$file" "$base" "$pacfile" | $SUDO tee "$merged" > /dev/null; then + tar -xOf "$base_tar" "${file#/}" >"$base" + if $mergeprog "$file" "$base" "$pacfile" >"$merged"; then msg2 "Merged without conflicts." fi - if [[ -n "$SUDO" ]]; then - SUDO_EDITOR="$diffprog" sudoedit "$file" "$merged" - else - $diffprog "$file" "$merged" - fi + $diffprog "$file" "$merged" while :; do ask "Would you like to use the results of the merge? [y/n] " -- 2.31.0.208.g409f899ff0
On 3/29/21 5:02 AM, Denton Liu via pacman-contrib wrote:
From: Denton Liu via pacman-contrib <pacman-contrib@lists.archlinux.org>
In 19ab4fa (pacdiff: Add option to use sudo/sudoedit to manage files, 2021-03-27), pacdiff was taught to accept -s to run various commands with $SUDO. This introduced many instances of $SUDO in merge_file() where most of them are unnecessary.
In particular, it is not necessary to $SUDO to write the temporary files as /tmp should be writable by all.
Do you have docs for this? My systems have dirs created by systemd that are owned by root with 700 permissions.
Also, remove the usage of sudoedit when comparing the original file with the merge result. This is because the merged file is placed in a writable directory. Attempting to run sudoedit on this file results in the following error:
sudoedit: <file>: editing files in a writable directory is not permitted
Looks like you ran into:
Files located in a directory that is writable by the invoking user may not be edited unless that user is root (version 1.8.16 and higher).
$tempdir is owned by root if you use --sudo. Seems something went awry during your testing?
but root permissions are not really required since users should not write to the original file anyway. The merged file will be used to overwrite the original file at the end of the function anyway.
Will have to think about this more. Using the merged file is optional, users may want to edit the original file here for one reason or another. Might be good to keep sudoedit here even if just for consistency.
Remove these unnecessary usages of $SUDO.
Signed-off-by: Denton Liu <liu.denton@gmail.com> --- src/pacdiff.sh.in | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/src/pacdiff.sh.in b/src/pacdiff.sh.in index fe66a6e..3c34fdb 100644 --- a/src/pacdiff.sh.in +++ b/src/pacdiff.sh.in @@ -117,20 +117,16 @@ merge_file() { fi
basename="$(basename "$file")" - tempdir="$($SUDO mktemp -d --tmpdir "pacdiff-merge-$basename.XXX")" - base="$($SUDO mktemp "$tempdir"/"$basename.base.XXX")" - merged="$($SUDO mktemp "$tempdir"/"$basename.merged.XXX")" + tempdir="$(mktemp -d --tmpdir "pacdiff-merge-$basename.XXX")" + base="$(mktemp "$tempdir"/"$basename.base.XXX")" + merged="$(mktemp "$tempdir"/"$basename.merged.XXX")"
- tar -xOf "$base_tar" "${file#/}" | $SUDO tee "$base" > /dev/null - if $SUDO $mergeprog "$file" "$base" "$pacfile" | $SUDO tee "$merged" > /dev/null; then + tar -xOf "$base_tar" "${file#/}" >"$base" + if $mergeprog "$file" "$base" "$pacfile" >"$merged"; then msg2 "Merged without conflicts." fi
- if [[ -n "$SUDO" ]]; then - SUDO_EDITOR="$diffprog" sudoedit "$file" "$merged" - else - $diffprog "$file" "$merged" - fi + $diffprog "$file" "$merged"
while :; do ask "Would you like to use the results of the merge? [y/n] "
-- Best, Daniel <https://danielcapella.com>
Hi Daniel, On Tue, Mar 30, 2021 at 02:09:12AM -0400, Daniel M. Capella wrote:
On 3/29/21 5:02 AM, Denton Liu via pacman-contrib wrote:
From: Denton Liu via pacman-contrib <pacman-contrib@lists.archlinux.org>
In 19ab4fa (pacdiff: Add option to use sudo/sudoedit to manage files, 2021-03-27), pacdiff was taught to accept -s to run various commands with $SUDO. This introduced many instances of $SUDO in merge_file() where most of them are unnecessary.
In particular, it is not necessary to $SUDO to write the temporary files as /tmp should be writable by all.
Do you have docs for this? My systems have dirs created by systemd that are owned by root with 700 permissions.
I'm talking about /tmp, not /tmp/*. /tmp should be 777[0][1] which means that we should be able to create a new temporary directory inside it without being root.
Also, remove the usage of sudoedit when comparing the original file with the merge result. This is because the merged file is placed in a writable directory. Attempting to run sudoedit on this file results in the following error:
sudoedit: <file>: editing files in a writable directory is not permitted
Looks like you ran into:
Files located in a directory that is writable by the invoking user may not be edited unless that user is root (version 1.8.16 and higher).
$tempdir is owned by root if you use --sudo. Seems something went awry during your testing?
Ah yes, this was my mistake. This came up as part of my testing the removal of the unnecessary $SUDOs from the mktemp invocations. I'll remove this paragraph in the reroll.
but root permissions are not really required since users should not write to the original file anyway. The merged file will be used to overwrite the original file at the end of the function anyway.
Will have to think about this more. Using the merged file is optional, users may want to edit the original file here for one reason or another. Might be good to keep sudoedit here even if just for consistency.
Currently, users _shouldn't_ be touching the original file at all. It'll be overwritten at the end of the function by if ! $SUDO cp -v "$merged" "$file"; then warning "Unable to write merged file to %s. Merged file is preserved at %s" "$file" "$merged" return 1 fi anyway. As a result, I don't think that being root should be necessary at all until the very end only when we're copying the files over. (Although another improvement that might need to be made is making it more obvious that users should only be editing the merged file and that they should completely ignore the original file.) Thanks for your review, Denton [0]: https://unix.stackexchange.com/a/71625 [1]: https://serverfault.com/a/427290
In 19ab4fa (pacdiff: Add option to use sudo/sudoedit to manage files, 2021-03-27), pacdiff was taught to accept -s to run various commands with $SUDO. This introduced many instances of $SUDO in merge_file() where most of them are unnecessary. In particular, it is not necessary to $SUDO to write the temporary files as /tmp should be writable by all[0][1]. Also, the usage of sudoedit when comparing the original file with the merge result is unnecessary. This is because root permissions are not really required since users should not write to the original file anyway. The merged file will be used to overwrite the original file at the end of the function anyway. Remove these unnecessary usages of $SUDO. [0]: https://unix.stackexchange.com/a/71625 [1]: https://serverfault.com/a/427290 Signed-off-by: Denton Liu <liu.denton@gmail.com> --- Range-diff against v1: 1: ab12657 < -: ------- .mailmap: Map Daniel Parks 2: 9287d4e < -: ------- .mailmap: Map Denton Liu 3: 636e0fb < -: ------- pacdiff: update --sudo usages 4: 32bb4a1 ! 1: ad45bdd pacdiff: Don't use $SUDO on temporary files @@ Commit message where most of them are unnecessary. In particular, it is not necessary to $SUDO to write the temporary files - as /tmp should be writable by all. + as /tmp should be writable by all[0][1]. - Also, remove the usage of sudoedit when comparing the original file with - the merge result. This is because the merged file is placed in a - writable directory. Attempting to run sudoedit on this file results in - the following error: - - sudoedit: <file>: editing files in a writable directory is not permitted - - but root permissions are not really required since users should not - write to the original file anyway. The merged file will be used to - overwrite the original file at the end of the function anyway. + Also, the usage of sudoedit when comparing the original file with the + merge result is unnecessary. This is because root permissions are not + really required since users should not write to the original file + anyway. The merged file will be used to overwrite the original file at + the end of the function anyway. Remove these unnecessary usages of $SUDO. + [0]: https://unix.stackexchange.com/a/71625 + [1]: https://serverfault.com/a/427290 + ## src/pacdiff.sh.in ## @@ src/pacdiff.sh.in: merge_file() { fi @@ src/pacdiff.sh.in: merge_file() { + merged="$(mktemp "$tempdir"/"$basename.merged.XXX")" - tar -xOf "$base_tar" "${file#/}" | $SUDO tee "$base" > /dev/null -- if $SUDO $mergeprog "$file" "$base" "$pacfile" | $SUDO tee "$merged" > /dev/null; then +- $SUDO $mergeprog "$file" "$base" "$pacfile" | $SUDO tee "$merged" > /dev/null +- if [ "${PIPESTATUS[0]}" -ne "1" ]; then + tar -xOf "$base_tar" "${file#/}" >"$base" + if $mergeprog "$file" "$base" "$pacfile" >"$merged"; then msg2 "Merged without conflicts." src/pacdiff.sh.in | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/pacdiff.sh.in b/src/pacdiff.sh.in index 5af7e1c..3c34fdb 100644 --- a/src/pacdiff.sh.in +++ b/src/pacdiff.sh.in @@ -117,21 +117,16 @@ merge_file() { fi basename="$(basename "$file")" - tempdir="$($SUDO mktemp -d --tmpdir "pacdiff-merge-$basename.XXX")" - base="$($SUDO mktemp "$tempdir"/"$basename.base.XXX")" - merged="$($SUDO mktemp "$tempdir"/"$basename.merged.XXX")" + tempdir="$(mktemp -d --tmpdir "pacdiff-merge-$basename.XXX")" + base="$(mktemp "$tempdir"/"$basename.base.XXX")" + merged="$(mktemp "$tempdir"/"$basename.merged.XXX")" - tar -xOf "$base_tar" "${file#/}" | $SUDO tee "$base" > /dev/null - $SUDO $mergeprog "$file" "$base" "$pacfile" | $SUDO tee "$merged" > /dev/null - if [ "${PIPESTATUS[0]}" -ne "1" ]; then + tar -xOf "$base_tar" "${file#/}" >"$base" + if $mergeprog "$file" "$base" "$pacfile" >"$merged"; then msg2 "Merged without conflicts." fi - if [[ -n "$SUDO" ]]; then - SUDO_EDITOR="$diffprog" sudoedit "$file" "$merged" - else - $diffprog "$file" "$merged" - fi + $diffprog "$file" "$merged" while :; do ask "Would you like to use the results of the merge? [y/n] " -- 2.31.0.208.g409f899ff0
On 5/23/21 7:18 AM, Denton Liu wrote:
In 19ab4fa (pacdiff: Add option to use sudo/sudoedit to manage files, 2021-03-27), pacdiff was taught to accept -s to run various commands with $SUDO. This introduced many instances of $SUDO in merge_file() where most of them are unnecessary.
In particular, it is not necessary to $SUDO to write the temporary files as /tmp should be writable by all[0][1].
Also, the usage of sudoedit when comparing the original file with the merge result is unnecessary. This is because root permissions are not really required since users should not write to the original file anyway. The merged file will be used to overwrite the original file at the end of the function anyway.
Remove these unnecessary usages of $SUDO.
[0]: https://unix.stackexchange.com/a/71625 [1]: https://serverfault.com/a/427290
Signed-off-by: Denton Liu <liu.denton@gmail.com> --- Range-diff against v1: 1: ab12657 < -: ------- .mailmap: Map Daniel Parks 2: 9287d4e < -: ------- .mailmap: Map Denton Liu 3: 636e0fb < -: ------- pacdiff: update --sudo usages 4: 32bb4a1 ! 1: ad45bdd pacdiff: Don't use $SUDO on temporary files @@ Commit message where most of them are unnecessary.
In particular, it is not necessary to $SUDO to write the temporary files - as /tmp should be writable by all. + as /tmp should be writable by all[0][1].
- Also, remove the usage of sudoedit when comparing the original file with - the merge result. This is because the merged file is placed in a - writable directory. Attempting to run sudoedit on this file results in - the following error: - - sudoedit: <file>: editing files in a writable directory is not permitted - - but root permissions are not really required since users should not - write to the original file anyway. The merged file will be used to - overwrite the original file at the end of the function anyway. + Also, the usage of sudoedit when comparing the original file with the + merge result is unnecessary. This is because root permissions are not + really required since users should not write to the original file + anyway. The merged file will be used to overwrite the original file at + the end of the function anyway.
Remove these unnecessary usages of $SUDO.
+ [0]: https://unix.stackexchange.com/a/71625 + [1]: https://serverfault.com/a/427290 + ## src/pacdiff.sh.in ## @@ src/pacdiff.sh.in: merge_file() { fi @@ src/pacdiff.sh.in: merge_file() { + merged="$(mktemp "$tempdir"/"$basename.merged.XXX")"
- tar -xOf "$base_tar" "${file#/}" | $SUDO tee "$base" > /dev/null -- if $SUDO $mergeprog "$file" "$base" "$pacfile" | $SUDO tee "$merged" > /dev/null; then +- $SUDO $mergeprog "$file" "$base" "$pacfile" | $SUDO tee "$merged" > /dev/null +- if [ "${PIPESTATUS[0]}" -ne "1" ]; then + tar -xOf "$base_tar" "${file#/}" >"$base" + if $mergeprog "$file" "$base" "$pacfile" >"$merged"; then msg2 "Merged without conflicts."
src/pacdiff.sh.in | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/src/pacdiff.sh.in b/src/pacdiff.sh.in index 5af7e1c..3c34fdb 100644 --- a/src/pacdiff.sh.in +++ b/src/pacdiff.sh.in @@ -117,21 +117,16 @@ merge_file() { fi
basename="$(basename "$file")" - tempdir="$($SUDO mktemp -d --tmpdir "pacdiff-merge-$basename.XXX")" - base="$($SUDO mktemp "$tempdir"/"$basename.base.XXX")" - merged="$($SUDO mktemp "$tempdir"/"$basename.merged.XXX")" + tempdir="$(mktemp -d --tmpdir "pacdiff-merge-$basename.XXX")" + base="$(mktemp "$tempdir"/"$basename.base.XXX")" + merged="$(mktemp "$tempdir"/"$basename.merged.XXX")"
- tar -xOf "$base_tar" "${file#/}" | $SUDO tee "$base" > /dev/null - $SUDO $mergeprog "$file" "$base" "$pacfile" | $SUDO tee "$merged" > /dev/null - if [ "${PIPESTATUS[0]}" -ne "1" ]; then + tar -xOf "$base_tar" "${file#/}" >"$base" + if $mergeprog "$file" "$base" "$pacfile" >"$merged"; then msg2 "Merged without conflicts." fi
- if [[ -n "$SUDO" ]]; then - SUDO_EDITOR="$diffprog" sudoedit "$file" "$merged" - else - $diffprog "$file" "$merged" - fi + $diffprog "$file" "$merged"
while :; do ask "Would you like to use the results of the merge? [y/n] "
Pushed, thank you! -- Best, Daniel <https://danielcapella.com>
participants (2)
-
Daniel M. Capella
-
Denton Liu