[PATCH] git format-patch --signature

[Jeremiah Mahler]

Improved format-patch --signature option so that it can
read from a file as well as from a string.

  # from a string
  $ git format-patch --signature "from a string" origin

  # or from a file
  $ git format-patch --signature ~/.signature origin

Now signatures with newlines or other special characters
can be easily included.

Signed-off-by: Jeremiah Mahler 
---
 builtin/log.c | 26 --
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..5988f8f 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1147,6 +1147,27 @@ static int from_callback(const struct option *opt, const 
char *arg, int unset)
return 0;
 }
 
+static int signature_callback(const struct option *opt, const char *arg,
+   int unset)
+{
+   const char **signature = opt->value;
+   static char buf[1024];
+   size_t sz;
+   FILE *fp;
+
+   fp = fopen(arg, "r");
+   if (fp) {
+   sz = sizeof(buf);
+   sz = fread(buf, 1, sz - 1, fp);
+   buf[sz] = '\0';
+   *signature = buf;
+   fclose(fp);
+   } else {
+   *signature = arg;
+   }
+   return 0;
+}
+
 int cmd_format_patch(int argc, const char **argv, const char *prefix)
 {
struct commit *commit;
@@ -1228,8 +1249,9 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
{ OPTION_CALLBACK, 0, "thread", &thread, N_("style"),
N_("enable message threading, styles: shallow, 
deep"),
PARSE_OPT_OPTARG, thread_callback },
-   OPT_STRING(0, "signature", &signature, N_("signature"),
-   N_("add a signature")),
+   { OPTION_CALLBACK, 0, "signature", &signature, 
N_("signature-file"),
+   N_("add a signature from a string or a file"),
+   PARSE_OPT_NONEG, signature_callback },
OPT__QUIET(&quiet, N_("don't print the patch filenames")),
OPT_END()
};
-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler


--
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] git format-patch --signature

[Jeremiah Mahler]

On Tue, May 13, 2014 at 09:07:12AM -0700, Jonathan Nieder wrote:
> Hi,
> 
> Jeremiah Mahler wrote:
> 
> >   # from a string
> >   $ git format-patch --signature "from a string" origin
> >
> >   # or from a file
> >   $ git format-patch --signature ~/.signature origin
> 
> Interesting.  But... what if I want my patch to end with
> 
>   -- 
>   /home/jrnieder/.signature
> 
> ?  It seems safer to introduce a separate --signature-file option.
> 

It is probably smarter to avoid that corner case entirely.
Good idea.

> [...]
> >  builtin/log.c | 26 --
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> Tests?
> 

I added a test which checks that a valid patch is produced and that
the signature from the file appears in the output.

> Thanks and hope that helps,
> Jonathan

Attached is a revised patch.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
>From e5cbeaf50d85236d6dd53e64f8f7cf466b1acecd Mon Sep 17 00:00:00 2001
From: Jeremiah Mahler 
Date: Tue, 13 May 2014 18:10:53 -0700
Subject: [PATCH] format-patch --signature-file 

Added feature that allows a signature file to be used with format-patch.

  $ git format-patch --signature-file ~/.signature origin

Now signatures with newlines and other special characters can
be easily included.

Signed-off-by: Jeremiah Mahler 
---
 builtin/log.c   | 24 
 t/t4014-format-patch.sh | 13 +
 2 files changed, 37 insertions(+)

diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..1ec733b 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1147,6 +1147,27 @@ static int from_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+static int signature_file_callback(const struct option *opt, const char *arg,
+			int unset)
+{
+	const char **signature = opt->value;
+	static char buf[1024];
+	size_t sz;
+	FILE *fp;
+
+	fp = fopen(arg, "r");
+	if (fp) {
+		sz = sizeof(buf);
+		sz = fread(buf, 1, sz - 1, fp);
+		buf[sz] = '\0';
+		*signature = buf;
+		fclose(fp);
+	} else {
+		*signature = arg;
+	}
+	return 0;
+}
+
 int cmd_format_patch(int argc, const char **argv, const char *prefix)
 {
 	struct commit *commit;
@@ -1230,6 +1251,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_OPTARG, thread_callback },
 		OPT_STRING(0, "signature", &signature, N_("signature"),
 			N_("add a signature")),
+		{ OPTION_CALLBACK, 0, "signature-file", &signature, N_("signature-file"),
+N_("add a signature from contents of a file"),
+			PARSE_OPT_NONEG, signature_file_callback },
 		OPT__QUIET(&quiet, N_("don't print the patch filenames")),
 		OPT_END()
 	};
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 9c80633..19b67e3 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -762,6 +762,19 @@ test_expect_success 'format-patch --signature="" suppresses signatures' '
 	! grep "^-- \$" output
 '
 
+cat > expect << EOF
+Test User 
+http://git.kernel.org/cgit/git/git.git
+git.kernel.org/?p=git/git.git;a=summary
+EOF
+
+test_expect_success 'format-patch --signature-file file' '
+	git format-patch --stdout --signature-file expect -1 >output &&
+	check_patch output &&
+	fgrep -x -f output expect >output2 &&
+	diff expect output2
+'
+
 test_expect_success TTY 'format-patch --stdout paginates' '
 	rm -f pager_used &&
 	test_terminal env GIT_PAGER="wc >pager_used" git format-patch --stdout --all &&
-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler

[PATCH v2] format-patch --signature-file

[Jeremiah Mahler]

On Tue, May 13, 2014 at 09:07:12AM -0700, Jonathan Nieder wrote:
> Hi,
> 
> Jeremiah Mahler wrote:
> 
> >   # from a string
> >   $ git format-patch --signature "from a string" origin
> >
> >   # or from a file
> >   $ git format-patch --signature ~/.signature origin
> 
> Interesting.  But... what if I want my patch to end with
> 
>   -- 
>   /home/jrnieder/.signature
> 
> ?  It seems safer to introduce a separate --signature-file option.
> 

It is probably smarter to avoid that corner case entirely.
Good idea.

> [...]
> >  builtin/log.c | 26 --
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> Tests?
> 

I added a test which checks that a valid patch is produced and that
the signature from the file appears in the output.

> Thanks and hope that helps,
> Jonathan

In addition to addressing the suggestions from Jonathan I also
updated the Documentation.

This solution uses a static buffer to store the signature which does
create a size limitation (1024 bytes).  I considered a solution
using malloc but I could not figure out a clean way of determining when
to free the memory.

Thanks for the help and suggestions.

Jeremiah Mahler (1):
  format-patch --signature-file 

 Documentation/git-format-patch.txt |  7 +++
 builtin/log.c  | 24 ++++
 t/t4014-format-patch.sh| 13 +
 3 files changed, 44 insertions(+)

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler


--
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] format-patch --signature-file

[Jeremiah Mahler]

Added feature that allows a signature file to be used with format-patch.

  $ git format-patch --signature-file ~/.signature -1

Now signatures with newlines and other special characters can be
easily included.

Signed-off-by: Jeremiah Mahler 
---
 Documentation/git-format-patch.txt |  7 +++
 builtin/log.c  | 24 
 t/t4014-format-patch.sh| 13 +
 3 files changed, 44 insertions(+)

diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 5c0a4ab..dd57841 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -14,6 +14,7 @@ SYNOPSIS
   [(--attach|--inline)[=] | --no-attach]
   [-s | --signoff]
   [--signature= | --no-signature]
+  [--signature-file=]
   [-n | --numbered | -N | --no-numbered]
   [--start-number ] [--numbered-files]
   [--in-reply-to=Message-Id] [--suffix=.]
@@ -233,6 +234,12 @@ configuration options in linkgit:git-notes[1] to use this 
workflow).
signature option is omitted the signature defaults to the Git version
number.
 
+--signature-file=::
+   Add a signature, by including the contents of a file, to each message
+   produced. Per RFC 3676 the signature is separated from the body by a
+   line with '-- ' on it. If the signature option is omitted the signature
+   defaults to the Git version number.
+
 --suffix=.::
Instead of using `.patch` as the suffix for generated
filenames, use specified suffix.  A common alternative is
diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..4a7308d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1147,6 +1147,27 @@ static int from_callback(const struct option *opt, const 
char *arg, int unset)
return 0;
 }
 
+static int signature_file_callback(const struct option *opt, const char *arg,
+   int unset)
+{
+   const char **signature = opt->value;
+   static char buf[1024];
+   size_t sz;
+   FILE *fd;
+
+   fd = fopen(arg, "r");
+   if (fd) {
+   sz = sizeof(buf);
+   sz = fread(buf, 1, sz - 1, fd);
+   if (sz) {
+   buf[sz] = '\0';
+   *signature = buf;
+   }
+   fclose(fd);
+   }
+   return 0;
+}
+
 int cmd_format_patch(int argc, const char **argv, const char *prefix)
 {
struct commit *commit;
@@ -1230,6 +1251,9 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
PARSE_OPT_OPTARG, thread_callback },
OPT_STRING(0, "signature", &signature, N_("signature"),
N_("add a signature")),
+   { OPTION_CALLBACK, 0, "signature-file", &signature, 
N_("signature-file"),
+   N_("add a signature from contents of a file"),
+   PARSE_OPT_NONEG, signature_file_callback },
OPT__QUIET(&quiet, N_("don't print the patch filenames")),
OPT_END()
};
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 9c80633..19b67e3 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -762,6 +762,19 @@ test_expect_success 'format-patch --signature="" 
suppresses signatures' '
! grep "^-- \$" output
 '
 
+cat > expect << EOF
+Test User 
+http://git.kernel.org/cgit/git/git.git
+git.kernel.org/?p=git/git.git;a=summary
+EOF
+
+test_expect_success 'format-patch --signature-file file' '
+   git format-patch --stdout --signature-file expect -1 >output &&
+   check_patch output &&
+   fgrep -x -f output expect >output2 &&
+   diff expect output2
+'
+
 test_expect_success TTY 'format-patch --stdout paginates' '
rm -f pager_used &&
test_terminal env GIT_PAGER="wc >pager_used" git format-patch --stdout 
--all &&
-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler


--
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] format-patch --signature-file

[Jeremiah Mahler]

On Fri, May 16, 2014 at 04:14:45AM -0400, Jeff King wrote:
> On Thu, May 15, 2014 at 06:31:21PM -0700, Jeremiah Mahler wrote:
...
> 
> A few questions/comments:
> 
> > +static int signature_file_callback(const struct option *opt, const char 
> > *arg,
> > +   int unset)
> > +{
> > +   const char **signature = opt->value;
> > +   static char buf[1024];
> > +   size_t sz;
> > +   FILE *fd;
> > +
> > +   fd = fopen(arg, "r");
> > +   if (fd) {
> > +   sz = sizeof(buf);
> > +   sz = fread(buf, 1, sz - 1, fd);
> > +   if (sz) {
> > +   buf[sz] = '\0';
> > +   *signature = buf;
> > +   }
> > +   fclose(fd);
> > +   }
> > +   return 0;
> > +}
> 
> We have routines for reading directly into a strbuf, which eliminates
> the need for this 1024-byte limit. We even have a wrapper that can make
> this much shorter:
> 
>   struct strbuf buf = STRBUF_INIT;
> 
>   strbuf_read_file(&buf, arg, 128);
>   *signature = strbuf_detach(&buf, NULL);
> 

Yes, that is much cleaner.
The memory returned by strbuf_detach() will have to be freed as well.

> I notice that you ignore any errors. Is that intentional (so that we
> silently ignore missing --signature files)? If so, should we actually
> treat it as an empty file (e.g., in my code above, we always set
> *signature, even if the file was missing)?
> 
> Finally, I suspect that:
> 
>   cd path/in/repo &&
>   git format-patch --signature-file=foo
> 
> will not work, as we chdir() to the toplevel before evaluating the
> arguments. You can fix that either by using parse-option's OPT_FILENAME
> to save the filename, followed by opening the file after all arguments
> are processed; or by manually fixing up the filename.
> 

Yes, it wouldn't have worked.
Using OPT_FILENAME is a much better solution.

> Since parse-options already knows how to do this fixup (it does it for
> OPT_FILENAME), it would be nice if it were a flag rather than a full
> type, so you could specify at as an option to your callback here:
> 
> > +   { OPTION_CALLBACK, 0, "signature-file", &signature, 
> > N_("signature-file"),
> > +   N_("add a signature from contents of a file"),
> > +   PARSE_OPT_NONEG, signature_file_callback },
> 
> Noticing your OPT_NONEG, though, I wonder if you should simply use an
> OPT_FILENAME. I would expect --no-signature-file to countermand any
> earlier --signature-file on the command-line (or if we eventually grow a
> config option, which seems sensible, it would tell git to ignore the
> option). The usual ordering for that is:
> 

Another case is when both --signature="foo" and --no-signature-file are used.
Currently this would only negate the file option which would allow
the --signature option to be used.

>   1. Read config and store format.signatureFile as a string
>  "signature_file".
> 
>   2. Parse arguments. --signature-file=... sets signature_file, and
>  --no-signature-file sets it to NULL.
> 
>   3. If signature_file is non-NULL, load it.
> 
> And I believe OPT_FILENAME will implement (2) for you.
> 
> One downside of doing it this way is that you need to specify what will
> happen when both "--signature" (or format.signature) and
> "--signature-file" are set. With your current code, I think
> "--signature=foo --signature-file=bar" will take the second one. I think
> it would be fine to prefer one to the other, or to just notice that both
> are set and complain.
> 
> -Peff

Having --signature-file override --signature seems simpler to implement.
The signature variable has a default value which complicates
determining whether it was set or not.

Thanks for the great suggestions.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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] format-patch --signature-file

[Jeremiah Mahler]

On Sat, May 17, 2014 at 03:42:24AM -0400, Jeff King wrote:
> On Sat, May 17, 2014 at 12:25:48AM -0700, Jeremiah Mahler wrote:
> 
> > > We have routines for reading directly into a strbuf, which eliminates
> > > the need for this 1024-byte limit. We even have a wrapper that can make
> > > this much shorter:
> > > 
> > >   struct strbuf buf = STRBUF_INIT;
> > > 
> > >   strbuf_read_file(&buf, arg, 128);
> > >   *signature = strbuf_detach(&buf, NULL);
> > > 
> > 
> > Yes, that is much cleaner.
> > The memory returned by strbuf_detach() will have to be freed as well.
> 
> In cases like this, we often let the memory leak. It's in a global that
> stays valid through the whole program, so we just let the program's exit
> clean it up.
> 

It bugs me but I see your point.
It works just fine in this situation.

> > Having --signature-file override --signature seems simpler to implement.
> > The signature variable has a default value which complicates
> > determining whether it was set or not.
> 
> Yeah, the default value complicates it. I think you can handle that just
> by moving the default to the main logic, like:
> 
>   static const char *signature;
>   static const char *signature_file;
> 
>   ...
> 
>   if (signature) {
>   if (signature_file)
>   die("you cannot specify both a signature and a signature-file");
>   /* otherwise, we already have the value */
>   } else if (signature_file) {
>   struct strbuf buf = STRBUF_INIT;
>   strbuf_read(&buf, signature_file, 128);
>   signature = strbuf_detach(&buf);
>   } else
>   signature = git_version_string;
> 

Before, --no-signature would clear the &signature.
With this code it sees it as not being set and assigns
the default version string.

> and as a bonus, that keeps all of the logic together in one (fairly
> readable) chain.
> 
> -Peff

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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] Documentation/technical/api-hashmap: Remove source highlighting

[Jeremiah Mahler]

On Sat, May 17, 2014 at 07:08:55AM -0400, Anders Kaseorg wrote:
> The highlighting was pretty, but unfortunately, the failure mode when
> source-highlight is not installed was that the entire code block
> disappears.  See https://bugs.debian.org/745591,
> https://bugs.launchpad.net/bugs/1316810.
> 

I agree that a broken document is an unacceptable failure mode.

But I do not understand why 'source-highlight' is not an install
requirement for 'git-doc'.  If I install 'source-highlight' on
my Debian machine the code looks great.

  apt-get install source-highlight

I also noticed that this seems to be the single place where source
code highlighting is used in Documentation/technical.
So it might be worthwhile to eliminate this dependency all together
as Anders patch does.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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] format-patch --signature-file

[Jeremiah Mahler]

On Sat, May 17, 2014 at 06:00:14AM -0400, Jeff King wrote:
> On Sat, May 17, 2014 at 01:59:11AM -0700, Jeremiah Mahler wrote:
> 
> > >   if (signature) {
> > >   if (signature_file)
> > >   die("you cannot specify both a signature and a signature-file");
> > >   /* otherwise, we already have the value */
> > >   } else if (signature_file) {
> > >   struct strbuf buf = STRBUF_INIT;
> > >   strbuf_read(&buf, signature_file, 128);
> > >   signature = strbuf_detach(&buf);
> > >   } else
> > >   signature = git_version_string;
> > > 
> > 
> > Before, --no-signature would clear the &signature.
> > With this code it sees it as not being set and assigns
> > the default version string.
> 
> Ah, you're right. Thanks for catching it.
> 
> If you wanted to know whether it was set, I guess you'd have to compare
> it to the default, like:
> 
>   if (signature_file) {
>   if (signature && signature != git_version_string)
>   die("you cannot specify both a signature and a signature-file");
>   ... read signature file ...
>   }
> 

That works until someone changes the default value.
But if they did that then some tests should fail.

I like the address comparision which avoids a string comparision.

> though it's a bit ugly that this code has to know what the default is.
> Having signature-file take precedence is OK with me, but it feels
> somewhat arbitrary to me from the user's perspective.
> 
> -Peff

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 v3] format-patch --signature-file

[Jeremiah Mahler]

v3 patch to add format-patch --signature-file  option.

This revision includes changes suggested by Jeff King:

  - Instead of a custom OPTION_CALLBACK, use OPT_FILENAME which
correctly resolves file names which are not evaluated by the
shell such as "--signature-file="

  - Use strbuf_read_file() which eliminates the arbitrary size limit
and simplifies the code.

  - Generate an error if the file cannot be read instead of quietly
continuing.  This is accomplished with strbuf_read_file().

  - Die if both --signature and --signature-file are used.

Jeremiah Mahler (1):
  format-patch --signature-file 

 Documentation/git-format-patch.txt |  4 
 builtin/log.c  | 13 +
 t/t4014-format-patch.sh| 28 
 3 files changed, 45 insertions(+)

--
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 v3] format-patch --signature-file

[Jeremiah Mahler]

Added feature that allows a signature file to be used with format-patch.

  $ git format-patch --signature-file ~/.signature -1

Now signatures with newlines and other special characters can be
easily included.

Signed-off-by: Jeremiah Mahler 
---
 Documentation/git-format-patch.txt |  4 
 builtin/log.c  | 13 +
 t/t4014-format-patch.sh| 28 
 3 files changed, 45 insertions(+)

diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 5c0a4ab..c0fd470 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -14,6 +14,7 @@ SYNOPSIS
   [(--attach|--inline)[=] | --no-attach]
   [-s | --signoff]
   [--signature= | --no-signature]
+  [--signature-file=]
   [-n | --numbered | -N | --no-numbered]
   [--start-number ] [--numbered-files]
   [--in-reply-to=Message-Id] [--suffix=.]
@@ -233,6 +234,9 @@ configuration options in linkgit:git-notes[1] to use this 
workflow).
signature option is omitted the signature defaults to the Git version
number.
 
+--signature-file=::
+   Works just like --signature except the signature is read from a file.
+
 --suffix=.::
Instead of using `.patch` as the suffix for generated
filenames, use specified suffix.  A common alternative is
diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..af7d610 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -673,6 +673,7 @@ static void add_header(const char *value)
 static int thread;
 static int do_signoff;
 static const char *signature = git_version_string;
+static const char *signature_file;
 static int config_cover_letter;
 
 enum {
@@ -1230,6 +1231,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
PARSE_OPT_OPTARG, thread_callback },
OPT_STRING(0, "signature", &signature, N_("signature"),
N_("add a signature")),
+   OPT_FILENAME(0, "signature-file", &signature_file,
+   N_("add a signature from a file")),
OPT__QUIET(&quiet, N_("don't print the patch filenames")),
OPT_END()
};
@@ -1447,6 +1450,16 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
cover_letter = (config_cover_letter == COVER_ON);
}
 
