Re: [PATCH 1/2] format-patch: Add a config option format.from to set the default for --from

[Josh Triplett]

On Mon, Aug 01, 2016 at 01:32:49PM -0700, Junio C Hamano wrote:
> Josh Triplett  writes:
> 
> > +static char *from;
> >  static const char *signature = git_version_string;
> >  static const char *signature_file;
> >  static int config_cover_letter;
> > @@ -807,6 +808,17 @@ static int git_format_config(const char *var, const 
> > char *value, void *cb)
> > base_auto = git_config_bool(var, value);
> > return 0;
> > }
> > +   if (!strcmp(var, "format.from")) {
> > +   int b = git_config_maybe_bool(var, value);
> > +   free(from);
> > +   if (b < 0)
> > +   from = xstrdup(value);
> > +   else if (b)
> > +   from = xstrdup(git_committer_info(IDENT_NO_DATE));
> > +   else
> > +   from = NULL;
> > +   return 0;
> > +   }
> 
> One potential issue I see here is that if we ever plan to switch the
> default, we may want to issue a warning message to users who do not
> have any format.from configured when they do run the program on a
> commit that will get a different result before and after the switch
> in a release of Git before that default switch happens.  The message
> would say something like "you are formatting somebody else's commit.
> the output will change in future versions of Git and show an explicit
> in-body From: header; if you want to keep the current behaviour, set
> format.from configuration variable to false".

The previous discussion between you and Jeff King seemed to suggest that
a mention in the release notes might suffice, rather than a "noisy
deprecation" warning.

> But you cannot tell by looking at from that is NULL between two
> cases, it is NULL because we defaulted to it (in which case we do
> want to warn), or the user set it explicitly to false (we do not
> want to give the warning).

If we wanted to issue such a warning, I'd suggest a separate boolean
"from_set", set when either the configuration or command line sets
"from", and then the code that handles "from" could emit a warning to
stderr if it would produce different results and !from_set.

- Josh Triplett
--
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/2] format-patch: Add a config option format.from to set the default for --from

[Junio C Hamano]

Josh Triplett  writes:

> +static char *from;
>  static const char *signature = git_version_string;
>  static const char *signature_file;
>  static int config_cover_letter;
> @@ -807,6 +808,17 @@ static int git_format_config(const char *var, const char 
> *value, void *cb)
>   base_auto = git_config_bool(var, value);
>   return 0;
>   }
> + if (!strcmp(var, "format.from")) {
> + int b = git_config_maybe_bool(var, value);
> + free(from);
> + if (b < 0)
> + from = xstrdup(value);
> + else if (b)
> + from = xstrdup(git_committer_info(IDENT_NO_DATE));
> + else
> + from = NULL;
> + return 0;
> + }

One potential issue I see here is that if we ever plan to switch the
default, we may want to issue a warning message to users who do not
have any format.from configured when they do run the program on a
commit that will get a different result before and after the switch
in a release of Git before that default switch happens.  The message
would say something like "you are formatting somebody else's commit.
the output will change in future versions of Git and show an explicit
in-body From: header; if you want to keep the current behaviour, set
format.from configuration variable to false".

But you cannot tell by looking at from that is NULL between two
cases, it is NULL because we defaulted to it (in which case we do
want to warn), or the user set it explicitly to false (we do not
want to give the warning).

So "... makes it easier to change the default in the future." in the
log message is a bit of exaggeration.  The change in this patch is
not there yet.

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 1206c48..b0579dd 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -229,6 +229,46 @@ check_patch () {
>   grep -e "^Subject:" "$1"
>  }
>  
> +test_expect_success 'format.from=false' '
> +
> + git -c format.from=false format-patch --stdout master..side |
> + sed -e "/^\$/q" >patch &&
> + check_patch patch &&
> + ! grep "^From: C O Mitter \$" patch

I know you are only mimicking the style of the existing tests but
the piping loses a potential error exit code from format-patch.  I'd
suggest leaving this as low-hanging fruit for later, not fixing them
as part of this series.

This stops at the blank after the header, so there is no point doing
master..side to format three patches.  Just do "-1 side" instead?

More importantly, this only checks the From: in the header, which is
just the half of what --from does.  Shouldn't we be testing that the
in-body From: is added as necessary?

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

Re: [PATCH 1/2] format-patch: Add a config option format.from to set the default for --from

[Jeff King]

On Sat, Jul 30, 2016 at 11:12:46AM -0700, Josh Triplett wrote:

