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

[Junio C Hamano]

Josh Triplett  writes:

> I didn't realize you had already taken the patch series into next; I'd
> assumed from the various comments that you expected me to reroll it
> before you'd take it.
>
> Would you like me to write something up for the release notes regarding
> plans to change the default?

Given that we are at week #8 and -rc0 is coming soon, I suspect that
that note will happen not in this release but in the next one.

The patch in question (1/2) is merely a new convenience feature that
does not have to say anything about the future default, so we are
good with 1/2 as-is (not v2 version of it, but the original one
without enum), I think.




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

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

[Jeff King]

On Sun, Aug 07, 2016 at 07:02:18PM -1000, Josh Triplett wrote:

> > Portability; some compilers choke on it. C89 allows trailing commas in
> > array initialization but _not_ in enums. Most compilers allow it anyway
> > (though gcc complains with -Wpedantic).
> > 
> > This definitely broke the build on real systems early in Git's history
> > (I think the AIX compiler was one culprit),
> 
> Thanks for the explanation.  I assume such compilers also don't accept
> C99?

Correct. We don't allow other C99 features like variadic macros, either
(there are some in the code base, but you'll note they can all be
conditionally disabled).

> Perhaps the next Git user survey could ask "what compiler (including
> version) do you use to compile Git", and perhaps "does it accept the
> following code:"?

Maybe. I'm not sure I would consider a lack of responses there to be a
definite sign. It seems that once every few years people on bizarre
systems come out of the woodwork and do a round of portability fixes,
and then problems accrue, and so on. So I'm not sure that the survey
would hit the right people in a timely manner.

I think the breaking point will be just declaring "look, C99 is N years
old; if your compiler can't handle it, that's now your problem". When
Git started, N was only 6. It's now 17.

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

[Josh Triplett]

On Mon, Aug 08, 2016 at 12:54:41AM -0400, Jeff King wrote:
> On Sun, Aug 07, 2016 at 06:42:07PM -1000, Josh Triplett wrote:
> 
> > > Drop trailing comma after the last enum definition (trailing comma
> > > after the last element in an array is OK, though).
> > 
> > I realize this code didn't get included in the final version, but for
> > future reference, what's the rationale for this?  I tend to include a
> > final comma in cases like these (and likewise for initializers) to avoid
> > needing to change the last line when introducing a new element, reducing
> > noise in diffs.  I hadn't seen anything in any of the coding style
> > documentation talking about trailing commas (either pro or con).
> 
> Portability; some compilers choke on it. C89 allows trailing commas in
> array initialization but _not_ in enums. Most compilers allow it anyway
> (though gcc complains with -Wpedantic).
> 
> This definitely broke the build on real systems early in Git's history
> (I think the AIX compiler was one culprit),

Thanks for the explanation.  I assume such compilers also don't accept
C99?

> but at this point it's
> possible that all of those compilers have died off. It would be nice if
> we could start using it (for exactly the reasons you give).
> Unfortunately there's not a good way to know except "introduce it and
> see if people complain".

Fair enough.  I'll let someone else be the test case for that. :)

Perhaps the next Git user survey could ask "what compiler (including
version) do you use to compile Git", and perhaps "does it accept the
following code:"?

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

[Jeff King]

On Sun, Aug 07, 2016 at 06:42:07PM -1000, Josh Triplett wrote:

> > Drop trailing comma after the last enum definition (trailing comma
> > after the last element in an array is OK, though).
> 
> I realize this code didn't get included in the final version, but for
> future reference, what's the rationale for this?  I tend to include a
> final comma in cases like these (and likewise for initializers) to avoid
> needing to change the last line when introducing a new element, reducing
> noise in diffs.  I hadn't seen anything in any of the coding style
> documentation talking about trailing commas (either pro or con).

Portability; some compilers choke on it. C89 allows trailing commas in
array initialization but _not_ in enums. Most compilers allow it anyway
(though gcc complains with -Wpedantic).

This definitely broke the build on real systems early in Git's history
(I think the AIX compiler was one culprit), but at this point it's
possible that all of those compilers have died off. It would be nice if
we could start using it (for exactly the reasons you give).
Unfortunately there's not a good way to know except "introduce it and
see if people complain".

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

