[pacman-dev,v2,2/2] makepkg: clarify error when user passes -F

Message ID 20170915183052.2848-2-ivy.foster@gmail.com
State Superseded, archived
Headers show
Series [pacman-dev,v2,1/2] makepkg: implement error codes | expand

Commit Message

Ivy Foster Sept. 15, 2017, 6:30 p.m. UTC
From: Ivy Foster <ivy.foster@gmail.com>

---
 scripts/makepkg.sh.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Geert Hendrickx via pacman-dev Sept. 17, 2017, 9:34 a.m. UTC | #1
On 15.09.2017 20:30, ivy.foster@gmail.com wrote:
> -		error "$(gettext "Do not use the %s option. This option is only for use by %s.")" "'-F'" "makepkg"
> +		error "$(gettext "Do not use the %s option. This option is only for internal use by %s.")" "'-F'" "makepkg"

This seems like a very straight forward change. It's generally better if
you commit less invasive/more likely to be merged changes first so that
they can be merged more easily. Right now this patch should raise a
merge conflict if merged without the first one because of the changed
exit line. You also won't have to carry it along each time you make a
change to the more complex patch.

In case you've never done this before: Committing like this can be done
rather easily if you have all the changes in your working directory and
then use `git add -pi`. When it asks you to stage the hunk, you can edit
the diff and take out the exit code change and then stage only the error
message. Then commit and make a second commit for the rest.

Obviously you could also just commit it afterwards, perform an
interactive rebase to reorder the history and then fix the conflicts,
but I generally follow the above approach when I have small fixes that
come up during creation of a larger patch. It also ensures that `git
diff` is usable during development and I don't have to track the
possibility of performing that change afterwards so I can't forget it.
Also what's done is done.

Florian
Ivy Foster Sept. 17, 2017, 12:36 p.m. UTC | #2
Florian Pritz via pacman-dev <pacman-dev@archlinux.org> wrote:
> On 15.09.2017 20:30, ivy.foster@gmail.com wrote:
> > -           error "$(gettext "Do not use the %s option. \
> > This option is only for use by %s.")" "'-F'" "makepkg"
> > +           error "$(gettext "Do not use the %s option. \
> > This option is only for internal use by %s.")" "'-F'" "makepkg"
> 
> This seems like a very straight forward change. It's generally better if
> you commit less invasive/more likely to be merged changes first so that
> they can be merged more easily.

That makes a lot of sense.

> In case you've never done this before: [snip]

That's very handy! Thanks, Florian.

iff
Ivy Foster Sept. 22, 2017, 5:27 p.m. UTC | #3
Florian Pritz via pacman-dev <pacman-dev@archlinux.org> wrote:
> This seems like a very straight forward change. It's generally better if
> you commit less invasive/more likely to be merged changes first so that
> they can be merged more easily.

I've just sent a version of this patch that does not depend on the
makepkg errors patch. Revised makepkg errors patch to follow.

iff

Patch

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index a8a8eb41..953c43fb 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -1442,7 +1442,7 @@  catastrophic damage to your system.")" "makepkg"
 	fi
 else
 	if [[ -z $FAKEROOTKEY ]]; then
-		error "$(gettext "Do not use the %s option. This option is only for use by %s.")" "'-F'" "makepkg"
+		error "$(gettext "Do not use the %s option. This option is only for internal use by %s.")" "'-F'" "makepkg"
 		exit $E_INVALID_OPTION
 	fi
 fi