+   if (signature_file) {
+   if (signature && signature != git_version_string)
+   die(_("--signature and --signature-file are mutually 
exclusive"));
+
+   struct strbuf buf = STRBUF_INIT;
+
+   strbuf_read_file(&buf, signature_file, 128);
+   signature = strbuf_detach(&buf, NULL);
+   }
+
if (in_reply_to || thread || cover_letter)
rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
if (in_reply_to) {
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 9c80633..fb3dc1b 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -762,6 +762,34 @@ test_expect_success 'format-patch --signature="" 
suppresses signatures' '
! grep "^-- \$" output
 '
 
+cat > expect << EOF
+Test User 
+http://git.kernel.org/cgit/git/git.git
+git.kernel.org/?p=git/git.git;a=summary
+EOF
+
+test_expect_success 'format-patch --signature-file file' '
+   git format-patch --stdout --signature-file expect -1 >output &&
+   check_patch output &&
+   fgrep -x -f output expect >output2 &&
+   diff expect output2
+'
+
+test_expect_success 'format-patch --signature-file=file' '
+   git format-patch --stdout --signature-file=expect -1 >output &&
+   check_patch output &&
+   fgrep -x -f output expect >output2 &&
+   diff expect output2
+'
+
+test_expect_success 'format-patch --signature and --signature-file die' '
+   ! git format-patch --stdout --signature="foo" --signature-file=expect 
-1 >output
+'
+
+test_expect_success 'format-patch --no-signature and --signature-file OK' '
+   git format-patch --stdout --no-signature --signature-file=expect -1 
>output
+'
+
 test_expect_success TTY 'format-patch --stdout paginates' '
rm -f pager_used &&
test_terminal env GIT_PAGER="wc >pager_used" git format-patch --stdout 
--all &&
--
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] format-patch --signature-file

[Jeremiah Mahler]

Peff,

Lots of good suggestions.  I had to expand upon the signature
pattern match to get to work.

On Sun, May 18, 2014 at 07:20:20AM -0400, Jeff King wrote:
> On Sat, May 17, 2014 at 09:02:22AM -0700, Jeremiah Mahler wrote:
> 
...
> > +test_expect_success 'format-patch --signature-file file' '
> > +   git format-patch --stdout --signature-file expect -1 >output &&
> > +   check_patch output &&
> > +   fgrep -x -f output expect >output2 &&
> 
> Both of these fgrep options are in POSIX, but it looks like this will be
> the first use for either of them. I'm not sure if they will give us any
> portability problems.
> 
> We could probably do something like:
> 
>   sed -n '/^-- $/,$p'
> 

This gets the signature out but it will have '--' and
some trailing blank lines which were not in the original signature.
So then test_cmp won't work directly.

What I came up with was to use head and tail to remove the first line
and the last two lines.  Then test_cmp can be used normally.

test_expect_success 'format-patch --signature-file=file' '
git format-patch --stdout --signature-file=expect -1 >output &&
check_patch_output output &&
sed -n "/^-- $/,\$p" output2 &&
test_cmp expect output2
'

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 v4] format-patch --signature-file

[Jeremiah Mahler]

Added option that allows a signature file to be used with format-patch
so that signatures with newlines and other special characters can be
easily included.

  $ git format-patch --signature-file ~/.signature -1

The config variable format.signaturefile is also provided so that it
can be added by default.

  $ git config format.signaturefile ~/.signature

  $ git format-patch -1

Signed-off-by: Jeremiah Mahler 
---
 Documentation/config.txt   |  4 
 Documentation/git-format-patch.txt |  4 
 builtin/log.c  | 16 
 t/t4014-format-patch.sh| 33 +
 4 files changed, 57 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..140ed77 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1114,6 +1114,10 @@ format.signature::
Set this variable to the empty string ("") to suppress
signature generation.
 
+format.signaturefile::
+   Works just like format.signature except the contents of the
+   file specified by this variable will be used as the signature.
+
 format.suffix::
The default for format-patch is to output files with the suffix
`.patch`. Use this variable to change that suffix (make sure to
diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 5c0a4ab..c0fd470 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -14,6 +14,7 @@ SYNOPSIS
   [(--attach|--inline)[=] | --no-attach]
   [-s | --signoff]
   [--signature= | --no-signature]
+  [--signature-file=]
   [-n | --numbered | -N | --no-numbered]
   [--start-number ] [--numbered-files]
   [--in-reply-to=Message-Id] [--suffix=.]
@@ -233,6 +234,9 @@ configuration options in linkgit:git-notes[1] to use this 
workflow).
signature option is omitted the signature defaults to the Git version
number.
 
+--signature-file=::
+   Works just like --signature except the signature is read from a file.
+
 --suffix=.::
Instead of using `.patch` as the suffix for generated
filenames, use specified suffix.  A common alternative is
diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..0a8f417 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -673,6 +673,7 @@ static void add_header(const char *value)
 static int thread;
 static int do_signoff;
 static const char *signature = git_version_string;
+static const char *signature_file;
 static int config_cover_letter;
 
 enum {
@@ -742,6 +743,8 @@ static int git_format_config(const char *var, const char 
*value, void *cb)
}
if (!strcmp(var, "format.signature"))
return git_config_string(&signature, var, value);
+   if (!strcmp(var, "format.signaturefile"))
+   return git_config_string(&signature_file, var, value);
if (!strcmp(var, "format.coverletter")) {
if (value && !strcasecmp(value, "auto")) {
config_cover_letter = COVER_AUTO;
@@ -1230,6 +1233,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
PARSE_OPT_OPTARG, thread_callback },
OPT_STRING(0, "signature", &signature, N_("signature"),
N_("add a signature")),
+   OPT_FILENAME(0, "signature-file", &signature_file,
+   N_("add a signature from a file")),
OPT__QUIET(&quiet, N_("don't print the patch filenames")),
OPT_END()
};
@@ -1447,6 +1452,17 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
cover_letter = (config_cover_letter == COVER_ON);
}
 
+   if (signature_file) {
+   if (signature && signature != git_version_string)
+   die(_("cannot specify both signature and 
signature-file"));
+
+   struct strbuf buf = STRBUF_INIT;
+
+   if (strbuf_read_file(&buf, signature_file, 128) < 0)
+   die_errno(_("unable to read signature file '%s'"), 
signature_file);
+   signature = strbuf_detach(&buf, NULL);
+   }
+
if (in_reply_to || thread || cover_letter)
rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
if (in_reply_to) {
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 9c80633..c6d44b8 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -762,6 +762,39 @@ test_expect_success 'format-patch --signature="" 
suppresses signatures' '
! grep "^-- \$&qu

[PATCH v4] format-patch --signature-file

[Jeremiah Mahler]

v4 of patch to add format-patch --signature-file  option.

This revision includes more suggestions from Jeff King:

  - Added a format.signaturefile config option.

  - Fixed error messages, --signature and format.signature

  - Fixed error checking, dies if strbuf_read_file fails.

  - style: "cat > expect << EOF" to "cat >expect <<-\EOF"

  - Eliminated use of fgrep, which is not used anywhere else
in the project.  Possibility of portability problems.
Replaced with sed which is commonly used.

  - Eliminated use of diff in favor of test_cmp.

  - Removed redundant test, "--foo=bar" and "--foo bar"

  - Changed to use test_must_fail instead of "!"

Jeremiah Mahler (1):
  format-patch --signature-file 

 Documentation/config.txt   |  4 
 Documentation/git-format-patch.txt |  4 
 builtin/log.c  | 16 
 t/t4014-format-patch.sh| 33 +++++
 4 files changed, 57 insertions(+)

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler


--
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] format-patch --signature-file

[Jeremiah Mahler]

On Mon, May 19, 2014 at 09:54:33AM -0700, Junio C Hamano wrote:
> Jeremiah Mahler  writes:
> 
> > On Sat, May 17, 2014 at 06:00:14AM -0400, Jeff King wrote:
> >> 
>
> Avoiding that is easy with an indirection, no?  Something like this
> at the top:
> 
>   static const char *the_default_signature = git_version_string;
>   static const char *signature = the_default_signature;
> 
> and comparing to see if signature points at the same address as
> the_default_signature would give you what you want, I think.

I like your suggestion for a default signature variable.
Unfortunately, C doesn't like it as much.

static const char *the_default_signature = git_version_string;
static const char *signature = the_default_signature;  // ERROR
// initializer element is not constant

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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] format-patch --signature-file

[Jeremiah Mahler]

Added option that allows a signature file to be used with format-patch
so that signatures with newlines and other special characters can be
easily included.

  $ git format-patch --signature-file ~/.signature -1

The config variable format.signaturefile is also provided so that it
can be added by default.

  $ git config format.signaturefile ~/.signature

  $ git format-patch -1

Signed-off-by: Jeremiah Mahler 
---
 Documentation/config.txt   |  4 
 Documentation/git-format-patch.txt |  4 
 builtin/log.c  | 19 ++-
 t/t4014-format-patch.sh| 32 
 4 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..140ed77 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1114,6 +1114,10 @@ format.signature::
Set this variable to the empty string ("") to suppress
signature generation.
 
+format.signaturefile::
+   Works just like format.signature except the contents of the
+   file specified by this variable will be used as the signature.
+
 format.suffix::
The default for format-patch is to output files with the suffix
`.patch`. Use this variable to change that suffix (make sure to
diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 5c0a4ab..c0fd470 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -14,6 +14,7 @@ SYNOPSIS
   [(--attach|--inline)[=] | --no-attach]
   [-s | --signoff]
   [--signature= | --no-signature]
+  [--signature-file=]
   [-n | --numbered | -N | --no-numbered]
   [--start-number ] [--numbered-files]
   [--in-reply-to=Message-Id] [--suffix=.]
@@ -233,6 +234,9 @@ configuration options in linkgit:git-notes[1] to use this 
workflow).
signature option is omitted the signature defaults to the Git version
number.
 
+--signature-file=::
+   Works just like --signature except the signature is read from a file.
+
 --suffix=.::
Instead of using `.patch` as the suffix for generated
filenames, use specified suffix.  A common alternative is
diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..7ebe302 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -672,7 +672,9 @@ static void add_header(const char *value)
 #define THREAD_DEEP 2
 static int thread;
 static int do_signoff;
-static const char *signature = git_version_string;
+#define DEFAULT_SIGNATURE git_version_string
+static const char *signature = DEFAULT_SIGNATURE;
+static const char *signature_file;
 static int config_cover_letter;
 
 enum {
@@ -742,6 +744,8 @@ static int git_format_config(const char *var, const char 
*value, void *cb)
}
if (!strcmp(var, "format.signature"))
return git_config_string(&signature, var, value);
+   if (!strcmp(var, "format.signaturefile"))
+   return git_config_pathname(&signature_file, var, value);
if (!strcmp(var, "format.coverletter")) {
if (value && !strcasecmp(value, "auto")) {
config_cover_letter = COVER_AUTO;
@@ -1230,6 +1234,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
PARSE_OPT_OPTARG, thread_callback },
OPT_STRING(0, "signature", &signature, N_("signature"),
N_("add a signature")),
+   OPT_FILENAME(0, "signature-file", &signature_file,
+   N_("add a signature from a file")),
OPT__QUIET(&quiet, N_("don't print the patch filenames")),
OPT_END()
};
@@ -1447,6 +1453,17 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
cover_letter = (config_cover_letter == COVER_ON);
}
 
+   if (signature_file) {
+   if (signature && signature != DEFAULT_SIGNATURE)
+   die(_("cannot specify both signature and 
signature-file"));
+
+   struct strbuf buf = STRBUF_INIT;
+
+   if (strbuf_read_file(&buf, signature_file, 128) < 0)
+   die_errno(_("unable to read signature file '%s'"), 
signature_file);
+   signature = strbuf_detach(&buf, NULL);
+   }
+
if (in_reply_to || thread || cover_letter)
rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
if (in_reply_to) {
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 9c80633..74f0aec 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -762,6 +762

[PATCH v5] format-patch --signature-file

[Jeremiah Mahler]

v5 of patch to add format-patch --signature-file  option.

This revision includes more suggestions from Jeff King and Junio C Hamano:

  - Use git_config_pathname instead of git_config_string for ~ expansion.

  - Eliminated head/tail --lines which is not POSIX compliant.
Replaced with sed equivalents.

  - Used test_config instead of git config which also handles unsetting.

  - Use a DEFAULT_SIGNATURE variable to better describe its purpose
instead of git_version_string which could be confusing.


Jeremiah Mahler (1):
  format-patch --signature-file 

 Documentation/config.txt   |  4 
 Documentation/git-format-patch.txt |  4 
 builtin/log.c  | 19 ++-
 t/t4014-format-patch.sh| 32 
 4 files changed, 58 insertions(+), 1 deletion(-)

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler


--
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] format-patch --signature-file

[Jeremiah Mahler]

On Tue, May 20, 2014 at 04:27:40AM -0400, Jeff King wrote:
> On Tue, May 20, 2014 at 01:00:06AM -0700, Jeremiah Mahler wrote:
> 
...
> > +test_expect_success 'format-patch --signature-file=file' '
> > +   git format-patch --stdout --signature-file=expect -1 >output &&
> > +   check_patch output &&
> > +   sed -n -e "/^-- $/,\$p"  > "\$d" >output2 &&
> > +   test_cmp expect output2
> > +'
> 
> Here we drop two final lines from the email output. But if I just use
> vanilla git and try this, I get nothing:
> 
>   $ git format-patch -1 --stdout |
> sed -n -e "/^-- $/,\$p" | sed -e "1 d" | sed -e "\$d" | sed -e "\$d"
> 
> Removing the second dropped line works:
> 
>   $ git format-patch -1 --stdout |
> sed -n -e "/^-- $/,\$p" | sed -e "1 d" | sed -e "\$d"
>   2.0.0.rc1.436.g03cb729
> 
> I guess the signature code is adding its own newline, so that the
> default signature (or "--signature=foo") works. But it adds it
> unconditionally, which is probably not what we want for signatures read
> from a file.
> 
> Do we maybe want something like this right before your patch?
> 
> -- >8 --
> Subject: format-patch: make newline after signature conditional
> 
> When we print an email signature, we print the divider "--
> \n", then the signature string, then two newlines.
> Traditionally the signature is a one-liner (and the default
> is just the git version), so the extra newline makes sense.
> 
> But one could easily specify a longer, multi-line signature,
> like:
> 
>   git format-patch --signature='
>   this is my long signature
> 
>   it has multiple lines
>   ' ...
> 
> We should notice that it already has its own trailing
> newline, and suppress one of ours.
> 
> Signed-off-by: Jeff King 
> ---
> In the example above, there's also a newline before the signature
> starts. Should we suppress the first one, too?
> 
> Also, I'm not clear on the purpose of the extra trailing line in the
> first place. Emails now end with (">" added to show blanks):
> 
>   > -- 
>   > 2.0.0-rc3...
>   >
> 
> Is there are a reason for that final blank line?
> 
>  builtin/log.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/log.c b/builtin/log.c
> index 39e8836..5acc048 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -844,8 +844,13 @@ static void gen_message_id(struct rev_info *info, char 
> *base)
>  
>  static void print_signature(void)
>  {
> - if (signature && *signature)
> - printf("-- \n%s\n\n", signature);
> + if (!signature || !*signature)
> + return;
> +
> + printf("-- \n%s", signature);
> + if (signature[strlen(signature)-1] != '\n')
> + putchar('\n');
> + putchar('\n');
>  }
>  
>  static void add_branch_description(struct strbuf *buf, const char 
> *branch_name)
> -- 
> 2.0.0.rc1.436.g03cb729
> 

The simple solution, leaving things as they are, appeals to me.
For the purposes of testing just expect whatever odd number of blank
lines are produced.  Or perhaps slurp all the trailing blank lines and
then compare.

It would be nice if every signature was followed by exactly one blank line.
But if it had two it wouldn't bother me much.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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] format-patch --signature-file

[Jeremiah Mahler]

On Tue, May 20, 2014 at 08:06:50AM -0700, Junio C Hamano wrote:
> Jeff King  writes:
> 
> > You could do:
> >
> >   #define DEFAULT_SIGNATURE git_version_string
> >   static const char *signature = DEFAULT_SIGNATURE;
> >
> >   ...
> >
> >   if (signature == DEFAULT_SIGNATURE)
> >  ...
> >
> > but maybe that is getting a little unwieldy, considering the scope of
> > the original problem.
> 
> I agree.  It is an invitation for doing something like
> 
>   #define DEFAULT_SIGNATURE "-- \nfoo bar\n"
> 
> which defeats the purpose.
> 
> I have an unrelated and larger design level suggestion, but it may
> be better for me to refrain from sending it during the -rc freeze.

I will take DEFAULT_SIGNATURE out of the next patch revision.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 v6] format-patch --signature-file

[Jeremiah Mahler]

v6 of patch to add format-patch --signature-file  option.

This revision includes more suggestions from Jeff King and Junio C Hamano:

  - Adding #define DEFAULT_SIGNATURE was a good idea but it could be
used in a way that nullifies the pointer comparison used to see if
the default has changed.  So this was removed.

  - The test cases depend on there being two blank lines after a
signature from a file.  This is an odd problem because if --signature
or the default is used only one blank line is produced.
This problem can be addressed in another patch.

Jeremiah Mahler (1):
  format-patch --signature-file 

 Documentation/config.txt   |  4 
 Documentation/git-format-patch.txt |  4 
 builtin/log.c  | 16 
 t/t4014-format-patch.sh| 32 
 4 files changed, 56 insertions(+)

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler


--
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 v6] format-patch --signature-file

[Jeremiah Mahler]

Added option that allows a signature file to be used with format-patch
so that signatures with newlines and other special characters can be
easily included.

  $ git format-patch --signature-file ~/.signature -1

The config variable format.signaturefile is also provided so that it
can be added by default.

  $ git config format.signaturefile ~/.signature

  $ git format-patch -1

Signed-off-by: Jeremiah Mahler 
---
 Documentation/config.txt   |  4 
 Documentation/git-format-patch.txt |  4 
 builtin/log.c  | 16 
 t/t4014-format-patch.sh| 32 
 4 files changed, 56 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..140ed77 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1114,6 +1114,10 @@ format.signature::
Set this variable to the empty string ("") to suppress
signature generation.
 
+format.signaturefile::
+   Works just like format.signature except the contents of the
+   file specified by this variable will be used as the signature.
+
 format.suffix::
The default for format-patch is to output files with the suffix
`.patch`. Use this variable to change that suffix (make sure to
diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 5c0a4ab..c0fd470 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -14,6 +14,7 @@ SYNOPSIS
   [(--attach|--inline)[=] | --no-attach]
   [-s | --signoff]
   [--signature= | --no-signature]
+  [--signature-file=]
   [-n | --numbered | -N | --no-numbered]
   [--start-number ] [--numbered-files]
   [--in-reply-to=Message-Id] [--suffix=.]
@@ -233,6 +234,9 @@ configuration options in linkgit:git-notes[1] to use this 
workflow).
signature option is omitted the signature defaults to the Git version
number.
 
+--signature-file=::
+   Works just like --signature except the signature is read from a file.
+
 --suffix=.::
Instead of using `.patch` as the suffix for generated
filenames, use specified suffix.  A common alternative is
diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..ab4718b 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -673,6 +673,7 @@ static void add_header(const char *value)
 static int thread;
 static int do_signoff;
 static const char *signature = git_version_string;
+static const char *signature_file;
 static int config_cover_letter;
 
 enum {
@@ -742,6 +743,8 @@ static int git_format_config(const char *var, const char 
*value, void *cb)
}
if (!strcmp(var, "format.signature"))
return git_config_string(&signature, var, value);
+   if (!strcmp(var, "format.signaturefile"))
+   return git_config_pathname(&signature_file, var, value);
if (!strcmp(var, "format.coverletter")) {
if (value && !strcasecmp(value, "auto")) {
config_cover_letter = COVER_AUTO;
@@ -1230,6 +1233,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
PARSE_OPT_OPTARG, thread_callback },
OPT_STRING(0, "signature", &signature, N_("signature"),
N_("add a signature")),
+   OPT_FILENAME(0, "signature-file", &signature_file,
+   N_("add a signature from a file")),
OPT__QUIET(&quiet, N_("don't print the patch filenames")),
OPT_END()
};
@@ -1447,6 +1452,17 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
cover_letter = (config_cover_letter == COVER_ON);
}
 
