Re: [PATCH 04/18] Offer a function to demote fsck errors to warnings

2014-12-23 Thread Johannes Schindelin 
Hi Junio,

On Mon, 22 Dec 2014, Junio C Hamano wrote:

 Johannes Schindelin johannes.schinde...@gmx.de writes:
 
  For example, try to spot the error here:
 
  ...
  F(ALMOST_HAPPY, INFO) \
  F(CANNOT_RECOVER, ERROR) \
  F(COFFEE_IS_EMPTY, WARN) \
  F(JUST_BEING_CHATTY, INFO) \
  F(LIFE_IS_GOOD, INFO) \
  F(MISSING_SOMETHING_VITAL, FATAL_ERROR) \
  F(NEED_TO_SLEEP, WARN) \
  F(SOMETHING_WENT_WRONG, ERROR) \
  ...
 
 But that is not what is being suggested at all.  I already said that
 FIRST_SOMETHING is fine as a measure to initialize, didn't I?
 
 I am only saying that if you have a place to store customized level,
 you should initialize that part with default levels and always look
 it up from that place at runtime.  It is perfectly fine for the
 initialization step to take advantage of the ordering and
 FIRST_SOMETHING constants.

Thanks for clarifying, I was worried that you wanted to encode the
severity levels explicitly (F(ID, LEVEL) instead of F(ID) in the correct
order). The DRY principle also suggests that we should not encode the
severity level in two ways (which would leave the door open for
inconsistencies). That means that we should not initialize a static array
of severity levels, but initialize the array using a loop.

Okay, now that we have established that the initial ordering by severity
makes sense, let's examine the initialization step.

Basically, our approaches differ only in *when* to initialize that array
of severity levels: you want to initialize it always, and I want to
initialize it only when the severity levels are customized by the caller.

Now, let's have a look how the fsck_options are currently initialized. My
code follows the convention established with strbufs, argv_arrays, etc:
there is a preprocessor definition (_DEFAULT, imitating the _INIT
definitions) that allows us to initialize such structs very conveniently.
Please note that no loop is required, and certainly no extra code has to
be called to initialize the struct. We get away with initializing that
array lazily in the fsck_strict_mode() function when we detect that it
needs to be initialized, being populated by the very same function that
provides the default settings before customization. This is a very robust
setup as the knowledge about, say, the size of that array is confined
strictly to fsck.c.

However, if we had to change the lookup such that it uses an array always,
we would have to introduce a function to initialize the struct, always, in
particular we would have to find a place to call that initialization
function in, say, builtin/fsck.c (actually, in every code path that calls
into the fsck machinery). Arguably, the code would get more complex –
introducing new call paths just to initialize the fsck_options struct –
and I would argue further that there is no gain from an elegance,
readability and maintenance point of view: whether the array is
initialized lazily or not, it will be initialized the exact same way. All
it means is that we have to introduce separate code paths because we would
separate explicitly the initialization from the configuration step.

Therefore, I do not believe that introducing an fsck_options_init() is
what you would really want.

An alternative would be to initialize the array at compile time – we would
have to violate the DRY principle for that, repeating the severity levels
many times over, and we could no longer confine the visibility to the
message IDs to fsck.c because not only the size of the array of severity
levels would have to be known to every user of fsck.h, but the exact
default severity levels themselves, to be able to initialize the struct.
But we could initialize the struct with a known set of settings via the
_DEFAULTS definition that way.

However, you already expressed slight disagreement with the preprocessor
magic needed to initialize both the enum and the array of message ID
strings from the same list in a way that lets the compiler ensure
consistency; I am afraid that if I were to modify _DEFAULTS to populate the
entire severity level array, the resulting code would find your utter
contempt.

I believe, therefore, that this is also not what you want.

