[PATCH 0/2] teach git pull to handle --log=<n>

[PATCH 0/2] teach git pull to handle --log=<n>

[Paul Tan]

Since efb779f (merge, pull: add '--(no-)log' command line option,
2008-04-06) git-pull supported the (--no-)log switch and would pass it
to git-merge.

96e9420 (merge: Make '--log' an integer option for number of shortlog
entries, 2010-09-08) implemented support for the --log=<n> switch, which
would explicitly set the number of shortlog entries. However, git-pull
does not recognize this option, and will instead pass it to git-fetch,
leading to "unknown option" errors.

This patch series implements a failing test that demonstrates the above,
and teaches git-pull to handle the switch --log=<n>.

Paul Tan (2):
  t5524: test --log=1 limits shortlog length
  pull: handle --log=<n>

 git-pull.sh         |  4 ++--
 t/t5524-pull-msg.sh | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

-- 
2.1.4

[PATCH 1/2] t5524: test --log=1 limits shortlog length

[Paul Tan]

Since efb779f (merge, pull: add '--(no-)log' command line option,
2008-04-06) git-pull supported the (--no-)log switch and would pass it
to git-merge.

96e9420 (merge: Make '--log' an integer option for number of shortlog
entries, 2010-09-08) implemented support for the --log=<n> switch, which
would explicitly set the number of shortlog entries. However, git-pull
does not recognize this option, and will instead pass it to git-fetch,
leading to "unknown option" errors.

Implement a failing test that demonstrates this.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 t/t5524-pull-msg.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
index 8cccecc..593405d 100755
--- a/t/t5524-pull-msg.sh
+++ b/t/t5524-pull-msg.sh
@@ -17,6 +17,9 @@ test_expect_success setup '
 		git commit -m "add bfile"
 	) &&
 	test_tick && test_tick &&
+	echo "second" >afile &&
+	git add afile &&
+	git commit -m "second commit" &&
 	echo "original $dollar" >afile &&
 	git add afile &&
 	git commit -m "do not clobber $dollar signs"
@@ -32,4 +35,18 @@ test_expect_success pull '
 )
 '
 
+test_expect_failure '--log=1 limits shortlog length' '
+(
+	cd cloned &&
+	git reset --hard HEAD^ &&
+	verbose test "$(cat afile)" = original &&
+	verbose test "$(cat bfile)" = added &&
+	git pull --log=1 &&
+	git log -3 &&
+	git cat-file commit HEAD >result &&
+	verbose grep Dollar result &&
+	verbose grep -v "second commit" result
+)
+'
+
 test_done
-- 
2.1.4

[PATCH 2/2] pull: handle --log=<n>

[Paul Tan]

Since efb779f (merge, pull: add '--(no-)log' command line option,
2008-04-06) git-pull supported the (--no-)log switch and would pass it
to git-merge.

96e9420 (merge: Make '--log' an integer option for number of shortlog
entries, 2010-09-08) implemented support for the --log=<n> switch, which
would explicitly set the number of shortlog entries. However, git-pull
does not recognize this option, and will instead pass it to git-fetch,
leading to "unknown option" errors.

Fix this by matching --log=* in addition to --log and --no-log.

This fixes the failing test '--log=1 limits shortlog length'.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 git-pull.sh         | 4 ++--
 t/t5524-pull-msg.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index 9ed01fd..5ff4545 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -81,8 +81,8 @@ do
 		diffstat=--no-stat ;;
 	--stat|--summary)
 		diffstat=--stat ;;
-	--log|--no-log)
-		log_arg=$1 ;;
+	--log|--log=*|--no-log)
+		log_arg="$1" ;;
 	--no-c|--no-co|--no-com|--no-comm|--no-commi|--no-commit)
 		no_commit=--no-commit ;;
 	--c|--co|--com|--comm|--commi|--commit)
diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
index 593405d..64754b1 100755
--- a/t/t5524-pull-msg.sh
+++ b/t/t5524-pull-msg.sh
@@ -35,7 +35,7 @@ test_expect_success pull '
 )
 '
 
-test_expect_failure '--log=1 limits shortlog length' '
+test_expect_success '--log=1 limits shortlog length' '
 (
 	cd cloned &&
 	git reset --hard HEAD^ &&
-- 
2.1.4

Re: [PATCH 2/2] pull: handle --log=<n>

[Matthieu Moy]

Paul Tan <pyokagan@gmail.com> writes:

> -	--log|--no-log)
> -		log_arg=$1 ;;
> +	--log|--log=*|--no-log)
> +		log_arg="$1" ;;