+   if (signature_file) {
+   if (signature && signature != git_version_string)
+   die(_("cannot specify both signature and 
signature-file"));
+
+   struct strbuf buf = STRBUF_INIT;
+
+   if (strbuf_read_file(&buf, signature_file, 128) < 0)
+   die_errno(_("unable to read signature file '%s'"), 
signature_file);
+   signature = strbuf_detach(&buf, NULL);
+   }
+
if (in_reply_to || thread || cover_letter)
rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
if (in_reply_to) {
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 9c80633..049493d 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -762,6 +762,38 @@ test_expect_success 'format-patch --signature="" 
suppresses signatures' '
! grep "^-- \$&qu

Re: [PATCH v5] format-patch --signature-file

[Jeremiah Mahler]

On Wed, May 21, 2014 at 12:42:55PM -0400, Jeff King wrote:
> On Tue, May 20, 2014 at 11:46:51AM -0700, Junio C Hamano wrote:
> 
> > Jeff King  writes:
> > 
> > > If it were just "--signature", I'd agree. After all, nobody is even
> > > complaining. But this is also in preparation for --signature-file.
> > > Should the user create a file without a trailing newline?
> > 
> > Ahh, I missed that part.
> > 
> > I am fine with processing it with stripspace().
> 
> I wasn't planning on anything as drastic as stripspace. I really just
> wanted to suppress the one newline, which is almost certainly the right
> thing to include for "--signature", but the wrong thing for
> "--signature-file" (i.e., the patch I posted earlier).
> 
> Stripspace() would drop all extra whitespace, and I wondered if people
> would _want_ it in their sigs (e.g., a blank line after the "-- " but
> before their .sig content).
> 
> I dunno. Maybe it is not worth caring too much about. I don't want to
> hold up Jeremiah's patch for something that I suspect neither of us
> cares _that_ much about (I know I am not planning on using
> --signature-file myself). I just don't want to deal with a patch later
> that says "oh, this spacing is wrong" and have to respond "yes, but we
> have to retain it so as not to break existing users".
> 
> > By the way, at some point we may want to move that helper function
> > to strbuf.c, but that is a separate issue.
> 
> Agreed. I was touching some string functions earlier today and noticed
> that strbuf.c actually contains a lot of non-strbuf functions for
> dealing with C strings. That's fine, I guess, but I also wondered if we
> should have a separate file for C-string functions. I suppose it doesn't
> matter that much either way, as long as it's in a libgit.a file (and
> stripspace currently is _not_, which I assume is what you were
> indicating above).
> 
> -Peff

I am fine with including your previous patch.

Would like me to test it out and create another patch set?

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 v7 0/2] format-patch --signature-file

[Jeremiah Mahler]

v7 of patch to add format-patch --signature-file  option.

This revision includes a patch from Jeff King to fix the odd number
spaces produced by print_signature().

Jeff King (1):
  format-patch: make newline after signature conditional

Jeremiah Mahler (1):
  format-patch --signature-file 

 Documentation/config.txt   |  4 
 Documentation/git-format-patch.txt |  4 
 builtin/log.c  | 25 +++--
 t/t4014-format-patch.sh| 32 
 4 files changed, 63 insertions(+), 2 deletions(-)

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler

--
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 v7 1/2] format-patch: make newline after signature conditional

[Jeremiah Mahler]

From: Jeff King 

When we print an email signature, we print the divider "--
\n", then the signature string, then two newlines.
Traditionally the signature is a one-liner (and the default
is just the git version), so the extra newline makes sense.

But one could easily specify a longer, multi-line signature,
like:

  git format-patch --signature='
  this is my long signature

  it has multiple lines
  ' ...

We should notice that it already has its own trailing
newline, and suppress one of ours.

Signed-off-by: Jeff King 
Signed-off-by: Jeremiah Mahler 
---
 builtin/log.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..5acc048 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -844,8 +844,13 @@ static void gen_message_id(struct rev_info *info, char 
*base)
 
 static void print_signature(void)
 {
-   if (signature && *signature)
-   printf("-- \n%s\n\n", signature);
+   if (!signature || !*signature)
+   return;
+
+   printf("-- \n%s", signature);
+   if (signature[strlen(signature)-1] != '\n')
+   putchar('\n');
+   putchar('\n');
 }
 
 static void add_branch_description(struct strbuf *buf, const char *branch_name)
-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler

--
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 v7 2/2] format-patch --signature-file

[Jeremiah Mahler]

Added option that allows a signature file to be used with format-patch
so that signatures with newlines and other special characters can be
easily included.

  $ git format-patch --signature-file ~/.signature -1

The config variable format.signaturefile is also provided so that it
can be added by default.

  $ git config format.signaturefile ~/.signature

  $ git format-patch -1

Signed-off-by: Jeremiah Mahler 
---
 Documentation/config.txt   |  4 
 Documentation/git-format-patch.txt |  4 
 builtin/log.c  | 16 
 t/t4014-format-patch.sh| 32 
 4 files changed, 56 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..140ed77 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1114,6 +1114,10 @@ format.signature::
Set this variable to the empty string ("") to suppress
signature generation.
 
+format.signaturefile::
+   Works just like format.signature except the contents of the
+   file specified by this variable will be used as the signature.
+
 format.suffix::
The default for format-patch is to output files with the suffix
`.patch`. Use this variable to change that suffix (make sure to
diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 5c0a4ab..c0fd470 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -14,6 +14,7 @@ SYNOPSIS
   [(--attach|--inline)[=] | --no-attach]
   [-s | --signoff]
   [--signature= | --no-signature]
+  [--signature-file=]
   [-n | --numbered | -N | --no-numbered]
   [--start-number ] [--numbered-files]
   [--in-reply-to=Message-Id] [--suffix=.]
@@ -233,6 +234,9 @@ configuration options in linkgit:git-notes[1] to use this 
workflow).
signature option is omitted the signature defaults to the Git version
number.
 
+--signature-file=::
+   Works just like --signature except the signature is read from a file.
+
 --suffix=.::
Instead of using `.patch` as the suffix for generated
filenames, use specified suffix.  A common alternative is
diff --git a/builtin/log.c b/builtin/log.c
index 5acc048..28d22fd 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -673,6 +673,7 @@ static void add_header(const char *value)
 static int thread;
 static int do_signoff;
 static const char *signature = git_version_string;
+static const char *signature_file;
 static int config_cover_letter;
 
 enum {
@@ -742,6 +743,8 @@ static int git_format_config(const char *var, const char 
*value, void *cb)
}
if (!strcmp(var, "format.signature"))
return git_config_string(&signature, var, value);
+   if (!strcmp(var, "format.signaturefile"))
+   return git_config_pathname(&signature_file, var, value);
if (!strcmp(var, "format.coverletter")) {
if (value && !strcasecmp(value, "auto")) {
config_cover_letter = COVER_AUTO;
@@ -1235,6 +1238,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
PARSE_OPT_OPTARG, thread_callback },
OPT_STRING(0, "signature", &signature, N_("signature"),
N_("add a signature")),
+   OPT_FILENAME(0, "signature-file", &signature_file,
+   N_("add a signature from a file")),
OPT__QUIET(&quiet, N_("don't print the patch filenames")),
OPT_END()
};
@@ -1452,6 +1457,17 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
cover_letter = (config_cover_letter == COVER_ON);
}
 
+   if (signature_file) {
+   if (signature && signature != git_version_string)
+   die(_("cannot specify both signature and 
signature-file"));
+
+   struct strbuf buf = STRBUF_INIT;
+
+   if (strbuf_read_file(&buf, signature_file, 128) < 0)
+   die_errno(_("unable to read signature file '%s'"), 
signature_file);
+   signature = strbuf_detach(&buf, NULL);
+   }
+
if (in_reply_to || thread || cover_letter)
rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
if (in_reply_to) {
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 9c80633..e604f65 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -762,6 +762,38 @@ test_expect_success 'format-patch --signature="" 
suppresses signatures' '
! grep "^-- \$&qu

Re: [PATCH v5] format-patch --signature-file

[Jeremiah Mahler]

On Wed, May 21, 2014 at 04:47:39PM -0400, Jeff King wrote:
> On Wed, May 21, 2014 at 11:26:18AM -0700, Junio C Hamano wrote:
> 
...
> I think the ratio of usefulness to number of words in this sub-thread is
> getting off. I'd be OK with any of:
...
> 
> -Peff

I think this has become a discussion about the airspeed of an unladen swallow.
:-)

I think we're close to a final patch. v7 sent.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 v6] format-patch --signature-file

[Jeremiah Mahler]

On Wed, May 21, 2014 at 02:24:27PM -0700, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
...
> 
> It seems you sent v7 before I had a chance to comment on this one.
> The above was merely "it would be nicer..." and I am OK as-is.  The
> comments on the rest are a bit more serious, though.
> 
> Thanks.
> 

It is not a problem.  Thanks again for all your feedback.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 v6] format-patch --signature-file

[Jeremiah Mahler]

On Wed, May 21, 2014 at 02:13:06PM -0700, Junio C Hamano wrote:
> Jeremiah Mahler  writes:
> 
...
> > ! grep "^-- \$" output
...
> 
> We have been trying not to do the above in recent test updates.  It
> would be nice if this set-up did not have to be outside of the usual
> test_expect_success structure.
> 

Jeff caught those "! grep" instances in my patch.

I can submit a separate patch to fix instances like those in test cases
unrelated to this signature-file patch.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 v6] format-patch --signature-file

[Jeremiah Mahler]

On Wed, May 21, 2014 at 02:58:42PM -0700, Junio C Hamano wrote:
> Jeremiah Mahler  writes:
> 
...
> The problem is a "cat" you added outside test_expect_*; the recent
> push is to have as little executable outside them, especially the
> "set-up" code to prepare for the real tests.
> 
> i.e. we have been trying to write new tests (and convert old ones)
> like this:
> 
> test_expect_success 'I test such and such ' '
> cat >input-for-test <<-\EOF &&
> here comes input
> EOF
> git command-to-be-tested actual &&
> cat >expected <<-\EOF &&
> here comes expected output
> EOF
> test_cmp expected actual
> '
> 
> not like this:
> 
> cat >input-for-test <<-\EOF &&
> here comes input
> EOF
> test_expect_success 'I test such and such ' '
> git command-to-be-tested actual &&
> cat >expected <<-\EOF &&
> here comes expected output
> EOF
> test_cmp expected actual
> '

Now I understand.

Below is one of the updated test cases.

test_expect_success 'format-patch --signature-file=mail-signature' '
cat >mail-signature <<-\EOF

Test User 
http://git.kernel.org/cgit/git/git.git

    git.kernel.org/?p=git/git.git;a=summary

EOF
git format-patch --stdout --signature-file=mail-signature -1 >output &&
check_patch output &&
sed -n -e "/^-- $/,\$p" output2 &&
test_cmp mail-signature output2
'

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 v6] format-patch --signature-file

[Jeremiah Mahler]

On Wed, May 21, 2014 at 03:15:55PM -0700, Junio C Hamano wrote:
> Jeff King  writes:
> 
> Yeah, placing it in its own setup may be the best.  There are quite
> a many set-ups outside the tests in this script from the olden days,
> so I am OK if left it as-is and have a separate clean-up patch after
> this topic settles.  I am also OK to add a new one "the new right way"
> so that a later clean-up patch does not have to change what is added
> in this step.

I like the idea of limiting the scope of this data so it couldn't
inadvertently impact later tests.

But placing the same data inside multiple test cases creates duplication.

Is there a way to define data once for a limited set of tests?

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 v6] format-patch --signature-file

[Jeremiah Mahler]

On Wed, May 21, 2014 at 03:37:11PM -0700, Junio C Hamano wrote:
> Jeremiah Mahler  writes:
> 
> > Below is one of the updated test cases.
> >
> > test_expect_success 'format-patch --signature-file=mail-signature' '
> > cat >mail-signature <<-\EOF
> >
> > Test User 
> > http://git.kernel.org/cgit/git/git.git
> >
> > git.kernel.org/?p=git/git.git;a=summary
> >
> > EOF
> > git format-patch --stdout --signature-file=mail-signature -1 >output &&
> > check_patch output &&
> > sed -n -e "/^-- $/,\$p" output2 &&
> > test_cmp mail-signature output2
> > '
> 
> Hmph, there are still few things I do not understand in the above.
> 
>  * Why does mail-signature file have a leading blank line?  Is it
>typical users would want to have one there?
> 
>  * Similarly, why does mail-signature file need a trailing blank
>line?  Is it usual users would want to have one there?
> 

I think whatever the user puts in their signature should be reproduced.
It is probably isn't common to have leading or trailing blank lines.
But if they are there they should be reproduced.

>  * Why three sed in the pipeline?  Wouldn't
> 
> sed -e "1,/^-- \$/d"  
>be more direct way to start the pipeline without having to strip
>the "-- \n" line with the extra one?
> 

Yes, that is much cleaner.

>  * Given a mail-signature file that does not end with an incomplete
>line (i.e. we did not have to add the newline to make it
>complete), what are the expected lines in the output after the
>"-- \n" separator?
> 
>Whatever it is, I think it is easier to read the tests if done
>like so:
> 
>  sed -e "1,/^-- \$/d" actual &&
>  {
>  do something to turn mail-signature to what we expect
>  } >expect &&
>  test_cmp expect actual
> 
>If that "do something" is "to append an extra newline", then it
>would make it a lot clear to do
> 
>  {
>  cat mail-signature && echo
>  } >expect
> 
>than a 'sed -e "\$d"' tucked at the end of the pipeline that only
>tells us we are removing one line that has _something_ without
>explicitly saying what we are removing, no?
> 

This does make more sense.  Since Git prints a blank line when it prints
the signature, this test also adds a blank line to produce the same
expected result.  This is much better than deleting two lines for no
obvious reason.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 v8 0/2] format-patch --signature-file

[Jeremiah Mahler]

v8 of patch to add format-patch --signature-file  option.

This revision includes more suggestions from Jeff King and Junio C Hamano.

  - Renamed variable which stores the signature to 'mail-signature'
which better describes what is being stored.

  - Fixed a declaration after code (forbidden by ISO C90).

  - Simplified sed usage in test cases from a pipeline with
three stages to just one.

  - Enclosed the creation of test data ('cat >mail-signature') inside
a test_expect_* case.

  - Test cases now add one blank line to the actual signature to match
the signature output of Git.  Previously, the last of the output
was deleted.

Thanks again for all the suggestions.

Jeff King (1):
  format-patch: make newline after signature conditional

Jeremiah Mahler (1):
  format-patch --signature-file 

 Documentation/config.txt   |  4 
 Documentation/git-format-patch.txt |  4 
 builtin/log.c  | 25 +--
 t/t4014-format-patch.sh| 41 ++
 4 files changed, 72 insertions(+), 2 deletions(-)

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler

--
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 v8 1/2] format-patch: make newline after signature conditional

[Jeremiah Mahler]

From: Jeff King 

When we print an email signature, we print the divider "--
\n", then the signature string, then two newlines.
Traditionally the signature is a one-liner (and the default
is just the git version), so the extra newline makes sense.

But one could easily specify a longer, multi-line signature,
like:

  git format-patch --signature='
  this is my long signature

  it has multiple lines
  ' ...

We should notice that it already has its own trailing
newline, and suppress one of ours.

Signed-off-by: Jeff King 
Signed-off-by: Jeremiah Mahler 
---
 builtin/log.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..5acc048 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -844,8 +844,13 @@ static void gen_message_id(struct rev_info *info, char 
*base)
 
 static void print_signature(void)
 {
-   if (signature && *signature)
-   printf("-- \n%s\n\n", signature);
+   if (!signature || !*signature)
+   return;
+
+   printf("-- \n%s", signature);
+   if (signature[strlen(signature)-1] != '\n')
+   putchar('\n');
+   putchar('\n');
 }
 
 static void add_branch_description(struct strbuf *buf, const char *branch_name)
-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler

--
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 v8 2/2] format-patch --signature-file

[Jeremiah Mahler]

Added option that allows a signature file to be used with format-patch
so that signatures with newlines and other special characters can be
easily included.

  $ git format-patch --signature-file ~/.signature -1

The config variable format.signaturefile is also provided so that it
can be added by default.

  $ git config format.signaturefile ~/.signature

  $ git format-patch -1

Signed-off-by: Jeremiah Mahler 
---
 Documentation/config.txt   |  4 
 Documentation/git-format-patch.txt |  4 
 builtin/log.c  | 16 +++
 t/t4014-format-patch.sh| 41 ++
 4 files changed, 65 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..140ed77 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1114,6 +1114,10 @@ format.signature::
Set this variable to the empty string ("") to suppress
signature generation.
 
+format.signaturefile::
+   Works just like format.signature except the contents of the
+   file specified by this variable will be used as the signature.
+
 format.suffix::
The default for format-patch is to output files with the suffix
`.patch`. Use this variable to change that suffix (make sure to
diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 5c0a4ab..c0fd470 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -14,6 +14,7 @@ SYNOPSIS
   [(--attach|--inline)[=] | --no-attach]
   [-s | --signoff]
   [--signature= | --no-signature]
+  [--signature-file=]
   [-n | --numbered | -N | --no-numbered]
   [--start-number ] [--numbered-files]
   [--in-reply-to=Message-Id] [--suffix=.]
@@ -233,6 +234,9 @@ configuration options in linkgit:git-notes[1] to use this 
workflow).
signature option is omitted the signature defaults to the Git version
number.
 
+--signature-file=::
+   Works just like --signature except the signature is read from a file.
+
 --suffix=.::
Instead of using `.patch` as the suffix for generated
filenames, use specified suffix.  A common alternative is
diff --git a/builtin/log.c b/builtin/log.c
index 5acc048..5e3cc29 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -673,6 +673,7 @@ static void add_header(const char *value)
 static int thread;
 static int do_signoff;
 static const char *signature = git_version_string;
+static const char *signature_file;
 static int config_cover_letter;
 
 enum {
@@ -742,6 +743,8 @@ static int git_format_config(const char *var, const char 
*value, void *cb)
}
if (!strcmp(var, "format.signature"))
return git_config_string(&signature, var, value);
+   if (!strcmp(var, "format.signaturefile"))
+   return git_config_pathname(&signature_file, var, value);
if (!strcmp(var, "format.coverletter")) {
if (value && !strcasecmp(value, "auto")) {
config_cover_letter = COVER_AUTO;
@@ -1235,6 +1238,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
PARSE_OPT_OPTARG, thread_callback },
OPT_STRING(0, "signature", &signature, N_("signature"),
N_("add a signature")),
+   OPT_FILENAME(0, "signature-file", &signature_file,
+   N_("add a signature from a file")),
OPT__QUIET(&quiet, N_("don't print the patch filenames")),
OPT_END()
};
@@ -1452,6 +1457,17 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
cover_letter = (config_cover_letter == COVER_ON);
}
 
+   if (signature_file) {
+   struct strbuf buf = STRBUF_INIT;
+
+   if (signature && signature != git_version_string)
+   die(_("cannot specify both signature and 
signature-file"));
+
+   if (strbuf_read_file(&buf, signature_file, 128) < 0)
+   die_errno(_("unable to read signature file '%s'"), 
signature_file);
+   signature = strbuf_detach(&buf, NULL);
+   }
+
if (in_reply_to || thread || cover_letter)
rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
if (in_reply_to) {
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 9c80633..37d25c4 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -762,6 +762,47 @@ test_expect_success 'format-patch --signature="" 
suppresses signatures' '
! grep "^-- \

Re: [PATCH v8 0/2] format-patch --signature-file

[Jeremiah Mahler]

Junio,

On Wed, May 21, 2014 at 06:53:07PM -0700, Jeremiah Mahler wrote:
> v8 of patch to add format-patch --signature-file  option.
> 
...
> 

I just notice that my patch is in 'pu'.
But it is version 7 instead of the improved version 8.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 v8 0/2] format-patch --signature-file

[Jeremiah Mahler]

Junio,

On Thu, May 22, 2014 at 12:00:39PM -0700, Junio C Hamano wrote:
> Jeremiah Mahler  writes:
> 
> > I just notice that my patch is in 'pu'.
> > But it is version 7 instead of the improved version 8.
> 
> Yeah, I know.  In a distributed environment, multiple people work
> independently and a sequence of event can go like this:
> 
>  - I read v7, comment, and queue it only so that I won't forget;
>  - I attend to other topics;
>  - I start the day's integration cycle, merging topics to the
>integration branches, maint, master, next and pu;
>  - You reroll v8 and post it;
>  - I may not notice v8, or I may notice v8 but think it is not
>worth to discard the integration work so far only to replace
>v7 with v8.
>  - The integration result is pushed out.
> 
> A reminder is very much appreciated, but on the other hand it is not
> something unusual.  It happens all the time, and I usually am aware
> of it ;-)
> 
> Thanks.

Thanks for explaining how this process works.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 v8 2/2] format-patch --signature-file

[Jeremiah Mahler]

Junio,

On Thu, May 22, 2014 at 01:52:45PM -0700, Junio C Hamano wrote:
> Jeremiah Mahler  writes:
> 
...
>
> Something like:
> 
> To countermand the configuration variable for a specific run:
> 
>   $ git format-patch -1 --signature="This time only"
> $ git format-patch -1 --signature;# to use the default
> $ git format-patch -1 --signature="" ;# to add nothing
> 
...
>
> I didn't see offhand if the tests make sure that a configured mail
> signature can be overriden from the command line.  I think you would
> want to test, with format-patch.signature-file pointing at the
> mail-signature file, at least these three cases:
> 
>  - Run "format-patch --no-signature" and make sure that stops the
>contents from mail-signature file from being shown, and instead
>no mail-signature is given.
> 
--no-signature should inhibit all signatures.

>  - Run "format-patch --signature='this time only'" and make sure
>that stops the contents from mail-signature file from being shown
>and "this time only" is used instead.
> 
>  - Run "format-patch --signature-file=another-mail-signature" and
>make sure that stops the contents from mail-signature file from
>being shown and the contents from the other file is used instead.
> 
Arguments on the command line should take precendence over
anything in the config.

