patch(1): give /dev/null patches special treatment

We have a bad habit of duplicating contents of files that are sourced from
/dev/null and applied more than once... take the more sane (in most ways)
GNU route and complain if the file exists and offer reversal options.

This still falls short a little bit as selecting "don't reverse, apply
anyway" will still give you duplicated file contents. There's probably other
issues as well, but awareness is the first step to happiness.

MFC after:	2 weeks
Differential Revision:	https://reviews.freebsd.org/D21535

usr.bin/patch/patch.1

@@ -21,7 +21,7 @@
 .\"
 .\" $OpenBSD: patch.1,v 1.27 2014/04/15 06:26:54 jmc Exp $
 .\" $FreeBSD$
-.Dd August 15, 2015
+.Dd November 3, 2019
 .Dt PATCH 1
 .Os
 .Sh NAME
@@ -559,8 +559,10 @@ option as needed.
 .Pp
 Third, you can create a file by sending out a diff that compares a
 null file to the file you want to create.
-This will only work if the file you want to create does not exist already in
-the target directory.
+If the file you want to create already exists in the target directory when the
+diff is applied, then
+.Nm
+will identify the patch as potentially reversed and offer to reverse the patch.
 .Pp
 Fourth, take care not to send out reversed patches, since it makes people wonder
 whether they already applied the patch.

usr.bin/patch/patch.c

@@ -31,6 +31,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 
+#include <assert.h>
 #include <ctype.h>
 #include <getopt.h>
 #include <limits.h>
@@ -103,6 +104,7 @@ static void	dump_line(LINENUM, bool);
 static bool	patch_match(LINENUM, LINENUM, LINENUM);
 static bool	similar(const char *, const char *, int);
 static void	usage(void);
+static bool	handle_creation(bool, bool *);
 
 /* true if -E was specified on command line.  */
 static bool	remove_empty_files = false;