[Josh Triplett]

On Mon, Aug 01, 2016 at 02:18:47PM -0700, Junio C Hamano wrote:
> Josh Triplett  writes:
> > +enum from {
> > +   FROM_AUTHOR,
> > +   FROM_USER,
> > +   FROM_VALUE,
> 
> Drop trailing comma after the last enum definition (trailing comma
> after the last element in an array is OK, though).

I realize this code didn't get included in the final version, but for
future reference, what's the rationale for this?  I tend to include a
final comma in cases like these (and likewise for initializers) to avoid
needing to change the last line when introducing a new element, reducing
noise in diffs.  I hadn't seen anything in any of the coding style
documentation talking about trailing commas (either pro or con).
--
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 1/2] format-patch: Add a config option format.from to set the default for --from

[Josh Triplett]

On Sun, Aug 07, 2016 at 06:59:09PM -0700, Junio C Hamano wrote:
> Josh Triplett  writes:
> 
> > I'd actually seriously considered this exact approach, which I preferred
> > as well, but I'd discarded it because I figured it'd get rejected.
> > Given your suggestion, and Junio's comment, I'll go with this version.
> 
> Sorry, but your response is soo delayed that I am not sure what you
> are agreeing with

I'm on vacation right now.  I was agreeing with your comment that you
didn't care for the change in v2 to use an enum.

> and also am not sure if you are planning to reroll
> what has already been happily accepted to 'next', which is not quite
> welcome.

I didn't realize you had already taken the patch series into next; I'd
assumed from the various comments that you expected me to reroll it
before you'd take it.

Would you like me to write something up for the release notes regarding
plans to change the default?

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

[Junio C Hamano]

Josh Triplett  writes:

> I'd actually seriously considered this exact approach, which I preferred
> as well, but I'd discarded it because I figured it'd get rejected.
> Given your suggestion, and Junio's comment, I'll go with this version.

Sorry, but your response is soo delayed that I am not sure what you
are agreeing with and also am not sure if you are planning to reroll
what has already been happily accepted to 'next', which is not quite
welcome.

--
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 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:38:47PM -0400, Jeff King wrote:
> On Sat, Jul 30, 2016 at 12:11:11PM -0700, Josh Triplett wrote:
> 
> > +enum from {
> > +   FROM_AUTHOR,
> > +   FROM_USER,
> > +   FROM_VALUE,
> > +};
> > +
> > +static void set_from(enum from type, const char *value)
> > +{
> > +   free(from);
> > +   switch (type) {
> > +   case FROM_AUTHOR:
> > +   from = NULL;
> > +   break;
> > +   case FROM_USER:
> > +   from = xstrdup(git_committer_info(IDENT_NO_DATE));
> > +   break;
> > +   case FROM_VALUE:
> > +   from = xstrdup(value);
> > +   break;
> > +   }
> > +}
> 
> Thanks for looking into reducing the duplication. TBH, I am not sure it
> is really an improvement, just because of the amount of boilerplate (and
> this function interface is kind of weird, because of the rules for when
> "value" should or should not be NULL).
> 
> I guess another way to do it would be:
> 
>   #define FROM_AUTO_IDENT ((const char *)(intptr_t)1))
>   void set_from(const char *value)
>   {
>   if (value == FROM_AUTO_IDENT)
>   value = git_committer_info(IDENT_NO_DATE);
>   free(from);
>   from = xstrdup_or_null(value);
>   }

I'd actually seriously considered this exact approach, which I preferred
as well, but I'd discarded it because I figured it'd get rejected.
Given your suggestion, and Junio's comment, I'll go with this version.

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

[Junio C Hamano]

Josh Triplett  writes:

> Subject: Re: [PATCH v2 1/2] format-patch: Add a config option format.from ...

At least s/Add/add/; but I would prefer an even shorter

format-patch: format.from gives the default for --from

> +static char *from;

The same "this does not quite help the transition" comment applies
to this one.