Your comments make it clear that I have not accounted for all the possible
cases.  Below is a table of all the reasonable cases.  It should account
for cases you mentioned as well as others.

  Key:
  ---
  default: Git version number
  sig1: .signature from column 1
  file1: .signaturefile from column 1
  sig2: --signature from column 2
  file2: --signature-file in column 2

A preceeding 'format.' is assumed for config.  .signature -> format.signature
Command line arguments take precedence over config options.

|+-+-|
| config  (1)| argv  (2)   | |
|+-+-|
|| | default |
|| --signature | sig2|
|| --signature-file| file2   |
|| --no-signature  | none|
|| --no-signature-file | none|
|| --signature, --signature-file   | die |
|| --signature, --no-signature-file| sig2|
|| --signature-file, --no-signature| none|
|| --no-signature, --no-signature-file | none|
| .signature | | sig1|
| .signature | --signature | sig2|
| .signature | --signature-file| file2   |
| .signature | --no-signature  | none|
| .signature | --no-signature-file | sig1|
| .signature | --signature, --signature-file   | die |
| .signature | --signature, --no-signature-file| sig2|
| .signature | --signature-file, --no-signature| none|
| .signature | --no-signature, --no-signature-file | none|
| .signaturefile | | file1   |
| .signaturefile | --signature | sig2|
| .signaturefile | --signature-file| file2   |
| .signaturefile | --no-signature  | none|
| .signaturefile | --no-signature-file | default |
| .signaturefile | --signature, --signature-file   | die |
| .signaturefile | --signature, --no-signature-file| sig2|
| .signaturefile | --signature-file, --no-signature| none|
| .signaturefile | --no-signature, --no-signature-file | none|
| .signature, .signaturefile | | die |
| .signature, .signaturefile | --signature | sig2|
| .signature, .signaturefile | --signature-file| file2   |
| .signature, .signaturefile | --no-signature  | none|
| .signature, .signaturefile | --no-signature-file | sig1|
| .signature, .signaturefile | --signature, --signature-file   | die |
| .signature, .signaturefile | --signature, --no-signatu

[PATCH v9 2/2] format-patch --signature-file=

[Jeremiah Mahler]

Add an option to format-patch for reading a signature from a file.

  $ git format-patch -1 --signature-file=$HOME/.signature

The config variable `format.signaturefile` can also be used to make
this the default.

  $ git config format.signaturefile $HOME/.signature

  $ git format-patch -1

Signed-off-by: Jeremiah Mahler 
---
 Documentation/config.txt   |  4 +++
 Documentation/git-format-patch.txt |  4 +++
 builtin/log.c  | 17 +++
 t/t4014-format-patch.sh| 61 ++
 4 files changed, 86 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..140ed77 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1114,6 +1114,10 @@ format.signature::
Set this variable to the empty string ("") to suppress
signature generation.
 
+format.signaturefile::
+   Works just like format.signature except the contents of the
+   file specified by this variable will be used as the signature.
+
 format.suffix::
The default for format-patch is to output files with the suffix
`.patch`. Use this variable to change that suffix (make sure to
diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 5c0a4ab..c0fd470 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -14,6 +14,7 @@ SYNOPSIS
   [(--attach|--inline)[=] | --no-attach]
   [-s | --signoff]
   [--signature= | --no-signature]
+  [--signature-file=]
   [-n | --numbered | -N | --no-numbered]
   [--start-number ] [--numbered-files]
   [--in-reply-to=Message-Id] [--suffix=.]
@@ -233,6 +234,9 @@ configuration options in linkgit:git-notes[1] to use this 
workflow).
signature option is omitted the signature defaults to the Git version
number.
 
+--signature-file=::
+   Works just like --signature except the signature is read from a file.
+
 --suffix=.::
Instead of using `.patch` as the suffix for generated
filenames, use specified suffix.  A common alternative is
diff --git a/builtin/log.c b/builtin/log.c
index 5acc048..56cad39 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -673,6 +673,7 @@ static void add_header(const char *value)
 static int thread;
 static int do_signoff;
 static const char *signature = git_version_string;
+static const char *signature_file;
 static int config_cover_letter;
 
 enum {
@@ -742,6 +743,8 @@ static int git_format_config(const char *var, const char 
*value, void *cb)
}
if (!strcmp(var, "format.signature"))
return git_config_string(&signature, var, value);
+   if (!strcmp(var, "format.signaturefile"))
+   return git_config_pathname(&signature_file, var, value);
if (!strcmp(var, "format.coverletter")) {
if (value && !strcasecmp(value, "auto")) {
config_cover_letter = COVER_AUTO;
@@ -1235,6 +1238,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
PARSE_OPT_OPTARG, thread_callback },
OPT_STRING(0, "signature", &signature, N_("signature"),
N_("add a signature")),
+   OPT_FILENAME(0, "signature-file", &signature_file,
+   N_("add a signature from a file")),
OPT__QUIET(&quiet, N_("don't print the patch filenames")),
OPT_END()
};
@@ -1452,6 +1457,18 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
cover_letter = (config_cover_letter == COVER_ON);
}
 
+   if (!signature) {
+   /* --no-signature inhibits all signatures */
+   } else if (signature && signature != git_version_string) {
+   /* non-default signature already set */
+   } else if (signature_file) {
+   struct strbuf buf = STRBUF_INIT;
+
+   if (strbuf_read_file(&buf, signature_file, 128) < 0)
+   die_errno(_("unable to read signature file '%s'"), 
signature_file);
+   signature = strbuf_detach(&buf, NULL);
+   }
+
if (in_reply_to || thread || cover_letter)
rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
if (in_reply_to) {
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 9c80633..ae25353 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -762,6 +762,67 @@ test_expect_success 'format-patch --signature="" 
suppresses signatures' '
! grep "^-- \$" output
 '
 

[PATCH v9 0/2] format-patch --signature-file=

[Jeremiah Mahler]

v9 of patch to add format-patch --signature-file  option.

This revision includes more suggestions from Junio C Hamano.

  - Reworded patch description.  Using "special characters" is
not a feature of this patch.

  - Changed patch description to follow gitcli(7) guidlines.
"--signature-file=name" instead of "--signature-file name"

  - Reduced set of crucial test cases.

--signature-file works

format.signaturefile works

--no-signature suppresses format.signaturefile

--signature-file overrides format.signaturefile

--signature overrides format.signaturefile

  - Note, the control logic had to be re-worked to satisfy these test cases.


Jeff King (1):
  format-patch: make newline after signature conditional

Jeremiah Mahler (1):
  format-patch --signature-file=

 Documentation/config.txt   |  4 +++
 Documentation/git-format-patch.txt |  4 +++
 builtin/log.c  | 26 ++--
 t/t4014-format-patch.sh| 61 ++
 4 files changed, 93 insertions(+), 2 deletions(-)

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler

--
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 v9 1/2] format-patch: make newline after signature conditional

[Jeremiah Mahler]

From: Jeff King 

When we print an email signature, we print the divider "--
\n", then the signature string, then two newlines.
Traditionally the signature is a one-liner (and the default
is just the git version), so the extra newline makes sense.

But one could easily specify a longer, multi-line signature,
like:

  git format-patch --signature='
  this is my long signature

  it has multiple lines
  ' ...

We should notice that it already has its own trailing
newline, and suppress one of ours.

Signed-off-by: Jeff King 
Signed-off-by: Jeremiah Mahler 
---
 builtin/log.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..5acc048 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -844,8 +844,13 @@ static void gen_message_id(struct rev_info *info, char 
*base)
 
 static void print_signature(void)
 {
-   if (signature && *signature)
-   printf("-- \n%s\n\n", signature);
+   if (!signature || !*signature)
+   return;
+
+   printf("-- \n%s", signature);
+   if (signature[strlen(signature)-1] != '\n')
+   putchar('\n');
+   putchar('\n');
 }
 
 static void add_branch_description(struct strbuf *buf, const char *branch_name)
-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler

--
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: commit: support commit.verbose and --no-verbose

[Jeremiah Mahler]

On Fri, May 23, 2014 at 04:22:22PM -0500, Caleb Thompson wrote:
> This patch allows people to set `commit.verbose` to implicitly send 
> `--verbose`
...
>  
> +cat >check-for-no-diff < +#!$SHELL_PATH
> +exec grep -v '^diff --git' "\$1"
> +EOF
> +chmod +x check-for-no-diff
> +

For new tests, commands like this should be placed inside a
test_expect_success structure.  However, I can see why you did it this
way since the code just above it does it this way.
Perhaps others will have some recommendations.

Also, <<\-EOF is used instead of <check-for-no-diff <<\-EOF &&
#!SHELL_PATH
exec grep -v '^diff --git' "\$1"
EOF
chmod +x check-for-no-diff
'

>  
> +test_expect_success 'commit shows verbose diff with set commit.verbose' '
> + echo morecontent >file &&
> + git add file &&
> + git config commit.verbose true &&
> + check_message message
> +'

'test_config' should be used to set config variables since it
also takes care of un-setting them when the test is complete.

test_expect_success 'commit shows verbose diff with set commit.verbose' '
echo morecontent >file &&
git add file &&
test_config commit.verbose true &&
check_message message
'

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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] wording fixes in the user manual and glossary

[Jeremiah Mahler]

Some minor wording fixes in the user manual and glossary.

Signed-off-by: Jeremiah Mahler 
---
 Documentation/glossary-content.txt | 2 +-
 Documentation/user-manual.txt  | 8 
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/glossary-content.txt 
b/Documentation/glossary-content.txt
index be0858c..4e0b971 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -1,7 +1,7 @@
 [[def_alternate_object_database]]alternate object database::
Via the alternates mechanism, a <>
can inherit part of its <>
-   from another object database, which is called "alternate".
+   from another object database, which is called an "alternate".
 
 [[def_bare_repository]]bare repository::
A bare repository is normally an appropriately
diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index d33f884..efb3c97 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -416,7 +416,7 @@ REVISIONS" section of linkgit:gitrevisions[7].
 Updating a repository with git fetch
 
 
-Eventually the developer cloned from will do additional work in her
+Eventually the developer will do additional work in her cloned
 repository, creating new commits and advancing the branches to point
 at the new commits.
 
@@ -1811,8 +1811,8 @@ manner.
 You can then import these into your mail client and send them by
 hand.  However, if you have a lot to send at once, you may prefer to
 use the linkgit:git-send-email[1] script to automate the process.
-Consult the mailing list for your project first to determine how they
-prefer such patches be handled.
+Consult the mailing list for your project first to determine
+their requirements for submitting patches.
 
 [[importing-patches]]
 Importing patches to a project
@@ -2255,7 +2255,7 @@ $ git checkout test && git merge speed-up-spinlocks
 It is unlikely that you would have any conflicts here ... but you might if you
 spent a while on this step and had also pulled new versions from upstream.
 
-Some time later when enough time has passed and testing done, you can pull the
+Sometime later when enough time has passed and testing done, you can pull the
 same branch into the `release` tree ready to go upstream.  This is where you
 see the value of keeping each patch (or patch series) in its own branch.  It
 means that the patches can be moved into the `release` tree in any order.
-- 
2.0.0.rc4.479.gd6c9c28

--
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] commit: support commit.verbose and --no-verbose

[Jeremiah Mahler]

On Sun, May 25, 2014 at 01:24:27AM -0500, Caleb Thompson wrote:
...
>   would be committed at the bottom of the commit message
>   template.  Note that this diff output doesn't have its
> - lines prefixed with '#'.
> + lines prefixed with '#'.  The `commit.verbose` configuration
> + variable can be set to true to implicitly send this option.
> +
> +--no-verbose::
> + Do not show the unified diff  at the bottom of the commit message
> + template.  This is the default behavior, but can be used to override
> + the`commit.verbose` configuration variable.
> 
Why is there two spaces between "diff  at"?
Needs a space between "the`comm" -> "the `comm".

> +cat >check-for-no-diff < +#!$SHELL_PATH
> +exec grep -v '^diff --git' "\$1"
> +EOF
> +chmod +x check-for-no-diff
> +
Me personally, I would leave it like that for now, since that is
the style being used nearby.  We'll see what others have to say.

I certainly wouldn't convert all the other cases to use
test_expect_success.  Leave that for another patch.

> 
> @@ -48,6 +54,21 @@ test_expect_success 'verbose diff is stripped out 
> (mnemonicprefix)' '
>   check_message message
>  '
> 
> +test_expect_success 'commit shows verbose diff with set commit.verbose' '
> + echo morecontent >file &&
> + git add file &&
> + test_config commit.verbose true &&
> + check_message message
> +'
> +
> +test_expect_success 'commit does not show verbose diff with --no-verbose' '
> + echo morecontent >file &&
> +     git add file &&
> + test_config commit.verbose true &&
> + test_set_editor "$PWD/check-for-no-diff" &&
> + git commit --amend --no-verbose
> +'
> +
I like those better with 'test_config' instead of 'git config', good.

Keep working on it, it is looking better :-)

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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] commit: support commit.verbose and --no-verbose

[Jeremiah Mahler]

On Sun, May 25, 2014 at 01:24:27AM -0500, Caleb Thompson wrote:
> Incorporated changes from Duy Nguyen and Jeremiah Mahler.
> 
...
> 
> +test_expect_success 'commit shows verbose diff with set commit.verbose' '
> + echo morecontent >file &&
> + git add file &&
> + test_config commit.verbose true &&
> + check_message message
> +'

This test case doesn't appear to be checking for the verbose output.
No commit is made so it can't check for the presence of a diff.
"check_message message" passes as it did in the test above this (not shown).

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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] wording fixes in the user manual and glossary

[Jeremiah Mahler]

On Sun, May 25, 2014 at 07:56:41PM +1200, Chris Packham wrote:
> On 25/05/14 15:50, Jeremiah Mahler wrote:
> > Some minor wording fixes in the user manual and glossary.
...
> >  
> > -Eventually the developer cloned from will do additional work in her
> > +Eventually the developer will do additional work in her cloned
> >  repository, creating new commits and advancing the branches to point
> >  at the new commits.
> 
> I agree that the original wording isn't clear but I'm not sure the new
> wording is any clearer. The paragraph is trying to explain how to fetch
> upstream changes when they happen. My initial thought was to say
> "Eventually the developer will do additional work in the upstream
> repository" but perhaps it is to early to start throwing around terms
> like upstream. Perhaps just saying "her repository" would be clearest.
> 

The "developer cloned from will do" didn't sound right to me.
But it sounds like my interpretation is not what you were trying to
convey.  I do like the "upstream" term, it helps describe the
local/remote aspect.

How about this:

  Eventually the upstream developer will do additional work in their
  repository.  Creating new commits and advancing the branches to point
  at the new commits.

Then in the next paragraph it will discuss how to use `git fetch`
to get these remote changes in to the local repository.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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] t9138-git-svn-authors-prog.sh fixups

[Jeremiah Mahler]

Several fixups of the t9138-git-svn-authors-prog.sh test script to
follow current recommendations in t/README.

  - Fixed a Perl script with a full "#!/usr/bin/perl" shebang
to use write_script() and $PERL_PATH as per t/README.

  - Placed svn-authors data setup inside a test_expect_success.

  - Fixed trailing quotes to use the same indentation throughout.

Signed-off-by: Jeremiah Mahler 
---
 t/t9138-git-svn-authors-prog.sh | 35 +--
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/t/t9138-git-svn-authors-prog.sh b/t/t9138-git-svn-authors-prog.sh
index 83cc5fc..d54c37a 100755
--- a/t/t9138-git-svn-authors-prog.sh
+++ b/t/t9138-git-svn-authors-prog.sh
@@ -7,40 +7,39 @@ test_description='git svn authors prog tests'
 
 . ./lib-git-svn.sh
 
-cat > svn-authors-prog <<'EOF'
-#!/usr/bin/perl
-$_ = shift;
-if (s/-sub$//)  {
-   print "$_ <$_\@sub.example.com>\n";
-}
-else {
-   print "$_ <$_\@example.com>\n";
-}
+write_script svn-authors-prog $PERL_PATH <<-\EOF
+   $_ = shift;
+   if (s/-sub$//)  {
+   print "$_ <$_\@sub.example.com>\n";
+   } else {
+   print "$_ <$_\@example.com>\n";
+   }
 EOF
-chmod +x svn-authors-prog
 
-cat > svn-authors <<'EOF'
-ff = FFF FFF 
-EOF
+test_expect_success 'svn-authors setup' '
+   cat >svn-authors <<-\EOF
+   ff = FFF FFF 
+   EOF
+'
 
 test_expect_success 'setup svnrepo' '
for i in aa bb cc-sub dd-sub ee-foo ff
do
svn mkdir -m $i --username $i "$svnrepo"/$i
done
-   '
+'
 
 test_expect_success 'import authors with prog and file' '
git svn clone --authors-prog=./svn-authors-prog \
--authors-file=svn-authors "$svnrepo" x
-   '
+'
 
 test_expect_success 'imported 6 revisions successfully' '
(
cd x
test "`git rev-list refs/remotes/git-svn | wc -l`" -eq 6
)
-   '
+'
 
 test_expect_success 'authors-prog ran correctly' '
(
@@ -56,7 +55,7 @@ test_expect_success 'authors-prog ran correctly' '
git rev-list -1 --pretty=raw refs/remotes/git-svn~5 | \
  grep "^author aa  "
)
-   '
+'
 
 test_expect_success 'authors-file overrode authors-prog' '
(
@@ -64,7 +63,7 @@ test_expect_success 'authors-file overrode authors-prog' '
git rev-list -1 --pretty=raw refs/remotes/git-svn | \
  grep "^author FFF FFF  "
)
-   '
+'
 
 git --git-dir=x/.git config --unset svn.authorsfile
 git --git-dir=x/.git config --unset svn.authorsprog
-- 
2.0.0.rc4.1.g4a28f16.dirty

--
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] wording fixes in the user manual and glossary

[Jeremiah Mahler]

Various minor wording fixes throughout the user manual
and glossary.

The section on "Updating a repository with git fetch" was
substantially re-worded to try and better explain `git fetch`.

Signed-off-by: Jeremiah Mahler 
---

Notes:
From the feedback I received by Chris Packham [1] it was clear
that my re-wording of the section "Updating a repository with git fetch"
still wasn't quite right [1].

[1]: http://marc.info/?l=git&m=140100460903936&w=2

I re-worded it some more to try and emphasize the remote (upstream)
and local aspects of `git fetch`.  Chris liked those changes better [2].

[2]: http://marc.info/?l=git&m=140109062903038&w=2

I expanded upon this even further.  The section on git-pull is similar
so I tried to use that as a basis.  I also thought the relationship between
git fetch and git pull was worthy of a short note along with a link to
the section on git-pull.

 Documentation/glossary-content.txt |  2 +-
 Documentation/user-manual.txt  | 28 ++--
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/Documentation/glossary-content.txt 
b/Documentation/glossary-content.txt
index be0858c..4e0b971 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -1,7 +1,7 @@
 [[def_alternate_object_database]]alternate object database::
Via the alternates mechanism, a <>
can inherit part of its <>
-   from another object database, which is called "alternate".
+   from another object database, which is called an "alternate".
 
 [[def_bare_repository]]bare repository::
A bare repository is normally an appropriately
diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index d33f884..f5fd61b 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -416,14 +416,22 @@ REVISIONS" section of linkgit:gitrevisions[7].
 Updating a repository with git fetch
 
 
-Eventually the developer cloned from will do additional work in her
-repository, creating new commits and advancing the branches to point
-at the new commits.
+After you clone a repository and commit a few changes of your own, you
+may wish to check the original repository for updates.
+The linkgit:git-fetch[1] command is used to update all the remote-tracking
+branches to the latest version found in those repositories.
+It will not touch any of your own branches--not even the "master"
+branch that was created during clone.
+The linkgit:git-merge[1] command can then be used to merge the changes.
+
+-
+$ git fetch
+$ git merge origin/master
+-
 
-The command `git fetch`, with no arguments, will update all of the
-remote-tracking branches to the latest version found in her
-repository.  It will not touch any of your own branches--not even the
-"master" branch that was created for you on clone.
+The linkgit:git-pull[1] command,
+<>,
+performs both of these steps, a fetch followed by a merge.
 
 [[fetching-branches]]
 Fetching branches from other repositories
@@ -1811,8 +1819,8 @@ manner.
 You can then import these into your mail client and send them by
 hand.  However, if you have a lot to send at once, you may prefer to
 use the linkgit:git-send-email[1] script to automate the process.
-Consult the mailing list for your project first to determine how they
-prefer such patches be handled.
+Consult the mailing list for your project first to determine
+their requirements for submitting patches.
 
 [[importing-patches]]
 Importing patches to a project
@@ -2255,7 +2263,7 @@ $ git checkout test && git merge speed-up-spinlocks
 It is unlikely that you would have any conflicts here ... but you might if you
 spent a while on this step and had also pulled new versions from upstream.
 