> > These tests follow a different style from the "--from" tests later in
> > the script (and your second patch does follow it, and puts its test
> > close there). Any reason not to have all of the "from" tests together,
> > and using the same style?
> 
> The tests covered different things.  The later --from tests made sure
> that --from behaved as expected.  These tests made sure that format.from
> and --from/--no-from interacted in the expected way, with the
> command-line options overriding the configuration.  So, I put them next
> to the tests for other options like format.to and format.cc, which
> tested the same thing (overriding those with --no-to, --no-cc, etc).

OK. I would have grouped by "things that influence this area of
behavior", not by "config versus command-line". But I don't think either
is wrong or right. And since you are the one writing the patch, "how I
would have done it" is not a compelling review comment.

-Peff
--
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/2] format-patch: Add a config option format.from to set the default for --from

[Josh Triplett]

On Sat, Jul 30, 2016 at 11:40:34AM -0400, Jeff King wrote:
> On Sat, Jul 30, 2016 at 02:41:56AM -0700, Josh Triplett wrote:
> 
> > @@ -807,6 +808,17 @@ static int git_format_config(const char *var, const 
> > char *value, void *cb)
> > base_auto = git_config_bool(var, value);
> > return 0;
> > }
> > +   if (!strcmp(var, "format.from")) {
> > +   int b = git_config_maybe_bool(var, value);
> > +   free(from);
> > +   if (b < 0)
> > +   from = xstrdup(value);
> > +   else if (b)
> > +   from = xstrdup(git_committer_info(IDENT_NO_DATE));
> > +   else
> > +   from = NULL;
> > +   return 0;
> > +   }
> 
> This "free old, then handle tri-state" mirrors the code in the parseopt
> callback pretty closely. I wonder if they could share the logic (it is
> not many lines, but we would want the logic to stay identical). I
> suspect the helper function would end up with more boilerplate than it's
> worth, though, trying to handle the unset and default cases.

I looked at trying to share that code for exactly that reason, but
didn't find a convenient way to share the two, because from_callback
checked two separate variables (unset and arg), while the logic above
checks one.  So, while the *bodies* of the three-way if are duplicated,
the *conditions* aren't.

However, if you'd like to avoid the duplication between the three
values, I can do that with a set_from function that takes an enum and a
new value; it'll actually increase lines of code, but remove the
duplication (as well as the second patch's third copy of setting from to
the committer info).

> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> > index 1206c48..b0579dd 100755
> > --- a/t/t4014-format-patch.sh
> > +++ b/t/t4014-format-patch.sh
> > @@ -229,6 +229,46 @@ check_patch () {
> > grep -e "^Subject:" "$1"
> >  }
> >  
> > +test_expect_success 'format.from=false' '
> > +
> > +   git -c format.from=false format-patch --stdout master..side |
> > +   sed -e "/^\$/q" >patch &&
> > +   check_patch patch &&
> > +   ! grep "^From: C O Mitter \$" patch
> > +'
> 
> These tests follow a different style from the "--from" tests later in
> the script (and your second patch does follow it, and puts its test
> close there). Any reason not to have all of the "from" tests together,
> and using the same style?

The tests covered different things.  The later --from tests made sure
that --from behaved as expected.  These tests made sure that format.from
and --from/--no-from interacted in the expected way, with the
command-line options overriding the configuration.  So, I put them next
to the tests for other options like format.to and format.cc, which
tested the same thing (overriding those with --no-to, --no-cc, etc).

> Overall, the whole thing looks cleanly done, and I don't mind it going
> in as-is. These are just two things I noticed while reading it over.

I'll send a v2 with the code duplication fixed.

- Josh Triplett
--
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/2] format-patch: Add a config option format.from to set the default for --from

[Jeff King]

On Sat, Jul 30, 2016 at 02:41:56AM -0700, Josh Triplett wrote:

> @@ -807,6 +808,17 @@ static int git_format_config(const char *var, const char 
> *value, void *cb)
>   base_auto = git_config_bool(var, value);
>   return 0;
>   }
> + if (!strcmp(var, "format.from")) {
> + int b = git_config_maybe_bool(var, value);
> + free(from);
> + if (b < 0)
> + from = xstrdup(value);
> + else if (b)
> + from = xstrdup(git_committer_info(IDENT_NO_DATE));
> + else
> + from = NULL;
> + return 0;
> + }

This "free old, then handle tri-state" mirrors the code in the parseopt
callback pretty closely. I wonder if they could share the logic (it is
not many lines, but we would want the logic to stay identical). I
suspect the helper function would end up with more boilerplate than it's
worth, though, trying to handle the unset and default cases.

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 1206c48..b0579dd 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -229,6 +229,46 @@ check_patch () {
>   grep -e "^Subject:" "$1"
>  }
>  
> +test_expect_success 'format.from=false' '
> +
> + git -c format.from=false format-patch --stdout master..side |
> + sed -e "/^\$/q" >patch &&
> + check_patch patch &&
> + ! grep "^From: C O Mitter \$" patch
> +'

