All of lore.kernel.org
 help / color / mirror / Atom feed
* git log -z doesn't separate commits with NULs
@ 2012-02-23  9:14 Nikolaj Shurkaev
  2012-02-23 10:02 ` Luke Diamand
                   ` (3 more replies)
  0 siblings, 4 replies; 22+ messages in thread
From: Nikolaj Shurkaev @ 2012-02-23  9:14 UTC (permalink / raw)
  To: git

Hello all.

I wanted to generate several files with some statistics using "git log 
-z" command.
I did something like this:
git log -z --patch HEAD~10..HEAD -- SomePathHere | xargs -0 
--max-chars=1000000 ~/1.sh

If I put
echo "started"
into the file  ~/1.sh I see that the file is called only once instead of 
multiple times.

I'm newbie to xargs, thus I tested with and that worked as I expected.
find . -type f -print0 | xargs -0  ./1.sh
That produced a lost of "started" lines.

Thus I suspect there is a but in git log -z command and that doesn't 
"Separate the commits with NULs instead of with new newlines." as 
promised in the documents.
Is my understanding correct or I don't understand the documentation or 
somehow pass wrong parameters into git log?

Thank you.
Best regards,
Nikolaj

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-23  9:14 git log -z doesn't separate commits with NULs Nikolaj Shurkaev
@ 2012-02-23 10:02 ` Luke Diamand
  2012-02-23 10:27   ` Jeff King
  2012-02-23 10:17 ` Johannes Sixt
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 22+ messages in thread
From: Luke Diamand @ 2012-02-23 10:02 UTC (permalink / raw)
  To: Nikolaj Shurkaev; +Cc: git

On 23/02/12 09:14, Nikolaj Shurkaev wrote:
> Hello all.
>
> I wanted to generate several files with some statistics using "git log
> -z" command.
> I did something like this:
> git log -z --patch HEAD~10..HEAD -- SomePathHere | xargs -0
> --max-chars=1000000 ~/1.sh
>
> If I put
> echo "started"
> into the file ~/1.sh I see that the file is called only once instead of
> multiple times.
>
> I'm newbie to xargs, thus I tested with and that worked as I expected.
> find . -type f -print0 | xargs -0 ./1.sh
> That produced a lost of "started" lines.
>
> Thus I suspect there is a but in git log -z command and that doesn't
> "Separate the commits with NULs instead of with new newlines." as
> promised in the documents.
> Is my understanding correct or I don't understand the documentation or
> somehow pass wrong parameters into git log?

Just a guess, but doesn't the "--patch" option to git log ask it to 
produce a patch output? Surely that will override the -z: patch will not 
be expecting NULs.

>
> Thank you.
> Best regards,
> Nikolaj
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-23  9:14 git log -z doesn't separate commits with NULs Nikolaj Shurkaev
  2012-02-23 10:02 ` Luke Diamand
@ 2012-02-23 10:17 ` Johannes Sixt
  2012-02-23 12:11   ` Nikolaj Shurkaev
  2012-02-23 10:24 ` Jeff King
  2012-02-23 10:35 ` Andreas Schwab
  3 siblings, 1 reply; 22+ messages in thread
From: Johannes Sixt @ 2012-02-23 10:17 UTC (permalink / raw)
  To: Nikolaj Shurkaev; +Cc: git

Am 23.02.2012 10:14, schrieb Nikolaj Shurkaev:
> I wanted to generate several files with some statistics using "git log
> -z" command.
> I did something like this:
> git log -z --patch HEAD~10..HEAD -- SomePathHere | xargs -0
> --max-chars=1000000 ~/1.sh
> 
> If I put
> echo "started"
> into the file  ~/1.sh I see that the file is called only once instead of
> multiple times.

That is because xargs calls the program with as many arguments as
possible, unless directed otherwise. Put this in the script:

	echo "started $*"

and repeat. Then try this:

 ... | xargs -0 -n 1 ~/1.sh

-- Hannes

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-23  9:14 git log -z doesn't separate commits with NULs Nikolaj Shurkaev
  2012-02-23 10:02 ` Luke Diamand
  2012-02-23 10:17 ` Johannes Sixt