I think you actually don't need the double quotes here (var=$value works
even if $value has spaces IIRC), but they don't harm and I prefer having
them.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 2/2] pull: handle --log=<n>

[Johannes Schindelin]

Hi Matthieu,

On 2015-05-13 11:38, Matthieu Moy wrote:
> Paul Tan <pyokagan@gmail.com> writes:
> 
>> -	--log|--no-log)
>> -		log_arg=$1 ;;
>> +	--log|--log=*|--no-log)
>> +		log_arg="$1" ;;
> 
> I think you actually don't need the double quotes here (var=$value works
> even if $value has spaces IIRC), but they don't harm and I prefer having
> them.

I am far from a shell expert, but IIRC "$1" converts all whitespace to single spaces. In general, you therefore want to quote arguments, just in case.

Ciao,
Dscho

Re: [PATCH 2/2] pull: handle --log=<n>

[Matthieu Moy]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Hi Matthieu,
>
> On 2015-05-13 11:38, Matthieu Moy wrote:
>> Paul Tan <pyokagan@gmail.com> writes:
>> 
>>> -	--log|--no-log)
>>> -		log_arg=$1 ;;
>>> +	--log|--log=*|--no-log)
>>> +		log_arg="$1" ;;
>> 
>> I think you actually don't need the double quotes here (var=$value works
>> even if $value has spaces IIRC), but they don't harm and I prefer having
>> them.
>
> I am far from a shell expert, but IIRC "$1" converts all whitespace to
> single spaces.

In most places, $1 is split before being interpreted, but there are
exceptions and actually the RHS of assignment is one of them. Just for
curiosity, I digged a reference:

https://www.gnu.org/software/autoconf/manual/autoconf-2.67/html_node/Shell-Substitutions.html
http://unix.stackexchange.com/questions/68694/when-is-double-quoting-necessary

> In general, you therefore want to quote arguments, just in case.

Yes, and one benefit of quoting anyway is to avoid having to have the
discussion we're having ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 0/2] teach git pull to handle --log=<n>

[Matthieu Moy]

Paul Tan <pyokagan@gmail.com> writes:

> Paul Tan (2):
>   t5524: test --log=1 limits shortlog length
>   pull: handle --log=<n>
>
>  git-pull.sh         |  4 ++--
>  t/t5524-pull-msg.sh | 17 +++++++++++++++++
>  2 files changed, 19 insertions(+), 2 deletions(-)

I thought you forgot to update the documentation, but by the magic of
ascidoc includes, the feature was already documented even though it was
not implemented ;-).

Reviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 0/2] teach git pull to handle --log=<n>

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> Since efb779f (merge, pull: add '--(no-)log' command line option,
> 2008-04-06) git-pull supported the (--no-)log switch and would pass it
> to git-merge.
>
> 96e9420 (merge: Make '--log' an integer option for number of shortlog
> entries, 2010-09-08) implemented support for the --log=<n> switch, which
> would explicitly set the number of shortlog entries. However, git-pull
> does not recognize this option, and will instead pass it to git-fetch,
> leading to "unknown option" errors.
>
> This patch series implements a failing test that demonstrates the above,
> and teaches git-pull to handle the switch --log=<n>.

Looks good.

One advice; for a small patch like this one (and the "pull.ff vs
merge.ff" one, too), it is not necessary or even desirable to do a
two-step "first add a failure test and then another patch to fix and
flip the expectation" series.  Just do the fix and add a test to
expect success.

After all, the primary reason why we add test is *not* for you to
demonstrate that what you did works as expected.  It is to catch
other people breaking what you did in the future.

Thanks.

Re: [PATCH 0/2] teach git pull to handle --log=<n>

[Paul Tan]

Hi,

On Fri, May 15, 2015 at 1:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> One advice; for a small patch like this one (and the "pull.ff vs
> merge.ff" one, too), it is not necessary or even desirable to do a
> two-step "first add a failure test and then another patch to fix and
> flip the expectation" series.  Just do the fix and add a test to
> expect success.
>
> After all, the primary reason why we add test is *not* for you to
> demonstrate that what you did works as expected.  It is to catch
> other people breaking what you did in the future.

Since I will be re-rolling all the patches to remove the use of
"verbose" I guess I will squash the patches as well.

Thanks.