date:20150915

On Tue, Sep 15, 2015 at 11:36 AM, Jeff King  wrote:
> We sometimes sprintf into static buffers when we know that
> the size of the buffer is large enough to fit the input
> (either because it's a constant, or because it's numeric
> input that is bounded in size). Likewise with strcpy of
> constant strings.
>
> However, these sites make it hard to audit sprintf and
> strcpy calls for buffer overflows, as a reader has to
> cross-reference the size of the array with the input. Let's
> use xsnprintf instead, which communicates to a reader that
> we don't expect this to overflow (and catches the mistake in
> case we do).
>
> Signed-off-by: Jeff King 
> ---
> diff --git a/builtin/merge-index.c b/builtin/merge-index.c
> index 1a1eafa..1d66111 100644
> --- a/builtin/merge-index.c
> +++ b/builtin/merge-index.c
> @@ -23,7 +23,7 @@ static int merge_entry(int pos, const char *path)
> break;
> found++;
> strcpy(hexbuf[stage], sha1_to_hex(ce->sha1));
> -   sprintf(ownbuf[stage], "%o", ce->ce_mode);
> +   xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o", 
> ce->ce_mode);

Interesting. I wonder if there are any (old/broken) compilers which
would barf on this. If we care, perhaps sizeof(ownbuf[0]) instead?

> arguments[stage] = hexbuf[stage];
> arguments[stage + 4] = ownbuf[stage];
> } while (++pos < active_nr);
--
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 26/67] replace trivial malloc + sprintf /strcpy calls to xstrfmt

On Tue, Sep 15, 2015 at 11:45 AM, Jeff King  wrote:
> replace trivial malloc + sprintf /strcpy calls to xstrfmt

s/to/with/

Also, do you want either to add a space after '/' or drop the one before it?

> It's a common pattern to do:
>
>   foo = xmalloc(strlen(one) + strlen(two) + 1 + 1);
>   sprintf(foo, "%s %s", one, two);
>
> (or possibly some variant with strcpy()s or a more
> complicated length computation).  We can switch these to use
> xstrfmt, which is shorter, involves less error-prone manual
> computation, and removes many sprintf and strcpy calls which
> make it harder to audit the code for real buffer overflows.
>
> Signed-off-by: Jeff King 
> ---
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -889,9 +889,8 @@ static char *cram(const char *challenge_64, const char 
> *user, const char *pass)
> }
>
> /* response: " " */
> -   resp_len = strlen(user) + 1 + strlen(hex) + 1;
> -   response = xmalloc(resp_len);
> -   sprintf(response, "%s %s", user, hex);
> +   response = xstrfmt("%s %s", user, hex);
> +   resp_len = strlen(response);
>
> response_64 = xmalloc(ENCODED_SIZE(resp_len) + 1);

The original resp_len calculation included the NUL but the revised
does not. If I'm reading this correctly, the revised calculation is
correct, and the original was over-allocating response_64, right?

> encoded_len = EVP_EncodeBlock((unsigned char *)response_64,
--
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] strtoul_ui: actually report error in case of negative input

2015-09-15 Thread Max Kirillov

On Tue, Sep 15, 2015 at 08:50:03AM +0200, Matthieu Moy wrote:
> I think it would be better to just return a long to avoid needless
> limitations, but changing the argument to "long" would interfer with
> in-flight topics. Not worth the trouble.

Sure.