So that leaves only one alternative: to initialize a global array with the
default severity levels at *some* stage. I have no idea what that stage
would be, therefore we would have to either establish ugly, and
DRY-violating, compile time initialization, or we would have to call a
function before using any of the fsck machinery – but that is fragile: it
is too easy to forget one call path and access an uninitialized array!
Worse, even if we *had* a fully initialized array of default severity
levels, we would still have to have an on-demand copy (i.e. lazy
initialization) of said array lest we modify the global defaults in
fsck_strict_mode()! Essentially, we would just *add* complexity to the
current solution, not replace it with anything simpler.

Therefore, I believe

Re: [PATCH 04/18] Offer a function to demote fsck errors to warnings

2014-12-23 Thread Junio C Hamano 
Johannes Schindelin johannes.schinde...@gmx.de writes:

 However, if we had to change the lookup such that it uses an array always,
 we would have to introduce a function to initialize the struct, always, in
 particular we would have to find a place to call that initialization
 function in, say, builtin/fsck.c (actually, in every code path that calls
 into the fsck machinery).

You would need to call a function to initialize the table if you
support customization by reading the configuration files anyway.  I
am not sure why you think finding such a place is hard.  Puzzled.

Also I suspect that you can tell the compiler to initialize the
array in place with default values, perhaps like this?

-- 8 --
#include stdio.h

/* sorted by the default severity (lowest impact first) */
#define EVENT_LIST(F) \
F(EVENT_A), \
F(EVENT_B), \
F(EVENT_C), \
F(EVENT_D)

#define ID_(event) ID_ ## event
enum event_id {
EVENT_LIST(ID_)
};


enum severity_level {
severity_info, severity_warn, severity_error
};

/* below this one are INFO */
#define FIRST_WARN_EVENT_ID ID_EVENT_B
/* below this one are WARN */
#define FIRST_ERROR_EVENT_IDID_EVENT_C

#define STRING_(s) #s
#define DESC_(event) \
{ \
ID_ ## event, \
STRING_(event), \
(ID_ ## event  FIRST_WARN_EVENT_ID \
? severity_info \
: ID_ ## event  FIRST_ERROR_EVENT_ID \
? severity_warn \
: severity_error) \
}

struct event_config {
enum event_id id;
const char * name;
enum severity_level level;
} event[] = {
EVENT_LIST(DESC_)
};

int main(int ac, char **av)
{
int i;

for (i = 0; i  sizeof(event) / sizeof(event[0]); i++) {
printf(%d, %s, %d\n,
   event[i].id, event[i].name, event[i].level);
}
return 0;
}
--
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 04/18] Offer a function to demote fsck errors to warnings

2014-12-23 Thread Johannes Schindelin 
Hi Junio,

On Tue, 23 Dec 2014, Junio C Hamano wrote:

 Johannes Schindelin johannes.schinde...@gmx.de writes:
 
  However, if we had to change the lookup such that it uses an array
  always, we would have to introduce a function to initialize the
  struct, always, in particular we would have to find a place to call
  that initialization function in, say, builtin/fsck.c (actually, in
  every code path that calls into the fsck machinery).
 
 You would need to call a function to initialize the table if you
 support customization by reading the configuration files anyway.

Yes, this is the config machinery. But I need employ that only if I want
to let the caller customize the severity levels. However, the fsck
machinery is also called from places where such a customization is not
offered. They would now need to be changed, too.

 Also I suspect that you can tell the compiler to initialize the
 array in place with default values, perhaps like this?
 
 -- 8 --
 #include stdio.h
 
 /* sorted by the default severity (lowest impact first) */
 #define EVENT_LIST(F) \
   F(EVENT_A), \
   F(EVENT_B), \
   F(EVENT_C), \
   F(EVENT_D)
 
 #define ID_(event) ID_ ## event
 enum event_id {
   EVENT_LIST(ID_)
 };
 
 
 enum severity_level {
   severity_info, severity_warn, severity_error
 };
 
 /* below this one are INFO */
 #define FIRST_WARN_EVENT_ID   ID_EVENT_B
 /* below this one are WARN */
 #define FIRST_ERROR_EVENT_ID  ID_EVENT_C
 
 #define STRING_(s) #s
 #define DESC_(event) \
   { \
   ID_ ## event, \
   STRING_(event), \
   (ID_ ## event  FIRST_WARN_EVENT_ID \
   ? severity_info \
   : ID_ ## event  FIRST_ERROR_EVENT_ID \
   ? severity_warn \
   : severity_error) \
   }

