[PATCH] [RFC] Get subject of current patch in rebase-apply mode

[PATCH] [RFC] Get subject of current patch in rebase-apply mode

[Daniel Hahler]

From: Daniel Hahler <git@thequod.de>

---
 Functions/VCS_Info/Backends/VCS_INFO_get_data_git | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
index 18ba89a..93172fc 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
+++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
@@ -236,6 +236,15 @@ elif [[ -d "${gitdir}/rebase-apply" ]]; then
         done
         if [[ -f "${patchdir}/msg-clean" ]]; then
             subject="${$(< "${patchdir}/msg-clean")[(f)1]}"
+        else
+            local maxlines=10
+            while IFS='\n' read -r;  do
+                if [[ "$REPLY" == "Subject:"* ]]; then
+                    subject=${REPLY/(#s)Subject: /}
+                    break
+                fi
+                (( --maxlines )) || break
+            done < "${patchdir}/$(printf "%04d" $cur)"
         fi
         if [[ -f "${patchdir}/original-commit" ]]; then
             if [[ -n $subject ]]; then
-- 
2.10.2

Re: [PATCH] [RFC] Get subject of current patch in rebase-apply mode

[Daniel Hahler]

[-- Attachment #1.1: Type: text/plain, Size: 1273 bytes --]

Please acknowledge that this patch is fine, before i) commit it.

It works for me, but I am unsure about style etc.


On 20.11.2016 02:33, Daniel Hahler wrote:
> From: Daniel Hahler
> 
> ---
>  Functions/VCS_Info/Backends/VCS_INFO_get_data_git | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
> index 18ba89a..93172fc 100644
> --- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
> +++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
> @@ -236,6 +236,15 @@ elif [[ -d "${gitdir}/rebase-apply" ]]; then
>          done
>          if [[ -f "${patchdir}/msg-clean" ]]; then
>              subject="${$(< "${patchdir}/msg-clean")[(f)1]}"
> +        else
> +            local maxlines=10
> +            while IFS='\n' read -r;  do
> +                if [[ "$REPLY" == "Subject:"* ]]; then
> +                    subject=${REPLY/(#s)Subject: /}
> +                    break
> +                fi
> +                (( --maxlines )) || break
> +            done < "${patchdir}/$(printf "%04d" $cur)"
>          fi
>          if [[ -f "${patchdir}/original-commit" ]]; then
>              if [[ -n $subject ]]; then
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 163 bytes --]

Re: [PATCH] [RFC] Get subject of current patch in rebase-apply mode

[Frank Terbeck]

Hi,

I'd use a named local parameter instead of REPLY and clear out IFS
completely:

Daniel Hahler wrote:
> +            local maxlines=10
> +            while IFS='\n' read -r;  do

               local maxlines=10 line
               while IFS= read -r line;  do

…and then use $line instead of $REPLY in the rest.


Regards, Frank

Re: [PATCH] [RFC] Get subject of current patch in rebase-apply mode

[Daniel Shahaf]

Daniel Hahler wrote on Sun, Nov 20, 2016 at 02:33:25 +0100:
> From: Daniel Hahler <git@thequod.de>
> 
> ---
>  Functions/VCS_Info/Backends/VCS_INFO_get_data_git | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
> index 18ba89a..93172fc 100644
> --- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
> +++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
> @@ -236,6 +236,15 @@ elif [[ -d "${gitdir}/rebase-apply" ]]; then
>          done
>          if [[ -f "${patchdir}/msg-clean" ]]; then
>              subject="${$(< "${patchdir}/msg-clean")[(f)1]}"

I use an older version of git that still creates msg-clean files (see
36830), so I rm'd the msg-clean file by hand to test your patch.

> +        else
> +            local maxlines=10
> +            while IFS='\n' read -r;  do

Change '\n' to $'\n'.  (But read the last paragraph first.)

> +                if [[ "$REPLY" == "Subject:"* ]]; then
> +                    subject=${REPLY/(#s)Subject: /}
> +                    break
> +                fi
> +                (( --maxlines )) || break
> +            done < "${patchdir}/$(printf "%04d" $cur)"

I'm not an expert on git internal data structures, but I see two failure
modes to this patch: the file "$patchdir/${(l:4::0:)cur}" might not
exist, or might correspond to a different commit than $cur.  The former
case could be addressed by adding a [[ -f ]] existence check, as
elsewhere in the file.  The latter would cause information for a wrong
commit to be printed instead of a commit hash.  That would be a bug, but
it seems unlikely enough.

So I would suggest two code changes: use «${(l:4::0:)cur}» or the new
'printf -v' instead of a command substitution to save a fork on Cygwin,
and consider adding a [[ -f ]] check to prevent error messages.

One last thing I'd suggest is to consider using VCS_INFO_quilt-patch2subject,
since it implements rfc822 unfolding of long subject lines.

Cheers,

Daniel

>          fi
>          if [[ -f "${patchdir}/original-commit" ]]; then
>              if [[ -n $subject ]]; then
> -- 
> 2.10.2
> 
> 

[PATCH] fixup! [RFC] Get subject of current patch in rebase-apply mode

[Daniel Hahler]

From: Daniel Hahler <git@thequod.de>

---
 Functions/VCS_Info/Backends/VCS_INFO_get_data_git | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
index 93172fc..1560d7f 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
+++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
@@ -236,15 +236,15 @@ elif [[ -d "${gitdir}/rebase-apply" ]]; then
         done
         if [[ -f "${patchdir}/msg-clean" ]]; then
             subject="${$(< "${patchdir}/msg-clean")[(f)1]}"
-        else
-            local maxlines=10
-            while IFS='\n' read -r;  do
-                if [[ "$REPLY" == "Subject:"* ]]; then
-                    subject=${REPLY/(#s)Subject: /}
+        elif [[ -f "${patchdir}/${(l:4::0:)cur}" ]]; then
+            local maxlines=10 line
+            while IFS= read -r line;  do
+                if [[ "$line" == "Subject:"* ]]; then
+                    subject=${line/(#s)Subject: /}
                     break
                 fi
                 (( --maxlines )) || break
-            done < "${patchdir}/$(printf "%04d" $cur)"
+            done < "${patchdir}/${(l:4::0:)cur}"
         fi
         if [[ -f "${patchdir}/original-commit" ]]; then
             if [[ -n $subject ]]; then
-- 
2.10.2

Re: [PATCH] [RFC] Get subject of current patch in rebase-apply mode

[Daniel Hahler]

[-- Attachment #1.1: Type: text/plain, Size: 494 bytes --]

On 22.11.2016 05:13, Daniel Shahaf wrote:

I've included all your remarks in the fixup commit (just sent), except for:

> One last thing I'd suggest is to consider using VCS_INFO_quilt-patch2subject,
> since it implements rfc822 unfolding of long subject lines.

I think should be applied to the other places then, too - and in the long run being moved into a more general function then.

Thanks for the pointer, but I think it is out of scope for this patch.


Cheers,
Daniel.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] [RFC] Get subject of current patch in rebase-apply mode

[Daniel Shahaf]

Daniel Hahler wrote on Mon, Nov 28, 2016 at 23:59:18 +0100:
> On 22.11.2016 05:13, Daniel Shahaf wrote:
> 
> I've included all your remarks in the fixup commit (just sent), except for:
> 
> > One last thing I'd suggest is to consider using VCS_INFO_quilt-patch2subject,
> > since it implements rfc822 unfolding of long subject lines.
> 
> I think should be applied to the other places then, too - and in the
> long run being moved into a more general function then.
> 

For future reference — what are those "other places"?

> Thanks for the pointer, but I think it is out of scope for this patch.

Fair enough; perfect good enemy-of let-be don't.

Cheers,

Daniel

Re: [PATCH] [RFC] Get subject of current patch in rebase-apply mode

[Daniel Shahaf]

Daniel Hahler wrote on Mon, Nov 28, 2016 at 23:59:18 +0100:
> On 22.11.2016 05:13, Daniel Shahaf wrote:
> > One last thing I'd suggest is to consider using VCS_INFO_quilt-patch2subject,
> > since it implements rfc822 unfolding of long subject lines.
> 
> I think should be applied to the other places then, too - and in the long run being moved into a more general function then.
> 
> Thanks for the pointer, but I think it is out of scope for this patch.

I've gone ahead and factored that helper function out as a vcs_info-wide
helper function in 8891f5a33d26.

Re: [PATCH] [RFC] Get subject of current patch in rebase-apply mode

[Daniel Hahler]

[-- Attachment #1.1: Type: text/plain, Size: 519 bytes --]

>>> One last thing I'd suggest is to consider using VCS_INFO_quilt-patch2subject,
>>> since it implements rfc822 unfolding of long subject lines.
>>
>> I think should be applied to the other places then, too - and in the
>> long run being moved into a more general function then.
> 
> For future reference — what are those "other places"?

The other places (at least one) where a subject gets used.

Thanks for refactoring out in 400600.

I have committed my patch as-is for now.


Cheers,
Daniel.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[PATCH] vcs_info git: rfc822-unfold rebase-apply patch subjects when msg-clean is unavailable.

[Daniel Shahaf]

Example (in this repository):

    git -c merge.merge-changelog.driver=/bin/false rebase --onto=1955cce^^  1955cce^ 1955cce

---
I think this patch covers all the "other places" Daniel referred to.

Daniel

 Functions/VCS_Info/Backends/VCS_INFO_get_data_git | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
index 1560d7f..a92261f 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
+++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
@@ -237,14 +237,11 @@ elif [[ -d "${gitdir}/rebase-apply" ]]; then
         if [[ -f "${patchdir}/msg-clean" ]]; then
             subject="${$(< "${patchdir}/msg-clean")[(f)1]}"
         elif [[ -f "${patchdir}/${(l:4::0:)cur}" ]]; then
-            local maxlines=10 line
-            while IFS= read -r line;  do
-                if [[ "$line" == "Subject:"* ]]; then
-                    subject=${line/(#s)Subject: /}
-                    break
-                fi
-                (( --maxlines )) || break
-            done < "${patchdir}/${(l:4::0:)cur}"
+            () {
+              local REPLY
+              VCS_INFO_patch2subject "${patchdir}/${(l:4::0:)cur}"
+              subject=$REPLY
+            }
         fi
         if [[ -f "${patchdir}/original-commit" ]]; then
             if [[ -n $subject ]]; then
@@ -270,6 +267,7 @@ elif [[ -f "${gitdir}/MERGE_HEAD" ]]; then
     # This is 'git merge --no-commit'
     local -a heads=( ${(@f)"$(<"${gitdir}/MERGE_HEAD")"} )
     local subject;
+    # TODO: maybe read up to the first blank line
     IFS='' read -r subject < "${gitdir}/MERGE_MSG"
     # $subject is the subject line of the would-be commit
     # Maybe we can get the subject lines of MERGE_HEAD's commits cheaply?
@@ -295,6 +293,7 @@ elif [[ -f "${gitdir}/CHERRY_PICK_HEAD" ]]; then
     # ### be "1".  The %u/%c tuple will assume the values [(1,2), (1,1), (1,0)],
     # ### whereas the correct sequence would be [(1,2), (2,1), (3,0)].
     local subject
+    # TODO: maybe read up to the first blank line
     IFS='' read -r subject < "${gitdir}/MERGE_MSG"
     git_patches_applied=( "$(<${gitdir}/CHERRY_PICK_HEAD) ${subject}" )
     if [[ -f "${gitdir}/sequencer/todo" ]]; then