Re: [PATCH v8 1/4] usage: make error functions a stack

[Keller, Jacob E]

There was no way to get the current error routine now, and I figured
that a stack was a simple way of saving the old routine.

Essentially these two paths would be the same as a save/restore except
we manage it via a stack. I don't really see how that would end up any
different. I mean I don't mind either approach.

Thanks,
Jake

On Tue, 2014-07-15 at 17:49 -0700, Junio C Hamano wrote:
 I actually am not a big fan of stack for a thing like this, to be honest.
 Wouldn't it be sufficient for the callers who want specific behaviour from
 its callees to
 
  - save away the current error/warning routines;
  - set error/warning routines to its own custom versions;
  - call the callees;
  - set error/warning routines back to their original; and
  - return from it
 
 at least in the code paths under discussion?
 
 
 On Tue, Jul 15, 2014 at 4:26 PM, Keller, Jacob E
 jacob.e.kel...@intel.com wrote:
  On Tue, 2014-07-15 at 15:47 -0700, Junio C Hamano wrote:
  Jacob Keller jacob.e.kel...@intel.com writes:
 
extern void set_error_routine(void (*routine)(const char *err, va_list 
   params));
   +extern void pop_error_routine(void);
 
  pop that undoes set smells somewhat weird.  Perhaps we should rename
  set to push?  That would allow us catch possible topics that add new
  calls to set_error_routine() as well by forcing the system not to
  link when they are merged without necessary fixes.
 
 
  Also curious what your thoughts on making every set_*_routine to be a
  stack? For now I was only planning on error but it maybe makes sense to
  change them all?
 
  Thanks,
  Jake
 

Re: [PATCH v8 1/4] usage: make error functions a stack

[Junio C Hamano]

Jacob Keller jacob.e.kel...@intel.com writes:

  extern void set_error_routine(void (*routine)(const char *err, va_list 
 params));
 +extern void pop_error_routine(void);

pop that undoes set smells somewhat weird.  Perhaps we should rename
set to push?  That would allow us catch possible topics that add new
calls to set_error_routine() as well by forcing the system not to
link when they are merged without necessary fixes.

 +/* push error routine onto the error function stack */
  void set_error_routine(void (*routine)(const char *err, va_list params))
  {
 - error_routine = routine;
 + struct error_func_list *efl = xmalloc(sizeof(*efl));
 + efl-func = routine;
 + efl-next = error_funcs;
 + error_funcs = efl;
 +}
 +
 +/* pop a single error routine off of the error function stack, thus reverting
 + * to previous error. Should always be paired with a set_error_routine */
 +void pop_error_routine(void)
 +{
 + assert(error_funcs != default_error_func);
 +
 + struct error_func_list *efl = error_funcs;

decl-after-stmt.  Can be fixed easily by flipping the above two
lines.

 + if (efl-next) {
 + error_funcs = efl-next;
 + free(efl);
 + }
  }
  
  void set_die_is_recursing_routine(int (*routine)(void))
 @@ -144,7 +167,7 @@ int error(const char *err, ...)
   va_list params;
  
   va_start(params, err);
 - error_routine(err, params);
 + error_funcs-func(err, params);
   va_end(params);
   return -1;
  }
--
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 1/4] usage: make error functions a stack

[Keller, Jacob E]

On Tue, 2014-07-15 at 15:47 -0700, Junio C Hamano wrote:
 Jacob Keller jacob.e.kel...@intel.com writes:
 
   extern void set_error_routine(void (*routine)(const char *err, va_list 
  params));
  +extern void pop_error_routine(void);
 
 pop that undoes set smells somewhat weird.  Perhaps we should rename
 set to push?  That would allow us catch possible topics that add new
 calls to set_error_routine() as well by forcing the system not to
 link when they are merged without necessary fixes.
 

I thought about changing set too, but wasn't sure that made sense..?
That does make more sense now though. There *are* valid use cases where
a set_error_routine is used without a pop, (the one current use, I
think).

I'll update this patch with that change.

  +/* push error routine onto the error function stack */
   void set_error_routine(void (*routine)(const char *err, va_list params))
   {
  -   error_routine = routine;
  +   struct error_func_list *efl = xmalloc(sizeof(*efl));
  +   efl-func = routine;
  +   efl-next = error_funcs;
  +   error_funcs = efl;
  +}
  +
  +/* pop a single error routine off of the error function stack, thus 
  reverting
  + * to previous error. Should always be paired with a set_error_routine */
  +void pop_error_routine(void)
  +{
  +   assert(error_funcs != default_error_func);
  +
  +   struct error_func_list *efl = error_funcs;
 
 decl-after-stmt.  Can be fixed easily by flipping the above two
 lines.

Oh, right yes. I'll fix that in a resend as well.

Thanks,
Jake

Re: [PATCH v8 1/4] usage: make error functions a stack

[Keller, Jacob E]

On Tue, 2014-07-15 at 15:47 -0700, Junio C Hamano wrote:
 Jacob Keller jacob.e.kel...@intel.com writes:
 
   extern void set_error_routine(void (*routine)(const char *err, va_list 
  params));
  +extern void pop_error_routine(void);
 
 pop that undoes set smells somewhat weird.  Perhaps we should rename
 set to push?  That would allow us catch possible topics that add new
 calls to set_error_routine() as well by forcing the system not to
 link when they are merged without necessary fixes.
 

Also curious what your thoughts on making every set_*_routine to be a
stack? For now I was only planning on error but it maybe makes sense to
change them all?

Thanks,
Jake

Re: [PATCH v8 1/4] usage: make error functions a stack

[Junio C Hamano]

I actually am not a big fan of stack for a thing like this, to be honest.
Wouldn't it be sufficient for the callers who want specific behaviour from
its callees to

 - save away the current error/warning routines;
 - set error/warning routines to its own custom versions;
 - call the callees;
 - set error/warning routines back to their original; and
 - return from it

at least in the code paths under discussion?


On Tue, Jul 15, 2014 at 4:26 PM, Keller, Jacob E
jacob.e.kel...@intel.com wrote:
 On Tue, 2014-07-15 at 15:47 -0700, Junio C Hamano wrote:
 Jacob Keller jacob.e.kel...@intel.com writes:

   extern void set_error_routine(void (*routine)(const char *err, va_list 
  params));
  +extern void pop_error_routine(void);

 pop that undoes set smells somewhat weird.  Perhaps we should rename
 set to push?  That would allow us catch possible topics that add new
 calls to set_error_routine() as well by forcing the system not to
 link when they are merged without necessary fixes.


 Also curious what your thoughts on making every set_*_routine to be a
 stack? For now I was only planning on error but it maybe makes sense to
 change them all?

 Thanks,
 Jake

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