RE: [PATCH v4 4/4] make poll() work on platforms that can't recv() on a non-socket

[Joachim Schmitz]

 From: Erik Faye-Lund [mailto:kusmab...@gmail.com]
 Sent: Saturday, September 08, 2012 1:32 PM
 To: Joachim Schmitz
 Cc: Junio C Hamano; git@vger.kernel.org
 Subject: Re: [PATCH v4 4/4] make poll() work on platforms that can't recv() 
 on a non-socket
 
 On Fri, Sep 7, 2012 at 5:43 PM, Joachim Schmitz j...@schmitz-digital.de 
 wrote:
  This way it gets added to gnulib too.
 
  Signed-off-by: Joachim Schmitz j...@schmitz-digital.de
  ---
   compat/poll/poll.c | 5 +
   1 file changed, 4 insertions(+)
 
  diff --git a/compat/poll/poll.c b/compat/poll/poll.c
  index e4b8319..10a204e 100644
  --- a/compat/poll/poll.c
  +++ b/compat/poll/poll.c
  @@ -306,6 +306,10 @@ compute_revents (int fd, int sought, fd_set *rfds, 
  fd_set *wfds, fd_set *efds)
 || socket_errno == ECONNABORTED || socket_errno == ENETRESET)
  happened |= POLLHUP;
 
  +  /* some systems can't use recv() on non-socket, including HP NonStop 
  */
  +  else if (/* (r == -1)  */ socket_errno == ENOTSOCK)
 
 Why add commented-out code ((r == -1)  )?

I'm not really sure whether it may be needed, esp. for Mac OS X, where en 
ENOTSOCK is expected from a recv() but then dealt with by
an ioctl(), which does not resert socket_errno, but does set r to something not 
-1. And wouldn't need this code path?
There's some similar code a few lines up, which too has that part commented out:

  /* If the event happened on an unconnected server socket,
 that's fine. */
  else if (r  0 || ( /* (r == -1)  */ socket_errno == ENOTCONN))
happened |= (POLLIN | POLLRDNORM)  sought;

Bye, Jojo

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cherry-pick: Append -x line on separate paragraph

[Robin Stocker]

Junio C Hamano writes:
 Robin Stocker ro...@nibor.org writes:
 
  Junio C Hamano writes:
  Robin Stocker ro...@nibor.org writes:
 
if (opts-record_origin) {
   + /* Some implementations don't terminate message with final \n,
   so
   add it */
   + if (msg.message[strlen(msg.message)-1] != '\n')
   + strbuf_addch(msgbuf, '\n');
 
  I can agree that this is a good change.
 
   + strbuf_addch(msgbuf, '\n');
 
  But this is somewhat dubious. Even if what we are adding is merely
  an extra LF, that changes the mechanically generated output format
  and can break existing hooks that read from these generated commit
  log template.
 
  Hm, for a script to break because of an extra LF it would have to be
  very badly written. If it looks for \n(cherry picked ..., it would
  still work. But I see the point.
 
 If you approach this change from the information left by -x is
 somehow useful point of view, it wouldn't make any difference.
 Scripts can match (cherry picked from ... and glean useful
 information out of it, with or without an empty line around it.
 
 But if you look at it from the other perspective [*1*], it makes a
 big difference. A script that wants to excise these lines used to
 be able to just delete such a line. With the proposed change, it
 also has to be aware of an empty line next to it.

Ok, didn't think that a script would want to remove such a line. It
makes sense when considering that it used to always add the line.
Thanks for explaining.

  Is there a reason better than having an empty line there look
  better to _me_ to justify this change?
 
  Yes:
 
 Then please have them in the proposed commit log message to justify
 the change. I think the following analysis I quoted from your
 message summarizes the motivation well.
 
  * If the original commit message consisted just of a summary line,
the commit message after -x would then not have a blank second
line, which is bad style, e.g.:
 
  The title of the original commit
  (cherry picked ...)
 
 This is very true. So we at least want an empty line added when the
 original is one-liner.
 
  * If the original message did not have any trailers, the appended
text would stick to the last paragraph, even though it is a
separate thing.
 
 The other side of this argument is if there are trailers, we would
 not want an extra blank line. We need to look at the last paragraph
 of the log message and if it does not end with a trailer, we want an
 additional empty line.
 
  Maybe the solution is to detect if the original commit message
  ends with a trailer and in that case keep the existing behavior
  of not inserting a blank line?
 
 Yeah, that sounds like a good change from this makes the result
 look better point of view.

How do you think we could best detect a tailer? Check if all
lines of the last paragraph start with /[\w-]+: /?

  Oh, I like that proposal. I'd lean towards a new --trailer option I
  think.
 
  It would have the same problem of having to append it on a separate
  paragraph if the original commit message does not already have a
  trailer though.
 
 Yes. The logic would be the same. First terminate the incomplete
 last line, if any, and then look at the last paragraph of the commit
 log message (one liner is a natural degenerate case of this; its
 single-line title is the last paragraph) and if and only if it does
 not end with a trailer, add a blank line before adding the marker.
 
 The only difference between the two would be how the cherry-picked
 from line is formatted.

Right.

I'm going to work on this and submit a new version of the patch. The
Cherry-picked-from change could then be made later on top of that.

 [Footnote]
 
 *1* Originally, we added (cherry picked from ... by default, and
 had a switch to disable it; later we made it off by default and made
 it optional (and discouraged) with -x and this was for a reason.
 Unless the original commit object is also available to the reader of
 the history, the line is a useless noise, and many people are found
 to cherry-pick from their private branches; by definition, the line
 is useless in the resulting commit of such a cherry-pick.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3] Support for setitimer() on platforms lacking it

[Joachim Schmitz]

 From: Joachim Schmitz [mailto:j...@schmitz-digital.de]
 Sent: Friday, September 07, 2012 11:55 AM
 To: 'Junio C Hamano'
 Cc: 'git@vger.kernel.org'
 Subject: RE: [PATCH v3] Support for setitimer() on platforms lacking it
 
 HP NonStop (currently) doesn't have setitimer(). The previous attempt of an
 emulation (reverted by this commit) was not a real substitute for a recurring
 itimer (as here we also don't have SA_RESTART, so can't re-arm the timer).
 As setitimer() is only used in cases of perceived latency and it doesn't 
 affect
 correctness, it now gets disabled entirely, if NO_SETITIMER is set.
 HP NonStop does provide struct itimerval, but other platforms may not, so this
 is taken care of in this commit too, by setting NO_STRUCT_ITIMERVAL.
 
 Signed-off-by: Joachim Schmitz j...@schmitz-digital.de
 ---
  Makefile  |  5 +
  compat/itimer.c   | 50 --
  git-compat-util.h | 11 +--
  3 files changed, 14 insertions(+), 52 deletions(-)
  delete mode 100644 compat/itimer.c
 
 diff --git a/Makefile b/Makefile
 index ac49320..7be555b 100644
 --- a/Makefile
 +++ b/Makefile
 @@ -157,6 +157,11 @@ all::
  # Define NO_PREAD if you have a problem with pread() system call (e.g.
  # cygwin1.dll before v1.5.22).
  #
 +# Define NO_SETITIMER if you don't have setitimer()
 +#
 +# Define NO_STRUCT_ITIMERVAL if you don't have struct itimerval
 +# This also implies NO_SETITIMER
 +#
  # Define NO_THREAD_SAFE_PREAD if your pread() implementation is not
  # thread-safe. (e.g. compat/pread.c or cygwin)
  #

Here too (just like in my MKDIR_WO_TRAILING_SLASH patch) it is missing the part 
that adds

ifdef NO_STRUCT_ITIMERVAL
COMPAT_CFLAGS += -DNO_STRUCT_ITIMERVAL
NO_SETITIMER=YesPlease
endif
ifdef NO_SETITIMER
COMPAT_CFLAGS += -DNO_SETITIMER
endif

 diff --git a/compat/itimer.c b/compat/itimer.c
 deleted file mode 100644
 index 713f1ff..000
 --- a/compat/itimer.c
 +++ /dev/null
 @@ -1,50 +0,0 @@
 -#include ../git-compat-util.h
 -
 -static int git_getitimer(int which, struct itimerval *value)
 -{
 - int ret = 0;
 -
 - switch (which) {
 - case ITIMER_REAL:
 - value-it_value.tv_usec = 0;
 - value-it_value.tv_sec = alarm(0);
 - ret = 0; /* if alarm() fails, we get a SIGLIMIT */
 - break;
 - case ITIMER_VIRTUAL: /* FALLTHRU */
 - case ITIMER_PROF: errno = ENOTSUP; ret = -1; break;
 - default: errno = EINVAL; ret = -1;
 - }
 - return ret;
 -}
 -
 -int git_setitimer(int which, const struct itimerval *value,
 - struct itimerval *ovalue)
 -{
 - int ret = 0;
 -
 - if (!value
 - || value-it_value.tv_usec  0
 - || value-it_value.tv_usec  100
 - || value-it_value.tv_sec  0) {
 - errno = EINVAL;
 - return -1;
 - }
 -
 - else if (ovalue)
 - if (!git_getitimer(which, ovalue))
 - return -1; /* errno set in git_getitimer() */
 -
 - else
 - switch (which) {
 - case ITIMER_REAL:
 - alarm(value-it_value.tv_sec +
 - (value-it_value.tv_usec  0) ? 1 : 0);
 - ret = 0; /* if alarm() fails, we get a SIGLIMIT */
 - break;
 - case ITIMER_VIRTUAL: /* FALLTHRU */
 - case ITIMER_PROF: errno = ENOTSUP; ret = -1; break;
 - default: errno = EINVAL; ret = -1;
 - }
 -
 - return ret;
 -}
 diff --git a/git-compat-util.h b/git-compat-util.h
 index 18089f0..4628d7a 100644
 --- a/git-compat-util.h
 +++ b/git-compat-util.h
 @@ -162,9 +162,16 @@
  #define probe_utf8_pathname_composition(a,b)
  #endif
 
 +#ifdef NO_STRUCT_ITIMERVAL
 +struct itimerval {
 + struct timeval it_interval;
 + struct timeval it_value;
 +}
 +#define NO_SETITIMER

The above line gets obsolete with further up mentioned change in Makefile

 +#endif
 +
  #ifdef NO_SETITIMER
 -#define setitimer(a,b,c) git_setitimer((a),(b),(c))
 -extern int git_setitimer(int, const struct itimerval *, struct itimerval *);
 +#define setitimer(which,value,ovalue)
  #endif
 
  #ifdef MKDIR_WO_TRAILING_SLASH
 --
 1.7.12

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] doc: move rev-list option -n from git-log.txt to rev-list-options.txt

[Nguyen Thai Ngoc Duy]

On Sat, Sep 8, 2012 at 12:14 AM, Junio C Hamano gits...@pobox.com wrote:
 Michael J Gruber g...@drmicha.warpmail.net writes:
  Documentation/git-log.txt  | 6 ++
  Documentation/rev-list-options.txt | 3 ++-
  2 tập tin đã bị thay đổi, 4 được thêm vào(+), 5 bị xóa(-)

 That is one reason why core.local=C (repo specific) and git -c
 core.locale=C (can be used in an alias) would be useful ;)

 Or LC_ALL=C LANG=C git format-patch 

The only problem is I forget to do that from time to time (and doing
that bothers me too)

 It does not bother me (even though I do not read Vietnamese), but
 this has been brought up a few times, and we may want to revert the
 i18n of the diffstat summary.  It does not seem to add much value to
 the system but annoys people.

That's one step towards a better interface for non English speaking
users. git log interface for example still shows Author, Commit,
Date in English and these strings are shared with format-patch.
Reverting back to English to me is a step back.

This brings back to a series I posted about two weeks ago and got no comments

http://article.gmane.org/gmane.comp.version-control.git/204285

I think it's a reasonable approach. Use English for machine interface,
otherwise a native language if available.

 After all, the upstream diffstat
 does not localizes this string (I just checked diffstat-1.55 with
 Jan 2012 timestamp).
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4] Support for setitimer() on platforms lacking it

