James Carlson wrote: > Nathan Bush writes: >> Proposed fix: >> http://cr.opensolaris.org/~nbush/scm-migration/372/webrev/ > > 1525-1531,1596-1603: nit: indenting seems to get a little strange > around here.
The preferred indenting style is not clear to me in these cases. I tried to follow a rule of tab characters only, except for four spaces on continuation lines. Can you suggest the proper changes? > 1548-1551: I thought 'cp' preserved modes ... what is this for? It does, however, it looks like SCCS/Teamware's behavior with respect to permissions is that once the file is placed under SCCS control, the permissions of the SCCS/s.file always take precedence. For example, an edit/chmod/delget cycle on the working file will not actually change the permissions of the SCCS/s.file. So the purpose of this is to always copy the working file contents (and implicitly, permissions) into the webrev, but if it's also already under version control, apply the permissions of the SCCS/s.file. Then normalize the permissions in the webrev with "chmod u-w,go=u". In fact, it looks like there is no way to change the permissions of an existing file in a parent workspace via a putback. I think the only way to do this is via a chmod of the SCCS/s.file directly in the parent. So it may be somewhat misleading to print messages in a webrev that say it contains a permission "change". But, I think it's still useful to flag this condition even if it cannot be propagated to the parent. It indicates that the child workspace owner must have done an improper manual chmod of their SCCS/s.file, and they might have come to depend on the effects of this in other ways, such as by forgetting a makefile rule to do a chmod explicitly. > 1596-1597: could this be made easier to maintain with awk or some more > capable tools? This pipe is hard to read. (It looks to me like it > could even be a single sed expression.) Yes. I made it into a single sed command. It's not an exact translation of the old pipe, but I think it's equivalent or better. > 2719-2721,2738,2742-2743: would be easier to read as a single > expression. Yes. I also revised line 1612 for the same reason. > Otherwise, the code looks good. > >> Sample execution: >> http://cr.opensolaris.org/~nbush/scm-migration/372/sample/ > > I like it! > Thanks! New URLs: http://cr.opensolaris.org/~nbush/scm-migration/372/webrev.v2/ http://cr.opensolaris.org/~nbush/scm-migration/372/sample.v2/ I already diff'd the samples v1 vs. v2, I think the only changes were timestamps or Hg changeset hashes. The webrev.sh diff v1 vs. v2 is: 1596,1597c1596,1603 < head -4 | tail +2 | egrep -v '^(---)|(\+\+\+) ' | < sed -e 's/ .* 100/ /' -e 's/deleted/old/' | --- > sed -n ' > 2,3 { > s/^deleted file /old / > s/^new file /new / > s/ mode 100/ / > /^old /p > /^new /p > }' | 1612c1618 < if [[ -z "$old_mode" ]] && [[ -z "$new_mode" ]]; then --- > if [[ -z "$old_mode" && -z "$new_mode" ]]; then 2719,2721c2725,2727 < if [[ $SCM_MODE == "teamware" ]] || < [[ $SCM_MODE == "mercurial" ]] || < [[ $SCM_MODE == "unknown" ]]; then --- > if [[ $SCM_MODE == "teamware" || > $SCM_MODE == "mercurial" || > $SCM_MODE == "unknown" ]]; then 2738c2744 < if [[ -z "$old_mode" ]] && [[ "$new_mode" = *[1357]* ]]; then --- > if [[ -z "$old_mode" && "$new_mode" = *[1357]* ]]; then 2742,2743c2748,2749 < elif [[ -n "$old_mode" ]] && [[ -n "$new_mode" ]] && < [[ "$old_mode" != "$new_mode" ]]; then --- > elif [[ -n "$old_mode" && -n "$new_mode" && > "$old_mode" != "$new_mode" ]]; then --Nathan
