Re: [PATCH v2] config: add --comment option to add a comment

"Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes:

For readability, you probably would want to have a SP before the given string, i.e.,

	variable = "value" # message comes here

These two lines should not be needed. It is customary to just state that the users need to be able to do X and adding feature Y is one way to allow that, without such preamble.

It is unclear what happens when a user gives comment that already has the comment introducer, e.g., --comment="# this is comment".

"not supported" can take many shapes, ranging from "producing a random result and may corrupt the resulting configuration file" and "the second and subsequent lines are silently ignored", to "results in an error, stops the command before doing anything".

"justify"

The above two lines say the same thing and are unnecessary, especially before a sentence that begins with "They are aimed at".

No need to repeat a wall of text below the three-dash line if they do not give additional information on top of what is already in the proposed commit log message.

There is Patrick's proposal to revamp the UI of this command,

  https://lore.kernel.org/git/cover.1709724089.git.ps@pks.im/

which may make the above simpler (e.g., there won't be two lines that talks about "set" commands, with and without "--add" option, for example). This topic may want to at least wait for the conclusion of that design discussion, and possibly its implementation.

Other options that take mandatory parameter are are described with their parameter on the item heading line. It may help to somehow mention that the "appending" is done at the end on the same line. Perhaps something along this line.

	--comment <message>::
		Append the _<message>_ at the end of the new or
		modified lines with the value.  A comment introducer
		` # ` is prepended to _<message>_ if it does not
		already have one.  The command errors out if given
		a _<message>_ that spans multiple lines.

This overly long line is both visually annoying and harder to maintain. If you can design a solution that makes it easier to read a future patch that adds support of the comment option to more actions (or removes it from an action), that would be great. Otherwise, do at least:

	if (comment &&
	    !(actions & (...)) {

If I were doing this, I'd probably add this new block after the fixed-value thing, as it is bad manners to add new code _before_ existing ones, as if your new invention were somehow more important than all the others, when the order does not matter. If there is a valid technical reason (e.g., the new code modifies the state of the program that affects the beahviour of the later and existing parts of the code), the above comment of course does not apply.

It is somewhat annoying that we have to see so much code churn to add this new parameter X-<. I notice that we are sending "comment" to the underlying machinery without doing ANY sanity checking (like, "ah, we got a message without the '#' prefix, so we should add '# ' in front of it before asking the API to add that comment string to the value line"). We may also want to have a code that says:

    Yikes, this message has LF in it, but the underlying machinery
    does not check for it and end up corrupting the configuration
    file by creating

	[section]
		var = value #comment
	that spans on two lines

    when given

	LF='
	'
        git config --comment="comment${LF}that spans..." section.var value

or something. The underlying machinery should be updated to die() when given such a message instead of silently corrupting the resulting file, but the front-end that receives the end-user input should check for obviously problematic payload before bothering the API with it.

The original was probably OK without naming these parameters of the same type, as it is (arguably) obvious to have the filename first, because that is what differenciates the "in-file" variant. And then key followed by value, because that is the usual "assignment" order people would naturally expect.

But it was already on the borderline. The idea is that we do not have to (but still can) name the parameters in our function declarations when it is obvious from their types what they are. Addition of this comment thing will push us way over the line.

The same comment applies to many of the function declarations touched by this patch.

If I were doing this patch, I'd have at least one clean-up patch that updates this line to read:

    int git_config_set_in_file_gently(const char *filename, const char *key, const char *value);

And then write a separate patch to add the "--comment" feature.

I however have some suspicion that we might move away from "listing many random parameters" style to "having only the essential parameters, together with a single pointer to a structure that holds the optional bits" style, especially when the UI revamp Patrick proposed comes. In the case of config API, <key, value> is the essential bit in the write direction, and value-pattern restriction and the bit that says key is an regexp would be in "the optional bits" group. This <comment> thing will also be in the latter class.

Add test for at least two more cases and show what should happen.

	--comment="# the user already has the pound"
	--comment="this is being ${LF} naughty to break configuration"

Thanks.