This is exactly the ugly, ugly preprocessor construct I thought you would
meet with contempt. I mean, compared to this, my FUNC() hack is outright
pretty ;-)

And *still*, this is *just* a global table with defaults. I would *still*
need to copy-on-write when the first customization of the severity level
takes place because I cannot allow the global defaults to be modified by
one caller (that would defeat the whole purpose of having per-caller
settings bundled in the fsck_options struct).

You see, I still would need to have a lazy initialization, the complexity
in that part would not be reduced at all.

So I am afraid that this approach really adds complexity rather than
replacing it with something simpler than my current code.

Ciao,
Dscho
--
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 04/18] Offer a function to demote fsck errors to warnings

2014-12-23 Thread Junio C Hamano 
Johannes Schindelin johannes.schinde...@gmx.de writes:

 And *still*, this is *just* a global table with defaults. I would *still*
 need to copy-on-write when the first customization of the severity level
 takes place because I cannot allow the global defaults to be modified by
 one caller (that would defeat the whole purpose of having per-caller
 settings bundled in the fsck_options struct).

If you allocate a per-invocation fsck_options struct, then the
initialization the default with code is dead easy---you can just do
it immediately after you x[cm]alloc()---no?

What am I missing?
--
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 04/18] Offer a function to demote fsck errors to warnings

2014-12-23 Thread Johannes Schindelin 
Hi Junio,

On Tue, 23 Dec 2014, Junio C Hamano wrote:

 Johannes Schindelin johannes.schinde...@gmx.de writes:
 
  And *still*, this is *just* a global table with defaults. I would *still*
  need to copy-on-write when the first customization of the severity level
  takes place because I cannot allow the global defaults to be modified by
  one caller (that would defeat the whole purpose of having per-caller
  settings bundled in the fsck_options struct).
 
 If you allocate a per-invocation fsck_options struct, then the
 initialization the default with code is dead easy---you can just do
 it immediately after you x[cm]alloc()---no?

There is no alloc. Right now, the initialization reads:

struct fsck_options options = strict ?
FSCK_OPTIONS_STRICT : FSCK_OPTIONS_DEFAULT;

Ciao,
Dscho
--
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 04/18] Offer a function to demote fsck errors to warnings

2014-12-23 Thread Junio C Hamano 
On Tue, Dec 23, 2014 at 9:28 AM, Johannes Schindelin
johannes.schinde...@gmx.de wrote:
 Johannes Schindelin johannes.schinde...@gmx.de writes:

  And *still*, this is *just* a global table with defaults. I would *still*
  need to copy-on-write when the first customization of the severity level
  takes place because I cannot allow the global defaults to be modified by
  one caller (that would defeat the whole purpose of having per-caller
  settings bundled in the fsck_options struct).

 There is no alloc. Right now, the initialization reads:

 struct fsck_options options = strict ?
 FSCK_OPTIONS_STRICT : FSCK_OPTIONS_DEFAULT;

Then it is just the matter of having

   fsck_options_init(options);
   if (strict)
options.some_field = make_it_strict;

as the first few statements, no?

I am not sure why it is so difficult
--
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 04/18] Offer a function to demote fsck errors to warnings

2014-12-23 Thread Johannes Schindelin 
Hi Junio,

On Tue, 23 Dec 2014, Junio C Hamano wrote:

 On Tue, Dec 23, 2014 at 9:28 AM, Johannes Schindelin
 johannes.schinde...@gmx.de wrote:
  Johannes Schindelin johannes.schinde...@gmx.de writes:
 
   And *still*, this is *just* a global table with defaults. I would *still*
   need to copy-on-write when the first customization of the severity level
   takes place because I cannot allow the global defaults to be modified by
   one caller (that would defeat the whole purpose of having per-caller
   settings bundled in the fsck_options struct).
 
  There is no alloc. Right now, the initialization reads:
 
  struct fsck_options options = strict ?
  FSCK_OPTIONS_STRICT : FSCK_OPTIONS_DEFAULT;
 
 Then it is just the matter of having
 