-Some time later when enough time has passed and testing done, you can pull the
+Sometime later when enough time has passed and testing done, you can pull the
 same branch into the `release` tree ready to go upstream.  This is where you
 see the value of keeping each patch (or patch series) in its own branch.  It
 means that the patches can be moved into the `release` tree in any order.
-- 
2.0.0.rc4.480.gad29d77

--
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 5/5] commit: support commit.verbose and --no-verbose

[Jeremiah Mahler]

j
On Mon, May 26, 2014 at 01:56:26PM -0500, Caleb Thompson wrote:
> Add a new configuration variable commit.verbose to implicitly pass
>  
...
> +test_expect_success 'commit shows verbose diff with set commit.verbose=true' 
> '
> + echo morecontent >>file &&
> + git add file &&
> + test_config commit.verbose true &&
> + test_set_editor "$(pwd)/check-for-diff" &&
> + git commit --amend
> +'
> +
> +test_expect_success 'commit --verbose overrides verbose=false' '
> + echo evenmorecontent >>file &&
> + git add file &&
> + test_config commit.verbose false  &&
> + test_set_editor "$(pwd)/check-for-diff" &&
> + git commit --amend --verbose
> +'
> +
> +test_expect_success 'commit does not show verbose diff with 
> commit.verbose=false' '
> + echo evenmorecontent >>file &&
> + git add file &&
> + test_config commit.verbose false &&
> + test_set_editor "$(pwd)/check-for-no-diff" &&
> + git commit --amend
> +'
> +
> +test_expect_success 'commit --no-verbose overrides commit.verbose=true' '
> + echo evenmorecontent >>file &&
> + git add file &&
> + test_config commit.verbose true &&
> + test_set_editor "$(pwd)/check-for-no-diff" &&
> + git commit --amend --no-verbose
> +'
> +
...
> 

It appears that these tests still aren't checking to see if the
"verbose" output appears in the commit message.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 5/5] commit: support commit.verbose and --no-verbose

[Jeremiah Mahler]

On Mon, May 26, 2014 at 03:39:55PM -0500, Caleb Thompson wrote:
> The editors, `check-for-diff` and `check-for-no-diffs`, are grepping for
> the output and lack thereof, respectively.
...
> >
> > It appears that these tests still aren't checking to see if the
> > "verbose" output appears in the commit message.
> >
> >

OK, got it.  The editor, set by test_set_editor, is run as part of
the commit.  Thanks for explaining that.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 5/5] commit: support commit.verbose and --no-verbose

[Jeremiah Mahler]

Caleb,

On Mon, May 26, 2014 at 01:56:26PM -0500, Caleb Thompson wrote:
> Add a new configuration variable commit.verbose to implicitly pass
> `--verbose` to `git-commit`. Add `--no-verbose` to commit to negate that
> setting.
> 
> Signed-off-by: Caleb Thompson 
> ---
>  Documentation/config.txt   |  5 +
>  '
...
>  
> +test_expect_success 'commit shows verbose diff with set commit.verbose=true' 
> '
> + echo morecontent >>file &&
...
> +'
> +
> +test_expect_success 'commit --verbose overrides verbose=false' '
> + echo evenmorecontent >>file &&
...
> +
> +test_expect_success 'commit does not show verbose diff with 
> commit.verbose=false' '
> + echo evenmorecontent >>file &&
...
> +'
> +
> +test_expect_success 'commit --no-verbose overrides commit.verbose=true' '
> + echo evenmorecontent >>file &&
...
> +'
> +
>  

Some minor style nits...

Use a consistent naming convention for your tests.  verbose=false looks
different than commit.verbose=false at first glance.  Also, since
"commit.verbose=false" is an invalid syntax for a config option, I would
remove the '=' and just make it "commit.verbose false".

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 0/5] commit: support commit.verbose and --no-verbose

[Jeremiah Mahler]

Caleb,

On Mon, May 26, 2014 at 01:56:21PM -0500, Caleb Thompson wrote:
> This patch allows people to set commit.verbose to implicitly send
> --verbose to git-commit. It also introduces --no-verbose to
> override the configuration setting.
> 
> This version incorporates changes suggested by Eric Sunshine, Duy
> Nguyen, and Jeremiah Mahler.
> 
...
> 

Other than the minor style issue I pointed out in another email, it looks
good, and the patch set works properly on my machine.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 01/15] builtin/add.c: rearrange xcalloc arguments

[Jeremiah Mahler]

Brian,

On Tue, May 27, 2014 at 12:33:42AM +0900, Brian Gesiak wrote:
> xcalloc takes two arguments: the number of elements and their size.
> run_add_interactive passes the arguments in reverse order, passing the
> size of a char*, followed by the number of char* to be allocated.
> Rearrgange them so they are in the correct order.
> 
> Signed-off-by: Brian Gesiak 
> ---
>  builtin/add.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/builtin/add.c b/builtin/add.c
> index 672adc0..488acf4 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -248,7 +248,7 @@ int run_add_interactive(const char *revision, const char 
> *patch_mode,
>   int status, ac, i;
>   const char **args;
>  
> - args = xcalloc(sizeof(const char *), (pathspec->nr + 6));
> + args = xcalloc((pathspec->nr + 6), sizeof(const char *));
>   ac = 0;
>   args[ac++] = "add--interactive";
>   if (patch_mode)
> 

This patch doesn't apply to any of the branches I have available
(master, pu, next).  And there is no line containing "pathspec->nr + 6"
anywhere in my builtin/add.c.  Which branch is your work based off?

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 00/15] Rearrange xcalloc arguments

[Jeremiah Mahler]

Brian,

On Tue, May 27, 2014 at 12:33:41AM +0900, Brian Gesiak wrote:
> xcalloc takes two arguments: the number of elements and their size.
> The vast majority of the Git codebase passes these arguments in the
> correct order, but there are some exceptions. This patch series
> corrects those exceptions.
> 

Let me see if I understand the issue underlying this patch set.

xcalloc works like calloc and takes two arguments, the number of
elements and the size of each element.  However, many calls specified
these arguments in the reverse order.  It didn't produce a compile
error because both arguments are the same type.  And it didn't produce
a run time error because A*B is the same as B*A.

If this behaved like dd, performance would be different depending on the
order.

  dd if=in of=out bs=1count=1024
  dd if=in of=out bs=1024 count=1

Nonetheless, it appears to be a good fix.  Nice job!

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 01/15] builtin/add.c: rearrange xcalloc arguments

[Jeremiah Mahler]

On Tue, May 27, 2014 at 11:22:00AM +0900, Brian Gesiak wrote:
> My apologies! I based my work off of maint, branching off of eea591.
> 
> My reasoning was that Documentation/SubmittingPatches states that "a
> bugfix should be based on 'maint'". [1] Now that I think about it,
> this is probably not the kind of "bug" that statement had in mind.
> 
> Should I reroll the patch based on master?
> 
> - Brian Gesiak
> 
> [1] 
> https://github.com/git/git/blob/4a28f169ad29ba452e0e7bea2583914c10c58322/Documentation/SubmittingPatches#L9
> 

OK, got it.  I should have read Documentation/SubmittingPatches more
closely like you did :-)  No need to reroll I can just use the maint
branch to test it out.  Thanks!

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 2/5] commit test: Change $PWD to $(pwd)

[Jeremiah Mahler]

On Tue, May 27, 2014 at 07:46:59AM +0200, Johannes Sixt wrote:
> Am 5/26/2014 20:56, schrieb Caleb Thompson:
> > Signed-off-by: Caleb Thompson 
> > ---
> >  t/t7507-commit-verbose.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> > index 6d778ed..3b06d73 100755
> > --- a/t/t7507-commit-verbose.sh
> > +++ b/t/t7507-commit-verbose.sh
> > @@ -8,7 +8,7 @@ cat >check-for-diff < >  exec grep '^diff --git' "\$1"
> >  EOF
> >  chmod +x check-for-diff
> > -test_set_editor "$PWD/check-for-diff"
> > +test_set_editor "$(pwd)/check-for-diff"
> >  
> >  cat >message <<'EOF'
> >  subject
> 
> Why? I see no benefit. Both $PWD and $(pwd) work fine everywhere,
> including Windows, and the former is faster, particularly on Windows.
> 
> -- Hannes

I don't know the technical details of why this change is needed.
But someone felt it was important enough to put in t/README.

  - When a test checks for an absolute path that a git command generated,
construct the expected value using $(pwd) rather than $PWD,
$TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference on
Windows, where the shell (MSYS bash) mangles absolute path names.
For details, see the commit message of 4114156ae9.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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] t9138-git-svn-authors-prog.sh fixups

[Jeremiah Mahler]

Junio,

On Tue, May 27, 2014 at 12:43:06PM -0700, Junio C Hamano wrote:
> Jeremiah Mahler  writes:
> 
...
> > diff --git a/t/t9138-git-svn-authors-prog.sh 
> > b/t/t9138-git-svn-authors-prog.sh
> > index 83cc5fc..d54c37a 100755
> > --- a/t/t9138-git-svn-authors-prog.sh
> > +++ b/t/t9138-git-svn-authors-prog.sh
> > @@ -7,40 +7,39 @@ test_description='git svn authors prog tests'
> >  
> >  . ./lib-git-svn.sh
> >  
> > -cat > svn-authors-prog <<'EOF'
> > -#!/usr/bin/perl
> > -$_ = shift;
> > -if (s/-sub$//)  {
> > -   print "$_ <$_\@sub.example.com>\n";
> > -}
> > -else {
> > -   print "$_ <$_\@example.com>\n";
> > -}
> > +write_script svn-authors-prog $PERL_PATH <<-\EOF
> 
> I think you meant to dq "$PERL_PATH" here.  Other than that, looks
> OK to me.
> 
> Thanks.

Ah, you're right, it needs the quotes.  Can this minor changed be fixed
by editing the patch or should I re-send it?

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 v3] wording fixes in the user manual and glossary

[Jeremiah Mahler]

Re-worded the section on "Updating a repository with git fetch" in the
user manual.

Various other minor fixes in the manual and glossary.

Signed-off-by: Jeremiah Mahler 
---

Notes:
This revision includes suggestions from Chris Packham, Ben Aveling
and Junio C Hamano [1].

[1]: http://marc.info/?l=git&m=140122033314094&w=2

  - The changes to the "git fetch" section have been reduced.

- Kept the first sentence which Junio liked.

- Kept the second sentence as Chris had it originally except for
  swapping 'command `git fetch` with '`git fetch` command' and
  using 'original repository' instead of 'her repository'.
  'original repository' agrees with the sentence before it.

 - Other minor edits are the same as the previous version.

 Documentation/glossary-content.txt |  2 +-
 Documentation/user-manual.txt  | 15 +++
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/Documentation/glossary-content.txt 
b/Documentation/glossary-content.txt
index be0858c..4e0b971 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -1,7 +1,7 @@
 [[def_alternate_object_database]]alternate object database::
Via the alternates mechanism, a <>
can inherit part of its <>
-   from another object database, which is called "alternate".
+   from another object database, which is called an "alternate".
 
 [[def_bare_repository]]bare repository::
A bare repository is normally an appropriately
diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index d33f884..7330d88 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -416,12 +416,11 @@ REVISIONS" section of linkgit:gitrevisions[7].
 Updating a repository with git fetch
 
 
-Eventually the developer cloned from will do additional work in her
-repository, creating new commits and advancing the branches to point
-at the new commits.
+After you clone a repository and commit a few changes of your own, you
+may wish to check the original repository for updates.
 
-The command `git fetch`, with no arguments, will update all of the
-remote-tracking branches to the latest version found in her
+The `git-fetch` command, with no arguments, will update all of the
+remote-tracking branches to the latest version found in the original
 repository.  It will not touch any of your own branches--not even the
 "master" branch that was created for you on clone.
 
@@ -1811,8 +1810,8 @@ manner.
 You can then import these into your mail client and send them by
 hand.  However, if you have a lot to send at once, you may prefer to
 use the linkgit:git-send-email[1] script to automate the process.
-Consult the mailing list for your project first to determine how they
-prefer such patches be handled.
+Consult the mailing list for your project first to determine
+their requirements for submitting patches.
 
 [[importing-patches]]
 Importing patches to a project
@@ -2255,7 +2254,7 @@ $ git checkout test && git merge speed-up-spinlocks
 It is unlikely that you would have any conflicts here ... but you might if you
 spent a while on this step and had also pulled new versions from upstream.
 
-Some time later when enough time has passed and testing done, you can pull the
+Sometime later when enough time has passed and testing done, you can pull the
 same branch into the `release` tree ready to go upstream.  This is where you
 see the value of keeping each patch (or patch series) in its own branch.  It
 means that the patches can be moved into the `release` tree in any order.
-- 
2.0.0.rc4.480.g058e066

--
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 01/15] builtin/add.c: rearrange xcalloc arguments

[Jeremiah Mahler]

On Tue, May 27, 2014 at 05:35:29PM -0400, Eric Sunshine wrote:
> On Tue, May 27, 2014 at 7:32 AM, Brian Gesiak  wrote:
> > Oomph, how embarrassing. Thanks for pointing that out!
> 
> Etiquette on this list is to avoid top-posting [1].
> 
> [1]: https://lkml.org/lkml/2005/1/11/111
> 

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

That is the funniest post I have ever seen by Kroah.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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/5] replace signal() with sigaction()

[Jeremiah Mahler]

>From signal(2)

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.  See Portability below.

This patch set replaces calls to signal() with sigaction() in all files
except sigchain.c.  sigchain.c is a bit more complicated than the others
and will be done in a separate patch.

Jeremiah Mahler (5):
  progress.c: replace signal() with sigaction()
  daemon.c run_service(): replace signal() with sigaction()
  daemon.c child_handler(): replace signal() with sigaction()
  daemon.c service_loop(): replace signal() with sigaction()
  connect.c: replace signal() with sigaction()

 connect.c  |  5 -
 daemon.c   | 15 ---
 progress.c |  6 +-
 3 files changed, 21 insertions(+), 5 deletions(-)

-- 
2.0.0.rc4.6.g127602c

--
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/5] daemon.c service_loop(): replace signal() with sigaction()

[Jeremiah Mahler]

>From signal(2)

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.  See Portability below.

Replaced signal() with sigaction() in daemon.c service_loop()

Signed-off-by: Jeremiah Mahler 
---
 daemon.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/daemon.c b/daemon.c
index 3c48dc0..8c28b3d 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1002,6 +1002,7 @@ static int service_loop(struct socketlist *socklist)
 {
struct pollfd *pfd;
int i;
+   struct sigaction sa;
 
pfd = xcalloc(socklist->nr, sizeof(struct pollfd));
 
@@ -1010,7 +1011,9 @@ static int service_loop(struct socketlist *socklist)
pfd[i].events = POLLIN;
}
 
-   signal(SIGCHLD, child_handler);
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = child_handler;
+   sigaction(SIGCHLD, &sa, 0);
 
for (;;) {
int i;
-- 
2.0.0.rc4.6.g127602c

--
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 3/5] daemon.c child_handler(): replace signal() with sigaction()

[Jeremiah Mahler]

>From signal(2)

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.  See Portability below.

Replaced signal() with sigaction() in daemon.c child_handler()

Signed-off-by: Jeremiah Mahler 
---
 daemon.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/daemon.c b/daemon.c
index 78c4a1c..3c48dc0 100644
--- a/daemon.c
+++ b/daemon.c
@@ -791,12 +791,15 @@ static void handle(int incoming, struct sockaddr *addr, 
socklen_t addrlen)
 
 static void child_handler(int signo)
 {
+   struct sigaction sa;
/*
 * Otherwise empty handler because systemcalls will get interrupted
 * upon signal receipt
 * SysV needs the handler to be rearmed
 */
-   signal(SIGCHLD, child_handler);
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = child_handler;
+   sigaction(SIGCHLD, &sa, 0);
 }
 
 static int set_reuse_addr(int sockfd)
-- 
2.0.0.rc4.6.g127602c

--
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 5/5] connect.c: replace signal() with sigaction()

[Jeremiah Mahler]

>From signal(2)

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.  See Portability below.

Replaced signal() with sigaction() in connect.c

Signed-off-by: Jeremiah Mahler 
---
 connect.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index a983d06..b2a33c9 100644
--- a/connect.c
+++ b/connect.c
@@ -665,11 +665,14 @@ struct child_process *git_connect(int fd[2], const char 
*url,
enum protocol protocol;
const char **arg;
struct strbuf cmd = STRBUF_INIT;
+   struct sigaction sa;
 
/* Without this we cannot rely on waitpid() to tell
 * what happened to our children.
 */
-   signal(SIGCHLD, SIG_DFL);
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = SIG_DFL;
+   sigaction(SIGCHLD, &sa, 0);
 
protocol = parse_connect_url(url, &hostandport, &path);
if (flags & CONNECT_DIAG_URL) {
-- 
2.0.0.rc4.6.g127602c

--
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/5] progress.c: replace signal() with sigaction()

[Jeremiah Mahler]

>From signal(2)

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.  See Portability below.

Replaced signal() with sigaction() in progress.c

Signed-off-by: Jeremiah Mahler 
---
 progress.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/progress.c b/progress.c
index 261314e..24df263 100644
--- a/progress.c
+++ b/progress.c
@@ -66,8 +66,12 @@ static void set_progress_signal(void)
 static void clear_progress_signal(void)
 {
struct itimerval v = {{0,},};
+   struct sigaction sa;
+
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = SIG_IGN;
setitimer(ITIMER_REAL, &v, NULL);
-   signal(SIGALRM, SIG_IGN);
+   sigaction(SIGALRM, &sa, 0);
progress_update = 0;
 }
 
-- 
2.0.0.rc4.6.g127602c

--
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/5] daemon.c run_service(): replace signal() with sigaction()

[Jeremiah Mahler]

>From signal(2)

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.  See Portability below.

Replaced signal() with sigaction() in daemon.c run_service()

Signed-off-by: Jeremiah Mahler 
---
 daemon.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/daemon.c b/daemon.c
index eba1255..78c4a1c 100644
--- a/daemon.c
+++ b/daemon.c
@@ -322,6 +322,7 @@ static int run_service(char *dir, struct daemon_service 
*service)
 {
const char *path;
int enabled = service->enabled;
+   struct sigaction sa;
 
loginfo("Request %s for '%s'", service->name, dir);
 
@@ -376,7 +377,9 @@ static int run_service(char *dir, struct daemon_service 
*service)
 * We'll ignore SIGTERM from now on, we have a
 * good client.
 */
-   signal(SIGTERM, SIG_IGN);
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = SIG_IGN;
+   sigaction(SIGTERM, &sa, 0);
 
return service->fn();
 }
-- 
2.0.0.rc4.6.g127602c

--
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 1/5] progress.c: replace signal() with sigaction()

[Jeremiah Mahler]

On Wed, May 28, 2014 at 08:07:38PM +1200, Chris Packham wrote:
> On 28/05/14 18:14, Jeremiah Mahler wrote:
> > From signal(2)
> > 
> >   The behavior of signal() varies across UNIX versions, and has also var‐
> >   ied historically across different versions of Linux.   Avoid  its  use:
> >   use sigaction(2) instead.  See Portability below.
> 
> Minor nit. The last sentence applies to the man page you're quoting and
> doesn't really make sense when viewed in the context of this commit
> message. Same applies to other patches in this series.
> 
Yes, that is confusing.

...
> > @@ -66,8 +66,12 @@ static void set_progress_signal(void)
> >  static void clear_progress_signal(void)
> >  {
> > struct itimerval v = {{0,},};
> > +   struct sigaction sa;
> > +
> > +   memset(&sa, 0, sizeof(sa));
> > +   sa.sa_handler = SIG_IGN;
> 
> A C99 initialiser here would save the call to memset. Unfortunately
> Documentation/CodingGuidelines is fairly clear on not using C99
> initialisers, given the fact we're now at git 2.0 maybe it's time to
> revisit this policy?
> 

struct sigaction sa = { .sa_handler = SIG_IGN };

I do like that.

This brings up another issue.  memset(&sa, 0, sizeof(sa)); The sigaction
examples I have seen always use memset.  The manpage for sigaction(2)
doesn't mention it.  Other code it Git uses memset with sigaction (see
fast-import.c line 530).

Is this struct guaranteed to be initialized to zero so I don't have to
use memset?

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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/5] replace signal() with sigaction()

[Jeremiah Mahler]

On Wed, May 28, 2014 at 09:40:55AM +0200, Johannes Sixt wrote:
> Am 5/28/2014 8:14, schrieb Jeremiah Mahler:
> > From signal(2)
> > 
> >   The behavior of signal() varies across UNIX versions, and has also var‐
> >   ied historically across different versions of Linux.   Avoid  its  use:
> >   use sigaction(2) instead.  See Portability below.
> > 
> > This patch set replaces calls to signal() with sigaction() in all files
> > except sigchain.c.  sigchain.c is a bit more complicated than the others
> > and will be done in a separate patch.
> 
> In compat/mingw.c we have:
> 
> int sigaction(int sig, struct sigaction *in, struct sigaction *out)
> {
>   if (sig != SIGALRM)
>   return errno = EINVAL,
>   error("sigaction only implemented for SIGALRM");
>   if (out != NULL)
>   return errno = EINVAL,
>   error("sigaction: param 3 != NULL not implemented");
> 
>   timer_fn = in->sa_handler;
>   return 0;
> }
> 
> Notice "only implemented for SIGALRM". Are adding the missing signals
> somewhere (here or in a later patch)?
> 
Good catch Johannes, I did not account for this.