These tests follow a different style from the "--from" tests later in
the script (and your second patch does follow it, and puts its test
close there). Any reason not to have all of the "from" tests together,
and using the same style?


Overall, the whole thing looks cleanly done, and I don't mind it going
in as-is. These are just two things I noticed while reading it over.

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

[PATCH 1/2] format-patch: Add a config option format.from to set the default for --from

[Josh Triplett]

This helps users who would prefer format-patch to default to --from, and
makes it easier to change the default in the future.

Signed-off-by: Josh Triplett 
---
 Documentation/config.txt   | 10 +++-
 builtin/log.c  | 13 -
 contrib/completion/git-completion.bash |  1 +-
 t/t4014-format-patch.sh| 40 +++-
 4 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8b1aee4..bd34774 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1253,6 +1253,16 @@ format.attach::
value as the boundary.  See the --attach option in
linkgit:git-format-patch[1].
 
+format.from::
+   Provides the default value for the `--from` option to format-patch.
+   Accepts a boolean value, or a name and email address.  If false,
+   format-patch defaults to `--no-from`, using commit authors directly in
+   the "From:" field of patch mails.  If true, format-patch defaults to
+   `--from`, using your committer identity in the "From:" field of patch
+   mails and including a "From:" field in the body of the patch mail if
+   different.  If set to a non-boolean value, format-patch uses that
+   value instead of your committer identity.  Defaults to false.
+
 format.numbered::
A boolean which can enable or disable sequence numbers in patch
subjects.  It defaults to "auto" which enables it only if there
diff --git a/builtin/log.c b/builtin/log.c
index fd1652f..1f116be 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -719,6 +719,7 @@ static void add_header(const char *value)
 static int thread;
 static int do_signoff;
 static int base_auto;
+static char *from;
 static const char *signature = git_version_string;
 static const char *signature_file;
 static int config_cover_letter;
@@ -807,6 +808,17 @@ static int git_format_config(const char *var, const char 
*value, void *cb)
base_auto = git_config_bool(var, value);
return 0;
}
+   if (!strcmp(var, "format.from")) {
+   int b = git_config_maybe_bool(var, value);
+   free(from);
+   if (b < 0)
+   from = xstrdup(value);
+   else if (b)
+   from = xstrdup(git_committer_info(IDENT_NO_DATE));
+   else
+   from = NULL;
+   return 0;
+   }
 
return git_log_config(var, value, cb);
 }
@@ -1384,7 +1396,6 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
int quiet = 0;
int reroll_count = -1;
char *branch_name = NULL;
-   char *from = NULL;
char *base_commit = NULL;
struct base_tree_info bases;
 
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 10f6d52..4393033 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2181,6 +2181,7 @@ _git_config ()
format.attach
format.cc
format.coverLetter
+   format.from
format.headers
format.numbered
format.pretty
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 1206c48..b0579dd 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -229,6 +229,46 @@ check_patch () {
grep -e "^Subject:" "$1"
 }
 
+test_expect_success 'format.from=false' '
+
+   git -c format.from=false format-patch --stdout master..side |
+   sed -e "/^\$/q" >patch &&
+   check_patch patch &&
+   ! grep "^From: C O Mitter \$" patch
+'
+
+test_expect_success 'format.from=true' '
+
+   git -c format.from=true format-patch --stdout master..side |
+   sed -e "/^\$/q" >patch &&
+   check_patch patch &&
+   grep "^From: C O Mitter \$" patch
+'
+
+test_expect_success 'format.from with address' '
+
+   git -c format.from="F R Om " format-patch --stdout 
master..side |
+   sed -e "/^\$/q" >patch &&
+   check_patch patch &&
+   grep "^From: F R Om \$" patch
+'
+
+test_expect_success '--no-from overrides format.from' '
+
+   git -c format.from="F R Om " format-patch --no-from 
--stdout master..side |
+   sed -e "/^\$/q" >patch &&
+   check_patch patch &&
+   ! grep "^From: F R Om \$" patch
+'
+
+test_expect_success '--from overrides format.from' '
+
+   git -c format.from="F R Om " format-patch --from 
--stdout master..side |
+   sed -e "/^\$/q" >patch &&
+   check_patch patch &&
+   ! grep "^From: F R Om \$" patch
+'
+
 test_expect_success '--no-to overrides config.to' '
 
git config --replace-all format.to \
-- 
git-series 0.8.7