> +enum from {
> + FROM_AUTHOR,
> + FROM_USER,
> + FROM_VALUE,

Drop trailing comma after the last enum definition (trailing comma
after the last element in an array is OK, though).

> +static void set_from(enum from type, const char *value)
> +{
> + free(from);
> + switch (type) {
> + case FROM_AUTHOR:
> + from = NULL;
> + break;
> + case FROM_USER:
> + from = xstrdup(git_committer_info(IDENT_NO_DATE));
> + break;
> + case FROM_VALUE:
> + from = xstrdup(value);
> + break;
> + }
> +}

I tend to agree with what Jeff said; I'd queue 1/2 from the original
round for now.

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

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

[Jeff King]

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

> +enum from {
> + FROM_AUTHOR,
> + FROM_USER,
> + FROM_VALUE,
> +};
> +
> +static void set_from(enum from type, const char *value)
> +{
> + free(from);
> + switch (type) {
> + case FROM_AUTHOR:
> + from = NULL;
> + break;
> + case FROM_USER:
> + from = xstrdup(git_committer_info(IDENT_NO_DATE));
> + break;
> + case FROM_VALUE:
> + from = xstrdup(value);
> + break;
> + }
> +}

Thanks for looking into reducing the duplication. TBH, I am not sure it
is really an improvement, just because of the amount of boilerplate (and
this function interface is kind of weird, because of the rules for when
"value" should or should not be NULL).

I guess another way to do it would be:

  #define FROM_AUTO_IDENT ((const char *)(intptr_t)1))
  void set_from(const char *value)
  {
if (value == FROM_AUTO_IDENT)
value = git_committer_info(IDENT_NO_DATE);
free(from);
from = xstrdup_or_null(value);
  }

but I think the effort to polish further here is outweighing the
magnitude of the patch itself. So I offer that as "how I would have done
it" in case you like it, but again, I am fine with either this version
or the previous.

-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 v2 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  | 46 +--
 contrib/completion/git-completion.bash |  1 +-
 t/t4014-format-patch.sh| 40 +++-
 4 files changed, 88 insertions(+), 9 deletions(-)

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..dbd2da7 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;
@@ -731,6 +732,28 @@ enum {
COVER_AUTO
 };
 
+enum from {
+   FROM_AUTHOR,
+   FROM_USER,
+   FROM_VALUE,
+};
+
+static void set_from(enum from type, const char *value)
+{
+   free(from);
+   switch (type) {
+   case FROM_AUTHOR:
+   from = NULL;
+   break;
+   case FROM_USER:
+   from = xstrdup(git_committer_info(IDENT_NO_DATE));
+   break;
+   case FROM_VALUE:
+   from = xstrdup(value);
+   break;
+   }
+}
+
 static int git_format_config(const char *var, const char *value, void *cb)
 {
if (!strcmp(var, "format.headers")) {
@@ -807,6 +830,16 @@ 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);
+   if (b < 0)
+   set_from(FROM_VALUE, value);
+   else if (b)
+   set_from(FROM_USER, NULL);
+   else
+   set_from(FROM_AUTHOR, NULL);
+   return 0;
+   }
 
return git_log_config(var, value, cb);
 }
@@ -1199,16 +1232,12 @@ static int cc_callback(const struct option *opt, const 
char *arg, int unset)
 
 static int from_callback(const struct option *opt, const char *arg, int unset)
 {
-   char **from = opt->value;
-
-   free(*from);
-
if (unset)
-   *from = NULL;
+   set_from(FROM_AUTHOR, NULL);
else if (arg)
-   *from = xstrdup(arg);
+   set_from(FROM_VALUE, arg);
else
-   *from = xstrdup(git_committer_info(IDENT_NO_DATE));
+   set_from(FROM_USER, NULL);
return 0;
 }
 
@@ -1384,7 +1413,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;
 
@@ -1433,7 +1461,7 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
0, to_callback },
{ OPTION_CALLBACK, 0, "cc", NULL, N_("email"), N_("add Cc: 
header"),
0, cc_callback },
-   { OPTION_CALLBACK, 0, "from", , N_("ident"),
+   { OPTION_CALLBACK, 0, "from", NULL, N_("ident"),
N_("set From address to  (or committer ident 
if absent)"),
PARSE_OPT_OPTARG, from_callback },
OPT_STRING(0, "in-reply-to", _reply_to, N_("message-id"),
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