* Re: git-gui desktop launcher
From: Marc Branchaud @ 2024-02-06 13:57 UTC (permalink / raw)
To: Johannes Sixt, Tobias Boesch; +Cc: git
In-Reply-To: <993e6823-7fa7-4130-8c0a-69ed31da5fbe@kdbg.org>
On 2024-02-06 01:50, Johannes Sixt wrote:
>
>> Comment=A portable graphical interface to Git
>
> I have two gripes with this Comment:
>
> - That the program is portable is irrelevant for the user. The word need
> not occur in this Comment.
>
> - I had hoped for a more precise description. In particular, when a
> program is advertised as "graphical interface to Git", then I would
> expect that it can do a bit more than initialize repositories and make
> commits. At a minimum, I would expect a history viewer; but Git Gui
> doesn't have one. Unless you count the two "Visualize" entries in the
> "Repository" menu that invoke gitk as such. So, I dunno.
Perhaps
A graphical tool for creating Git commits
?
M.
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2024, #02; Fri, 2)
From: Phillip Wood @ 2024-02-06 13:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Patrick Steinhardt, Karthik Nayak
In-Reply-To: <xmqqjznimrbe.fsf@gitster.g>
Hi Junio
On 05/02/2024 23:20, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> I'm concerned that the UI could use some improvement. If I understand
>> correctly the proposal is to make
>>
>> git for-each-ref
>>
>> and
>>
>> git for-each-ref ""
>>
>> behave differently so that the latter prints the pseudorefs from the
>> current worktree and the former does not.
>
> I would actually think that is perfectly sensible. The optional
> arguments for-each-ref name "filtering patterns" and you can view
> the behaviour of the command as using "refs/" as the default
> filtering pattern when nothing is given. But it is easy to defeat
> the unfortunate and historical default filtering pattern, by saying
> "we do not limit to any hierarchy, anything goes" by giving "" as
> the prefix.
There is a logic to that if one ignores
"refs/{main-worktree,worktrees/$worktree}/*" not being shown. As Patrick
has pointed out the change in the handling of the empty prefix here is a
breaking change though [1]. To me, the handling of the empty prefix
feels inconsistent with the rest of the pattern space. The patterns "*"
and "*_HEAD" don't match anything and there is no way to filter a subset
of pseudorefs. If all we want is a way to show all the refs including
pseudorefs in a worktree then I think an option to do that which errored
out if a pattern is given would be a better approach. I'd prefer an
option (say "--include-pseudorefs") that included pseudorefs in the list
of refs to be filtered and allowed the user to filter them as they
wanted. That way we could later add a "--all-worktrees" option that
included refs/{main-worktree,worktrees/$worktree} in the list of refs to
be filtered as well.
Best Wishes
Phillip
[1] https://lore.kernel.org/git/ZcHEmyvvMR_b_Idl@tanuki/
^ permalink raw reply
* Re: [PATCH 1/4] notes: print note blob to stdout directly
From: Kristoffer Haugsbakk @ 2024-02-06 13:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Teng Long, Maarten Bosmans, Maarten Bosmans
In-Reply-To: <xmqqil32l0i6.fsf@gitster.g>
On Tue, Feb 6, 2024, at 04:44, Junio C Hamano wrote:
> I actually was hoping, after seeing the use case description in the
> cover letter, that the series would be introducing a batch mode
> interface to allow callers to ask notes for many objects and have
> the command respond notes for these objects in a way that which
> piece of output corresponds to which object in the request, reducing
> the process overhead amortised over many objects.
That sounds great. I imagine that would be very useful.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern
From: Phillip Wood @ 2024-02-06 13:55 UTC (permalink / raw)
To: Karthik Nayak, phillip.wood; +Cc: git, gitster, ps
In-Reply-To: <CAOLa=ZR=_tt=ppphGMkxqj_YB5G+YkTMWGzRzcHTbrZz4ysb5w@mail.gmail.com>
Hi Karthik
On 06/02/2024 08:52, Karthik Nayak wrote:
> On Mon, Feb 5, 2024 at 7:48 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 29/01/2024 11:35, Karthik Nayak wrote:
>>> When the user uses an empty string pattern (""), we don't match any refs
>>> in git-for-each-ref(1). This is because in git-for-each-ref(1), we use
>>> path based matching and an empty string doesn't match any path.
>>>
>>> In this commit we change this behavior by making empty string pattern
>>> match all references. This is done by introducing a new flag
>>> `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly
>>> introduced `refs_for_each_all_refs()` function to iterate over all the
>>> refs in the repository.
>>
>> It actually iterates over all the refs in the current worktree, not all
>> the refs in the repository. I have to say that I find it extremely
>> unintuitive that "" behaves differently to not giving a pattern. I
>> wonder if we can find a better UI here - perhaps a command line option
>> to include pseudorefs?
>>
>
> As Patrick mentioned, this was discussed a while ago and we decided to
> move forward with the `git for-each-ref ""` syntax.
Thanks I'd missed that discussion. I see that at the end of that
discussion Junio was concerned that the proposed "" did not account for
"refs/worktrees/$worktree/*" [1] - how has that been resolved?
Best Wishes
Phillip
[1] https://lore.kernel.org/git/xmqq1qawr6p4.fsf@gitster.g/
>>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>>> ---
>>> Documentation/git-for-each-ref.txt | 3 ++-
>>> builtin/for-each-ref.c | 21 +++++++++++++++++-
>>> ref-filter.c | 13 ++++++++++--
>>> ref-filter.h | 4 +++-
>>> t/t6302-for-each-ref-filter.sh | 34 ++++++++++++++++++++++++++++++
>>> 5 files changed, 70 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>>> index be9543f684..b1cb482bf5 100644
>>> --- a/Documentation/git-for-each-ref.txt
>>> +++ b/Documentation/git-for-each-ref.txt
>>> @@ -32,7 +32,8 @@ OPTIONS
>>> If one or more patterns are given, only refs are shown that
>>> match against at least one pattern, either using fnmatch(3) or
>>> literally, in the latter case matching completely or from the
>>> - beginning up to a slash.
>>> + beginning up to a slash. If an empty string is provided all refs
>>> + are printed, including HEAD and pseudorefs.
>>
>> I think it would be helpful to clarify that it is all the refs for the
>> current worktree that are printed, not all the refs in the repository -
>> we still don't have a way to iterate over the per-worktree refs from
>> other worktrees
>>
>
> I agree, based on if we keep the current commits or remove them, I'll
> send in a new patch or amend the current series.
>
> Thanks!
^ permalink raw reply
* [PATCH v2] .github/PULL_REQUEST_TEMPLATE.md: add a note about single-commit PRs
From: Philippe Blain via GitGitGadget @ 2024-02-06 13:20 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Philippe Blain, Philippe Blain
In-Reply-To: <pull.1665.git.git.1707069808205.gitgitgadget@gmail.com>
From: Philippe Blain <levraiphilippeblain@gmail.com>
Contributors using Gitgitgadget continue to send single-commit PRs with
their commit message text duplicated below the three-dash line,
increasing the signal-to-noise ratio for reviewers.
This is because Gitgitgadget copies the pull request description as an
in-patch commentary, for single-commit PRs, and _GitHub_ defaults to
prefilling the pull request description with the commit message, for
single-commit PRs (followed by the content of the pull request
template).
Add a note in the pull request template mentioning that for
single-commit PRs, the PR description should thus be kept empty, in the
hope that contributors read it and act on it.
This partly addresses:
https://github.com/gitgitgadget/gitgitgadget/issues/340
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
.github/PULL_REQUEST_TEMPLATE.md: add a note about single-commit PRs
Changes since v1:
* simplified the wording as suggested by Junio
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1665%2Fphil-blain%2Fempty-description-single-commit-prs-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1665/phil-blain/empty-description-single-commit-prs-v2
Pull-Request: https://github.com/git/git/pull/1665
Range-diff vs v1:
1: 4786f84884a ! 1: f719e11b1df .github/PULL_REQUEST_TEMPLATE.md: add a note about single-commit PRs
@@ .github/PULL_REQUEST_TEMPLATE.md: a mailing list (git@vger.kernel.org) for code
bug reports. Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/)
to conveniently send your Pull Requests commits to our mailing list.
-+If you use Gitgitgadget for a single-commit pull request, please *leave the pull
-+request description empty*: your commit message itself should describe your
-+changes.
++For a single-commit pull request, please *leave the pull request description
++empty*: your commit message itself should describe your changes.
+
Please read the "guidelines for contributing" linked above!
.github/PULL_REQUEST_TEMPLATE.md | 3 +++
1 file changed, 3 insertions(+)
diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md
index 952c7c3a2aa..37654cdfd7a 100644
--- a/.github/PULL_REQUEST_TEMPLATE.md
+++ b/.github/PULL_REQUEST_TEMPLATE.md
@@ -4,4 +4,7 @@ a mailing list (git@vger.kernel.org) for code submissions, code reviews, and
bug reports. Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/)
to conveniently send your Pull Requests commits to our mailing list.
+For a single-commit pull request, please *leave the pull request description
+empty*: your commit message itself should describe your changes.
+
Please read the "guidelines for contributing" linked above!
base-commit: bc7ee2e5e16f0d1e710ef8fab3db59ab11f2bbe7
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] .github/PULL_REQUEST_TEMPLATE.md: add a note about single-commit PRs
From: Philippe Blain @ 2024-02-06 13:17 UTC (permalink / raw)
To: Junio C Hamano, Philippe Blain via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <xmqqsf28oufw.fsf@gitster.g>
Hi Junio,
Le 2024-02-04 à 15:17, Junio C Hamano a écrit :
> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md
>> index 952c7c3a2aa..65fa3a37173 100644
>> --- a/.github/PULL_REQUEST_TEMPLATE.md
>> +++ b/.github/PULL_REQUEST_TEMPLATE.md
>> @@ -4,4 +4,8 @@ a mailing list (git@vger.kernel.org) for code submissions, code reviews, and
>> bug reports. Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/)
>> to conveniently send your Pull Requests commits to our mailing list.
>>
>> +If you use Gitgitgadget for a single-commit pull request, please *leave the pull
>> +request description empty*: your commit message itself should describe your
>> +changes.
>> +
>> Please read the "guidelines for contributing" linked above!
>
> Making it easier for contributors to come up with the right output
> is greatly appreciated. I think "If you use Gitgitgadget for" ->
> "For" is probably a good change, for two reasons (one, we do not
> take pull request except for GGG gateway in the first place, and
> two, PR messages being similar to cover letters, you do not want to
> have a detailed PR message that (a) takes extra time to write, (b)
> can duplicate what the you already have written, and (c) could
> contradict what the commit log message says.
Good idea, I'll tweak that in v2.
> I wonder if such a rule can be also enforced at the GGG side? It
> apparently knows if it is dealing with a single-patch request or a
> multi-patch series (as the types of messages this documentation
> update tries to address are the only ones that get duplicates under
> the three-dash line), and I've seen GGG complain on the contents of
> the commit log message (e.g., "not signed off") so there is enough
> support to inspect things in a PR and add instruction to the PR
> discussion. Unless the machinery GGG uses lack the ability to read
> the PR message (unlike the commit log messages that it can read
> apparently), it may be able to enforce that "for a single patch, PR
> message should be empty" rule before the /submit instruction. It
> might make things even more helpful if it can notice the commit log
> message is similar enough or superset of PR message and refrain from
> inserting it after the three-dash line, but that might be asking too
> much ;-)
Yes, it probably is possible, and Dscho suggested the same in [1] and [2].
I'll maybe get a crack at it eventually, but I hope in the meantime this
simple addition to the PR template will help a bit.
Thanks,
Philippe.
[1] https://github.com/gitgitgadget/gitgitgadget/issues/340#issuecomment-717782054
[2] https://github.com/gitgitgadget/gitgitgadget.github.io/pull/8#issuecomment-1133758832
^ permalink raw reply
* Re: is it a bug that git status show the in-progress 'edit' in an interactive rebase as 'done'?
From: Oswald Buddenhagen @ 2024-02-06 11:32 UTC (permalink / raw)
To: Britton Kerin; +Cc: git
In-Reply-To: <CAC4O8c_oKT+a0hm+tqSOG7d1=AuJJKy5bsh72cJKVsWynvHw+w@mail.gmail.com>
On Tue, Feb 06, 2024 at 01:02:43AM -0900, Britton Kerin wrote:
>>Last command done (1 command done):
>> edit 71b73de914 message for first commit
>>...
>>You are currently editing a commit while rebasing branch
>>...
>
>This seems wrong, because until git rebase --continue has been done
>the edit operation for the first commit is *ongoing* and it would be
>much clearer for the output of status to accurately say so.
>
it makes a lot of more sense when you decompose 'edit' into 'pick'
followed by 'break', which it essentially is. so from git's perspective,
the command really _is_ already done. note that in this state, you can
do all kinds of crazy things - including adding new commits (possibly by
cherry-picking them) and even dropping already rewritten commits (using
a hard reset). so in a way, the message above is even a bit too
suggestive.
^ permalink raw reply
* Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern
From: Phillip Wood @ 2024-02-06 10:49 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, git, gitster
In-Reply-To: <ZcHEmyvvMR_b_Idl@tanuki>
Hi Patrick
On 06/02/2024 05:33, Patrick Steinhardt wrote:
> On Mon, Feb 05, 2024 at 06:48:52PM +0000, Phillip Wood wrote:
>> Hi Karthik
>>
>> On 29/01/2024 11:35, Karthik Nayak wrote:
>>> When the user uses an empty string pattern (""), we don't match any refs
>>> in git-for-each-ref(1). This is because in git-for-each-ref(1), we use
>>> path based matching and an empty string doesn't match any path.
>>>
>>> In this commit we change this behavior by making empty string pattern
>>> match all references. This is done by introducing a new flag
>>> `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly
>>> introduced `refs_for_each_all_refs()` function to iterate over all the
>>> refs in the repository.
>>
>> It actually iterates over all the refs in the current worktree, not all the
>> refs in the repository. I have to say that I find it extremely unintuitive
>> that "" behaves differently to not giving a pattern. I wonder if we can find
>> a better UI here - perhaps a command line option to include pseudorefs?
>
> I tend to agree, and have argued for a flag in another thread, too [1].
Thanks for that, I'd missed that discussion
> Something like `--show-all-refs` feels a lot more intuitive to me and
> also doesn't have the potential to break preexisting scripts which pass
> the empty prefix (which would have returned the empty set of refs, but
> now returns all refs).
Yes, and as you point out in that other thread flag would allow the
pseuderefs to be filtered and allow us to show the refs for all
worktrees as well in the future.
Best Wishes
Phillip
> [1]: https://lore.kernel.org/git/ZZWCXFghtql4i4YE@tanuki/
>
> Patrick
^ permalink raw reply
* gitk view could use a checkbox for stash
From: Britton Kerin @ 2024-02-06 10:39 UTC (permalink / raw)
To: git
If the Branches & tags field of gitk view is populated then stash ends
up filtered out.
If stash is added to the list it shows, but then an error ends up
happening if there is no stash.
These behaviors are literally correct so certainly shouldn't be
changed, but they're potentially confusing and unlikely to be what the
user actually wants.
Perhaps adding a Stash checkbox to the list of checkboxes would be
worthwhile? It would actually mean 'Stash (if present)' but I think
it would be clear enough. IMO it would then be good to have it
checked by default in new views.
A similar consideration applies when there's an ongoing interactive
rebase: it's unfortunate that the commits and staged and unstaged
changes on the in-progress rebase end up filtered out. They can be
explicitly added back in by adding 'git rev-list --ignore-missing
REBASE_HEAD..HEAD' to the 'Command to generate more commits to
include:' field, but it might be nice if there was a more obvious way
to achieve this. It's less obvious here what it might be though.
The general thing here is that it would be nice if branch views had an
obvious way to show the stuff that users probably want to see when
working on branches. I'm definitely not in favor of outright cheating
and making filter options not really do what they say (keeping
interface and interface close is more important than usability in
context of git IMO) but if there was some box to check to "show all
that stuff" and it was reasonably clear what the stuff was I'd be
happy to click it :)
Britton
^ permalink raw reply
* gitk view Branches & tags field would benefit from globs
From: Britton Kerin @ 2024-02-06 10:19 UTC (permalink / raw)
To: git
Well something like globs but for refs.
For example when I have my_branch-v1 my_branch-v2 my_branch-v2.1
my_branch-v3 etc. I'd like to be able to see them all without having
to go back and insert every one into the list in Branches & tags.
Or is there already a way to do this?
Britton
^ permalink raw reply
* is it a bug that git status show the in-progress 'edit' in an interactive rebase as 'done'?
From: Britton Kerin @ 2024-02-06 10:02 UTC (permalink / raw)
To: git
If I do 'git rebase -i master' from a branch then set the action of
the first two commits of a branch being rebased to 'edit' and exit the
editor, an immediately subsequent 'git status' shows (for example):
git status
interactive rebase in progress; onto e79552d197
Last command done (1 command done):
edit 71b73de914 message for first commit
Next commands to do (6 remaining commands):
edit 3a478a7a08 message for second commit
pick fab7159cf4 message for third commit
(use "git rebase --edit-todo" to view and edit)
You are currently editing a commit while rebasing branch
'my_completion_updates' on 'e79552d197'.
(use "git commit --amend" to amend the current commit)
(use "git rebase --continue" once you are satisfied with your changes)
This seems wrong, because until git rebase --continue has been done
the edit operation for the first commit is *ongoing* and it would be
much clearer for the output of status to accurately say so.
The same applies to edit operations that don't happen to be the first.
Is this a bug or is there some reason it's like this that I'm not seeing?
Britton
^ permalink raw reply
* Re: [PATCH 1/4] notes: print note blob to stdout directly
From: Maarten Bosmans @ 2024-02-06 9:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Teng Long, Maarten Bosmans
In-Reply-To: <xmqqil32l0i6.fsf@gitster.g>
Op di 6 feb 2024 om 04:44 schreef Junio C Hamano <gitster@pobox.com>:
>
> Maarten Bosmans <mkbosmans@gmail.com> writes:
> > Avoid the need to launch a subprocess by calling stream_blob_to_fd
> > directly. This does not only get rid of the overhead of a separate
> > child process, but also avoids the initalization of the whole log
> > machinery that `git show` does. That is needed for example to show
> > decorated commits and applying the mailmap. For simply displaying
> > a blob however, the only useful thing show does is enabling the pager.
> >
> > Signed-off-by: Maarten Bosmans <maarten.bosmans@vortech.nl>
> > ---
> > builtin/notes.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
>
> I am not sure if we want to accept an approach that feels somewhat
> narrow/short sighted, like this patch. When "git show" learns an
> improved way to show blob objects, who will update the code this
> patch touches to teach it to use the same improved way to show the
> notes?
Yes, you would loose some of the flexibility by just calling out to
another git command. But whether that is actually an issue depends on
the way git show would be extended. As I mentioned in the cover
letter, there is some handling of textconv
being done in git show for the blob case. I though it would not be
applicable to note blobs, but am not entirely sure.
To address your concern and the textconv case, instead of calling
stream_blob_to_fd() directly, show_blob_object() could be used. Right
now that is a static function in building/log.c I guess that needs to
be moved then to log.c (or show.c)?
I'm still not sure that is a good approach though, as the notes
edit/append subcommands use repo_read_object_file() directly to fill
the current note contents. So anything fancy that git show would do to
blob output is not reflected when editing a note.
> I actually was hoping, after seeing the use case description in the
> cover letter, that the series would be introducing a batch mode
> interface to allow callers to ask notes for many objects and have
> the command respond notes for these objects in a way that which
> piece of output corresponds to which object in the request, reducing
> the process overhead amortised over many objects.
That is also a cool idea. That would probably use the functionality of
the cat-file batch mode, right? In that case you loose any future
changes to git show anyway. At least the current behavior of git show
when fed multiple blob hashes is to simply output them directly after
another, without any way of identifying the parts.
My concern would be that this additional functionality would not get
used much, as it requires a bit more than just a quick-n-dirty bash
script with some loops and command substitutions. If that approach is
reasonably fast (the object of this patch series), then there is not
much to be gained in a practical sense by a batch mode. To me it seems
the git notes code is already a bit undermaintained currently, so I'd
rather keep it generic, instead of customizing it to more specific use
cases.
Maarten
^ permalink raw reply
* [PATCH 4/4] Always check `parse_tree*()`'s return value
From: Johannes Schindelin via GitGitGadget @ 2024-02-06 9:49 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1651.git.1707212981.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Otherwise we may easily run into serious crashes: For example, if we run
`init_tree_desc()` directly after a failed `parse_tree()`, we are
accessing uninitialized data or trying to dereference `NULL`.
Note that the `parse_tree()` function already takes care of showing an
error message. The `parse_tree_indirectly()` and
`repo_get_commit_tree()` functions do not, therefore those latter call
sites need to show a useful error message while the former do not.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/checkout.c | 19 ++++++++++++++++---
builtin/clone.c | 3 ++-
builtin/commit.c | 3 ++-
builtin/merge-tree.c | 6 ++++++
builtin/read-tree.c | 3 ++-
builtin/reset.c | 4 ++++
cache-tree.c | 4 ++--
merge-ort.c | 3 +++
merge-recursive.c | 3 ++-
merge.c | 5 ++++-
reset.c | 5 +++++
sequencer.c | 4 ++++
12 files changed, 52 insertions(+), 10 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f02434bc155..84108ec3635 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -707,7 +707,8 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
init_checkout_metadata(&opts.meta, info->refname,
info->commit ? &info->commit->object.oid : null_oid(),
NULL);
- parse_tree(tree);
+ if (parse_tree(tree) < 0)
+ return 128;
init_tree_desc(&tree_desc, tree->buffer, tree->size);
switch (unpack_trees(1, &tree_desc, &opts)) {
case -2:
@@ -786,9 +787,15 @@ static int merge_working_tree(const struct checkout_opts *opts,
if (new_branch_info->commit)
BUG("'switch --orphan' should never accept a commit as starting point");
new_tree = parse_tree_indirect(the_hash_algo->empty_tree);
- } else
+ if (!new_tree)
+ BUG("unable to read empty tree");
+ } else {
new_tree = repo_get_commit_tree(the_repository,
new_branch_info->commit);
+ if (!new_tree)
+ return error(_("unable to read tree %s"),
+ oid_to_hex(&new_branch_info->commit->object.oid));
+ }
if (opts->discard_changes) {
ret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info);
if (ret)
@@ -823,7 +830,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
oid_to_hex(old_commit_oid));
init_tree_desc(&trees[0], tree->buffer, tree->size);
- parse_tree(new_tree);
+ if (parse_tree(new_tree) < 0)
+ exit(128);
tree = new_tree;
init_tree_desc(&trees[1], tree->buffer, tree->size);
@@ -1239,10 +1247,15 @@ static void setup_new_branch_info_and_source_tree(
if (!new_branch_info->commit) {
/* not a commit */
*source_tree = parse_tree_indirect(rev);
+ if (!*source_tree)
+ die(_("unable to read tree %s"), oid_to_hex(rev));
} else {
parse_commit_or_die(new_branch_info->commit);
*source_tree = repo_get_commit_tree(the_repository,
new_branch_info->commit);
+ if (!*source_tree)
+ die(_("unable to read tree %s"),
+ oid_to_hex(&new_branch_info->commit->object.oid));
}
}
diff --git a/builtin/clone.c b/builtin/clone.c
index c6357af9498..4410b55be98 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -736,7 +736,8 @@ static int checkout(int submodule_progress, int filter_submodules)
tree = parse_tree_indirect(&oid);
if (!tree)
die(_("unable to parse commit %s"), oid_to_hex(&oid));
- parse_tree(tree);
+ if (parse_tree(tree) < 0)
+ exit(128);
init_tree_desc(&t, tree->buffer, tree->size);
if (unpack_trees(1, &t, &opts) < 0)
die(_("unable to checkout working tree"));
diff --git a/builtin/commit.c b/builtin/commit.c
index 781af2e206c..0723f06de7a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -339,7 +339,8 @@ static void create_base_index(const struct commit *current_head)
tree = parse_tree_indirect(¤t_head->object.oid);
if (!tree)
die(_("failed to unpack HEAD tree object"));
- parse_tree(tree);
+ if (parse_tree(tree) < 0)
+ exit(128);
init_tree_desc(&t, tree->buffer, tree->size);
if (unpack_trees(1, &t, &opts))
exit(128); /* We've already reported the error, finish dying */
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 2d4ce5b3886..ba84d00deee 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -447,12 +447,18 @@ static int real_merge(struct merge_tree_options *o,
if (repo_get_oid_treeish(the_repository, merge_base, &base_oid))
die(_("could not parse as tree '%s'"), merge_base);
base_tree = parse_tree_indirect(&base_oid);
+ if (!base_tree)
+ die(_("unable to read tree %s"), oid_to_hex(&base_oid));
if (repo_get_oid_treeish(the_repository, branch1, &head_oid))
die(_("could not parse as tree '%s'"), branch1);
parent1_tree = parse_tree_indirect(&head_oid);
+ if (!parent1_tree)
+ die(_("unable to read tree %s"), oid_to_hex(&head_oid));
if (repo_get_oid_treeish(the_repository, branch2, &merge_oid))
die(_("could not parse as tree '%s'"), branch2);
parent2_tree = parse_tree_indirect(&merge_oid);
+ if (!parent2_tree)
+ die(_("unable to read tree %s"), oid_to_hex(&merge_oid));
opt.ancestor = merge_base;
merge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 8196ca9dd85..5923ed36893 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -263,7 +263,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
cache_tree_free(&the_index.cache_tree);
for (i = 0; i < nr_trees; i++) {
struct tree *tree = trees[i];
- parse_tree(tree);
+ if (parse_tree(tree) < 0)
+ return 128;
init_tree_desc(t+i, tree->buffer, tree->size);
}
if (unpack_trees(nr_trees, t, &opts))
diff --git a/builtin/reset.c b/builtin/reset.c
index 4b018d20e3b..f030f57f4e9 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -119,6 +119,10 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
if (reset_type == MIXED || reset_type == HARD) {
tree = parse_tree_indirect(oid);
+ if (!tree) {
+ error(_("unable to read tree %s"), oid_to_hex(oid));
+ goto out;
+ }
prime_cache_tree(the_repository, the_repository->index, tree);
}
diff --git a/cache-tree.c b/cache-tree.c
index 641427ed410..c6508b64a5c 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -779,8 +779,8 @@ static void prime_cache_tree_rec(struct repository *r,
struct cache_tree_sub *sub;
struct tree *subtree = lookup_tree(r, &entry.oid);
- if (!subtree->object.parsed)
- parse_tree(subtree);
+ if (!subtree->object.parsed && parse_tree(subtree) < 0)
+ exit(128);
sub = cache_tree_sub(it, entry.path);
sub->cache_tree = cache_tree();
diff --git a/merge-ort.c b/merge-ort.c
index 79d9e18f63d..534ddaf16ba 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4983,6 +4983,9 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
if (result->clean >= 0) {
result->tree = parse_tree_indirect(&working_tree_oid);
+ if (!result->tree)
+ die(_("unable to read tree %s"),
+ oid_to_hex(&working_tree_oid));
/* existence of conflicted entries implies unclean */
result->clean &= strmap_empty(&opt->priv->conflicted);
}
diff --git a/merge-recursive.c b/merge-recursive.c
index e3beb0801b1..10d41bfd487 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -410,7 +410,8 @@ static inline int merge_detect_rename(struct merge_options *opt)
static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)
{
- parse_tree(tree);
+ if (parse_tree(tree) < 0)
+ exit(128);
init_tree_desc(desc, tree->buffer, tree->size);
}
diff --git a/merge.c b/merge.c
index b60925459c2..14a7325859d 100644
--- a/merge.c
+++ b/merge.c
@@ -80,7 +80,10 @@ int checkout_fast_forward(struct repository *r,
return -1;
}
for (i = 0; i < nr_trees; i++) {
- parse_tree(trees[i]);
+ if (parse_tree(trees[i]) < 0) {
+ rollback_lock_file(&lock_file);
+ return -1;
+ }
init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
}
diff --git a/reset.c b/reset.c
index 48da0adf851..a93fdbc12e3 100644
--- a/reset.c
+++ b/reset.c
@@ -158,6 +158,11 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
}
tree = parse_tree_indirect(oid);
+ if (!tree) {
+ ret = error(_("unable to read tree %s"), oid_to_hex(oid));
+ goto leave_reset_head;
+ }
+
prime_cache_tree(r, r->index, tree);
if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0) {
diff --git a/sequencer.c b/sequencer.c
index d584cac8ed9..407473bab28 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -715,6 +715,8 @@ static int do_recursive_merge(struct repository *r,
o.show_rename_progress = 1;
head_tree = parse_tree_indirect(head);
+ if (!head_tree)
+ return error(_("unable to read tree %s"), oid_to_hex(head));
next_tree = next ? repo_get_commit_tree(r, next) : empty_tree(r);
base_tree = base ? repo_get_commit_tree(r, base) : empty_tree(r);
@@ -3887,6 +3889,8 @@ static int do_reset(struct repository *r,
}
tree = parse_tree_indirect(&oid);
+ if (!tree)
+ return error(_("unable to read tree %s"), oid_to_hex(&oid));
prime_cache_tree(r, r->index, tree);
if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)
--
gitgitgadget
^ permalink raw reply related
* [PATCH 3/4] t4301: verify that merge-tree fails on missing blob objects
From: Johannes Schindelin via GitGitGadget @ 2024-02-06 9:49 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1651.git.1707212981.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
We just fixed a problem where `merge-tree` would not fail on missing
tree objects. Let's ensure that that problem does not occur with blob
objects (and won't, in the future, either).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t4301-merge-tree-write-tree.sh | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index 4ea1b74445d..86807a57d4d 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -961,4 +961,18 @@ test_expect_success 'error out on missing tree objects' '
test_must_be_empty actual
'
+test_expect_success 'error out on missing blob objects' '
+ seq1=$(test_seq 1 10 | git hash-object -w --stdin) &&
+ seq2=$(test_seq 1 11 | git hash-object -w --stdin) &&
+ seq3=$(test_seq 0 10 | git hash-object -w --stdin) &&
+ tree1=$(printf "100644 blob %s\tsequence" $seq1 | git mktree) &&
+ tree2=$(printf "100644 blob %s\tsequence" $seq2 | git mktree) &&
+ tree3=$(printf "100644 blob %s\tsequence" $seq3 | git mktree) &&
+ git init --bare missing-blob.git &&
+ test_write_lines $seq1 $seq3 $tree1 $tree2 $tree3 |
+ git pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&
+ test_must_fail git --git-dir=missing-blob.git merge-tree --merge-base=$tree1 $tree2 $tree3 >actual &&
+ test_must_be_empty actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/4] merge-ort: do check `parse_tree()`'s return value
From: Johannes Schindelin via GitGitGadget @ 2024-02-06 9:49 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1651.git.1707212981.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The previous commit fixed a bug where a missing tree was reported, but
not treated as an error.
This patch addresses the same issue for the remaining two callers of
`parse_tree()`.
This change is not accompanied by a regression test because the code in
question is only reached at the `checkout` stage, i.e. after the merge
has happened (and therefore the tree objects could only be missing if
the disk had gone bad in that short time window, or something similarly
tricky to recreate in the test suite).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
merge-ort.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/merge-ort.c b/merge-ort.c
index c37fc035f13..79d9e18f63d 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4379,9 +4379,11 @@ static int checkout(struct merge_options *opt,
unpack_opts.verbose_update = (opt->verbosity > 2);
unpack_opts.fn = twoway_merge;
unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */
- parse_tree(prev);
+ if (parse_tree(prev) < 0)
+ return -1;
init_tree_desc(&trees[0], prev->buffer, prev->size);
- parse_tree(next);
+ if (parse_tree(next) < 0)
+ return -1;
init_tree_desc(&trees[1], next->buffer, next->size);
ret = unpack_trees(2, trees, &unpack_opts);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/4] merge-tree: fail with a non-zero exit code on missing tree objects
From: Johannes Schindelin via GitGitGadget @ 2024-02-06 9:49 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1651.git.1707212981.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When `git merge-tree` encounters a missing tree object, it should error
out and not continue quietly as if nothing had happened.
However, as of time of writing, `git merge-tree` _does_ continue, and
then offers the empty tree as result.
Let's fix this.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
merge-ort.c | 7 ++++---
t/t4301-merge-tree-write-tree.sh | 10 ++++++++++
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/merge-ort.c b/merge-ort.c
index 6491070d965..c37fc035f13 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1659,9 +1659,10 @@ static int collect_merge_info(struct merge_options *opt,
info.data = opt;
info.show_all_errors = 1;
- parse_tree(merge_base);
- parse_tree(side1);
- parse_tree(side2);
+ if (parse_tree(merge_base) < 0 ||
+ parse_tree(side1) < 0 ||
+ parse_tree(side2) < 0)
+ return -1;
init_tree_desc(t + 0, merge_base->buffer, merge_base->size);
init_tree_desc(t + 1, side1->buffer, side1->size);
init_tree_desc(t + 2, side2->buffer, side2->size);
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index 7d0fa74da74..4ea1b74445d 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -950,5 +950,15 @@ test_expect_success '--merge-base with tree OIDs' '
git merge-tree --merge-base=side1^^{tree} side1^{tree} side3^{tree} >with-trees &&
test_cmp with-commits with-trees
'
+test_expect_success 'error out on missing tree objects' '
+ git init --bare missing-tree.git &&
+ (
+ git rev-list side3 &&
+ git rev-parse side3^:
+ ) | git pack-objects missing-tree.git/objects/pack/side3-tree-is-missing &&
+ side3=$(git rev-parse side3) &&
+ test_must_fail git --git-dir=missing-tree.git merge-tree $side3^ $side3 >actual &&
+ test_must_be_empty actual
+'
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/4] merge-tree: handle missing objects correctly
From: Johannes Schindelin via GitGitGadget @ 2024-02-06 9:49 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
I recently looked into issues where git merge-tree calls returned bogus data
(in one instance returning an empty tree for non-empty merge parents). By
the time I had a look at the corresponding repository, the issue was no
longer reproducible, but a closer look at the code combined with some manual
experimenting turned up the fact that missing tree objects aren't handled as
errors by git merge-tree.
While at it, I added a commit on top that tries to catch all remaining
unchecked parse_tree() calls.
This patch series is based on js/merge-tree-3-trees because I introduced
three unchecked parse_tree() calls in that topic branch.
Johannes Schindelin (4):
merge-tree: fail with a non-zero exit code on missing tree objects
merge-ort: do check `parse_tree()`'s return value
t4301: verify that merge-tree fails on missing blob objects
Always check `parse_tree*()`'s return value
builtin/checkout.c | 19 ++++++++++++++++---
builtin/clone.c | 3 ++-
builtin/commit.c | 3 ++-
builtin/merge-tree.c | 6 ++++++
builtin/read-tree.c | 3 ++-
builtin/reset.c | 4 ++++
cache-tree.c | 4 ++--
merge-ort.c | 16 +++++++++++-----
merge-recursive.c | 3 ++-
merge.c | 5 ++++-
reset.c | 5 +++++
sequencer.c | 4 ++++
t/t4301-merge-tree-write-tree.sh | 24 ++++++++++++++++++++++++
13 files changed, 84 insertions(+), 15 deletions(-)
base-commit: 5f43cf5b2e4b68386d3774bce880b0f74d801635
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1651%2Fdscho%2Fmerge-tree-and-missing-objects-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1651/dscho/merge-tree-and-missing-objects-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1651
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH v3 0/9] reftable: code style improvements
From: Karthik Nayak @ 2024-02-06 9:10 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Eric Sunshine, Junio C Hamano, Toon Claes
In-Reply-To: <cover.1707200355.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 454 bytes --]
Hello,
Patrick Steinhardt <ps@pks.im> writes:
> Hi,
>
> this is the third version of my patch series that tries to align the
> reftable library's coding style to be closer to Git's own code style.
>
> The only change compared to v2 is that I've now also converted some
> calls to `reftable_malloc()` to use `REFTABLE_ALLOC_ARRAY`.
>
The range-diff looks good to me, since this was the only change I
requested, the patch series looks good now.
Thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern
From: Karthik Nayak @ 2024-02-06 8:52 UTC (permalink / raw)
To: phillip.wood; +Cc: git, gitster, ps
In-Reply-To: <98d79d33-0d7e-4a9c-a6a3-ed9b58cd7445@gmail.com>
Hello,
On Mon, Feb 5, 2024 at 7:48 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Karthik
>
> On 29/01/2024 11:35, Karthik Nayak wrote:
> > When the user uses an empty string pattern (""), we don't match any refs
> > in git-for-each-ref(1). This is because in git-for-each-ref(1), we use
> > path based matching and an empty string doesn't match any path.
> >
> > In this commit we change this behavior by making empty string pattern
> > match all references. This is done by introducing a new flag
> > `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly
> > introduced `refs_for_each_all_refs()` function to iterate over all the
> > refs in the repository.
>
> It actually iterates over all the refs in the current worktree, not all
> the refs in the repository. I have to say that I find it extremely
> unintuitive that "" behaves differently to not giving a pattern. I
> wonder if we can find a better UI here - perhaps a command line option
> to include pseudorefs?
>
As Patrick mentioned, this was discussed a while ago and we decided to
move forward with the `git for-each-ref ""` syntax.
> > Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> > ---
> > Documentation/git-for-each-ref.txt | 3 ++-
> > builtin/for-each-ref.c | 21 +++++++++++++++++-
> > ref-filter.c | 13 ++++++++++--
> > ref-filter.h | 4 +++-
> > t/t6302-for-each-ref-filter.sh | 34 ++++++++++++++++++++++++++++++
> > 5 files changed, 70 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> > index be9543f684..b1cb482bf5 100644
> > --- a/Documentation/git-for-each-ref.txt
> > +++ b/Documentation/git-for-each-ref.txt
> > @@ -32,7 +32,8 @@ OPTIONS
> > If one or more patterns are given, only refs are shown that
> > match against at least one pattern, either using fnmatch(3) or
> > literally, in the latter case matching completely or from the
> > - beginning up to a slash.
> > + beginning up to a slash. If an empty string is provided all refs
> > + are printed, including HEAD and pseudorefs.
>
> I think it would be helpful to clarify that it is all the refs for the
> current worktree that are printed, not all the refs in the repository -
> we still don't have a way to iterate over the per-worktree refs from
> other worktrees
>
I agree, based on if we keep the current commits or remove them, I'll
send in a new patch or amend the current series.
Thanks!
^ permalink raw reply
* Re: [PATCH 0/4] Speed up git-notes show
From: Maarten Bosmans @ 2024-02-06 8:51 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <fa98146f-9308-4844-bec9-4605575b9cd9@app.fastmail.com>
Op di 6 feb 2024 om 08:08 schreef Kristoffer Haugsbakk <code@khaugsbakk.name>:
> Nice. I’ve seen a few tools that do that:
>
> • https://github.com/spotify/git-test
> • https://github.com/mhagger/git-test
Cool to see others are doing something similar.
The first git-test stores the test results in a local cache though,
not in git notes.
The second git-test stores the results in a note attached to the tree
instead of the commit. This is a clever idea, as you avoid running
tests twice on the same tree (e.g. the merge commit of a `git merge
--no-ff` merged branch that was already rebased to be current). Down
side is that git log and can display commit notes after the message,
but cannot display tree notes.
In our setup there's one CI server running these tests and pushing the
notes to the repo. This way everybody (developer, reviewer) can see
the state of in-progress work without having to run all build test
variations themselves.
Anyway, good to see wider usage of the git notes feature as
inspiration for new possibilities.
Maarten
^ permalink raw reply
* Re: [Outreachy][PATCH 1/2] date: refactor 64 bit prereq code into reusable functions
From: Christian Couder @ 2024-02-06 8:39 UTC (permalink / raw)
To: rsbecker; +Cc: Achu Luma, git, Christian Couder
In-Reply-To: <020a01da5859$89e41210$9dac3630$@nexbridge.com>
On Mon, Feb 5, 2024 at 6:34 PM <rsbecker@nexbridge.com> wrote:
> On Monday, February 5, 2024 11:25 AM, Achu Luma wrote:
> I would suggest that you also take into account whether time_t is signed or
> not (more difficult perhaps). Some platforms use signed time_t to allow
> representation of dates prior to 1970-01-01, while others make this signed.
> Some other platforms (S/390 for example) have retained time_t as 32-bits but
> have a time64_t for 64 bits. It might be useful to account for this.
The goal of this small series is just to port some existing tests to
the new unit test framework. I think it's a different topic to improve
the existing tests to take into account whether time_t is signed or
not. But thanks for the info.
^ permalink raw reply
* Re: [PATCH] Add ideas for GSoC 2024
From: Christian Couder @ 2024-02-06 8:26 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Taylor Blau, Junio C Hamano, Victoria Dye, Karthik Nayak
In-Reply-To: <ZcHH9UCiQgGvugyc@tanuki>
On Tue, Feb 6, 2024 at 6:47 AM Patrick Steinhardt <ps@pks.im> wrote:
> On Mon, Feb 05, 2024 at 05:43:17PM +0100, Christian Couder wrote:
> > "Difficulty: Low" might not be very accurate from the point of view of
> > contributors. I think it's always quite difficult to contribute
> > something significant to Git, and sometimes more than we expected.
>
> That's certainly true. I understood the difficulty levels here as being
> relative to the already-high bar that the Git project typically sets.
I am not sure potential contributors are aware of the high bar that
the Git project typically sets.
> Otherwise there wouldn't be much use to specify difficulty in the first
> place if all items had the same difficulty.
>
> Or is the intent of the difficulty level rather on a global GSoC level?
Yeah, I think it makes more sense to consider it like this.
> In that case I agree that we should bump the difficulty to "medium".
Yeah, I think we should bump it to "medium".
> > From reading the discussion it looks like everyone is Ok with doing
> > this. I really hope that we are not missing something that might make
> > us decide early not to do this though.
>
> I agree that this is a risky one, and that's what I tried to bring
> across with the "harder part will be to hash out how to handle backwards
> compatibility". Overall I think this project will be more about selling
> the patch and reasoning about how it can be done without breaking
> backwards compatibility.
>
> We could bump the difficulty to high to reflect that better. But if you
> deem the risk to be too high then I'm also happy to drop the topic
> completely.
I think we can keep this topic with a "Medium" difficulty. Perhaps it
will actually not be very difficult if all goes well.
Yeah, it may seem strange, but I think unless we start to have
projects not related much to our code base, like perhaps projects
related to our web sites or some infrastructure or the Git Rev News or
our docs, I think most projects should have a "Medium" difficulty. We
might want to use "High" sometimes if we want to discourage
contributors unless they have some special background related to the
specific topic (like multi-threading for example if we had related
projects).
^ permalink raw reply
* Re: [PATCH] Add ideas for GSoC 2024
From: Christian Couder @ 2024-02-06 8:13 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Kaartic Sivaraam, git, Taylor Blau, Junio C Hamano, Victoria Dye,
Karthik Nayak
In-Reply-To: <ZcHIxcrKbgyhdyWn@tanuki>
On Tue, Feb 6, 2024 at 6:51 AM Patrick Steinhardt <ps@pks.im> wrote:
> On Tue, Feb 06, 2024 at 12:25:31AM +0530, Kaartic Sivaraam wrote:
> > Makes sense. Also, I'm kind of cat-one-the-wall about whether it makes sense
> > to have two projects about the unit test migration effort itself. If we're
> > clear that both of them would not overlap, it should be fine. Otherwise, it
> > would be better to merge them as Patrick suggests.
>
> I don't quite mind either way. I think overall we have enough tests that
> can be converted even if both projects got picked up separately. And the
> reftable unit tests are a bit more involved than the other tests given
> that their coding style doesn't fit at all into the Git project. So it's
> not like they can just be copied over, they definitely need some special
> care.
>
> Also, the technical complexity of the "reftable" backend is rather high,
> which is another hurdle to take.
>
> Which overall makes me lean more towards keeping this as a separate
> project now that I think about it.
Ok, for me. If we have a contributor working on each of these 2
projects, we just need to be clear that the contributors should not
work together on the 2 projects as I think the GSoC forbids that.
> > That said, how helpful would it be to link the following doc in the unit
> > testing related ideas?
> >
> > https://github.com/git/git/blob/master/Documentation/technical/unit-tests.txt
>
> Makes sense to me.
To me too.
> > Would it worth linking the reftable technical doc for the above ideas?
> >
> > https://git-scm.com/docs/reftable
> >
> > I could see it goes into a lot of detail. I'm just wondering if link to it
> > would help someone who's looking to learn about reftable.
>
> Definitely doesn't hurt.
I agree.
Thanks!
^ permalink raw reply
* Re: Migrate away from vger to GitHub or (on-premise) GitLab?
From: Dragan Simic @ 2024-02-06 8:06 UTC (permalink / raw)
To: Hans Meiser
Cc: Theodore Ts'o, Junio C Hamano, rsbecker,
'Sergey Organov', git
In-Reply-To: <DB9P195MB213080E6DD9ECA0EE3D2B491E2462@DB9P195MB2130.EURP195.PROD.OUTLOOK.COM>
Hello Hans,
On 2024-02-06 08:22, Hans Meiser wrote:
>> Please, keep in mind that not everyone lives in a web browser and
>> loves to click around. Some people simply prefer to use the CLI
>> utilities and to press the keys on their keyboards, and are very
>> efficient while doing that.
>
> You are aware of the fact that all these Git collaboration websites
> are providing a REST interface? So, you are free to access any
> function by means of CLI?
Perhaps I wasn't clear enough, so please allow me to clarify a bit.
To me, it isn't just about using the CLI or TUI utilities. It's
actually about using standard CLI/TUI utilities, such as using git
directly, instead of using some specialized CLI/TUI utilities that
are made to interact with a forge in a forge-specific way.
Perhaps you'll ask why I find using a forge such a bad thing, so
I'll try to provide an answer in advance.
Git is a widespread, standard system of utilities that isn't backed
by some company that sees its profit as the main goal. On the other
hand, most forges are backed by a company, and as we know, companies
don't last forever, and they often pivot due to business decisions.
What we don't want is to tie the project into something that isn't
expected to virtually last forever.
It's similar to the concept of bit rot. A lot of data is poured
into something and it slowly starts to degrade over time. Though,
in the case of a forge becoming no longer available it wouldn't be
a gradual decay, but an abrupt disruption that would make all the
data unavailable and unusable. Of course, the data perhaps can be
exported from a forge in some format, including the discussions,
but who's going to sift through years worth of such data and make
it usable through some other interface or in some other format?
Frankly, I wouldn't see that happening.
On the other hand, discussion in form of mailing lists aren't tied
to anything, the underlying data format has been around for decades,
and the raw data can be accessed by any editor or viewer, such as
less(1). It isn't tied to anything.
>> As a Linux kernel subsystem maintainer, I am super grateful for those
>> who do code reviews and those who work test regressions, because in
>> general, that which doesn't get done by other developers ends up
>> getting done by the maintainers and project leads if it's going to
>> happen at all.
>>
>> When it comes to requests like "you should migrate the project to use
>> some forge web site, because we can't be bothered to use e-mail, and
>> web interfaces are the new hotness", the entitlement that comes from
>> that request (which is in the subject line of this thread), can
>> sometimes be a bit frustrating.
>>
>> Going back to the original topic of this thread, my personal
>> experience has been that the *vest* percentage of pull requests that I
>> get from github tend to be drive-by pull requests that are very low
>> quality, especially compared to those that I get via the mailing list.
>> So making a change to use a forge which might result in a larger
>> number of lower quality code contributions, when code review bandwidth
>> might be more of a bottlenck, might not be as appealing as some might
>> think.
>
> Again, you are aware of the fact that Git collaboration websites
> provide a powerful user rights management?
> (https://docs.gitlab.com/ee/user/permissions.html
> https://docs.github.com/en/organizations/managing-user-access-to-your-organizations-repositories/managing-repository-roles/repository-roles-for-an-organization)
>
> Using Git collaboration websites you can easily control and filter who
> will be contributing. And you are able to focus on issues and filter
> out spammers. It's quite the contrary of of what you have now with
> your mailing list. A vanilla student from the "axis of evil" could
> bomb your mailing list in a snap by just registering a dozen new
> e-mail accounts and writing a script that bloated your mailing list.
> And you cannot thwart that at all.
I don't remember such cases. It doesn't mean something like that will
never happen, though. Also, pretty much anyone can create dozens of
fake accounts on a forge and do malicious things.
Please note that creating an account of any kind is often unacceptable
to many people. I was a bit surprised to discover that.
> With your mailing list approach you don't have ANY sort of gateway to
> keep away spam or "low quality" contributions other by means of the
> intrinsic clumsiness and intricateness of a mailing list. After having
> subscribed to your mailing list, my e-mail spam rate immediately
> increased significantly.
You must be having bad luck for some reason. Knocking on wood,
I've received zero spam emails directed to my email address since
subscribing to the list.
> Again, on Git collaboration websites you can hide your personal access
> information and focus on your repository tasks rather than wasting
> your time on cumbersome additional and unneccessary work.
If you ask me, one's identity shouldn't be hidden when one willingly
contributes to a public project. Taking part in the discussions is
also a way of contributing.
> I'm getting the impression that you didn't yet seriously investigate
> on the features these Git collaboration websites provide.
>
> Let me finish this thread from my side now. I suggested a way to
> improve your daily business by employing tools that have been
> established and proven to raise code and documentation quality and
> that will allow you to focus on important tasks rather than wasting
> time on an old fashioned workflow. Well, it's up to you now to decide
> whether to stick here or to migrate.
As I noted already, these days it's expected too much that some
utilities will do the programmer's work. Also, being unable to
follow a moderately busy mailing list, such as the git's, may also
show that one needs to improve their own skills in some areas.
In the case of high-volume mailing lists, I admit that things can
be different and much harder to handle. That's why I plan to work
on extending mlmmj to support muting and unmuting threads. [1]
[1]
https://lore.kernel.org/git/93be64af474b228e914a4c39443b5a9c@manjaro.org/
^ permalink raw reply
* Re: [PATCH v5 5/7] completion: new function __git_complete_log_opts
From: Patrick Steinhardt @ 2024-02-06 7:40 UTC (permalink / raw)
To: Britton Leo Kerin; +Cc: git, Junio C Hamano
In-Reply-To: <20240206020930.312164-6-britton.kerin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1999 bytes --]
On Mon, Feb 05, 2024 at 05:09:28PM -0900, Britton Leo Kerin wrote:
> The options accepted by git-log are also accepted by at least one other
> command (git-bisect). Factor the common option completion code into
> a new function and use it from _git_log.
>
> Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
> ---
> contrib/completion/git-completion.bash | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 57c6e09968..8c3b1b8e96 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2089,10 +2089,12 @@ __git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-c
> __git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
> __git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
>
> -_git_log ()
> +# Complete porcelain (i.e. not git-rev-list) options and at least some
> +# option arguments accepted by git-log. Note that this same set of options
> +# are also accepted by some other git commands besides git-log.
> +__git_complete_log_opts ()
> {
> - __git_has_doubledash && return
> - __git_find_repo_path
> + COMPREPLY=""
Nit: this is indented with spaces instead of tabs.
It would also be nice to mention in the commit message why we empty
COMPREPLY here. It didn't happen before either, so this is essentially
not a no-op conversion.
Patrick
>
> local merge=""
> if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
> @@ -2186,6 +2188,16 @@ _git_log ()
> return
> ;;
> esac
> +}
> +
> +_git_log ()
> +{
> + __git_has_doubledash && return
> + __git_find_repo_path
> +
> + __git_complete_log_opts
> + [ -z "$COMPREPLY" ] || return
> +
> __git_complete_revlist
> }
>
> --
> 2.43.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ 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