If I am reading this correctly, sigaction() can only be used for
SIGALRM, and no other signals.  But signal() it would have worked for
these other signals.

My first idea is why not call signal() from within this sigaction()
call?  The handler could be read from the struct sigaction passed in.

> > Jeremiah Mahler (5):
> >   progress.c: replace signal() with sigaction()
> >   daemon.c run_service(): replace signal() with sigaction()
> >   daemon.c child_handler(): replace signal() with sigaction()
> >   daemon.c service_loop(): replace signal() with sigaction()
> >   connect.c: replace signal() with sigaction()
> > 
> >  connect.c  |  5 -
> >  daemon.c   | 15 ---
> >  progress.c |  6 +-
> >  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> Isn't this a bit too much of code churn, given that there were no bug
> reports where signal handling is identified as the culprit despite
> the warning you cited above?
> 
We might get bugs from continuing to use signal().  We might get bugs from
changing to sigaction().  I feel more comfortable using sigaction() since
its man page is so adamant about not using signal().

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 2/2] connect.c: replace signal() with sigaction()

[Jeremiah Mahler]

>From signal(2) man page:

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.

Replaced signal() with sigaction() in connect.c

Signed-off-by: Jeremiah Mahler 
---
 connect.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index a983d06..b2a33c9 100644
--- a/connect.c
+++ b/connect.c
@@ -665,11 +665,14 @@ struct child_process *git_connect(int fd[2], const char 
*url,
enum protocol protocol;
const char **arg;
struct strbuf cmd = STRBUF_INIT;
+   struct sigaction sa;
 
/* Without this we cannot rely on waitpid() to tell
 * what happened to our children.
 */
-   signal(SIGCHLD, SIG_DFL);
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = SIG_DFL;
+   sigaction(SIGCHLD, &sa, 0);
 
protocol = parse_connect_url(url, &hostandport, &path);
if (flags & CONNECT_DIAG_URL) {
-- 
2.0.0.2.g1d11d5d

--
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 1/2] compat/mingw.c: expand MinGW support for sigaction

[Jeremiah Mahler]

Due to portability issues across UNIX versions sigaction(2) should be used
instead of signal(2).

>From the signal(2) man page:

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.

Unfortunately MinGW under Windows has limited support for signal and no
support for sigaction.  And this prevents sigaction from being used across
the entire Git project.

In compat/mingw.c there is a faux sigaction function but it only supports
SIGALARM.  Hence the need for continuing to use signal() in other cases.

This patch expands the faux sigaction function so that it calls signal in
cases other than SIGALRM.  Now sigaction can be used across the entire Git
project and MinGW will still work with signal as it did before.

Signed-off-by: Jeremiah Mahler 
---
 compat/mingw.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index e9892f8..e504cef 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1651,14 +1651,15 @@ int setitimer(int type, struct itimerval *in, struct 
itimerval *out)
 
 int sigaction(int sig, struct sigaction *in, struct sigaction *out)
 {
-   if (sig != SIGALRM)
-   return errno = EINVAL,
-   error("sigaction only implemented for SIGALRM");
if (out != NULL)
return errno = EINVAL,
error("sigaction: param 3 != NULL not implemented");
 
-   timer_fn = in->sa_handler;
+   if (sig == SIGALRM)
+   timer_fn = in->sa_handler;
+   else
+   signal(sig, in->sa_handler);
+
return 0;
 }
 
-- 
2.0.0.2.g1d11d5d

--
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 0/2] replace signal() with sigaction()

[Jeremiah Mahler]

This is the second revision to the patch set to replace signal(2) with
sigaction(2) [1].

As Johannes pointed out [2], replacing signal with sigaction would break
MinGW compatibility.  The first patch in this series addresses this
problem by expanding the faux sigaction function in compat/mingw.c to
support signals other than just SIGALRM.  Details are in the patch
description.

The second patch is a proof of concept.  It converts signal to sigaction
in a case where signal SIGCHILD was used.  Previously this would have
failed with MinGW since the faux sigaction function only supported
SIGALRM.  Now it works as expected.

I have tested these changes under Linux and under Windows 7 using Msysgit.

[1]: http://marc.info/?l=git&m=140125769223552&w=2
[2]: http://marc.info/?l=git&m=140126288325213&w=2

Jeremiah Mahler (2):
  compat/mingw.c: expand MinGW support for sigaction
  connect.c: replace signal() with sigaction()

 compat/mingw.c | 9 +
 connect.c  | 5 -
 2 files changed, 9 insertions(+), 5 deletions(-)

-- 
2.0.0.2.g1d11d5d

--
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] connect.c: replace signal() with sigaction()

[Jeremiah Mahler]

Chris,

On Sat, May 31, 2014 at 10:39:39PM +1200, Chris Packham wrote:
> On 31/05/14 08:58, Jeremiah Mahler wrote:
> > From signal(2) man page:
> > 
...
> > -   signal(SIGCHLD, SIG_DFL);
> > +   memset(&sa, 0, sizeof(sa));
> > +   sa.sa_handler = SIG_DFL;
> > +   sigaction(SIGCHLD, &sa, 0);
> 
> I think this got lost in the wash with v1 but
> Documentation/CodingGuidelines says to use NULL here instead of 0.
> 
Got it.  Will be in to the next revision.

  sigaction(SIGCHLD, &sa, NULL);

Thanks,
-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 v3 2/9] connect.c: replace signal() with sigaction()

[Jeremiah Mahler]

>From the signal(2) man page:

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.

Replaced signal() with sigaction() in connect.c

Signed-off-by: Jeremiah Mahler 
---
 connect.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index a983d06..1dc51b2 100644
--- a/connect.c
+++ b/connect.c
@@ -665,11 +665,14 @@ struct child_process *git_connect(int fd[2], const char 
*url,
enum protocol protocol;
const char **arg;
struct strbuf cmd = STRBUF_INIT;
+   struct sigaction sa;
 
/* Without this we cannot rely on waitpid() to tell
 * what happened to our children.
 */
-   signal(SIGCHLD, SIG_DFL);
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = SIG_DFL;
+   sigaction(SIGCHLD, &sa, NULL);
 
protocol = parse_connect_url(url, &hostandport, &path);
if (flags & CONNECT_DIAG_URL) {
-- 
2.0.0.8.g7bf6e1f.dirty

--
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 v3 1/9] compat/mingw.c: expand MinGW support for sigaction

[Jeremiah Mahler]

Due to portability issues across UNIX versions sigaction(2) should be used
instead of signal(2).

>From the signal(2) man page:

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.

Unfortunately MinGW under Windows has limited support for signal and no
support for sigaction.  And this prevents sigaction from being used across
the entire Git project.

In compat/mingw.c there is a faux sigaction function but it only supports
SIGALARM.  Hence the need for continuing to use signal() in other cases.

This patch expands the faux sigaction function so that it calls signal in
cases other than SIGALRM.  Now sigaction can be used across the entire Git
project and MinGW will still work with signal as it did before.

Signed-off-by: Jeremiah Mahler 
---
 compat/mingw.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index e9892f8..e504cef 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1651,14 +1651,15 @@ int setitimer(int type, struct itimerval *in, struct 
itimerval *out)
 
 int sigaction(int sig, struct sigaction *in, struct sigaction *out)
 {
-   if (sig != SIGALRM)
-   return errno = EINVAL,
-   error("sigaction only implemented for SIGALRM");
if (out != NULL)
return errno = EINVAL,
error("sigaction: param 3 != NULL not implemented");
 
-   timer_fn = in->sa_handler;
+   if (sig == SIGALRM)
+   timer_fn = in->sa_handler;
+   else
+   signal(sig, in->sa_handler);
+
return 0;
 }
 
-- 
2.0.0.8.g7bf6e1f.dirty

--
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 v3 0/9] replace signal() with sigaction()

[Jeremiah Mahler]

This is version 3 of the patch set to convert signal(2) to sigaction(2)
(previous discussion [1]).

[1]: http://marc.info/?l=git&m=140148352416926&w=2

Changes in this revision include:

  - Using NULL pointers instead of 0 as per the
Documentation/CodingGuidlines pointed out by Chris Packham.

sigaction(SIGCHLD, &sa, NULL);

  - Conversion of all remaining files which used signal().

  - sigchain.c required the most changes.  Both the old signal handler
was used and the return value from signal() was being checked.
signal() would return the previous error handler which would be
SIG_ERR if an error occurred.  sigaction() just returns -1 in this
case.

Jeremiah Mahler (9):
  compat/mingw.c: expand MinGW support for sigaction
  connect.c: replace signal() with sigaction()
  progress.c: replace signal() with sigaction()
  write_or_die.c: replace signal() with sigaction()
  daemon.c: replace signal() with sigaction()
  builtin/log.c: replace signal() with sigaction()
  builtin/merge-index.c: replace signal() with sigaction()
  builtin/verify-tag.c: replace signal() with sigaction()
  sigchain.c: replace signal() with sigaction()

 builtin/log.c |  6 +-
 builtin/merge-index.c |  5 -
 builtin/verify-tag.c  |  5 -
 compat/mingw.c|  9 +
 connect.c |  5 -
 daemon.c  | 16 +---
 progress.c|  6 +-
 sigchain.c| 14 +++---
 write_or_die.c|  6 +-
 9 files changed, 56 insertions(+), 16 deletions(-)

-- 
2.0.0.8.g7bf6e1f.dirty

--
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 v3 4/9] write_or_die.c: replace signal() with sigaction()

[Jeremiah Mahler]

>From the signal(2) man page:

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.

Replaced signal() with sigaction() in write_or_die.c

Signed-off-by: Jeremiah Mahler 
---
 write_or_die.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/write_or_die.c b/write_or_die.c
index b50f99a..f5fdec8 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -2,8 +2,12 @@
 
 static void check_pipe(int err)
 {
+   struct sigaction sa;
+
if (err == EPIPE) {
-   signal(SIGPIPE, SIG_DFL);
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = SIG_DFL;
+   sigaction(SIGPIPE, &sa, NULL);
raise(SIGPIPE);
/* Should never happen, but just in case... */
exit(141);
-- 
2.0.0.8.g7bf6e1f.dirty

--
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 v3 3/9] progress.c: replace signal() with sigaction()

[Jeremiah Mahler]

>From the signal(2) man page:

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.

Replaced signal() with sigaction() in progress.c

Signed-off-by: Jeremiah Mahler 
---
 progress.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/progress.c b/progress.c
index 261314e..ff676bc 100644
--- a/progress.c
+++ b/progress.c
@@ -66,8 +66,12 @@ static void set_progress_signal(void)
 static void clear_progress_signal(void)
 {
struct itimerval v = {{0,},};
+   struct sigaction sa;
+
setitimer(ITIMER_REAL, &v, NULL);
-   signal(SIGALRM, SIG_IGN);
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = SIG_IGN;
+   sigaction(SIGALRM, &sa, NULL);
progress_update = 0;
 }
 
-- 
2.0.0.8.g7bf6e1f.dirty

--
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 v3 5/9] daemon.c: replace signal() with sigaction()

[Jeremiah Mahler]

>From the signal(2) man page:

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.

Replaced signal() with sigaction() in daemon.c

Signed-off-by: Jeremiah Mahler 
---
 daemon.c | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/daemon.c b/daemon.c
index eba1255..615426e 100644
--- a/daemon.c
+++ b/daemon.c
@@ -322,6 +322,7 @@ static int run_service(char *dir, struct daemon_service 
*service)
 {
const char *path;
int enabled = service->enabled;
+   struct sigaction sa;
 
loginfo("Request %s for '%s'", service->name, dir);
 
@@ -376,7 +377,9 @@ static int run_service(char *dir, struct daemon_service 
*service)
 * We'll ignore SIGTERM from now on, we have a
 * good client.
 */
-   signal(SIGTERM, SIG_IGN);
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = SIG_IGN;
+   sigaction(SIGTERM, &sa, NULL);
 
return service->fn();
 }
@@ -788,12 +791,16 @@ static void handle(int incoming, struct sockaddr *addr, 
socklen_t addrlen)
 
 static void child_handler(int signo)
 {
+   struct sigaction sa;
+
/*
 * Otherwise empty handler because systemcalls will get interrupted
 * upon signal receipt
 * SysV needs the handler to be rearmed
 */
-   signal(SIGCHLD, child_handler);
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = child_handler;
+   sigaction(SIGCHLD, &sa, NULL);
 }
 
 static int set_reuse_addr(int sockfd)
@@ -996,6 +1003,7 @@ static int service_loop(struct socketlist *socklist)
 {
struct pollfd *pfd;
int i;
+   struct sigaction sa;
 
pfd = xcalloc(socklist->nr, sizeof(struct pollfd));
 
@@ -1004,7 +1012,9 @@ static int service_loop(struct socketlist *socklist)
pfd[i].events = POLLIN;
}
 
-   signal(SIGCHLD, child_handler);
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = child_handler;
+   sigaction(SIGCHLD, &sa, NULL);
 
for (;;) {
int i;
-- 
2.0.0.8.g7bf6e1f.dirty

--
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 v3 6/9] builtin/log.c: replace signal() with sigaction()

[Jeremiah Mahler]

>From the signal(2) man page:

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.

Replaced signal() with sigaction() in builtin/log.c

Signed-off-by: Jeremiah Mahler 
---
 builtin/log.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..f1deea1 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -315,7 +315,11 @@ static void setup_early_output(struct rev_info *rev)
 static void finish_early_output(struct rev_info *rev)
 {
int n = estimate_commit_count(rev, rev->commits);
-   signal(SIGALRM, SIG_IGN);
+   struct sigaction sa;
+
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = SIG_IGN;
+   sigaction(SIGALRM, &sa, NULL);
show_early_header(rev, "done", n);
 }
 
-- 
2.0.0.8.g7bf6e1f.dirty

--
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 v3 9/9] sigchain.c: replace signal() with sigaction()

[Jeremiah Mahler]

>From the signal(2) man page:

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.

Replaced signal() with sigaction() in sigchain.c

Signed-off-by: Jeremiah Mahler 
---
 sigchain.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/sigchain.c b/sigchain.c
index 1118b99..deab262 100644
--- a/sigchain.c
+++ b/sigchain.c
@@ -19,11 +19,16 @@ static void check_signum(int sig)
 int sigchain_push(int sig, sigchain_fun f)
 {
struct sigchain_signal *s = signals + sig;
+   struct sigaction sa, sa_old;
+   int result;
check_signum(sig);
 
ALLOC_GROW(s->old, s->n + 1, s->alloc);
-   s->old[s->n] = signal(sig, f);
-   if (s->old[s->n] == SIG_ERR)
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = f;
+   result = sigaction(sig, &sa, &sa_old);
+   s->old[s->n] = sa_old.sa_handler;
+   if (result == -1)
return -1;
s->n++;
return 0;
@@ -32,11 +37,14 @@ int sigchain_push(int sig, sigchain_fun f)
 int sigchain_pop(int sig)
 {
struct sigchain_signal *s = signals + sig;
+   struct sigaction sa, sa_old;
check_signum(sig);
if (s->n < 1)
return 0;
 
-   if (signal(sig, s->old[s->n - 1]) == SIG_ERR)
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = s->old[s->n - 1];
+   if (sigaction(sig, &sa, &sa_old) == -1)
return -1;
s->n--;
return 0;
-- 
2.0.0.8.g7bf6e1f.dirty

--
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 v3 8/9] builtin/verify-tag.c: replace signal() with sigaction()

[Jeremiah Mahler]

>From the signal(2) man page:

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.

Replaced signal() with sigaction() in builtin/verify-tag.c

Signed-off-by: Jeremiah Mahler 
---
 builtin/verify-tag.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 9cdf332..d5ccbad 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -73,6 +73,7 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
OPT__VERBOSE(&verbose, N_("print tag contents")),
OPT_END()
};
+   struct sigaction sa;
 
git_config(git_verify_tag_config, NULL);
 
@@ -83,7 +84,9 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
 
/* sometimes the program was terminated because this signal
 * was received in the process of writing the gpg input: */
-   signal(SIGPIPE, SIG_IGN);
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = SIG_IGN;
+   sigaction(SIGPIPE, &sa, NULL);
while (i < argc)
if (verify_tag(argv[i++], verbose))
had_error = 1;
-- 
2.0.0.8.g7bf6e1f.dirty

--
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 v3 7/9] builtin/merge-index.c: replace signal() with sigaction()

[Jeremiah Mahler]

>From the signal(2) man page:

  The behavior of signal() varies across UNIX versions, and has also var‐
  ied historically across different versions of Linux.   Avoid  its  use:
  use sigaction(2) instead.

Replaced signal() with sigaction() in builtin/merge-index.c

Signed-off-by: Jeremiah Mahler 
---
 builtin/merge-index.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index b416d92..25db374 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -68,11 +68,14 @@ static void merge_all(void)
 int cmd_merge_index(int argc, const char **argv, const char *prefix)
 {
int i, force_file = 0;
+   struct sigaction sa;
 
/* Without this we cannot rely on waitpid() to tell
 * what happened to our children.
 */
-   signal(SIGCHLD, SIG_DFL);
+   memset(&sa, 0, sizeof(sa));
+   sa.sa_handler = SIG_DFL;
+   sigaction(SIGCHLD, &sa, NULL);
 
if (argc < 3)
usage("git merge-index [-o] [-q]  (-a | [--] 
*)");
-- 
2.0.0.8.g7bf6e1f.dirty

--
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 0/9] replace signal() with sigaction()

[Jeremiah Mahler]

Hannes,

On Mon, Jun 02, 2014 at 01:28:25PM +0200, Johannes Sixt wrote:
> Am 6/1/2014 20:10, schrieb Jeremiah Mahler:
> > This is version 3 of the patch set to convert signal(2) to sigaction(2)
> > (previous discussion [1]).
> > 
...
> >   sigchain.c: replace signal() with sigaction()
> 
> The series without patch 9/9 works on Windows so far.
> 
> Without patch patch 9/9 and a more complete implementation of sigaction in
> compat/mingw.c the series misses its goal. But even if you complete it, it
> is IMHO only code churn without practical merits.
> 
> -- Hannes
> 

You are right, I missed the case where the old signal was used, as is
done in sigchain.c.  Sorry about that.

Thanks again for looking at my patch.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 0/9] replace signal() with sigaction()

[Jeremiah Mahler]

On Mon, Jun 02, 2014 at 12:05:29PM -0700, Junio C Hamano wrote:
> Johannes Sixt  writes:
> 
> >> Jeremiah Mahler (9):
> >>   compat/mingw.c: expand MinGW support for sigaction
> >>   connect.c: replace signal() with sigaction()
> >>   progress.c: replace signal() with sigaction()
> >>   write_or_die.c: replace signal() with sigaction()
> >>   daemon.c: replace signal() with sigaction()
> >>   builtin/log.c: replace signal() with sigaction()
> >>   builtin/merge-index.c: replace signal() with sigaction()
> >>   builtin/verify-tag.c: replace signal() with sigaction()
> >>   sigchain.c: replace signal() with sigaction()
> >
> > The series without patch 9/9 works on Windows so far.
> >
> > Without patch patch 9/9 and a more complete implementation of sigaction in
> > compat/mingw.c the series misses its goal. But even if you complete it, it
> > is IMHO only code churn without practical merits.
> 
> Hmm, you sound a bit harsher than you usually do---although I
> sort of share with you the doubt on the practical merits.
> 

Alright, I'm dropping it.  Too much work for no real gain other than
some piece of mind.

Thanks Johannes and Junio for your feedback.

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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] t/t7810-grep.sh: remove duplicate test_config()

[Jeremiah Mahler]

t/t7810-grep.sh had its own test_config() function which served the
same purpose as the one in t/test-lib-functions.sh.  Removed, all tests
pass.

Signed-off-by: Jeremiah Mahler 
---
 t/t7810-grep.sh | 5 -
 1 file changed, 5 deletions(-)

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 63b3039..40615de 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1096,11 +1096,6 @@ test_expect_success 'grep -E pattern with 
grep.patternType=fixed' '
test_cmp expected actual
 '
 
-test_config() {
-   git config "$1" "$2" &&
-   test_when_finished "git config --unset $1"
-}
-
 cat >expected <:int main(int argc, const char **argv)
 hello.c-{
-- 
2.0.0.153.g79d

--
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] api-strbuf.txt minor typos

[Jeremiah Mahler]

Fixed some minor typos in api-strbuf.txt: 'A' instead of 'An', 'have'
instead of 'has', a overlong line, and 'another' instead of 'an other'.