fsck_options_init(options);
if (strict)
 options.some_field = make_it_strict;
 
 as the first few statements, no?
 
 I am not sure why it is so difficult

It is not difficult. But I try to avoid complexity when I can. Since you
asked specifically, I will introduce it, though. Hopefully still this year
(I'll not be available for a while starting tomorrow).

Ciao,
Dscho
--
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 04/18] Offer a function to demote fsck errors to warnings

2014-12-22 Thread Johannes Schindelin 
Hi Junio,

On Wed, 10 Dec 2014, Junio C Hamano wrote:

 Johannes Schindelin johannes.schinde...@gmx.de writes:
 
  There are legacy repositories out there whose older commits and tags
  have issues that prevent pushing them when 'receive.fsckObjects' is set.
  One real-life example is a commit object that has been hand-crafted to
  list two authors.
 
  Often, it is not possible to fix those issues without disrupting the
  work with said repositories, yet it is still desirable to perform checks
  by setting `receive.fsckObjects = true`. This commit is the first step
  to allow demoting specific fsck issues to mere warnings.
 
  The function added by this commit parses a list of settings in the form:
 
  missing-email=warn,bad-name=warn,...
 
  Unfortunately, the FSCK_WARN/FSCK_ERROR flag is only really heeded by
  git fsck so far, but other call paths (e.g. git index-pack --strict)
  error out *always* no matter what type was specified. Therefore, we
  need to take extra care to default to all FSCK_ERROR in those cases.
 
  Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
  ---
   fsck.c | 58 ++
   fsck.h |  7 +--
   2 files changed, 63 insertions(+), 2 deletions(-)
 
  diff --git a/fsck.c b/fsck.c
  index 05b146c..9e6d70f 100644
  --- a/fsck.c
  +++ b/fsck.c
  @@ -97,9 +97,63 @@ static int parse_msg_id(const char *text, int len)
   
   int fsck_msg_type(enum fsck_msg_id msg_id, struct fsck_options *options)
   {
  +   if (options-strict_mode  msg_id = 0  msg_id  FSCK_MSG_MAX)
  +   return options-strict_mode[msg_id];
  +   if (options-strict)
  +   return FSCK_ERROR;
  return msg_id  FIRST_WARNING ? FSCK_ERROR : FSCK_WARN;
   }
 
 Hmm, if you are later going to allow demoting (hopefully also promoting)
 error to warn, etc., would the comparison between msg_id and FIRST_WARNING
 make much sense?

A later patch indeed adds that option. The reason the comparison still
makes sense is that the pure infos do not return at all so far, but all of
the reported warnings are fatal in strict mode (i.e. when
receive.fsckObjects = true). In another later patch it is made possible to
promote even infos (such as 'missing tagger') to warnings or even errors,
and that is when the return FSCK_ERROR is changed to return msg_id 
FIRST_INFO ? FSCK_ERROR : FSCK_WARN.

 In other words, at some point wouldn't we be better off with
 something like this
 
   struct {
   enum id;
 const char *id_string;
 enum error_level { FSCK_PASS, FSCK_WARN, FSCK_ERROR };
   } possible_fsck_errors[];

I considered that, and Michael Haggerty also suggested that in a private
mail. However, I find that there is a clear hierarchy in the default
messages: fatal errors, errors, warnings and infos. This should be
reflected by the order IMHO.

But I guess it would make a lot of sense to insert those levels as special
enum values to make it harder to forget to adjust, say, #define
FIRST_WARNING FSCK_MSG_BAD_FILEMODE when introducing another warning that
sorts before said ID alphabetically. In other words, I think that we can
really afford to put something like

