threads / patch / 39324

patch, 2 partsteach git pull to handle --log=<n>

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

## tl;dr

9 messages between May 13, 2015 and May 15, 2015. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Paul Tan· May 13, 2015, 09:17 UTC · lore

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
Paul Tan· May 13, 2015, 09:17 UTC · re: Paul Tan · lore

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

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(+)
Show changes to t/t5524-pull-msg.sh +17 −0
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
Paul Tan· May 13, 2015, 09:17 UTC · re: Paul Tan · lore

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

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(-)
Show changes to 2 files +3 −3

git-pull.sh, t/t5524-pull-msg.sh

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
Matthieu Moy· May 13, 2015, 09:38 UTC · re: Paul Tan · lore

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

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/
Johannes Schindelin· May 13, 2015, 12:23 UTC · re: Matthieu Moy · lore

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

Hi Matthieu,
On 2015-05-13 11:38, Matthieu Moy wrote:
Show 10 quoted lines
> 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

Matthieu Moy· May 13, 2015, 20:03 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 16 quoted lines
> 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/
Matthieu Moy· May 13, 2015, 09:40 UTC · re: Paul Tan · lore

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

Paul Tan <pyokagan@gmail.com> writes:
Show 7 quoted lines
> 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/
Junio C Hamano· May 14, 2015, 17:11 UTC · re: Paul Tan · lore

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

Paul Tan <pyokagan@gmail.com> writes:
Show 12 quoted lines
> 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.
Paul Tan· May 15, 2015, 11:15 UTC · re: Junio C Hamano · lore

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

Hi,
On Fri, May 15, 2015 at 1:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> 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.

← back to recent threads