[Joachim Schmitz]

HP NonStop (currently) doesn't have setitimer().
As setitimer() is only used in cases of perceived latency and it doesn't affect
correctness, it gets disabled entirely if NO_SETITIMER is set.
HP NonStop does provide struct itimerval, but other platforms may not, so this
is taken care of in this commit too, by setting NO_STRUCT_ITIMERVAL.

Signed-off-by: Joachim Schmitz j...@schmitz-digital.de
---
 Makefile  | 12 
 git-compat-util.h | 11 +++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index ac49320..7be555b 100644
--- a/Makefile
+++ b/Makefile
@@ -157,6 +157,11 @@ all::
 # Define NO_PREAD if you have a problem with pread() system call (e.g.
 # cygwin1.dll before v1.5.22).
 #
+# Define NO_SETITIMER if you don't have setitimer()
+#
+# Define NO_STRUCT_ITIMERVAL if you don't have struct itimerval
+# This also implies NO_SETITIMER
+#
 # Define NO_THREAD_SAFE_PREAD if your pread() implementation is not
 # thread-safe. (e.g. compat/pread.c or cygwin)
 #
@@ -1647,6 +1652,13 @@
 ifdef OBJECT_CREATION_USES_RENAMES
COMPAT_CFLAGS += -DOBJECT_CREATION_MODE=1
 endif
+ifdef NO_STRUCT_ITIMERVAL
+   COMPAT_CFLAGS += -DNO_STRUCT_ITIMERVAL
+   NO_SETITIMER=YesPlease
+endif
+ifdef NO_SETITIMER
+   COMPAT_CFLAGS += -DNO_SETITIMER
+endif
 ifdef NO_PREAD
COMPAT_CFLAGS += -DNO_PREAD
COMPAT_OBJS += compat/pread.o
diff --git a/git-compat-util.h b/git-compat-util.h
index 18089f0..4628d7a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -162,6 +162,17 @@
 #define probe_utf8_pathname_composition(a,b)
 #endif
 
+#ifdef NO_STRUCT_ITIMERVAL
+struct itimerval {
+   struct timeval it_interval;
+   struct timeval it_value;
+}
+#endif
+
+#ifdef NO_SETITIMER
+#define setitimer(which,value,ovalue)
+#endif
+
 #ifdef MKDIR_WO_TRAILING_SLASH
 #define mkdir(a,b) compat_mkdir_wo_trailing_slash((a),(b))
 extern int compat_mkdir_wo_trailing_slash(const char*, mode_t);
-- 
1.7.12

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] Document MKDIR_WO_TRAILING_SLASH in Makefile

[Joachim Schmitz]

Signed-off-by: Joachim Schmitz j...@schmitz-digital.de
---
Makefile | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/Makefile b/Makefile
index 66e8216..21b4816 100644
--- a/Makefile
+++ b/Makefile
@@ -90,6 +90,8 @@ all::
 #
 # Define NO_MKDTEMP if you don't have mkdtemp in the C library.
 #
+# Define MKDIR_WO_TRAILING_SLASH if your mkdir() can't deal with trailing 
slash.
+#
 # Define NO_MKSTEMPS if you don't have mkstemps in the C library.
 #
 # Define NO_STRTOK_R if you don't have strtok_r in the C library.
@@ -1639,6 +1641,10 @@ ifdef NO_MKDTEMP
COMPAT_CFLAGS += -DNO_MKDTEMP
COMPAT_OBJS += compat/mkdtemp.o
 endif
