Re: [PATCH] Documentation: complete config list from other manpages

This is second review, with the same comments ad the one before, and containing proposal for simpler autogeneration script.

On Sat, 24 Oct 2010, Thomas Rast wrote:

Actualy it is "Add ... and run it to generate config-vars.txt", or "Create ... and use it to ...", isn't it.

I find this itemized list slightly hard to read (as you can see by the confusion shown in previous review of this patch). I think it would be better to describe _goal_ of this script first, and only then how it does that.

For example:

  The Documentation/make-config-list.perl script finds all config
  variables that are defined in individual manpages that are missing
  from the list of all config variable (in git-config(1) manpage).
  It then generates stub[1] documentation for those missing config
  variables at appropriate place in Documentation/config-vars.txt,
  using the following form:

    foo.bar::
    foo.baz::
    	See linkgit:git-foo[1].

  It does that by parsing config-vars-src.txt (now source for 
  config-vars.txt) getting list of all config variables to find later
  which ones are missing, parsing each manpage source provided on 
  command line (following includes) for config variable headers to
  find which config variables are described directly on individual
  manpages, then it assembles config-vars.txt with missing variables
  added[2].

  NOTE that as a side-effect list of config variables would be sorted
  in alphabetical order[3]

[1] Not a best word, but I couldn't think of a better one.
[2] This paragraph could be done as itemized list, as before.
[3] In my proposal it is not necessary; note that some of config 
    variables are grouped _by function_ to be close together, and
    your script would destroy that.

Stray changes (there are 2 of them).

Stray change, doesn't belong in this commit (and probably should be skipped altogether).

BTW. there was a proposal to do something like that by saving quote in a variable, and using it in shell quoting...

Very, very minor nitpick: wouldn't it read better as the following?

  +	./make-config-list.perl \
  +		--mainlist=config-vars-src.txt \
  +		--ignore=config-vars.txt \
  +		$(MAN1_TXT) $(MAN5_TXT) $(MAN7_TXT) \
  +		> config-vars.txt+ && \

Stray change (spaces to tab), which should be done as separate "whitespace cleanup" patch.

Errr, wouldn't it be better to do it this way:

  +my $sort = 1;
  +my $rc = GetOptions(
  +	"mainlist=s" => \my $mainlistfile,
  +	"ignore=s" => \my @ignore,
  +	"sort!" => \$sort,
  +);
  +my %ignore = map { $_ => 1 } @ignore;

Though it is not necessary; I guess it is a matter of taste.

BTW. does your sript work correctly with `--no-sort`?

I'd rather you did a better error handling:

  +if (!$rc || !defined $mainlistfile) {
  +	print "$0 --mainlist=<mainlist> [--ignore=<ignore>...] <asciidoc_manpage>...\n";
  +	exit 1;
  +}

and leave handling '!-r $mainlistfile' to read_varlist() subroutine.

This subroutine needs description, I think; at least oneline comment describing it.

You are a bit inconsistent here.  read_varlist() has

   	open my $fh, "<", $file or die "cannot open $file: $!";

while read_file() has

   	my $fp;
   	open $fp, '<', $name or die "open $name failed: $!";

Note that you can use the same name of filehandle (e.g. $fh), as it is local to the subroutine.

I would done this differently, and IMHO in a simpler way. Instead of generating %mainvars structure, that holds for each variable the contents of its description (in the form of list of lines), adding missing variables to it, and then (re)generating config-vars.txt from it:

  --mainlist=config-vars-src.txt
          |
          v
  @mainlist, %mainvars
          |
          |  <-- %vars <-- <asciidoc_manpage>...
          |
          v
  @all_keys, %out
          |
          v
  > config-vars.txt+

wouldn't it be better to simply store where description of each variable begins in a file (line number, or file position), and then use it to inject generated description for missing variables in correct place?

Note: because you are (re)generating config-vars.txt from those data
structures this has an unfortunate side-effect of lowercasing all config
variable names in final config-bars.txt, e.g. it would replace
`core.ignoreCygwinFSTricks` with `core.ignorecygwinfstricks`.

So in the proposed implementation it would simply be:

  +			$mainlist{lc($v)} = $.;
  +			push @mainvars, $v;

I think lc($v) is needed only in %mainlist keys.

Besides, if I understand it correctly, that any empty line which does not precede config variable definition, and is not inside literal block, is a *bug* in AsciiDoc markup. It should be "^+$" (continuation), not "^$".

This branch of conditional would be simply not required in proposed alternate solution.

