Re: [PATCH] dim: allow to add link to the current HEAD
On Mon, 18 Nov 2019, Lucas De Marchi wrote:
> On Mon, Nov 18, 2019 at 12:16:19PM -0800, Rodrigo Vivi wrote:
>> On Nov 18, 2019, at 12:14 PM, Lucas De Marchi
>> <[1]lucas.demar...@intel.com> wrote:
>> On Mon, Nov 18, 2019 at 04:57:34PM +0200, Jani Nikula wrote:
>>
>> On Fri, 15 Nov 2019, Lucas De Marchi <[2]lucas.demar...@intel.com>
>> wrote:
>>
>> Add method add-link-head that applies the link to head without any
>> additional check. This is useful either to reduce typing or to add a
>> link while in the middle of an interactive rebase.
>>
>> I see the patch does what it says on the box. However:
>>
>> 1) Most commands expect you to supply the branch name so you know what
>> you're doing. There's tab completion to aid you, but you need to see
>> what branch you're operating on. This has been in place since there
>> was only one gun and two feet; now there are quite a few more people
>> with opportunities to shoot themselves in the foot.
>>
>> 2) I think we should discourage pushing locally rebased
>> patches. Recommend only pushing patches that apply cleanly. I'd go as
>> far as removing the add-link* subcommands altogether.
>>
>> Not sure how this is helping or avoiding that. "push" was never
>> mentioned here and it's not what this is about.
>>
>> This was useful for me while rebasing a "feature branch"
>> to prep a new revision. It's useful for the future developer reading
>> the patch to have a link to the revision where the patch was discussed,
>> rather than a link to a last revision where all the reviews were already
>> collected.
>>
>> Very good point. But we need to do in a way that we don't loose the
>> last link. The last one is the proof that it passed CI.
>
> If you later apply the patch using, say, "dim aq" a second link will be
> added, the link that is already there won't be replaced.
>
> I think the link to the review is much more useful than the link where
> CI reported a pass. Either because of CI flip flops or because a patch
> that passed CI being applied on top of 10 additional commits doesn't
> prove anything.
I think the Link: tag should point at the patch on the mailing list that
was applied. You should only apply patches that passed CI. This is
unambiguous and does not leave any subjective wiggle room. It provides
the audit trail (for want of a better expression) we want. And more
often than review, I'm actually after the CI results of a commit.
> Anyway, if people just put the automatically-added "Link" when you
> download a patch from patchwork, I will just stop having more work on my
> end and follow that.
I don't download patches from patchwork, I apply them from my mailbox. I
apply the patch that passed CI.
BR,
Jani.
>
> Lucas De Marchi
>
>>
>> Also any patch being applied to any branch are in other words being
>> rebased since your branch probably changed from the time you wrote it
>> and the time it was applied, even if it applies cleanly (your rebase
>> might have being clean, too).
>>
>> Lucas De Marchi
>>
>> BR,
>> Jani.
>>
>> Signed-off-by: Lucas De Marchi <[3]lucas.demar...@intel.com>
>> ---
>> dim | 23 +++
>> 1 file changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/dim b/dim
>> index 1c2da80..3aad25f 100755
>> --- a/dim
>> +++ b/dim
>> @@ -1255,17 +1255,11 @@ function dim_rebase
>> fi
>> }
>>
>> -function dim_add_link
>> +function dim_add_link_head
>> {
>> - local branch file message_id
>> + local file message_id
>>
>> - branch=${1:?$usage}
>> - shift
>> file=$(mktemp)
>> -
>> - assert_branch $branch
>> - assert_repo_clean
>> -
>> cat > $file
>>
>> message_id=$(message_get_id $file)
>> @@ -1279,6 +1273,19 @@ function dim_add_link
>> fi
>> }
>>
>> +function dim_add_link
>> +{
>> + local branch file message_id
>> +
>> + branch=${1:?$usage}
>> + shift
>> +
>> + assert_branch $branch
>> + assert_repo_clean
>> +
>> + dim_add_link_head
>> +}
>> +
>> function dim_add_link_queued
>> {
>> dim_add_link drm-intel-next-queued "$@"
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center
>>
>> ___
>> dim-tools mailing list
>> [4]dim-tools@lists.freedesktop.org
>> [5]https://lists.freedesktop.org/mailman/listinfo/dim-tools
>>
>>References
>>
>> Visible links
>> 1. mailto:lucas.demar...@intel.com
>> 2. mailto:lucas.demar...@intel.com
>> 3. mailto:lucas.demar...@intel.com
>> 4. mailto:dim-tools@lists.freedesktop.org
>> 5.
Re: [PATCH] dim: allow to add link to the current HEAD
On Mon, Nov 18, 2019 at 12:16:19PM -0800, Rodrigo Vivi wrote:
On Nov 18, 2019, at 12:14 PM, Lucas De Marchi
<[1]lucas.demar...@intel.com> wrote:
On Mon, Nov 18, 2019 at 04:57:34PM +0200, Jani Nikula wrote:
On Fri, 15 Nov 2019, Lucas De Marchi <[2]lucas.demar...@intel.com>
wrote:
Add method add-link-head that applies the link to head without any
additional check. This is useful either to reduce typing or to add a
link while in the middle of an interactive rebase.
I see the patch does what it says on the box. However:
1) Most commands expect you to supply the branch name so you know what
you're doing. There's tab completion to aid you, but you need to see
what branch you're operating on. This has been in place since there
was only one gun and two feet; now there are quite a few more people
with opportunities to shoot themselves in the foot.
2) I think we should discourage pushing locally rebased
patches. Recommend only pushing patches that apply cleanly. I'd go as
far as removing the add-link* subcommands altogether.
Not sure how this is helping or avoiding that. "push" was never
mentioned here and it's not what this is about.
This was useful for me while rebasing a "feature branch"
to prep a new revision. It's useful for the future developer reading
the patch to have a link to the revision where the patch was discussed,
rather than a link to a last revision where all the reviews were already
collected.
Very good point. But we need to do in a way that we don't loose the
last link. The last one is the proof that it passed CI.
If you later apply the patch using, say, "dim aq" a second link will be
added, the link that is already there won't be replaced.
I think the link to the review is much more useful than the link where
CI reported a pass. Either because of CI flip flops or because a patch
that passed CI being applied on top of 10 additional commits doesn't
prove anything.
Anyway, if people just put the automatically-added "Link" when you
download a patch from patchwork, I will just stop having more work on my
end and follow that.
Lucas De Marchi
Also any patch being applied to any branch are in other words being
rebased since your branch probably changed from the time you wrote it
and the time it was applied, even if it applies cleanly (your rebase
might have being clean, too).
Lucas De Marchi
BR,
Jani.
Signed-off-by: Lucas De Marchi <[3]lucas.demar...@intel.com>
---
dim | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/dim b/dim
index 1c2da80..3aad25f 100755
--- a/dim
+++ b/dim
@@ -1255,17 +1255,11 @@ function dim_rebase
fi
}
-function dim_add_link
+function dim_add_link_head
{
- local branch file message_id
+ local file message_id
- branch=${1:?$usage}
- shift
file=$(mktemp)
-
- assert_branch $branch
- assert_repo_clean
-
cat > $file
message_id=$(message_get_id $file)
@@ -1279,6 +1273,19 @@ function dim_add_link
fi
}
+function dim_add_link
+{
+ local branch file message_id
+
+ branch=${1:?$usage}
+ shift
+
+ assert_branch $branch
+ assert_repo_clean
+
+ dim_add_link_head
+}
+
function dim_add_link_queued
{
dim_add_link drm-intel-next-queued "$@"
--
Jani Nikula, Intel Open Source Graphics Center
___
dim-tools mailing list
[4]dim-tools@lists.freedesktop.org
[5]https://lists.freedesktop.org/mailman/listinfo/dim-tools
References
Visible links
1. mailto:lucas.demar...@intel.com
2. mailto:lucas.demar...@intel.com
3. mailto:lucas.demar...@intel.com
4. mailto:dim-tools@lists.freedesktop.org
5. https://lists.freedesktop.org/mailman/listinfo/dim-tools
___
dim-tools mailing list
dim-tools@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dim-tools
Re: [PATCH] dim: allow to add link to the current HEAD
On Nov 18, 2019, at 12:14 PM, Lucas De Marchi
mailto:lucas.demar...@intel.com>> wrote:
On Mon, Nov 18, 2019 at 04:57:34PM +0200, Jani Nikula wrote:
On Fri, 15 Nov 2019, Lucas De Marchi
mailto:lucas.demar...@intel.com>> wrote:
Add method add-link-head that applies the link to head without any
additional check. This is useful either to reduce typing or to add a
link while in the middle of an interactive rebase.
I see the patch does what it says on the box. However:
1) Most commands expect you to supply the branch name so you know what
you're doing. There's tab completion to aid you, but you need to see
what branch you're operating on. This has been in place since there
was only one gun and two feet; now there are quite a few more people
with opportunities to shoot themselves in the foot.
2) I think we should discourage pushing locally rebased
patches. Recommend only pushing patches that apply cleanly. I'd go as
far as removing the add-link* subcommands altogether.
Not sure how this is helping or avoiding that. "push" was never
mentioned here and it's not what this is about.
This was useful for me while rebasing a "feature branch"
to prep a new revision. It's useful for the future developer reading
the patch to have a link to the revision where the patch was discussed,
rather than a link to a last revision where all the reviews were already
collected.
Very good point. But we need to do in a way that we don't loose the
last link. The last one is the proof that it passed CI.
Also any patch being applied to any branch are in other words being
rebased since your branch probably changed from the time you wrote it
and the time it was applied, even if it applies cleanly (your rebase
might have being clean, too).
Lucas De Marchi
BR,
Jani.
Signed-off-by: Lucas De Marchi
mailto:lucas.demar...@intel.com>>
---
dim | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/dim b/dim
index 1c2da80..3aad25f 100755
--- a/dim
+++ b/dim
@@ -1255,17 +1255,11 @@ function dim_rebase
fi
}
-function dim_add_link
+function dim_add_link_head
{
- local branch file message_id
+ local file message_id
- branch=${1:?$usage}
- shift
file=$(mktemp)
-
- assert_branch $branch
- assert_repo_clean
-
cat > $file
message_id=$(message_get_id $file)
@@ -1279,6 +1273,19 @@ function dim_add_link
fi
}
+function dim_add_link
+{
+ local branch file message_id
+
+ branch=${1:?$usage}
+ shift
+
+ assert_branch $branch
+ assert_repo_clean
+
+ dim_add_link_head
+}
+
function dim_add_link_queued
{
dim_add_link drm-intel-next-queued "$@"
--
Jani Nikula, Intel Open Source Graphics Center
___
dim-tools mailing list
dim-tools@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dim-tools
___
dim-tools mailing list
dim-tools@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dim-tools
Re: [PATCH] dim: allow to add link to the current HEAD
On Mon, Nov 18, 2019 at 04:57:34PM +0200, Jani Nikula wrote:
On Fri, 15 Nov 2019, Lucas De Marchi wrote:
Add method add-link-head that applies the link to head without any
additional check. This is useful either to reduce typing or to add a
link while in the middle of an interactive rebase.
I see the patch does what it says on the box. However:
1) Most commands expect you to supply the branch name so you know what
you're doing. There's tab completion to aid you, but you need to see
what branch you're operating on. This has been in place since there
was only one gun and two feet; now there are quite a few more people
with opportunities to shoot themselves in the foot.
2) I think we should discourage pushing locally rebased
patches. Recommend only pushing patches that apply cleanly. I'd go as
far as removing the add-link* subcommands altogether.
Not sure how this is helping or avoiding that. "push" was never
mentioned here and it's not what this is about.
This was useful for me while rebasing a "feature branch"
to prep a new revision. It's useful for the future developer reading
the patch to have a link to the revision where the patch was discussed,
rather than a link to a last revision where all the reviews were already
collected.
Also any patch being applied to any branch are in other words being
rebased since your branch probably changed from the time you wrote it
and the time it was applied, even if it applies cleanly (your rebase
might have being clean, too).
Lucas De Marchi
BR,
Jani.
Signed-off-by: Lucas De Marchi
---
dim | 23 +++
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/dim b/dim
index 1c2da80..3aad25f 100755
--- a/dim
+++ b/dim
@@ -1255,17 +1255,11 @@ function dim_rebase
fi
}
-function dim_add_link
+function dim_add_link_head
{
- local branch file message_id
+ local file message_id
- branch=${1:?$usage}
- shift
file=$(mktemp)
-
- assert_branch $branch
- assert_repo_clean
-
cat > $file
message_id=$(message_get_id $file)
@@ -1279,6 +1273,19 @@ function dim_add_link
fi
}
+function dim_add_link
+{
+ local branch file message_id
+
+ branch=${1:?$usage}
+ shift
+
+ assert_branch $branch
+ assert_repo_clean
+
+ dim_add_link_head
+}
+
function dim_add_link_queued
{
dim_add_link drm-intel-next-queued "$@"
--
Jani Nikula, Intel Open Source Graphics Center
___
dim-tools mailing list
dim-tools@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dim-tools
