Re: [PATCH v4 8/8] t5520: check reflog action in fast-forward merge

2015-05-21 Thread Paul Tan 
Hi,

On Mon, May 18, 2015 at 11:20 PM, Johannes Schindelin
johannes.schinde...@gmx.de wrote:
 Hi Paul,

 On 2015-05-18 15:32, Paul Tan wrote:

 @@ -95,7 +94,11 @@ test_expect_success 'test . as a remote' '
   git checkout copy 
   test $(cat file) = file 
   git pull 
 - test $(cat file) = updated
 + test $(cat file) = updated 
 + git reflog -1 reflog.actual 
 + sed s/$_x05[0-9a-f]*/OBJID/g reflog.actual reflog.fuzzy 

 Actually, let's use s/^[0-9a-f]*/OBJID/ instead: you only want to replace 
 the first few characters.

Did you mean s/^$_x05[0-9a-f]*/OBJID/? (with $_x05 expanding to
'[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]' from test-lib.sh). If not,
then it would match even if there was no SHA1 hash.

But yes, without the ^ there will very likely be false positives.
Thanks for catching.

 @@ -106,7 +109,11 @@ test_expect_success 'the default remote . should
 not break explicit pull' '
   git reset --hard HEAD^ 
   test $(cat file) = file 
   git pull . second 
 - test $(cat file) = modified
 + test $(cat file) = modified 
 + git reflog -1 reflog.actual 
 + sed s/$_x05[0-9a-f]*/OBJID/g reflog.actual reflog.fuzzy 

 Same here.

Regards,
Paul
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 8/8] t5520: check reflog action in fast-forward merge

2015-05-21 Thread Junio C Hamano 
Paul Tan pyoka...@gmail.com writes:

 On Mon, May 18, 2015 at 11:20 PM, Johannes Schindelin
 ...
 + sed s/$_x05[0-9a-f]*/OBJID/g reflog.actual reflog.fuzzy 

 Actually, let's use s/^[0-9a-f]*/OBJID/ instead: you only want to
 replace the first few characters.

 Did you mean s/^$_x05[0-9a-f]*/OBJID/? (with $_x05 expanding to
 '[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]' from test-lib.sh). If not,
 then it would match even if there was no SHA1 hash.

 But yes, without the ^ there will very likely be false positives.
 Thanks for catching.

I think the suggestion was more about do we guarantee that there
would always be at least five?  It might happen to be the case with
our current default, but these tests do not fundamentally rely on
that default staying the same.  s/^[0-9a-f][0-9a-f]*/OBJECTNAME/g is
probably the right balance between cautiousness (you do not want to
match an empty string) and future-proofing (you do not want to rely
on us having at least five).

Thanks.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html