+ifdef MKDIR_WO_TRAILING_SLASH
+   COMPAT_CFLAGS += -DMKDIR_WO_TRAILING_SLASH
+   COMPAT_OBJS += compat/mkdir.o
+endif
 ifdef NO_MKSTEMPS
COMPAT_CFLAGS += -DNO_MKSTEMPS
 endif
-- 
1.7.12

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Restore hostname logging in inetd mode

[Jan Engelhardt]

The following changes since commit 0ce986446163b37c7f663ce7a408e7f94c31ba63:

  The fourth batch for 1.8.0 (2012-09-07 11:25:22 -0700)

are available in the git repository at:

  git://git.inai.de/git master

for you to fetch changes up to 864633738f6432574402afc43b6bd83c83fc8916:

  daemon: restore getpeername(0,...) use (2012-09-08 19:00:35 +0200)


Jan Engelhardt (1):
  daemon: restore getpeername(0,...) use

 daemon.c |   55 +++
 1 file changed, 51 insertions(+), 4 deletions(-)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] daemon: restore getpeername(0,...) use

[Jan Engelhardt]

This reverts f9c87be6b42dd0f8b31a4bb8c6a44326879fdd1a, in a sense,
because that commit broke logging of Connection from ... when
git-daemon is run under xinetd.

This patch here computes the text representation of the peer and then
copies that to environment variables such that the code in execute()
and subfunctions can stay as-is.

Signed-off-by: Jan Engelhardt jeng...@inai.de
---
 daemon.c |   55 +++
 1 file changed, 51 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index 4602b46..eaf08c2 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1,3 +1,4 @@
+#include stdbool.h
 #include cache.h
 #include pkt-line.h
 #include exec_cmd.h
@@ -1164,6 +1165,54 @@ static int serve(struct string_list *listen_addr, int 
listen_port,
return service_loop(socklist);
 }
 
+static void inetd_mode_prepare(void)
+{
+   struct sockaddr_storage ss;
+   struct sockaddr *addr = (void *)ss;
+   socklen_t slen = sizeof(ss);
+   char addrbuf[256], portbuf[6] = ;
+
+   if (!freopen(/dev/null, w, stderr))
+   die_errno(failed to redirect stderr to /dev/null);
+
+   /*
+* Windows is said to not be able to handle this, so we will simply
+* ignore failure here. (It only affects a log message anyway.)
+*/
+   if (getpeername(0, addr, slen)  0)
+   return;
+
+   if (addr-sa_family == AF_INET) {
+   const struct sockaddr_in *sin_addr = (void *)addr;
+
+   if (inet_ntop(addr-sa_family, sin_addr-sin_addr,
+ addrbuf, sizeof(addrbuf)) == NULL)
+   return;
+   snprintf(portbuf, sizeof(portbuf), %hu,
+ntohs(sin_addr-sin_port));
+#ifndef NO_IPV6
+   } else if (addr-sa_family == AF_INET6) {
+   const struct sockaddr_in6 *sin6_addr = (void *)addr;
+
+   addrbuf[0] = '[';
+   addrbuf[1] = '\0';
+   if (inet_ntop(AF_INET6, sin6_addr-sin6_addr, addrbuf + 1,
+ sizeof(addrbuf) - 2) == NULL)
+   return;
+   strcat(addrbuf, ]);
+
+   snprintf(portbuf, sizeof(portbuf), %hu,
+ntohs(sin6_addr-sin6_port));
+#endif
+   } else {
+   snprintf(addrbuf, sizeof(addrbuf), AF %d,
+addr-sa_family);
+   }
+   if (setenv(REMOTE_ADDR, addrbuf, true)  0)
+   return;
+   setenv(REMOTE_PORT, portbuf, true);
+}
+
 int main(int argc, char **argv)
 {
int listen_port = 0;
@@ -1341,10 +1390,8 @@ int main(int argc, char **argv)
die(base-path '%s' does not exist or is not a directory,
base_path);
 
-   if (inetd_mode) {
-   if (!freopen(/dev/null, w, stderr))
-   die_errno(failed to redirect stderr to /dev/null);
-   }
+   if (inetd_mode)
+   inetd_mode_prepare();
 
if (inetd_mode || serve_mode)
return execute();
-- 
1.7.10.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] daemon: restore getpeername(0,...) use

[Joachim Schmitz]

Jan Engelhardt wrote:

This reverts f9c87be6b42dd0f8b31a4bb8c6a44326879fdd1a, in a sense,
because that commit broke logging of Connection from ... when
git-daemon is run under xinetd.

This patch here computes the text representation of the peer and then
copies that to environment variables such that the code in execute()
and subfunctions can stay as-is.

Signed-off-by: Jan Engelhardt jeng...@inai.de
---
daemon.c |   55
+++ 1 file
changed, 51 insertions(+), 4 deletions(-) 


diff --git a/daemon.c b/daemon.c
index 4602b46..eaf08c2 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1,3 +1,4 @@
+#include stdbool.h
#include cache.h
#include pkt-line.h
#include exec_cmd.h
@@ -1164,6 +1165,54 @@ static int serve(struct string_list
 *listen_addr, int listen_port, return service_loop(socklist);
}

+static void inetd_mode_prepare(void)
+{
+ struct sockaddr_storage ss;
+ struct sockaddr *addr = (void *)ss;
+ socklen_t slen = sizeof(ss);
+ char addrbuf[256], portbuf[6] = ;
+
+ if (!freopen(/dev/null, w, stderr))
+ die_errno(failed to redirect stderr to /dev/null);
+
+ /*
+ * Windows is said to not be able to handle this, so we will simply
+ * ignore failure here. (It only affects a log message anyway.)
+ */
+ if (getpeername(0, addr, slen)  0)
+ return;
+
+ if (addr-sa_family == AF_INET) {
+ const struct sockaddr_in *sin_addr = (void *)addr;
+
+ if (inet_ntop(addr-sa_family, sin_addr-sin_addr,
+   addrbuf, sizeof(addrbuf)) == NULL)
+ return;
+ snprintf(portbuf, sizeof(portbuf), %hu,
+ ntohs(sin_addr-sin_port));
+#ifndef NO_IPV6
+ } else if (addr-sa_family == AF_INET6) {
+ const struct sockaddr_in6 *sin6_addr = (void *)addr;
+
+ addrbuf[0] = '[';
+ addrbuf[1] = '\0';
+ if (inet_ntop(AF_INET6, sin6_addr-sin6_addr, addrbuf + 1,
+   sizeof(addrbuf) - 2) == NULL)
+ return;
+ strcat(addrbuf, ]);
+
+ snprintf(portbuf, sizeof(portbuf), %hu,
+ ntohs(sin6_addr-sin6_port));
+#endif
+ } else {
+ snprintf(addrbuf, sizeof(addrbuf), AF %d,
+ addr-sa_family);
+ }
+ if (setenv(REMOTE_ADDR, addrbuf, true)  0)
+ return;
+ setenv(REMOTE_PORT, portbuf, true);


setenv() is not a function available on all plattfomrs.

Bye, Jojo

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG] 'git show' gives duplicate errors for ambiguous args

[Ori Avtalion]

With the git.git repository:

  $ git show abcd
  error: short SHA1 abcd is ambiguous.
  error: short SHA1 abcd is ambiguous.
  fatal: ambiguous argument 'abcd': unknown revision or path not in the
working tree.
  Use '--' to separate paths from revisions

The is ambiguous message shouldn't be shown twice.

The first error is printed by revision.c:1796 (a call to
handle_revision_arg), and the second by revision.c:1808 (a call to
verify_filename). I don't see an easy way to suppress it.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: checkout extra files

[Junio C Hamano]

Angelo Borsotti angelo.borso...@gmail.com writes:

 It makes quite clear that the command accepts wildcards
 (not expanded by the shell), which was is not clear in the current
 man page (although one could imagine that path could also be a
 wildcard).

 P.S. In the man page there is also a pathspec

 *git checkout* [-p|--patch] [tree-ish] [--] pathspec...

 that should perhaps be a path

