threads / patch / 39567

v2git-rebase--interactive.sh: add config option for custom instruction format

Subject: [PATCH v2] git-rebase--interactive.sh: add config option for custom instruction format

## tl;dr

9 messages between Jun 8, 2015 and Jun 10, 2015. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Michael Rappazzo· Jun 8, 2015, 21:00 UTC · lore
Difference between v1 and v2 of this patch:
    - Fixed indentation from spaces to match the existing style
    - Changed the prepended sha1 from short (%h) to long (%H)
    - Used bash variable default when the config option is not present
Michael Rappazzo (1):
  git-rebase--interactive.sh: add config option for custom instruction
    format
 git-rebase--interactive.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
-- 
2.4.2
Michael Rappazzo· Jun 8, 2015, 21:00 UTC · re: Michael Rappazzo · lore

A config option 'rebase.instructionFormat' can override the default 'oneline' format of the rebase instruction list.

Since the list is parsed using the left, right or boundary mark plus the sha1, they are prepended to the instruction format.

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 git-rebase--interactive.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to git-rebase--interactive.sh +3 −1
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index dc3133f..b92375e 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -977,7 +977,9 @@ else
 	revisions=$onto...$orig_head
 	shortrevisions=$shorthead
 fi
-git rev-list $merges_option --pretty=oneline --reverse --left-right --topo-order \
+format=$(git config --get rebase.instructionFormat)
+# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse
+git rev-list $merges_option --format="%m%H ${format-%s}" --reverse --left-right --topo-order \
 	$revisions ${restrict_revision+^$restrict_revision} | \
 	sed -n "s/^>//p" |
 while read -r sha1 rest

-- 
2.4.2
Junio C Hamano· Jun 8, 2015, 21:29 UTC · re: Michael Rappazzo · lore

Re: [PATCH v2] git-rebase--interactive.sh: add config option for custom instruction format

Michael Rappazzo <rappazzo@gmail.com> writes:
Show 26 quoted lines
> A config option 'rebase.instructionFormat' can override the
> default 'oneline' format of the rebase instruction list.
>
> Since the list is parsed using the left, right or boundary mark plus
> the sha1, they are prepended to the instruction format.
>
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
>  git-rebase--interactive.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index dc3133f..b92375e 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -977,7 +977,9 @@ else
>  	revisions=$onto...$orig_head
>  	shortrevisions=$shorthead
>  fi
> -git rev-list $merges_option --pretty=oneline --reverse --left-right --topo-order \
> +format=$(git config --get rebase.instructionFormat)
> +# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse
> +git rev-list $merges_option --format="%m%H ${format-%s}" --reverse --left-right --topo-order \
>  	$revisions ${restrict_revision+^$restrict_revision} | \
>  	sed -n "s/^>//p" |
>  while read -r sha1 rest
Looks OK, but
 - Needs a patch to Documentation/config.txt and possibly also
   Documentation/git-rebase.txt
 - Needs tests somewhere in t34?? series.

Also I think "git rev-list" line has got way too long. As it is already using backslash-continuation, do not hesitate to fold it further, e.g. 

        git rev-list $merges_option --format="%m%H ${format-%s}" \
                --reverse --left-right --topo-order \
                $revisions ${restrict_revision+^$restrict_revision} | \
                sed -n "s/^>//p" |
	while ...
Thanks.
Johannes Schindelin· Jun 9, 2015, 09:36 UTC · re: Michael Rappazzo · lore

Re: [PATCH v2] git-rebase--interactive.sh: add config option for custom instruction format

Hi,
On 2015-06-08 23:00, Michael Rappazzo wrote:
Show 15 quoted lines
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index dc3133f..b92375e 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -977,7 +977,9 @@ else
>  	revisions=$onto...$orig_head
>  	shortrevisions=$shorthead
>  fi
> -git rev-list $merges_option --pretty=oneline --reverse --left-right
> --topo-order \
> +format=$(git config --get rebase.instructionFormat)
> +# the 'rev-list .. | sed' requires %m to parse; the instruction
> requires %H to parse
> +git rev-list $merges_option --format="%m%H ${format-%s}" --reverse
> --left-right --topo-order \
These two lines are too long (longer than 80 columns)...
Besides, are you sure you don't want to substitute an empty 'rebase.instructionFormat' by '%s'? I would have expected to read `${format:-%s}` (note the colon), but then, this was Junio's suggestion... Junio, what do you think, should we not rather substitute empty values by `%s` as if the config setting was unset?
>  	$revisions ${restrict_revision+^$restrict_revision} | \
>  	sed -n "s/^>//p" |
>  while read -r sha1 rest

Ciao, Johannes

Mike Rappazzo· Jun 9, 2015, 10:26 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2] git-rebase--interactive.sh: add config option for custom instruction format

I see your point, and I'll explore that avenue.

Personally, I like the idea that one could also use the short hash if the custom instruction started with "%h ", but I see the value in leaving the variable blank.

After running the tests with a custom format enabled, I did find that autosquash doesn't work, so I am working to correct that.