> 
> One potential issue with your patch is that you're forbidding the
> interval [2^31, 2^32[ which was previously allowed, both on 32 and 64
> bits. I'm not sure whether we have a use for this in the codebase.

As far as I could see it was used only for file modes. Which
does not need that big numbers.

> This alternative patch is rather ugly to, but I think it is less
> limiting and does not have the "large negative wrapped to positive"
> issue:
> 
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -814,6 +814,9 @@ static inline int strtoul_ui(char const *s, int base, 
> unsigned int *result)
> char *p;
>  
> errno = 0;
> +   /* negative values would be accepted by strtoul */
> +   if (strchr(s, '-'))
> +   return -1;
> ul = strtoul(s, , base);
> if (errno || *p || p == s || (unsigned int) ul != ul)
> return -1;
> 
> What do you think?

Explicit rejection of '-' is of course useful addition.

I still find "(unsigned int) ul != ul" bad. As far as I
understand it makes no sense for i386. And even for 64-bit
it's too obscure. In form of "(ul & 0xL) == 0" it
would be more clear. Or just make explicit comparison with
intended limit, like I did.

Well, actually I don't have strong preferences as long as
"make -C t" does not alarm me with things I did not break.
Maybe somebody else will comment more.

-- 
Max
--
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 29/67] use strip_suffix and xstrfmt to replace suffix

On Tue, Sep 15, 2015 at 11:47 AM, Jeff King  wrote:
> When we want to convert "foo.pack" to "foo.idx", we do it by
> duplicating the original string and then munging the bytes
> in place. Let's use strip_suffix and xstrfmt instead, which
> has several advantages:
>
>   1. It's more clear what the intent is.
>
>   2. It does not implicitly rely on the fact that
>  strlen(".idx") <= strlen(".pack") to avoid an overflow.
>
>   3. We communicate the assumption that the input file ends
>  with ".pack" (and get a run-time check that this is so).
>
>   4. We drop calls to strcpy, which makes auditing the code
>  base easier.
>
> Likewise, we can do this to convert ".pack" to ".bitmap",
> avoiding some manual memory computation.
>
> Signed-off-by: Jeff King 
> ---
> diff --git a/http.c b/http.c
> index 7b02259..e0ff876 100644
> --- a/http.c
> +++ b/http.c
> @@ -1511,6 +1511,7 @@ int finish_http_pack_request(struct http_pack_request 
> *preq)
> struct packed_git **lst;
> struct packed_git *p = preq->target;
> char *tmp_idx;
> +   size_t len;
> struct child_process ip = CHILD_PROCESS_INIT;
> const char *ip_argv[8];
>
> @@ -1524,9 +1525,9 @@ int finish_http_pack_request(struct http_pack_request 
> *preq)
> lst = &((*lst)->next);
> *lst = (*lst)->next;
>
> -   tmp_idx = xstrdup(preq->tmpfile);
> -   strcpy(tmp_idx + strlen(tmp_idx) - strlen(".pack.temp"),
> -  ".idx.temp");
> +   if (!strip_suffix(preq->tmpfile, ".pack.temp", ))
> +   die("BUG: pack tmpfile does not end in .pack.temp?");
> +   tmp_idx = xstrfmt("%.*s.idx.temp", (int)len, preq->tmpfile);

These instances of repeated replacement code may argue in favor of a
general purpose replace_suffix() function:

char *replace_suffix(const char *s, const char *old, const char *new)
{
size_t n;
if (!strip_suffix(s, old, ))
die("BUG: '%s' does not end with '%s', s, old);
return xstrfmt("%.*s%s", (int)n, s, new);
}

or something.

> ip_argv[0] = "index-pack";
> ip_argv[1] = "-o";
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 637770a..7dfcb34 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -252,16 +252,11 @@ static int load_bitmap_entries_v1(struct bitmap_index 
> *index)
>
>  static char *pack_bitmap_filename(struct packed_git *p)
>  {
> -   char *idx_name;
> -   int len;
> -
> -   len = strlen(p->pack_name) - strlen(".pack");
> -   idx_name = xmalloc(len + strlen(".bitmap") + 1);
> -
> -   memcpy(idx_name, p->pack_name, len);
> -   memcpy(idx_name + len, ".bitmap", strlen(".bitmap") + 1);
> +   size_t len;
>
> -   return idx_name;
> +   if (!strip_suffix(p->pack_name, ".pack", ))
> +   die("BUG: pack_name does not end in .pack");
> +   return xstrfmt("%.*s.bitmap", (int)len, p->pack_name);
>  }
>
>  static int open_pack_bitmap_1(struct packed_git *packfile)
> diff --git a/sha1_file.c b/sha1_file.c
> index 28352a5..88996f0 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -671,13 +671,15 @@ static int check_packed_git_idx(const char *path, 
> struct packed_git *p)
>  int open_pack_index(struct packed_git *p)
>  {
> char *idx_name;
> +   size_t len;
> int ret;
>
> if (p->index_data)
> return 0;
>
> -   idx_name = xstrdup(p->pack_name);
> -   strcpy(idx_name + strlen(idx_name) - strlen(".pack"), ".idx");
> +   if (!strip_suffix(p->pack_name, ".pack", ))
> +   die("BUG: pack_name does not end in .pack");
> +   idx_name = xstrfmt("%.*s.idx", (int)len, p->pack_name);
> ret = check_packed_git_idx(idx_name, p);
> free(idx_name);
> return ret;
> --
> 2.6.0.rc2.408.ga2926b9
--
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 15/67] convert trivial sprintf / strcpy calls to xsnprintf

Ramsay Jones  writes:

> How about using strlcpy() instead? Thus:
>
> - strcpy(header.name, "pax_global_header");
> + strlcpy(header.name, "pax_global_header", sizeof(header.name));
>
> Ditto for other similar (strcpy->xsnprintf) hunks below.

Please do not advocate use of strlcpy(), which substitutes
overwriting beyond the end of the buffer (which is a bug) with a
silent truncation (which is almost always another bug, unless in a
very narrow case of producing a non-essential string result where
loss of information does not matter).

--
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 08/67] add reentrant variants of sha1_to_hex and find_unique_abbrev

Jeff King  writes:

>> Hmm, I haven't read any other patches yet (including those which use these
>> new '_to' functions), but I can't help feeling they should be named something
>> like 'sha1_to_hex_str()' and 'find_unique_abbrev_str()' instead.  i.e. I 
>> don't get
>> the '_to' thing - not that I'm any good at naming things ...
>
> I meant it as a contrast with their original. sha1_to_hex() formats into
> an internal buffer and returns it. But sha1_to_hex_to() formats "to" a
> buffer of your choice.