...
FUNC(UNKNOWN_TYPE) \
FUNC(ZERO_PADDED_DATE) \
FUNC(___WARNINGS) \
FUNC(BAD_FILEMODE) \
FUNC(EMPTY_NAME) \
...

at the price of making the parsing a little more complicated and wasting a
slight bit of more space for the msg_id_str array.

What do you think?

Ciao,
Dscho
Ciao,
Dscho
--
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 04/18] Offer a function to demote fsck errors to warnings

2014-12-22 Thread Junio C Hamano 
Johannes Schindelin johannes.schinde...@gmx.de writes:

 In other words, at some point wouldn't we be better off with
 something like this
 
  struct {
  enum id;
 const char *id_string;
 enum error_level { FSCK_PASS, FSCK_WARN, FSCK_ERROR };
  } possible_fsck_errors[];

 I considered that, and Michael Haggerty also suggested that in a private
 mail. However, I find that there is a clear hierarchy in the default
 messages: fatal errors, errors, warnings and infos.

I am glad I am not alone ;-)

These classes are ordered from more severe to less, but I do not
think it makes much sense to force the default view of if you
customize to demote a questionable Q that is classified as an error
by default as an warning, you must demote all the other ones that we
deem less serious than Q, which come earlier (or later---I do not
remember which) in our predefined list.  So in that sense, I do not
consider that various kinds of questionables fsck can detect are
hierarchical at all.

I do agree that it makes it easier to code the initialization of
such an array to have up to this point we assign the level 'fatal
error' by default constants.  Then the initialization can become

for (i = 0; i  FIRST_WARN; i++)
possible_fsck_errors[i].error_level = FSCK_INFO;
while (i  FIRST_ERROR)
possible_fsck_errors[i++].error_level = FSCK_WARN;
while (i  ARRAY_SIZE(possible_fsck_errors))
possible_fsck_errors[i++].error_level = FSCK_ERROR;

or something.  So I am not against the FIRST_WARNING constant at
all, but I find it very questionable in a fully customizable system
to use such a constant anywhere other than the initialization time.

--
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 04/18] Offer a function to demote fsck errors to warnings

2014-12-22 Thread Johannes Schindelin 
Hi Junio,

On Mon, 22 Dec 2014, Junio C Hamano wrote:

 Johannes Schindelin johannes.schinde...@gmx.de writes:
 
  In other words, at some point wouldn't we be better off with
  something like this
  
 struct {
 enum id;
  const char *id_string;
  enum error_level { FSCK_PASS, FSCK_WARN, FSCK_ERROR };
 } possible_fsck_errors[];
 
  I considered that, and Michael Haggerty also suggested that in a private
  mail. However, I find that there is a clear hierarchy in the default
  messages: fatal errors, errors, warnings and infos.
 
 I am glad I am not alone ;-)
 
 These classes are ordered from more severe to less, but I do not
 think it makes much sense to force the default view of if you
 customize to demote a questionable Q that is classified as an error
 by default as an warning, you must demote all the other ones that we
 deem less serious than Q, which come earlier (or later---I do not
 remember which) in our predefined list.  So in that sense, I do not
 consider that various kinds of questionables fsck can detect are
 hierarchical at all.

Oh, but please understand that this hierarchy only applies to the default
settings. All of these settings can be overridden individually – and the
first override will initialize a full array with the default settings.

So the order really only plays a role for the defaults, no more.

 I do agree that it makes it easier to code the initialization of
 such an array to have up to this point we assign the level 'fatal
 error' by default constants.  Then the initialization can become
 
   for (i = 0; i  FIRST_WARN; i++)
   possible_fsck_errors[i].error_level = FSCK_INFO;
   while (i  FIRST_ERROR)
   possible_fsck_errors[i++].error_level = FSCK_WARN;
   while (i  ARRAY_SIZE(possible_fsck_errors))
   possible_fsck_errors[i++].error_level = FSCK_ERROR;
 
 or something.  So I am not against the FIRST_WARNING constant at
 all, but I find it very questionable in a fully customizable system
 to use such a constant anywhere other than the initialization time.