That's backwards.  Saying path as if it means a plain vanilla
pathname is a cause of confusion.  The command takes pathspec, which
is a pattern (see git help glossary). The places in the text that
say path may need to be fixed.

It just happens that you do not realize that you are using pathspec
when you say git checkout hello.c, as the pattern hello.c only
matches the one pathname hello.c.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Restore hostname logging in inetd mode

[Junio C Hamano]

Please don't throw a pull request for a patch whose worth hasn't
been justified in a discussion on the list.  Thanks.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] gitk: Teach Reread references to reload tags

[David Aguilar]

Tag contents, once read, are forever cached in memory.
This makes gitk unable to notice when tag contents change.

Allow users to cause a reload of the tag contents by using
the File-Reread references action.

Reported-by: Tim McCormack cor...@brainonfire.net
Suggested-by: Junio C Hamano gits...@pobox.com
Signed-off-by: David Aguilar dav...@gmail.com
---
 gitk |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/gitk b/gitk
index 9bba9aa..a124822 100755
--- a/gitk
+++ b/gitk
@@ -10599,7 +10599,7 @@ proc movedhead {hid head} {
 }
 
 proc changedrefs {} {
-global cached_dheads cached_dtags cached_atags
+global cached_dheads cached_dtags cached_atags tagcontents
 global arctags archeads arcnos arcout idheads idtags
 
 foreach id [concat [array names idheads] [array names idtags]] {
@@ -10611,6 +10611,7 @@ proc changedrefs {} {
}
}
 }
+catch {unset tagcontents}
 catch {unset cached_dtags}
 catch {unset cached_atags}
 catch {unset cached_dheads}
-- 
1.7.7.2.448.gee6df

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] daemon: restore getpeername(0,...) use

[Junio C Hamano]

Joachim Schmitz j...@schmitz-digital.de writes:

 + setenv(REMOTE_PORT, portbuf, true);

 setenv() is not a function available on all plattfomrs.

Please do some homework before adding irrelevant noise.  At the
minimum, run git grep to see if we already use it in other places,
and investigate why we can use it safely across platforms we already
support.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Restore hostname logging in inetd mode

[Jan Engelhardt]

On Saturday 2012-09-08 20:57, Junio C Hamano wrote:

Please don't throw a pull request for a patch whose worth hasn't
been justified in a discussion on the list.  Thanks.

Let me postulate that people like to get cover letters with the
git:// URL so they can fetch+look at it, a diffstat and shortlog.

And 'lo, that is exactly what git-request-pull thankfully generates.

In my defense: Just because the command is called request-pull,
does not mean you absolutely have to merge/pull it. In fact, it does
not even mention merge/pull at all.


The following changes since commit [SHA]:
  [Commit Message]
are available in the git repository at:
  git://[...]
for you to fetch changes up to [SHA]:
  [Commit Message]
[diffstat,shortlog]


In contrast to many a LKML postings which explicitly state the pull
intent:


Hi [Maintainer],

Please pull from the git repository at
  [URL]
to receive [...]
  [SHA]
  [Commit Message]
on top of commit [SHA]
  [Commit Message]
[diffstat,shortlog]

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] daemon: restore getpeername(0,...) use

[Joachim Schmitz]

 From: Junio C Hamano [mailto:gits...@pobox.com]
 Sent: Saturday, September 08, 2012 9:04 PM
 To: Joachim Schmitz
 Cc: git@vger.kernel.org
 Subject: Re: [PATCH] daemon: restore getpeername(0,...) use
 
 Joachim Schmitz j...@schmitz-digital.de writes:
 
  + setenv(REMOTE_PORT, portbuf, true);
 
  setenv() is not a function available on all plattfomrs.
 
 Please do some homework before adding irrelevant noise.  At the
 minimum, run git grep to see if we already use it in other places,
 and investigate why we can use it safely across platforms we already
 support.

Hmm, guess I missed the indirect inclusion of git-compat-util.h
And didn't know about 'git grep', so thanks for the hint

Will look closer next time ;-)

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] daemon: restore getpeername(0,...) use

[Jan Engelhardt]

On Saturday 2012-09-08 20:59, Junio C Hamano wrote:
 diff --git a/daemon.c b/daemon.c
 index 4602b46..eaf08c2 100644
 --- a/daemon.c
 +++ b/daemon.c
 @@ -1,3 +1,4 @@
 +#include stdbool.h
  #include cache.h
  #include pkt-line.h
  #include exec_cmd.h

Platform agnostic parts of the code that use git-compat-util.h
(users of cache.h are indirectly users of it) are not allowed to
do platform specific include like this at their beginning.

This is the first use of stdbool.h; what do you need it for?

For the use in setenv(,,true). It was not entirely obvious in which .h 
to add it; the most reasonable place was daemon.c itself, since the 
other .c files do not seem to need it.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] gitk: Rename 'tagcontents' to 'cached_tagcontent'

[David Aguilar]

Name the 'tagcontents' variable similarly to the rest of the
variables cleared in the changedrefs() function.

This makes the naming consistent and provides a hint that it
should be cleared when reloading gitk's cache.

Suggested-by: Junio C Hamano gits...@pobox.com
Signed-off-by: David Aguilar dav...@gmail.com
---

Follow-up to 'gitk: Teach Reread references to reload tags'

 gitk |   14 +++---
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/gitk b/gitk
index a124822..6f24f53 100755
--- a/gitk
+++ b/gitk
@@ -10599,7 +10599,7 @@ proc movedhead {hid head} {
 }
 
 proc changedrefs {} {
-global cached_dheads cached_dtags cached_atags tagcontents
+global cached_dheads cached_dtags cached_atags cached_tagcontent
 global arctags archeads arcnos arcout idheads idtags
 
 foreach id [concat [array names idheads] [array names idtags]] {
@@ -10611,7 +10611,7 @@ proc changedrefs {} {
}
}
 }
-catch {unset tagcontents}
+catch {unset cached_tagcontent}
 catch {unset cached_dtags}
 catch {unset cached_atags}
 catch {unset cached_dheads}
