| Message ID | 20210423194230.1388945-15-lukeshu@lukeshu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | subtree: clean up, improve UX | expand |
On Fri, 23 Apr 2021 13:42:14 -0600,
Luke Shumaker wrote:
> "Ignore space change" is probably helpful when viewing this diff.
For mailing-list reading, the `git show -w` is:
---
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 9ca498f81c..4503564f7e 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -852,16 +852,6 @@ cmd_merge () {
rev="$new"
fi
- version=$(git version)
- if test "$version" \< "git version 1.7"
- then
- if test -n "$message"
- then
- git merge -s subtree --message="$message" "$rev"
- else
- git merge -s subtree "$rev"
- fi
- else
if test -n "$message"
then
git merge -Xsubtree="$prefix" \
@@ -869,7 +859,6 @@ cmd_merge () {
else
git merge -Xsubtree="$prefix" $rev
fi
- fi
}
cmd_pull () {
On Fri, Apr 23, 2021 at 3:43 PM Luke Shumaker <lukeshu@lukeshu.com> wrote: > That was nice to have when git-subtree lived out-of-tree. But now that > it lives in git.git, it's not nescessary to keep around. s/nescessary/necessary/ > Signed-off-by: Luke Shumaker <lukeshu@datawire.io> Is there a higher reason for this change aside from "let's do it because we can"? For instance, are subsequent changes going to take advantage of features only present in more recent Git versions which would be painful or impossible to support with the older Git? The downside of this change is that, while git-subtree may live in git.git, it's still just "contrib", so people might grab git-subtree from a modern git.git but then end up using it with an older Git installation. That's not to say that doing such a thing is guaranteed to work anyhow, but we don't need to make it harder on people if there isn't a good reason (hence my question about whether subsequent changes will actually take advantage of newer Git features).
On Fri, 23 Apr 2021 14:31:46 -0600, Eric Sunshine wrote: > > On Fri, Apr 23, 2021 at 3:43 PM Luke Shumaker <lukeshu@lukeshu.com> wrote: > > That was nice to have when git-subtree lived out-of-tree. But now that > > it lives in git.git, it's not nescessary to keep around. > > s/nescessary/necessary/ Ack. > > Signed-off-by: Luke Shumaker <lukeshu@datawire.io> > > Is there a higher reason for this change aside from "let's do it > because we can"? For instance, are subsequent changes going to take > advantage of features only present in more recent Git versions which > would be painful or impossible to support with the older Git? > > The downside of this change is that, while git-subtree may live in > git.git, it's still just "contrib", so people might grab git-subtree > from a modern git.git but then end up using it with an older Git > installation. That's not to say that doing such a thing is guaranteed > to work anyhow, but we don't need to make it harder on people if there > isn't a good reason (hence my question about whether subsequent > changes will actually take advantage of newer Git features). With the goal of making the whole thing easier to hack on, this just seemed like an easy (if small) piece of fat to trim. I guess I should include here that my bias is: With the 'git' package that Parabola inherits from Arch Linux, you install 'git', you get a working 'git subtree'. If you want 'git send-email' to work, you also need to install several other Perl packages. Like, it might live in the "contrib" directory, but it's de-facto at least as much a "part of" Git as send-email is. So that's the mindset I started from. It looks like the latest macOS supports me on that, but other distros like Fedora don't (Fedora has a separate 'git-subtree' package). If I take a step back, I realize that mindset is flawed, but that's where I started from. I don't think any of the other work depends on this (I think the only commit that will conflict if we drop it is the "rename a some variables" commit in this patchset). It's very possible that something else I do relies on newer git features (I'm not testing against older git), but that's not something I did intentionally. I just figured this would be a welcome piece of cleanup. If that's not the case, I don't have a problem dropping it.
On Fri, Apr 23, 2021 at 7:28 PM Luke Shumaker <lukeshu@lukeshu.com> wrote: > On Fri, 23 Apr 2021 14:31:46 -0600, Eric Sunshine wrote: > > Is there a higher reason for this change aside from "let's do it > > because we can"? For instance, are subsequent changes going to take > > advantage of features only present in more recent Git versions which > > would be painful or impossible to support with the older Git? > > > > The downside of this change is that, while git-subtree may live in > > git.git, it's still just "contrib", so people might grab git-subtree > > from a modern git.git but then end up using it with an older Git > > installation. That's not to say that doing such a thing is guaranteed > > to work anyhow, but we don't need to make it harder on people if there > > isn't a good reason (hence my question about whether subsequent > > changes will actually take advantage of newer Git features). > > With the goal of making the whole thing easier to hack on, this just > seemed like an easy (if small) piece of fat to trim. > > I guess I should include here that my bias is: With the 'git' package > that Parabola inherits from Arch Linux, you install 'git', you get a > working 'git subtree'. If you want 'git send-email' to work, you also > need to install several other Perl packages. Like, it might live in > the "contrib" directory, but it's de-facto at least as much a "part > of" Git as send-email is. > > So that's the mindset I started from. It looks like the latest macOS > supports me on that, but other distros like Fedora don't (Fedora has a > separate 'git-subtree' package). If I take a step back, I realize > that mindset is flawed, but that's where I started from. > > I don't think any of the other work depends on this (I think the only > commit that will conflict if we drop it is the "rename a some > variables" commit in this patchset). It's very possible that > something else I do relies on newer git features (I'm not testing > against older git), but that's not something I did intentionally. > > I just figured this would be a welcome piece of cleanup. If that's > not the case, I don't have a problem dropping it. As a person who does not and has never used git-subtree, I don't have a strong opinion, and any git-subtree opinion I might have wouldn't carry any weight. I do have a bit of reflexive negative reaction to such removals in general if there's no clear and strong benefit, perhaps because my daily-use computers are old (perhaps ancient, as in 10-25 years old), so I am regularly stung by support being dropped by packages I use. That's why I was asking if later patches would take advantage of some newer Git feature. Anyhow, I wasn't specifically asking for the patch to be dropped. As a reviewer I rather wanted to better understand the reason for the change beyond the somewhat hand-wavy "git-subtree now lives in-tree". If you happen to re-roll, perhaps you can expand the commit message a bit with something more concrete (for instance, how some packagers include git-subtree by default) to better sell the patch to reviewers who actually have investment in git-subtree (of which I am not one). Considering how old Git <1.7 is, it's quite likely that no current git-subtree users/reviewers would care about dropping support for such old versions.
On Fri, 23 Apr 2021 17:50:11 -0600, Eric Sunshine wrote: > > On Fri, Apr 23, 2021 at 7:28 PM Luke Shumaker <lukeshu@lukeshu.com> wrote: > > On Fri, 23 Apr 2021 14:31:46 -0600, Eric Sunshine wrote: > > > Is there a higher reason for this change aside from "let's do it > > > because we can"? For instance, are subsequent changes going to take > > > advantage of features only present in more recent Git versions which > > > would be painful or impossible to support with the older Git? > > > > > > The downside of this change is that, while git-subtree may live in > > > git.git, it's still just "contrib", so people might grab git-subtree > > > from a modern git.git but then end up using it with an older Git > > > installation. That's not to say that doing such a thing is guaranteed > > > to work anyhow, but we don't need to make it harder on people if there > > > isn't a good reason (hence my question about whether subsequent > > > changes will actually take advantage of newer Git features). > > > > With the goal of making the whole thing easier to hack on, this just > > seemed like an easy (if small) piece of fat to trim. > > > > I guess I should include here that my bias is: With the 'git' package > > that Parabola inherits from Arch Linux, you install 'git', you get a > > working 'git subtree'. If you want 'git send-email' to work, you also > > need to install several other Perl packages. Like, it might live in > > the "contrib" directory, but it's de-facto at least as much a "part > > of" Git as send-email is. > > > > So that's the mindset I started from. It looks like the latest macOS > > supports me on that, but other distros like Fedora don't (Fedora has a > > separate 'git-subtree' package). If I take a step back, I realize > > that mindset is flawed, but that's where I started from. > > > > I don't think any of the other work depends on this (I think the only > > commit that will conflict if we drop it is the "rename a some > > variables" commit in this patchset). It's very possible that > > something else I do relies on newer git features (I'm not testing > > against older git), but that's not something I did intentionally. > > > > I just figured this would be a welcome piece of cleanup. If that's > > not the case, I don't have a problem dropping it. > > As a person who does not and has never used git-subtree, I don't have > a strong opinion, and any git-subtree opinion I might have wouldn't > carry any weight. I do have a bit of reflexive negative reaction to > such removals in general if there's no clear and strong benefit, > perhaps because my daily-use computers are old (perhaps ancient, as in > 10-25 years old), so I am regularly stung by support being dropped by > packages I use. That's why I was asking if later patches would take > advantage of some newer Git feature. I'm sending this from my work laptop, which is a little newer (2015), but my personal laptop (2007) is sitting on the end-table. So I commiserate. I'm a modern-software on old-hardware kind of guy though, so I don't mind dropping support for old software versions. > Anyhow, I wasn't specifically asking for the patch to be dropped. As a > reviewer I rather wanted to better understand the reason for the > change beyond the somewhat hand-wavy "git-subtree now lives in-tree". > If you happen to re-roll, perhaps you can expand the commit message a > bit with something more concrete (for instance, how some packagers > include git-subtree by default) to better sell the patch to reviewers > who actually have investment in git-subtree (of which I am not one). > Considering how old Git <1.7 is, it's quite likely that no current > git-subtree users/reviewers would care about dropping support for such > old versions. I'll expand the commit message.
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index 9ca498f81c..4503564f7e 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -852,23 +852,12 @@ cmd_merge () { rev="$new" fi - version=$(git version) - if test "$version" \< "git version 1.7" + if test -n "$message" then - if test -n "$message" - then - git merge -s subtree --message="$message" "$rev" - else - git merge -s subtree "$rev" - fi + git merge -Xsubtree="$prefix" \ + --message="$message" "$rev" else - if test -n "$message" - then - git merge -Xsubtree="$prefix" \ - --message="$message" "$rev" - else - git merge -Xsubtree="$prefix" $rev - fi + git merge -Xsubtree="$prefix" $rev fi }