Signed-off-by: Jeremiah Mahler 
---
 Documentation/technical/api-strbuf.txt | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/technical/api-strbuf.txt 
b/Documentation/technical/api-strbuf.txt
index 077a709..f9c06a7 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -7,10 +7,10 @@ use the mem* functions than a str* one (memchr vs. strchr 
e.g.).
 Though, one has to be careful about the fact that str* functions often
 stop on NULs and that strbufs may have embedded NULs.
 
-An strbuf is NUL terminated for convenience, but no function in the
+A strbuf is NUL terminated for convenience, but no function in the
 strbuf API actually relies on the string being free of NULs.
 
-strbufs has some invariants that are very important to keep in mind:
+strbufs have some invariants that are very important to keep in mind:
 
 . The `buf` member is never NULL, so it can be used in any usual C
 string operations safely. strbuf's _have_ to be initialized either by
@@ -56,8 +56,8 @@ Data structures
 * `struct strbuf`
 
 This is the string buffer structure. The `len` member can be used to
-determine the current length of the string, and `buf` member provides access to
-the string itself.
+determine the current length of the string, and `buf` member provides
+access to the string itself.
 
 Functions
 -
@@ -202,7 +202,7 @@ strbuf_addstr(sb, "immediate string");
 
 `strbuf_addbuf`::
 
-   Copy the contents of an other buffer at the end of the current one.
+   Copy the contents of another buffer at the end of the current one.
 
 `strbuf_adddup`::
 
-- 
2.0.0.574.gc6f278f

--
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/RFC v1 4/5] fast-import.c: cleanup using strbuf_set operations

[Jeremiah Mahler]

Simplified cases where a strbuf_reset was immediately followed by a
strbuf_add using the new strbuf_set operations.

Signed-off-by: Jeremiah Mahler 
---
 fast-import.c | 19 ++-
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index e8ec34d..c23935c 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2741,8 +2741,7 @@ static void parse_new_commit(void)
hashcpy(b->branch_tree.versions[0].sha1,
b->branch_tree.versions[1].sha1);
 
-   strbuf_reset(&new_data);
-   strbuf_addf(&new_data, "tree %s\n",
+   strbuf_setf(&new_data, "tree %s\n",
sha1_to_hex(b->branch_tree.versions[1].sha1));
if (!is_null_sha1(b->sha1))
strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(b->sha1));
@@ -2829,9 +2828,7 @@ static void parse_new_tag(void)
parse_data(&msg, 0, NULL);
 
