Felipe Contreras <felipe.contreras@xxxxxxxxx> writes:
> On Sat, Feb 14, 2009 at 11:28 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Felipe Contreras <felipe.contreras@xxxxxxxxx> writes:
>>
>>> Reorganizing the code to use parseopt as suggested by Johannes
>>> Schindelin.
>>>
>>> Signed-off-by: Felipe Contreras <felipe.contreras@xxxxxxxxx>
>>> ---
>>> builtin-config.c | 422 +++++++++++++++++++++++++++---------------------------
>>> 1 files changed, 210 insertions(+), 212 deletions(-)
>>
>> Whew. That's a lot of changes.
>>
>> Is this just "I am using parseopt because I can", or "I want to do this
>> first because I am planning to do such and such things to this program,
>> and the current mess needs to be cleaned up first before doing so"?
>>
>> I am asking this _not_ because I'd want to reject the patch if the answer
>> is former.
>
> Then why are you asking?
So that I can prioritize topics and patches that are more important in the
short, medium and longer time according to the urgency. For example, it
is a very bad tradeoff for me to take "This does not change anything
externally visible, but cleans up the implementation" near or during the
release freeze. "This cleans up, and then the next one will fix a
longstanding bug building on top of the cleaner code" is different.
And preferably the latter kind should be queued for 'maint', not for
'master', if the fix that will come later is important enough.
> This is more a "I would like to increase the chances of my patches
> being accepted so I'd do some chores to gain the trust of some
> developers", and Johannes Schindelin was pushing me to do this.
>
> Also it's a bit of "I would like to improve git and learn the API
> while doing so".
I personally do not think "I rewrote this command's option parser using
parseopt" earns any "trust point". I think the latter is a *great* thing
to do, though.
> The only problem is that --bool and --int would be possible in the
> same command and there would be no way to output an error, but I guess
> that's not a big problem.
The existing code does not have a check, and you are right, it would be
nice to have such an error check, perhaps as a separate follow-up patch.
>>> +static const char *get_color_name, *get_colorbool_name;
>>> +static int end_null;
>>> +
>>> +static struct option builtin_config_options[] = {
>>> + OPT_GROUP("Config file location"),
>>> + OPT_BOOLEAN(0, "global", &use_global_config, "use global config file"),
>>> + OPT_BOOLEAN(0, "system", &use_system_config, "use system config file"),
>>> + OPT_STRING('f', "file", &given_config_file, "FILE", "use given config file"),
>>
>> Why CAPS?
>
> I looked at some other code, like builtin-commit.c and that's what is
> used there.
I see, there are mixtures; apply, blame, fmt-merge-msg and tag say "file"
and commit and fast-export say "FILE". commit's one could be almost
justifyable, because it uses caps in all (except for -u=<mode>), but
output from fast-export is a different story. Compare "git commit -h" and
"git fast-export -h" to see what I mean.
One possible patch could make "git cmd -h" messages for all "cmd" spell
their "fill your value here" markings consistently, and the explanation of
the change in the proposed commit log message would have
(1) an analysis similar to the one I did in the above paragraph (but
doing it more thouroughly; I didn't look beyond commands that grep
for "file" or "FILE" hits in the sources) of the current situation;
and
(2) the reason why you picked the case (be it lower or upper, I do not
deeply care either way).
Such a patch could earn a "trust point", not because such it is extremely
important for the system's usability (it is *not*), but because it is a
good way to demonstrate that you can do a thorough job and think things
through.
>> Hmph. I wonder if use of OPT_BIT() and HAS_MULTI_BITS() make this simpler.
>
> Yeap, definitely, thanks for the suggestion.
>
> Again, it would have been nice to see any example of this.
I actually had to look for HAS_MULTI_BITS because I *knew* such a facility
existed, I did not recall what it was called, and api-parse-options.txt in
Documentation/technical did not talk about it. We can definitely improve
things in many places.
Mistakes made in the past and resulting flaws that remain in the current
source do not justify a new patch adding more mistakes to the codebase.
Reviewers help the author from adding more.
One bad thing about the current process (and I am certainly one of the
guilty parties because I do a lot of reviews) around this area is that a
review comment that points out a mistake similar to the ones made in the
past sound to the author of the patch as if the reviewer is telling that
the patch will not be accepted unless the existing mistakes are also fixed
by the patch author. Such a review certainly does not mean that --- it is
more likely that the patch author simply mimicked existing code that is
doing a wrong thing without realizing it being wrong, and because the
reviewer *did not know* that the mistake was copied from elsewhere, he
just pointed it out the one in the patch.
In such a situation, I've seen two kinds of responses from the patch
author (this assumes that the nit pointed out by the reviewer was indeed a
good thing to fix):
* This and that code already do things in a similar way. I won't be
changing my patch.
* Ok, I'll fix the issue in my patch, but this and that code have the
same mistake. Should I fix them in the same patch as well?
Neither is an exactly optimal solution. It is very likely that the patch
author knows more of the existing instances of the same mistake than the
reviewer, but there probably are more instances than he tried to defend
his patch with. The places he knows of were found merely because he was
looking for an example to do something similar to what he wanted to do,
not because he was looking for all instances of a specific class of
mistake to fix in the existing code.
An ideal response would be a follow up patch that fixes the issue that the
original patch had, and a separate message that summarizes such issues in
the existing codebase (saying that the instances listed are not
exhaustive). The patch author can attack them in a separate series if he
wants to, and that would certainly be appreciated, but he does not have
to. It is a separate issue that others (not necessarily the reviewer who
first pointed out the mistake in the patch) can attack.
Unfortunately, not many patch authors write such a summary. Sometimes we
see summaries on things that were discussed but nobody has followed
through posted by third parties (including myself), but we do not seem to
have enough helpers to do that either. This does not take much technical
skills but is a good "trust point" earner.
> Yes the arguments check need to be revised.
>
> My hope was somebody would review this and suggest a clever and
> generic way of doing this. Perhaps a util function check_min_args, or
> maybe something in parseopt that receives the number of args?
I thought what the original code before your patch had was quite
reasonable, switching on argc.
> Anyway, I can create the helper functions again, and maybe have some
> function parameters instead of passing argc and argv
It might actually be a good idea to turn the cascade of if..elseif..fi
into a form of look-up in a table whose elements have a pointer to a
function. A loooong function like this cmd_config() is very hard to
follow its logic.
>> Overall, with the same number of lines, we lost a lot of error checking in
>> exchange for an ability to say "git config --remove-sec" instead of "git
>> config --remove-section", and "git config --help" may give an easier to
>> read message.
>>
>> Given that "git config" is primarily meant for script use, this particular
>> round does not look like a good tradeoff to me.
>
> That doesn't mean it shouldn't have a nice UI.
The need for "UI" is much less than that of Porcelain for this particular
command, so it certainly affects the tradeoff points.
> Also, I think the code would be easier to maintain with parseopt.
Not with the patch you posted, but yes, the current mess could be
improved, and parseopt could be one of the tools for such code
restructuring.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