This is indeed the case. The code we are discussing comes after the

if (options-strict_mode)
return options-strict_mode[msg_id];

In other words, once the overrides are in place, the default settings are
skipped entirely.

Ciao,
Dscho

Re: [PATCH 04/18] Offer a function to demote fsck errors to warnings

2014-12-22 Thread Junio C Hamano 
Johannes Schindelin johannes.schinde...@gmx.de writes:

 Hi Junio,

 On Mon, 22 Dec 2014, Junio C Hamano wrote:

 Johannes Schindelin johannes.schinde...@gmx.de writes:
 
  In other words, at some point wouldn't we be better off with
  something like this
  
struct {
enum id;
  const char *id_string;
  enum error_level { FSCK_PASS, FSCK_WARN, FSCK_ERROR };
} possible_fsck_errors[];
 
  I considered that, and Michael Haggerty also suggested that in a private
  mail. However, I find that there is a clear hierarchy in the default
  messages: fatal errors, errors, warnings and infos.
 
 I am glad I am not alone ;-)
 ...
 Oh, but please understand that this hierarchy only applies to the default
 settings. All of these settings can be overridden individually – and the
 first override will initialize a full array with the default settings.

But that means that the runtime needs to switch between two code
with and without override, no?

   if (options-strict_mode)
   return options-strict_mode[msg_id];

In other words, I think this is misleading and unnecessary
optimization for the full array allocation.  A code that uses an
array of a struct like the above that Michael and I independently
suggested would initialize once with or without an override and then
at the runtime there is no if the array is there use it
conditional.

I do not know why Michael suggested the same thing, but the reason
why I prefer that arrangement is because I think it would be easier
to read and maintain.

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 04/18] Offer a function to demote fsck errors to warnings

2014-12-22 Thread Johannes Schindelin 
Hi Junio,

On Mon, 22 Dec 2014, Junio C Hamano wrote:

 Johannes Schindelin johannes.schinde...@gmx.de writes:
 
  On Mon, 22 Dec 2014, Junio C Hamano wrote:
 
  Johannes Schindelin johannes.schinde...@gmx.de writes:
  
   In other words, at some point wouldn't we be better off with
   something like this
   
   struct {
   enum id;
   const char *id_string;
   enum error_level { FSCK_PASS, FSCK_WARN, FSCK_ERROR };
   } possible_fsck_errors[];
  
   I considered that, and Michael Haggerty also suggested that in a private
   mail. However, I find that there is a clear hierarchy in the default
   messages: fatal errors, errors, warnings and infos.
  
  I am glad I am not alone ;-)
  ...
  Oh, but please understand that this hierarchy only applies to the default
  settings. All of these settings can be overridden individually – and the
  first override will initialize a full array with the default settings.
 
 But that means that the runtime needs to switch between two code
 with and without override, no?
 
  if (options-strict_mode)
  return options-strict_mode[msg_id];
 
 In other words, I think this is misleading and unnecessary
 optimization for the full array allocation.  A code that uses an
 array of a struct like the above that Michael and I independently
 suggested would initialize once with or without an override and then
 at the runtime there is no if the array is there use it
 conditional.
 
 I do not know why Michael suggested the same thing, but the reason
 why I prefer that arrangement is because I think it would be easier
 to read and maintain.

Well, I disagree that it would be easier to maintain, because it appears
to me that the clear hierarchy keeps things simple. For example if some
clearly fatal error is clustered with non-fatal ones due to alphabetical
ordering, it is much harder to spot when it is marked as a demoteable
error by mistake.

For example, try to spot the error here:

...
F(ALMOST_HAPPY, INFO) \
F(CANNOT_RECOVER, ERROR) \
F(COFFEE_IS_EMPTY, WARN) \
F(JUST_BEING_CHATTY, INFO) \
F(LIFE_IS_GOOD, INFO) \
F(MISSING_SOMETHING_VITAL, FATAL_ERROR) \
F(NEED_TO_SLEEP, WARN) \
F(SOMETHING_WENT_WRONG, ERROR) \
...