By the way, why you do not follow includes in this file (for example 'include::merge-config.txt[]'?

This branch of conditional would be simply not required in proposed alternate solution.

Well... assuming that we do not have lines matching /^(\S+)::/ in literal blocks (which we don't, and which you also assume).

You are a bit inconsistent here.  read_varlist() has

   	close $fh or die "eh? close failed: $!";

while read_file() has

   	close $fp or die "close $name failed: $!";

I think those variables needs better names, for example %vars_manpages (as it is mapping from config variable name found in manpage to list of manpages it was found in) and %manpage_section (as it is mapping from manpage name to man section, for purpose of linking).

Though perhaps %vars could be left as is...

This subroutine needs description, I think; at least oneline comment describing it. And probably better name, like read_manpage().

Wouldn't it be simpler to pass $filename, $manpage as arguments to read_file(), rather than $filename/$name, $main_name?

It would read as:

  +# Parse manpages for config variables and sections, following includes
  +sub read_manpage {
  +	my ($filename, $manpage) = @_;
  +	if (!defined $manpage) {
  +		$manpage = $filename;
  +		$manpage =~ s/\.txt//;
  +	}

And of course s/$name/$filename/ and s/$manpage_name/$manpage/. Well, the change of variable names is not really necessary.

O.K. nicely done. This would be unchanged in proposed alternate solution.

I propose the following:

  +my @missing_vars = ();
  +foreach my $k (keys %vars) {
  +	if (!exists $mainlist->{$k}) {
  +		push @missing_vars, $k;
  +	}
  +}

or even

  +foreach my $k (keys %vars) {
  +	if (exists $mainlist->{$k}) {
  +		delete $vars{$k};
  +	}
  +}

so that %vars would now contain only missing variables.

So if you pass `--no-sort` to this script, all missing variables would be put at the end, and if you pass it, the order of variables would be changed, isn't it?

Instead of that, I would propose the following UNTESTED code:

-- 8< -- %vars = find_insertion_points($mainlist, \%vars); # %vars has now the following format: # 'name' => { # 'lineno' => line of $mainfile where description should be inserted # 'manpages' => [ <manpage_name>... ] # }

# group by insertion point
my %insertion_point; ## or just %insert
foreach my $v (keys %vars) {
	push @{$insertion_point{$v->{'lineno'}}}, $v;
}

open my $fh, '<', $mainfile
	or die "cannot open '$mainfile': $!";
while (my $line = <$fh>) {
	if (exists $insertion_point{$.}) {
		print document_vars($insertion_point{$.}, \%vars);
		print "\n";
	}
	print $line;
}
# special case: insertion after last line in $mainfile
print document_vars($insertion_point{-1}, \%vars)
	if exists $insertion_point{-1};
close $fh
	or die "cannot close '$mainfile': $!";

exit 0;

## those subroutines should probably be defined earlier

sub find_insertion_points {
	my ($mainlist, $missing_vars) = @_;

	my %res; ## not required if we modify %$missing_vars in-place

	my %all_vars = (%$mainlist, %$missing_vars);
	my $lineno = -1; # means after last line

	# reverse order because we want to find a place before which to insert
	# generated documentation; it is easy to find where description
	# of variable begins, but in general harder to find where it ends.
	foreach my $key (reverse sort { lc($a) cmp lc($b) } keys %all_vars) {
		my $val = $all_vars{$key};
		if (ref $val) {
			# this came from %$missing_vars
			$res{$v} = { 'lineno' => $lineno, 'manpages' => $val };
			## or $missing_vars->{$v} instead of $res{$v} if in-place
		} else {
			# this came from %$mainlist
			$lineno = $value;
		}
	}
	return %res; ## not required if in-place
}

## this is single place where we need to change code if output changes,
## e.g. to
##
##   format.attach         linkgit:git-format-patch[1]
##   format.cc             linkgit:git-format-patch[1]
##   format.headers        linkgit:git-format-patch[1]
##   format.pretty         linkgit:gitpretty[7]
sub document_vars {
	my ($keylist, $vars) = @_;

	# generate output for each key now, to compact output, because it is
	# easier to compare strings than arrays
	foreach my $k (@$keylist) {
		$vars->{$k}{'output'} = "\tSee: ".gen_links($vars->{$k}{'manpages'}).".\n";
	}

	my $output = '';
	## the below could be done in different way, e.g. using new array for
	## sorted keys, and using 'my $k = pop @keys' etc.
	$keylist = [ sort @$keylist ];
	for (my $i = 0; $i < @$keylist; $i++) {
		my $k = $keylist->[$i];
		$output .= "$k::\n";
		unless ($i <= $#@$keylist &&
		        $keylist->[$i]{'output'} eq $keylist->[$i+1]{'output'}) {
			$output .= $k->{'output'};
		}
	}

	return $output;
}

## we can make it smarter in the future, for example:
## 'linkgit:git-foo[1], linkgit:git-bar[1] and linkgit:git-baz[1] for details';
sub gen_links {
	my $manpages = shift;
	return join(", ", map { linkgit($_) } @$manpages);
}

sub linkgit {
	my $manpage = shift;

	if (!exists $manpage_section{$manpage}) {
		warn "section for $manpage unknown\n";
		$manpage_section{$manpage} = 1;
	}
	return "linkgit:${manpage}[$manpage_section{$manpage}]";
}

__END__ -- >8 --