On Tue, Jun 9, 2015 at 5:36 AM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 30 quoted lines
> Hi,
>
> On 2015-06-08 23:00, Michael Rappazzo wrote:
>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index dc3133f..b92375e 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -977,7 +977,9 @@ else
>>       revisions=$onto...$orig_head
>>       shortrevisions=$shorthead
>>  fi
>> -git rev-list $merges_option --pretty=oneline --reverse --left-right
>> --topo-order \
>> +format=$(git config --get rebase.instructionFormat)
>> +# the 'rev-list .. | sed' requires %m to parse; the instruction
>> requires %H to parse
>> +git rev-list $merges_option --format="%m%H ${format-%s}" --reverse
>> --left-right --topo-order \
>
> These two lines are too long (longer than 80 columns)...
>
> Besides, are you sure you don't want to substitute an empty 'rebase.instructionFormat' by '%s'? I would have expected to read `${format:-%s}` (note the colon), but then, this was Junio's suggestion... Junio, what do you think, should we not rather substitute empty values by `%s` as if the config setting was unset?
>
>>       $revisions ${restrict_revision+^$restrict_revision} | \
>>       sed -n "s/^>//p" |
>>  while read -r sha1 rest
>
> Ciao,
> Johannes
Junio C Hamano· Jun 9, 2015, 19:23 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2] git-rebase--interactive.sh: add config option for custom instruction format

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> Besides, are you sure you don't want to substitute an empty
> rebase.instructionFormat' by '%s'? I would have expected to read
> ${format:-%s}` (note the colon), but then, this was Junio's
> suggestion...

That was me simply being sloppy myself, expecting people not to copy and paste literally without thinking. Thanks for noticing.

Mike Rappazzo· Jun 9, 2015, 19:38 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] git-rebase--interactive.sh: add config option for custom instruction format

I have since reworked this script to support the short hash in the custom format as a special case:

-git rev-list $merges_option --pretty=oneline --reverse --left-right
--topo-order \
+format=$(git config --get rebase.instructionFormat)
+no_format=$?
+if test ${no_format} -ne 0
+then
+ format="%H %s"
+elif test "${format:0:3}" != "%H " && test "${format:0:3}" != "%h "
+then
+ format="%H ${format}"
+fi
+# the 'rev-list .. | sed' requires %m to parse; the instruction
requires %H to parse
+git rev-list $merges_option --format="%m${format}" \
+ --reverse --left-right --topo-order \

I also use the $no_format variable later on in the autosquash re-ordering, and have the tests passing. I want to add some new tests on the custom format, and will send a new patch when that is complete.

On Tue, Jun 9, 2015 at 3:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> Besides, are you sure you don't want to substitute an empty
>> rebase.instructionFormat' by '%s'? I would have expected to read
>> ${format:-%s}` (note the colon), but then, this was Junio's
>> suggestion...
>
> That was me simply being sloppy myself, expecting people not to copy
> and paste literally without thinking.  Thanks for noticing.
Junio C Hamano· Jun 10, 2015, 00:44 UTC · re: Mike Rappazzo · lore

Re: [PATCH v2] git-rebase--interactive.sh: add config option for custom instruction format

Mike Rappazzo <rappazzo@gmail.com> writes:
> I have since reworked this script to support the short hash in the
> custom format as a special case:

I thought that we always give short form when presenting it to the end user to edit, but for internal bookkeeping purposes we make sure that we use the full SHA-1, so that new objects created during the rebase session will not cause "used to be unique but not anymore" ambiguity in the commit object names in the instruction sheet.

I have been assuming that the "rev-list" we have been mucking with was to prepare the latter, the internal bookkeeping data, which must always spell 40-hex (that is why the default "oneline" is not "--oneline" but "--pretty=oneline).

Why is it necessary to make %m%H part customizable in the first place?

Puzzled....
Show 9 quoted lines
>
> -git rev-list $merges_option --pretty=oneline --reverse --left-right
> --topo-order \
> +format=$(git config --get rebase.instructionFormat)
> +no_format=$?
> +if test ${no_format} -ne 0
> +then
> + format="%H %s"
> +elif test "${format:0:3}" != "%H " && test "${format:0:3}" != "%h "
Do not use bash-ism "substring".
Show 22 quoted lines
> +then
> + format="%H ${format}"
> +fi
> +# the 'rev-list .. | sed' requires %m to parse; the instruction
> requires %H to parse
> +git rev-list $merges_option --format="%m${format}" \
> + --reverse --left-right --topo-order \
>
> I also use the $no_format variable later on in the autosquash
> re-ordering, and have the tests passing.  I want to add some new tests
> on the custom format, and will send a new patch when that is complete.
>
> On Tue, Jun 9, 2015 at 3:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>>> Besides, are you sure you don't want to substitute an empty
>>> rebase.instructionFormat' by '%s'? I would have expected to read
>>> ${format:-%s}` (note the colon), but then, this was Junio's
>>> suggestion...
>>
>> That was me simply being sloppy myself, expecting people not to copy
>> and paste literally without thinking.  Thanks for noticing.
Junio C Hamano· Jun 8, 2015, 22:17 UTC · re: Michael Rappazzo · lore

Re: [PATCH v2] git-rebase--interactive.sh: add config option for custom instruction format

Michael Rappazzo <rappazzo@gmail.com> writes:
Show 12 quoted lines
> Difference between v1 and v2 of this patch:
>
>     - Fixed indentation from spaces to match the existing style
>     - Changed the prepended sha1 from short (%h) to long (%H)
>     - Used bash variable default when the config option is not present
>
> Michael Rappazzo (1):
>   git-rebase--interactive.sh: add config option for custom instruction
>     format
>
>  git-rebase--interactive.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
Does this pass tests?  I see many failures including t3415.

← back to recent threads