svn commit: r354328 - in head/usr.bin/patch: . tests
Kyle Evans
kevans at FreeBSD.org
Mon Nov 4 03:07:03 UTC 2019
Author: kevans
Date: Mon Nov 4 03:07:01 2019
New Revision: 354328
URL: https://svnweb.freebsd.org/changeset/base/354328
Log:
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
Modified:
head/usr.bin/patch/patch.1
head/usr.bin/patch/patch.c
head/usr.bin/patch/pch.c
head/usr.bin/patch/pch.h
head/usr.bin/patch/tests/unified_patch_test.sh
head/usr.bin/patch/util.c
Modified: head/usr.bin/patch/patch.1
==============================================================================
--- head/usr.bin/patch/patch.1 Mon Nov 4 02:29:58 2019 (r354327)
+++ head/usr.bin/patch/patch.1 Mon Nov 4 03:07:01 2019 (r354328)
@@ -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: head/usr.bin/patch/patch.c
==============================================================================
--- head/usr.bin/patch/patch.c Mon Nov 4 02:29:58 2019 (r354327)
+++ head/usr.bin/patch/patch.c Mon Nov 4 03:07:01 2019 (r354328)
@@ -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: head/usr.bin/patch/pch.c
==============================================================================
--- head/usr.bin/patch/pch.c Mon Nov 4 02:29:58 2019 (r354327)
+++ head/usr.bin/patch/pch.c Mon Nov 4 03:07:01 2019 (r354328)
@@ -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: head/usr.bin/patch/pch.h
==============================================================================
--- head/usr.bin/patch/pch.h Mon Nov 4 02:29:58 2019 (r354327)
+++ head/usr.bin/patch/pch.h Mon Nov 4 03:07:01 2019 (r354328)
@@ -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: head/usr.bin/patch/tests/unified_patch_test.sh
==============================================================================
--- head/usr.bin/patch/tests/unified_patch_test.sh Mon Nov 4 02:29:58 2019 (r354327)
+++ head/usr.bin/patch/tests/unified_patch_test.sh Mon Nov 4 03:07:01 2019 (r354328)
@@ -103,26 +103,27 @@ 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
Modified: head/usr.bin/patch/util.c
==============================================================================
--- head/usr.bin/patch/util.c Mon Nov 4 02:29:58 2019 (r354327)
+++ head/usr.bin/patch/util.c Mon Nov 4 03:07:01 2019 (r354328)
@@ -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-head
mailing list