On 13/08 09:46, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx> writes:
>
> >> @@ -16,12 +16,13 @@ add_file () {
> >> owd=$(pwd)
> >> cd "$sm"
> >> for name; do
> >> - echo "$name" > "$name" &&
> >> + echo "$name" >"$name" &&
> >> git add "$name" &&
> >> test_tick &&
> >> git commit -m "Add $name"
> >> done >/dev/null
> >> - git rev-parse --verify HEAD | cut -c1-7
> >> + git rev-parse --verify HEAD >out &&
> >> + cut -c1-7 out
> >
> > In any case, I believe we can avoid the 'cut' altogether in both places
> > by doing something like this instead:
> >
> > git rev-parse --short=7 HEAD
>
> Ah, I missed the fact that this was a helper function and most of
> the error status is discarded anyway. For example, we still run the
> rev-parse even after the for loop fails.
>
> If the focus of this test script were to ensure that rev-parse works
> correctly, being careful to catch its exit status might have had a
> good value, but for that, all the other operations that happen in
> this helper function (including the "what happens when the loop body
> fails for $name that is not at the end of the argument list?") must
> also be checked for their exit status in the first place.
>
> Since that is not done, and since testing rev-parse should not have
> to be part of the job for submodule test, the net effect of the
> above change has quite dubious value---it clobbered a file 'out'
> that may be used by the caller.
>
> Doing "cd" without introducing a subshell is a bit harder to fix, as
> test_tick relies on the global counter in the topmost process. It
> can be done, but I do not think it is worth doing here. Most of the
> users of this helper function call it in var=$(add_file ...)
> subshell anyway (so test_tick is incrementing the timestamp
> independently for each caller and discarding the resulting
> timestamp). As a NEEDSWORK comment added in the series says, this
> script may need a bit more work.
>
> I agree with you that the split of "rev-parse | cut -c1-7" into two
> statements and clobbering 'out' is a bad change---that part should
> be reverted. The style change on 'echo "$name" >"$name"' line is
> OK, though.
>
> Thanks.
Understood. I will revert the change. Though, what Kaartic suggested, to
do a '--short=7', that will be okay to keep right? Something like:
git rev-parse --short=7 HEAD
This way we will not need a 'cut'. This change goes as a separate commit
obviously.