@@ -10664,7 +10664,7 @@ proc listrefs {id} {
 }
 
 proc showtag {tag isnew} {
-global ctext tagcontents tagids linknum tagobjid
+global ctext cached_tagcontent tagids linknum tagobjid
 
 if {$isnew} {
addtohistory [list showtag $tag 0] savectextpos
@@ -10673,13 +10673,13 @@ proc showtag {tag isnew} {
 clear_ctext
 settabs 0
 set linknum 0
-if {![info exists tagcontents($tag)]} {
+if {![info exists cached_tagcontent($tag)]} {
catch {
-   set tagcontents($tag) [exec git cat-file tag $tag]
+   set cached_tagcontent($tag) [exec git cat-file tag $tag]
}
 }
-if {[info exists tagcontents($tag)]} {
-   set text $tagcontents($tag)
+if {[info exists cached_tagcontent($tag)]} {
+   set text $cached_tagcontent($tag)
 } else {
set text [mc Tag]: $tag\n[mc Id]:  $tagids($tag)
 }
-- 
1.7.7.2.448.gee6df

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: checkout extra files

[Philip Oakley]

From: Junio C Hamano gits...@pobox.com
Sent: Friday, September 07, 2012 9:49 PM

Junio C Hamano gits...@pobox.com writes:


But that is not what is happening at all.  What goes on is far
simpler than that.

 - the shell sees '*', matches it against working tree files, to
   obtain f1 and f2;

 - the shell tells git to checkout e6f935e -- f1 f2;

 - git looks into the tree of e6f935e to find paths that match
   f1 and f2.

When git is run by the shell in the last step, it has _no_ clue
that the end user typed * from the shell.  It only sees f1 and
f2 on the command line.  There is no set T to be intersected
with set W, so stop thinking in those terms, and you will be fine.

Now the question is, _you_ will be fine, but can the documentation
be updated in such a way so that it will help _others_ to also stop
thinking about intersection between set W and set T?  I do not
have a good answer to that.


Let's do this.  I do not want a shell tutorial in git checkout
documentation, but this would fit better in the documentation for
the CLI convention.


The difficulty with putting it in gitcli is that it is referenced from 
almost nowhere, so won't provide help to the user.


Having said that, it would therefore be better to point folk at gitcli 
in a few more places, not just the 'see also' line at the very end of 
the general 'git' page, and buried within rev-parse.




-- 8 --
gitcli: contrast wildcard given to shell and to git

People who are not used to working with shell may intellectually
understand how the command line argument is massaged by the shell
but still have a hard time visualizing the difference between
letting the shell expand fileglobs and having Git see the fileglob
to use as a pathspec.

Signed-off-by: Junio C Hamano gits...@pobox.com
---
Documentation/gitcli.txt | 16 
1 file changed, 16 insertions(+)

diff --git c/Documentation/gitcli.txt w/Documentation/gitcli.txt
index ea17f7a..220621b 100644
--- c/Documentation/gitcli.txt
+++ w/Documentation/gitcli.txt
@@ -38,6 +38,22 @@ arguments.  Here are the rules:
   you have to say either `git diff HEAD --` or `git diff -- HEAD` to
   disambiguate.

+ * Many commands allow wildcards in paths, but you need to protect
+them from getting globbed by the shell.  These two mean different 
things:

++
+
+$ git checkout -- *.c
+$ git checkout -- \*.c
+
++
+The former lets your shell expand the fileglob, and you are asking
+the dot-C files in your working tree to be overwritten with the 
version

+in the index.  The latter passes the `*.c` to Git, and you are asking
+the paths in the index that match the pattern to be checked out to 
your
+working tree.  After running `git add hello.c; rm hello.c`, you will 
_not_
+see `hello.c` in your working tree with the former, but with the 
latter

+you will.
+
When writing a script that is expected to handle random user-input, it 
is
a good practice to make it explicit which arguments are which by 
placing

disambiguating `--` at appropriate places.


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: checkout extra files

[Junio C Hamano]

Philip Oakley philipoak...@iee.org writes:

 Having said that, it would therefore be better to point folk at gitcli
 in a few more places, not just the 'see also' line at the very end of
 the general 'git' page, and buried within rev-parse.

Didn't we update the very early part of git(1) for exactly for that
reason recently?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths

[Michael Haggerty]

On 09/07/2012 01:08 AM, Junio C Hamano wrote:
 mhag...@alum.mit.edu writes:
 
 From: Michael Haggerty mhag...@alum.mit.edu

 There is currently a bug: if passed an absolute top-level path that
 doesn't exist (e.g., /foo) it incorrectly interprets the path as a
 relative path (e.g., returns $(pwd)/foo).  So mark the test as
 failing.

 Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
 ---
  t/t-basic.sh | 12 +++-
  1 file changed, 11 insertions(+), 1 deletion(-)

 diff --git a/t/t-basic.sh b/t/t-basic.sh
 index 1a51634..ad002ee 100755
 --- a/t/t-basic.sh
 +++ b/t/t-basic.sh
 @@ -458,7 +458,17 @@ test_expect_success 'real path rejects the empty 
 string' '
  test_must_fail test-path-utils real_path 
  '
  
 -test_expect_success SYMLINKS 'real path works as expected' '
 +test_expect_failure 'real path works on absolute paths' '
 +nopath=hopefully-absent-path 
 +test / = $(test-path-utils real_path /) 
 +test /$nopath = $(test-path-utils real_path /$nopath) 
 
 You could perhaps do
 
   sfx=0 
 while test -e /$nopath$sfx
 do
   sfx=$(( $sfx + 1 ))
   done  nopath=$nopath$sfx 
   test /$nopath = $(test-path-utils real_path /$nopath) 
 
 if you really cared.

The possibility is obvious.  Are you advocating it?

I considered that approach, but came to the opinion that it would be
overkill that would only complicate the code for no real advantage,
given that (1) I picked a name that is pretty implausible for an
existing file, (2) the test suite only reads the file, never writes it
(so there is no risk that a copy from a previous run gets left behind),
(3) it's only test suite code, and any failures would have minor
consequences.

Please let me know if you assess the situation differently.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] Teaching -Xours/-Xtheirs to binary ll-merge driver

[Junio C Hamano]

The part that grants Stephen's wish is unchanged from the earlier
perhaps like this patch, but this time with a bit of documentation
and test.

A more important change between the two actually is [PATCH 2/2].
When a binary synthetic attribute is given to a path, we used to
(1) disable textual diff and (2) disable CR/LF conversion, but it is
also sane to disable the textual merge for a path marked as
binary, and setting the -merge attribute to summon the binary
ll-merge driver is the way to do so.

Junio C Hamano (2):
  merge: teach -Xours/-Xtheirs to binary ll-merge driver
  attr: binary attribute should choose built-in binary merge driver

 Documentation/gitattributes.txt|  2 +-
 Documentation/merge-strategies.txt |  3 ++-
 attr.c |  2 +-
 ll-merge.c | 25 -
 t/t6037-merge-ours-theirs.sh   | 14 +-
 5 files changed, 37 insertions(+), 9 deletions(-)

-- 
1.7.12.322.g2c7d289

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] merge: teach -Xours/-Xtheirs to binary ll-merge driver

[Junio C Hamano]

The (discouraged) -Xours/-Xtheirs modes of merge are supposed to
give a quick and dirty way to come up with a random mixture of
cleanly merged parts and punted conflict resolution to take contents
from one side in conflicting parts.  These options however were only
passed down to the low level merge driver for text.

Teach the built-in binary merge driver to notice them as well.

Signed-off-by: Junio C Hamano gits...@pobox.com
---
 Documentation/merge-strategies.txt |  3 ++-
 ll-merge.c | 25 -
 t/t6037-merge-ours-theirs.sh   | 14 +-
 3 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/Documentation/merge-strategies.txt 
b/Documentation/merge-strategies.txt
index 595a3cf..66db802 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -32,13 +32,14 @@ ours;;
This option forces conflicting hunks to be auto-resolved cleanly by
favoring 'our' version.  Changes from the other tree that do not
conflict with our side are reflected to the merge result.
+   For a binary file, the entire contents are taken from our side.
 +
 This should not be confused with the 'ours' merge strategy, which does not
 even look at what the other tree contains at all.  It discards everything
 the other tree did, declaring 'our' history contains all that happened in it.
 
 theirs;;
-   This is opposite of 'ours'.
+   This is the opposite of 'ours'.
 
 patience;;
With this option, 'merge-recursive' spends a little extra time
diff --git a/ll-merge.c b/ll-merge.c
index da59738..8535e2d 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -46,16 +46,31 @@ static int ll_binary_merge(const struct ll_merge_driver 
*drv_unused,
assert(opts);
 
/*
-* The tentative merge result is ours for the final round,
-* or common ancestor for an internal merge.  Still return
-* conflicted merge status.
+* The tentative merge result is the or common ancestor for an internal 
merge.
 */
-   stolen = opts-virtual_ancestor ? orig : src1;
+   if (opts-virtual_ancestor) {
+   stolen = orig;
+   } else {
+   switch (opts-variant) {
+   default:
+   case XDL_MERGE_FAVOR_OURS:
+   stolen = src1;
+   break;
+   case XDL_MERGE_FAVOR_THEIRS:
+   stolen = src2;
+   break;
+   }
+   }
 
result-ptr = stolen-ptr;
result-size = stolen-size;
stolen-ptr = NULL;
-   return 1;
+
+   /*
+* With -Xtheirs or -Xours, we have cleanly merged;
+* otherwise we got a conflict.
+*/
+   return (opts-variant ? 0 : 1);
 }
 
 static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
diff --git a/t/t6037-merge-ours-theirs.sh b/t/t6037-merge-ours-theirs.sh
index 2cf42c7..8d05671 100755
--- a/t/t6037-merge-ours-theirs.sh
+++ b/t/t6037-merge-ours-theirs.sh
@@ -53,7 +53,19 @@ test_expect_success 'recursive favouring ours' '
! grep 1 file
 '
 
-test_expect_success 'pull with -X' '
+test_expect_success 'binary file with -Xours/-Xtheirs' '
+   echo file -merge .gitattributes 
+
+   git reset --hard master 
+   git merge -s recursive -X theirs side 
+   git diff --exit-code side HEAD -- file 
+
+   git reset --hard master 
+   git merge -s recursive -X ours side 
+   git diff --exit-code master HEAD -- file
+'
+
+test_expect_success 'pull passes -X to underlying merge' '
git reset --hard master  git pull -s recursive -Xours . side 
git reset --hard master  git pull -s recursive -X ours . side 
git reset --hard master  git pull -s recursive -Xtheirs . side 
-- 
1.7.12.322.g2c7d289

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] attr: binary attribute should choose built-in binary merge driver

[Junio C Hamano]

The built-in binary attribute macro expands to -diff -text, so
that textual diff is not produced, and the contents will not go
through any CR/LF conversion ever.  During a merge, it should also
choose the binary low-level merge driver, but it didn't.

Make it expand to -diff -merge -text.

Signed-off-by: Junio C Hamano gits...@pobox.com
---
 Documentation/gitattributes.txt | 2 +-
 attr.c  | 2 +-
 t/t6037-merge-ours-theirs.sh| 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index a85b187..ead7254 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -904,7 +904,7 @@ file at the toplevel (i.e. not in any subdirectory).  The 
built-in
 macro attribute binary is equivalent to:
 
 
-[attr]binary -diff -text
+[attr]binary -diff -merge -text
 
 
 
diff --git a/attr.c b/attr.c
index 303751f..3f581b3 100644
--- a/attr.c
+++ b/attr.c
@@ -306,7 +306,7 @@ static void free_attr_elem(struct attr_stack *e)
 }
 
 static const char *builtin_attr[] = {
-   [attr]binary -diff -text,
+   [attr]binary -diff -merge -text,
NULL,
 };
 
diff --git a/t/t6037-merge-ours-theirs.sh b/t/t6037-merge-ours-theirs.sh
index 8d05671..3889eca 100755
--- a/t/t6037-merge-ours-theirs.sh
+++ b/t/t6037-merge-ours-theirs.sh
@@ -54,7 +54,7 @@ test_expect_success 'recursive favouring ours' '
 '
 
 test_expect_success 'binary file with -Xours/-Xtheirs' '
-   echo file -merge .gitattributes 
+   echo file binary .gitattributes 
 
git reset --hard master 
git merge -s recursive -X theirs side 
-- 
1.7.12.322.g2c7d289

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] Support for setitimer() on platforms lacking it

