* [PATCH v5 1/3] compat: add a mkstemps() compatibility function
@ 2009-05-30 2:18 David Aguilar
2009-05-30 2:18 ` [PATCH v5 2/3] path: add a find_basename() portability function David Aguilar
0 siblings, 1 reply; 5+ messages in thread
From: David Aguilar @ 2009-05-30 2:18 UTC (permalink / raw)
To: git; +Cc: gitster, peff, markus.heidelberg, jnareb, j.sixt, David Aguilar
mkstemps() is a BSD extension so provide an implementation
for cross-platform use.
Signed-off-by: David Aguilar <davvid@gmail.com>
Tested-by: Johannes Sixt <j6t@kdbg.org> (Windows)
---
This is unchanged since v3, but I'm including it for
completeness.
Makefile | 19 +++++++++++++++
compat/mkstemps.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++
config.mak.in | 1 +
configure.ac | 6 ++++
git-compat-util.h | 5 ++++
5 files changed, 98 insertions(+), 0 deletions(-)
create mode 100644 compat/mkstemps.c
diff --git a/Makefile b/Makefile
index eaae45d..a70b5f0 100644
--- a/Makefile
+++ b/Makefile
@@ -52,6 +52,8 @@ all::
#
# Define NO_MKDTEMP if you don't have mkdtemp in the C library.
#
+# Define NO_MKSTEMPS if you don't have mkstemps in the C library.
+#
# Define NO_SYS_SELECT_H if you don't have sys/select.h.
#
# Define NO_SYMLINK_HEAD if you never want .git/HEAD to be a symbolic link.
@@ -636,10 +638,12 @@ EXTLIBS =
ifeq ($(uname_S),Linux)
NO_STRLCPY = YesPlease
+ NO_MKSTEMPS = YesPlease
THREADED_DELTA_SEARCH = YesPlease
endif
ifeq ($(uname_S),GNU/kFreeBSD)
NO_STRLCPY = YesPlease
+ NO_MKSTEMPS = YesPlease
THREADED_DELTA_SEARCH = YesPlease
endif
ifeq ($(uname_S),UnixWare)
@@ -651,6 +655,7 @@ ifeq ($(uname_S),UnixWare)
SHELL_PATH = /usr/local/bin/bash
NO_IPV6 = YesPlease
NO_HSTRERROR = YesPlease
+ NO_MKSTEMPS = YesPlease
BASIC_CFLAGS += -Kthread
BASIC_CFLAGS += -I/usr/local/include
BASIC_LDFLAGS += -L/usr/local/lib
@@ -674,6 +679,7 @@ ifeq ($(uname_S),SCO_SV)
SHELL_PATH = /usr/bin/bash
NO_IPV6 = YesPlease
NO_HSTRERROR = YesPlease
+ NO_MKSTEMPS = YesPlease
BASIC_CFLAGS += -I/usr/local/include
BASIC_LDFLAGS += -L/usr/local/lib
NO_STRCASESTR = YesPlease
@@ -702,6 +708,7 @@ ifeq ($(uname_S),SunOS)
NO_MEMMEM = YesPlease
NO_HSTRERROR = YesPlease
NO_MKDTEMP = YesPlease
+ NO_MKSTEMPS = YesPlease
OLD_ICONV = UnfortunatelyYes
ifeq ($(uname_R),5.8)
NO_UNSETENV = YesPlease
@@ -724,6 +731,7 @@ ifeq ($(uname_O),Cygwin)
NO_D_INO_IN_DIRENT = YesPlease
NO_STRCASESTR = YesPlease
NO_MEMMEM = YesPlease
+ NO_MKSTEMPS = YesPlease
NO_SYMLINK_HEAD = YesPlease
NEEDS_LIBICONV = YesPlease
NO_FAST_WORKING_DIRECTORY = UnfortunatelyYes
@@ -767,11 +775,13 @@ ifeq ($(uname_S),NetBSD)
BASIC_LDFLAGS += -L/usr/pkg/lib $(CC_LD_DYNPATH)/usr/pkg/lib
THREADED_DELTA_SEARCH = YesPlease
USE_ST_TIMESPEC = YesPlease
+ NO_MKSTEMPS = YesPlease
endif
ifeq ($(uname_S),AIX)
NO_STRCASESTR=YesPlease
NO_MEMMEM = YesPlease
NO_MKDTEMP = YesPlease
+ NO_MKSTEMPS = YesPlease
NO_STRLCPY = YesPlease
NO_NSEC = YesPlease
FREAD_READS_DIRECTORIES = UnfortunatelyYes
@@ -787,12 +797,14 @@ endif
ifeq ($(uname_S),GNU)
# GNU/Hurd
NO_STRLCPY=YesPlease
+ NO_MKSTEMPS = YesPlease
endif
ifeq ($(uname_S),IRIX64)
NO_IPV6=YesPlease
NO_SETENV=YesPlease
NO_STRCASESTR=YesPlease
NO_MEMMEM = YesPlease
+ NO_MKSTEMPS = YesPlease
NO_STRLCPY = YesPlease
NO_SOCKADDR_STORAGE=YesPlease
SHELL_PATH=/usr/gnu/bin/bash
@@ -805,6 +817,7 @@ ifeq ($(uname_S),HP-UX)
NO_SETENV=YesPlease
NO_STRCASESTR=YesPlease
NO_MEMMEM = YesPlease
+ NO_MKSTEMPS = YesPlease
NO_STRLCPY = YesPlease
NO_MKDTEMP = YesPlease
NO_UNSETENV = YesPlease
@@ -834,6 +847,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
NO_C99_FORMAT = YesPlease
NO_STRTOUMAX = YesPlease
NO_MKDTEMP = YesPlease
+ NO_MKSTEMPS = YesPlease
SNPRINTF_RETURNS_BOGUS = YesPlease
NO_SVN_TESTS = YesPlease
NO_PERL_MAKEMAKER = YesPlease
@@ -853,6 +867,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
endif
ifneq (,$(findstring arm,$(uname_M)))
ARM_SHA1 = YesPlease
+ NO_MKSTEMPS = YesPlease
endif
-include config.mak.autogen
@@ -1011,6 +1026,10 @@ ifdef NO_MKDTEMP
COMPAT_CFLAGS += -DNO_MKDTEMP
COMPAT_OBJS += compat/mkdtemp.o
endif
+ifdef NO_MKSTEMPS
+ COMPAT_CFLAGS += -DNO_MKSTEMPS
+ COMPAT_OBJS += compat/mkstemps.o
+endif
ifdef NO_UNSETENV
COMPAT_CFLAGS += -DNO_UNSETENV
COMPAT_OBJS += compat/unsetenv.o
diff --git a/compat/mkstemps.c b/compat/mkstemps.c
new file mode 100644
index 0000000..87ebc2a
--- /dev/null
+++ b/compat/mkstemps.c
@@ -0,0 +1,67 @@
+#include "../git-compat-util.h"
+
+#ifndef TMP_MAX
+#define TMP_MAX 16384
+#endif
+
+/* Adapted from libiberty's mkstemp.c. */
+int gitmkstemps(char *pattern, int suffix_len)
+{
+ static const char letters[] =
+ "abcdefghijklmnopqrstuvwxyz"
+ "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+ "0123456789";
+ static const int num_letters = 62;
+ uint64_t value;
+ struct timeval tv;
+ char *template;
+ size_t len;
+ int fd, count;
+
+ len = strlen(pattern);
+
+ if (len < 6 + suffix_len) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ /* Replace pattern's XXXXXX characters with randomness.
+ * Try TMP_MAX different filenames.
+ */
+ gettimeofday(&tv, NULL);
+ value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid();
+ template = &pattern[len - 6 - suffix_len];
+ for (count = 0; count < TMP_MAX; ++count) {
+ uint64_t v = value;
+ /* Fill in the random bits. */
+ template[0] = letters[v % num_letters]; v/= num_letters;
+ template[1] = letters[v % num_letters]; v/= num_letters;
+ template[2] = letters[v % num_letters]; v/= num_letters;
+ template[3] = letters[v % num_letters]; v/= num_letters;
+ template[4] = letters[v % num_letters]; v/= num_letters;
+ template[5] = letters[v % num_letters]; v/= num_letters;
+
+ fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);
+ if (fd > 0)
+ return fd;
+ /* Fatal error (EPERM, ENOSPC etc).
+ * It doesn't make sense to loop.
+ */
+ if (errno != EEXIST)
+ break;
+ /* This is a random value. It is only necessary that
+ * the next TMP_MAX values generated by adding 7777 to
+ * VALUE are different with (module 2^32).
+ */
+ value += 7777;
+ }
+ /* We return the null string if we can't find a unique file name. */
+ pattern[0] = '\0';
+ errno = EINVAL;
+ return -1;
+}
diff --git a/config.mak.in b/config.mak.in
index 7cce0c1..b6619af 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -46,6 +46,7 @@ NO_STRTOUMAX=@NO_STRTOUMAX@
NO_SETENV=@NO_SETENV@
NO_UNSETENV=@NO_UNSETENV@
NO_MKDTEMP=@NO_MKDTEMP@
+NO_MKSTEMPS=@NO_MKSTEMPS@
NO_ICONV=@NO_ICONV@
OLD_ICONV=@OLD_ICONV@
NO_DEFLATE_BOUND=@NO_DEFLATE_BOUND@
diff --git a/configure.ac b/configure.ac
index 4e728bc..95dccd4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -676,6 +676,12 @@ GIT_CHECK_FUNC(mkdtemp,
[NO_MKDTEMP=],
[NO_MKDTEMP=YesPlease])
AC_SUBST(NO_MKDTEMP)
+# Define NO_MKSTEMPS if you don't have mkstemps in the C library.
+GIT_CHECK_FUNC(mkstemps,
+[NO_MKSTEMPS=],
+[NO_MKSTEMPS=YesPlease])
+AC_SUBST(NO_MKSTEMPS)
+#
#
# Define NO_MMAP if you want to avoid mmap.
#
diff --git a/git-compat-util.h b/git-compat-util.h
index c7cf2d5..f7217ad 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -232,6 +232,11 @@ extern int gitsetenv(const char *, const char *, int);
extern char *gitmkdtemp(char *);
#endif
+#ifdef NO_MKSTEMPS
+#define mkstemps gitmkstemps
+extern int gitmkstemps(char *, int);
+#endif
+
#ifdef NO_UNSETENV
#define unsetenv gitunsetenv
extern void gitunsetenv(const char *);
--
1.6.3.1.178.g4daa97
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v5 2/3] path: add a find_basename() portability function
2009-05-30 2:18 [PATCH v5 1/3] compat: add a mkstemps() compatibility function David Aguilar
@ 2009-05-30 2:18 ` David Aguilar
2009-05-30 2:18 ` [PATCH v5 3/3] diff: generate pretty filenames in prep_temp_blob() David Aguilar
2009-05-30 14:05 ` [PATCH v5 2/3] path: add a find_basename() portability function Jeff King
0 siblings, 2 replies; 5+ messages in thread
From: David Aguilar @ 2009-05-30 2:18 UTC (permalink / raw)
To: git; +Cc: gitster, peff, markus.heidelberg, jnareb, j.sixt, David Aguilar
find_basename() is a simpler version of basename().
It maintains constness and thus does not require you
to pass in a copy. It is only concerned with finding
the last directory separator in a string and returning
a pointer.
This function was written because Windows does not
have basename(). It is called "find_basename()"
to avoid name collisions and to provide a hint that
it is not quite the same thing as POSIX basename().
Signed-off-by: David Aguilar <davvid@gmail.com>
---
As suggested by Peff.
cache.h | 1 +
path.c | 14 ++++++++++++++
2 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/cache.h b/cache.h
index b8503ad..08b6f42 100644
--- a/cache.h
+++ b/cache.h
@@ -640,6 +640,7 @@ static inline int is_absolute_path(const char *path)
return path[0] == '/' || has_dos_drive_prefix(path);
}
int is_directory(const char *);
+const char *find_basename(const char *path);
const char *make_absolute_path(const char *path);
const char *make_nonrelative_path(const char *path);
const char *make_relative_path(const char *abs, const char *base);
diff --git a/path.c b/path.c
index 8a0a674..7a2fe14 100644
--- a/path.c
+++ b/path.c
@@ -358,6 +358,20 @@ int set_shared_perm(const char *path, int mode)
return 0;
}
+/* return the basename of a path */
+const char *find_basename(const char *path)
+{
+ const char *basename = path + strlen(path) - 1;
+ while(*basename && basename > path) {
+ basename--;
+ if (is_dir_sep(*basename)) {
+ basename++;
+ break;
+ }
+ }
+ return basename;
+}
+
const char *make_relative_path(const char *abs, const char *base)
{
static char buf[PATH_MAX + 1];
--
1.6.3.1.178.g4daa97
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v5 3/3] diff: generate pretty filenames in prep_temp_blob()
2009-05-30 2:18 ` [PATCH v5 2/3] path: add a find_basename() portability function David Aguilar
@ 2009-05-30 2:18 ` David Aguilar
2009-05-30 14:05 ` [PATCH v5 2/3] path: add a find_basename() portability function Jeff King
1 sibling, 0 replies; 5+ messages in thread
From: David Aguilar @ 2009-05-30 2:18 UTC (permalink / raw)
To: git; +Cc: gitster, peff, markus.heidelberg, jnareb, j.sixt, David Aguilar
Naturally, prep_temp_blob() did not care about filenames.
As a result, GIT_EXTERNAL_DIFF and textconv generated
filenames such as ".diff_XXXXXX".
This modifies prep_temp_blob() to generate user-friendly
filenames when creating temporary files.
Diffing "name.ext" now generates "XXXXXX_name.ext".
Signed-off-by: David Aguilar <davvid@gmail.com>
---
The difference since v4 is that find_basename() was
factored out into its own function.
cache.h | 2 ++
diff.c | 10 +++++++++-
path.c | 16 ++++++++++++++++
t/t4020-diff-external.sh | 9 +++++++++
4 files changed, 36 insertions(+), 1 deletions(-)
diff --git a/cache.h b/cache.h
index 08b6f42..337458d 100644
--- a/cache.h
+++ b/cache.h
@@ -614,6 +614,8 @@ extern int is_empty_blob_sha1(const unsigned char *sha1);
int git_mkstemp(char *path, size_t n, const char *template);
+int git_mkstemps(char *path, size_t n, const char *template, int suffix_len);
+
/*
* NOTE NOTE NOTE!!
*
diff --git a/diff.c b/diff.c
index dcfbcb0..7edee28 100644
--- a/diff.c
+++ b/diff.c
@@ -1964,8 +1964,15 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
{
int fd;
struct strbuf buf = STRBUF_INIT;
+ struct strbuf template = STRBUF_INIT;
+ const char *basename = find_basename(path);
- fd = git_mkstemp(temp->tmp_path, PATH_MAX, ".diff_XXXXXX");
+ /* Generate "XXXXXX_basename.ext" */
+ strbuf_addstr(&template, "XXXXXX_");
+ strbuf_addstr(&template, basename);
+
+ fd = git_mkstemps(temp->tmp_path, PATH_MAX, template.buf,
+ strlen(basename) + 1);
if (fd < 0)
die("unable to create temp-file: %s", strerror(errno));
if (convert_to_working_tree(path,
@@ -1981,6 +1988,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
temp->hex[40] = 0;
sprintf(temp->mode, "%06o", mode);
strbuf_release(&buf);
+ strbuf_release(&template);
}
static struct diff_tempfile *prepare_temp_file(const char *name,
diff --git a/path.c b/path.c
index 7a2fe14..4f7fe8f 100644
--- a/path.c
+++ b/path.c
@@ -139,6 +139,22 @@ int git_mkstemp(char *path, size_t len, const char *template)
return mkstemp(path);
}
+/* git_mkstemps() - create tmp file with suffix honoring TMPDIR variable. */
+int git_mkstemps(char *path, size_t len, const char *template, int suffix_len)
+{
+ const char *tmp;
+ size_t n;
+
+ tmp = getenv("TMPDIR");
+ if (!tmp)
+ tmp = "/tmp";
+ n = snprintf(path, len, "%s/%s", tmp, template);
+ if (len <= n) {
+ errno = ENAMETOOLONG;
+ return -1;
+ }
+ return mkstemps(path, suffix_len);
+}
int validate_headref(const char *path)
{
diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index 0720001..4ea42e0 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -136,6 +136,15 @@ test_expect_success 'GIT_EXTERNAL_DIFF with more than one changed files' '
GIT_EXTERNAL_DIFF=echo git diff
'
+test_expect_success 'GIT_EXTERNAL_DIFF generates pretty paths' '
+ touch file.ext &&
+ git add file.ext &&
+ echo with extension > file.ext &&
+ GIT_EXTERNAL_DIFF=echo git diff file.ext | grep ......_file\.ext &&
+ git update-index --force-remove file.ext &&
+ rm file.ext
+'
+
echo "#!$SHELL_PATH" >fake-diff.sh
cat >> fake-diff.sh <<\EOF
cat $2 >> crlfed.txt
--
1.6.3.1.178.g4daa97
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v5 2/3] path: add a find_basename() portability function
2009-05-30 2:18 ` [PATCH v5 2/3] path: add a find_basename() portability function David Aguilar
2009-05-30 2:18 ` [PATCH v5 3/3] diff: generate pretty filenames in prep_temp_blob() David Aguilar
@ 2009-05-30 14:05 ` Jeff King
2009-05-30 22:14 ` David Aguilar
1 sibling, 1 reply; 5+ messages in thread
From: Jeff King @ 2009-05-30 14:05 UTC (permalink / raw)
To: David Aguilar; +Cc: git, gitster, markus.heidelberg, jnareb, j.sixt
On Fri, May 29, 2009 at 07:18:09PM -0700, David Aguilar wrote:
> +/* return the basename of a path */
> +const char *find_basename(const char *path)
> +{
> + const char *basename = path + strlen(path) - 1;
> + while(*basename && basename > path) {
> + basename--;
> + if (is_dir_sep(*basename)) {
> + basename++;
> + break;
> + }
> + }
> + return basename;
> +}
Hmm. Is there any point to the *basename condition in the loop? By using
strlen, you are not going to go past any NULs, and you are already
checking that you don't go past the beginning of the string.
Speaking of which, how does this handle an input of ""? It seems that it
would return a pointer to one character before the string. Given your
loop, you need to special-case when *path is NUL.
Also, how should trailing dir_seps be handled? basename() will actually
return "" if given "foo/". Your implementation, when given "/foo/bar/"
will return "bar/" (and it must keep the trailing bit since we are
neither reallocating nor munging the input string). But given
"/foo/bar//", it will return simply "/". I could see an argument for
either "bar//" or "", but I think behaving differently for trailing "/"
versus trailing "//" doesn't make sense.
-Peff
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5 2/3] path: add a find_basename() portability function
2009-05-30 14:05 ` [PATCH v5 2/3] path: add a find_basename() portability function Jeff King
@ 2009-05-30 22:14 ` David Aguilar
0 siblings, 0 replies; 5+ messages in thread
From: David Aguilar @ 2009-05-30 22:14 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, markus.heidelberg, jnareb, j.sixt
On Sat, May 30, 2009 at 10:05:19AM -0400, Jeff King wrote:
> On Fri, May 29, 2009 at 07:18:09PM -0700, David Aguilar wrote:
>
> > +/* return the basename of a path */
> > +const char *find_basename(const char *path)
> > +{
> > + const char *basename = path + strlen(path) - 1;
> > + while(*basename && basename > path) {
> > + basename--;
> > + if (is_dir_sep(*basename)) {
> > + basename++;
> > + break;
> > + }
> > + }
> > + return basename;
> > +}
>
> Hmm. Is there any point to the *basename condition in the loop? By using
> strlen, you are not going to go past any NULs, and you are already
> checking that you don't go past the beginning of the string.
>
> Speaking of which, how does this handle an input of ""? It seems that it
> would return a pointer to one character before the string. Given your
> loop, you need to special-case when *path is NUL.
>
> Also, how should trailing dir_seps be handled? basename() will actually
> return "" if given "foo/". Your implementation, when given "/foo/bar/"
> will return "bar/" (and it must keep the trailing bit since we are
> neither reallocating nor munging the input string). But given
> "/foo/bar//", it will return simply "/". I could see an argument for
> either "bar//" or "", but I think behaving differently for trailing "/"
> versus trailing "//" doesn't make sense.
>
> -Peff
You know.. going with the compat/basename.c solution is probably
a better idea now that you've pointed out all these issues.
Then we can just use the system's basename() when available and
use our compat/basename.c otherwise (is it only for Windows?).
Here's basename() from libiberty.
It drops constness (likely to keep in line with the standard)
and has some #defines that I'll have to translate into git
equivalents (e.g. IS_DIR_SEPARATOR -> is_dir_sep).
Do we have a cross-platform ISALPHA thing (just thinking out
loud -- I can check). I'm under the impression that going with
something like this should get us in a better place.
What do you think?
char *basename (const char *name)
{
const char *base;
#if defined (HAVE_DOS_BASED_FILE_SYSTEM)
/* Skip over the disk name in MSDOS pathnames. */
if (ISALPHA (name[0]) && name[1] == ':')
name += 2;
#endif
for (base = name; *name; name++) {
if (IS_DIR_SEPARATOR (*name)) {
base = name + 1;
}
}
return (char *) base;
}
--
David
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-05-30 22:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-05-30 2:18 [PATCH v5 1/3] compat: add a mkstemps() compatibility function David Aguilar
2009-05-30 2:18 ` [PATCH v5 2/3] path: add a find_basename() portability function David Aguilar
2009-05-30 2:18 ` [PATCH v5 3/3] diff: generate pretty filenames in prep_temp_blob() David Aguilar
2009-05-30 14:05 ` [PATCH v5 2/3] path: add a find_basename() portability function Jeff King
2009-05-30 22:14 ` David Aguilar
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).