svn commit: r355351 - in stable: 11/etc/mtree 11/usr.bin/patch 11/usr.bin/patch/tests 12/etc/mtree 12/usr.bin/patch 12/usr.bin/patch/tests
Kyle Evans
kevans at FreeBSD.org
Tue Dec 3 18:55:13 UTC 2019
Author: kevans
Date: Tue Dec 3 18:55:09 2019
New Revision: 355351
URL: https://svnweb.freebsd.org/changeset/base/355351
Log:
MFC r351836, r351866, r354328: patch(1) /dev/null testing+improvement
r351836: patch(1): add some basic tests
Summary:
- basic: test application of patches created by diff -u at the
beginning/middle/end of file, which have differing amounts of context
before and after chunks being added
- limited_ctx: stems from PR 74127 in which a rogue line was getting added
when the patch should have been rejected. Similar behavior was
reproducible with larger contexts near the beginning/end of a file. See
r326084 for details
- file_creation: patch sourced from /dev/null should create the file
- file_nodupe: said patch sourced from /dev/null shouldn't dupe the contents
when re-applied (personal vendetta, WIP, see comment)
- file_removal: this follows from nodupe; the reverse of a patch sourced
from /dev/null is most naturally deleting the file, as is expected based
on GNU patch behavior (WIP)
r351866: patch(1): fix the file removal test, strengthen it a bit
To remain compatible with GNU patch, we should ensure that once we're
removing empty files after a reversed /dev/null patch we don't remove files
that have been modified. GNU patch leaves these intact and just reverses the
hunk that created the file, effectively implying --remove-empty-files for
reversed /dev/null patches.
r354328: 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.
Added:
stable/12/usr.bin/patch/tests/
- copied from r351836, head/usr.bin/patch/tests/
Modified:
stable/12/etc/mtree/BSD.tests.dist
stable/12/usr.bin/patch/Makefile
stable/12/usr.bin/patch/patch.1
stable/12/usr.bin/patch/patch.c
stable/12/usr.bin/patch/pch.c
stable/12/usr.bin/patch/pch.h
stable/12/usr.bin/patch/tests/unified_patch_test.sh
stable/12/usr.bin/patch/util.c
Directory Properties:
stable/12/ (props changed)
Changes in other areas also in this revision:
Added:
stable/11/usr.bin/patch/tests/
- copied from r351836, head/usr.bin/patch/tests/
Modified:
stable/11/etc/mtree/BSD.tests.dist
stable/11/usr.bin/patch/Makefile
stable/11/usr.bin/patch/patch.1
stable/11/usr.bin/patch/patch.c
stable/11/usr.bin/patch/pch.c
stable/11/usr.bin/patch/pch.h
stable/11/usr.bin/patch/tests/unified_patch_test.sh
stable/11/usr.bin/patch/util.c
Directory Properties:
stable/11/ (props changed)
Modified: stable/12/etc/mtree/BSD.tests.dist
==============================================================================
--- stable/12/etc/mtree/BSD.tests.dist Tue Dec 3 18:50:18 2019 (r355350)
+++ stable/12/etc/mtree/BSD.tests.dist Tue Dec 3 18:55:09 2019 (r355351)
@@ -1010,6 +1010,8 @@
..
opensm
..
+ patch
+ ..
pr
..
printf
Modified: stable/12/usr.bin/patch/Makefile
==============================================================================
--- stable/12/usr.bin/patch/Makefile Tue Dec 3 18:50:18 2019 (r355350)
+++ stable/12/usr.bin/patch/Makefile Tue Dec 3 18:55:09 2019 (r355351)
@@ -1,8 +1,13 @@
# $OpenBSD: Makefile,v 1.4 2005/05/16 15:22:46 espie Exp $
# $FreeBSD$
+.include <src.opts.mk>
+
PROG= patch
SRCS= backupfile.c inp.c mkpath.c patch.c pch.c util.c
+
+HAS_TESTS=
+SUBDIR.${MK_TESTS}+= tests
.include <bsd.prog.mk>
Modified: stable/12/usr.bin/patch/patch.1
==============================================================================
--- stable/12/usr.bin/patch/patch.1 Tue Dec 3 18:50:18 2019 (r355350)
+++ stable/12/usr.bin/patch/patch.1 Tue Dec 3 18:55:09 2019 (r355351)
@@ -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.
Modified: stable/12/usr.bin/patch/patch.c
==============================================================================
--- stable/12/usr.bin/patch/patch.c Tue Dec 3 18:50:18 2019 (r355350)
+++ stable/12/usr.bin/patch/patch.c Tue Dec 3 18:55:09 2019 (r355351)
@@ -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;
@@ -1083,4 +1138,85 @@ 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);
}
Modified: stable/12/usr.bin/patch/pch.c
==============================================================================
--- stable/12/usr.bin/patch/pch.c Tue Dec 3 18:50:18 2019 (r355350)
+++ stable/12/usr.bin/patch/pch.c Tue Dec 3 18:55:09 2019 (r355351)
@@ -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');
@@ -402,6 +409,24 @@ scan_exit:
struct file_name tmp = names[OLD_FILE];
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)
Modified: stable/12/usr.bin/patch/pch.h
==============================================================================
--- stable/12/usr.bin/patch/pch.h Tue Dec 3 18:50:18 2019 (r355350)
+++ stable/12/usr.bin/patch/pch.h Tue Dec 3 18:55:09 2019 (r355351)
@@ -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);
Modified: stable/12/usr.bin/patch/tests/unified_patch_test.sh
==============================================================================
--- head/usr.bin/patch/tests/unified_patch_test.sh Thu Sep 5 03:16:14 2019 (r351836)
+++ stable/12/usr.bin/patch/tests/unified_patch_test.sh Tue Dec 3 18:55:09 2019 (r355351)
@@ -102,32 +102,44 @@ file_creation_body()
# commits. If a file is created by a diff, patch(1) will happily duplicate the
# 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.
+# 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 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
+ # Check that the file is removed completely if it was sourced from
+ # /dev/null
atf_check -x "patch -Rs < foo.diff"
- atf_check -s not-exit:0 -o ignore stat foo
+ atf_check -s not-exit:0 -e ignore stat foo
+
+ # But if it had been modified, we'll only remove the portion that the
+ # patch would have created. This makes us compatible with GNU patch's
+ # behavior, at least. Whether that is the sane action or not is a
+ # question for further study, and then this comment may be removed.
+ printf "x\ny\n" > foo
+ atf_check -x "patch -Rs < foo.diff"
+ atf_check -o inline:"y\n" cat foo
}
atf_init_test_cases()
Modified: stable/12/usr.bin/patch/util.c
==============================================================================
--- stable/12/usr.bin/patch/util.c Tue Dec 3 18:50:18 2019 (r355350)
+++ stable/12/usr.bin/patch/util.c Tue Dec 3 18:55:09 2019 (r355351)
@@ -366,8 +366,10 @@ fetchname(const char *at, bool *exists, int strip_lead
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;
More information about the svn-src-stable
mailing list