[Junio C Hamano]

Will queue (together with the latest MKDIR one).  Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/8] absolute_path(): reject the empty string

[Michael Haggerty]

On 09/07/2012 01:09 AM, Junio C Hamano wrote:
 Michael Haggerty mhag...@alum.mit.edu writes:
 
 Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
 ---
 
 I think I asked why this matters (iow, why it is the right thing to
 do to reject an empty string, instead of treating it as the current
 directory) in the previous round.  I would have expected to find
 the answer be above the S-o-b line here.

The reasons that the change is desirable:

1. The empty string is not a legitimate path according to POSIX; e.g.,
see Linux's path_resolution(7):

   Empty pathname
   In the original UNIX, the empty pathname referred to the current
   directory.   Nowadays  POSIX  decrees  that  an  empty  pathname
   must not be resolved successfully.  Linux returns ENOENT in this
   case.

Accordingly, comparable standard functions like realpath(3) reject the
empty string.

2. The functions did not handle the empty path consistently with the way
they handled other paths (namely, the return value contained a trailing
slash).

3. This unusual behavior was undocumented.

The above points let me to the conclusion that the anomalous handling of
the empty string was a bug in the implementation rather than an intended
behavior.  Moreover, a quick check of callers didn't turn up any that
seemed to rely on the strange behavior.

Do you want a re-roll with this verbiage added to the commit messages of
the two relevant commits?

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths

[Junio C Hamano]

Michael Haggerty mhag...@alum.mit.edu writes:

 The possibility is obvious.  Are you advocating it?

 I considered that approach, but came to the opinion that it would be
 overkill that would only complicate the code for no real advantage,
 given that (1) I picked a name that is pretty implausible for an
 existing file, (2) the test suite only reads the file, never writes it
 (so there is no risk that a copy from a previous run gets left behind),
 (3) it's only test suite code, and any failures would have minor
 consequences.

(4) if it only runs once at the very beginning of the test and sets
a variable that is named prominently clear what it means and lives
throughout the test, then we do not even have to say hopefully and
appear lazy and loose to the readers of the test who wonders what
happens when the path does exist; doing so will help reducing the
noise on the mailing list in the future.


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/8] absolute_path(): reject the empty string

[Junio C Hamano]

Michael Haggerty mhag...@alum.mit.edu writes:

 On 09/07/2012 01:09 AM, Junio C Hamano wrote:
 Michael Haggerty mhag...@alum.mit.edu writes:
 
 Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
 ---
 
 I think I asked why this matters (iow, why it is the right thing to
 do to reject an empty string, instead of treating it as the current
 directory) in the previous round.  I would have expected to find
 the answer be above the S-o-b line here.

 The reasons that the change is desirable:

 1. The empty string is not a legitimate path according to POSIX; e.g.,
 see Linux's path_resolution(7):

Empty pathname
In the original UNIX, the empty pathname referred to the current
directory.   Nowadays  POSIX  decrees  that  an  empty  pathname
must not be resolved successfully.  Linux returns ENOENT in this
case.

 Accordingly, comparable standard functions like realpath(3) reject the
 empty string.

 2. The functions did not handle the empty path consistently with the way
 they handled other paths (namely, the return value contained a trailing
 slash).

 3. This unusual behavior was undocumented.

 The above points let me to the conclusion that the anomalous handling of
 the empty string was a bug in the implementation rather than an intended
 behavior.  Moreover, a quick check of callers didn't turn up any that
 seemed to rely on the strange behavior.

 Do you want a re-roll with this verbiage added to the commit messages of
 the two relevant commits?

What the function used to do does not really matter when we are
trying to see if the behaviour of the new implementation makes
sense, even though it is a good supporting argument to help us judge
that this change will not cause regressions to existing callers.

A two sentence paragraph, with realpath(3) from POSIX.1 rejects an
empty string as input as the primary justification, with no
existing callers passes an empty string and expects to get the
current directory as a supporting argument, should suffice, I would
think.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gitk: Rename 'tagcontents' to 'cached_tagcontent'

[Junio C Hamano]

I've applied these two, on top of Paul's master branch at

git://ozlabs.org/~paulus/gitk.git

and tentatively queued in 'pu', but I would prefer to see it
eyeballed by and queued in his tree first.

Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] t0000: verify that real_path() works correctly with absolute paths

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 (4) if it only runs once at the very beginning of the test and sets
 a variable that is named prominently clear what it means and lives
 throughout the test, then we do not even have to say hopefully and
 appear lazy and loose to the readers of the test who wonders what
 happens when the path does exist; doing so will help reducing the
 noise on the mailing list in the future.

Having said that, I really do not deeply care either way.  If you
are rerollilng the series for other changes, I wouldn't shed tears
even if I do not see any change to this part.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/4] Add some string_list-related functions

[Michael Haggerty]

