* [PATCH 6/8] t7900: fix `pfx` dependency
From: Kristoffer Haugsbakk @ 2023-10-14 21:45 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, stolee
In-Reply-To: <cover.1697319294.git.code@khaugsbakk.name>
Test `start and stop when several schedulers are available` depends on
`pfx` from `start and stop macOS maintenance`.
Duplicate the behavior.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
t/t7900-maintenance.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index ebde3e8a212..15a8653b583 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -794,6 +794,7 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
'
test_expect_success 'start and stop when several schedulers are available' '
+ pfx=$(cd "$HOME" && pwd) &&
write_script print-args <<-\EOF &&
printf "%s\n" "$*" | sed "s:gui/[0-9][0-9]*:gui/[UID]:; s:\(schtasks /create .* /xml\).*:\1:;" >>args
EOF
--
2.42.0.2.g879ad04204
^ permalink raw reply related
* [PATCH 7/8] t7900: fix `print-args` dependency
From: Kristoffer Haugsbakk @ 2023-10-14 21:45 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, stolee
In-Reply-To: <cover.1697319294.git.code@khaugsbakk.name>
Test `use launchctl list to prevent extra work` depends on `print-args`
from `start and stop macOS maintenance`.
Duplicate the script writing.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
t/t7900-maintenance.sh | 3 +++
1 file changed, 3 insertions(+)
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 15a8653b583..99279e41787 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -709,6 +709,9 @@ test_expect_success 'start and stop macOS maintenance' '
'
test_expect_success 'use launchctl list to prevent extra work' '
+ write_script print-args <<-\EOF &&
+ echo $* | sed "s:gui/[0-9][0-9]*:gui/[UID]:" >>args
+ EOF
# ensure we are registered
GIT_TEST_MAINT_SCHEDULER=launchctl:./print-args git maintenance start --scheduler=launchctl &&
--
2.42.0.2.g879ad04204
^ permalink raw reply related
* [PATCH 8/8] t7900: factor out packfile dependency
From: Kristoffer Haugsbakk @ 2023-10-14 21:45 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, stolee
In-Reply-To: <cover.1697319294.git.code@khaugsbakk.name>
Tests `'--schedule inheritance weekly -> daily -> hourly` and
`maintenance.strategy inheritance` depend on the packfile made in
`incremental-repack task`.
Factor out the packfile creation.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
t/t7900-maintenance.sh | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 99279e41787..bc417b518b5 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -257,13 +257,15 @@ test_expect_success 'maintenance.loose-objects.auto' '
test_subcommand git prune-packed --quiet <trace-loC
'
-test_expect_success 'incremental-repack task' '
+test_expect_success 'setup packfile' '
packDir=.git/objects/pack &&
for i in $(test_seq 1 5)
do
test_commit $i || return 1
- done &&
+ done
+'
+test_expect_success 'incremental-repack task' '
# Create three disjoint pack-files with size BIG, small, small.
echo HEAD~2 | git pack-objects --revs $packDir/test-1 &&
test_tick &&
--
2.42.0.2.g879ad04204
^ permalink raw reply related
* [PATCH 9/8] t7900: fix register dependency
From: Kristoffer Haugsbakk @ 2023-10-14 23:00 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, stolee
In-Reply-To: <cover.1697319294.git.code@khaugsbakk.name>
The test `maintenance.auto config option` will fail if any preceding test
has run `git maintenance register` since that turns `maintenance.auto` off
for that repository and later calls to `unregister` will not turn it back
to the default `true` value.
Start with a fresh repository in this test.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
I found this after publishing the series.
t/t7900-maintenance.sh | 2 ++
1 file changed, 2 insertions(+)
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index bc417b518b..dbc5e1eb44 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -55,6 +55,8 @@ test_expect_success 'run [--auto|--quiet]' '
'
test_expect_success 'maintenance.auto config option' '
+ rm -rf .git &&
+ git init &&
GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
test_subcommand git maintenance run --auto --quiet <default &&
GIT_TRACE2_EVENT="$(pwd)/true" \
--
2.42.0.2.g879ad04204
^ permalink raw reply related
* Re: [PATCH] bugreport: add 'seconds' to default outfile name
From: Jacob Stopak @ 2023-10-15 3:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq4jitw4nk.fsf@gitster.g>
On Sat, Oct 14, 2023 at 09:27:27AM -0700, Junio C Hamano wrote:
> Jacob Stopak <jacob@initialcommit.io> writes:
>
> Is "postfix" a verb that is commonly understood? I would say
> "append" would be understood by more readers.
It's probably true that "append" or "suffix" (which is used in the code)
would be more easily understood. I'll switch in my updated messages.
> Also, is "calendar"
> hour different from other kinds of hours, perhaps stopwatch hours
> and microwave-oven hours?
Lol! By saying "calendar" I mean "falling on the official boundaries
of", like 11:15:00 - 11:16:00. Unlike the time between 11:15:30 -
11:16:30 which is also a minute, but it's not a "calendar" minute
because it overlaps into the next minute. I guess in this case it's more
of a "clock" minute than a "calendar" minute though ':D... I guess
"calendar" terminology is used more for months/years...
> I personally do not think it is a problem, simply because a quality
> bug report that would capture information necessary to diagnose any
> issue concisely in a readable fashion would take at least 90 seconds
> or more to produce, though.
This is true, when the user intentionally opens the bugreport with the
intent to start filling it out immediately, I assume they would almost
always cross the minute barrier and avoid the issue.
However, there are edge cases like the one I outlined, where the user
might open and close the report quickly, followed by rerunning the
command. This could be someone learning to use the command for the first
time. Or the case where a user only fills in a small part of the report
before closing it and running the command again.
These cases are certainly "the exception" but it seems the program could
be a bit more consistent/intuitive when they do occur.
> Instead of lengthening the filename for all files by 2 digits, the
> command can retry by adding say "+1", "+2", etc. after the failed
> filename to find a unique suffix within the same minute. It would
> mean that after writing git-bugreport-2023-10-14-0920.txt and you
> start another one without spending enough time, the new one may
> become git-bugreport-2023-10-14-0920+1.txt or something unique. It
> would be really unlikely that you would run out after failing to
> find a vacant single digit suffix nine times, i.e. trying "+9". It
> would also help preserve existing user's workflow, e.g. they may
> have written automation that assumes the down-to-minute format and
> it would keep working on their bug reports without breaking.
I agree with all of this, and to me it's a better solution than
_appending_ the second value :). I have a patchset almost ready for this
so I'll try to submit it later tonight.
^ permalink raw reply
* Re: [PATCH 0/8] t7900: untangle test dependencies
From: Jeff King @ 2023-10-15 3:04 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, stolee
In-Reply-To: <cover.1697319294.git.code@khaugsbakk.name>
On Sat, Oct 14, 2023 at 11:45:51PM +0200, Kristoffer Haugsbakk wrote:
> § CI
>
> The CI failed but it didn't look relevant.
>
> https://github.com/LemmingAvalanche/git/actions/runs/6518415327/job/17703822606
From a brief look, I think it is that your branch is based on v2.42.0,
which does not contain 0763c3a2c4 (http: update curl http/2 info
matching for curl 8.3.0, 2023-09-15). And the macos CI image has since
been updated to a more recent version of curl.
So yeah, not anything to worry about for your series.
-Peff
^ permalink raw reply
* Re: [PATCH] bugreport: add 'seconds' to default outfile name
From: Jacob Stopak @ 2023-10-15 3:07 UTC (permalink / raw)
To: Dragan Simic; +Cc: Junio C Hamano, git
In-Reply-To: <438a5edf1a17ffac201436a950ce50fa@manjaro.org>
On Sat, Oct 14, 2023 at 07:52:32PM +0200, Dragan Simic wrote:
>
> Speaking in general, I somehow find "20220712" a bit more readable than
> "2022-07-12" as part of a filename, but that's of course just my personal
> preference.
It's funny how we all have our own preferences for this kind of thing.
Mine happens to be separating the date part from the rest of the
filename with an underscore, but using a hyphen to separate individual
date components like:
filename_2022-07-12.ext
^ permalink raw reply
* Re: [PATCH] bugreport: add 'seconds' to default outfile name
From: Dragan Simic @ 2023-10-15 3:13 UTC (permalink / raw)
To: Jacob Stopak; +Cc: Junio C Hamano, git
In-Reply-To: <ZStXbjgFTlO11Pp7.jacob@initialcommit.io>
On 2023-10-15 05:07, Jacob Stopak wrote:
> On Sat, Oct 14, 2023 at 07:52:32PM +0200, Dragan Simic wrote:
>>
>> Speaking in general, I somehow find "20220712" a bit more readable
>> than
>> "2022-07-12" as part of a filename, but that's of course just my
>> personal
>> preference.
>
> It's funny how we all have our own preferences for this kind of thing.
> Mine happens to be separating the date part from the rest of the
> filename with an underscore, but using a hyphen to separate individual
> date components like:
>
> filename_2022-07-12.ext
Yes, it's quite funny. I gave it some thought, and I think that my
preference is a result of dealing with many PDF files containing
schematics, for which some kind of a defacto standard is the "-20220712"
naming convention.
^ permalink raw reply
* Re: [PATCH 21/20] t5319: make corrupted large-offset test more robust
From: Jeff King @ 2023-10-15 3:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Taylor Blau
In-Reply-To: <xmqq4jitt2ie.fsf@gitster.g>
On Sat, Oct 14, 2023 at 12:42:01PM -0700, Junio C Hamano wrote:
> > The set of objects created in the test is deterministic. But the delta
> > selection seems not to be (which is not too surprising, as it is
> > multi-threaded).
>
> So, the offsets of the objects are also not deterministic?
Hmm, yeah, you're right. The pack clearly is not deterministic, and so
neither will its offsets be.
And thus...
> > b. The "objects64" setup could use --delta-base-offset. This would fix
> > our problem, but earlier tests have many hard-coded offsets. Using
> > OFS_DELTA would change the locations of objects in the pack (this
> > might even be OK because I think most of the offsets are within the
> > .idx file, but it seems brittle and I'm afraid to touch it).
>
> I am not sure I follow, as it does not sound a solution to anything
> if the offsets are not deterministic (and "earlier tests" that have
> hard coded offsets are broken no matter what, which is not a problem
> this patch introduces). Puzzled, but not curious enough to think
> about it further, as you have already rejected this approach ;-)
...yes, my "this might even be OK" is true. So it would work to solve
the problem by using --delta-base-offset. Those earlier tests are
actually OK (they munge only the idx and the midx, not the pack). And if
we have delta base offsets, then walking delta chains never requires
looking at the pack idx.
I'm not sure if that is any less subtle than the solution I did come up
with, though.
The most unsubtle thing is cleaning up the broken .idx immediately
after generating the midx. I didn't want to do that because it disrupts
the state of the objects64 directory. But we could always turn its setup
into a function or something. I dunno. It is probably not worth spending
too many brain cycles on. My hope is that nobody ever has to touch this
test ever again. ;)
> > @@ -1129,8 +1129,10 @@ test_expect_success 'reader bounds-checks large offset table' '
> > git multi-pack-index --object-dir=../objects64 write &&
> > midx=../objects64/pack/multi-pack-index &&
> > corrupt_chunk_file $midx LOFF clear &&
> > - test_must_fail git cat-file \
> > - --batch-check --batch-all-objects 2>err &&
> > + # using only %(objectsize) is important here; see the commit
> > + # message for more details
> > + test_must_fail git cat-file --batch-all-objects \
> > + --batch-check="%(objectsize)" 2>err &&
>
> A rather unfriendly message to readers, as it is unclear which
> commit you are talking about, and a fun thing is that you cannot
> say which one.
Yeah, I had a similar thought process. I sort of assume anybody working
on git.git is capable of turning to "git blame" in a situation like
this. But maybe:
# using only %(objectsize) is important here; run "git blame"
# on these lines for more details
would spell it out more clearly.
-Peff
^ permalink raw reply
* Re: [PATCH] grep: die gracefully when outside repository
From: Jeff King @ 2023-10-15 3:26 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, ks1322
In-Reply-To: <087c92e3904dd774f672373727c300bf7f5f6369.1697317276.git.code@khaugsbakk.name>
On Sat, Oct 14, 2023 at 11:02:38PM +0200, Kristoffer Haugsbakk wrote:
> Die gracefully when `git grep --no-index` is run outside of a Git
> repository and the path is outside the directory tree.
>
> If you are not in a Git repository and say:
>
> git grep --no-index search ..
>
> You trigger a `BUG`:
>
> BUG: environment.c:213: git environment hasn't been setup
> Aborted (core dumped)
>
> Because `..` is a valid path which is treated as a pathspec. Then
> `pathspec` figures out that it is not in the current directory tree. The
> `BUG` is triggered when `pathspec` tries to advice the user about the path
> to the (non-existing) repository.
Is it even reasonable for "grep --no-index" to care about leaving the
tree in the first place? That is, is there a reason we should not allow:
git grep --no-index foo ../bar
? And if we do want to care, there is a weirdness here that even with
your patch, we check to see if the file exists:
$ git grep --no-index foo ../does-exist
fatal: '../does-exist' is outside the directory tree
$ git grep --no-index foo ../does-not-exist
fatal: ../does-not-exist: no such path in the working tree.
If we want to avoid leaving the current directory, then I think we need
to be checking much sooner (but again, I would argue that it is not
worth caring about in no-index mode).
I do think your patch does not make anything worse (and indeed makes the
error output much better). So I do not mind it in the meantime. But I
have a feeling that we'd end up reverting it as part of the fix for the
larger issue.
-Peff
^ permalink raw reply
* [PATCH v2 0/3] bugreport: include +i in outfile suffix as needed
From: Jacob Stopak @ 2023-10-15 3:42 UTC (permalink / raw)
To: git; +Cc: Jacob Stopak, Junio C Hamano, Dragan Simic, Kristoffer Haugsbakk
In-Reply-To: <20231014040101.8333-1-jacob@initialcommit.io>
Update git bugreport to allow creation of multiple bugreports with
default settings during a given minute interval.
Address several edge cases where users might run git bugreport multiple
times within a minute and expect multiple reports to be created.
Handle these cases by checking to see if a file with the default
timestamped filename already exists, and if so, appending a '+1' value
to the filename suffix. Keep doing so until a unique filename is reached
or '+9' is reached. At this point, if uniqueness is still not found,
the previous error that a file with the given name already exists.
If the --diagnose flag is supplied, apply the same '+i' incremented
filename suffix to the diagnostics zip file.
Reorder the code block that creates the diagnostics zip file so it isn't
created in the event the bugreport itself isn't created due to an error.
Jacob Stopak (3):
bugreport: include +i in outfile suffix as needed
bugreport: match diagnostics filename with report
bugreport: don't create --diagnose zip w/o report
builtin/bugreport.c | 54 ++++++++++++++++++++++++++++++---------------
1 file changed, 36 insertions(+), 18 deletions(-)
base-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09
--
2.42.0.298.gd89efca819.dirty
^ permalink raw reply
* [PATCH v2 1/3] bugreport: include +i in outfile suffix as needed
From: Jacob Stopak @ 2023-10-15 3:42 UTC (permalink / raw)
To: git; +Cc: Jacob Stopak
In-Reply-To: <20231015034238.100675-1-jacob@initialcommit.io>
When the -s flag is absent, git bugreport includes the current hour and
minute values in the default bugreport filename (and diagnostics zip
filename if --diagnose is supplied).
If a user runs the bugreport command more than once within a calendar
minute, a filename conflict with an existing file occurs and the program
errors, since the new output filename was already used for the previous
file. If the user waits anywhere from 1 to 60 seconds (depending on
_when during the calendar minute_ the first command was run) the command
works again with no error since the default filename is now unique, and
multiple bug reports are able to be created with default settings.
This is a minor thing but can cause confusion especially for first time
users of the bugreport command, who are likely to run it multiple times
in quick succession to learn how it works, (like I did).
Add a '+i' into the bugreport filename suffix to make the filename
unique, where 'i' is an integer starting at 1 and able to grow up to 9
in the unlikely event a user runs the command 9 times in a single
minute. This leads to default output filenames like:
git-bugreport-%Y-%m-%d-%H%M+1.txt
git-bugreport-%Y-%m-%d-%H%M+2.txt
...
git-bugreport-%Y-%m-%d-%H%M+9.txt
This means the user will end up with multiple bugreport files being
created if they run the command multiple times quickly, but that feels
more intuitive and consistent than an error arbitrarily occuring within
a calendar minute, especially given that the time window in which the
error currently occurs is variable as described above.
Signed-off-by: Jacob Stopak <jacob@initialcommit.io>
---
builtin/bugreport.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index d2ae5c305d..71ee7d7f4b 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -3,7 +3,6 @@
#include "editor.h"
#include "gettext.h"
#include "parse-options.h"
-#include "strbuf.h"
#include "help.h"
#include "compat/compiler.h"
#include "hook.h"
@@ -11,6 +10,7 @@
#include "diagnose.h"
#include "object-file.h"
#include "setup.h"
+#include "dir.h"
static void get_system_info(struct strbuf *sys_info)
{
@@ -101,12 +101,13 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
{
struct strbuf buffer = STRBUF_INIT;
struct strbuf report_path = STRBUF_INIT;
+ struct strbuf option_suffix = STRBUF_INIT;
+ struct strbuf default_option_suffix = STRBUF_INIT;
int report = -1;
time_t now = time(NULL);
struct tm tm;
enum diagnose_mode diagnose = DIAGNOSE_NONE;
char *option_output = NULL;
- char *option_suffix = "%Y-%m-%d-%H%M";
const char *user_relative_path = NULL;
char *prefixed_filename;
size_t output_path_len;
@@ -118,11 +119,14 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
PARSE_OPT_OPTARG, option_parse_diagnose),
OPT_STRING('o', "output-directory", &option_output, N_("path"),
N_("specify a destination for the bugreport file(s)")),
- OPT_STRING('s', "suffix", &option_suffix, N_("format"),
+ OPT_STRING('s', "suffix", &option_suffix.buf, N_("format"),
N_("specify a strftime format suffix for the filename(s)")),
OPT_END()
};
+ strbuf_addstr(&default_option_suffix, "%Y-%m-%d-%H%M");
+ strbuf_addstr(&option_suffix, default_option_suffix.buf);
+
argc = parse_options(argc, argv, prefix, bugreport_options,
bugreport_usage, 0);
@@ -134,9 +138,20 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
output_path_len = report_path.len;
strbuf_addstr(&report_path, "git-bugreport-");
- strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
+ strbuf_addftime(&report_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0);
strbuf_addstr(&report_path, ".txt");
+ if (strbuf_cmp(&option_suffix, &default_option_suffix) == 0) {
+ int i = 1;
+ int pos = report_path.len - 4;
+ while (file_exists(report_path.buf) && i < 10) {
+ if (i > 1)
+ strbuf_remove(&report_path, pos, 2);
+ strbuf_insertf(&report_path, pos, "+%d", i);
+ i++;
+ }
+ }
+
switch (safe_create_leading_directories(report_path.buf)) {
case SCLD_OK:
case SCLD_EXISTS:
@@ -151,7 +166,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
struct strbuf zip_path = STRBUF_INIT;
strbuf_add(&zip_path, report_path.buf, output_path_len);
strbuf_addstr(&zip_path, "git-diagnostics-");
- strbuf_addftime(&zip_path, option_suffix, localtime_r(&now, &tm), 0, 0);
+ strbuf_addftime(&zip_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0);
strbuf_addstr(&zip_path, ".zip");
if (create_diagnostics_archive(&zip_path, diagnose))
@@ -188,6 +203,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
free(prefixed_filename);
strbuf_release(&buffer);
+ strbuf_release(&default_option_suffix);
ret = !!launch_editor(report_path.buf, NULL, NULL);
strbuf_release(&report_path);
--
2.42.0.298.gd89efca819.dirty
^ permalink raw reply related
* [PATCH v2 2/3] bugreport: match diagnostics filename with report
From: Jacob Stopak @ 2023-10-15 3:42 UTC (permalink / raw)
To: git; +Cc: Jacob Stopak
In-Reply-To: <20231015034238.100675-1-jacob@initialcommit.io>
When using the --diagnose flag, match the diagnostics zip filename with
the bugreport filename in the scenario where '+i' syntax is used.
Signed-off-by: Jacob Stopak <jacob@initialcommit.io>
---
builtin/bugreport.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 71ee7d7f4b..573d270677 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -112,6 +112,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
char *prefixed_filename;
size_t output_path_len;
int ret;
+ int i = 1;
const struct option bugreport_options[] = {
OPT_CALLBACK_F(0, "diagnose", &diagnose, N_("mode"),
@@ -142,7 +143,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
strbuf_addstr(&report_path, ".txt");
if (strbuf_cmp(&option_suffix, &default_option_suffix) == 0) {
- int i = 1;
int pos = report_path.len - 4;
while (file_exists(report_path.buf) && i < 10) {
if (i > 1)
@@ -167,6 +167,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
strbuf_add(&zip_path, report_path.buf, output_path_len);
strbuf_addstr(&zip_path, "git-diagnostics-");
strbuf_addftime(&zip_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0);
+ if (i > 1) strbuf_addf(&zip_path, "+%d", i-1);
strbuf_addstr(&zip_path, ".zip");
if (create_diagnostics_archive(&zip_path, diagnose))
--
2.42.0.298.gd89efca819.dirty
^ permalink raw reply related
* [PATCH v2 3/3] bugreport: don't create --diagnose zip w/o report
From: Jacob Stopak @ 2023-10-15 3:42 UTC (permalink / raw)
To: git; +Cc: Jacob Stopak
In-Reply-To: <20231015034238.100675-1-jacob@initialcommit.io>
Prevent the diagnostics zip file from being created when the bugreport
itself is not created due to an error.
Signed-off-by: Jacob Stopak <jacob@initialcommit.io>
---
builtin/bugreport.c | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 573d270677..91567806c9 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -161,21 +161,6 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
report_path.buf);
}
- /* Prepare diagnostics, if requested */
- if (diagnose != DIAGNOSE_NONE) {
- struct strbuf zip_path = STRBUF_INIT;
- strbuf_add(&zip_path, report_path.buf, output_path_len);
- strbuf_addstr(&zip_path, "git-diagnostics-");
- strbuf_addftime(&zip_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0);
- if (i > 1) strbuf_addf(&zip_path, "+%d", i-1);
- strbuf_addstr(&zip_path, ".zip");
-
- if (create_diagnostics_archive(&zip_path, diagnose))
- die_errno(_("unable to create diagnostics archive %s"), zip_path.buf);
-
- strbuf_release(&zip_path);
- }
-
/* Prepare the report contents */
get_bug_template(&buffer);
@@ -202,6 +187,22 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
fprintf(stderr, _("Created new report at '%s'.\n"),
user_relative_path);
+ /* Prepare diagnostics, if requested */
+ if (diagnose != DIAGNOSE_NONE) {
+ struct strbuf zip_path = STRBUF_INIT;
+ strbuf_add(&zip_path, report_path.buf, output_path_len);
+ strbuf_addstr(&zip_path, "git-diagnostics-");
+ strbuf_addftime(&zip_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0);
+ if (i > 1) strbuf_addf(&zip_path, "+%d", i-1);
+ strbuf_addstr(&zip_path, ".zip");
+
+ if (create_diagnostics_archive(&zip_path, diagnose))
+ die_errno(_("unable to create diagnostics archive %s"), zip_path.buf);
+
+ strbuf_release(&zip_path);
+ }
+
+
free(prefixed_filename);
strbuf_release(&buffer);
strbuf_release(&default_option_suffix);
--
2.42.0.298.gd89efca819.dirty
^ permalink raw reply related
* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Elijah Newren @ 2023-10-15 6:44 UTC (permalink / raw)
To: Josh Triplett; +Cc: Junio C Hamano, Sebastian Thiel, git
In-Reply-To: <ZSouSI_zPusOefsv@localhost>
Hi,
On Fri, Oct 13, 2023 at 11:00 PM Josh Triplett <josh@joshtriplett.org> wrote:
>
> On Tue, Oct 10, 2023 at 10:02:20AM -0700, Junio C Hamano wrote:
> > Sebastian Thiel <sebastian.thiel@icloud.com> writes:
> >
> > > I'd like to propose adding a new standard gitattribute "precious".
> >
> > ;-).
> >
> > Over the years, I've seen many times scenarios that would have been
> > helped if we had not just "tracked? ignored? unignored?" but also
> > the fourth kind [*]. The word "ignored" (or "excluded") has always
> > meant "not tracked, not to be tracked, and expendable" to Git, and
> > "ignored but unexpendable" class was missing. I even used the term
> > "precious" myself in those discussions. At the concept level, I
> > support the effort 100%, but as always, the devil will be in the
> > details.
>
> "I've already wanted this for years" is, honestly, the best response we
> could *possibly* have hoped for.
>
> > Scenarios that people wished for "precious" traditionally have been
> >
> > * You are working on 'master'. You have in your .gitignore or
> > .git/info/exclude a line to ignore path A, and have random
> > scribbles in a throw-away file there. There is another branch
> > 'seen', where they added some tracked contents at path A/B. You
> > do "git checkout seen" and your file A that is an expendable file,
> > because it is listed as ignored in .git/info/exclude, is removed
> > to make room for creating A/B.
>
> Ouch, I hadn't even thought about the issue of branch-switching
> overwriting a file like that, but that's another great reason to have
> "precious". (I've been thinking about "precious" as primarily to protect
> files like `.config`, where they'd be unlikely to be checked in on any
> branch because they have an established purpose in the project. Though,
> of course, people *do* sometimes check in `.config` files in
> special-purpose branches that aren't meant for upstreaming.)
If we're going to implement precious files, I think we should take a
step back and figure out what parts of the system are affected. It's
way more than branch switching and `git clean`. Some notes (including
some useful implementation pointers):
A) You will probably learn a lot and get a leg up on the
implementation by grepping for "preserve_ignored"; lots of the
plumbing has already been created related to this.
B) checkout has a --no-overwrite-ignore, which for checkout
operations, essentially turns all ignored files into precious files.
B1) The code behind --no-overwrite-ignore will probably be helpful in
your implementation of precious files
B2) What happens to this --no-overwrite-ignore option?
Deprecate/remove it after adding precious files, since precious files
now serve that purpose? Keep the flag anyway? What happens to the
docs around the flag?
B3) If we keep --no-overwrite-ignore, do we also need to add a
--overwrite-precious option to allow those to be similarly tweaked?
C) merge has a --no-overwrite-ignore option, which for supported merge
operations (which are sadly very few), essentially turns all ignored
files into precious files.
C1) Same comments as A1-A3 but for merges
C2) Sadly, merge's --no-overwrite-ignore is almost garbage.
builtin/merge.c will only pass this option to the "fast-forward" merge
backend, causing any other type of merge to overwrite ignored files
despite any such flag.
C3) most merge backends don't have logic to handle
--no-overwrite-ignore even if they were passed it, and would need
explicit support added.
C4) merge-ort would only essentially need a one-liner; it basically
has the code in place and a comment but the flag was never plumbed
through
C5) it'd be a herculean effort to support this with merge-recursive,
and a sisyphean effort to attempt to maintain. Deprecating and
removing merge-recursive is probably a better option
C6) merge-resolve and merge-octopus could probably be handled
automatically by ensuring `git read-tree` gained support for it.
C7) there'd be no way to ensure user-written merge algorithms would
support it, but that's kind of a general problem with user-written
anythings
D) git ls-files would need to have a way to query for precious files,
much as it can currently be used to query for ignored files (or
tracked files, or conflicted file, or skip-worktree files, or...).
Backward compatibility questions arise about whether precious files
should appear in `git ls-files -o` output.
E) git status has an --ignored option, with multiple flags for
controlling it. We'll likely need more flags to be able to pick out
precious files.
F) We'd probably need to look through several other commands and look
at what they need for special handling. e.g., am, stash, reset. I
suspect stash will be a particularly sore point, as its unfortunate
design of implementing shell in C code and attempting to decompose the
command in terms of other high-level commands is basically a leaky
abstraction that is very likely to be susceptible to edge and corner
cases here. (In fact, I think I may have left some of the issues for
untracked/ignored files in stash broken when I was fixing such
problems for other commands.)
G) Documentation. Commands like `git reset --hard`, `git checkout
-f`, and `read-tree --reset` are documented to nuke untracked files
specifically because we expect most commands to preserve untracked
files. These would need to mention that "precious" files are also
nuked (or, if we don't nuke them, why precious-and-ignored files are
more precious than untracked files).
H) Design of `reset --hard`. As per
https://lore.kernel.org/git/xmqqr1e2ejs9.fsf@gitster.g/, `git reset
--hard` is a little funny and we have thought about changing it. Will
the addition of "precious" objects provide more impetus to do so, and
should a migration story be part of such a new feature?
I) Although git's stated behavior was to nuke ignored files and
protect untracked files, just a couple years ago we found _many_ bugs
where Git didn't do that. A series was pushed to fix most of those[1]
(incidentally, the same series the introduced the preserve_ignored
flag I pointed you to earlier), but it left a few things
commented/broken. The cover letter and some emails in the thread also
discussed in more detail some of the ramifications around a "precious"
setting. It may be worth reading to catch other things to cover and
think about.
J) To implement this feature, you're going to have to touch dir.c.
Good luck with that. (Seriously, good luck. The more people that
touch it that aren't me, the less I'll be pinged/queried about that
monstrosity.)
> > In any case, the "precious" paths are expected to be small minority
> > of what people never want to "git add" or "git commit", so coming up
> > with a special syntax to be used in .gitignore, even if that special
> > syntax is ugly and cumbersome to type, would be perfectly OK.
>
> [Following up both to this and to Sebastian's response.]
>
> One potentially important question: should the behavior of old git be to
> treat precious files as ignored, or as not-ignored? If the syntax were
> something like
>
> $.config
>
> then old git would treat the file as not-ignored. If the syntax were
> something like
>
> $precious
> .config
>
> then old git would treat the file as ignored.
>
> Seems like it would be obtrusive if `git status` in old git showed the
> file, and `git add .` in old git added it.
A very good set of questions, along similar lines as the question
about `git ls-files -o` handling.
Anyway, I'm a bit worried after digging up and dumping all these
concerns on you, that it'll sound like I'm trying to bury the feature
and discourage folks. In the past I have been against this at times,
but mostly because it looked like lots of work, I didn't want to touch
dir.c anymore, and I was worried we'd add a bunch more edge & corner
cases to the code (when we already had plenty with our more limited
number of file types). In a way, the preserve_ignored stuff kind of
made this a lot more reasonable for us to switch to. But I do still
think it's a fair amount of work, and I am kind of worried about
potential new edge and corner case.
Hope that helps,
Elijah
^ permalink raw reply
* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Sebastian Thiel @ 2023-10-15 7:33 UTC (permalink / raw)
To: Elijah Newren; +Cc: Josh Triplett, Junio C Hamano, git
In-Reply-To: <CABPp-BEg6vxiUpcJAG_=KB_sTrVgCF19JZh-+ZGCTPXdbo9ekg@mail.gmail.com>
Thanks so much Elijah for your eye-opening response. Thus far I was both
naive and ignorant about the complexity of the matter, and also never
asked the question as to why it wasn't tackled earlier since it came up
already.
Since so many areas of git are affected by precious files, it seems that
rolling it out with everything working is unrealistic and I wonder if
it even had to be behind a feature toggle at first.
A particularly interesting question brought up here also was the question
of what's more important: untracked files, or precious files? Are they
effectively treated the same, or is there a difference?
In any case, it seems easiest to set the desired syntax for such a
feature and/or validate it, and then devise a plan for how it could all
come together.
On 15 Oct 2023, at 8:44, Elijah Newren wrote:
> Hi,
>
> On Fri, Oct 13, 2023 at 11:00 PM Josh Triplett <josh@joshtriplett.org> wrote:
>>
>> On Tue, Oct 10, 2023 at 10:02:20AM -0700, Junio C Hamano wrote:
>>> Sebastian Thiel <sebastian.thiel@icloud.com> writes:
>>>
>>>> I'd like to propose adding a new standard gitattribute "precious".
>>>
>>> ;-).
>>>
>>> Over the years, I've seen many times scenarios that would have been
>>> helped if we had not just "tracked? ignored? unignored?" but also
>>> the fourth kind [*]. The word "ignored" (or "excluded") has always
>>> meant "not tracked, not to be tracked, and expendable" to Git, and
>>> "ignored but unexpendable" class was missing. I even used the term
>>> "precious" myself in those discussions. At the concept level, I
>>> support the effort 100%, but as always, the devil will be in the
>>> details.
>>
>> "I've already wanted this for years" is, honestly, the best response we
>> could *possibly* have hoped for.
>>
>>> Scenarios that people wished for "precious" traditionally have been
>>>
>>> * You are working on 'master'. You have in your .gitignore or
>>> .git/info/exclude a line to ignore path A, and have random
>>> scribbles in a throw-away file there. There is another branch
>>> 'seen', where they added some tracked contents at path A/B. You
>>> do "git checkout seen" and your file A that is an expendable file,
>>> because it is listed as ignored in .git/info/exclude, is removed
>>> to make room for creating A/B.
>>
>> Ouch, I hadn't even thought about the issue of branch-switching
>> overwriting a file like that, but that's another great reason to have
>> "precious". (I've been thinking about "precious" as primarily to protect
>> files like `.config`, where they'd be unlikely to be checked in on any
>> branch because they have an established purpose in the project. Though,
>> of course, people *do* sometimes check in `.config` files in
>> special-purpose branches that aren't meant for upstreaming.)
>
> If we're going to implement precious files, I think we should take a
> step back and figure out what parts of the system are affected. It's
> way more than branch switching and `git clean`. Some notes (including
> some useful implementation pointers):
>
> A) You will probably learn a lot and get a leg up on the
> implementation by grepping for "preserve_ignored"; lots of the
> plumbing has already been created related to this.
>
> B) checkout has a --no-overwrite-ignore, which for checkout
> operations, essentially turns all ignored files into precious files.
> B1) The code behind --no-overwrite-ignore will probably be helpful in
> your implementation of precious files
> B2) What happens to this --no-overwrite-ignore option?
> Deprecate/remove it after adding precious files, since precious files
> now serve that purpose? Keep the flag anyway? What happens to the
> docs around the flag?
> B3) If we keep --no-overwrite-ignore, do we also need to add a
> --overwrite-precious option to allow those to be similarly tweaked?
>
> C) merge has a --no-overwrite-ignore option, which for supported merge
> operations (which are sadly very few), essentially turns all ignored
> files into precious files.
> C1) Same comments as A1-A3 but for merges
> C2) Sadly, merge's --no-overwrite-ignore is almost garbage.
> builtin/merge.c will only pass this option to the "fast-forward" merge
> backend, causing any other type of merge to overwrite ignored files
> despite any such flag.
> C3) most merge backends don't have logic to handle
> --no-overwrite-ignore even if they were passed it, and would need
> explicit support added.
> C4) merge-ort would only essentially need a one-liner; it basically
> has the code in place and a comment but the flag was never plumbed
> through
> C5) it'd be a herculean effort to support this with merge-recursive,
> and a sisyphean effort to attempt to maintain. Deprecating and
> removing merge-recursive is probably a better option
> C6) merge-resolve and merge-octopus could probably be handled
> automatically by ensuring `git read-tree` gained support for it.
> C7) there'd be no way to ensure user-written merge algorithms would
> support it, but that's kind of a general problem with user-written
> anythings
>
> D) git ls-files would need to have a way to query for precious files,
> much as it can currently be used to query for ignored files (or
> tracked files, or conflicted file, or skip-worktree files, or...).
> Backward compatibility questions arise about whether precious files
> should appear in `git ls-files -o` output.
>
> E) git status has an --ignored option, with multiple flags for
> controlling it. We'll likely need more flags to be able to pick out
> precious files.
>
> F) We'd probably need to look through several other commands and look
> at what they need for special handling. e.g., am, stash, reset. I
> suspect stash will be a particularly sore point, as its unfortunate
> design of implementing shell in C code and attempting to decompose the
> command in terms of other high-level commands is basically a leaky
> abstraction that is very likely to be susceptible to edge and corner
> cases here. (In fact, I think I may have left some of the issues for
> untracked/ignored files in stash broken when I was fixing such
> problems for other commands.)
>
> G) Documentation. Commands like `git reset --hard`, `git checkout
> -f`, and `read-tree --reset` are documented to nuke untracked files
> specifically because we expect most commands to preserve untracked
> files. These would need to mention that "precious" files are also
> nuked (or, if we don't nuke them, why precious-and-ignored files are
> more precious than untracked files).
>
> H) Design of `reset --hard`. As per
> https://lore.kernel.org/git/xmqqr1e2ejs9.fsf@gitster.g/, `git reset
> --hard` is a little funny and we have thought about changing it. Will
> the addition of "precious" objects provide more impetus to do so, and
> should a migration story be part of such a new feature?
>
> I) Although git's stated behavior was to nuke ignored files and
> protect untracked files, just a couple years ago we found _many_ bugs
> where Git didn't do that. A series was pushed to fix most of those[1]
> (incidentally, the same series the introduced the preserve_ignored
> flag I pointed you to earlier), but it left a few things
> commented/broken. The cover letter and some emails in the thread also
> discussed in more detail some of the ramifications around a "precious"
> setting. It may be worth reading to catch other things to cover and
> think about.
>
> J) To implement this feature, you're going to have to touch dir.c.
> Good luck with that. (Seriously, good luck. The more people that
> touch it that aren't me, the less I'll be pinged/queried about that
> monstrosity.)
>
>
>>> In any case, the "precious" paths are expected to be small minority
>>> of what people never want to "git add" or "git commit", so coming up
>>> with a special syntax to be used in .gitignore, even if that special
>>> syntax is ugly and cumbersome to type, would be perfectly OK.
>>
>> [Following up both to this and to Sebastian's response.]
>>
>> One potentially important question: should the behavior of old git be to
>> treat precious files as ignored, or as not-ignored? If the syntax were
>> something like
>>
>> $.config
>>
>> then old git would treat the file as not-ignored. If the syntax were
>> something like
>>
>> $precious
>> .config
>>
>> then old git would treat the file as ignored.
>>
>> Seems like it would be obtrusive if `git status` in old git showed the
>> file, and `git add .` in old git added it.
>
> A very good set of questions, along similar lines as the question
> about `git ls-files -o` handling.
>
>
> Anyway, I'm a bit worried after digging up and dumping all these
> concerns on you, that it'll sound like I'm trying to bury the feature
> and discourage folks. In the past I have been against this at times,
> but mostly because it looked like lots of work, I didn't want to touch
> dir.c anymore, and I was worried we'd add a bunch more edge & corner
> cases to the code (when we already had plenty with our more limited
> number of file types). In a way, the preserve_ignored stuff kind of
> made this a lot more reasonable for us to switch to. But I do still
> think it's a fair amount of work, and I am kind of worried about
> potential new edge and corner case.
>
>
> Hope that helps,
> Elijah
^ permalink raw reply
* Re: [PATCH] grep: die gracefully when outside repository
From: Kristoffer Haugsbakk @ 2023-10-15 8:00 UTC (permalink / raw)
To: Jeff King; +Cc: git, ks1322 ks1322
In-Reply-To: <20231015032636.GC554702@coredump.intra.peff.net>
On Sun, Oct 15, 2023, at 05:26, Jeff King wrote:
> Is it even reasonable for "grep --no-index" to care about leaving the
> tree in the first place? That is, is there a reason we should not allow:
>
> git grep --no-index foo ../bar
>
> ?
On second thought yeah, it doesn't make sense. We are outside of any
repository already so what does it matter where the file is relative to
the current working directory?
It seems that `pathspec.c:init_pathspec_item` should let you through in
this case.
--
Kristoffer Haugsbakk
^ permalink raw reply
* [PATCH] typo: fix the typo 'neeed' into 'needed' in the comment under merge-ort.c
From: Wangchangxin via GitGitGadget @ 2023-10-15 14:08 UTC (permalink / raw)
To: git; +Cc: Wangchangxin, foril
From: foril <1571825323@qq.com>
Signed-off-by: 王常新 (Wang Changxin) <foril@foril.space>
---
typo: fix the typo 'neeed' into 'needed' in the comment under merge-o…
the comments on line 2039 under merge-ort.c should be :
this is needed if we have content merges of content merges rather than
this is neeed if we have content merges of content merges
fix the typo
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1592%2FforiLLL%2Fcomment_patch-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1592/foriLLL/comment_patch-v1
Pull-Request: https://github.com/git/git/pull/1592
merge-ort.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/merge-ort.c b/merge-ort.c
index 7857ce9fbd1..aee6f7d8173 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2036,7 +2036,7 @@ static int handle_content_merge(struct merge_options *opt,
* the three blobs to merge on various sides of history.
*
* extra_marker_size is the amount to extend conflict markers in
- * ll_merge; this is neeed if we have content merges of content
+ * ll_merge; this is needed if we have content merges of content
* merges, which happens for example with rename/rename(2to1) and
* rename/add conflicts.
*/
base-commit: a9ecda2788e229afc9b611acaa26d0d9d4da53ed
--
gitgitgadget
^ permalink raw reply related
* Re: [RFC] Define "precious" attribute and support it in `git clean`
From: Junio C Hamano @ 2023-10-15 16:31 UTC (permalink / raw)
To: Sebastian Thiel; +Cc: Elijah Newren, Josh Triplett, git
In-Reply-To: <B088FC28-BE30-424D-9CDD-7A53EDFC1710@icloud.com>
Sebastian Thiel <sebastian.thiel@icloud.com> writes:
> A particularly interesting question brought up here also was the question
> of what's more important: untracked files, or precious files? Are they
> effectively treated the same, or is there a difference?
Think of it this way. There are two orthogonal axes.
(1) Are you a candidate to be tracked, even though you are not
tracked right now?
(2) Should you be kept and make an operation fail that wants to
remove you to make room?
For untracked files, both are "Yes". As we already saw in the long
discussion, precious files are "not to be added and not to be
clobbered", so you'd answer "No" and "Yes" [*].
In other words, both are equally protected from getting cloberred.
Side note: for completeness, for ignored files, the answers are
"No", and "No". The introduction of "precious" class makes a
combination "No-Yes" that hasn't been possible so far.
Elijah, thanks for doing a very good job of creating a catalog of
kludges we accumulated over the years for the lack of proper support
for the precious paths. I think they should be kept for backward
compatibility, but for new users they should not have to learn any
of them once we have the support for precious paths.
^ permalink raw reply
* Re: [PATCH] Add x64 SHA-NI implementation to sha1 and sha256 in git ?
From: Beat Bolli @ 2023-10-15 16:37 UTC (permalink / raw)
To: brian m. carlson, Feiyang Xue, git
In-Reply-To: <ZSmxFopwJyBGmZY-@tapette.crustytoothpaste.net>
Hi
On 13.10.23 23:05, brian m. carlson wrote:
> On 2023-10-12 at 20:04:47, Feiyang Xue wrote:
>> Looking for comments on if this is a good idea to add SHA-NI option to git,
>> And if so, what'd be a good implementation.
>>
>> The attached patch is more of a proof of concept, using a sha-ni example
>> found on internet and have it tapped into git's "hash-ll.h" when it sees make
>> flags "SHA1_SHANI=1" and/or "SHA256_SHANI=1" for sha1/sha256 respectively.
>>
>> "git hash-object" is about 3-ish times faster for me
>
> You almost certainly don't want to use SHA-1 using native instructions
> because it doesn't detect collisions, unlike the default implementation
> (SHA-1-DC). Since SHA-1 collisions are relatively cheap (less than USD
> 45,000) to make, not detecting collisions would be a security issue
> worthy of a CVE.
>
> It's fine for SHA-256, but see below.
>
>> There are few topics that i'm uncertain about.
>>
>> 1. Is it good idea to have machine-specific asm codes in git. I read there was
>> commit fd1ec82547 which removed assembly implementation for PPC_SHA1. Does that
>> imply maintainers doesn't want machine-specific assembly in source?
>
> I'd prefer we didn't.
>
> Nettle has SHA-NI extensions on systems that support it, and so does
> OpenSSL. OpenSSL can't be used by distros for licensing reasons, but
> Nettle can, and both of those libraries automatically detect the best
> code to use based on the chip. One of those libraries is almost always
> going to be linked into Git at some point because of libcurl anyway.
>
> If we ship the code, then someone has to maintain it, and I don't know
> if we have any assembly experts. We might also have a bug that produced
> incorrect results, which would be pretty disastrous for the affected
> users. It's much better for maintainability if we push that off onto
> the cryptographic library maintainers who are much more competent in
> that regard than I am (I will not speak for others) and let distro
> maintainers simply link the appropriate library, which, as I said above,
> they're already effectively doing.
In addition to all the valid points brian makes, the patch uses a
non-standard assembler (yasm, see http://yasm.tortall.net/) that's not
part of the default GNU toolchain.
Cheers, Beat
^ permalink raw reply
* Re: [PATCH 21/20] t5319: make corrupted large-offset test more robust
From: Junio C Hamano @ 2023-10-15 17:04 UTC (permalink / raw)
To: Jeff King; +Cc: git, Taylor Blau
In-Reply-To: <20231015031732.GB554702@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Yeah, I had a similar thought process. I sort of assume anybody working
> on git.git is capable of turning to "git blame" in a situation like
> this. But maybe:
>
> # using only %(objectsize) is important here; run "git blame"
> # on these lines for more details
>
> would spell it out more clearly.
The comment was about "the commit" being not so clear which commit.
"see the message of the commit that added this comment" would have
been perfectly fine and they are not required to use "blame" if they
don't like it.
^ permalink raw reply
* Re: [PATCH] bugreport: add 'seconds' to default outfile name
From: Junio C Hamano @ 2023-10-15 17:06 UTC (permalink / raw)
To: Jacob Stopak; +Cc: git
In-Reply-To: <ZStWB1/LX7cTbVGr.jacob@initialcommit.io>
Jacob Stopak <jacob@initialcommit.io> writes:
> Lol! By saying "calendar" I mean "falling on the official boundaries
> of", like 11:15:00 - 11:16:00. Unlike the time between 11:15:30 -
> 11:16:30 which is also a minute, but it's not a "calendar" minute
> because it overlaps into the next minute. I guess in this case it's more
> of a "clock" minute than a "calendar" minute though ':D... I guess
> "calendar" terminology is used more for months/years...
People use "calendar" days usually in order to to differenciate it
with "business" days. It may take your package to come cross
country and take 5 business days, but if you ship it out on Friday,
it will not arrive by Wednesday.
^ permalink raw reply
* RE: [PATCH] Add x64 SHA-NI implementation to sha1 and sha256 in git ?
From: rsbecker @ 2023-10-15 17:30 UTC (permalink / raw)
To: 'Beat Bolli', 'brian m. carlson',
'Feiyang Xue', git
In-Reply-To: <fd59abd6-49a5-4f6a-8d6c-c608cd3350f7@drbeat.li>
On Sunday, October 15, 2023 12:37 PM, Beat Bolli wrote:
>On 13.10.23 23:05, brian m. carlson wrote:
>> On 2023-10-12 at 20:04:47, Feiyang Xue wrote:
>>> Looking for comments on if this is a good idea to add SHA-NI option
>>> to git, And if so, what'd be a good implementation.
>>>
>>> The attached patch is more of a proof of concept, using a sha-ni
>>> example found on internet and have it tapped into git's "hash-ll.h"
>>> when it sees make flags "SHA1_SHANI=1" and/or "SHA256_SHANI=1" for
>sha1/sha256 respectively.
>>>
>>> "git hash-object" is about 3-ish times faster for me
>>
>> You almost certainly don't want to use SHA-1 using native instructions
>> because it doesn't detect collisions, unlike the default
>> implementation (SHA-1-DC). Since SHA-1 collisions are relatively
>> cheap (less than USD
>> 45,000) to make, not detecting collisions would be a security issue
>> worthy of a CVE.
>>
>> It's fine for SHA-256, but see below.
>>
>>> There are few topics that i'm uncertain about.
>>>
>>> 1. Is it good idea to have machine-specific asm codes in git. I read
>>> there was commit fd1ec82547 which removed assembly implementation for
>>> PPC_SHA1. Does that imply maintainers doesn't want machine-specific assembly
>in source?
>>
>> I'd prefer we didn't.
>>
>> Nettle has SHA-NI extensions on systems that support it, and so does
>> OpenSSL. OpenSSL can't be used by distros for licensing reasons, but
>> Nettle can, and both of those libraries automatically detect the best
>> code to use based on the chip. One of those libraries is almost
>> always going to be linked into Git at some point because of libcurl anyway.
>>
>> If we ship the code, then someone has to maintain it, and I don't know
>> if we have any assembly experts. We might also have a bug that
>> produced incorrect results, which would be pretty disastrous for the
>> affected users. It's much better for maintainability if we push that
>> off onto the cryptographic library maintainers who are much more
>> competent in that regard than I am (I will not speak for others) and
>> let distro maintainers simply link the appropriate library, which, as
>> I said above, they're already effectively doing.
>
>In addition to all the valid points brian makes, the patch uses a non-standard
>assembler (yasm, see http://yasm.tortall.net/) that's not part of the default GNU
>toolchain.
Aside from that, not everyone uses the GNU toolchain. I would suggest that if someone needs ASM support for SHA-1, they could reimplement SHA-1-DC using rotate ASM primitives instead of directly using SHA1 assembly, but I question whether that is significantly faster than having the compiler optimizer generate the equivalent rotates. Still, maintaining any ASM code requires out of box skillsets that are unlikely sustainable.
--Randall
^ permalink raw reply
* Re: [PATCH v2 1/3] bugreport: include +i in outfile suffix as needed
From: Junio C Hamano @ 2023-10-15 17:36 UTC (permalink / raw)
To: Jacob Stopak; +Cc: git
In-Reply-To: <20231015034238.100675-2-jacob@initialcommit.io>
Jacob Stopak <jacob@initialcommit.io> writes:
> When the -s flag is absent, git bugreport includes the current hour and
> minute values in the default bugreport filename (and diagnostics zip
> filename if --diagnose is supplied).
>
> If a user runs the bugreport command more than once within a calendar
> minute, a filename conflict with an existing file occurs and the program
> errors, since the new output filename was already used for the previous
> file. If the user waits anywhere from 1 to 60 seconds (depending on
> _when during the calendar minute_ the first command was run) the command
Drop "calendar" here (see below). "If the user waits from running
the command within the same minute" may be easier to understand than
"from 1 to 60 seconds"; after all, the user does not have to wait
for more than 0.5 seconds if the previous attempt was within 0.5
seconds from the minute boundary.
> works again with no error since the default filename is now unique, and
> multiple bug reports are able to be created with default settings.
>
> This is a minor thing but can cause confusion especially for first time
> users of the bugreport command, who are likely to run it multiple times
> in quick succession to learn how it works, (like I did).
Perhaps we should refine the error message we give in this case and
we are done, then?
$ GIT_EDITOR=: git bugreport ; GIT_EDITOR=: git bugreport
Created new report at 'git-bugreport-2023-10-15-1008.txt'.
fatal: unable to create 'git-bugreport-2023-10-15-1008.txt': File exists
The second message can be a bit more friendly and suggest removing
the stale file.
> Add a '+i' into the bugreport filename suffix to make the filename
> unique, where 'i' is an integer starting at 1 and able to grow up to 9
> in the unlikely event a user runs the command 9 times in a single
> minute. This leads to default output filenames like:
What downside do you see in using 2023-10-15-1008+10 after you tried
9 of them? The code to limit the upper bound smells like a wasted
effort that helps nobody in practice because it is "unlikely".
And limiting the upper bound also means you now have to have extra
code to deal with "we ran out---error out and help the user how to
recover" anyway.
Notice that you said "in a single minute" without "calendar" and the
sentence is perfectly understandable? (see above and below).
> This means the user will end up with multiple bugreport files being
> created if they run the command multiple times quickly, but that feels
> more intuitive and consistent than an error arbitrarily occuring within
> a calendar minute, especially given that the time window in which the
> error currently occurs is variable as described above.
And drop "calendar" here, too (see above).
"Within the same minute" is fine.
> @@ -3,7 +3,6 @@
> #include "editor.h"
> #include "gettext.h"
> #include "parse-options.h"
> -#include "strbuf.h"
Looks like an unrelated change here. The updated code does use
strbuf service, so even if other header files happen to include
it, keep including it here is a good discipline for readability.
> @@ -101,12 +101,13 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> {
> struct strbuf buffer = STRBUF_INIT;
> struct strbuf report_path = STRBUF_INIT;
> + struct strbuf option_suffix = STRBUF_INIT;
> + struct strbuf default_option_suffix = STRBUF_INIT;
> int report = -1;
> time_t now = time(NULL);
> struct tm tm;
> enum diagnose_mode diagnose = DIAGNOSE_NONE;
> char *option_output = NULL;
> - char *option_suffix = "%Y-%m-%d-%H%M";
> const char *user_relative_path = NULL;
> char *prefixed_filename;
> size_t output_path_len;
> @@ -118,11 +119,14 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> PARSE_OPT_OPTARG, option_parse_diagnose),
> OPT_STRING('o', "output-directory", &option_output, N_("path"),
> N_("specify a destination for the bugreport file(s)")),
> - OPT_STRING('s', "suffix", &option_suffix, N_("format"),
> + OPT_STRING('s', "suffix", &option_suffix.buf, N_("format"),
> N_("specify a strftime format suffix for the filename(s)")),
> OPT_END()
> };
>
> + strbuf_addstr(&default_option_suffix, "%Y-%m-%d-%H%M");
> + strbuf_addstr(&option_suffix, default_option_suffix.buf);
It usually pays for a reviewer when two variables, one called
default and the other not, gets initialized to the same value,
because it is a sign that there is something fishy going on. A more
normal pattern is to set up the default, do whatever is needed and
if the non-default one has not been touched, then copy that default
value to the real one, and the code needed deviation from it for
whatever reason. Let's read on.
> @@ -134,9 +138,20 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> output_path_len = report_path.len;
>
> strbuf_addstr(&report_path, "git-bugreport-");
> - strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
> + strbuf_addftime(&report_path, option_suffix.buf, localtime_r(&now, &tm), 0, 0);
> strbuf_addstr(&report_path, ".txt");
OK.
> + if (strbuf_cmp(&option_suffix, &default_option_suffix) == 0) {
Style: never compare with 0 or NULL inside a conditional.
if (!compare(...)) {
is the idiom to use.
You may have written it this way to avoid appending after what the
user gave you as a custom pattern, but this if() statement is a
failed way to do so. The user can give what happens to be the same
as the hardcoded default pattern and you cannot tell if it came from
them or your initially hardcoded one by string comparison.
> + int i = 1;
> + int pos = report_path.len - 4;
Totally unclear where the magic "4" comes from.
> + while (file_exists(report_path.buf) && i < 10) {
> + if (i > 1)
> + strbuf_remove(&report_path, pos, 2);
> + strbuf_insertf(&report_path, pos, "+%d", i);
> + i++;
> + }
> + }
We see TOCTOU here.
Do it more like this instead.
* Discard default_option_suffix variable.
* Introduce a boolean option_suffix_is_from_user and
initialize it to false.
* Initialize option_suffix to an empty string.
* After parse_options() returns, if option_suffix is still
empty, then add the default pattern. Otherwise toggle
option_suffix_is_from_user true.
* Prepare the code so that you can recompute report_path as you
need to increment the suffix added to option_suffix.
Then where we do xopen() on report_path.buf, have a fallback loop,
and you can do something like
again:
report = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);
if (report < 0 && errno == EEXISTS && !option_suffix_is_from_user) {
increment_suffix(&report_path);
goto again;
} else if (report < 0) {
die_errno(_("unable to open '%s'", report_path.buf));
}
to avoid TOCTOU.
HTH.
^ permalink raw reply
* Re: [PATCH] grep: die gracefully when outside repository
From: Junio C Hamano @ 2023-10-15 17:57 UTC (permalink / raw)
To: Jeff King; +Cc: Kristoffer Haugsbakk, git, ks1322
In-Reply-To: <20231015032636.GC554702@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Is it even reasonable for "grep --no-index" to care about leaving the
> tree in the first place? That is, is there a reason we should not allow:
>
> git grep --no-index foo ../bar
A huge difference between the bare "grep" and "git grep" is that we
know the scope of the project tree, so it goes recursive by default.
Should the above command line recursively go below ../bar? Would we
allow "/" to be given?
I actually do not think these "we are allowing Git tools to be used
on random garbage" is a good idea to begin with X-<. If we invented
something nice for our variant in "git grep" and wish we can use it
outside the repository, contributing the feature to implementations
of "grep" would have been the right way to move forward, instead of
contaminating the codebase with things that are not related to Git.
Whoever did 59332d13 (Resurrect "git grep --no-index", 2010-02-06)
should be punished X-<.
Anyway.
2e48fcdb (grep docs: document --no-index option, 2010-02-25) seems
to have wanted to explicitly limit the search within the "current
directory", and I am fine to keep the search space limited by the
cwd. On the other hand, of course, the users can shoot themselves
in the foot with "grep -r foo /", so letting them use "git grep" the
same way is perhaps OK. Especially if it simplifies the code if we
lift the limitation, that is a very tempting thing to do.
> If we want to avoid leaving the current directory, then I think we need
> to be checking much sooner (but again, I would argue that it is not
> worth caring about in no-index mode).
^ 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