@ 2012-02-23 10:24 ` Jeff King
  2012-02-23 12:17   ` Nikolaj Shurkaev
  2012-02-23 10:35 ` Andreas Schwab
  3 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2012-02-23 10:24 UTC (permalink / raw)
  To: Nikolaj Shurkaev; +Cc: git

On Thu, Feb 23, 2012 at 12:14:23PM +0300, Nikolaj Shurkaev wrote:

> I wanted to generate several files with some statistics using "git
> log -z" command.
> I did something like this:
> git log -z --patch HEAD~10..HEAD -- SomePathHere | xargs -0
> --max-chars=1000000 ~/1.sh

I'm not sure what "1.sh" is expecting to take as input, but that will
feed entire commits, including their commit message and entire diff, to
the script on its command line.

That seems like an awkward interface, but we don't really know what your
script intends to do. Maybe it is worth sharing the contents of the
script.

> If I put echo "started" into the file  ~/1.sh I see that the file is
> called only once instead of multiple times.

Yes. The point of xargs is usually to cram as many arguments into each
invocation of "1.sh" as possible, splitting into multiple invocations
only when we hit the argument-list memory limit that the OS imposes.

If you want xargs to give each argument its own invocation of the
script, use "xargs -n1".

> I'm newbie to xargs, thus I tested with and that worked as I expected.
> find . -type f -print0 | xargs -0  ./1.sh
> That produced a lost of "started" lines.

If you instrument your 1.sh more[1], you will find that is not executing
once per file, but rather getting a large chunk of files per invocation.

[1] Try adding: echo "got args: $*"

> Thus I suspect there is a but in git log -z command and that doesn't
> "Separate the commits with NULs instead of with new newlines." as
> promised in the documents.

You could verify that assertion by looking at the output. Try piping
your "git log" command through "cat -A | less". When I try it, I see a
NUL between each commit (cat -A will show it as "^@").

-Peff

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-23 10:02 ` Luke Diamand
@ 2012-02-23 10:27   ` Jeff King
  0 siblings, 0 replies; 22+ messages in thread
From: Jeff King @ 2012-02-23 10:27 UTC (permalink / raw)
  To: Luke Diamand; +Cc: Nikolaj Shurkaev, git

On Thu, Feb 23, 2012 at 10:02:31AM +0000, Luke Diamand wrote:

> >Thus I suspect there is a but in git log -z command and that doesn't
> >"Separate the commits with NULs instead of with new newlines." as
> >promised in the documents.
> >Is my understanding correct or I don't understand the documentation or
> >somehow pass wrong parameters into git log?
> 
> Just a guess, but doesn't the "--patch" option to git log ask it to
> produce a patch output? Surely that will override the -z: patch will
> not be expecting NULs.

No. You will get the patch text and the log message together, with
commits separated by NUL. Some diff output formats will also respect
"-z" to produce NULs internally (e.g., "--raw" will use it to separate
filenames), but "--patch" does not.

-Peff

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-23  9:14 git log -z doesn't separate commits with NULs Nikolaj Shurkaev
                   ` (2 preceding siblings ...)
  2012-02-23 10:24 ` Jeff King
@ 2012-02-23 10:35 ` Andreas Schwab
  2012-02-23 12:19   ` Nikolaj Shurkaev
  3 siblings, 1 reply; 22+ messages in thread
From: Andreas Schwab @ 2012-02-23 10:35 UTC (permalink / raw)
  To: Nikolaj Shurkaev; +Cc: git

Nikolaj Shurkaev <snnicky@gmail.com> writes:

> I did something like this:
> git log -z --patch HEAD~10..HEAD -- SomePathHere | xargs -0 
> --max-chars=1000000 ~/1.sh
>
> If I put
> echo "started"
> into the file  ~/1.sh I see that the file is called only once instead of
> multiple times.

If you want the command to be called once for each commit you need to
pass --max-args=1 to xargs.  Otherwise xargs will cumulate the arguments
until --max-chars is reached, and the command is expected to loop over
them.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-23 10:17 ` Johannes Sixt
@ 2012-02-23 12:11   ` Nikolaj Shurkaev
  0 siblings, 0 replies; 22+ messages in thread
From: Nikolaj Shurkaev @ 2012-02-23 12:11 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: git

Hello.

You were right. I added parameter --max-args=1 to xargs command and that 
started work as I wanted initially.
Thank you very much.

--
Nikolaj

23.02.2012 13:17, Johannes Sixt пишет:
> Am 23.02.2012 10:14, schrieb Nikolaj Shurkaev:
>> I wanted to generate several files with some statistics using "git log
>> -z" command.
>> I did something like this:
>> git log -z --patch HEAD~10..HEAD -- SomePathHere | xargs -0
>> --max-chars=1000000 ~/1.sh
>>
>> If I put
>> echo "started"
>> into the file  ~/1.sh I see that the file is called only once instead of
>> multiple times.
> That is because xargs calls the program with as many arguments as
> possible, unless directed otherwise. Put this in the script:
>
> 	echo "started $*"
>
> and repeat. Then try this:
>
>   ... | xargs -0 -n 1 ~/1.sh
>
> -- Hannes
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-23 10:24 ` Jeff King
@ 2012-02-23 12:17   ` Nikolaj Shurkaev
  2012-02-23 13:15     ` Jakub Narebski
  0 siblings, 1 reply; 22+ messages in thread
From: Nikolaj Shurkaev @ 2012-02-23 12:17 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Hello.

Thank you very much for your tips. They really helped me. I was trying 
to create patches that would affect only some given files or folders. By 
this moment I have the following:

GeneratePatches.sh
---------------------
#!/bin/bash
#parameter 1 - <since>..<to>
#parameter 2 - path to file
git log -z --reverse --format=email --patch "$1" -- "$2" | xargs --null 
--max-args=1 ./CreatePatchFile.sh
---------------------

and CreatePatchFile.sh
---------------------
#!/bin/bash

myPatchNumber=$(ls ./*-patch.patch 2>/dev/null | wc -l)
let "myPatchNumber += 1"

patchFile="./"$(printf "%04d" $myPatchNumber)"-patch.patch"
echo "$@" > "$patchFile"
---------------------

I call
./GeneratePatches.sh HEAD~3..HEAD SomePath
and that produces something very similar to what I want.

Perhaps there is a better way to do that.

Thank you once again.
---
Best regards,
Nikolaj

23.02.2012 13:24, Jeff King пишет:
> On Thu, Feb 23, 2012 at 12:14:23PM +0300, Nikolaj Shurkaev wrote:
>
>> I wanted to generate several files with some statistics using "git
>> log -z" command.
>> I did something like this:
>> git log -z --patch HEAD~10..HEAD -- SomePathHere | xargs -0
>> --max-chars=1000000 ~/1.sh
> I'm not sure what "1.sh" is expecting to take as input, but that will
> feed entire commits, including their commit message and entire diff, to
> the script on its command line.
>
> That seems like an awkward interface, but we don't really know what your
> script intends to do. Maybe it is worth sharing the contents of the
> script.
>
>> If I put echo "started" into the file  ~/1.sh I see that the file is
>> called only once instead of multiple times.
> Yes. The point of xargs is usually to cram as many arguments into each
> invocation of "1.sh" as possible, splitting into multiple invocations
> only when we hit the argument-list memory limit that the OS imposes.
>
> If you want xargs to give each argument its own invocation of the
> script, use "xargs -n1".
>
>> I'm newbie to xargs, thus I tested with and that worked as I expected.
>> find . -type f -print0 | xargs -0  ./1.sh
>> That produced a lost of "started" lines.
> If you instrument your 1.sh more[1], you will find that is not executing
> once per file, but rather getting a large chunk of files per invocation.
>
> [1] Try adding: echo "got args: $*"
>
>> Thus I suspect there is a but in git log -z command and that doesn't
>> "Separate the commits with NULs instead of with new newlines." as
>> promised in the documents.
> You could verify that assertion by looking at the output. Try piping
> your "git log" command through "cat -A | less". When I try it, I see a
> NUL between each commit (cat -A will show it as "^@").
>
> -Peff
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-23 10:35 ` Andreas Schwab
@ 2012-02-23 12:19   ` Nikolaj Shurkaev
  0 siblings, 0 replies; 22+ messages in thread
From: Nikolaj Shurkaev @ 2012-02-23 12:19 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: git

Thank you.

That really helped me.
---
Nikolaj

23.02.2012 13:35, Andreas Schwab пишет:
> Nikolaj Shurkaev<snnicky@gmail.com>  writes:
>
>> I did something like this:
>> git log -z --patch HEAD~10..HEAD -- SomePathHere | xargs -0
>> --max-chars=1000000 ~/1.sh
>>
>> If I put
>> echo "started"
>> into the file  ~/1.sh I see that the file is called only once instead of
>> multiple times.
> If you want the command to be called once for each commit you need to
> pass --max-args=1 to xargs.  Otherwise xargs will cumulate the arguments
> until --max-chars is reached, and the command is expected to loop over
> them.
>
> Andreas.
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-23 12:17   ` Nikolaj Shurkaev
@ 2012-02-23 13:15     ` Jakub Narebski
  2012-02-23 13:48       ` Nikolaj Shurkaev
  0 siblings, 1 reply; 22+ messages in thread
From: Jakub Narebski @ 2012-02-23 13:15 UTC (permalink / raw)
  To: Nikolaj Shurkaev; +Cc: Jeff King, git

Nikolaj Shurkaev <snnicky@gmail.com> writes:

> Thank you very much for your tips. They really helped me. I was trying
> to create patches that would affect only some given files or
> folders. By this moment I have the following:
> 
> GeneratePatches.sh
> ---------------------
> #!/bin/bash
> #parameter 1 - <since>..<to>
> #parameter 2 - path to file
> git log -z --reverse --format=email --patch "$1" -- "$2" | xargs
> --null --max-args=1 ./CreatePatchFile.sh
> ---------------------
> 
> and CreatePatchFile.sh
> ---------------------
> #!/bin/bash
> 
> myPatchNumber=$(ls ./*-patch.patch 2>/dev/null | wc -l)
> let "myPatchNumber += 1"
> 
> patchFile="./"$(printf "%04d" $myPatchNumber)"-patch.patch"
> echo "$@" > "$patchFile"
> ---------------------
> 
> I call
> ./GeneratePatches.sh HEAD~3..HEAD SomePath
> and that produces something very similar to what I want.
> 
> Perhaps there is a better way to do that.

So what git-format-patch is lacking?

-- 
Jakub Narebski

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-23 13:15     ` Jakub Narebski
@ 2012-02-23 13:48       ` Nikolaj Shurkaev
  2012-02-23 19:34         ` Jeff King
  0 siblings, 1 reply; 22+ messages in thread
From: Nikolaj Shurkaev @ 2012-02-23 13:48 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: Jeff King, git

For example there are commits that affect not only files in folder A but 
files in folder B, C and so on.
If I do format-patch that will give me nice patches, but there are 
modifications of folders B, C and so on there.
I do not know a way to generate patches via format-patch that affect 
only files in folder A.

This is why I wrote those scripts.

23.02.2012 16:15, Jakub Narebski пишет:
> Nikolaj Shurkaev<snnicky@gmail.com>  writes:
>
>> Thank you very much for your tips. They really helped me. I was trying
>> to create patches that would affect only some given files or
>> folders. By this moment I have the following:
>>
>> GeneratePatches.sh
>> ---------------------
>> #!/bin/bash
>> #parameter 1 -<since>..<to>
>> #parameter 2 - path to file
>> git log -z --reverse --format=email --patch "$1" -- "$2" | xargs
>> --null --max-args=1 ./CreatePatchFile.sh
>> ---------------------
>>
>> and CreatePatchFile.sh
>> ---------------------
>> #!/bin/bash
>>
>> myPatchNumber=$(ls ./*-patch.patch 2>/dev/null | wc -l)
>> let "myPatchNumber += 1"
>>
>> patchFile="./"$(printf "%04d" $myPatchNumber)"-patch.patch"
>> echo "$@">  "$patchFile"
>> ---------------------
>>
>> I call
>> ./GeneratePatches.sh HEAD~3..HEAD SomePath
>> and that produces something very similar to what I want.
>>
>> Perhaps there is a better way to do that.
> So what git-format-patch is lacking?
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-23 13:48       ` Nikolaj Shurkaev
@ 2012-02-23 19:34         ` Jeff King
  2012-02-23 20:07           ` Junio C Hamano
  0 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2012-02-23 19:34 UTC (permalink / raw)
  To: Nikolaj Shurkaev; +Cc: Jakub Narebski, git

On Thu, Feb 23, 2012 at 04:48:43PM +0300, Nikolaj Shurkaev wrote:

> For example there are commits that affect not only files in folder A
> but files in folder B, C and so on.  If I do format-patch that will
> give me nice patches, but there are modifications of folders B, C and
> so on there.  I do not know a way to generate patches via format-patch
> that affect only files in folder A.

Doesn't:

  git format-patch HEAD~3..HEAD SomePath

do what you want? It is certainly designed to, and it seems to work for
me.

-Peff

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-23 19:34         ` Jeff King
@ 2012-02-23 20:07           ` Junio C Hamano
  2012-02-24  9:21             ` Nikolaj Shurkaev
  0 siblings, 1 reply; 22+ messages in thread
From: Junio C Hamano @ 2012-02-23 20:07 UTC (permalink / raw)
  To: Jeff King; +Cc: Nikolaj Shurkaev, Jakub Narebski, git

Jeff King <peff@peff.net> writes:

> On Thu, Feb 23, 2012 at 04:48:43PM +0300, Nikolaj Shurkaev wrote:
>
> Doesn't:
>
>   git format-patch HEAD~3..HEAD SomePath
>
> do what you want? It is certainly designed to, and it seems to work for
> me.

It is not quite "designed to", though.

It happens to work that way, and I do not think we want to forbid its use,
but we would want to discourage anybody from blindly using it without
thinking if the end results suits his/her purpose (and the reason should
be obvious to those who think, the hint is "log message").

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-23 20:07           ` Junio C Hamano
@ 2012-02-24  9:21             ` Nikolaj Shurkaev
  2012-02-24  9:52               ` Jeff King
  0 siblings, 1 reply; 22+ messages in thread
From: Nikolaj Shurkaev @ 2012-02-24  9:21 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Jakub Narebski, git

Hello.

Thank you for the hint.

git format-patch HEAD~3..HEAD -- SomePath

does exactly what I need. But that way of usage is not described in git 
documentation thus I thought there is no way to do that. I've just 
double checked

git format-patch --help

doesn't describe that. I'll propose to put something like this into git 
documentation
--------------------------------------------------------------------------------------------
diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 6ea9be7..63267c6 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -23,6 +23,7 @@ SYNOPSIS
[--cover-letter] [--quiet]
[<common diff options>]
[ <since> | <revision range> ]
+ [[\--] <path>...]

DESCRIPTION
-----------
@@ -219,6 +220,12 @@ you can use `--suffix=-patch` to get 
`0001-description-of-my-change-patch`.
range are always formatted as creation patches, independently
of this flag.

+[\--] <path>...::
+ Put in patches only those modifications that affect specified files
+ and folders. It's important to understand that log message of the
+ commit may become inappropriate because some parts of patch may be
+ cut off.
+
CONFIGURATION
-------------
You can specify extra mail header lines to be added to each message,
--------------------------------------------------------------------------------------------

--
Nikolaj.

23.02.2012 23:07, Junio C Hamano пишет:
> Jeff King<peff@peff.net>  writes:
>
>> On Thu, Feb 23, 2012 at 04:48:43PM +0300, Nikolaj Shurkaev wrote:
>>
>> Doesn't:
>>
>>    git format-patch HEAD~3..HEAD SomePath
>>
>> do what you want? It is certainly designed to, and it seems to work for
>> me.
> It is not quite "designed to", though.
>
> It happens to work that way, and I do not think we want to forbid its use,
> but we would want to discourage anybody from blindly using it without
> thinking if the end results suits his/her purpose (and the reason should
> be obvious to those who think, the hint is "log message").
>

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-24  9:21             ` Nikolaj Shurkaev
@ 2012-02-24  9:52               ` Jeff King
  2012-02-24 20:03                 ` Junio C Hamano
  0 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2012-02-24  9:52 UTC (permalink / raw)
  To: Nikolaj Shurkaev; +Cc: Junio C Hamano, Jakub Narebski, git

On Fri, Feb 24, 2012 at 12:21:13PM +0300, Nikolaj Shurkaev wrote:

> I'll propose to put something like this into git documentation
> --------------------------------------------------------------------------------------------
> diff --git a/Documentation/git-format-patch.txt
> b/Documentation/git-format-patch.txt
> index 6ea9be7..63267c6 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -23,6 +23,7 @@ SYNOPSIS
> [--cover-letter] [--quiet]
> [<common diff options>]
> [ <since> | <revision range> ]
> + [[\--] <path>...]
> 
> DESCRIPTION
> -----------
> @@ -219,6 +220,12 @@ you can use `--suffix=-patch` to get
> `0001-description-of-my-change-patch`.
> range are always formatted as creation patches, independently
> of this flag.
> 
> +[\--] <path>...::
> + Put in patches only those modifications that affect specified files
> + and folders. It's important to understand that log message of the
> + commit may become inappropriate because some parts of patch may be
> + cut off.
> +

I think that text looks OK. But to my mind, it is not that format-patch
accepts a path parameter, but rather that it takes arbitrary log-like
arguments. So you could do "git format-patch --grep=whatever", or even
something like "git format-patch --cherry".

I don't know how well tested every option is, though, so maybe it's not
a good idea to encourage the use of random options.

-Peff

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-24  9:52               ` Jeff King
@ 2012-02-24 20:03                 ` Junio C Hamano
  2012-02-24 20:46                   ` Jeff King
  0 siblings, 1 reply; 22+ messages in thread
From: Junio C Hamano @ 2012-02-24 20:03 UTC (permalink / raw)
  To: Jeff King; +Cc: Nikolaj Shurkaev, Jakub Narebski, git

Jeff King <peff@peff.net> writes:

>> DESCRIPTION
>> -----------
>> @@ -219,6 +220,12 @@ you can use `--suffix=-patch` to get
>> `0001-description-of-my-change-patch`.
>> range are always formatted as creation patches, independently
>> of this flag.
>> 
>> +[\--] <path>...::
>> + Put in patches only those modifications that affect specified files
>> + and folders. It's important to understand that log message of the
>> + commit may become inappropriate because some parts of patch may be
>> + cut off.
>> +
>
> I think that text looks OK. But to my mind, it is not that format-patch
> accepts a path parameter, but rather that it takes arbitrary log-like
> arguments.

The above text is not telling the entire truth, though.

When the command is run with the "--full-diff" option, seleted commits
will be shown in full.  This is useful for example when you want to pick
commits that add a new "frotz" driver, which obviously needs to include
"drivers/frotz/" subdirectory, without missing necessary changes to the
Makefiles in the higher level (e.g. "drivers/Makefile"), e.g.

	git format-patch --full-diff v1.0..v1.1 -- drivers/frotz

In such a case, "some parts may be cut off" does not make the log message
inappropriate.

On the other hand, people often do not use the resulting history of taking
partial patches (i.e. without --full-diff) and feeding them to "am" as-is.
The operation is used merely to give them a starting point for working on
(possibly) an unrelated topic, and the history is further tweaked with
"rebase -i" or even "commit --amend".  It is not "inappropriate" that the
log says more than what the patch does in such a use case.  What the log
says is irrelevant.

> I don't know how well tested every option is, though, so maybe it's not
> a good idea to encourage the use of random options.

I obviously agree and also suspect that the real question is not "how well
tested" but "if it makes sense".

I am reasonably sure that over time the options and features that make
sense in the context of producing something that is useful with "am" have
been already made to work well, but I also am fairly certain that the
coverage of the code to explicitly reject options that do not make sense
in that context would be spotty at best.  For example, did we carefully
design and implement how format-patch should behave when "-z" is given,
or does the code happen to do whatever it happens to do?  If the latter,
did we consider rejecting "-z" when given from the command line and
implement such safety?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-24 20:03                 ` Junio C Hamano
@ 2012-02-24 20:46                   ` Jeff King
  2012-02-24 21:14                     ` Junio C Hamano
  0 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2012-02-24 20:46 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Nikolaj Shurkaev, Jakub Narebski, git

On Fri, Feb 24, 2012 at 12:03:39PM -0800, Junio C Hamano wrote:

> The above text is not telling the entire truth, though.
>
> When the command is run with the "--full-diff" option, seleted commits
> will be shown in full.  This is useful for example when you want to pick
> commits that add a new "frotz" driver, which obviously needs to include
> "drivers/frotz/" subdirectory, without missing necessary changes to the
> Makefiles in the higher level (e.g. "drivers/Makefile"), e.g.
> 
> 	git format-patch --full-diff v1.0..v1.1 -- drivers/frotz
> 
> In such a case, "some parts may be cut off" does not make the log message
> inappropriate.

True. That is also a slightly dangerous thing to do, though, because you
are omitting full patches in the middle that touch the same paths as the
patches you include. So I might send you a patch against Makefile that
does not apply, and nor do you have the interim sha1.

Of course that is a general problem with any commit-limiting in
format-patch (e.g., --grep), and even with sending patches in general (I
have to make sure I based my patch off of something reasonable in the
first place). The key is to be clueful about what you are doing. So
perhaps we are better off to refer the user to git-log(1), say that
commit limiting options in general would work, but be careful with
sending a partial result.

> On the other hand, people often do not use the resulting history of taking
> partial patches (i.e. without --full-diff) and feeding them to "am" as-is.
> The operation is used merely to give them a starting point for working on
> (possibly) an unrelated topic, and the history is further tweaked with
> "rebase -i" or even "commit --amend".  It is not "inappropriate" that the
> log says more than what the patch does in such a use case.  What the log
> says is irrelevant.

Right. I think this comes down to the "clueful" bit. If you understand
what it is you are asking git to do and deciding that the consequences
are OK (either because you are not using the log message, or you know
that the subset of a series you are sending should apply to what the
receiver has).

> > I don't know how well tested every option is, though, so maybe it's not
> > a good idea to encourage the use of random options.
> 
> I obviously agree and also suspect that the real question is not "how well
> tested" but "if it makes sense".
> 
> I am reasonably sure that over time the options and features that make
> sense in the context of producing something that is useful with "am" have
> been already made to work well, but I also am fairly certain that the
> coverage of the code to explicitly reject options that do not make sense
> in that context would be spotty at best.  For example, did we carefully
> design and implement how format-patch should behave when "-z" is given,
> or does the code happen to do whatever it happens to do?  If the latter,
> did we consider rejecting "-z" when given from the command line and
> implement such safety?

Yeah, I think that is a good way of putting it. I tend to think the
commit-limiting options are the useful and working ones, which is why I
suggested mentioning them explicitly above. But I admit I don't use them
myself, so I'm just guessing.

-Peff

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-24 20:46                   ` Jeff King
@ 2012-02-24 21:14                     ` Junio C Hamano
  2012-02-24 21:16                       ` Jeff King
  2012-02-24 22:11                       ` Jakub Narebski
  0 siblings, 2 replies; 22+ messages in thread
From: Junio C Hamano @ 2012-02-24 21:14 UTC (permalink / raw)
  To: Jeff King; +Cc: Nikolaj Shurkaev, Jakub Narebski, git

Jeff King <peff@peff.net> writes:

> True. That is also a slightly dangerous thing to do, though, because you
> are omitting full patches in the middle that touch the same paths as the
> patches you include....
> ... So
> perhaps we are better off to refer the user to git-log(1), say that
> commit limiting options in general would work, but be careful with
> sending a partial result.

You seem to have spelled out everything I originally wrote in my reply
that I later deleted before sending it out, and I think the reason that
brought you to the three-line conclusion is the same one that made me I
delete them ;-).

Using a partial patch essentially has the same risk as cherry-picking a
commit into different context, and it is a more generic issue that this
particular manual page should not waste tons of space to teach readers
about.  I think "Be careful and clueful" is sufficient and the best we can
do without writing a textbook on distributed software development
disciplines.

> ... I tend to think the
> commit-limiting options are the useful and working ones, which is why I
> suggested mentioning them explicitly above.

I think I agree that is a sane thing to do.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-24 21:14                     ` Junio C Hamano
@ 2012-02-24 21:16                       ` Jeff King
  2012-03-03 13:41                         ` Nikolaj Shurkaev
  2012-02-24 22:11                       ` Jakub Narebski
  1 sibling, 1 reply; 22+ messages in thread
From: Jeff King @ 2012-02-24 21:16 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Nikolaj Shurkaev, Jakub Narebski, git

On Fri, Feb 24, 2012 at 01:14:05PM -0800, Junio C Hamano wrote:

> > True. That is also a slightly dangerous thing to do, though, because you
> > are omitting full patches in the middle that touch the same paths as the
> > patches you include....
> > ... So
> > perhaps we are better off to refer the user to git-log(1), say that
> > commit limiting options in general would work, but be careful with
> > sending a partial result.
> 
> You seem to have spelled out everything I originally wrote in my reply
> that I later deleted before sending it out, and I think the reason that
> brought you to the three-line conclusion is the same one that made me I
> delete them ;-).

OK, good. :)

Nikolaj, have you followed all of this? Do you want to try to improve
your patch in this direction?

-Peff

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-24 21:14                     ` Junio C Hamano
  2012-02-24 21:16                       ` Jeff King
@ 2012-02-24 22:11                       ` Jakub Narebski
  2012-02-24 22:27                         ` Junio C Hamano
  1 sibling, 1 reply; 22+ messages in thread
From: Jakub Narebski @ 2012-02-24 22:11 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Nikolaj Shurkaev, git

Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > True. That is also a slightly dangerous thing to do, though, because you
> > are omitting full patches in the middle that touch the same paths as the
> > patches you include....
> > ... So
> > perhaps we are better off to refer the user to git-log(1), say that
> > commit limiting options in general would work, but be careful with
> > sending a partial result.
> 
> You seem to have spelled out everything I originally wrote in my reply
> that I later deleted before sending it out, and I think the reason that
> brought you to the three-line conclusion is the same one that made me I
> delete them ;-).
> 
> Using a partial patch essentially has the same risk as cherry-picking a
> commit into different context, and it is a more generic issue that this
> particular manual page should not waste tons of space to teach readers
> about.  I think "Be careful and clueful" is sufficient and the best we can
> do without writing a textbook on distributed software development
> disciplines.

Perhaps git-format-patch should mention that it was created with
path-limited patch in some email pseudo-header like X-Pathspec:
or something, don't you think?

-- 
Jakub Narebski
Poland

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-24 22:11                       ` Jakub Narebski
@ 2012-02-24 22:27                         ` Junio C Hamano
  0 siblings, 0 replies; 22+ messages in thread
From: Junio C Hamano @ 2012-02-24 22:27 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: Jeff King, Nikolaj Shurkaev, git

Jakub Narebski <jnareb@gmail.com> writes:

> Perhaps git-format-patch should mention that it was created with
> path-limited patch in some email pseudo-header like X-Pathspec:
> or something, don't you think?

What kind of workflow are you assuming, and who would benefit from such a
header under that assumption?

It obviously would not help the person who is running format-patch, as he
is very well aware that he is giving a pathspec when he runs the command.
If the result is used privately to prepare a starting point of possibly
unrelated work, it does not matter.

If you are assuming a workflow that involves a public review of mailed
patches, it is very likely that such a header will be lost unless you are
using git-send-email, as we strongly discourage copying and pasting the
entire thing in the message body.  Anybody sitting on the receiving end
worth her salt would judge the submission by looking at the log message,
diffstat and the patch, and the sender having used pathspec to format the
patch would not be a reason for rejection at all---the quality of the
submission is.

So offhand, I do not know under what workflow such an extra header would
benefit people in what position in the workflow.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: git log -z doesn't separate commits with NULs
  2012-02-24 21:16                       ` Jeff King
@ 2012-03-03 13:41                         ` Nikolaj Shurkaev
  0 siblings, 0 replies; 22+ messages in thread
From: Nikolaj Shurkaev @ 2012-03-03 13:41 UTC (permalink / raw)
  Cc: git

As far as I understood from what I read and from a brief look at source 
code of git (builtin/log.c file) the best what we can do is just mention 
that a lot of log options are applicable and warn user to use them with 
understanding of the consequences. I do not think that documentation of 
some of options in Documentation/git-format-patch.txt makes sense 
because some of the options are connected one with another. For example 
--full-diff and <path> are connected as Junio C Hamano wrote. And that 
dependency is already explained in Documentation/git-log.txt.

Another option that I see is to put git-log options description into a 
separate file and include that from git-log.txt and from 
git-format-patch.txt like that is done with rev-list-options.txt. That 
could be useful in a long term. Because new options may appear or some 
may disappear. Some dependency between options may be documented. 
However for me that would require to get better understanding how all 
that documentation is built. And that could require modifications of 
git-log help also.

Please let me know how to proceed with that properly. Perhaps I should 
start a separate discussion thread devoted to the documentation 
enhancement. Does thread subject matter?

Thank you.
----------------------
diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 6ea9be7..d49ec80 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -68,6 +68,15 @@ If given `--thread`, `git-format-patch` will generate 
`In-Reply-To` and
  as replies to the first mail; this also generates a `Message-Id` header to
  reference.

+NOTES
+-----
+
+`git format-patch` and `git log` share significant part of their
+implementations. As a result a lot of options available and described for
+`git log` will work for `git format-patch` also. However it's required to
+use them carefully with understanding of the consequences. For example
+applying `<path>` option may make log messages irrelevant.
+
  OPTIONS
  -------
  :git-format-patch: 1


25.02.2012 0:16, Jeff King пишет:
> On Fri, Feb 24, 2012 at 01:14:05PM -0800, Junio C Hamano wrote:
>
>>> True. That is also a slightly dangerous thing to do, though, because you
>>> are omitting full patches in the middle that touch the same paths as the
>>> patches you include....
>>> ... So
>>> perhaps we are better off to refer the user to git-log(1), say that
>>> commit limiting options in general would work, but be careful with
>>> sending a partial result.
>> You seem to have spelled out everything I originally wrote in my reply
>> that I later deleted before sending it out, and I think the reason that
>> brought you to the three-line conclusion is the same one that made me I
>> delete them ;-).
> OK, good. :)
>
> Nikolaj, have you followed all of this? Do you want to try to improve
> your patch in this direction?
>
> -Peff
>

^ permalink raw reply related	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2012-03-03 13:41 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-23  9:14 git log -z doesn't separate commits with NULs Nikolaj Shurkaev
2012-02-23 10:02 ` Luke Diamand
2012-02-23 10:27   ` Jeff King
2012-02-23 10:17 ` Johannes Sixt
2012-02-23 12:11   ` Nikolaj Shurkaev
2012-02-23 10:24 ` Jeff King
2012-02-23 12:17   ` Nikolaj Shurkaev
2012-02-23 13:15     ` Jakub Narebski
2012-02-23 13:48       ` Nikolaj Shurkaev
2012-02-23 19:34         ` Jeff King
2012-02-23 20:07           ` Junio C Hamano
2012-02-24  9:21             ` Nikolaj Shurkaev
2012-02-24  9:52               ` Jeff King
2012-02-24 20:03                 ` Junio C Hamano
2012-02-24 20:46                   ` Jeff King
2012-02-24 21:14                     ` Junio C Hamano
2012-02-24 21:16                       ` Jeff King
2012-03-03 13:41                         ` Nikolaj Shurkaev
2012-02-24 22:11                       ` Jakub Narebski
2012-02-24 22:27                         ` Junio C Hamano
2012-02-23 10:35 ` Andreas Schwab
2012-02-23 12:19   ` Nikolaj Shurkaev

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.