@@ -147,8 +149,10 @@ static char		end_defined[128];
 int
 main(int argc, char *argv[])
 {
+	struct stat statbuf;
 	int	error = 0, hunk, failed, i, fd;
-	bool	patch_seen, reverse_seen;
+	bool	out_creating, out_existed, patch_seen, remove_file;
+	bool	reverse_seen;
 	LINENUM	where = 0, newwhere, fuzz, mymaxfuzz;
 	const	char *tmpdir;
 	char	*v;
@@ -219,6 +223,12 @@ main(int argc, char *argv[])
 	    reinitialize_almost_everything()) {
 		/* for each patch in patch file */
 
+		if (source_file != NULL && (diff_type == CONTEXT_DIFF ||
+		    diff_type == NEW_CONTEXT_DIFF ||
+		    diff_type == UNI_DIFF))
+			out_creating = strcmp(source_file, _PATH_DEVNULL) == 0;
+		else
+			out_creating = false;
 		patch_seen = true;
 
 		warn_on_invalid_line = true;
@@ -226,6 +236,19 @@ main(int argc, char *argv[])
 		if (outname == NULL)
 			outname = xstrdup(filearg[0]);
 
+		/*
+		 * At this point, we know if we're supposed to be creating the
+		 * file and we know if we should be trying to handle a conflict
+		 * between the patch and the file already existing.  We defer
+		 * handling it until hunk processing because we want to swap
+		 * the hunk if they opt to reverse it, but we want to make sure
+		 * we *can* swap the hunk without running into memory issues
+		 * before we offer it.  We also want to be verbose if flags or
+		 * user decision cause us to skip -- this is explained a little
+		 * more later.
+		 */
+		out_existed = stat(outname, &statbuf) == 0;
+
 		/* for ed script just up and do it and exit */
 		if (diff_type == ED_DIFF) {
 			do_ed_script();
@@ -252,9 +275,28 @@ main(int argc, char *argv[])
 		failed = 0;
 		reverse_seen = false;
 		out_of_mem = false;
+		remove_file = false;
 		while (another_hunk()) {
+			assert(!out_creating || hunk == 0);
 			hunk++;
 			fuzz = 0;
+
+			/*
+			 * There are only three cases in handle_creation() that
+			 * results in us skipping hunk location, in order:
+			 *
+			 * 1.) Potentially reversed but -f/--force'd,
+			 * 2.) Potentially reversed but -N/--forward'd
+			 * 3.) Reversed and the user's opted to not apply it.
+			 *
+			 * In all three cases, we still want to inform the user
+			 * that we're ignoring it in the standard way, which is
+			 * also tied to this hunk processing loop.
+			 */
+			if (out_creating)
+				reverse_seen = handle_creation(out_existed,
+				    &remove_file);
+
 			mymaxfuzz = pch_context();
 			if (maxfuzz < mymaxfuzz)
 				mymaxfuzz = maxfuzz;
@@ -372,7 +414,6 @@ main(int argc, char *argv[])
 		/* and put the output where desired */
 		ignore_signals();
 		if (!skip_rest_of_patch) {
-			struct stat	statbuf;
 			char	*realout = outname;
 
 			if (!check_only) {
@@ -383,7 +424,18 @@ main(int argc, char *argv[])
 				} else
 					chmod(outname, filemode);
 
-				if (remove_empty_files &&
+				/*
+				 * remove_file is a per-patch flag indicating
+				 * whether it's OK to remove the empty file.
+				 * This is specifically set when we're reversing
+				 * the creation of a file and it ends up empty.
+				 * This is an exception to the global policy
+				 * (remove_empty_files) because the user would
+				 * likely not expect the reverse of file
+				 * creation to leave an empty file laying
+				 * around.
+				 */
+				if ((remove_empty_files || remove_file) &&
 				    stat(realout, &statbuf) == 0 &&
 				    statbuf.st_size == 0) {
 					if (verbose)
@@ -444,6 +496,9 @@ reinitialize_almost_everything(void)
 		filearg[0] = NULL;
 	}
 
+	free(source_file);
+	source_file = NULL;
+
 	free(outname);
 	outname = NULL;
 
@@ -1084,3 +1139,84 @@ similar(const char *a, const char *b, int len)
 	return true;		/* actually, this is not reached */
 	/* since there is always a \n */
 }
+
+static bool
+handle_creation(bool out_existed, bool *remove)
+{
+	bool reverse_seen;
+
+	reverse_seen = false;
+	if (reverse && out_existed) {
+		/*
+		 * If the patch creates the file and we're reversing the patch,
+		 * then we need to indicate to the patch processor that it's OK
+		 * to remove this file.
+		 */
+		*remove = true;
+	} else if (!reverse && out_existed) {
+		/*
+		 * Otherwise, we need to blow the horn because the patch appears
+		 * to be reversed/already applied.  For non-batch jobs, we'll
+		 * prompt to figure out what we should be trying to do to raise
+		 * awareness of the issue.  batch (-t) processing suppresses the
+		 * questions and just assumes that we're reversed if it looks
+		 * like we are, which is always the case if we've reached this
+		 * branch.
+		 */
+		if (force) {
+			skip_rest_of_patch = true;
+			return (false);
+		}
+		if (noreverse) {
+			/* If -N is supplied, however, we bail out/ignore. */
+			say("Ignoring previously applied (or reversed) patch.\n");
+			skip_rest_of_patch = true;
+			return (false);
+		}
+
+		/* Unreversed... suspicious if the file existed. */
+		if (!pch_swap())
+			fatal("lost hunk on alloc error!\n");
+
+		reverse = !reverse;
+
+		if (batch) {
+			if (verbose)
+				say("Patch creates file that already exists, %s %seversed",
+				    reverse ? "Assuming" : "Ignoring",
+				    reverse ? "R" : "Unr");
+		} else {
+			ask("Patch creates file that already exists!  %s -R? [y] ",
+			    reverse ? "Assume" : "Ignore");
+
+			if (*buf == 'n') {
+				ask("Apply anyway? [n]");
+				if (*buf != 'y')
+					/* Don't apply; error out. */
+					skip_rest_of_patch = true;
+				else
+					/* Attempt to apply. */
+					reverse_seen = true;
+				reverse = !reverse;
+				if (!pch_swap())
+					fatal("lost hunk on alloc error!\n");
+			} else {
+				/*
+				 * They've opted to assume -R; effectively the
+				 * same as the first branch in this function,
+				 * but the decision is here rather than in a
+				 * prior patch/hunk as in that branch.
+				 */
+				*remove = true;
+			}
+		}
+	}
+
+	/*
+	 * The return value indicates if we offered a chance to reverse but the
+	 * user declined.  This keeps the main patch processor in the loop since
+	 * we've taken this out of the normal flow of hunk processing to
+	 * simplify logic a little bit.
+	 */
+	return (reverse_seen);
+}

usr.bin/patch/pch.c

@@ -70,6 +70,8 @@ static LINENUM	p_bfake = -1;	/* beg of faked up lines */
 static FILE	*pfp = NULL;	/* patch file pointer */
 static char	*bestguess = NULL;	/* guess at correct filename */
 
+char		*source_file;
+
 static void	grow_hunkmax(void);
 static int	intuit_diff_type(void);
 static void	next_intuit_at(off_t, LINENUM);
@@ -218,7 +220,12 @@ there_is_another_patch(void)
 			bestguess = xstrdup(buf);
 			filearg[0] = fetchname(buf, &exists, 0);
 		}
-		if (!exists) {
+		/*
+		 * fetchname can now return buf = NULL, exists = true, to
+		 * indicate to the caller that /dev/null was specified.  Retain
+		 * previous behavior for now until this can be better evaluted.
+		 */
+		if (filearg[0] == NULL || !exists) {
 			int def_skip = *bestguess == '\0';
 			ask("No file found--skip this patch? [%c] ",
 			    def_skip  ? 'y' : 'n');
@@ -403,6 +410,24 @@ scan_exit:
 		names[OLD_FILE] = names[NEW_FILE];
 		names[NEW_FILE] = tmp;
 	}
+
+	/* Invalidated */
+	free(source_file);
+	source_file = NULL;
+
+	if (retval != 0) {
+		/*
+		 * If we've successfully determined a diff type, stored in
+		 * retval, path == NULL means _PATH_DEVNULL if exists is set.
+		 * Explicitly specify it here to make it easier to detect later
+		 * on that we're actually creating a file and not that we've
+		 * just goofed something up.
+		 */
+		if (names[OLD_FILE].path != NULL)
+			source_file = xstrdup(names[OLD_FILE].path);
+		else if (names[OLD_FILE].exists)
+			source_file = xstrdup(_PATH_DEVNULL);
+	}
 	if (filearg[0] == NULL) {
 		if (posix)
 			filearg[0] = posix_name(names, ok_to_create_file);

usr.bin/patch/pch.h

@@ -37,6 +37,8 @@ struct file_name {
 	bool exists;
 };
 
+extern char	*source_file;
+
 void		re_patch(void);
 void		open_patch_file(const char *);
 void		set_hunkmax(void);

usr.bin/patch/tests/unified_patch_test.sh

@@ -103,27 +103,28 @@ file_creation_body()
 # contents as many times as you apply the diff.  It should instead detect that
 # a source of /dev/null creates the file, so it shouldn't exist.  Furthermore,
 # the reverse of creation is deletion -- hence the next test, which ensures that
-# the file is removed if it's empty once the patch is reversed.
+# the file is removed if it's empty once the patch is reversed.  The size checks
+# are scattered throughout to make sure that we didn't get some kind of false
+# error, and the first size check is merely a sanity check that should be
+# trivially true as this is executed in a sandbox.
 atf_test_case file_nodupe
 file_nodupe_body()
 {
 
-	# WIP
-	atf_expect_fail "patch(1) erroneously duplicates created files"
 	echo "x" > foo
 	diff -u /dev/null foo > foo.diff
 
-	atf_check -x "patch -s < foo.diff"
-	atf_check -s not-exit:0 -x "patch -fs < foo.diff"
+	atf_check -o inline:"2\n" stat -f "%z" foo
+	atf_check -s not-exit:0 -o ignore -x "patch -Ns < foo.diff"
+	atf_check -o inline:"2\n" stat -f "%z" foo
+	atf_check -s not-exit:0 -o ignore -x "patch -fs < foo.diff"
+	atf_check -o inline:"2\n" stat -f "%z" foo
 }
 
 atf_test_case file_removal
 file_removal_body()
 {
 
-	# WIP
-	atf_expect_fail "patch(1) does not yet recognize /dev/null as creation"
-
 	echo "x" > foo
 	diff -u /dev/null foo > foo.diff
 

usr.bin/patch/util.c

@@ -366,8 +366,10 @@ fetchname(const char *at, bool *exists, int strip_leading)
 		say("fetchname %s %d\n", at, strip_leading);
 #endif
 	/* So files can be created by diffing against /dev/null.  */
-	if (strnEQ(at, _PATH_DEVNULL, sizeof(_PATH_DEVNULL) - 1))
+	if (strnEQ(at, _PATH_DEVNULL, sizeof(_PATH_DEVNULL) - 1)) {
+		*exists = true;
 		return NULL;
+	}
 	name = fullname = t = savestr(at);
 
 	tab = strchr(t, '\t') != NULL;