Re: [PATCH v3 1/9] rebase -i: generate the script via rebase--helper

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 28, 2017, 02:36 UTC
Message-ID: <xmqqy3ull0m9.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqbmrhmlsw.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 11 quoted lines

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