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

Josh Triplett [off-list ref] 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.

On Mon, Aug 01, 2016 at 02:18:47PM -0700, Junio C Hamano wrote:

Josh Triplett [off-list ref] writes:

quoted

+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).

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

quoted

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

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:

quoted

quoted

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

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

quoted

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