Personally, I find it very, very hard to spot that CANNOT_RECOVER is
marked as a mere ERROR instead of a FATAL_ERROR. Even if it is nicely
alphabetically ordered.

I will sleep over this, though. Maybe I can come up with a solution that
makes all three of us happy.

Ciao,
Dscho

Re: [PATCH 04/18] Offer a function to demote fsck errors to warnings

2014-12-22 Thread Junio C Hamano 
Johannes Schindelin johannes.schinde...@gmx.de writes:

 For example, try to spot the error here:

   ...
   F(ALMOST_HAPPY, INFO) \
   F(CANNOT_RECOVER, ERROR) \
   F(COFFEE_IS_EMPTY, WARN) \
   F(JUST_BEING_CHATTY, INFO) \
   F(LIFE_IS_GOOD, INFO) \
   F(MISSING_SOMETHING_VITAL, FATAL_ERROR) \
   F(NEED_TO_SLEEP, WARN) \
   F(SOMETHING_WENT_WRONG, ERROR) \
   ...

But that is not what is being suggested at all.  I already said that
FIRST_SOMETHING is fine as a measure to initialize, didn't I?

I am only saying that if you have a place to store customized level,
you should initialize that part with default levels and always look
it up from that place at runtime.  It is perfectly fine for the
initialization step to take advantage of the ordering and
FIRST_SOMETHING constants.

--
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 04/18] Offer a function to demote fsck errors to warnings

2014-12-10 Thread Junio C Hamano 
Johannes Schindelin johannes.schinde...@gmx.de writes:

 There are legacy repositories out there whose older commits and tags
 have issues that prevent pushing them when 'receive.fsckObjects' is set.
 One real-life example is a commit object that has been hand-crafted to
 list two authors.

 Often, it is not possible to fix those issues without disrupting the
 work with said repositories, yet it is still desirable to perform checks
 by setting `receive.fsckObjects = true`. This commit is the first step
 to allow demoting specific fsck issues to mere warnings.

 The function added by this commit parses a list of settings in the form:

   missing-email=warn,bad-name=warn,...

 Unfortunately, the FSCK_WARN/FSCK_ERROR flag is only really heeded by
 git fsck so far, but other call paths (e.g. git index-pack --strict)
 error out *always* no matter what type was specified. Therefore, we
 need to take extra care to default to all FSCK_ERROR in those cases.

 Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
 ---
  fsck.c | 58 ++
  fsck.h |  7 +--
  2 files changed, 63 insertions(+), 2 deletions(-)

 diff --git a/fsck.c b/fsck.c
 index 05b146c..9e6d70f 100644
 --- a/fsck.c
 +++ b/fsck.c
 @@ -97,9 +97,63 @@ static int parse_msg_id(const char *text, int len)
  
  int fsck_msg_type(enum fsck_msg_id msg_id, struct fsck_options *options)
  {
 + if (options-strict_mode  msg_id = 0  msg_id  FSCK_MSG_MAX)
 + return options-strict_mode[msg_id];
 + if (options-strict)
 + return FSCK_ERROR;
   return msg_id  FIRST_WARNING ? FSCK_ERROR : FSCK_WARN;
  }

Hmm, if you are later going to allow demoting (hopefully also promoting)
error to warn, etc., would the comparison between msg_id and FIRST_WARNING
make much sense?

In other words, at some point wouldn't we be better off with
something like this

struct {
enum id;
const char *id_string;
enum error_level { FSCK_PASS, FSCK_WARN, FSCK_ERROR };
} possible_fsck_errors[];

--
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 04/18] Offer a function to demote fsck errors to warnings

2014-12-08 Thread Johannes Schindelin 
There are legacy repositories out there whose older commits and tags
have issues that prevent pushing them when 'receive.fsckObjects' is set.
One real-life example is a commit object that has been hand-crafted to
list two authors.

Often, it is not possible to fix those issues without disrupting the
work with said repositories, yet it is still desirable to perform checks
by setting `receive.fsckObjects = true`. This commit is the first step
to allow demoting specific fsck issues to mere warnings.

