| Message ID | 20220211163627.598166-4-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/4] log: fix memory leak if --graph is passed multiple times | expand |
Alex Henrie <alexhenrie24@gmail.com> writes: > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> > --- > v3: no changes > --- > gitk-git/gitk | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) Please base a patch on gitk part to base on Paul's tree, not mine, meaning that the first few lines of diff should begin like so: diff --git a/gitk b/gitk index ... --- a/gitk +++ b/gitk and not as part of the series. What the first two patches want to do is a good thing regardless, so I'll take a deeper look at them and queue them. I am very skeptical to log.graph=yes/no configuration for obvious reasons that setting such a variable *will* break existing tools and users. It is not even "it might break but we don't know until we try", as this patch loudly demonstrates. Thanks. > diff --git a/gitk-git/gitk b/gitk-git/gitk > index 23d9dd1fe0..24099ce0b8 100755 > --- a/gitk-git/gitk > +++ b/gitk-git/gitk > @@ -411,8 +411,8 @@ proc start_rev_list {view} { > } > > if {[catch { > - set fd [open [concat | git log --no-color -z --pretty=raw $show_notes \ > - --parents --boundary $args "--" $files] r] > + set fd [open [concat | git log --no-color --no-graph -z --pretty=raw \ > + $show_notes --parents --boundary $args "--" $files] r] > } err]} { > error_popup "[mc "Error executing git log:"] $err" > return 0 > @@ -559,8 +559,9 @@ proc updatecommits {} { > set args $vorigargs($view) > } > if {[catch { > - set fd [open [concat | git log --no-color -z --pretty=raw $show_notes \ > - --parents --boundary $args "--" $vfilelimit($view)] r] > + set fd [open [concat | git log --no-color --no-graph -z --pretty=raw > + $show_notes --parents --boundary $args "--" > + $vfilelimit($view)] r] > } err]} { > error_popup "[mc "Error executing git log:"] $err" > return
On Fri, Feb 11, 2022 at 11:12 AM Junio C Hamano <gitster@pobox.com> wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > > gitk-git/gitk | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > Please base a patch on gitk part to base on Paul's tree, not mine, > meaning that the first few lines of diff should begin like so: > > diff --git a/gitk b/gitk > index ... > --- a/gitk > +++ b/gitk > > and not as part of the series. Okay. I'll hang onto this patch until the previous one in the series is accepted. > What the first two patches want to do is a good thing regardless, so > I'll take a deeper look at them and queue them. I am very skeptical > to log.graph=yes/no configuration for obvious reasons that setting > such a variable *will* break existing tools and users. It is not > even "it might break but we don't know until we try", as this patch > loudly demonstrates. What if we make log.graph=true also require feature.experimental=true? The log.graph option would really be a useful feature for people who use Git exclusively from the CLI without any external tools. It seems that the main challenge is how to give others time to adjust. Thanks for all the feedback, -Alex
Alex Henrie <alexhenrie24@gmail.com> writes: > What if we make log.graph=true also require feature.experimental=true? No. feature.experimental is to give people an opt-in opportunity for features that we are considering to enable by default. > The log.graph option would really be a useful feature for people who > use Git exclusively from the CLI without any external tools. It seems > that the main challenge is how to give others time to adjust. Those who want to see log by default must need to twaek their configuration. Instead of doing "git config log.graph true" and breaking tools, they can do "git config alias.mylog 'log --graph'" with the same ease, without breaking anything. So...
Junio C Hamano <gitster@pobox.com> writes: > Alex Henrie <alexhenrie24@gmail.com> writes: > >> What if we make log.graph=true also require feature.experimental=true? > > No. feature.experimental is to give people an opt-in opportunity > for features that we are considering to enable by default. > >> The log.graph option would really be a useful feature for people who >> use Git exclusively from the CLI without any external tools. It seems >> that the main challenge is how to give others time to adjust. Let me clarify the first point by stating it a bit differently. feature.experimental is all about this: We have an idea for this new feature. We made it useful, and also made it not to regress the end-user experience for those who do not need the new feature, to the best of our ability. But there may be use cases we failed to consider while doing so. So let's ask early adopters, who may use Git in contexts that are very different from ours, to try testing it out in their daily use, to see if there are unexpected glitches. You do not have to argue how the --graph feature may be useful for character terminal users. We already know it is, otherwise we wouldn't have added it in the first place. And arguing how --graph feature is useful does not help prove anything, when at the issue is if it is a good idea to allow the log.graph configuration variable to affect (unfortunate) scripts people wrote around "git log", instead of using plumbing commands, negatively. We already know it will hurt to force everybody to update their script to explicitly pass --no-graph on the command line. This series hasn't done any of the "not to regress to the best of our ability" part. If there were an agreement on the general direction to _forbid_ use of "git log" in scripts, which would require coordinated efforts to help people migrate over time, e.g. - improve plumbing by adding features that people piled only on "git log" without allowing plumbing users the same over time. - perhaps an automated way to convert scripts that use "git log" to instead use "git log --no-graph" to help script writers migrate away from "git log", adding log.graph configuration variable may become very a good idea. But without such effort starting at the same time and gaining consensus (or already underway), just adding such a variable to break existing scripts would not be a good idea worth asking the early adopters to test. We already know it would break scripts.
On Fri, Feb 11, 2022 at 12:20 PM Junio C Hamano <gitster@pobox.com> wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > > What if we make log.graph=true also require feature.experimental=true? > > No. feature.experimental is to give people an opt-in opportunity > for features that we are considering to enable by default. > > > The log.graph option would really be a useful feature for people who > > use Git exclusively from the CLI without any external tools. It seems > > that the main challenge is how to give others time to adjust. > > Those who want to see log by default must need to twaek their > configuration. Instead of doing "git config log.graph true" and > breaking tools, they can do "git config alias.mylog 'log --graph'" > with the same ease, without breaking anything. > > So... Yeah, that's not a bad solution. I actually have `git graph` aliased to `git log --graph --abbrev-commit --pretty=oneline` for this purpose. If a lot of people are doing that, maybe a new command should be added to Git itself. It seems like there's not much demand for that at the moment though. -Alex
On Fri, Feb 11, 2022 at 1:00 PM Junio C Hamano <gitster@pobox.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > Alex Henrie <alexhenrie24@gmail.com> writes: > > > >> What if we make log.graph=true also require feature.experimental=true? > > > > No. feature.experimental is to give people an opt-in opportunity > > for features that we are considering to enable by default. > > > >> The log.graph option would really be a useful feature for people who > >> use Git exclusively from the CLI without any external tools. It seems > >> that the main challenge is how to give others time to adjust. > > Let me clarify the first point by stating it a bit differently. > > feature.experimental is all about this: > > We have an idea for this new feature. We made it useful, and > also made it not to regress the end-user experience for those > who do not need the new feature, to the best of our ability. > But there may be use cases we failed to consider while doing > so. So let's ask early adopters, who may use Git in contexts > that are very different from ours, to try testing it out in > their daily use, to see if there are unexpected glitches. > > You do not have to argue how the --graph feature may be useful for > character terminal users. We already know it is, otherwise we > wouldn't have added it in the first place. > > And arguing how --graph feature is useful does not help prove > anything, when at the issue is if it is a good idea to allow the > log.graph configuration variable to affect (unfortunate) scripts > people wrote around "git log", instead of using plumbing commands, > negatively. We already know it will hurt to force everybody to > update their script to explicitly pass --no-graph on the command > line. This series hasn't done any of the "not to regress to the > best of our ability" part. > > If there were an agreement on the general direction to _forbid_ use > of "git log" in scripts, which would require coordinated efforts to > help people migrate over time, e.g. > > - improve plumbing by adding features that people piled only on > "git log" without allowing plumbing users the same over time. > > - perhaps an automated way to convert scripts that use "git log" to > instead use "git log --no-graph" > > to help script writers migrate away from "git log", adding log.graph > configuration variable may become very a good idea. > > But without such effort starting at the same time and gaining > consensus (or already underway), just adding such a variable to > break existing scripts would not be a good idea worth asking the > early adopters to test. We already know it would break scripts. Okay. Thanks for the explanation! -Alex
diff --git a/gitk-git/gitk b/gitk-git/gitk index 23d9dd1fe0..24099ce0b8 100755 --- a/gitk-git/gitk +++ b/gitk-git/gitk @@ -411,8 +411,8 @@ proc start_rev_list {view} { } if {[catch { - set fd [open [concat | git log --no-color -z --pretty=raw $show_notes \ - --parents --boundary $args "--" $files] r] + set fd [open [concat | git log --no-color --no-graph -z --pretty=raw \ + $show_notes --parents --boundary $args "--" $files] r] } err]} { error_popup "[mc "Error executing git log:"] $err" return 0 @@ -559,8 +559,9 @@ proc updatecommits {} { set args $vorigargs($view) } if {[catch { - set fd [open [concat | git log --no-color -z --pretty=raw $show_notes \ - --parents --boundary $args "--" $vfilelimit($view)] r] + set fd [open [concat | git log --no-color --no-graph -z --pretty=raw + $show_notes --parents --boundary $args "--" + $vfilelimit($view)] r] } err]} { error_popup "[mc "Error executing git log:"] $err" return
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- v3: no changes --- gitk-git/gitk | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)