This patch series adds a few functions to the string_list API.  They
will be used in two upcoming patch series.  Unfortunately, both of the
series (which are otherwise logically independent) need the same
function; therefore, I am submitting these string-list enhancements as
a separate series on which the other two can depend.

This patch series applies to current master.

Michael Haggerty (4):
  Add a new function, string_list_split_in_place()
  Add a new function, filter_string_list()
  Add a new function, string_list_remove_duplicates()
  Add a function string_list_longest_prefix()

 .gitignore  |  1 +
 Documentation/technical/api-string-list.txt | 32 ++
 Makefile|  1 +
 string-list.c   | 77 
 string-list.h   | 41 +
 t/t0063-string-list.sh  | 93 +
 test-string-list.c  | 47 +++
 7 files changed, 292 insertions(+)
 create mode 100755 t/t0063-string-list.sh
 create mode 100644 test-string-list.c

-- 
1.7.11.3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] Add a new function, string_list_split_in_place()

[Michael Haggerty]

Split a string into a string_list on a separator character.

This is similar to the strbuf_split_*() functions except that it works
with the more powerful string_list interface.  If strdup_strings is
false, it reuses the memory from the input string (thereby needing no
string memory allocations, though of course allocations are still
needed for the string_list_items array).

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---

In the tests, I use here documents to specify the expected output.  Is
this OK?  (It is certainly convenient.)

 .gitignore  |  1 +
 Documentation/technical/api-string-list.txt | 12 ++
 Makefile|  1 +
 string-list.c   | 23 +++
 string-list.h   | 19 +
 t/t0063-string-list.sh  | 63 +
 test-string-list.c  | 25 
 7 files changed, 144 insertions(+)
 create mode 100755 t/t0063-string-list.sh
 create mode 100644 test-string-list.c

diff --git a/.gitignore b/.gitignore
index bb5c91e..0ca7df8 100644
--- a/.gitignore
+++ b/.gitignore
@@ -193,6 +193,7 @@
 /test-run-command
 /test-sha1
 /test-sigchain
+/test-string-list
 /test-subprocess
 /test-svn-fe
 /common-cmds.h
diff --git a/Documentation/technical/api-string-list.txt 
b/Documentation/technical/api-string-list.txt
index 5a0c14f..3b959a2 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -124,6 +124,18 @@ counterpart for sorted lists, which performs a binary 
search.
is set. The third parameter controls if the `util` pointer of the
items should be freed or not.
 
+`string_list_split_in_place`::
+
+   Split string into substrings on character delim and append the
+   substrings to a string_list.  The delimiter characters in
+   string are overwritten with NULs in the process.  If maxsplit
+   is a positive integer, then split at most maxsplit times.  If
+   list.strdup_strings is not set, then the new string_list_items
+   point into string, which therefore must not be modified or
+   freed while the string_list is in use.  Return the number of
+   substrings appended to the list.
+
+
 Data structures
 ---
 
diff --git a/Makefile b/Makefile
index 66e8216..ebbb381 100644
--- a/Makefile
+++ b/Makefile
@@ -501,6 +501,7 @@ TEST_PROGRAMS_NEED_X += test-run-command
 TEST_PROGRAMS_NEED_X += test-scrap-cache-tree
 TEST_PROGRAMS_NEED_X += test-sha1
 TEST_PROGRAMS_NEED_X += test-sigchain
+TEST_PROGRAMS_NEED_X += test-string-list
 TEST_PROGRAMS_NEED_X += test-subprocess
 TEST_PROGRAMS_NEED_X += test-svn-fe
 
diff --git a/string-list.c b/string-list.c
index d9810ab..110449c 100644
--- a/string-list.c
+++ b/string-list.c
@@ -194,3 +194,26 @@ void unsorted_string_list_delete_item(struct string_list 
*list, int i, int free_
list-items[i] = list-items[list-nr-1];
list-nr--;
 }
+
+int string_list_split_in_place(struct string_list *list, char *string,
+  int delim, int maxsplit)
+{
+   int count = 0;
+   char *p = string, *end;
+   for (;;) {
+   count++;
+   if (maxsplit  0  count  maxsplit) {
+   string_list_append(list, p);
+   return count;
+   }
+   end = strchr(p, delim);
+   if (end) {
+   *end = '\0';
+   string_list_append(list, p);
+   p = end + 1;
+   } else {
+   string_list_append(list, p);
+   return count;
+   }
+   }
+}
diff --git a/string-list.h b/string-list.h
index 0684cb7..7e51d03 100644
--- a/string-list.h
+++ b/string-list.h
@@ -45,4 +45,23 @@ int unsorted_string_list_has_string(struct string_list 
*list, const char *string
 struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
 const char *string);
 void unsorted_string_list_delete_item(struct string_list *list, int i, int 
free_util);
+
+/*
+ * Split string into substrings on character delim and append the
+ * substrings to list.  The delimiter characters in string are
+ * overwritten with NULs in the process.  If maxsplit is a positive
+ * integer, then split at most maxsplit times.  If list.strdup_strings
+ * is not set, then the new string_list_items point into string, which
+ * therefore must not be modified or freed while the string_list
+ * is in use.  Return the number of substrings appended to list.
+ *
+ * Examples:
+ *   string_list_split_in_place(l, foo:bar:baz, ':', -1) - [foo, bar, 
baz]
+ *   string_list_split_in_place(l, foo:bar:baz, ':', 1) - [foo, bar:baz]
+ *   string_list_split_in_place(l, foo:bar:, ':', -1) - [foo, bar, ]
+ *   string_list_split_in_place(l, , ':', 

[PATCH 3/4] Add a new function, string_list_remove_duplicates()

[Michael Haggerty]

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 Documentation/technical/api-string-list.txt |  4 
 string-list.c   | 17 +
 string-list.h   |  5 +
 3 files changed, 26 insertions(+)

diff --git a/Documentation/technical/api-string-list.txt 
b/Documentation/technical/api-string-list.txt
index 15b8072..9206f8f 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -104,6 +104,10 @@ write `string_list_insert(...)-util = ...;`.
Look up a given string in the string_list, returning the containing
string_list_item. If the string is not found, NULL is returned.
 
+`string_list_remove_duplicates`::
+
+   Remove all but the first entry that has a given string value.
+
 * Functions for unsorted lists only
 
 `string_list_append`::
diff --git a/string-list.c b/string-list.c
index 72610ce..bfef6cf 100644
--- a/string-list.c
+++ b/string-list.c
@@ -92,6 +92,23 @@ struct string_list_item *string_list_lookup(struct 
string_list *list, const char
return list-items + i;
 }
 
+void string_list_remove_duplicates(struct string_list *list, int free_util)
+{
+   if (list-nr  1) {
+   int src, dst;
+   for (src = dst = 1; src  list-nr; src++) {
+   if (!strcmp(list-items[dst - 1].string, 
list-items[src].string)) {
+   if (list-strdup_strings)
+   free(list-items[src].string);
+   if (free_util)
+   free(list-items[src].util);
+   } else
+   list-items[dst++] = list-items[src];
+   }
+   list-nr = dst;
+   }
+}
+
 int for_each_string_list(struct string_list *list,
 string_list_each_func_t fn, void *cb_data)
 {
diff --git a/string-list.h b/string-list.h
index 84996aa..c4dc659 100644
--- a/string-list.h
+++ b/string-list.h
@@ -38,6 +38,7 @@ int for_each_string_list(struct string_list *list,
 void filter_string_list(struct string_list *list, int free_util,
string_list_each_func_t fn, void *cb_data);
 
+
 /* Use these functions only on sorted lists: */
 int string_list_has_string(const struct string_list *list, const char *string);
 int string_list_find_insert_index(const struct string_list *list, const char 
*string,
@@ -47,6 +48,10 @@ struct string_list_item *string_list_insert_at_index(struct 
string_list *list,
 int insert_at, const char 
*string);
 struct string_list_item *string_list_lookup(struct string_list *list, const 
char *string);
 
+/* Remove all but the first entry that has a given string value. */
+void string_list_remove_duplicates(struct string_list *list, int free_util);
+
+
 /* Use these functions only on unsorted lists: */
 struct string_list_item *string_list_append(struct string_list *list, const 
char *string);
 void sort_string_list(struct string_list *list);
-- 
1.7.11.3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] Add a new function, filter_string_list()

