@matheusbsbs / matheus-bagliano / commits / 479b0ae81c

Fork 0

diff: refactor tempfile cleanup handling

There are two pieces of code that create tempfiles for diff: run_external_diff and run_textconv. The former cleans up its tempfiles in the face of premature death (i.e., by die() or by signal), but the latter does not. After this patch, they will both use the same cleanup routines. To make clear what the change is, let me first explain what happens now: - run_external_diff uses a static global array of 2 diff_tempfile structs (since it knows it will always need exactly 2 tempfiles). It calls prepare_temp_file (which doesn't know anything about the global array) on each of the structs, creating the tempfiles that need to be cleaned up. It then registers atexit and signal handlers to look through the global array and remove the tempfiles. If it succeeds, it calls the handler manually (which marks the tempfile structs as unused). - textconv has its own tempfile struct, which it allocates using prepare_temp_file and cleans up manually. No signal or atexit handlers. The new code moves the installation of cleanup handlers into the prepare_temp_file function. Which means that that function now has to understand that there is static tempfile storage. So what happens now is: - run_external_diff calls prepare_temp_file - prepare_temp_file calls claim_diff_tempfile, which allocates an unused slot from our global array - prepare_temp_file installs (if they have not already been installed) atexit and signal handlers for cleanup - prepare_temp_file sets up the tempfile as usual - prepare_temp_file returns a pointer to the allocated tempfile The advantage being that run_external_diff no longer has to care about setting up cleanup handlers. Now by virtue of calling prepare_temp_file, run_textconv gets the same benefit, as will any future users of prepare_temp_file. There are also a few side benefits to the specific implementation: - we now install cleanup handlers _before_ allocating the tempfile, closing a race which could leave temp cruft - when allocating a slot in the global array, we will now detect a situation where the old slots were not properly vacated (i.e., somebody forgot to call remove upon leaving the function). In the old code, such a situation would silently overwrite the tempfile names, meaning we would forget to clean them up. The new code dies with a bug warning. - we make sure only to install the signal handler once. This isn't a big deal, since we are just overwriting the old handler, but will become an issue when a later patch converts the code to use sigchain Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Jan 22, 2009 at 00:59 UTC 479b0ae81c9291a8bb8d7b2347cc58eeaa701304

1 file changed +55 -52

diff.c

+55 -52

@@ -165,6 +165,33 @@ static struct diff_tempfile {

165 char tmp_path[PATH_MAX];

166 } diff_temp[2];

167

168 +static struct diff_tempfile *claim_diff_tempfile(void) {

169 + int i;

170 + for (i = 0; i < ARRAY_SIZE(diff_temp); i++)

171 + if (!diff_temp[i].name)

172 + return diff_temp + i;

173 + die("BUG: diff is failing to clean up its tempfiles");

174 +}

175 +

176 +static int remove_tempfile_installed;

177 +

178 +static void remove_tempfile(void)

179 +{

180 + int i;

181 + for (i = 0; i < ARRAY_SIZE(diff_temp); i++)

182 + if (diff_temp[i].name == diff_temp[i].tmp_path) {

183 + unlink(diff_temp[i].name);

184 + diff_temp[i].name = NULL;

185 + }

186 +}

187 +

188 +static void remove_tempfile_on_signal(int signo)

189 +{

190 + remove_tempfile();

191 + signal(SIGINT, SIG_DFL);

192 + raise(signo);

193 +}

194 +

195 static int count_lines(const char *data, int size)

196 {

197 int count, ch, completely_empty = 1, nl_just_seen = 0;

@@ -1857,10 +1884,11 @@ static void prep_temp_blob(struct diff_tempfile *temp,

1884 sprintf(temp->mode, "%06o", mode);

1885 }

1886

1860 -static void prepare_temp_file(const char *name,

1861 - struct diff_tempfile *temp,

1862 - struct diff_filespec *one)

1887 +static struct diff_tempfile *prepare_temp_file(const char *name,

1888 + struct diff_filespec *one)

1889 {

1890 + struct diff_tempfile *temp = claim_diff_tempfile();

1891 +

1892 if (!DIFF_FILE_VALID(one)) {

1893 not_a_valid_file:

1894 /* A '-' entry produces this for file-2, and

@@ -1869,7 +1897,13 @@ static void prepare_temp_file(const char *name,

1897 temp->name = "/dev/null";

1898 strcpy(temp->hex, ".");

1899 strcpy(temp->mode, ".");

1872 - return;

1900 + return temp;

1901 + }

1902 +

1903 + if (!remove_tempfile_installed) {

1904 + atexit(remove_tempfile);

1905 + signal(SIGINT, remove_tempfile_on_signal);

1906 + remove_tempfile_installed = 1;

1907 }

1908

1909 if (!one->sha1_valid ||

@@ -1909,7 +1943,7 @@ static void prepare_temp_file(const char *name,

1943 */

1944 sprintf(temp->mode, "%06o", one->mode);

1945 }

1912 - return;

1946 + return temp;

1947 }

1948 else {

1949 if (diff_populate_filespec(one, 0))

@@ -1917,24 +1951,7 @@ static void prepare_temp_file(const char *name,

1951 prep_temp_blob(temp, one->data, one->size,

1952 one->sha1, one->mode);

1953 }

1920 -}

1921 -

1922 -static void remove_tempfile(void)

1923 -{

1924 - int i;

1925 -

1926 - for (i = 0; i < 2; i++)

1927 - if (diff_temp[i].name == diff_temp[i].tmp_path) {

1928 - unlink(diff_temp[i].name);

1929 - diff_temp[i].name = NULL;

1930 - }

1931 -}

1932 -

1933 -static void remove_tempfile_on_signal(int signo)

1934 -{

1935 - remove_tempfile();

1936 - signal(SIGINT, SIG_DFL);

1937 - raise(signo);

1954 + return temp;

1955 }

1956

1957 /* An external diff command takes:

@@ -1952,34 +1969,22 @@ static void run_external_diff(const char *pgm,

1969 int complete_rewrite)

1970 {

1971 const char *spawn_arg[10];

1955 - struct diff_tempfile *temp = diff_temp;

1972 int retval;

1957 - static int atexit_asked = 0;

1958 - const char *othername;

1973 const char **arg = &spawn_arg[0];

1974

1961 - othername = (other? other : name);

1962 - if (one && two) {

1963 - prepare_temp_file(name, &temp[0], one);

1964 - prepare_temp_file(othername, &temp[1], two);

1965 - if (! atexit_asked &&

1966 - (temp[0].name == temp[0].tmp_path ||

1967 - temp[1].name == temp[1].tmp_path)) {

1968 - atexit_asked = 1;

1969 - atexit(remove_tempfile);

1970 - }

1971 - signal(SIGINT, remove_tempfile_on_signal);

1972 - }

1973 -

1975 if (one && two) {

1976 + struct diff_tempfile *temp_one, *temp_two;

1977 + const char *othername = (other ? other : name);

1978 + temp_one = prepare_temp_file(name, one);

1979 + temp_two = prepare_temp_file(othername, two);

1980 *arg++ = pgm;

1981 *arg++ = name;

1977 - *arg++ = temp[0].name;

1978 - *arg++ = temp[0].hex;

1979 - *arg++ = temp[0].mode;

1980 - *arg++ = temp[1].name;

1981 - *arg++ = temp[1].hex;

1982 - *arg++ = temp[1].mode;

1982 + *arg++ = temp_one->name;

1983 + *arg++ = temp_one->hex;

1984 + *arg++ = temp_one->mode;

1985 + *arg++ = temp_two->name;

1986 + *arg++ = temp_two->hex;

1987 + *arg++ = temp_two->mode;

1988 if (other) {

1989 *arg++ = other;

1990 *arg++ = xfrm_msg;

@@ -3448,15 +3453,15 @@ void diff_unmerge(struct diff_options *options,

3453 static char *run_textconv(const char *pgm, struct diff_filespec *spec,

3454 size_t *outsize)

3455 {

3451 - struct diff_tempfile temp;

3456 + struct diff_tempfile *temp;

3457 const char *argv[3];

3458 const char **arg = argv;

3459 struct child_process child;

3460 struct strbuf buf = STRBUF_INIT;

3461

3457 - prepare_temp_file(spec->path, &temp, spec);

3462 + temp = prepare_temp_file(spec->path, spec);

3463 *arg++ = pgm;

3459 - *arg++ = temp.name;

3464 + *arg++ = temp->name;

3465 *arg = NULL;

3466

3467 memset(&child, 0, sizeof(child));

@@ -3465,13 +3470,11 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,

3470 if (start_command(&child) != 0 ||

3471 strbuf_read(&buf, child.out, 0) < 0 ||

3472 finish_command(&child) != 0) {

3468 - if (temp.name == temp.tmp_path)

3469 - unlink(temp.name);

3473 + remove_tempfile();

3474 error("error running textconv command '%s'", pgm);

3475 return NULL;

3476 }

3473 - if (temp.name == temp.tmp_path)

3474 - unlink(temp.name);

3477 + remove_tempfile();

3478

3479 return strbuf_detach(&buf, outsize);

3480 }