Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
> It was done because the very syntax of the config suggests it be a
> user-editable file. I do not want to mess with the comments more than
> necessary.
I personally feel that is a lost cause _unless_ you come up with
a reasonable convention for where to put comments, stress that
rule to the user in the documentation, _and_ make repo-config to
follow that rule as well.
We _do_ want to treat config file as hand editable and cat
reviewable file, not an unreadable gunk like xml, so trying to
preserve user comments is important and I am not opposed to that
you did (at least some of) it. But as the code currently
stands, what it does is at best half baked, at worst somewhat
confusing.
A demonstration. What is wrong with this picture?
$ cat .git/config
[core]
repositoryformatversion = 0
; are the mode bits trustworthy?
filemode = true ; yes, on ext3
; We want symlinked HEAD because we will bisect
; recent kernel history.
prefersymlinkrefs = true
$ git repo-config core.prefersymlinkrefs false
$ git repo-config core.filemode false
$ cat .git/config
[core]
repositoryformatversion = 0
; are the mode bits trustworthy?
filemode = false
; We want symlinked HEAD because we will bisect
; recent kernel history.
prefersymlinkrefs = false
$ exit
The comment given to "filemode" is "reasonable" in that it
describes what the value that is set to the variable does, and
losing the original comment given to its "true" when we set it
to false is better than keeping it, so that part happens to be
doing the right thing -- only because I knew what repo-config
would do to the comments and arranged original comments in the
file that way.
But what about prefersymlinkrefs one? When setting the variable
to such a non-standard value, it is unreasonable for people to
want to justify why with a comment like the above. But after
resetting the value the comment becomes stale.
It gets worse:
$ git repo-config --unset core.filemode
$ cat .git/config
[core]
repositoryformatversion = 0
; please please use symlinks please
prefersymlinkrefs = false
; are the mode bits trustworthy?
$ exit
There now is a confusing trailing comment left that does not
comment anything. Removing core.filemode is not so common, but
this can happen whenever you remove any variable, so we can use
any other variable as an example.
Now, enough being negative and pointing out problems. Time to
become constructive. Probably a reasonable convention would be
to define the config file format to be something like this:
<comment that applies to the section>
[section]
<comment that applies to the variable stands on
its own before the variable>
variable [= value] <comment that applies to the
fact the variable is set to
this particular value starts
on the same line as the
"variable = value" thing>
- when a variable is reset to another value, remove the
"value comment";
- when a variable disappears, remove "variable comment";
- when a section disappears, remove "section comment";
- otherwise leave the comment intact.
Then we could tell the user the rule is like above, and tell
them to structure the file with comments that way, if they ever
want to edit the file by hand.
Now if we wanted to do something like the above, I suspect that
it would be easier and less error prone to first scan the config
file, note what appears where, and do the processing in-core,
and then write the results out, perhaps using data structures
like these:
struct config_section {
char *pre_comment;
char *name; /* e.g. "core" */
struct config_section *next; /* next section */
struct config_var *vars; /* pointer to the first one */
};
struct config_var {
char *pre_comment;
char *name;
char *value; /* "existence" bool may have NULL,
* otherwise probably a string "= value"
*/
char *value_comment;
struct config_var *next; /* pointer to the next one
* in the section
*/
};
Obviously, data structures like these would make it even easier
if we decide we do _not_ care about comments (we would just lose
x_comment fields, parse the thing and write the resulting list
out).
-
: 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