[Michael Haggerty]

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 Documentation/technical/api-string-list.txt |  8 
 string-list.c   | 17 +
 string-list.h   |  9 +
 3 files changed, 34 insertions(+)

diff --git a/Documentation/technical/api-string-list.txt 
b/Documentation/technical/api-string-list.txt
index 3b959a2..15b8072 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -60,6 +60,14 @@ Functions
 
 * General ones (works with sorted and unsorted lists as well)
 
+`filter_string_list`::
+
+   Apply a function to each item in a list, retaining only the
+   items for which the function returns true.  If free_util is
+   true, call free() on the util members of any items that have
+   to be deleted.  Preserve the order of the items that are
+   retained.
+
 `print_string_list`::
 
Dump a string_list to stdout, useful mainly for debugging purposes. It
diff --git a/string-list.c b/string-list.c
index 110449c..72610ce 100644
--- a/string-list.c
+++ b/string-list.c
@@ -102,6 +102,23 @@ int for_each_string_list(struct string_list *list,
return ret;
 }
 
+void filter_string_list(struct string_list *list, int free_util,
+   string_list_each_func_t fn, void *cb_data)
+{
+   int src, dst = 0;
+   for (src = 0; src  list-nr; src++) {
+   if (fn(list-items[src], cb_data)) {
+   list-items[dst++] = list-items[src];
+   } else {
+   if (list-strdup_strings)
+   free(list-items[src].string);
+   if (free_util)
+   free(list-items[src].util);
+   }
+   }
+   list-nr = dst;
+}
+
 void string_list_clear(struct string_list *list, int free_util)
 {
if (list-items) {
diff --git a/string-list.h b/string-list.h
index 7e51d03..84996aa 100644
--- a/string-list.h
+++ b/string-list.h
@@ -29,6 +29,15 @@ int for_each_string_list(struct string_list *list,
 #define for_each_string_list_item(item,list) \
for (item = (list)-items; item  (list)-items + (list)-nr; ++item)
 
+/*
+ * Apply fn to each item in list, retaining only the ones for which
+ * the function returns true.  If free_util is true, call free() on
+ * the util members of any items that have to be deleted.  Preserve
+ * the order of the items that are retained.
+ */
+void filter_string_list(struct string_list *list, int free_util,
+   string_list_each_func_t fn, void *cb_data);
+
 /* Use these functions only on sorted lists: */
 int string_list_has_string(const struct string_list *list, const char *string);
 int string_list_find_insert_index(const struct string_list *list, const char 
*string,
-- 
1.7.11.3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] Add a function string_list_longest_prefix()

[Michael Haggerty]

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 Documentation/technical/api-string-list.txt |  8 
 string-list.c   | 20 +++
 string-list.h   |  8 
 t/t0063-string-list.sh  | 30 +
 test-string-list.c  | 22 +
 5 files changed, 88 insertions(+)

diff --git a/Documentation/technical/api-string-list.txt 
b/Documentation/technical/api-string-list.txt
index 9206f8f..291ac4c 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -68,6 +68,14 @@ Functions
to be deleted.  Preserve the order of the items that are
retained.
 
+`string_list_longest_prefix`::
+
+   Return the longest string within a string_list that is a
+   prefix (in the sense of prefixcmp()) of the specified string,
+   or NULL if no such prefix exists.  This function does not
+   require the string_list to be sorted (it does a linear
+   search).
+
 `print_string_list`::
 
Dump a string_list to stdout, useful mainly for debugging purposes. It
diff --git a/string-list.c b/string-list.c
index bfef6cf..043f6c4 100644
--- a/string-list.c
+++ b/string-list.c
@@ -136,6 +136,26 @@ void filter_string_list(struct string_list *list, int 
free_util,
list-nr = dst;
 }
 
+char *string_list_longest_prefix(const struct string_list *prefixes,
+const char *string)
+{
+   int i, max_len = -1;
+   char *retval = NULL;
+
+   for (i = 0; i  prefixes-nr; i++) {
+   char *prefix = prefixes-items[i].string;
+   if (!prefixcmp(string, prefix)) {
+   int len = strlen(prefix);
+   if (len  max_len) {
+   retval = prefix;
+   max_len = len;
+   }
+   }
+   }
+
+   return retval;
+}
+
 void string_list_clear(struct string_list *list, int free_util)
 {
if (list-items) {
diff --git a/string-list.h b/string-list.h
index c4dc659..680916c 100644
--- a/string-list.h
+++ b/string-list.h
@@ -38,6 +38,14 @@ int for_each_string_list(struct string_list *list,
 void filter_string_list(struct string_list *list, int free_util,
string_list_each_func_t fn, void *cb_data);
 
+/*
+ * Return the longest string in prefixes that is a prefix (in the
+ * sense of prefixcmp()) of string, or NULL if no such prefix exists.
+ * This function does not require the string_list to be sorted (it
+ * does a linear search).
+ */
+char *string_list_longest_prefix(const struct string_list *prefixes, const 
char *string);
+
 
 /* Use these functions only on sorted lists: */
 int string_list_has_string(const struct string_list *list, const char *string);
diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
index 0eede83..fa96eba 100755
--- a/t/t0063-string-list.sh
+++ b/t/t0063-string-list.sh
@@ -15,6 +15,14 @@ string_list_split_in_place() {

 }
 
+longest_prefix() {
+   test $(test-string-list longest_prefix $1 $2) = $3
+}
+
+no_longest_prefix() {
+   test_must_fail test-string-list longest_prefix $1 $2
+}
+
 string_list_split_in_place foo:bar:baz : -1 EOF
 3
 [0]: foo
@@ -60,4 +68,26 @@ string_list_split_in_place : : -1 EOF
 [1]: 
 EOF
 
+test_expect_success test longest_prefix '
+   no_longest_prefix - '' 
+   no_longest_prefix - x 
+   longest_prefix  x  
+   longest_prefix x x x 
+   longest_prefix  foo  
+   longest_prefix : foo  
+   longest_prefix f foo f 
+   longest_prefix foo foobar foo 
+   longest_prefix foo foo foo 
+   no_longest_prefix bar foo 
+   no_longest_prefix bar:bar foo 
+   no_longest_prefix foobar foo 
+   longest_prefix foo:bar foo foo 
+   longest_prefix foo:bar bar bar 
+   longest_prefix foo::bar foo foo 
+   longest_prefix foo:foobar foo foo 
+   longest_prefix foobar:foo foo foo 
+   longest_prefix foo: bar  
+   longest_prefix :foo bar 
+'
+
 test_done
diff --git a/test-string-list.c b/test-string-list.c
index f08d3cc..c7e71f2 100644
--- a/test-string-list.c
+++ b/test-string-list.c
@@ -19,6 +19,28 @@ int main(int argc, char **argv)
return 0;
}
 
+   if (argc == 4  !strcmp(argv[1], longest_prefix)) {
+   /* arguments: colon-separated-prefixes|- string */
+   struct string_list prefixes = STRING_LIST_INIT_NODUP;
+   int retval;
+   char *prefix_string = xstrdup(argv[2]);
+   char *string = argv[3];
+   char *match;
+
+   if (strcmp(prefix_string, -))
+   string_list_split_in_place(prefixes, prefix_string, 
':', -1);
+   match = string_list_longest_prefix(prefixes, string);
+   if (match) {
+