I think that naming makes perfect sense.
--
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 07/67] strbuf: make strbuf_complete_line more generic

Eric Sunshine  writes:

>> +static inline void strbuf_complete(struct strbuf *sb, char term)
>> +{
>> +   if (sb->len && sb->buf[sb->len - 1] != term)
>> +   strbuf_addch(sb, term);
>> +}
>
> Hmm, so this only adds 'term' if not already present *and* if 'sb' is
> not empty, which doesn't seem to match the documentation which says
> that it "ensures" termination.
>
> But, is that reasonable behavior? Intuitively, I'd expect 'term' to be
> added when 'sb' is empty:
>
> if (!sb->len || sb->buf[sb->len - 1] != term)
> strbuf_addch(sb, term);
>
> strbuf_complete_line()'s existing behavior of not adding '\n' to an
> empty string may have been intentional, but actually smells like a
> bug.

I would expect two different scenarios for which this function would
be useful.

One is when dealing with a text file and want to avoid incomplete
lines at the end.  In this scenario, an empty file with zero lines
should be left as-is, instead of getting turned into a file with one
empty line.  "Leave the empty input as-is" is the behaviour the
callers want.

The other is when you are given a directory name in the strbuf, you
have a name of a file you want to be in that directory, and want to
have the full path to the file in the strbuf.  In this scenario,
what does it mean for the caller to give you an empty "directory
name"?  I think at least in our codebase, that almost always would
mean that "the path is relative to $CWD", i.e. you would want to see
the "complete" to leave the input intact and then append the
filename there.

So to these two plausible and different set of callers that would be
helped by this function, the behaviour Peff gives it would match
what the callers want better than your version.
--
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 0/67] war on sprintf, strcpy, etc

Jeff King  writes:

> Obviously this is not intended for v2.6.0. But all of the spots touched
> here are relatively quiet right now, so I wanted to get it out onto the
> list.  There are a few minor conflicts against "pu", but they're all
> just from touching nearby lines.

Thanks.  Looks like a lot of good work you did ;-)
--
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] strtoul_ui: actually report error in case of negative input

Matthieu Moy  writes:

> Not just the return type (which is the error status), but also the type
> of the result argument indeed. It's not clear to me whether this is
> intentional (09f2825 (git-grep: don't use sscanf, 2007-03-12) introduced
> it, the commit message doesn't help). I first read strtoul_ui as
> "strtoul with a better UI (user interface)", but maybe the name was
> meant to say "a fuction that uses strtoul and returns an ui (unsigned
> int)".

Just for this part.  Yes, ui does not mean user interface but "we
are grabbing an unsigned int and as its internal implementation we
happen to use strtoul" is where the name comes from.

> I went through the thread quickly, my understanding is that there were
> more work to do, but no objection to merging.

Yes, there were some in-flight topics that interfered with it and
the topic quickly went stale without getting rerolled.  There wasn't
any fundamental issue with the topic itself.
--
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: "Medium" log format: change proposal for author != committer

"Robin H. Johnson"  writes:

> Specifically, if the author is NOT the same as the committer, then
> display both in the header. Otherwise continue to display only the
> author.

I too found myself wanting to see both of the names sometimes, and
the "fuller" format was added explicitly for that purpose.

Even though I agree "show only one, and both only when they are
different" is a reasonable and possibly useful format, it is out of
question to change what "--pretty=medium" does.  It has been with us
forever and people and their scripts do rely on it.

It would be good if we can say

$ git log --pretty=robinsformat

but with a better name to show such an output.

Having said that, I'm moderately negative about adding it as yet
another hard-coded format.  We simply have too many, and we do not
need one more.  What we need instead is a flexible framework to let
users get what they want.

I think what needs to happen is:

 * Enhance the "--pretty=format:" thing so that the current set of
   hardcoded --pretty=medium,short,... formats and your modified
   "medium" can be expressed as a custom format string.

 * Introduce a configuration mechanism to allow users to define new
   short-hand, e.g. if you have this in your $HOME/.gitconfig:

[pretty "robin"]
format = "commit %H%nAuthor: %an <%ae>%n..."

   and run "git log --pretty=robin", it would behave as if you said
   "git log --pretty="format:commit %H%nAuthor: %an <%ae>%n...".

 * (optional) Replace the hardcoded implementations of pretty
   formats with short-hand names like "medium", "short", etc. with a
   built-in set of pretty.$name.format using the configuration
   mechanism.  But we need to make sure this does not hurt
   performance for common cases.

--
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 v5] remote: add get-url subcommand