/* build the tag object */
-   strbuf_reset(&new_data);
-
-   strbuf_addf(&new_data,
+   strbuf_setf(&new_data,
"object %s\n"
"type %s\n"
"tag %s\n",
@@ -2898,8 +2895,7 @@ static void cat_blob(struct object_entry *oe, unsigned 
char sha1[20])
 * Output based on batch_one_object() from cat-file.c.
 */
if (type <= 0) {
-   strbuf_reset(&line);
-   strbuf_addf(&line, "%s missing\n", sha1_to_hex(sha1));
+   strbuf_setf(&line, "%s missing\n", sha1_to_hex(sha1));
cat_blob_write(line.buf, line.len);
strbuf_release(&line);
free(buf);
@@ -2910,8 +2906,7 @@ static void cat_blob(struct object_entry *oe, unsigned 
char sha1[20])
if (type != OBJ_BLOB)
die("Object %s is a %s but a blob was expected.",
sha1_to_hex(sha1), typename(type));
-   strbuf_reset(&line);
-   strbuf_addf(&line, "%s %s %lu\n", sha1_to_hex(sha1),
+   strbuf_setf(&line, "%s %s %lu\n", sha1_to_hex(sha1),
typename(type), size);
cat_blob_write(line.buf, line.len);
strbuf_release(&line);
@@ -3034,14 +3029,12 @@ static void print_ls(int mode, const unsigned char 
*sha1, const char *path)
 
if (!mode) {
/* missing SP path LF */
-   strbuf_reset(&line);
-   strbuf_addstr(&line, "missing ");
+   strbuf_setstr(&line, "missing ");
quote_c_style(path, &line, NULL, 0);
strbuf_addch(&line, '\n');
} else {
/* mode SP type SP object_name TAB path LF */
-   strbuf_reset(&line);
-   strbuf_addf(&line, "%06o %s %s\t",
+   strbuf_setf(&line, "%06o %s %s\t",
mode & ~NO_DELTA, type, sha1_to_hex(sha1));
quote_c_style(path, &line, NULL, 0);
strbuf_addch(&line, '\n');
-- 
2.0.0.573.ged771ce.dirty

--
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/RFC v1 1/5] add strbuf_set operations

[Jeremiah Mahler]

Currently, the data in a strbuf is modified using add operations.  To
set the buffer to some data a reset must be performed before an add.

  strbuf_reset(buf);
  strbuf_add(buf, cb.buf.buf, cb.buf.len);

And this is a common sequence of operations with 70 occurrences found in
the current source code.  This includes all the different variations
(add, addf, addstr, addbuf, addch).

  FILES=`find ./ -name '*.c'`
  CNT=$(pcregrep -M "strbuf_reset.*\n.*strbuf_add" $FILES | wc -l)
  CNT=$(echo "$CNT / 2" | bc)
  echo $CNT
  70

These patches add strbuf_set operations which allow this common sequence
to be performed in one line instead of two.

  strbuf_set(buf, cb.buf.buf, cb.buf.len);

Signed-off-by: Jeremiah Mahler 
---
 strbuf.c | 21 +
 strbuf.h | 14 ++
 2 files changed, 35 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index ac62982..9d64b00 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -189,6 +189,27 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t 
len,
strbuf_setlen(sb, sb->len + dlen - len);
 }
 
+void strbuf_set(struct strbuf *sb, const void *data, size_t len)
+{
+   strbuf_reset(sb);
+   strbuf_add(sb, data, len);
+}
+
+void strbuf_setf(struct strbuf *sb, const char *fmt, ...)
+{
+   va_list ap;
+   strbuf_reset(sb);
+   va_start(ap, fmt);
+   strbuf_vaddf(sb, fmt, ap);
+   va_end(ap);
+}
+
+void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2)
+{
+   strbuf_reset(sb);
+   strbuf_add(sb, sb2->buf, sb2->len);
+}
+
 void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t len)
 {
strbuf_splice(sb, pos, 0, data, len);
diff --git a/strbuf.h b/strbuf.h
index e9ad03e..b339f08 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -101,6 +101,20 @@ static inline struct strbuf **strbuf_split(const struct 
strbuf *sb,
  */
 extern void strbuf_list_free(struct strbuf **);
 
+/*- set buffer to data -*/
+
+extern void strbuf_set(struct strbuf *sb, const void *data, size_t len);
+
+static inline void strbuf_setstr(struct strbuf *sb, const char *s)
+{
+   strbuf_set(sb, s, strlen(s));
+}
+
+__attribute__((format (printf,2,3)))
+extern void strbuf_setf(struct strbuf *sb, const char *fmt, ...);
+
+extern void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2);
+
 /*- add data in your buffer -*/
 static inline void strbuf_addch(struct strbuf *sb, int c)
 {
-- 
2.0.0.573.ged771ce.dirty

--
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/RFC v1 3/5] sha1_name.c: cleanup using strbuf_set operations

[Jeremiah Mahler]

Simplified cases where a strbuf_reset was immediately followed by a
strbuf_add using the new strbuf_set operations.

Signed-off-by: Jeremiah Mahler 
---
 sha1_name.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 2b6322f..f88b66c 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -920,8 +920,7 @@ static int grab_nth_branch_switch(unsigned char *osha1, 
unsigned char *nsha1,
return 0;
if (--(cb->remaining) == 0) {
len = target - match;
-   strbuf_reset(&cb->buf);
-   strbuf_add(&cb->buf, match, len);
+   strbuf_set(&cb->buf, match, len);
return 1; /* we are done */
}
return 0;
@@ -957,8 +956,7 @@ static int interpret_nth_prior_checkout(const char *name, 
int namelen,
 
retval = 0;
if (0 < for_each_reflog_ent_reverse("HEAD", grab_nth_branch_switch, 
&cb)) {
-   strbuf_reset(buf);
-   strbuf_add(buf, cb.buf.buf, cb.buf.len);
+   strbuf_set(buf, cb.buf.buf, cb.buf.len);
retval = brace - name + 1;
}
 
@@ -1025,8 +1023,7 @@ static int interpret_empty_at(const char *name, int 
namelen, int len, struct str
if (next != name + 1)
return -1;
 
-   strbuf_reset(buf);
-   strbuf_add(buf, "HEAD", 4);
+   strbuf_set(buf, "HEAD", 4);
return 1;
 }
 
@@ -1044,8 +1041,7 @@ static int reinterpret(const char *name, int namelen, int 
len, struct strbuf *bu
strbuf_setlen(buf, used);
return len;
}
-   strbuf_reset(buf);
-   strbuf_addbuf(buf, &tmp);
+   strbuf_setbuf(buf, &tmp);
strbuf_release(&tmp);
/* tweak for size of {-N} versus expanded ref name */
return ret - used + len;
@@ -1054,8 +1050,7 @@ static int reinterpret(const char *name, int namelen, int 
len, struct strbuf *bu
 static void set_shortened_ref(struct strbuf *buf, const char *ref)
 {
char *s = shorten_unambiguous_ref(ref, 0);
-   strbuf_reset(buf);
-   strbuf_addstr(buf, s);
+   strbuf_setstr(buf, s);
free(s);
 }
 
-- 
2.0.0.573.ged771ce.dirty

--
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/RFC v1 5/5] builtin/remote.c: cleanup using strbuf_set operations

[Jeremiah Mahler]

Simplified cases where a strbuf_reset was immediately followed by a
strbuf_add using the new strbuf_set operations.

Signed-off-by: Jeremiah Mahler 
---
 builtin/remote.c | 51 +--
 1 file changed, 17 insertions(+), 34 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index c9b67ff..b059852 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -193,8 +193,7 @@ static int add(int argc, const char **argv)
return 1;
 
if (!mirror || mirror & MIRROR_FETCH) {
-   strbuf_reset(&buf);
-   strbuf_addf(&buf, "remote.%s.fetch", name);
+   strbuf_setf(&buf, "remote.%s.fetch", name);
if (track.nr == 0)
string_list_append(&track, "*");
for (i = 0; i < track.nr; i++) {
@@ -205,15 +204,13 @@ static int add(int argc, const char **argv)
}
 
if (mirror & MIRROR_PUSH) {
-   strbuf_reset(&buf);
-   strbuf_addf(&buf, "remote.%s.mirror", name);
+   strbuf_setf(&buf, "remote.%s.mirror", name);
if (git_config_set(buf.buf, "true"))
return 1;
}
 
if (fetch_tags != TAGS_DEFAULT) {
-   strbuf_reset(&buf);
-   strbuf_addf(&buf, "remote.%s.tagopt", name);
+   strbuf_setf(&buf, "remote.%s.tagopt", name);
if (git_config_set(buf.buf,
fetch_tags == TAGS_SET ? "--tags" : "--no-tags"))
return 1;
@@ -223,11 +220,9 @@ static int add(int argc, const char **argv)
return 1;
 
if (master) {
-   strbuf_reset(&buf);
-   strbuf_addf(&buf, "refs/remotes/%s/HEAD", name);
+   strbuf_setf(&buf, "refs/remotes/%s/HEAD", name);
 
-   strbuf_reset(&buf2);
-   strbuf_addf(&buf2, "refs/remotes/%s/%s", name, master);
+   strbuf_setf(&buf2, "refs/remotes/%s/%s", name, master);
 
if (create_symref(buf.buf, buf2.buf, "remote add"))
return error(_("Could not setup master '%s'"), master);
@@ -589,14 +584,12 @@ static int migrate_file(struct remote *remote)
if (git_config_set_multivar(buf.buf, remote->url[i], "^$", 0))
return error(_("Could not append '%s' to '%s'"),
remote->url[i], buf.buf);
-   strbuf_reset(&buf);
-   strbuf_addf(&buf, "remote.%s.push", remote->name);
+   strbuf_setf(&buf, "remote.%s.push", remote->name);
for (i = 0; i < remote->push_refspec_nr; i++)
if (git_config_set_multivar(buf.buf, remote->push_refspec[i], 
"^$", 0))
return error(_("Could not append '%s' to '%s'"),
remote->push_refspec[i], buf.buf);
-   strbuf_reset(&buf);
-   strbuf_addf(&buf, "remote.%s.fetch", remote->name);
+   strbuf_setf(&buf, "remote.%s.fetch", remote->name);
for (i = 0; i < remote->fetch_refspec_nr; i++)
if (git_config_set_multivar(buf.buf, remote->fetch_refspec[i], 
"^$", 0))
return error(_("Could not append '%s' to '%s'"),
@@ -644,23 +637,20 @@ static int mv(int argc, const char **argv)
if (!valid_fetch_refspec(buf.buf))
die(_("'%s' is not a valid remote name"), rename.new);
 
-   strbuf_reset(&buf);
-   strbuf_addf(&buf, "remote.%s", rename.old);
+   strbuf_setf(&buf, "remote.%s", rename.old);
strbuf_addf(&buf2, "remote.%s", rename.new);
if (git_config_rename_section(buf.buf, buf2.buf) < 1)
return error(_("Could not rename config section '%s' to '%s'"),
buf.buf, buf2.buf);
 
-   strbuf_reset(&buf);
-   strbuf_addf(&buf, "remote.%s.fetch", rename.new);
+   strbuf_setf(&buf, "remote.%s.fetch", rename.new);
if (git_config_set_multivar(buf.buf, NULL, NULL, 1))
return error(_("Could not remove config section '%s'"), 
buf.buf);
strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old);
for (i = 0; i < oldremote->fetch_refspec_nr; i++) {
char *ptr;
 
-   strbuf_reset(&buf2);
-   st

[PATCH/RFC v1 2/5] add strbuf_set operations documentation

[Jeremiah Mahler]

Add documentation of the strbuf_set operations to
technical/api-strbuf.txt.

Signed-off-by: Jeremiah Mahler 
---
 Documentation/technical/api-strbuf.txt | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/technical/api-strbuf.txt 
b/Documentation/technical/api-strbuf.txt
index 077a709..ab430d9 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -149,6 +149,24 @@ Functions
than zero if the first buffer is found, respectively, to be less than,
to match, or be greater than the second buffer.
 
+* Setting the buffer
+
+`strbuf_set`::
+
+Set the buffer to some data up to a given length.
+
+`strbuf_setstr`::
+
+   Set the buffer to a NUL-terminated string.
+
+`strbuf_setf`::
+
+   Set the buffer to a formatted string.
+
+`strbuf_setbuf`::
+
+   Set the current buffer to the contents of some other buffer.
+
 * Adding data to the buffer
 
 NOTE: All of the functions in this section will grow the buffer as necessary.
-- 
2.0.0.573.ged771ce.dirty

--
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/RFC v1 0/5] add strbuf_set operations

[Jeremiah Mahler]

Currently, the data in a strbuf is modified using add operations.  To
set the buffer to some data a reset must be performed before an add.

  strbuf_reset(buf);
  strbuf_add(buf, cb.buf.buf, cb.buf.len);

And this is a common sequence of operations with 70 occurrences found in
the current source code.  This includes all the different variations
(add, addf, addstr, addbuf, addch).

  FILES=`find ./ -name '*.c'`
  CNT=$(pcregrep -M "strbuf_reset.*\n.*strbuf_add" $FILES | wc -l)
  CNT=$(echo "$CNT / 2" | bc)
  echo $CNT
  70

These patches add strbuf_set operations which allow this common sequence
to be performed in one line instead of two.

  strbuf_set(buf, cb.buf.buf, cb.buf.len);

Only the first few files have been converted in this preliminary patch set.

Jeremiah Mahler (5):
  add strbuf_set operations
  add strbuf_set operations documentation
  sha1_name.c: cleanup using strbuf_set operations
  fast-import.c: cleanup using strbuf_set operations
  builtin/remote.c: cleanup using strbuf_set operations

 Documentation/technical/api-strbuf.txt | 18 
 builtin/remote.c   | 51 --
 fast-import.c  | 19 -
 sha1_name.c| 15 --
 strbuf.c   | 21 ++
 strbuf.h   | 14 ++
 6 files changed, 81 insertions(+), 57 deletions(-)

-- 
2.0.0.573.ged771ce.dirty

--
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/RFC v1 2/5] add strbuf_set operations documentation

[Jeremiah Mahler]

Eric,

On Mon, Jun 09, 2014 at 05:53:49AM -0400, Eric Sunshine wrote:
> On Mon, Jun 9, 2014 at 4:36 AM, Jeremiah Mahler  wrote:
> > Add documentation of the strbuf_set operations to
> > technical/api-strbuf.txt.
> 
> Since this patch is concise and so closely related to patch 1/5, it
> probably should be squashed into that one.
> 
Fixed.

> More below.
> 
> > Signed-off-by: Jeremiah Mahler 
> > ---
> >  Documentation/technical/api-strbuf.txt | 18 ++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/Documentation/technical/api-strbuf.txt 
> > b/Documentation/technical/api-strbuf.txt
> > index 077a709..ab430d9 100644
> > --- a/Documentation/technical/api-strbuf.txt
> > +++ b/Documentation/technical/api-strbuf.txt
> > @@ -149,6 +149,24 @@ Functions
> > than zero if the first buffer is found, respectively, to be less 
> > than,
> > to match, or be greater than the second buffer.
> >
> > +* Setting the buffer
> > +
> > +`strbuf_set`::
> > +
> > +Set the buffer to some data up to a given length.
> 
> I personally find this slightly ambiguous. Upon reading it, the first
> question that pops into my mind is whether or not the existing strbuf
> content is replaced (even though "set" should imply that it is). I
> wonder if it would make sense to rewrite as:
> 
> Set the buffer to [...], replacing the old content
> of the buffer.
> 
> Alternately:
> 
> Replace the buffer content with [...].
> 
On a second reading, I agree that it is ambigous.  'Replace' is much
more clear.  Great suggestion.

> Ditto for the others.
> 
> > +`strbuf_setstr`::
> > +
> > +   Set the buffer to a NUL-terminated string.
> > +
> > +`strbuf_setf`::
> > +
> > +   Set the buffer to a formatted string.
> > +
> > +`strbuf_setbuf`::
> > +
> > +   Set the current buffer to the contents of some other buffer.
> > +
> >  * Adding data to the buffer
> >
> >  NOTE: All of the functions in this section will grow the buffer as 
> > necessary.
> > --

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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/RFC v1 4/5] fast-import.c: cleanup using strbuf_set operations

[Jeremiah Mahler]

Eric,

On Mon, Jun 09, 2014 at 06:12:12AM -0400, Eric Sunshine wrote:
> On Mon, Jun 9, 2014 at 4:36 AM, Jeremiah Mahler  wrote:
> > Subject: fast-import.c: cleanup using strbuf_set operations
> 
...
> 
> > Signed-off-by: Jeremiah Mahler 
> > ---
> >  fast-import.c | 19 ++-
> >  1 file changed, 6 insertions(+), 13 deletions(-)
> >
> > diff --git a/fast-import.c b/fast-import.c
> > index e8ec34d..c23935c 100644
> > --- a/fast-import.c
> > +++ b/fast-import.c
> > @@ -2741,8 +2741,7 @@ static void parse_new_commit(void)
> > hashcpy(b->branch_tree.versions[0].sha1,
> > b->branch_tree.versions[1].sha1);
> >
> > -   strbuf_reset(&new_data);
> > -   strbuf_addf(&new_data, "tree %s\n",
> > +   strbuf_setf(&new_data, "tree %s\n",
> > sha1_to_hex(b->branch_tree.versions[1].sha1));
> > if (!is_null_sha1(b->sha1))
> > strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(b->sha1));
> 
> Unlike the cases in patches 3/5 and 5/5 where the strbuf is used or
> returned immediately following the strbuf_set() call, I am not
> convinced that this change is an improvement. This code has the
> general form:
> 
> strbuf_reset(...);
> strbuf_add(...);
> if (condition)
> strbuf_add(...);
> strbuf_add(...);
> 
> in which it is clear that the string is being built piecemeal, and
> it's easy for a programmer to insert, remove, or re-order strbuf_add()
> calls.
> 
> Replacing the first two lines with strbuf_set() somewhat obscures the
> fact that the string is going to be built up piecemeal. Plus, the
> change makes it more difficult to insert, remove, or re-order the
> strbuf_add() invocations.
> 
> This isn't a strong objection, but the benefit of the change seems
> minimal or non-existent.
> 
> Ditto for several remaining cases in this patch.
> 
...

This is a great observation that I certainly did overlook.  Using
strbuf_add or strbuf_set to help make it more obvious what the code is
doing.

By the same token, strbuf_set can be used to replace strbuf_add to make
it clear that nothing important was being added to and that the entire
buffer is being replaced.

  struct strbuf mybuf = STRBUF_INIT;

  strbuf_add(&mybuf, ...);  /* Was something there before? */

  strbuf_set(&mybuf, ...);  /* Replace everything. */

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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/RFC v1 0/5] add strbuf_set operations

[Jeremiah Mahler]

Duy,

On Mon, Jun 09, 2014 at 05:39:12PM +0700, Duy Nguyen wrote:
> On Mon, Jun 9, 2014 at 3:36 PM, Jeremiah Mahler  wrote:
> > Currently, the data in a strbuf is modified using add operations.  To
> > set the buffer to some data a reset must be performed before an add.
> >
> >   strbuf_reset(buf);
> >   strbuf_add(buf, cb.buf.buf, cb.buf.len);
> >
> > And this is a common sequence of operations with 70 occurrences found in
> > the current source code.  This includes all the different variations
> > (add, addf, addstr, addbuf, addch).
> >
> >   FILES=`find ./ -name '*.c'`
> >   CNT=$(pcregrep -M "strbuf_reset.*\n.*strbuf_add" $FILES | wc -l)
> 
> Hmm.. I wonder if git-grep could do this.. There's pcre support but I
> never tried.
> 
Not sure if git-grep does this.  The multi-line (-M) support was the
thing I needed.

> >   CNT=$(echo "$CNT / 2" | bc)
> >   echo $CNT
> >   70
> 
> The change in this series looks nice. There's another pattern, save
> strbuf length, then strbuf_setlen() at the beginning or the end of a
> loop. But I think it's less often.

A quick look did not see any obvious patterns for this.  I think you are
right, there may be fewer cases.

> -- 
> Duy

-- 
Jeremiah Mahler
jmmah...@gmail.com
http://github.com/jmahler
--
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 00/19] add strbuf_set operations

[Jeremiah Mahler]

Version 2 of the patch set to add strbuf_set operations.

Includes suggestions from Eric Sunshine [1]:

  - New operations and their documentation placed in one patch.

  - Less ambiguous documentation: "Replace the buffer content with [...]"

  - Use imperative mood in log messages.

  - Don't use strbuf_set operations in cases where strbuf_add is used to
build up a buffer in multiple steps.  Multiple adds suggest that
re-ordering is possible whereas a strbuf_set at the beginning
suggests that it isn't.  This is confusing.

Using strbuf_set before a sequence of adds can be confusing.  But using
strbuf_add when nothing important was being added to can also be
confusing.  strbuf_set can be used to make these cases more clear.

  struct strbuf mybuf = STRBUF_INIT;

  strbuf_add(&mybuf, ...);  /* Was something there before? */

  strbuf_set(&mybuf, ...);  /* Replace everything. */

Several single line replacements were made for this reason.

Additional files have also been converted compared to version 1 [1].

[1]: http://marc.info/?l=git&m=140230874124060&w=2

Jeremiah Mahler (19):
  add strbuf_set operations
  sha1_name: simplify via strbuf_set()
  fast-import: simplify via strbuf_set()
  builtin/remote: simplify via strbuf_set()
  branch: simplify via strbuf_set()
  builtin/branch: simplify via strbuf_set()
  builtin/checkout: simplify via strbuf_set()
  builtin/mailinfo: simplify via strbuf_set()
  builtin/tag: simplify via strbuf_set()
  date: simplify via strbuf_set()
  diffcore-order: simplify via strbuf_set()
  http-backend: simplify via strbuf_set()
  http: simplify via strbuf_set()
  ident: simplify via strbuf_set()
  remote-curl: simplify via strbuf_set()
  submodule: simplify via strbuf_set()
  transport: simplify via strbuf_set()
  vcs-svn/svndump: simplify via strbuf_set()
  wt-status: simplify via strbuf_set()

 Documentation/technical/api-strbuf.txt | 18 +++
 branch.c   |  6 ++--
 builtin/branch.c   |  3 +-
 builtin/checkout.c | 18 ---
 builtin/mailinfo.c | 18 ---
 builtin/remote.c   | 59 --
 builtin/tag.c  |  3 +-
 date.c |  3 +-
 diffcore-order.c   |  3 +-
 fast-import.c  |  6 ++--
 http-backend.c |  6 ++--
 http.c |  3 +-
 ident.c|  6 ++--
 remote-curl.c  |  3 +-
 sha1_name.c| 15 +++--
 strbuf.c   | 21 
 strbuf.h   | 14 
 submodule.c|  5 ++-
 transport.c|  3 +-
 vcs-svn/svndump.c  |  3 +-
 wt-status.c|  3 +-
 21 files changed, 110 insertions(+), 109 deletions(-)

-- 
2.0.0.592.gf55b190

--
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 01/19] add strbuf_set operations

[Jeremiah Mahler]

Currently, the data in a strbuf is modified using add operations.  To
set the buffer to some data a reset must be performed before an add.

  strbuf_reset(buf);
  strbuf_add(buf, cb.buf.buf, cb.buf.len);

And this is a common sequence of operations with 70 occurrences found in
the current source code.  This includes all the different variations
(add, addf, addstr, addbuf, addch).

  FILES=`find ./ -name '*.c'`
  CNT=$(pcregrep -M "strbuf_reset.*\n.*strbuf_add" $FILES | wc -l)
  CNT=$(echo "$CNT / 2" | bc)
  echo $CNT
  70

These patches add strbuf_set operations which allow this common sequence
to be performed in one line instead of two.

  strbuf_set(buf, cb.buf.buf, cb.buf.len);

Signed-off-by: Jeremiah Mahler 
---
 Documentation/technical/api-strbuf.txt | 18 ++
 strbuf.c   | 21 +
 strbuf.h   | 14 ++
 3 files changed, 53 insertions(+)

diff --git a/Documentation/technical/api-strbuf.txt 
b/Documentation/technical/api-strbuf.txt
index 077a709..b7e23da 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -149,6 +149,24 @@ Functions
than zero if the first buffer is found, respectively, to be less than,
to match, or be greater than the second buffer.
 
+* Setting the buffer
+
+`strbuf_set`::
+
+   Replace the buffer content with data of a given length.
+
+`strbuf_setstr`::
+
+   Replace the buffer content with data from a NUL-terminated string.
+
+`strbuf_setf`::
+
+   Replace the buffer content with a formatted string.
+
+`strbuf_setbuf`::
+
+   Replace the buffer content with data from another buffer.
+
 * Adding data to the buffer
 
 NOTE: All of the functions in this section will grow the buffer as necessary.
diff --git a/strbuf.c b/strbuf.c
index ac62982..9d64b00 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -189,6 +189,27 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t 
len,
strbuf_setlen(sb, sb->len + dlen - len);
 }
 
+void strbuf_set(struct strbuf *sb, const void *data, size_t len)
+{
+   strbuf_reset(sb);
+   strbuf_add(sb, data, len);
+}
+
+void strbuf_setf(struct strbuf *sb, const char *fmt, ...)
+{
+   va_list ap;
+   strbuf_reset(sb);
+   va_start(ap, fmt);
+   strbuf_vaddf(sb, fmt, ap);
+   va_end(ap);
+}
+
+void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2)
+{
+   strbuf_reset(sb);
+   strbuf_add(sb, sb2->buf, sb2->len);
+}
+
 void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t len)
 {
strbuf_splice(sb, pos, 0, data, len);
diff --git a/strbuf.h b/strbuf.h
index e9ad03e..b339f08 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -101,6 +101,20 @@ static inline struct strbuf **strbuf_split(const struct 
strbuf *sb,
  */
 extern void strbuf_list_free(struct strbuf **);
 
+/*- set buffer to data -*/
+
+extern void strbuf_set(struct strbuf *sb, const void *data, size_t len);
+
+static inline void strbuf_setstr(struct strbuf *sb, const char *s)
+{
+   strbuf_set(sb, s, strlen(s));
+}
+
+__attribute__((format (printf,2,3)))
+extern void strbuf_setf(struct strbuf *sb, const char *fmt, ...);
+
+extern void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2);
+
 /*- add data in your buffer -*/
 static inline void strbuf_addch(struct strbuf *sb, int c)
 {
-- 
2.0.0.592.gf55b190

--
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 07/19] builtin/checkout: simplify via strbuf_set()

[Jeremiah Mahler]

Simplify cases where a strbuf_reset is immediately followed by a
strbuf_add by using strbuf_set operations.

Signed-off-by: Jeremiah Mahler 
---
 builtin/checkout.c | 18 ++
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9cbe7d1..38fc0ce 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -912,8 +912,7 @@ static int prepare_linked_checkout(const struct 
checkout_opts *opts,
  sb_git.buf);
junk_work_tree = path;
 
-   strbuf_reset(&sb);
-   strbuf_addf(&sb, "%s/gitdir", sb_repo.buf);
+   strbuf_setf(&sb, "%s/gitdir", sb_repo.buf);
write_file(sb.buf, 1, "%s\n", real_path(sb_git.buf));
write_file(sb_git.buf, 1, "gitdir: %s/repos/%s\n",
   real_path(get_git_common_dir()), name);
@@ -923,11 +922,9 @@ static int prepare_linked_checkout(const struct 
checkout_opts *opts,
 * value would do because this value will be ignored and
 * replaced at the next (real) checkout.
 */
-   strbuf_reset(&sb);
-   strbuf_addf(&sb, "%s/HEAD", sb_repo.buf);
+   strbuf_setf(&sb, "%s/HEAD", sb_repo.buf);
write_file(sb.buf, 1, "%s\n", sha1_to_hex(new->commit->object.sha1));
-   strbuf_reset(&sb);
-   strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
+   strbuf_setf(&sb, "%s/commondir", sb_repo.buf);
write_file(sb.buf, 1, "../..\n");
 
if (!opts->quiet)
@@ -942,8 +939,7 @@ static int prepare_linked_checkout(const struct 
checkout_opts *opts,
ret = run_command(&cp);
if (!ret)
is_junk = 0;
-   strbuf_reset(&sb);
-   strbuf_addf(&sb, "%s/locked", sb_repo.buf);
+   strbuf_setf(&sb, "%s/locked", sb_repo.buf);
unlink_or_warn(sb.buf);
strbuf_release(&sb);
strbuf_release(&sb_repo);
@@ -1048,8 +1044,7 @@ static void check_linked_checkouts(struct branch_info 
*new)
return;
}
 
-   strbuf_reset(&path);
-   strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
+   strbuf_setf(&path, "%s/HEAD", get_git_common_dir());
/*
 * $GIT_COMMON_DIR/HEAD is practically outside
 * $GIT_DIR so resolve_ref_unsafe() won't work (it
@@ -1064,8 +1059,7 @@ static void check_linked_checkouts(struct branch_info 
*new)
while ((d = readdir(dir)) != NULL) {
if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
continue;
-   strbuf_reset(&path);
-   strbuf_addf(&path, "%s/repos/%s/HEAD",
+   strbuf_setf(&path, "%s/repos/%s/HEAD",
get_git_common_dir(), d->d_name);
if (check_linked_checkout(new, d->d_name, path.buf))
break;
-- 
2.0.0.592.gf55b190

--
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 05/19] branch: simplify via strbuf_set()

[Jeremiah Mahler]

Simplify cases where a strbuf_reset is immediately followed by a
strbuf_add by using strbuf_set operations.

Signed-off-by: Jeremiah Mahler 
---
 branch.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/branch.c b/branch.c
index 660097b..bc7cc7e 100644
--- a/branch.c
+++ b/branch.c
@@ -65,13 +65,11 @@ void install_branch_config(int flag, const char *local, 
const char *origin, cons
strbuf_addf(&key, "branch.%s.remote", local);
git_config_set(key.buf, origin ? origin : ".");
 
-   strbuf_reset(&key);
-   strbuf_addf(&key, "branch.%s.merge", local);
+   strbuf_setf(&key, "branch.%s.merge", local);
git_config_set(key.buf, remote);
 
if (rebasing) {
-   strbuf_reset(&key);
-   strbuf_addf(&key, "branch.%s.rebase", local);
+   strbuf_setf(&key, "branch.%s.rebase", local);
git_config_set(key.buf, "true");
}
strbuf_release(&key);
-- 
2.0.0.592.gf55b190

--
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 06/19] builtin/branch: simplify via strbuf_set()

[Jeremiah Mahler]

Simplify cases where a strbuf_reset is immediately followed by a
strbuf_add by using strbuf_set operations.

Signed-off-by: Jeremiah Mahler 
---
 builtin/branch.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 2d1c57c..ad641b6 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -984,8 +984,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
 
strbuf_addf(&buf, "branch.%s.remote", branch->name);
git_config_set_multivar(buf.buf, NULL, NULL, 1);
-   strbuf_reset(&buf);
-   strbuf_addf(&buf, "branch.%s.merge", branch->name);
+   strbuf_setf(&buf, "branch.%s.merge", branch->name);
git_config_set_multivar(buf.buf, NULL, NULL, 1);
strbuf_release(&buf);
} else if (argc > 0 && argc <= 2) {
-- 
2.0.0.592.gf55b190

--
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 02/19] sha1_name: simplify via strbuf_set()

[Jeremiah Mahler]

Simplify cases where a strbuf_reset is immediately followed by a
strbuf_add by using strbuf_set operations.

Signed-off-by: Jeremiah Mahler 
---
 sha1_name.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 2b6322f..f88b66c 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -920,8 +920,7 @@ static int grab_nth_branch_switch(unsigned char *osha1, 
unsigned char *nsha1,
return 0;
if (--(cb->remaining) == 0) {
len = target - match;
-   strbuf_reset(&cb->buf);
-   strbuf_add(&cb->buf, match, len);
+   strbuf_set(&cb->buf, match, len);
return 1; /* we are done */
}
return 0;
@@ -957,8 +956,7 @@ static int interpret_nth_prior_checkout(const char *name, 
int namelen,
 
retval = 0;
if (0 < for_each_reflog_ent_reverse("HEAD", grab_nth_branch_switch, 
&cb)) {
-   strbuf_reset(buf);
-   strbuf_add(buf, cb.buf.buf, cb.buf.len);
+   strbuf_set(buf, cb.buf.buf, cb.buf.len);
retval = brace - name + 1;
}
 
@@ -1025,8 +1023,7 @@ static int interpret_empty_at(const char *name, int 
namelen, int len, struct str
if (next != name + 1)
return -1;
 
-   strbuf_reset(buf);
-   strbuf_add(buf, "HEAD", 4);
+   strbuf_set(buf, "HEAD", 4);
return 1;
 }
 
@@ -1044,8 +1041,7 @@ static int reinterpret(const char *name, int namelen, int 
len, struct strbuf *bu
strbuf_setlen(buf, used);
return len;
}
-   strbuf_reset(buf);
-   strbuf_addbuf(buf, &tmp);
+   strbuf_setbuf(buf, &tmp);
strbuf_release(&tmp);
/* tweak for size of {-N} versus expanded ref name */
return ret - used + len;
@@ -1054,8 +1050,7 @@ static int reinterpret(const char *name, int namelen, int 
len, struct strbuf *bu
 static void set_shortened_ref(struct strbuf *buf, const char *ref)
 {
char *s = shorten_unambiguous_ref(ref, 0);
-   strbuf_reset(buf);
-   strbuf_addstr(buf, s);
+   strbuf_setstr(buf, s);
free(s);
 }
 
-- 
2.0.0.592.gf55b190

--
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 04/19] builtin/remote: simplify via strbuf_set()

[Jeremiah Mahler]

Simplify cases where a strbuf_reset is immediately followed by a
strbuf_add by using strbuf_set operations.

Signed-off-by: Jeremiah Mahler 
---
 builtin/remote.c | 59 
 1 file changed, 21 insertions(+), 38 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index c9b67ff..f2d809d 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -193,8 +193,7 @@ static int add(int argc, const char **argv)
return 1;
 
if (!mirror || mirror & MIRROR_FETCH) {
-   strbuf_reset(&buf);
-   strbuf_addf(&buf, "remote.%s.fetch", name);
+   strbuf_setf(&buf, "remote.%s.fetch", name);
if (track.nr == 0)
string_list_append(&track, "*");
for (i = 0; i < track.nr; i++) {
@@ -205,15 +204,13 @@ static int add(int argc, const char **argv)
}
 
if (mirror & MIRROR_PUSH) {
-   strbuf_reset(&buf);
-   strbuf_addf(&buf, "remote.%s.mirror", name);
+   strbuf_setf(&buf, "remote.%s.mirror", name);
if (git_config_set(buf.buf, "true"))
return 1;
}
 
if (fetch_tags != TAGS_DEFAULT) {
-   strbuf_reset(&buf);
-   strbuf_addf(&buf, "remote.%s.tagopt", name);
+   strbuf_setf(&buf, "remote.%s.tagopt", name);
if (git_config_set(buf.buf,
fetch_tags == TAGS_SET ? "--tags" : "--no-tags"))
return 1;
@@ -223,11 +220,9 @@ static int add(int argc, const char **argv)
return 1;
 
if (master) {
-   strbuf_reset(&buf);
-   strbuf_addf(&buf, "refs/remotes/%s/HEAD", name);
+   strbuf_setf(&buf, "refs/remotes/%s/HEAD", name);
 
-   strbuf_reset(&buf2);
-   strbuf_addf(&buf2, "refs/remotes/%s/%s", name, master);
+   strbuf_setf(&buf2, "refs/remotes/%s/%s", name, master);
 
if (create_symref(buf.buf, buf2.buf, "remote add"))
return error(_("Could not setup master '%s'"), master);
@@ -584,19 +579,17 @@ static int migrate_file(struct remote *remote)
int i;
const char *path = NULL;
 
-   strbuf_addf(&buf, "remote.%s.url", remote->name);
+   strbuf_setf(&buf, "remote.%s.url", remote->name);
for (i = 0; i < remote->url_nr; i++)
if (git_config_set_multivar(buf.buf, remote->url[i], "^$", 0))
return error(_("Could not append '%s' to '%s'"),
remote->url[i], buf.buf);
-   strbuf_reset(&buf);
-   strbuf_addf(&buf, "remote.%s.push", remote->name);
+   strbuf_setf(&buf, "remote.%s.push", remote->name);
for (i = 0; i < remote->push_refspec_nr; i++)
if (git_config_set_multivar(buf.buf, remote->push_refspec[i], 
"^$", 0))
return error(_("Could not append '%s' to '%s'"),
remote->push_refspec[i], buf.buf);
-   strbuf_reset(&buf);
-   strbuf_addf(&buf, "remote.%s.fetch", remote->name);
+   strbuf_setf(&buf, "remote.%s.fetch", remote->name);
for (i = 0; i < remote->fetch_refspec_nr; i++)
if (git_config_set_multivar(buf.buf, remote->fetch_refspec[i], 
"^$", 0))
return error(_("Could not append '%s' to '%s'"),
@@ -640,27 +633,24 @@ static int mv(int argc, const char **argv)
if (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))
die(_("remote %s already exists."), rename.new);
 
-   strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new);
+   strbuf_setf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new);
if (!valid_fetch_refspec(buf.buf))
die(_("'%s' is not a valid remote name"), rename.new);
 
-   strbuf_reset(&buf);
-   strbuf_addf(&buf, "remote.%s", rename.old);
-   strbuf_addf(&buf2, "remote.%s", rename.new);
+   strbuf_setf(&buf, "remote.%s", rename.old);
+   strbuf_setf(&buf2, "remote.%s", rename.new);
if (git_config_rename_section(buf.buf, buf2.buf) < 1)
return error(_("Could not rename config section '%s' to 

[PATCH v2 03/19] fast-import: simplify via strbuf_set()

[Jeremiah Mahler]

Simplify cases where a strbuf_reset is immediately followed by a
strbuf_add by using strbuf_set operations.

Signed-off-by: Jeremiah Mahler 
---
 fast-import.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index e8ec34d..cfe9404 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2898,8 +2898,7 @@ static void cat_blob(struct object_entry *oe, unsigned 
char sha1[20])
 * Output based on batch_one_object() from cat-file.c.
 */
if (type <= 0) {
-   strbuf_reset(&line);
-   strbuf_addf(&line, "%s missing\n", sha1_to_hex(sha1));
+   strbuf_setf(&line, "%s missing\n", sha1_to_hex(sha1));
cat_blob_write(line.buf, line.len);
strbuf_release(&line);
free(buf);
@@ -2910,8 +2909,7 @@ static void cat_blob(struct object_entry *oe, unsigned 
char sha1[20])
if (type != OBJ_BLOB)
die("Object %s is a %s but a blob was expected.",
sha1_to_hex(sha1), typename(type));
-   strbuf_reset(&line);
-   strbuf_addf(&line, "%s %s %lu\n", sha1_to_hex(sha1),
+   strbuf_setf(&line, "%s %s %lu\n", sha1_to_hex(sha1),
typename(type), size);
cat_blob_write(line.buf, line.len);
strbuf_release(&line);
-- 
2.0.0.592.gf55b190

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