Junio C Hamano <gitster@xxxxxxxxx> writes:
> Yes. The "pretty_given" trick is one example that the underlying
> implementation can change over time. If you wrote this patch before
> 66b2ed09 ("Fix "log" family not to be too agressive about showing
> notes", 2010-01-20) happened, you wouldn't have known to flip this
> bit on to emulate the command line parsing of "--pretty" and
> friends, and you would have required the author of that change to
> know that you have this cut & pasted duplicated code here when the
> commit is primarily about updating revision.c
>
> So I am very serious when I say that this is adding an unnecessary
> maintenance burden.
I _am_ sympathetic to your wish to have the compiler catch a
misspelt "revs.verboes_header = 1". A misspelt "--formta=..."
would not be caught until the execution time.
But the compiler's static name checking helps only one time while
you are writing _this_ patch, and it does not help at all to protect
this duplicated code from future breakages. The way "rev-list" and
friends _internally_ implement "--format=..." or any other options
sed by the rev-list command whose behaviour you are recreating here
can (and will) change in the future, just like it already did change
in early 2010. We didn't have pretty_given field for several years
after "--pretty" etc. that currently set the field were originally
introduced.
In an ideal world, we would probably have specific methods to
manipulate "struct rev_info" and set_format_string() method, which
would be called when the command line parser is reacting to
"--format=..." in setup_revisions(), may encapsulate the
implementation detail of setting verbose_header and pretty_given
fields in addition to calling get_commit_format() method on the
rev_info object, and your new code may be calling that method,
without having to know the implementation detail.
We do not live in that ideal world, and it is _not_ the theme of
your topic to bring us closer to the ideal world. Under that
constraint, a future-proof way to set up the revision machinery is
to have setup_revisions() parse an av[] array. What will be done to
your copy of revs will stay compatible with what rev-list would do
after the implementation detail of setup_revisions() changes that
way. It is true that a misspelt "--formta=..." would not be caught
until the execution time, but once the code in this part is written,
it is less likely to get broken by a change coming from needs by
other parts of the system (e.g. the addition of pretty_given came
not because we wanted to enhance how --format or --pretty worked; it
came because we wanted to make sure they are not affected by changes
to another option).
So after being forced by your response to rethink about it, I feel
even firmer about this than I felt when I sent my first review
comment.
Thanks.