2015-09-15 Thread Ben Boeckel

Expanding `insteadOf` is a part of ls-remote --url and there is no way
to expand `pushInsteadOf` as well. Add a get-url subcommand to be able
to query both as well as a way to get all configured urls.

Signed-off-by: Ben Boeckel 
---
 Documentation/git-remote.txt | 10 
 builtin/remote.c | 59 
 t/t5505-remote.sh| 39 +
 3 files changed, 108 insertions(+)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 4c6d6de..3c9bf45 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -15,6 +15,7 @@ SYNOPSIS
 'git remote remove' 
 'git remote set-head'  (-a | --auto | -d | --delete | )
 'git remote set-branches' [--add]  ...
+'git remote get-url' [--push] [--all] 
 'git remote set-url' [--push]   []
 'git remote set-url --add' [--push]  
 'git remote set-url --delete' [--push]  
@@ -131,6 +132,15 @@ The named branches will be interpreted as if specified 
with the
 With `--add`, instead of replacing the list of currently tracked
 branches, adds to that list.
 
+'get-url'::
+
+Retrieves the URLs for a remote. Configurations for `insteadOf` and
+`pushInsteadOf` are expanded here. By default, only the first URL is listed.
++
+With '--push', push URLs are queried rather than fetch URLs.
++
+With '--all', all URLs for the remote will be listed.
+
 'set-url'::
 
 Changes URLs for the remote. Sets first URL for remote  that matches
diff --git a/builtin/remote.c b/builtin/remote.c
index 181668d..e4c3ea1 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -18,6 +18,7 @@ static const char * const builtin_remote_usage[] = {
N_("git remote prune [-n | --dry-run] "),
N_("git remote [-v | --verbose] update [-p | --prune] [( | 
)...]"),
N_("git remote set-branches [--add]  ..."),
+   N_("git remote get-url [--push] [--all] "),
N_("git remote set-url [--push]   []"),
N_("git remote set-url --add  "),
N_("git remote set-url --delete  "),
@@ -65,6 +66,11 @@ static const char * const builtin_remote_update_usage[] = {
NULL
 };
 
+static const char * const builtin_remote_geturl_usage[] = {
+   N_("git remote get-url [--push] [--all] "),
+   NULL
+};
+
 static const char * const builtin_remote_seturl_usage[] = {
N_("git remote set-url [--push]   []"),
N_("git remote set-url --add  "),
@@ -1467,6 +1473,57 @@ static int set_branches(int argc, const char **argv)
return set_remote_branches(argv[0], argv + 1, add_mode);
 }
 
