Re: [PATCH 1/8] am: suppress error output from a conditional
Junio C Hamano wrote: If you are introducing dotest exists but next/last may not be present as a valid new state, it probably should check the presence and/or absence of them explicitly, Um, a test -f preceding the cat? Why, when cat implicitly checks existence anyway? but more importantly, it is a good idea to move test $# != 0 higher. This? diff --git a/git-am.sh b/git-am.sh index 47c1021..1e10e31 100755 --- a/git-am.sh +++ b/git-am.sh @@ -446,9 +446,9 @@ done # If the dotest directory exists, but we have finished applying all the # patches in them, clear it out. if test -d $dotest + test $# != 0 last=$(cat $dotest/last 2/dev/null) next=$(cat $dotest/next 2/dev/null) - test $# != 0 test $next -gt $last then rm -fr $dotest Earlier it did not matter because when $dotest existed, next/last were supposed to be there and absence of them was an error codepath. Now, missing these files is not an error but is a perfectly normal state, It's not a normal state for the purposes of git-am.sh. There is no codepath in the program that depends only on $dotest, but not next/last. I would frame this as: checking for $dotest is not a sufficient prerequisite anymore; we have to additionally look for next/last. To git-am.sh, an empty $dotest or just a $dotest/autostash is equivalent to no $dotest at all. so checking what can be checked more cheaply makes sense as a general code hygiene. Yeah, I agree. See am: tighten a conditional that checks for $dotest, where I get away with just checking $dotest/last (and assume that $dotest/next is present by extension). How do I apply your suggestion to this particular patch? -- 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 1/8] am: suppress error output from a conditional
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: If you are introducing dotest exists but next/last may not be present as a valid new state, it probably should check the presence and/or absence of them explicitly, Um, a test -f preceding the cat? Why, when cat implicitly checks existence anyway? With that logic, 'test -d $dotest' on the first line would also be redundant, as we read from $dotest/last and that will catch an error. It is more about what is conceptually right. You need to think what the condition _means_. We protect the remove $dotest directory with the precondition you are changing, but what is that precondition really trying to say? If $dotest does not exist, obviously we do not need to remove it, but what is the essence of that sereis of tests? It is what the comment says: We have finished applying all the patches in them Earlier, presense of $dotest _must_ have meant that last and next should exist (otherwise you have a corrupt state and we did want to see error messages), and the check original did was perfectly fine. Now, a mere presense of $dotest does _not_ mean we have finished applying all the patches, because sometimes you create it without having anything to put in last or next yet. That is why it starts to make sense to do if test -d $dotest test -f $dotest/last test -f $dotest/next last=$(cat $dotest/last) ... -- 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
[PATCH 1/8] am: suppress error output from a conditional
In preparation for a later patch that creates $dotest/autostash in git-rebase.sh before anything else happens, don't assume that the presence of a $dotest directory implies the existence of the $next and $last files. The check for the files is in a conditional anyway, but `cat` is executed on potentially non-existent files. Suppress this error output. Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- git-am.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/git-am.sh b/git-am.sh index c092855..88aa438 100755 --- a/git-am.sh +++ b/git-am.sh @@ -446,8 +446,8 @@ done # If the dotest directory exists, but we have finished applying all the # patches in them, clear it out. if test -d $dotest - last=$(cat $dotest/last) - next=$(cat $dotest/next) + last=$(cat $dotest/last 2/dev/null) + next=$(cat $dotest/next 2/dev/null) test $# != 0 test $next -gt $last then -- 1.8.3.rc1.52.gc14258d -- 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 1/8] am: suppress error output from a conditional
Ramkumar Ramachandra artag...@gmail.com writes: In preparation for a later patch that creates $dotest/autostash in git-rebase.sh before anything else happens, don't assume that the presence of a $dotest directory implies the existence of the $next and $last files. The check for the files is in a conditional anyway, but `cat` is executed on potentially non-existent files. Suppress this error output. Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- git-am.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/git-am.sh b/git-am.sh index c092855..88aa438 100755 --- a/git-am.sh +++ b/git-am.sh @@ -446,8 +446,8 @@ done # If the dotest directory exists, but we have finished applying all the # patches in them, clear it out. if test -d $dotest - last=$(cat $dotest/last) - next=$(cat $dotest/next) + last=$(cat $dotest/last 2/dev/null) + next=$(cat $dotest/next 2/dev/null) test $# != 0 test $next -gt $last then If you are introducing dotest exists but next/last may not be present as a valid new state, it probably should check the presence and/or absence of them explicitly, but more importantly, it is a good idea to move test $# != 0 higher. Earlier it did not matter because when $dotest existed, next/last were supposed to be there and absence of them was an error codepath. Now, missing these files is not an error but is a perfectly normal state, so checking what can be checked more cheaply makes sense as a general code hygiene. As you may already know, I am not taking a patch that is not meant for 'master' to fix regressions in 1.8.3 at this point in the cycle after -rc2; please hold onto this and other patches as they won't stay in my mailbox. -- 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
