* Re: [PATCH v2] bugreport: reject positional arguments
From: Dragan Simic @ 2023-10-26 3:52 UTC (permalink / raw)
To: Eric Sunshine; +Cc: emilyshaffer, git, Emily Shaffer, Sheik
In-Reply-To: <CAPig+cT4G9vdu+se9Fbbs0TRCyPoAYFgVtkSwph_U=sWf-kQ9g@mail.gmail.com>
On 2023-10-26 05:43, Eric Sunshine wrote:
> On Wed, Oct 25, 2023 at 8:55 PM <emilyshaffer@google.com> wrote:
>> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
>> @@ -126,6 +126,12 @@ int cmd_bugreport(int argc, const char **argv,
>> const char *prefix)
>> + if (argc) {
>> + if (argv[0])
>> + error(_("unknown argument `%s'"), argv[0]);
>> + usage(bugreport_usage[0]);
>> + }
>
> Can it actually happen that argc is non-zero but argv[0] is NULL? (I
> don't have parse-options in front of me to check.) If not, then the
> extra `if (argv[0])` conditional may confuse future readers.
According to https://stackoverflow.com/a/2794171/22330192 it can't, but
argv[0] can be a zero-length string.
^ permalink raw reply
* Re: [PATCH v2] bugreport: reject positional arguments
From: Eric Sunshine @ 2023-10-26 3:43 UTC (permalink / raw)
To: emilyshaffer; +Cc: git, Emily Shaffer, Sheik
In-Reply-To: <20231026005542.872301-1-nasamuffin@google.com>
On Wed, Oct 25, 2023 at 8:55 PM <emilyshaffer@google.com> wrote:
> git-bugreport already rejected unrecognized flag arguments, like
> `--diaggnose`, but this doesn't help if the user's mistake was to forget
> the `--` in front of the argument. This can result in a user's intended
> argument not being parsed with no indication to the user that something
> went wrong. Since git-bugreport presently doesn't take any positionals
> at all, let's reject all positionals and give the user a usage hint.
>
> Signed-off-by: Emily Shaffer <nasamuffin@google.com>
> ---
> Per Eric's suggestion, added a citation of the first positional arg
> found. I don't think it's necessary to unroll the entire argv array
> here, though.
Thanks. I had the same thought about the first positional argument
being sufficient since it should provide enough context on its own.
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> @@ -126,6 +126,12 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> + if (argc) {
> + if (argv[0])
> + error(_("unknown argument `%s'"), argv[0]);
> + usage(bugreport_usage[0]);
> + }
Can it actually happen that argc is non-zero but argv[0] is NULL? (I
don't have parse-options in front of me to check.) If not, then the
extra `if (argv[0])` conditional may confuse future readers.
^ permalink raw reply
* Re: Why sometimes branch merging is associated with this commit: Merge remote-tracking branch 'remotes/p4/HEAD'
From: Thomas Guyot @ 2023-10-26 3:23 UTC (permalink / raw)
To: Yuri, Git Mailing List
In-Reply-To: <dd4befe1-dea7-b27d-720d-9f1616b129ee@tsoft.com>
Hi Yuri,
On 2023-10-20 15:46, Yuri wrote:
> I use git-p4 to use the perforce repository through git.
>
>
> There are 3 branches:
>
> * master
> remotes/p4/HEAD -> p4/master
> remotes/p4/master
>
> git-p4 syncs remotes/p4/HEAD with the perforce repository.
>
> Then the user (me) needs to merge remotes/p4/HEAD into master.
>
> Initially such merge doesn't cause "Merge remote-tracking branch
> 'remotes/p4/HEAD'" commits.
>
> But then, after several cycles of submit/sync something happens, and git
> forces me to commit with the "Merge remote-tracking branch
> 'remotes/p4/HEAD'" comment.
>
>
> What makes the merge from remotes/p4/HEAD into master to require "Merge
> remote-tracking branch 'remotes/p4/HEAD'"?
I believe what is happening is once other developers merge code in the
same branch your master branch ends up like a topic branch that never
merge back to the remote tracking branch.
I've worked a lot more with git-svn but both behaves somewhat similarly
AFAIK - when you "send" your commit upstream (I'm using "send" as I
don't recall the exact terminology for git-p4) what really happen is
like if your commit was cherry-picked on top of the remote branch - P4
tracks commits linearly and has an incompatible concept of merges.
When you sync later, it will get both your commits and other developer's
commit, but because they don't have the same history anymore git has no
choice but to merge the branches (this doesn't happen initially if
you're the only committer as your branch remain sync with the remote).
> What does this mean?
>
> What is changed in remotes/p4/HEAD or master that later requires this?
>
> How to eliminate the need for "Merge remote-tracking branch
> 'remotes/p4/HEAD'"?
>
You probably don't "need" to eliminate this, you can just keep creating
new commits on top and pushing them. The end result will be the same for
other p4 users.
If you still want to re-sync with upstream all you have to do is to
rebase on top of the remote tracking branch. You'll end up with the same
work tree but your history will get "linearized" based on the commits in p4.
Let's take for example this branch:
A---B---C [master, remotes/p4/HEAD]
Now imagine you and another developer both work on the repo separately
(let's assume different files, no possible conflicts) and the other
developer commits before you in p4, you end up with:
A---B---C---E [remotes/p4/HEAD]
\
`-D [master]
When you send *your* change, D, on the p4 repo, it cannot merge like git
does. It will instead apply your commit on top of E - we'll call your
commit D' as it's now a different commit hash, although it's the same
change:
A---B---C---E---D' [remotes/p4/HEAD]
\
`-D [master]
Now if you sync with p4, git can't fast-forward anymore because D and D'
are different commits (and don't even have the same parent!) therefore
it will merge (F) with the remote tracking branch:
A---B---C---E---D' [remotes/p4/HEAD]
\ \
`-D-----+-F [master]
F and D' are identical, they have the same work tree contents, but have
a different commit hash (implied by the difference in parents). If you
rebase master to remotes/p4/HEAD git will discard the duplicate D commit
from your branch and restore it at the tip of remotes/p4/HEAD. If you
has any new commit after F, those would be applied after D', for ex:
A---B---C---E---D' [remotes/p4/HEAD]
\ \
`-D-----+-F---G [master]
Rebases to:
A---B---C---E---D' [remotes/p4/HEAD]
\
`-G' [master]
Personally I would always rebase with the remote repo, and with git-svn
I was actually doing that systematically before pushing anything (I
think I had no choice anyway, or maybe I just didn't want any local
merge commits...) In any case you should be able to rebase with the p4
remote tracking branch before or after committing into p4 and that will
keep your history in line with p4's history.
Regards,
-
Thomas
^ permalink raw reply
* [PATCH v2] bugreport: reject positional arguments
From: emilyshaffer @ 2023-10-26 0:55 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer, Eric Sunshine, Sheik
In-Reply-To: <CAPig+cQtoEpTDK1U5R+wUD4qovFtpcx6+zyOQE-SAYp5SE2q2Q@mail.gmail.com>
From: Emily Shaffer <nasamuffin@google.com>
git-bugreport already rejected unrecognized flag arguments, like
`--diaggnose`, but this doesn't help if the user's mistake was to forget
the `--` in front of the argument. This can result in a user's intended
argument not being parsed with no indication to the user that something
went wrong. Since git-bugreport presently doesn't take any positionals
at all, let's reject all positionals and give the user a usage hint.
Signed-off-by: Emily Shaffer <nasamuffin@google.com>
---
Per Eric's suggestion, added a citation of the first positional arg
found. I don't think it's necessary to unroll the entire argv array
here, though.
- Emily
builtin/bugreport.c | 6 ++++++
t/t0091-bugreport.sh | 7 +++++++
2 files changed, 13 insertions(+)
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index d2ae5c305d..8a69a23397 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -126,6 +126,12 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, bugreport_options,
bugreport_usage, 0);
+ if (argc) {
+ if (argv[0])
+ error(_("unknown argument `%s'"), argv[0]);
+ usage(bugreport_usage[0]);
+ }
+
/* Prepare the path to put the result */
prefixed_filename = prefix_filename(prefix,
option_output ? option_output : "");
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index f6998269be..5b1b3e8d07 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -69,6 +69,13 @@ test_expect_success 'incorrect arguments abort with usage' '
test_path_is_missing git-bugreport-*
'
+test_expect_success 'incorrect positional arguments abort with usage and hint' '
+ test_must_fail git bugreport false 2>output &&
+ test_i18ngrep usage output &&
+ test_i18ngrep false output &&
+ test_path_is_missing git-bugreport-*
+'
+
test_expect_success 'runs outside of a git dir' '
test_when_finished rm non-repo/git-bugreport-* &&
nongit git bugreport
--
2.42.0.758.gaed0368e0e-goog
^ permalink raw reply related
* Re: git bugreport with invalid CLI argument does not report error
From: Eric Sunshine @ 2023-10-25 23:11 UTC (permalink / raw)
To: Emily Shaffer; +Cc: Sheik, git
In-Reply-To: <ZTmcVJaY2TjFCVyw@google.com>
On Wed, Oct 25, 2023 at 6:53 PM Emily Shaffer <nasamuffin@google.com> wrote:
> On Thu, Oct 26, 2023 at 07:59:16AM +1100, Sheik wrote:
> > Running git bugreport with an invalid CLI argument in a valid Git directory
> > does not report error. Expected behaviour would be that it reports an error.
>
> It looks like parse-options.[ch] helps us here for misspelled dashed
> options, like `--diaggnose`. But it doesn't complain when there are
> unexpected positional arguments. I think we can just notice if there are
> any argc left over, complain, and print usage.
>
> I put together a quick patch; could be that we don't need to leave this
> error about "positional arguments" and can leave it as an exercise to
> the reader to compare their previous command to the usage text. I guess
> we could also unroll remaining argv but it was just a hair more time
> than I wanted to spend ;)
>
> --- 8< ---
> Subject: [PATCH] bugreport: reject positional arguments
>
> git-bugreport already rejected unrecognized flag arguments, like
> `--diaggnose`, but this doesn't help if the user's mistake was to forget
> the `--` in front of the argument. This can result in a user's intended
> argument not being parsed with no indication to the user that something
> went wrong. Since git-bugreport presently doesn't take any positionals
> at all, let's reject all positionals and give the user a usage hint.
>
> Signed-off-by: Emily Shaffer <nasamuffin@google.com>
> ---
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> @@ -126,6 +126,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> + if (argc) {
> + error(_("git bugreport does not take positional arguments"));
> + usage(bugreport_usage[0]);
> + }
This error message may be somewhat difficult to interpret. Providing
additional context could help the user understand the actual problem.
For instance:
% git bugreport diagnose
error: unrecognized argument "diagnose"
^ permalink raw reply
* Re: git bugreport with invalid CLI argument does not report error
From: Emily Shaffer @ 2023-10-25 22:53 UTC (permalink / raw)
To: Sheik; +Cc: git
In-Reply-To: <7a92b537-ba88-4667-bb18-2e8c74aa9915@gmail.com>
On Thu, Oct 26, 2023 at 07:59:16AM +1100, Sheik wrote:
>
> Hi Maintainers,
>
>
> Running git bugreport with an invalid CLI argument in a valid Git directory
> does not report error. Expected behaviour would be that it reports an error.
>
>
> #Example git commands which should have reported an error but continues to
> succeed
>
> cd $ToAnyDirectory
>
> git bugreport diagnose
It looks like parse-options.[ch] helps us here for misspelled dashed
options, like `--diaggnose`. But it doesn't complain when there are
unexpected positional arguments. I think we can just notice if there are
any argc left over, complain, and print usage.
I put together a quick patch; could be that we don't need to leave this
error about "positional arguments" and can leave it as an exercise to
the reader to compare their previous command to the usage text. I guess
we could also unroll remaining argv but it was just a hair more time
than I wanted to spend ;)
- Emily
--- 8< ---
From 2031c7f55652559b8b4ec3c67ce4c4f94a355762 Mon Sep 17 00:00:00 2001
From: Emily Shaffer <nasamuffin@google.com>
Date: Wed, 25 Oct 2023 15:45:25 -0700
Subject: [PATCH] bugreport: reject positional arguments
git-bugreport already rejected unrecognized flag arguments, like
`--diaggnose`, but this doesn't help if the user's mistake was to forget
the `--` in front of the argument. This can result in a user's intended
argument not being parsed with no indication to the user that something
went wrong. Since git-bugreport presently doesn't take any positionals
at all, let's reject all positionals and give the user a usage hint.
Signed-off-by: Emily Shaffer <nasamuffin@google.com>
---
builtin/bugreport.c | 5 +++++
t/t0091-bugreport.sh | 6 ++++++
2 files changed, 11 insertions(+)
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index d2ae5c305d..eb6234a50d 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -126,6 +126,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, bugreport_options,
bugreport_usage, 0);
+ if (argc) {
+ error(_("git bugreport does not take positional arguments"));
+ usage(bugreport_usage[0]);
+ }
+
/* Prepare the path to put the result */
prefixed_filename = prefix_filename(prefix,
option_output ? option_output : "");
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index f6998269be..2061d6f386 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -69,6 +69,12 @@ test_expect_success 'incorrect arguments abort with usage' '
test_path_is_missing git-bugreport-*
'
+test_expect_success 'incorrect positional arguments abort with usage' '
+ test_must_fail git bugreport false 2>output &&
+ test_i18ngrep usage output &&
+ test_path_is_missing git-bugreport-*
+'
+
test_expect_success 'runs outside of a git dir' '
test_when_finished rm non-repo/git-bugreport-* &&
nongit git bugreport
--
2.42.0.758.gaed0368e0e-goog
^ permalink raw reply related
* Re: Cloning new bare repository using ssh does not respect bare repository --initial-branch
From: Sheik @ 2023-10-25 22:24 UTC (permalink / raw)
To: sahibzone; +Cc: git
In-Reply-To: <39e013af-69c7-4185-99ae-1e8fe5edef5c@gmail.com>
Hi Maintainers,
Sorry thought this wouldn't make it through due to html, please
disregard this and see latest posted here
https://lore.kernel.org/git/63eb269e-72b9-4830-98fc-aeef8b8180d7@gmail.com/
Thanks
Sheik
^ permalink raw reply
* Re: git diagnose in non git directory results in core dump
From: Kristoffer Haugsbakk @ 2023-10-25 21:01 UTC (permalink / raw)
To: Sheik; +Cc: git
In-Reply-To: <3e409165-2468-47be-895e-f3ec86b94730@gmail.com>
Being worked on: https://lore.kernel.org/git/CAKFQ_Q9WjF9i-Rx2jdCw-adPVQrWNfNKrDY-em8Rpa5RNLXz4A@mail.gmail.com/
--
Kristoffer Haugsbakk
^ permalink raw reply
* git bugreport with invalid CLI argument does not report error
From: Sheik @ 2023-10-25 20:59 UTC (permalink / raw)
To: git
Hi Maintainers,
Running git bugreport with an invalid CLI argument in a valid Git
directory does not report error. Expected behaviour would be that it
reports an error.
#Example git commands which should have reported an error but continues
to succeed
cd $ToAnyDirectory
git bugreport diagnose
Thanks
Sheik
[System Info]
git version:
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.0-2-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.5.6-1
(2023-10-07) x86_64
compiler info: gnuc: 13.2
libc info: glibc: 2.37
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
^ permalink raw reply
* git diagnose with invalid CLI argument does not report error
From: Sheik @ 2023-10-25 20:49 UTC (permalink / raw)
To: git
Hi Maintainers,
Running git diagnose with an invalid CLI argument in a valid Git
directory does not report error. Expected behaviour would be that it
reports an error.
#Example shell commands which should have reported an error but
continues to succeed
cd $ToAGitDirectory
git diagnose mod
git diagnose mode
git diagnose mode=all
Thanks
Sheik
[System Info]
git version:
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.0-2-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.5.6-1
(2023-10-07) x86_64
compiler info: gnuc: 13.2
libc info: glibc: 2.37
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
^ permalink raw reply
* git diagnose in non git directory results in core dump
From: Sheik @ 2023-10-25 20:41 UTC (permalink / raw)
To: git
Hi Maintainers,
Running git diagnose in a non git directory results in a core dump.
Expected behaviour would be that it reports an error.
mkdir test
cd test
git diagnose
#Output...
Segmentation fault (core dumped)
Thanks
Sheik
[System Info]
git version:
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.0-2-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.5.6-1
(2023-10-07) x86_64
compiler info: gnuc: 13.2
libc info: glibc: 2.37
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
^ permalink raw reply
* Repository cloned using SSH does not respect bare repository initial branch
From: Sheik @ 2023-10-25 20:36 UTC (permalink / raw)
To: git
Hi Maintainers,
Repository cloned using SSH does not use the branch configured in the
bare repository however repository cloned using filesystem does as
expected. Shouldn't they both behave the same?
Thanks
Sheik
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
#Create bare repository
su - $user
git init --bare --initial-branch=test test.git
#Clone repository
git clone ssh://$user@$computer:/home/$user/test.git test1
cd test1
git remote show origin
#Output [ORIGIN1]
echo abc >> abc.txt && git add * && git commit -a -m test && git push
#Output...
#* [new branch] master -> master
git remote show origin
#Output [ORIGIN2]
What did you expect to happen? (Expected behavior)
Repository cloned using ssh should push to test branch just like
repository cloned using filesystem does below as configured in the bare
repository.
#Clone repository
git clone home/$user/test.git test2
git remote show origin
#Output [ORIGIN3]
echo abc >> abc.txt && git add * && git commit -a -m test && git push
#Output...
#* [new branch] test -> test
git remote show origin
#Output [ORIGIN4]
What happened instead? (Actual behavior)
Repository cloned using ssh pushed to master branch disregarding
configuration in bare repository.
What's different between what you expected and what actually happened?
For ssh cloned repository test branch should been the default branch
however master was the default.
Anything else you want to add:
[ORIGIN1]
* remote origin
HEAD branch: (unknown)
Local branch configured for 'git pull':
master merges with remote master
[ORIGIN2]
* remote origin
HEAD branch: (unknown)
Remote branch:
master tracked
Local branch configured for 'git pull':
master merges with remote master
Local ref configured for 'git push':
master pushes to master (up to date)
[ORIGIN3]
* remote origin
HEAD branch: (unknown)
Local branch configured for 'git pull':
test merges with remote test
[ORIGIN4]
* remote origin
HEAD branch: test
Remote branches:
master new (next fetch will store in remotes/origin)
test tracked
Local branch configured for 'git pull':
test merges with remote test
Local ref configured for 'git push':
test pushes to test (up to date)
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.0-2-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.5.6-1
(2023-10-07) x86_64
compiler info: gnuc: 13.2
libc info: glibc: 2.37
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
^ permalink raw reply
* Cloning new bare repository using ssh does not respect bare repository --initial-branch
From: Sheik @ 2023-10-25 19:57 UTC (permalink / raw)
To: git
Hi Maintainers,
When a new bare repository is cloned using SSH then cloned HEAD does not
point to initial branch from the bare repository. However if cloned
using filesystem then HEAD points to initial branch as expected.
Shouldn't cloning via ssh also exhibit the same behaviour as cloning via
filesystem?
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
#Create bare repository
su - $user
git init --bare --initial-branch=test test.git
#Clone repository
git clone ssh://$user@$computer:/home/$user/test.git test1
cd test1
#git remote show origin
#Output [ORIGIN1]
echo abc >> abc.txt && git add * && git commit -a -m test && git push
#Output...
* [new branch] master -> master
#git remote show origin
#Output [ORIGIN2]
What did you expect to happen? (Expected behavior)
Repository cloned using ssh should push to test branch just like
repository cloned using filesystem does below as configured in the bare
repository.
#Clone repository
git clone home/$user/test.git test2
#git remote show origin
#Output [ORIGIN3]
echo abc >> abc.txt && git add * && git commit -a -m test && git push
#Output...
* [new branch] test -> test
#git remote show origin
#Output [ORIGIN4]
What happened instead? (Actual behavior)
Repository cloned using ssh pushed to master branch disregarding
configuration in bare repository.
What's different between what you expected and what actually happened?
For ssh cloned repository test branch should been the default branch
however master was the default.
Anything else you want to add:
[ORIGIN1]
* remote origin
HEAD branch: (unknown)
Local branch configured for 'git pull':
master merges with remote master
[ORIGIN2]
* remote origin
HEAD branch: (unknown)
Remote branch:
master tracked
Local branch configured for 'git pull':
master merges with remote master
Local ref configured for 'git push':
master pushes to master (up to date)
[ORIGIN3]
* remote origin
HEAD branch: (unknown)
Local branch configured for 'git pull':
test merges with remote test
[ORIGIN4]
* remote origin
HEAD branch: test
Remote branches:
master new (next fetch will store in remotes/origin)
test tracked
Local branch configured for 'git pull':
test merges with remote test
Local ref configured for 'git push':
test pushes to test (up to date)
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.0-2-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.5.6-1
(2023-10-07) x86_64
compiler info: gnuc: 13.2
libc info: glibc: 2.37
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
^ permalink raw reply
* Re: [PATCH v3 0/5] config-parse: create config parsing library
From: Josh Steadmon @ 2023-10-25 19:37 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Taylor Blau, Junio C Hamano, git, calvinwan, glencbz
In-Reply-To: <20231024225005.1191555-1-jonathantanmy@google.com>
On 2023.10.24 15:50, Jonathan Tan wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> > But I am not sure that I agree that this series is moving us in the
> > right direction necessarily. Or at least I am not convinced that
> > shipping the intermediate state is worth doing before we have callers
> > that could drop '#include "config.h"' for just the parser.
> >
> > This feels like churn that does not yield a tangible pay-off, at least
> > in the sense that the refactoring and code movement delivers us
> > something that we can substantively use today.
> >
> > I dunno.
> >
> > Thanks,
> > Taylor
>
> Thanks for calling this out. We do want our changes to be good for both
> the libification and the non-libification cases as much as possible. As
> it is, I do agree that since we won't have callers that can use the new
> parser header (I think the likeliest cause of having such a caller is
> if we have a "interpret-config" command, like "interpret-trailers"), we
> probably shouldn't merge this (at least, the last 2 patches).
>
> I think patches 1-3 are still usable (they make some internals of config
> parsing less confusing) but I'm also OK if we hold off on them until
> we find a compelling use case that motivates refactoring on the config
> parser.
Thanks everyone for the revived discussion here. I think I agree, this
series is not going in the right direction. Additionally, our internal
use case for this change has evaporated, so let's just drop the series.
We can pick it up again later if interest returns.
^ permalink raw reply
* Re: [PATCH] merge: --ff-one-only to apply FF if commit is one
From: Kristoffer Haugsbakk @ 2023-10-25 19:04 UTC (permalink / raw)
To: Ruslan Yakauleu; +Cc: git
In-Reply-To: <e38ebf04-cf92-c80b-3432-bf86ecda1054@gmail.com>
On Wed, Oct 25, 2023, at 20:31, Ruslan Yakauleu wrote:
> Hi
>
> If we use squash we will lose some context and occasionally, we need
> multiple small features combined into one big release. We would rather
> not mix it into one monolithic non-readable blob.
The only context you lose is the point where your branch started, if that
matters. (Later in the email you say that you rebase before you merge so
apparently it does not matter to you.)
To squash one single commit doesn’t make a “monolithic” commit—it is
exactly as monolithic as the commit you started with.
> For us, sometimes it is better to rebase something to make history more
> accurate than squash everything into one commit. We can use squash only
> for one story.
No, squash one single commit. Not multiple.
> Anyway, squash is a different feature. Same as rebase (of course we're
> doing the rebase before merge to clarify history and to make some
> regression tests)
Squash and rebase are functionally identical in this case. And since you
rebase before merge anyway it is identical to the flow of
1. Rebase before merge
2. Merge with your new option
Except you don’t have to split it into two steps.
^ permalink raw reply
* [PATCH v2] send-email: move validation code below process_address_list
From: Michael Strawbridge @ 2023-10-25 18:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
In-Reply-To: <xmqqil6v3cgq.fsf@gitster.g>
From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001
From: Michael Strawbridge <michael.strawbridge@amd.com>
Date: Wed, 11 Oct 2023 16:13:13 -0400
Subject: [PATCH v2] send-email: move validation code below
process_address_list
Move validation logic below processing of email address lists so that
email validation gets the proper email addresses. As a side effect,
some initialization needed to be moved down. In order for validation
and the actual email sending to have the same initial state, the
initialized variables that get modified by pre_process_file are
encapsulated in a new function.
This fixes email address validation errors when the optional
perl module Email::Valid is installed and multiple addresses are passed
in on a single to/cc argument like --to=foo@example.com,bar@example.com.
A new test was added to t9001 to expose failures with this case in the
future.
Fixes: a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")
Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
---
git-send-email.perl | 60 +++++++++++++++++++++++--------------------
t/t9001-send-email.sh | 19 ++++++++++++++
2 files changed, 51 insertions(+), 28 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 288ea1ae80..ce22a5e06d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -799,30 +799,6 @@ sub is_format_patch_arg {
$time = time - scalar $#files;
-if ($validate) {
- # FIFOs can only be read once, exclude them from validation.
- my @real_files = ();
- foreach my $f (@files) {
- unless (-p $f) {
- push(@real_files, $f);
- }
- }
-
- # Run the loop once again to avoid gaps in the counter due to FIFO
- # arguments provided by the user.
- my $num = 1;
- my $num_files = scalar @real_files;
- $ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
- foreach my $r (@real_files) {
- $ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
- pre_process_file($r, 1);
- validate_patch($r, $target_xfer_encoding);
- $num += 1;
- }
- delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
- delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
-}
-
@files = handle_backup_files(@files);
if (@files) {
@@ -1754,10 +1730,6 @@ sub send_message {
return 1;
}
-$in_reply_to = $initial_in_reply_to;
-$references = $initial_in_reply_to || '';
-$message_num = 0;
-
sub pre_process_file {
my ($t, $quiet) = @_;
@@ -2023,6 +1995,38 @@ sub process_file {
return 1;
}
+sub initialize_modified_loop_vars {
+ $in_reply_to = $initial_in_reply_to;
+ $references = $initial_in_reply_to || '';
+ $message_num = 0;
+}
+
+if ($validate) {
+ # FIFOs can only be read once, exclude them from validation.
+ my @real_files = ();
+ foreach my $f (@files) {
+ unless (-p $f) {
+ push(@real_files, $f);
+ }
+ }
+
+ # Run the loop once again to avoid gaps in the counter due to FIFO
+ # arguments provided by the user.
+ my $num = 1;
+ my $num_files = scalar @real_files;
+ $ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
+ initialize_modified_loop_vars();
+ foreach my $r (@real_files) {
+ $ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
+ pre_process_file($r, 1);
+ validate_patch($r, $target_xfer_encoding);
+ $num += 1;
+ }
+ delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
+ delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
+}
+
+initialize_modified_loop_vars();
foreach my $t (@files) {
while (!process_file($t)) {
# user edited the file
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 263db3ad17..ccff2ad647 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -633,6 +633,25 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
test_cmp expect actual
'
+test_expect_success $PREREQ "--validate hook supports multiple addresses in arguments" '
+ hooks_path="$(pwd)/my-hooks" &&
+ test_config core.hooksPath "$hooks_path" &&
+ test_when_finished "rm my-hooks.ran" &&
+ test_must_fail git send-email \
+ --from="Example <nobody@example.com>" \
+ --to=nobody@example.com,abc@example.com \
+ --smtp-server="$(pwd)/fake.sendmail" \
+ --validate \
+ longline.patch 2>actual &&
+ test_path_is_file my-hooks.ran &&
+ cat >expect <<-EOF &&
+ fatal: longline.patch: rejected by sendemail-validate hook
+ fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1
+ warning: no patches were sent
+ EOF
+ test_cmp expect actual
+'
+
test_expect_success $PREREQ "--validate hook supports header argument" '
write_script my-hooks/sendemail-validate <<-\EOF &&
if test "$#" -ge 2
--
2.42.GIT
^ permalink raw reply related
* Re: [PATCH] send-email: move validation code below process_address_list
From: Michael Strawbridge @ 2023-10-25 18:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
In-Reply-To: <xmqqil6v3cgq.fsf@gitster.g>
On 10/24/23 18:03, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Michael Strawbridge <michael.strawbridge@amd.com> writes:
>>
>>> Subject: [PATCH] send-email: move validation code below process_address_list
>>>
>>> Move validation logic below processing of email address lists so that
>>> email validation gets the proper email addresses.
>>
>> Hmph, without this patch, the tip of 'seen' passes t9001 on my box,
>> but with it, it claims that it failed 58, 87, and 89.
>
> FWIW, when this patch is used with 'master' (not 'seen'), t9001
> claims the same three tests failed. The way #58 fails seems to be
> identical to the way 'seen' with this patch failed, shown in the
> message I am responding to.
I'm sorry to have wasted your time with patch 1. I had done the other manual
tests but ended up forgetting the automated ones.
^ permalink raw reply
* Re: [PATCH] send-email: move validation code below process_address_list
From: Michael Strawbridge @ 2023-10-25 18:47 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List
In-Reply-To: <20231025065033.GB2094463@coredump.intra.peff.net>
On 10/25/23 02:50, Jeff King wrote:
> On Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:
>
>> Move validation logic below processing of email address lists so that
>> email validation gets the proper email addresses.
>>
>> This fixes email address validation errors when the optional
>> perl module Email::Valid is installed and multiple addresses are passed
>> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
>
> Is there a test we can include here?
>
>> @@ -2023,6 +1999,30 @@ sub process_file {
>> return 1;
>> }
>>
>> +if ($validate) {
>
> So the new spot is right before we call process_file() on each of the
> input files. It is a little hard to follow because of the number of
> functions defined inline in the middle of the script, but I think that
> is a reasonable spot. It is after we have called process_address_list()
> on to/cc/bcc, which I think fixes the regression. But it is also after
> we sanitize $reply_to, etc, which seems like a good idea.
>
> But I think putting it down that far is the source of the test failures.
>
> The culprit seems not to be the call to validate_patch() in the loop you
> moved, but rather pre_process_file(), which was added in your earlier
> a8022c5f7b (send-email: expose header information to git-send-email's
> sendemail-validate hook, 2023-04-19).
>
> It looks like the issue is the global $message_num variable which is
> incremented by pre_process_file(). On line 1755 (on the current tip of
> master), we set it to 0. And your patch moves the validation across
> there (from line ~799 to ~2023).
>
> And that's why the extra increments didn't matter when you added the
> calls to pre_process_file() in your earlier patch; they all happened
> before we reset $message_num to 0. But now they happen after.
>
> To be fair, this is absolutely horrific perl code. There's over a
> thousand lines of function definitions, and then hidden in the middle
> are some global variable assignments!
I agree. Following where things are initialized seems to be especially troublesome.
>
> So we have a few options, I think:
>
> 1. Reset $message_num to 0 after validating (maybe we also need
> to reset $in_reply_to, etc, set at the same time? I didn't check).
> This feels like a hack.
>
> 2. Move the validation down, but not so far down. Like maybe right
> after we set up the @files list with the $compose.final name. This
> is the smallest diff, but feels like we haven't really made the
> world a better place.
>
> 3. Move the $message_num, etc, initialization to right before we call
> the process_file() loop, which is what expects to use them. Like
> this (squashed into your patch):
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index a898dbc76e..d44db14223 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1730,10 +1730,6 @@ sub send_message {
> return 1;
> }
>
> -$in_reply_to = $initial_in_reply_to;
> -$references = $initial_in_reply_to || '';
> -$message_num = 0;
> -
> sub pre_process_file {
> my ($t, $quiet) = @_;
>
> @@ -2023,6 +2019,10 @@ sub process_file {
> delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
> }
>
> +$in_reply_to = $initial_in_reply_to;
> +$references = $initial_in_reply_to || '';
> +$message_num = 0;
> +
> foreach my $t (@files) {
> while (!process_file($t)) {
> # user edited the file
>
The above patch was a great place to start. Thank you! In order to address
the fact that validation and actually sending the emails should have the same
initial conditions I created a new function to set the variables and call it
instead.
> That seems to make the test failures go away. It is still weird that the
> validation code is calling pre_process_file(), which increments
> $message_num, without us having set it up in any meaningful way. I'm not
> sure if there are bugs lurking there or not. I'm not impressed by the
> general quality of this code, and I'm kind of afraid to keep looking
> deeper.
>
> -Peff
^ permalink raw reply
* Re: [PATCH] merge: --ff-one-only to apply FF if commit is one
From: Ruslan Yakauleu @ 2023-10-25 18:31 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <da246287-8530-4680-8fcc-f68f881bc24b@app.fastmail.com>
Hi
If we use squash we will lose some context and occasionally, we need
multiple small features combined into one big release. We would rather
not mix it into one monolithic non-readable blob. For us, sometimes it
is better to rebase something to make history more accurate than squash
everything into one commit. We can use squash only for one story.
Anyway, squash is a different feature.
Same as rebase (of course we're doing the rebase before merge to clarify
history and to make some regression tests)
Then we have a set of branches. Some branches contain only one commit.
For example, we have 4 branches:
- two commits
- one commit
- one commit
- three commits
With --no-ff (only merges) we have next graph with extra merges (E', F')
for branches with one commit
B---C E F G---H---I
/ \ / \ / \ / \
A-------D---E'--F'----------J
With --ff (fast-forward everything) we miss merge branches (D, J) and
it's harder to fast revert some problematic releases properly, because
it's not clear that commits G-H-I - is one release
A---B---C---E---F---G---H---I
Story which --ff-one-only should build without manual control
B---C G---H---I
/ \ / \
A-------D---E---F-----------J
There we have merge commits (D, J) only for complex branches.
Branches E and F fast-forwarded to prevent extra merges.
Sorry if my explanation isn't clear enough
--
Ruslan
^ permalink raw reply
* Re: [PATCH v5 3/5] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
From: Eric Sunshine @ 2023-10-25 17:21 UTC (permalink / raw)
To: Taylor Blau
Cc: Patrick Steinhardt, git, Elijah Newren, Eric W. Biederman,
Jeff King, Junio C Hamano
In-Reply-To: <ZTk3zoncT6nvV3aQ@nand.local>
On Wed, Oct 25, 2023 at 11:44 AM Taylor Blau <me@ttaylorr.com> wrote:
> On Wed, Oct 25, 2023 at 09:58:06AM +0200, Patrick Steinhardt wrote:
> > On Mon, Oct 23, 2023 at 06:45:01PM -0400, Taylor Blau wrote:
> > > In order to support streaming from a location in memory, we must
> > > implement a new kind of bulk_checkin_source that does just that. These
> > > implementation in spread out across:
> >
> > Nit: the commit message is a bit off here. Probably not worth a reroll
> > though.
>
> Your eyes are definitely mine, because I'm not seeing where the commit
> message is off! But hopefully since you already don't think it's worth a
> reroll, and I'm not even sure what the issue is that we can just leave
> it ;-).
Perhaps:
s/implementation in/implementations are/
^ permalink raw reply
* Re: [PATCH] merge: --ff-one-only to apply FF if commit is one
From: Kristoffer Haugsbakk @ 2023-10-25 16:27 UTC (permalink / raw)
To: Ruslan Yakauleu; +Cc: git, Josh Soref
In-Reply-To: <pull.1599.git.git.1698224280816.gitgitgadget@gmail.com>
Hi
On Wed, Oct 25, 2023, at 10:58, Ruslan Yakauleu via GitGitGadget wrote:
> A new option --ff-one-only to control the merging strategy.
> For one commit option works like -ff to avoid extra merge commit.
> In other cases the option works like --no-ff to create merge commit for
> complex features.
I do something similar for my pull requests:
- If more than one commit: `--no-ff`
- One commit: rebase/squash
Your change would (according to the commit message) only *not* create a
merge if it so happens to be the case that you can fast-forward the
branch. So it would create a merge commit if (say) your branch was three
commits behind and one ahead (your commit) of the target branch. But isn’t
it fine to just rebase/squash in this case? That seems more general.
(Maybe `--squash-one-only`.)
But it seems that your new option would work nicely with “semi-linear”
merges:[1]
- Rebase on the target branch
- `--ff-one-only`
- Now you either get:
- a “semi-linear merge”; or
- a single commit (as opposed to a merge which joins together a branch
which is 0 behind and 1 ahead of the target branch)
--
Kristoffer
^ permalink raw reply
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Taylor Blau @ 2023-10-25 15:46 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <ZTjKk8E55M7lQN1m@tanuki>
On Wed, Oct 25, 2023 at 09:58:11AM +0200, Patrick Steinhardt wrote:
> > +test_expect_success 'merge-tree can pack its result with --write-pack' '
> > + test_when_finished "rm -rf repo" &&
> > + git init repo &&
> > +
> > + # base has lines [3, 4, 5]
> > + # - side adds to the beginning, resulting in [1, 2, 3, 4, 5]
> > + # - other adds to the end, resulting in [3, 4, 5, 6, 7]
> > + #
> > + # merging the two should result in a new blob object containing
> > + # [1, 2, 3, 4, 5, 6, 7], along with a new tree.
> > + test_commit -C repo base file "$(test_seq 3 5)" &&
> > + git -C repo branch -M main &&
> > + git -C repo checkout -b side main &&
> > + test_commit -C repo side file "$(test_seq 1 5)" &&
> > + git -C repo checkout -b other main &&
> > + test_commit -C repo other file "$(test_seq 3 7)" &&
> > +
> > + find repo/$packdir -type f -name "pack-*.idx" >packs.before &&
> > + tree="$(git -C repo merge-tree --write-pack \
> > + refs/tags/side refs/tags/other)" &&
> > + blob="$(git -C repo rev-parse $tree:file)" &&
> > + find repo/$packdir -type f -name "pack-*.idx" >packs.after &&
>
> While we do assert that we write a new packfile, we don't assert whether
> parts of the written object may have been written as loose objects. Do
> we want to tighten the checks to verify that?
We could, but the tests in t1050 already verify this, so I'm not sure
that we would be significantly hardening our test coverage here. If you
feel strongly about it, though, I'm happy to change it up.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v5 3/5] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
From: Taylor Blau @ 2023-10-25 15:44 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <ZTjKjkRMkmCuxDU1@tanuki>
On Wed, Oct 25, 2023 at 09:58:06AM +0200, Patrick Steinhardt wrote:
> On Mon, Oct 23, 2023 at 06:45:01PM -0400, Taylor Blau wrote:
> > Introduce `index_blob_bulk_checkin_incore()` which allows streaming
> > arbitrary blob contents from memory into the bulk-checkin pack.
> >
> > In order to support streaming from a location in memory, we must
> > implement a new kind of bulk_checkin_source that does just that. These
> > implementation in spread out across:
>
> Nit: the commit message is a bit off here. Probably not worth a reroll
> though.
Your eyes are definitely mine, because I'm not seeing where the commit
message is off! But hopefully since you already don't think it's worth a
reroll, and I'm not even sure what the issue is that we can just leave
it ;-).
> > +static off_t bulk_checkin_source_seek_incore(struct bulk_checkin_source *source,
> > + off_t offset)
> > +{
> > + if (!(0 <= offset && offset < source->size))
> > + return (off_t)-1;
>
> At the risk of showing my own ignorance, but why is the cast here
> necessary?
It's not necessary, see this godbolt example that shows two functions
doing the same thing (one with the explicit cast, one without):
https://godbolt.org/z/f737EcGfG
But there are a couple of other spots within the bulk-checkin code
(specifically within the deflate_blob_to_pack() routine) that explicitly
cast -1 to an off_t, so I was more trying to imitate the local style.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v5 1/5] bulk-checkin: extract abstract `bulk_checkin_source`
From: Taylor Blau @ 2023-10-25 15:39 UTC (permalink / raw)
To: Jeff King
Cc: git, Elijah Newren, Eric W. Biederman, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <20231025073736.GB2145145@coredump.intra.peff.net>
On Wed, Oct 25, 2023 at 03:37:36AM -0400, Jeff King wrote:
> I don't mind this in-between state. It is a funny layering violating
> from an OO standpoint, but it's not like we expect an unbounded set of
> concrete types to "inherit" from the source struct.
Yeah, this was exactly my thinking when writing up the changes for this
round. Since all of the "sub-classes" are local to the bulk-checkin.o
compilation unit, I don't have grave concerns about one implementation
peering into the details of another's.
Gotta stop somewhere ;-).
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 00/12] show-ref: introduce mode to check for ref existence
From: Phillip Wood @ 2023-10-25 14:44 UTC (permalink / raw)
To: Han-Wen Nienhuys, Junio C Hamano; +Cc: Patrick Steinhardt, git, Eric Sunshine
In-Reply-To: <CAFQ2z_PqNsz+zycSxz=q2cUVOpJS-AEjwHxEM-fiafxd3dxc9g@mail.gmail.com>
On 25/10/2023 15:26, Han-Wen Nienhuys wrote:
> On Tue, Oct 24, 2023 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>>> this patch series introduces a new `--exists` mode to git-show-ref(1) to
>>> explicitly check for the existence of a reference, only.
>>
>> I agree that show-ref would be the best place for this feature (not
>> rev-parse, which is already a kitchen sink). After all, the command
>> was designed for validating refs in 358ddb62 (Add "git show-ref"
>> builtin command, 2006-09-15).
>>
>> Thanks. Hopefully I can take a look before I go offline.
>
> The series description doesn't say why users would care about this.
>
> If this is just to ease testing, I suggest adding functionality to a
> suitable test helper. Anything you add to git-show-ref is a publicly
> visible API that needs documentation and comes with a stability
> guarantee that is more expensive to maintain than test helper
> functionality.
Does the new functionality provide a way for scripts to see if a branch
is unborn (i.e. has not commits yet)? I don't think we have a way to
distinguish between a ref that points to a missing object and an unborn
branch at the moment.
Best Wishes
Phillip
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox