Re: [devel] [git update] packages/hasher: heads/master

Re: [devel] [git update] packages/hasher: heads/master

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 2026 bytes --]

On Fri, Aug 10, 2007 at 08:29:33PM +0400, Alexey M. Tourbin wrote:
> Update of /people/at/packages/hasher.git
[...]
>     On the "install ... && Verbose ... || Fatal ..." clauses.  I think this
>     is overkill.

Мне важно не только видеть причину сбоя, но и реакцию программы на сбой.
Возможно, диагностика от install и достаточна.  Решение принимается
субъективно в каждом конкретном случае.

>     On the "if shell_function ...; then ...' clauses.  This is evil.  Don't use
>     shell function in the "if" conditions, because "sh -e" mode is disabled for
>     such calls.  You can no longer expect that something "just fails".

То же самое касается и shell_function && on_success || on_failure.

>     On the "cat file |while read ...; grep || echo >bad; .. done' clauses.
>     I replaced them with "while ... done <file".

Если можно обойтись без подпроцессов, то так и надо делать.

> +	rm -rf cache/contents/list.new
> +	mkdir -p $verbose cache/contents/list.new

Здесь и далее, "rm -rf" с последующим "mkdir -p" на один и тот же каталог --
это неконсистентно; "-p" сбивает с толку читающего код.

> +	rm -rf cache/contents/tmp
> +	mkdir -p cache/contents/tmp
> +	local contents=cache/contents/tmp/contents
[...]
> +	contents_index_bin=chroot/.host/contents_index_bin
[...]
> +	cat <"$contents.bin" >"$contents_index_bin"
> +	rm -rf cache/contents/tmp

Здесь можно заменить cat на mv.

[...]
> +	rm -rf cache/contents/{index,list}
> +	install -p -m644 $verbose chroot/.host/contents_index_bin cache/contents/contents_index_bin
> +	install -p -m644 $verbose chroot/.host/contents_index_all.gz cache/contents/contents_index_all.gz

Тогда уж ln.

> +	contents_index_bin=chroot/.host/contents_index_bin
> +	contents_index_all=chroot/.host/contents_index_all.gz
> +	install -p -m644 $verbose cache/contents/contents_index_bin "$contents_index_bin"
> +	install -p -m644 $verbose cache/contents/contents_index_all.gz "$contents_index_all"

И здесь можно ln.


-- 
ldv

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [devel] [git update] packages/hasher: heads/master

[Alexey Tourbin]

[-- Attachment #1: Type: text/plain, Size: 2575 bytes --]

On Sat, Aug 11, 2007 at 02:17:38AM +0400, Dmitry V. Levin wrote:
> On Fri, Aug 10, 2007 at 08:29:33PM +0400, Alexey M. Tourbin wrote:
> > Update of /people/at/packages/hasher.git
> [...]
> >     On the "install ... && Verbose ... || Fatal ..." clauses.  I think this
> >     is overkill.
> 
> Мне важно не только видеть причину сбоя, но и реакцию программы на сбой.
> Возможно, диагностика от install и достаточна.  Решение принимается
> субъективно в каждом конкретном случае.

Я там дальше написал, что нельзя вообще всё проверить, и приходится
полагаться на некоторые вещи.  Например, когда ты пишешь

	if grep ^... file; then ...

ты не ожидаешь, что grep упадёт в SEGV, однако же #9941 (fixed).
Более строго, наверное, надо писать

if grep ^... file; rc=$?; [ $rc -eq 0 ]; then
	match
elif [ $rc -eq 1 ]; then
	no match
else
	internal error
fi

А также если строго проверять статус, то нельзя пользоваться пайпами.
В общем, нужно каким-то образом предвидеть причину сбоя, тогда стоит
проверять и писать более человеческую диагностику.  Иначе режим "sh -e"
даёт как бы оптимальный вариант -- не надо дописывать к каждой команде
"|| Fatal ...".

> >     On the "if shell_function ...; then ...' clauses.  This is evil.  Don't use
> >     shell function in the "if" conditions, because "sh -e" mode is disabled for
> >     such calls.  You can no longer expect that something "just fails".
> 
> То же самое касается и shell_function && on_success || on_failure.

Это очень неприятно. :(

> > +	rm -rf cache/contents/list.new
> > +	mkdir -p $verbose cache/contents/list.new
> 
> Здесь и далее, "rm -rf" с последующим "mkdir -p" на один и тот же каталог --
> это неконсистентно; "-p" сбивает с толку читающего код.

Да, надо было без -p делать.

> > +	rm -rf cache/contents/tmp
> > +	mkdir -p cache/contents/tmp
> > +	local contents=cache/contents/tmp/contents
> [...]
> > +	contents_index_bin=chroot/.host/contents_index_bin
> [...]
> > +	cat <"$contents.bin" >"$contents_index_bin"
> > +	rm -rf cache/contents/tmp
> 
> Здесь можно заменить cat на mv.

Да.

> [...]
> > +	rm -rf cache/contents/{index,list}
> > +	install -p -m644 $verbose chroot/.host/contents_index_bin cache/contents/contents_index_bin
> > +	install -p -m644 $verbose chroot/.host/contents_index_all.gz cache/contents/contents_index_all.gz
> 
> Тогда уж ln.

Я сходу не сообразил, какие там права получаются.  Как я понимаю,
изнутри чрута файл модифицировать нельзя, а вот снаружи чрута
модифицировать можно, и он изменится в чруте.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]