+static int get_url(int argc, const char **argv)
+{
+   int i, push_mode = 0, all_mode = 0;
+   const char *remotename = NULL;
+   struct remote *remote;
+   const char **url;
+   int url_nr;
+   struct option options[] = {
+   OPT_BOOL('\0', "push", _mode,
+N_("query push URLs rather than fetch URLs")),
+   OPT_BOOL('\0', "all", _mode,
+N_("return all URLs")),
+   OPT_END()
+   };
+   argc = parse_options(argc, argv, NULL, options, 
builtin_remote_geturl_usage, 0);
+
+   if (argc != 1)
+   usage_with_options(builtin_remote_geturl_usage, options);
+
+   remotename = argv[0];
+
+   if (!remote_is_configured(remotename))
+   die(_("No such remote '%s'"), remotename);
+   remote = remote_get(remotename);
+
+   url_nr = 0;
+   if (push_mode) {
+   url = remote->pushurl;
+   url_nr = remote->pushurl_nr;
+   }
+   /* else fetch mode */
+
+   /* Use the fetch URL when no push URLs were found or requested. */
+   if (!url_nr) {
+   url = remote->url;
+   url_nr = remote->url_nr;
+   }
+
+   if (!url_nr)
+   die(_("no URLs configured for remote '%s'"), remotename);
+
+   if (all_mode) {
+   for (i = 0; i < url_nr; i++)
+   printf_ln("%s", url[i]);
+   } else {
+   printf_ln("%s", *url);
+   }
+
+   return 0;
+}
+
 static int set_url(int argc, const char **argv)
 {
int i, push_mode = 0, add_mode = 0, delete_mode = 0;
@@ -1576,6 +1633,8 @@ int cmd_remote(int argc, const char **argv, const char 
*prefix)
result = set_head(argc, argv);
else if (!strcmp(argv[0], "set-branches"))
result = set_branches(argc, argv);
+   else if (!strcmp(argv[0], "get-url"))
+   result = get_url(argc, argv);
else if (!strcmp(argv[0], "set-url"))
result = set_url(argc, argv);
else if (!strcmp(argv[0], "show"))
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 7a8499c..f03ba4c 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -919,6 +919,21 @@ test_expect_success 'new remote' '
cmp expect actual
 '
 
+get_url_test () {
+   cat >expect &&
+   test_expect_success

Re: [PATCH 07/67] strbuf: make strbuf_complete_line more generic

On Tue, Sep 15, 2015 at 11:25 AM, Jeff King  wrote:
> The strbuf_complete_line function make sure that a buffer

s/make/makes/

> ends in a newline. But we may want to do this for any
> character (e.g., "/" on the end of a path). Let's factor out
> a generic version, and keep strbuf_complete_line as a thin
> wrapper.
>
> Signed-off-by: Jeff King 
> ---
> +/**
> + * Ensure that `sb` ends with the character `term`, if it does not
> + * already.
> + */
> +static inline void strbuf_complete(struct strbuf *sb, char term)
> +{
> +   if (sb->len && sb->buf[sb->len - 1] != term)
> +   strbuf_addch(sb, term);
> +}

Hmm, so this only adds 'term' if not already present *and* if 'sb' is
not empty, which doesn't seem to match the documentation which says
that it "ensures" termination.

But, is that reasonable behavior? Intuitively, I'd expect 'term' to be
added when 'sb' is empty:

if (!sb->len || sb->buf[sb->len - 1] != term)
strbuf_addch(sb, term);

strbuf_complete_line()'s existing behavior of not adding '\n' to an
empty string may have been intentional, but actually smells like a
bug.

> +
> +/**
> + * Ensure that `sb` ends with a newline.
> + */
>  static inline void strbuf_complete_line(struct strbuf *sb)
>  {
> -   if (sb->len && sb->buf[sb->len - 1] != '\n')
> -   strbuf_addch(sb, '\n');
> +   strbuf_complete(sb, '\n');
>  }
--
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 10/67] mailsplit: make PATH_MAX buffers dynamic

On Tue, Sep 15, 2015 at 11:28 AM, Jeff King  wrote:
> There are several static PATH_MAX-sized buffers in
> mailsplit, along with some questionable uses of sprintf.
> These are not really of security interest, as local
> mailsplit pathnames are not typically under control of an
> attacker.  But it does not hurt to be careful, and as a
> bonus we lift some limits for systems with too-small
> PATH_MAX varibles.
>
> Signed-off-by: Jeff King 
> ---
> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
> index 9de06e3..fb0bc08 100644
> --- a/builtin/mailsplit.c
> +++ b/builtin/mailsplit.c
> @@ -148,8 +155,7 @@ static int maildir_filename_cmp(const char *a, const char 
> *b)
>  static int split_maildir(const char *maildir, const char *dir,
> int nr_prec, int skip)
>  {
> -   char file[PATH_MAX];
> -   char name[PATH_MAX];
> +   struct strbuf file = STRBUF_INIT;
> FILE *f = NULL;
> int ret = -1;
> int i;
> @@ -161,20 +167,25 @@ static int split_maildir(const char *maildir, const 
> char *dir,
> goto out;
>
> for (i = 0; i < list.nr; i++) {
> -   snprintf(file, sizeof(file), "%s/%s", maildir, 
> list.items[i].string);
> -   f = fopen(file, "r");
> +   char *name;
> +
> +   strbuf_reset();
> +   strbuf_addf(, "%s/%s", maildir, list.items[i].string);
> +
> +   f = fopen(file.buf, "r");
> if (!f) {
> -   error("cannot open mail %s (%s)", file, 
> strerror(errno));
> +   error("cannot open mail %s (%s)", file.buf, 
> strerror(errno));
> goto out;
> }
>
> if (strbuf_getwholeline(, f, '\n')) {
> -   error("cannot read mail %s (%s)", file, 
> strerror(errno));
> +   error("cannot read mail %s (%s)", file.buf, 
> strerror(errno));
> goto out;
> }
>
> -   sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
> +   name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
> split_one(f, name, 1);
> +   free(name);

Hmm, why does 'file' become a strbuf which is re-used each time
through the loop, but 'name' is treated differently and gets
re-allocated upon each iteration? Why doesn't 'name' deserve the same
treatment as 'file'?

> fclose(f);
> f = NULL;
> @@ -184,6 +195,7 @@ static int split_maildir(const char *maildir, const char 
> *dir,
>  out:
> if (f)
> fclose(f);
> +   strbuf_release();
> string_list_clear(, 1);
> return ret;
>  }
> @@ -191,7 +203,6 @@ out:
>  static int split_mbox(const char *file, const char *dir, int allow_bare,
>   int nr_prec, int skip)
>  {
> -   char name[PATH_MAX];
> int ret = -1;
> int peek;
>
> @@ -218,8 +229,9 @@ static int split_mbox(const char *file, const char *dir, 
> int allow_bare,
> }
>
> while (!file_done) {
> -   sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
> +   char *name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
> file_done = split_one(f, name, allow_bare);
> +   free(name);

Same question, pretty much: Why not make 'name' a strbuf which is
re-used in the loop? (I don't have a strong preference; I'm just
trying to understand the apparent inconsistency of treatment.)

> }
>
> if (f != stdin)
> --
> 2.6.0.rc2.408.ga2926b9
--
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 11/67] trace: use strbuf for quote_crnl output

On Tue, Sep 15, 2015 at 11:28 AM, Jeff King  wrote:
> When we output GIT_TRACE_SETUP paths, we quote any
> meta-characters. But our buffer to hold the result is only
> PATH_MAX bytes, and we could double the size of the input
> path (if every character needs quoted). We could use a

s/quoted/to be &/ ...or... s/quoted/quoting/

> 2*PATH_MAX buffer, if we assume the input will never be more
> than PATH_MAX. But it's easier still to just switch to a
> strbuf and not worry about whether the input can exceed
> PATH_MAX or not.
>
> Signed-off-by: Jeff King 
> ---
> diff --git a/trace.c b/trace.c
> index 7393926..0c06d71 100644
> --- a/trace.c
> +++ b/trace.c
> @@ -277,25 +277,25 @@ void trace_performance_fl(const char *file, int line, 
> uint64_t nanos,
>
>  static const char *quote_crnl(const char *path)
>  {
> -   static char new_path[PATH_MAX];
> +   static struct strbuf new_path = STRBUF_INIT;
> const char *p2 = path;
> -   char *p1 = new_path;

It's a little sad that this leaves a variable named 'p2' when there is
no corresponding 'p1'. Would this deserve a cleanup patch which
renames 'p2' to 'p' or do we not care enough?

> if (!path)
> return NULL;
>
> +   strbuf_reset(_path);
> +
> while (*p2) {
> switch (*p2) {
> -   case '\\': *p1++ = '\\'; *p1++ = '\\'; break;
> -   case '\n': *p1++ = '\\'; *p1++ = 'n'; break;
> -   case '\r': *p1++ = '\\'; *p1++ = 'r'; break;
> +   case '\\': strbuf_addstr(_path, ""); break;
> +   case '\n': strbuf_addstr(_path, "\\n"); break;
> +   case '\r': strbuf_addstr(_path, "\\r"); break;
> default:
> -   *p1++ = *p2;
> +   strbuf_addch(_path, *p2);
> }
> p2++;
> }
> -   *p1 = '\0';
> -   return new_path;
> +   return new_path.buf;
>  }
>
>  /* FIXME: move prefix to startup_info struct and get rid of this arg */
> --
> 2.6.0.rc2.408.ga2926b9
--
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: [Feature Request] git blame showing only revisions from git rev-list --first-parent

Jeff King  writes:

> It seems like nobody is actually that interested in what "blame
> --first-parent --reverse" does in the first place, though, and there's
> no reason for its complexity to hold up vanilla --first-parent. So what
> do you think of:

I like the part that explicitly disables the combination of the two
;-)

--
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

"Medium" log format: change proposal for author != committer

2015-09-15 Thread Robin H. Johnson

Hi,

I want to propose a change to the 'medium' log output format, to improve
readability.

Specifically, if the author is NOT the same as the committer, then
display both in the header. Otherwise continue to display only the
author.

This would aid quick review of changes in git-log & git-show output.

-- 
Robin Hugh Johnson
Gentoo Linux: Developer, Infrastructure Lead
E-Mail : robb...@gentoo.org
GnuPG FP   : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
--
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 2/2] git-p4: handle "Translation of file content failed"

On 15/09/15 16:38, Lars Schneider wrote:

On 15 Sep 2015, at 08:43, Luke Diamand wrote:

Do we know the mechanism by which we end up in this state?

Unfortunately no. I tried hard to reproduce the error with “conventional”
methods. As you can see I ended up manipulating the P4 database…

However, it looks like this error happens in the wild, too:
https://stackoverflow.com/questions/5156909/translation-of-file-content-failed-error-in-perforce
https://stackoverflow.com/questions/887006/perforce-translation-of-file-content-failed-error

It's described in the Perforce FAQ here:

http://answers.perforce.com/articles/KB/3117

i.e. it looks to be caused by mixing old and new P4 clients.

Known issue: This works only if git-p4 is executed in verbose mode.
In normal mode no exceptions are thrown and git-p4 just exits.

Does that mean that the error will only be detected in verbose mode? That
doesn't seem right!

Correct. I don’t like this either but I also don’t want to make huge changes to
git-p4.
You can see the root problem here:
https://github.com/git/git/blob/97d7ad75b6fe74960d2a12e4a9151a55a5a87d6d/git-p4.py#L110-L114

Any idea how to approach that best?

I guess what we have is not ideal but probably good enough.

+try:
+text = p4_read_pipe(['print', '-q', '-o', '-', '%s@%s' %
(file['depotFile'], file['change'])])
+except Exception as e:

Would it be better to specify which kind of Exception you are catching? Looks
like you could get OSError, ValueError and CalledProcessError; it's the last of
these you want (I think).

I think it is just a plain exception. See here:
https://github.com/git/git/blob/97d7ad75b6fe74960d2a12e4a9151a55a5a87d6d/git-p4.py#L111

OK, you're right (probably less than ideal behaviour from read_pipe()
and die() but let's not try to fix that).

+if p4_version_string().find('/NT') >= 0:
+text = text.replace('\r\n', '\n')
+contents = [ text ]

The indentation on this bit doesn't look right to me.

I believe it is exactly how it was:
https://github.com/git/git/blob/97d7ad75b6fe74960d2a12e4a9151a55a5a87d6d/git-p4.py#L2397-L2399

OK.

In general, what is the appropriate way to reference code in this email list?
Are GitHub links OK?

I'm not an expert, but it feels possibly a bit ephemeral - if someone is
digging through email archives in a future where that github project has
been moved elsewhere, the links will all be dead.

Luke
--
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: Git Deployment using existing multiple environments

2015-09-15 Thread Johannes Schindelin

Hi,

please stop top-posting. It is quite irritating by now. Thank you.

On 2015-09-15 08:50, Sukhwinder Singh wrote:

> Now lets say we set up a repository at github which has the latest
> code (all test code)., Now at each of our own servers we already have
> existing code, that is Test, UAT and Live. For example, first he'll
> pull code from github to our Test Server and then move branches to UAT
> and then to Live. Can it work?

Yes, it can work.

The major problem here will be that you have only a single, central repository 
that every developer has write access to, but you expect only a single trusted 
person to deploy that code via three different stages. That can be tricky, in 
particular if your trusted person is a newbie.

> If it can work then can I please get some some example commands or the 
> procedure to set it up?

Sorry, this feels a bit too much like "could you please do all my work for me?" 
to me. And if I really provide exact commands here, I may even be liable when 
it does not work out in the end? I am not going to do that.

Instead, I will strongly suggest to learn enough about Git to answer those 
quite simple questions ("How do I pull?", "What is a pull?", "How do I update a 
test machine's working directory with the newest branch tip?") yourself. I 
usually recommend https://try.github.io/

> Time is a bit of problem right now or I would have read book suggested by 
> Johannes. I have searched on the internet but couldn't find any similar case.

If I was in your shoes, I would spend the time *now*, rather than quite 
possibly spending 10x as much time later when I have to clean up a mess. In my 
experience, 
what looks like the cheap route (copying commands without understanding them, 
really), invariably turns out to be the most expensive route possible.

In any case, this was as much useful feedback as I had to give to your 
questions.

Ciao,
Johannes
--
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: What's cooking in git.git (Sep 2015, #03; Mon, 14)

2015-09-15 Thread Philip Oakley


From: "Junio C Hamano" 
Sent: Monday, September 14, 2015 11:43 PM

--



[New Topics]



* po/doc-branch-desc (2015-09-14) 1 commit
 (merged to 'next' on 2015-09-14 at 4934a96)
+ doc: show usage of branch description

The branch descriptions that are set with "git 
branch --edit-description"

option were used in many places but they weren't clearly documented.

Will merge to 'master'.


Thanks.
Shall I just rework/resend the V2 patch-up ($gmane/277829) that also 
links to 'merge's' usage as a fresh patch (would be tonight UK)?


--
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 2/2] git-p4: handle "Translation of file content failed"


On 14/09/15 17:55, larsxschnei...@gmail.com wrote:

From: Lars Schneider 

A P4 repository can get into a state where it contains a file with
type UTF-16 that does not contain a valid UTF-16 BOM. If git-p4


Sorry - what's a BOM? I'm assuming it's not a Bill of Materials?

Do we know the mechanism by which we end up in this state?




attempts to retrieve the file then the process crashes with a
"Translation of file content failed" error.

Fix this by detecting this error and retrieving the file as binary
instead. The result in Git is the same.

Known issue: This works only if git-p4 is executed in verbose mode.
In normal mode no exceptions are thrown and git-p4 just exits.


Does that mean that the error will only be detected in verbose mode? 
That doesn't seem right!




Signed-off-by: Lars Schneider 
---
  git-p4.py | 27 ---
  1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 073f87b..5ae25a6 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -134,13 +134,11 @@ def read_pipe(c, ignore_error=False):
  sys.stderr.write('Reading pipe: %s\n' % str(c))

  expand = isinstance(c,basestring)
-p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
-pipe = p.stdout
-val = pipe.read()
-if p.wait() and not ignore_error:
-die('Command failed: %s' % str(c))
-
-return val
+p = subprocess.Popen(c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, 
shell=expand)
+(out, err) = p.communicate()
+if p.returncode != 0 and not ignore_error:
+die('Command failed: %s\nError: %s' % (str(c), err))
+return out

  def p4_read_pipe(c, ignore_error=False):
  real_cmd = p4_build_cmd(c)
@@ -2186,10 +2184,17 @@ class P4Sync(Command, P4UserMap):
  # them back too.  This is not needed to the cygwin windows 
version,
  # just the native "NT" type.
  #
-text = p4_read_pipe(['print', '-q', '-o', '-', "%s@%s" % 
(file['depotFile'], file['change']) ])
-if p4_version_string().find("/NT") >= 0:
-text = text.replace("\r\n", "\n")
-contents = [ text ]
+try:
+text = p4_read_pipe(['print', '-q', '-o', '-', '%s@%s' % 
(file['depotFile'], file['change'])])
+except Exception as e:


Would it be better to specify which kind of Exception you are catching? 
Looks like you could get OSError, ValueError and CalledProcessError; 
it's the last of these you want (I think).



+if 'Translation of file content failed' in str(e):
+type_base = 'binary'
+else:
+raise e
+else:
+if p4_version_string().find('/NT') >= 0:
+text = text.replace('\r\n', '\n')
+contents = [ text ]


The indentation on this bit doesn't look right to me.



  if type_base == "apple":
  # Apple filetype files will be streamed as a concatenation of



Luke

--
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 v5 1/7] git-p4: add optional type specifier to gitConfig reader


On 14/09/15 14:26, larsxschnei...@gmail.com wrote:

From: Lars Schneider 

The functions “gitConfig” and “gitConfigBool” are almost identical. Make 
“gitConfig” more generic by adding an optional type specifier. Use the type 
specifier “—bool” with “gitConfig” to implement “gitConfigBool. This prepares 
the implementation of other type specifiers such as “—int”.


Looks good to me, Ack.




Signed-off-by: Lars Schneider 
---
  git-p4.py | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 073f87b..c139cab 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -604,9 +604,12 @@ def gitBranchExists(branch):

  _gitConfig = {}

-def gitConfig(key):
+def gitConfig(key, typeSpecifier=None):
  if not _gitConfig.has_key(key):
-cmd = [ "git", "config", key ]
+cmd = [ "git", "config" ]
+if typeSpecifier:
+cmd += [ typeSpecifier ]
+cmd += [ key ]
  s = read_pipe(cmd, ignore_error=True)
  _gitConfig[key] = s.strip()
  return _gitConfig[key]
@@ -617,10 +620,7 @@ def gitConfigBool(key):
 in the config."""

  if not _gitConfig.has_key(key):
-cmd = [ "git", "config", "--bool", key ]
-s = read_pipe(cmd, ignore_error=True)
-v = s.strip()
-_gitConfig[key] = v == "true"
+_gitConfig[key] = gitConfig(key, '--bool') == "true"
  return _gitConfig[key]

  def gitConfigList(key):



--
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 v7] git-p4: improve path encoding verbose output


On 15/09/15 08:31, Luke Diamand wrote:

On 14/09/15 18:10, larsxschnei...@gmail.com wrote:

It would be better to query this once at startup. Otherwise we're
potentially forking "git config" twice per file which on a large repo
could become significant. Make it an instance variable perhaps?


This is of course complete nonsense since gitConfig caches its results!


--
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 v7] git-p4: improve path encoding verbose output