The function added by this commit parses a list of settings in the form:

missing-email=warn,bad-name=warn,...

Unfortunately, the FSCK_WARN/FSCK_ERROR flag is only really heeded by
git fsck so far, but other call paths (e.g. git index-pack --strict)
error out *always* no matter what type was specified. Therefore, we
need to take extra care to default to all FSCK_ERROR in those cases.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
 fsck.c | 58 ++
 fsck.h |  7 +--
 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/fsck.c b/fsck.c
index 05b146c..9e6d70f 100644
--- a/fsck.c
+++ b/fsck.c
@@ -97,9 +97,63 @@ static int parse_msg_id(const char *text, int len)
 
 int fsck_msg_type(enum fsck_msg_id msg_id, struct fsck_options *options)
 {
+   if (options-strict_mode  msg_id = 0  msg_id  FSCK_MSG_MAX)
+   return options-strict_mode[msg_id];
+   if (options-strict)
+   return FSCK_ERROR;
return msg_id  FIRST_WARNING ? FSCK_ERROR : FSCK_WARN;
 }
 
+static inline int substrcmp(const char *string, int len, const char *match)
+{
+   int match_len = strlen(match);
+   if (match_len != len)
+   return -1;
+   return memcmp(string, match, len);
+}
+
+void fsck_strict_mode(struct fsck_options *options, const char *mode)
+{
+   int type = FSCK_ERROR;
+
+   if (!options-strict_mode) {
+   int i;
+   int *strict_mode = malloc(sizeof(int) * FSCK_MSG_MAX);
+   for (i = 0; i  FSCK_MSG_MAX; i++)
+   strict_mode[i] = fsck_msg_type(i, options);
+   options-strict_mode = strict_mode;
+   }
+
+   while (*mode) {
+   int len = strcspn(mode,  ,|), equal, msg_id;
+
+   if (!len) {
+   mode++;
+   continue;
+   }
+
+   for (equal = 0; equal  len; equal++)
+   if (mode[equal] == '=')
+   break;
+
+   if (equal  len) {
+   const char *type_str = mode + equal + 1;
+   int type_len = len - equal - 1;
+   if (!substrcmp(type_str, type_len, error))
+   type = FSCK_ERROR;
+   else if (!substrcmp(type_str, type_len, warn))
+   type = FSCK_WARN;
+   else
+   die(Unknown fsck message type: '%.*s',
+   len - equal - 1, type_str);
+   }
+
+   msg_id = parse_msg_id(mode, equal);
+   options-strict_mode[msg_id] = type;
+   mode += len;
+   }
+}
+
 __attribute__((format (printf, 4, 5)))
 static int report(struct fsck_options *options, struct object *object,
enum fsck_msg_id id, const char *fmt, ...)
@@ -585,6 +639,10 @@ int fsck_object(struct object *obj, void *data, unsigned 
long size,
 
 int fsck_error_function(struct object *obj, int type, const char *message)
 {
+   if (type == FSCK_WARN) {
+   warning(object %s: %s, sha1_to_hex(obj-sha1), message);
+   return 0;
+   }
error(object %s: %s, sha1_to_hex(obj-sha1), message);
return 1;
 }
diff --git a/fsck.h b/fsck.h
index a18e9a6..9d67ea2 100644
--- a/fsck.h
+++ b/fsck.h
@@ -6,6 +6,8 @@
 
 struct fsck_options;
 
+void fsck_strict_mode(struct fsck_options *options, const char *mode);
+
 /*
  * callback function for fsck_walk
  * type is the expected type of the object or OBJ_ANY
@@ -25,10 +27,11 @@ struct fsck_options {
fsck_walk_func walk;
fsck_error error_func;
int strict:1;
+   int *strict_mode;
 };
 
-#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0 }
-#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1 }
+#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL }
+#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL }
 
 /* descend in all linked child objects
  * the return value is:
-- 
2.0.0.rc3.9669.g840d1f9

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