* Re: [RESEND v2] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Phillip Wood @ 2023-10-24 9:22 UTC (permalink / raw)
To: Oswald Buddenhagen, phillip.wood
Cc: git, Junio C Hamano, Christian Couder, Charvi Mendiratta,
Marc Branchaud, Johannes Sixt
In-Reply-To: <ZTayxB0Nm7AEyafp@ugly>
Hi Oswald
On 23/10/2023 18:52, Oswald Buddenhagen wrote:
> On Mon, Oct 23, 2023 at 05:01:02PM +0100, Phillip Wood wrote:
>> On 23/10/2023 14:00, Oswald Buddenhagen wrote:
>>> +unless "fixup -c" is used. In the latter case, the message is obtained
>>> +only from the "fixup -c" commit (having more than one of these is
>>> +incorrect).
>>
>> This change is incorrect - it is perfectly fine to have more than one
>> "fixup -c" command. In that case we use the message of the commit of
>> the final "fixup -c" command.
>>
> i know that this is the case, see the previous thread (which i failed to
> link by header, cf.
> https://lore.kernel.org/all/20231020092707.917514-1-oswald.buddenhagen@gmx.de/T/#u ).
Ah, I see Marc has already raised this point.
>> One case where there can be multiple "fixup -c" commands is when a
>> commit has been reworded several times via "git commit
>> --fixup=reword:<commit>" and the user runs "git rebase --autosquash"
>>
> a cleaner solution would be recognizing the situation and not generating
> these contradicting commands in the first place. of course that would be
> more complexity, but it would also allow catching accidental use.
>
> of course i can go back to documenting the status quo, but it seems kind
> of wrong.
I agree there is an argument for improving the implementation of
--autosquash but until we do I think it is counterproductive to change
the documentation like this as it will cause users to wonder why "rebase
--autosquash" generates a todo list that is incorrect according to the
documentation.
>> In the case of
>>
>> pick A
>> fixup -C B
>>
>> don't we keep the authorship from A and just use the commit message
>> from B?
>>
> uhm. we clearly do. that means i was given incorrect advice in
> https://lore.kernel.org/all/YjXRM5HiRizZ035p@ugly/T/#u (and so the
> thread is still looking for a resolution) ...
I'll take a look at that thread and comment there.
I do think it is a good idea to document where the authorship of a
rebased commit comes from.
Best Wishes
Phillip
^ permalink raw reply
* Re: using oldest date when squashing commits
From: Phillip Wood @ 2023-10-24 9:26 UTC (permalink / raw)
To: Johannes Sixt, Oswald Buddenhagen; +Cc: git
In-Reply-To: <9fae5292-d58f-95da-245b-6e205383cb50@kdbg.org>
On 20/03/2022 08:05, Johannes Sixt wrote:
> Am 19.03.22 um 13:48 schrieb Oswald Buddenhagen:
>> during interactive rebasing, i sometimes find it necessary to move a
>> hunk from one commit to a later one in the branch. now, if that hunk
>> cannot be re-ordered with the later commit due to conflicting with it,
>> it becomes necessary to squash the later commit onto a temporary commit
>> created from the extracted hunk, not the other way around (or using a
>> stash). unfortunately, this causes the author date of the later commit
>> to be reset, which can rather seriously falsify the date if the branch
>> is long-lived.
>
> You want `fixup -C` in the todo-list. See the hints near the end of the
> todo-list.
Unfortunately "fixup -C" only copies the commit message not the
authorship (that's usually a good thing but not it means it wont work
for what Oswald wants to do). Maybe we should add another flag for
fixup/squash commands to take the authorship from that commit. In the
meantime creating the temporary commit with "git commit -C" is probably
the easiest way to keep the original authorship.
Best Wishes
Phillip
^ permalink raw reply
* Re: using oldest date when squashing commits
From: Oswald Buddenhagen @ 2023-10-24 10:18 UTC (permalink / raw)
To: phillip.wood; +Cc: Johannes Sixt, git
In-Reply-To: <a99b16a8-a06c-4d38-bb78-46ce17411597@gmail.com>
On Tue, Oct 24, 2023 at 10:26:29AM +0100, Phillip Wood wrote:
>On 20/03/2022 08:05, Johannes Sixt wrote:
>> Am 19.03.22 um 13:48 schrieb Oswald Buddenhagen:
>>> during interactive rebasing, i sometimes find it necessary to move a
>>> hunk from one commit to a later one in the branch. now, if that hunk
>>> cannot be re-ordered with the later commit due to conflicting with it,
>>> it becomes necessary to squash the later commit onto a temporary commit
>>> created from the extracted hunk, not the other way around (or using a
>>> stash). unfortunately, this causes the author date of the later commit
>>> to be reset, which can rather seriously falsify the date if the branch
>>> is long-lived.
>>
>> You want `fixup -C` in the todo-list. See the hints near the end of the
>> todo-list.
>
>Unfortunately "fixup -C" only copies the commit message not the
>authorship
>(that's usually a good thing
>
why? what would that be useful for? it seems rather counter-intuitive.
it's also inconsistent with commit -c/-C's behavior, which seems like a
red flag to me.
>but not it means it wont work for what Oswald wants to do).
>Maybe we should add another flag for fixup/squash commands to take the
>authorship from that commit.
>
that's a possibility. but given the above, it might be better to simply
change the behavior of -c/-C to keep the UI lean and consistent with
commit's behavior.
regards
^ permalink raw reply
* ls-remote bug
From: Lior Zeltzer @ 2023-10-24 10:55 UTC (permalink / raw)
To: git@vger.kernel.org
In-Reply-To: <BL0PR18MB2130C672D503E49F955E04E8BADFA@BL0PR18MB2130.namprd18.prod.outlook.com>
>uname -a
Linux dc3lp-veld0045 3.10.0-1160.21.1.el7.x86_64 #1 SMP Tue Mar 16 18:28:22 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Gerrit version :
3.8.0
Bug description :
When running ls-remote : sometime data gets cut in the middle
Reproducing :
You need a few files with a few repo names (I used 4 files with 10 repos each)
Call then l1..l4
And the code below just cd into each of them does ls-remote twice and compares the data
Doing it in parallel on all lists.
Data received in both ls-remotes should be the same , if not, it prints ***
Repos should contain a lot of tags and refs
Note :
1. without stderr redirection (2>&1) all works well
2. On local repos (not through gerrit) all works well
I compared various git vers and found the bug to be between 2.31.8 and 2.32.0
Comparing ls-remote.c file between those vers gave me :
Lines :
if (transport_disconnect(transport))
return 1;
moved to end of sub
copying ls-remote.c from 2.31.8 to 2.32.0 - fixed the bug
Code reproducing bug :
#!/proj/mislcad/areas/DAtools/tools/perl/5.10.1/bin/perl -w
use strict;
use Cwd qw(cwd);
my $count = 4;
for my $f (1..$count) {
my $child = fork();
if (!$child) {
my $curr = cwd();
my @repos = `cat l$f`;
foreach my $repo (@repos) {
chomp $repo;
print "$repo\n";
chdir($repo);
my $remote_tags_str = `git ls-remote 2>&1`;
my $remote_tags_str2 = `git ls-remote 2>&1 `;
chdir($curr);
if ( $remote_tags_str ne $remote_tags_str2) {
print "***\n";
}
}
exit(0);
}
}
while (wait != -1) {}
1;
^ permalink raw reply
* Re: [PATCH v3 3/3] rev-list: add commit object support in `--missing` option
From: Karthik Nayak @ 2023-10-24 11:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, ps
In-Reply-To: <xmqq1qdptffk.fsf@gitster.g>
On Fri, Oct 20, 2023 at 6:41 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Rather, I was wondering if we need to use object flags to mark these
> > objects, or can do what we want to do without using any object flags
> > at all. For the purpose of reporting "missing" objects, wouldn't it
> > be sufficient to walk the object graph and report our findings as we
> > go? To avoid reporting the same object twice, as we reasonably can
> > expect that the missing objects are minority (compared to the total
> > number of objects), perhaps the codepath that makes such a report
> > can use a hashmap of object_ids or something, for example.
>
> Digging from the bottom,
>
> * builtin/rev-list.c:show_commit() gets "struct rev_list_info *"
> that has "struct rev_info *" [*].
>
> * list-objects.c:do_traverse() calls revision.c:get_revision() to
> obtain commits, some of which may be missing ones, and things
> behind get_revision() are responsible for marking the commit as
> missing. It has "struct traversal_context *", among whose
> members is the "revs" member that is the "struct rev_info *".
>
> * revision.c:get_revision() and machinery behind it ultimately
> discovers a missing commit in the revision.c:process_parents()
> that loops over the parents commit_list. It of course has access
> to "struct rev_info *".
>
> So, presumably, if we add a new member to "struct rev_info" that
> optionally [*] points at an oidset that records the object names of
> missing objects we discovered so far (i.e., the set of missing
> objects), the location we set the MISSING bit of a commit can
> instead add the object name of the commit to the set. And we can
> export a function that takes "struct rev_info *" and "struct object
> *" (or "struct object_id *") to check for membership in the "set of
> missing objects", which would be used where we checked the MISSING
> bit of a commit.
>
> I do not know the performance implications of going this route, but
> if we do not find a suitable vacant bit, we do not have to use any
> object flags bit to do this, if we go this route, I would think. I
> may be missing some details that breaks the above outline, though.
>
>
> [Footnotes]
>
> * A potential #leftoverbits tangent.
>
> Why is "rev_list_info" structure declared in <bisect.h>? I
> suspect that this is a fallout from recent header file shuffling,
> but given who uses it (among which is rev-list:show_commit() that
> has very little to do with bisection and uses the information in
> rev_list_info when doing its normal non-bisect things), it does
> not make much sense.
>
> * When .do_not_die_on_missing_objects is false, it can and should
> be left NULL, but presumably we use the "do not die" bit even
> when we are not necessarily collecting the missing objects? So
> the new member cannot replace the "do not die" bit completely.
Thanks for the suggestion, this does seem like a good way to go ahead without
using flags. The only performance issue being if there are too many commits
which are missing, then oidset would be large.
But I think that's okay though.
> Thanks for researching. It sounds like it may be a better bit to
> steal than the one used by the commit-graph, as long as there is no
> reason to expect that blame may want to work in a corrupt repository
> with missing objects, but when it happens, we may regret the
> decision we are making here.
>
I don't see blame working with missing commits though, because it relies on
parsing commits to get information to show to the user. So I think it's a safe
bit to steal. Also, when the time comes we could always release the bit and
move to the solution you mentioned above.
Anyways on the whole I think keeping it future compatible makes a lot
more sense.
I'll send a patch series to implement an oidset instead of flags soon.
- Karthik
^ permalink raw reply
* [PATCH v4 1/3] revision: rename bit to `do_not_die_on_missing_objects`
From: Karthik Nayak @ 2023-10-24 12:26 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
In-Reply-To: <20231024122631.158415-1-karthik.188@gmail.com>
The bit `do_not_die_on_missing_tree` is used in revision.h to ensure the
revision walker does not die when encountering a missing tree. This is
currently exclusively set within `builtin/rev-list.c` to ensure the
`--missing` option works with missing trees.
In the upcoming commits, we will extend `--missing` to also support
missing commits. So let's rename the bit to
`do_not_die_on_missing_objects`, which is object type agnostic and can
be used for both trees/commits.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/reflog.c | 2 +-
builtin/rev-list.c | 2 +-
list-objects.c | 2 +-
revision.h | 17 +++++++++--------
4 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index df63a5892e..9e369a5977 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -298,7 +298,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
struct rev_info revs;
repo_init_revisions(the_repository, &revs, prefix);
- revs.do_not_die_on_missing_tree = 1;
+ revs.do_not_die_on_missing_objects = 1;
revs.ignore_missing = 1;
revs.ignore_missing_links = 1;
if (verbose)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ff715d6918..ea77489c38 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -561,7 +561,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
}
if (arg_missing_action)
- revs.do_not_die_on_missing_tree = 1;
+ revs.do_not_die_on_missing_objects = 1;
argc = setup_revisions(argc, argv, &revs, &s_r_opt);
diff --git a/list-objects.c b/list-objects.c
index c25c72b32c..47296dff2f 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -177,7 +177,7 @@ static void process_tree(struct traversal_context *ctx,
is_promisor_object(&obj->oid))
return;
- if (!revs->do_not_die_on_missing_tree)
+ if (!revs->do_not_die_on_missing_objects)
die("bad tree object %s", oid_to_hex(&obj->oid));
}
diff --git a/revision.h b/revision.h
index 50091bbd13..c73c92ef40 100644
--- a/revision.h
+++ b/revision.h
@@ -212,18 +212,19 @@ struct rev_info {
/*
* Blobs are shown without regard for their existence.
- * But not so for trees: unless exclude_promisor_objects
+ * But not so for trees/commits: unless exclude_promisor_objects
* is set and the tree in question is a promisor object;
* OR ignore_missing_links is set, the revision walker
- * dies with a "bad tree object HASH" message when
- * encountering a missing tree. For callers that can
- * handle missing trees and want them to be filterable
+ * dies with a "bad <type> object HASH" message when
+ * encountering a missing object. For callers that can
+ * handle missing trees/commits and want them to be filterable
* and showable, set this to true. The revision walker
- * will filter and show such a missing tree as usual,
- * but will not attempt to recurse into this tree
- * object.
+ * will filter and show such a missing object as usual,
+ * but will not attempt to recurse into this tree/commit
+ * object. The revision walker will also set the MISSING
+ * flag for such objects.
*/
- do_not_die_on_missing_tree:1,
+ do_not_die_on_missing_objects:1,
/* for internal use only */
exclude_promisor_objects:1;
--
2.42.0
^ permalink raw reply related
* [PATCH v4 2/3] rev-list: move `show_commit()` to the bottom
From: Karthik Nayak @ 2023-10-24 12:26 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
In-Reply-To: <20231024122631.158415-1-karthik.188@gmail.com>
The `show_commit()` function already depends on `finish_commit()`, and
in the upcoming commit, we'll also add a dependency on
`finish_object__ma()`. Since in C symbols must be declared before
they're used, let's move `show_commit()` below both `finish_commit()`
and `finish_object__ma()`, so the code is cleaner as a whole without the
need for declarations.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/rev-list.c | 85 +++++++++++++++++++++++-----------------------
1 file changed, 42 insertions(+), 43 deletions(-)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ea77489c38..98542e8b3c 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -100,7 +100,48 @@ static off_t get_object_disk_usage(struct object *obj)
return size;
}
-static void finish_commit(struct commit *commit);
+static inline void finish_object__ma(struct object *obj)
+{
+ /*
+ * Whether or not we try to dynamically fetch missing objects
+ * from the server, we currently DO NOT have the object. We
+ * can either print, allow (ignore), or conditionally allow
+ * (ignore) them.
+ */
+ switch (arg_missing_action) {
+ case MA_ERROR:
+ die("missing %s object '%s'",
+ type_name(obj->type), oid_to_hex(&obj->oid));
+ return;
+
+ case MA_ALLOW_ANY:
+ return;
+
+ case MA_PRINT:
+ oidset_insert(&missing_objects, &obj->oid);
+ return;
+
+ case MA_ALLOW_PROMISOR:
+ if (is_promisor_object(&obj->oid))
+ return;
+ die("unexpected missing %s object '%s'",
+ type_name(obj->type), oid_to_hex(&obj->oid));
+ return;
+
+ default:
+ BUG("unhandled missing_action");
+ return;
+ }
+}
+
+static void finish_commit(struct commit *commit)
+{
+ free_commit_list(commit->parents);
+ commit->parents = NULL;
+ free_commit_buffer(the_repository->parsed_objects,
+ commit);
+}
+
static void show_commit(struct commit *commit, void *data)
{
struct rev_list_info *info = data;
@@ -219,48 +260,6 @@ static void show_commit(struct commit *commit, void *data)
finish_commit(commit);
}
-static void finish_commit(struct commit *commit)
-{
- free_commit_list(commit->parents);
- commit->parents = NULL;
- free_commit_buffer(the_repository->parsed_objects,
- commit);
-}
-
-static inline void finish_object__ma(struct object *obj)
-{
- /*
- * Whether or not we try to dynamically fetch missing objects
- * from the server, we currently DO NOT have the object. We
- * can either print, allow (ignore), or conditionally allow
- * (ignore) them.
- */
- switch (arg_missing_action) {
- case MA_ERROR:
- die("missing %s object '%s'",
- type_name(obj->type), oid_to_hex(&obj->oid));
- return;
-
- case MA_ALLOW_ANY:
- return;
-
- case MA_PRINT:
- oidset_insert(&missing_objects, &obj->oid);
- return;
-
- case MA_ALLOW_PROMISOR:
- if (is_promisor_object(&obj->oid))
- return;
- die("unexpected missing %s object '%s'",
- type_name(obj->type), oid_to_hex(&obj->oid));
- return;
-
- default:
- BUG("unhandled missing_action");
- return;
- }
-}
-
static int finish_object(struct object *obj, const char *name UNUSED,
void *cb_data)
{
--
2.42.0
^ permalink raw reply related
* [PATCH v4 0/3] rev-list: add support for commits in `--missing`
From: Karthik Nayak @ 2023-10-24 12:26 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
In-Reply-To: <20231019121024.194317-1-karthik.188@gmail.com>
The `--missing` option in git-rev-list(1) was introduced intitally
to deal with missing blobs in the context of promissory notes.
Eventually the option was extended to also support tree objects in
7c0fe330d5 (rev-list: handle missing tree objects properly,2018-10-05).
This patch series extends the `--missing` option to also add support for
commit objects. We do this by introducing a new flag `MISSING` which is
added whenever we encounter a missing commit during traversal. Then in
`builtin/rev-list` we check for this flag and take the appropriate
action based on the `--missing=*` option used.
This series is an alternate to the patch series I had posted earlier:
https://lore.kernel.org/git/20230908174208.249184-1-karthik.188@gmail.com/.
In that patch, we introduced an option `--ignore-missing-links` which
was added to expose the `ignore_missing_links` bit to the user. The
issue in that patch was that, the option `--ignore-missing-links` didn't
play well the pre-existing `--missing` option. This series avoids that
route and just extends the `--missing` option for commits to solve the
same problem.
V3 of the series can be found here: https://lore.kernel.org/git/20231019121024.194317-1-karthik.188@gmail.com/T/#mf6a442e06f323a78a45af086ddd353998bab0052
Changes from v3:
- Instead of using flags on the object level as in the previous vesrions, we
add and use a missing_objects oidset in rev_info. This avoids flag bit-collision
since we were using an existing bit and doesn't require extending the flag size
as done in v3.
Changelog vs v3:
1: 8c469cf479 = 1: 8c469cf479 revision: rename bit to `do_not_die_on_missing_objects`
2: 76ce43d973 = 2: 76ce43d973 rev-list: move `show_commit()` to the bottom
3: 4c640f9ab4 ! 3: d892f0b82d rev-list: add commit object support in `--missing` option
@@ Commit message
a fatal error.
Let's extend the functionality of `--missing` option to also support
- commit objects. This is done by adding a new `MISSING` flag that the
- revision walker sets whenever it encounters a missing tree/commit. The
- revision walker will now continue the traversal and call `show_commit()`
- even for missing commits. In rev-list we can then check for this flag
- and call the existing code for parsing `--missing` objects.
+ commit objects. This is done by adding a `missing_objects` field to
+ `rev_info`. This field is an `oidset` to which we'll add the missing
+ commits as we encounter them. The revision walker will now continue the
+ traversal and call `show_commit()` even for missing commits. In rev-list
+ we can then check if the commit is a missing commit and call the
+ existing code for parsing `--missing` objects.
A scenario where this option would be used is to find the boundary
objects between different object directories. Consider a repository with
@@ Commit message
alternate object directory allows us to find the boundary objects
between the main and alternate object directory.
+ Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
## builtin/rev-list.c ##
@@ builtin/rev-list.c: static void show_commit(struct commit *commit, void *data)
display_progress(progress, ++progress_counter);
+ if (revs->do_not_die_on_missing_objects &&
-+ commit->object.flags & MISSING) {
++ oidset_contains(&revs->missing_objects, &commit->object.oid)) {
+ finish_object__ma(&commit->object);
+ return;
+ }
@@ builtin/rev-list.c: static void show_commit(struct commit *commit, void *data)
## list-objects.c ##
@@ list-objects.c: static void do_traverse(struct traversal_context *ctx)
- * an uninteresting boundary commit may not have its tree
- * parsed yet, but we are not going to show them anyway
*/
-- if (!ctx->revs->tree_objects)
-+ if (!ctx->revs->tree_objects || commit->object.flags & MISSING)
+ if (!ctx->revs->tree_objects)
; /* do not bother loading tree */
++ else if (ctx->revs->do_not_die_on_missing_objects &&
++ oidset_contains(&ctx->revs->missing_objects, &commit->object.oid))
++ ;
else if (repo_get_commit_tree(the_repository, commit)) {
struct tree *tree = repo_get_commit_tree(the_repository,
-
- ## object.h ##
-@@ object.h: void object_array_init(struct object_array *array);
-
- /*
- * object flag allocation:
-- * revision.h: 0---------10 15 23------27
-+ * revision.h: 0---------10 15 22------28
- * fetch-pack.c: 01 67
- * negotiator/default.c: 2--5
- * walker.c: 0-2
-@@ object.h: void object_array_init(struct object_array *array);
- * builtin/show-branch.c: 0-------------------------------------------26
- * builtin/unpack-objects.c: 2021
- */
--#define FLAG_BITS 28
-+#define FLAG_BITS 29
-
- #define TYPE_BITS 3
-
+ commit);
## revision.c ##
+@@
+ #include "object-name.h"
+ #include "object-file.h"
+ #include "object-store-ll.h"
++#include "oidset.h"
+ #include "tag.h"
+ #include "blob.h"
+ #include "tree.h"
@@ revision.c: static int process_parents(struct rev_info *revs, struct commit *commit,
- struct commit_list *parent = commit->parents;
- unsigned pass_flags;
-- if (commit->object.flags & ADDED)
-+ if (commit->object.flags & (ADDED | MISSING))
+ if (commit->object.flags & ADDED)
return 0;
++ if (revs->do_not_die_on_missing_objects &&
++ oidset_contains(&revs->missing_objects, &commit->object.oid))
++ return 0;
commit->object.flags |= ADDED;
+ if (revs->include_check &&
@@ revision.c: static int process_parents(struct rev_info *revs, struct commit *commit,
for (parent = commit->parents; parent; parent = parent->next) {
struct commit *p = parent->item;
@@ revision.c: static int process_parents(struct rev_info *revs, struct commit *com
+ if (!revs->do_not_die_on_missing_objects)
+ return -1;
+ else
-+ p->object.flags |= MISSING;
++ oidset_insert(&revs->missing_objects, &p->object.oid);
}
if (revs->sources) {
char **slot = revision_sources_at(revs->sources, p);
+@@ revision.c: int prepare_revision_walk(struct rev_info *revs)
+ FOR_EACH_OBJECT_PROMISOR_ONLY);
+ }
+
++ if (revs->do_not_die_on_missing_objects)
++ oidset_init(&revs->missing_objects, 0);
++
+ if (!revs->reflog_info)
+ prepare_to_use_bloom_filter(revs);
+ if (!revs->unsorted_input)
## revision.h ##
@@
- #define ANCESTRY_PATH (1u<<27)
- #define ALL_REV_FLAGS (((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR | PULL_MERGE)
+ #include "commit.h"
+ #include "grep.h"
+ #include "notes.h"
++#include "oidset.h"
+ #include "pretty.h"
+ #include "diff.h"
+ #include "commit-slab-decl.h"
+@@ revision.h: struct rev_info {
-+#define MISSING (1u<<28)
+ /* Location where temporary objects for remerge-diff are written. */
+ struct tmp_objdir *remerge_objdir;
+
- #define DECORATE_SHORT_REFS 1
- #define DECORATE_FULL_REFS 2
++ /* Missing objects to be tracked without failing traversal. */
++ struct oidset missing_objects;
+ };
+ /**
## t/t6022-rev-list-missing.sh (new) ##
@@
Karthik Nayak (3):
revision: rename bit to `do_not_die_on_missing_objects`
rev-list: move `show_commit()` to the bottom
rev-list: add commit object support in `--missing` option
builtin/reflog.c | 2 +-
builtin/rev-list.c | 93 +++++++++++++++++++------------------
list-objects.c | 5 +-
revision.c | 16 ++++++-
revision.h | 21 +++++----
t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++
6 files changed, 155 insertions(+), 56 deletions(-)
create mode 100755 t/t6022-rev-list-missing.sh
--
2.42.0
^ permalink raw reply
* [PATCH v4 3/3] rev-list: add commit object support in `--missing` option
From: Karthik Nayak @ 2023-10-24 12:26 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, ps
In-Reply-To: <20231024122631.158415-1-karthik.188@gmail.com>
The `--missing` object option in rev-list currently works only with
missing blobs/trees. For missing commits the revision walker fails with
a fatal error.
Let's extend the functionality of `--missing` option to also support
commit objects. This is done by adding a `missing_objects` field to
`rev_info`. This field is an `oidset` to which we'll add the missing
commits as we encounter them. The revision walker will now continue the
traversal and call `show_commit()` even for missing commits. In rev-list
we can then check if the commit is a missing commit and call the
existing code for parsing `--missing` objects.
A scenario where this option would be used is to find the boundary
objects between different object directories. Consider a repository with
a main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate
object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a
repository, using the `--missing=print` option while disabling the
alternate object directory allows us to find the boundary objects
between the main and alternate object directory.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/rev-list.c | 6 +++
list-objects.c | 3 ++
revision.c | 16 +++++++-
revision.h | 4 ++
t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++++++++++
5 files changed, 101 insertions(+), 2 deletions(-)
create mode 100755 t/t6022-rev-list-missing.sh
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 98542e8b3c..37b52520b5 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -149,6 +149,12 @@ static void show_commit(struct commit *commit, void *data)
display_progress(progress, ++progress_counter);
+ if (revs->do_not_die_on_missing_objects &&
+ oidset_contains(&revs->missing_objects, &commit->object.oid)) {
+ finish_object__ma(&commit->object);
+ return;
+ }
+
if (show_disk_usage)
total_disk_usage += get_object_disk_usage(&commit->object);
diff --git a/list-objects.c b/list-objects.c
index 47296dff2f..260089388c 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -389,6 +389,9 @@ static void do_traverse(struct traversal_context *ctx)
*/
if (!ctx->revs->tree_objects)
; /* do not bother loading tree */
+ else if (ctx->revs->do_not_die_on_missing_objects &&
+ oidset_contains(&ctx->revs->missing_objects, &commit->object.oid))
+ ;
else if (repo_get_commit_tree(the_repository, commit)) {
struct tree *tree = repo_get_commit_tree(the_repository,
commit);
diff --git a/revision.c b/revision.c
index 219dc76716..e60646c1a7 100644
--- a/revision.c
+++ b/revision.c
@@ -6,6 +6,7 @@
#include "object-name.h"
#include "object-file.h"
#include "object-store-ll.h"
+#include "oidset.h"
#include "tag.h"
#include "blob.h"
#include "tree.h"
@@ -1112,6 +1113,9 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
if (commit->object.flags & ADDED)
return 0;
+ if (revs->do_not_die_on_missing_objects &&
+ oidset_contains(&revs->missing_objects, &commit->object.oid))
+ return 0;
commit->object.flags |= ADDED;
if (revs->include_check &&
@@ -1168,7 +1172,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
for (parent = commit->parents; parent; parent = parent->next) {
struct commit *p = parent->item;
int gently = revs->ignore_missing_links ||
- revs->exclude_promisor_objects;
+ revs->exclude_promisor_objects ||
+ revs->do_not_die_on_missing_objects;
if (repo_parse_commit_gently(revs->repo, p, gently) < 0) {
if (revs->exclude_promisor_objects &&
is_promisor_object(&p->object.oid)) {
@@ -1176,7 +1181,11 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
break;
continue;
}
- return -1;
+
+ if (!revs->do_not_die_on_missing_objects)
+ return -1;
+ else
+ oidset_insert(&revs->missing_objects, &p->object.oid);
}
if (revs->sources) {
char **slot = revision_sources_at(revs->sources, p);
@@ -3800,6 +3809,9 @@ int prepare_revision_walk(struct rev_info *revs)
FOR_EACH_OBJECT_PROMISOR_ONLY);
}
+ if (revs->do_not_die_on_missing_objects)
+ oidset_init(&revs->missing_objects, 0);
+
if (!revs->reflog_info)
prepare_to_use_bloom_filter(revs);
if (!revs->unsorted_input)
diff --git a/revision.h b/revision.h
index c73c92ef40..f6bf422f0e 100644
--- a/revision.h
+++ b/revision.h
@@ -4,6 +4,7 @@
#include "commit.h"
#include "grep.h"
#include "notes.h"
+#include "oidset.h"
#include "pretty.h"
#include "diff.h"
#include "commit-slab-decl.h"
@@ -373,6 +374,9 @@ struct rev_info {
/* Location where temporary objects for remerge-diff are written. */
struct tmp_objdir *remerge_objdir;
+
+ /* Missing objects to be tracked without failing traversal. */
+ struct oidset missing_objects;
};
/**
diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
new file mode 100755
index 0000000000..40265a4f66
--- /dev/null
+++ b/t/t6022-rev-list-missing.sh
@@ -0,0 +1,74 @@
+#!/bin/sh
+
+test_description='handling of missing objects in rev-list'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+# We setup the repository with two commits, this way HEAD is always
+# available and we can hide commit 1.
+test_expect_success 'create repository and alternate directory' '
+ test_commit 1 &&
+ test_commit 2 &&
+ test_commit 3
+'
+
+for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
+do
+ test_expect_success "rev-list --missing=error fails with missing object $obj" '
+ oid="$(git rev-parse $obj)" &&
+ path=".git/objects/$(test_oid_to_path $oid)" &&
+
+ mv "$path" "$path.hidden" &&
+ test_when_finished "mv $path.hidden $path" &&
+
+ test_must_fail git rev-list --missing=error --objects \
+ --no-object-names HEAD
+ '
+done
+
+for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
+do
+ for action in "allow-any" "print"
+ do
+ test_expect_success "rev-list --missing=$action with missing $obj" '
+ oid="$(git rev-parse $obj)" &&
+ path=".git/objects/$(test_oid_to_path $oid)" &&
+
+ # Before the object is made missing, we use rev-list to
+ # get the expected oids.
+ git rev-list --objects --no-object-names \
+ HEAD ^$obj >expect.raw &&
+
+ # Blobs are shared by all commits, so evethough a commit/tree
+ # might be skipped, its blob must be accounted for.
+ if [ $obj != "HEAD:1.t" ]; then
+ echo $(git rev-parse HEAD:1.t) >>expect.raw &&
+ echo $(git rev-parse HEAD:2.t) >>expect.raw
+ fi &&
+
+ mv "$path" "$path.hidden" &&
+ test_when_finished "mv $path.hidden $path" &&
+
+ git rev-list --missing=$action --objects --no-object-names \
+ HEAD >actual.raw &&
+
+ # When the action is to print, we should also add the missing
+ # oid to the expect list.
+ case $action in
+ allow-any)
+ ;;
+ print)
+ grep ?$oid actual.raw &&
+ echo ?$oid >>expect.raw
+ ;;
+ esac &&
+
+ sort actual.raw >actual &&
+ sort expect.raw >expect &&
+ test_cmp expect actual
+ '
+ done
+done
+
+test_done
--
2.42.0
^ permalink raw reply related
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Uwe Kleine-König @ 2023-10-24 13:00 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: git, Luben Tuikov, entwicklung
In-Reply-To: <68d7e5c3-6b4a-4d0d-9885-f3d4e2199f26@amd.com>
[-- Attachment #1: Type: text/plain, Size: 2375 bytes --]
Hello,
On Fri, Oct 20, 2023 at 05:06:36PM -0400, Michael Strawbridge wrote:
> On 10/20/23 06:04, Uwe Kleine-König wrote:
> > hello,
> >
> > On Fri, Oct 13, 2023 at 04:14:37PM +0200, Uwe Kleine-König wrote:
> >> Hello,
> >>
> >> $ git send-email --to 'A B <a@b.org>, C D <c@d.org>' lala.patch
> >> Use of uninitialized value $address in sprintf at /usr/lib/git-core/git-send-email line 1172.
> >> error: unable to extract a valid address from:
> >>
> >> This happens for me with git 2.42.0 and also on master (59167d7d09fd, "The seventeenth batch").
> >>
> >> Bisection points at
> >>
> >> a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")
> >>
> >> I didn't try to understand that change and fix the problem.
> >
> > Another (similar?) problem with non-ascii-chars:
> >
> > $ git send-email --to 'Will Deacon <will@kernel.org>' --to 'Krzysztof Wilczyński <kw@linux.com>' --to 'Lorenzo Pieralisi <lpieralisi@kernel.org>' --cc 'Rob Herring <robh@kernel.org>' --to 'Bjorn Helgaas <bhelgaas@google.com>' --cc 'linux-pci@vger.kernel.org' --cc kernel@pengutronix.de -1 --base=@~
> > Use of uninitialized value $address in sprintf at /home/uwe/gsrc/git/git-send-email line 1162.
> > error: unable to extract a valid address from:
> >
> > Bisection points to the same commit, when dropping ń in Krzysztof's
> > name, it works fine.
> >
> This is interesting. Thanks for reporting it. If you are able, could you please try the patches found in the below threads:
> - https://public-inbox.org/git/20230918212004.GC2163162@coredump.intra.peff.net/T/#mae64003cbb72f015bf5c0c04216524fcb6bb8d09
On main (2e8e77cbac8a) this one is already applied, with that the error
message reduces to:
$ git send-email --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>' -1
error: unable to extract a valid address from: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> - https://public-inbox.org/git/f5c6a72b-f888-4d43-8be8-3ce2c878c669@gmail.com/T/#mca12dc95ccfd3ce2b94e7752ebaae9891201084f
This one doesn't help either. With it applied on top of main I get the
sams result as on vanilla main.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH 00/12] show-ref: introduce mode to check for ref existence
From: Patrick Steinhardt @ 2023-10-24 13:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
[-- Attachment #1: Type: text/plain, Size: 2772 bytes --]
Hi,
this patch series introduces a new `--exists` mode to git-show-ref(1) to
explicitly check for the existence of a reference, only. It tries to
address a gap in our plumbing tools: while we have a plethora of tools
to resolve revisions and thus also references, we do not have any tool
that can generically check for the existence of both direct and symoblic
references without resolving its contents.
This series has been split out of my other patch series that refactors
our test suite to reduce direct access to on-disk data structures. It is
structured as follows:
- Patches 1-8 refactor the code to stop relying on global variables,
addressing some smaller issues that surface. Furthermore, the
different modes that git-show-ref(1) has are made more explicit
such that the command becomes more extensible.
- Patch 9 ensures that the user does not request mutually exclusive
modes.
- Patch 10 updates the documentation to better reflect how the modes
are to be used.
- Patch 11 introduces the new `--exists` mode as well as a bunch of
tests for it.
- Patch 12 introduces two test helpers `test_ref_exists` and
`test_ref_missing` and updates many of our tests to use those
instead.
I admittedly may have went a bit overboard with this series. But I had a
hard time understanding git-show-ref(1) and how the global state affects
the different modes.
Patrick
[1]: <cover.1697607222.git.ps@pks.im>
Patrick Steinhardt (12):
builtin/show-ref: convert pattern to a local variable
builtin/show-ref: split up different subcommands
builtin/show-ref: fix leaking string buffer
builtin/show-ref: fix dead code when passing patterns
builtin/show-ref: refactor `--exclude-existing` options
builtin/show-ref: stop using global variable to count matches
builtin/show-ref: stop using global vars for `show_one()`
builtin/show-ref: refactor options for patterns subcommand
builtin/show-ref: ensure mutual exclusiveness of subcommands
builtin/show-ref: explicitly spell out different modes in synopsis
builtin/show-ref: add new mode to check for reference existence
t: use git-show-ref(1) to check for ref existence
Documentation/git-show-ref.txt | 16 +-
builtin/show-ref.c | 275 ++++++++++++++++++++++-----------
t/t1403-show-ref.sh | 70 +++++++++
t/t1430-bad-ref-name.sh | 27 ++--
t/t3200-branch.sh | 33 ++--
t/t5521-pull-options.sh | 4 +-
t/t5605-clone-local.sh | 2 +-
t/test-lib-functions.sh | 55 +++++++
8 files changed, 363 insertions(+), 119 deletions(-)
base-commit: a9ecda2788e229afc9b611acaa26d0d9d4da53ed
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 01/12] builtin/show-ref: convert pattern to a local variable
From: Patrick Steinhardt @ 2023-10-24 13:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698152926.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3886 bytes --]
The `pattern` variable is a global variable that tracks either the
reference names (not patterns!) for the `--verify` mode or the patterns
for the non-verify mode. This is a bit confusing due to the slightly
different meanings.
Convert the variable to be local. While this does not yet fix the double
meaning of the variable, this change allows us to address it in a
subsequent patch more easily by explicitly splitting up the different
subcommands of git-show-ref(1).
Note that we introduce a `struct show_ref_data` to pass the patterns to
`show_ref()`. While this is overengineered now, we will extend this
structure in a subsequent patch.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 46 ++++++++++++++++++++++++++++------------------
1 file changed, 28 insertions(+), 18 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 5110814f796..7efab14b96c 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -20,7 +20,6 @@ static const char * const show_ref_usage[] = {
static int deref_tags, show_head, tags_only, heads_only, found_match, verify,
quiet, hash_only, abbrev, exclude_arg;
-static const char **pattern;
static const char *exclude_existing_arg;
static void show_one(const char *refname, const struct object_id *oid)
@@ -50,15 +49,21 @@ static void show_one(const char *refname, const struct object_id *oid)
}
}
+struct show_ref_data {
+ const char **patterns;
+};
+
static int show_ref(const char *refname, const struct object_id *oid,
- int flag UNUSED, void *cbdata UNUSED)
+ int flag UNUSED, void *cbdata)
{
+ struct show_ref_data *data = cbdata;
+
if (show_head && !strcmp(refname, "HEAD"))
goto match;
- if (pattern) {
+ if (data->patterns) {
int reflen = strlen(refname);
- const char **p = pattern, *m;
+ const char **p = data->patterns, *m;
while ((m = *p++) != NULL) {
int len = strlen(m);
if (len > reflen)
@@ -180,6 +185,9 @@ static const struct option show_ref_options[] = {
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
+ struct show_ref_data show_ref_data = {0};
+ const char **patterns;
+
git_config(git_default_config, NULL);
argc = parse_options(argc, argv, prefix, show_ref_options,
@@ -188,38 +196,40 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
if (exclude_arg)
return exclude_existing(exclude_existing_arg);
- pattern = argv;
- if (!*pattern)
- pattern = NULL;
+ patterns = argv;
+ if (!*patterns)
+ patterns = NULL;
if (verify) {
- if (!pattern)
+ if (!patterns)
die("--verify requires a reference");
- while (*pattern) {
+ while (*patterns) {
struct object_id oid;
- if ((starts_with(*pattern, "refs/") || !strcmp(*pattern, "HEAD")) &&
- !read_ref(*pattern, &oid)) {
- show_one(*pattern, &oid);
+ if ((starts_with(*patterns, "refs/") || !strcmp(*patterns, "HEAD")) &&
+ !read_ref(*patterns, &oid)) {
+ show_one(*patterns, &oid);
}
else if (!quiet)
- die("'%s' - not a valid ref", *pattern);
+ die("'%s' - not a valid ref", *patterns);
else
return 1;
- pattern++;
+ patterns++;
}
return 0;
}
+ show_ref_data.patterns = patterns;
+
if (show_head)
- head_ref(show_ref, NULL);
+ head_ref(show_ref, &show_ref_data);
if (heads_only || tags_only) {
if (heads_only)
- for_each_fullref_in("refs/heads/", show_ref, NULL);
+ for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
if (tags_only)
- for_each_fullref_in("refs/tags/", show_ref, NULL);
+ for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
} else {
- for_each_ref(show_ref, NULL);
+ for_each_ref(show_ref, &show_ref_data);
}
if (!found_match) {
if (verify && !quiet)
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 02/12] builtin/show-ref: split up different subcommands
From: Patrick Steinhardt @ 2023-10-24 13:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698152926.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4249 bytes --]
While not immediately obvious, git-show-ref(1) actually implements three
different subcommands:
- `git show-ref <patterns>` can be used to list references that
match a specific pattern.
- `git show-ref --verify <refs>` can be used to list references.
These are _not_ patterns.
- `git show-ref --exclude-existing` can be used as a filter that
reads references from standard input, performing some conversions
on each of them.
Let's make this more explicit in the code by splitting up the three
subcommands into separate functions. This also allows us to address the
confusingly named `patterns` variable, which may hold either patterns or
reference names.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 100 ++++++++++++++++++++++++---------------------
1 file changed, 53 insertions(+), 47 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 7efab14b96c..56ee3250c5f 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -104,7 +104,7 @@ static int add_existing(const char *refname,
* (4) ignore if refname is a ref that exists in the local repository;
* (5) otherwise output the line.
*/
-static int exclude_existing(const char *match)
+static int cmd_show_ref__exclude_existing(const char *match)
{
static struct string_list existing_refs = STRING_LIST_INIT_DUP;
char buf[1024];
@@ -142,6 +142,53 @@ static int exclude_existing(const char *match)
return 0;
}
+static int cmd_show_ref__verify(const char **refs)
+{
+ if (!refs || !*refs)
+ die("--verify requires a reference");
+
+ while (*refs) {
+ struct object_id oid;
+
+ if ((starts_with(*refs, "refs/") || !strcmp(*refs, "HEAD")) &&
+ !read_ref(*refs, &oid)) {
+ show_one(*refs, &oid);
+ }
+ else if (!quiet)
+ die("'%s' - not a valid ref", *refs);
+ else
+ return 1;
+ refs++;
+ }
+
+ return 0;
+}
+
+static int cmd_show_ref__patterns(const char **patterns)
+{
+ struct show_ref_data show_ref_data = {
+ .patterns = (patterns && *patterns) ? patterns : NULL,
+ };
+
+ if (show_head)
+ head_ref(show_ref, &show_ref_data);
+ if (heads_only || tags_only) {
+ if (heads_only)
+ for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
+ if (tags_only)
+ for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
+ } else {
+ for_each_ref(show_ref, &show_ref_data);
+ }
+ if (!found_match) {
+ if (verify && !quiet)
+ die("No match");
+ return 1;
+ }
+
+ return 0;
+}
+
static int hash_callback(const struct option *opt, const char *arg, int unset)
{
hash_only = 1;
@@ -185,56 +232,15 @@ static const struct option show_ref_options[] = {
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
- struct show_ref_data show_ref_data = {0};
- const char **patterns;
-
git_config(git_default_config, NULL);
argc = parse_options(argc, argv, prefix, show_ref_options,
show_ref_usage, 0);
if (exclude_arg)
- return exclude_existing(exclude_existing_arg);
-
- patterns = argv;
- if (!*patterns)
- patterns = NULL;
-
- if (verify) {
- if (!patterns)
- die("--verify requires a reference");
- while (*patterns) {
- struct object_id oid;
-
- if ((starts_with(*patterns, "refs/") || !strcmp(*patterns, "HEAD")) &&
- !read_ref(*patterns, &oid)) {
- show_one(*patterns, &oid);
- }
- else if (!quiet)
- die("'%s' - not a valid ref", *patterns);
- else
- return 1;
- patterns++;
- }
- return 0;
- }
-
- show_ref_data.patterns = patterns;
-
- if (show_head)
- head_ref(show_ref, &show_ref_data);
- if (heads_only || tags_only) {
- if (heads_only)
- for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
- if (tags_only)
- for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
- } else {
- for_each_ref(show_ref, &show_ref_data);
- }
- if (!found_match) {
- if (verify && !quiet)
- die("No match");
- return 1;
- }
- return 0;
+ return cmd_show_ref__exclude_existing(exclude_existing_arg);
+ else if (verify)
+ return cmd_show_ref__verify(argv);
+ else
+ return cmd_show_ref__patterns(argv);
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 03/12] builtin/show-ref: fix leaking string buffer
From: Patrick Steinhardt @ 2023-10-24 13:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698152926.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]
Fix a leaking string buffer in `git show-ref --exclude-existing`. While
the buffer is technically not leaking because its variable is declared
as static, there is no inherent reason why it should be.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 56ee3250c5f..761669d28de 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -106,7 +106,7 @@ static int add_existing(const char *refname,
*/
static int cmd_show_ref__exclude_existing(const char *match)
{
- static struct string_list existing_refs = STRING_LIST_INIT_DUP;
+ struct string_list existing_refs = STRING_LIST_INIT_DUP;
char buf[1024];
int matchlen = match ? strlen(match) : 0;
@@ -139,6 +139,8 @@ static int cmd_show_ref__exclude_existing(const char *match)
printf("%s\n", buf);
}
}
+
+ string_list_clear(&existing_refs, 0);
return 0;
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 04/12] builtin/show-ref: fix dead code when passing patterns
From: Patrick Steinhardt @ 2023-10-24 13:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698152926.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 914 bytes --]
When passing patterns to `git show-ref` we have some code that will
cause us to die of `verify && !quiet` is true. But because `verify`
indicates a different subcommand of git-show-ref(1) that causes us to
execute `cmd_show_ref__verify()` and not `cmd_show_ref__patterns()`, the
condition cannot ever be true.
Let's remove this dead code.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 761669d28de..eb60f940a3c 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -182,11 +182,8 @@ static int cmd_show_ref__patterns(const char **patterns)
} else {
for_each_ref(show_ref, &show_ref_data);
}
- if (!found_match) {
- if (verify && !quiet)
- die("No match");
+ if (!found_match)
return 1;
- }
return 0;
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 05/12] builtin/show-ref: refactor `--exclude-existing` options
From: Patrick Steinhardt @ 2023-10-24 13:10 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698152926.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5784 bytes --]
It's not immediately obvious options which options are applicable to
what subcommand ni git-show-ref(1) because all options exist as global
state. This can easily cause confusion for the reader.
Refactor options for the `--exclude-existing` subcommand to be contained
in a separate structure. This structure is stored on the stack and
passed down as required. Consequentially, it clearly delimits the scope
of those options and requires the reader to worry less about global
state.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 72 +++++++++++++++++++++++++---------------------
1 file changed, 39 insertions(+), 33 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index eb60f940a3c..e130b999c0b 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -19,8 +19,7 @@ static const char * const show_ref_usage[] = {
};
static int deref_tags, show_head, tags_only, heads_only, found_match, verify,
- quiet, hash_only, abbrev, exclude_arg;
-static const char *exclude_existing_arg;
+ quiet, hash_only, abbrev;
static void show_one(const char *refname, const struct object_id *oid)
{
@@ -95,6 +94,11 @@ static int add_existing(const char *refname,
return 0;
}
+struct exclude_existing_options {
+ int enabled;
+ const char *pattern;
+};
+
/*
* read "^(?:<anything>\s)?<refname>(?:\^\{\})?$" from the standard input,
* and
@@ -104,11 +108,11 @@ static int add_existing(const char *refname,
* (4) ignore if refname is a ref that exists in the local repository;
* (5) otherwise output the line.
*/
-static int cmd_show_ref__exclude_existing(const char *match)
+static int cmd_show_ref__exclude_existing(const struct exclude_existing_options *opts)
{
struct string_list existing_refs = STRING_LIST_INIT_DUP;
char buf[1024];
- int matchlen = match ? strlen(match) : 0;
+ int matchlen = opts->pattern ? strlen(opts->pattern) : 0;
for_each_ref(add_existing, &existing_refs);
while (fgets(buf, sizeof(buf), stdin)) {
@@ -124,11 +128,11 @@ static int cmd_show_ref__exclude_existing(const char *match)
for (ref = buf + len; buf < ref; ref--)
if (isspace(ref[-1]))
break;
- if (match) {
+ if (opts->pattern) {
int reflen = buf + len - ref;
if (reflen < matchlen)
continue;
- if (strncmp(ref, match, matchlen))
+ if (strncmp(ref, opts->pattern, matchlen))
continue;
}
if (check_refname_format(ref, 0)) {
@@ -200,44 +204,46 @@ static int hash_callback(const struct option *opt, const char *arg, int unset)
static int exclude_existing_callback(const struct option *opt, const char *arg,
int unset)
{
+ struct exclude_existing_options *opts = opt->value;
BUG_ON_OPT_NEG(unset);
- exclude_arg = 1;
- *(const char **)opt->value = arg;
+ opts->enabled = 1;
+ opts->pattern = arg;
return 0;
}
-static const struct option show_ref_options[] = {
- OPT_BOOL(0, "tags", &tags_only, N_("only show tags (can be combined with heads)")),
- OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")),
- OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
- "requires exact ref path")),
- OPT_HIDDEN_BOOL('h', NULL, &show_head,
- N_("show the HEAD reference, even if it would be filtered out")),
- OPT_BOOL(0, "head", &show_head,
- N_("show the HEAD reference, even if it would be filtered out")),
- OPT_BOOL('d', "dereference", &deref_tags,
- N_("dereference tags into object IDs")),
- OPT_CALLBACK_F('s', "hash", &abbrev, N_("n"),
- N_("only show SHA1 hash using <n> digits"),
- PARSE_OPT_OPTARG, &hash_callback),
- OPT__ABBREV(&abbrev),
- OPT__QUIET(&quiet,
- N_("do not print results to stdout (useful with --verify)")),
- OPT_CALLBACK_F(0, "exclude-existing", &exclude_existing_arg,
- N_("pattern"), N_("show refs from stdin that aren't in local repository"),
- PARSE_OPT_OPTARG | PARSE_OPT_NONEG, exclude_existing_callback),
- OPT_END()
-};
-
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
+ struct exclude_existing_options exclude_existing_opts = {0};
+ const struct option show_ref_options[] = {
+ OPT_BOOL(0, "tags", &tags_only, N_("only show tags (can be combined with heads)")),
+ OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")),
+ OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
+ "requires exact ref path")),
+ OPT_HIDDEN_BOOL('h', NULL, &show_head,
+ N_("show the HEAD reference, even if it would be filtered out")),
+ OPT_BOOL(0, "head", &show_head,
+ N_("show the HEAD reference, even if it would be filtered out")),
+ OPT_BOOL('d', "dereference", &deref_tags,
+ N_("dereference tags into object IDs")),
+ OPT_CALLBACK_F('s', "hash", &abbrev, N_("n"),
+ N_("only show SHA1 hash using <n> digits"),
+ PARSE_OPT_OPTARG, &hash_callback),
+ OPT__ABBREV(&abbrev),
+ OPT__QUIET(&quiet,
+ N_("do not print results to stdout (useful with --verify)")),
+ OPT_CALLBACK_F(0, "exclude-existing", &exclude_existing_opts,
+ N_("pattern"), N_("show refs from stdin that aren't in local repository"),
+ PARSE_OPT_OPTARG | PARSE_OPT_NONEG, exclude_existing_callback),
+ OPT_END()
+ };
+
git_config(git_default_config, NULL);
argc = parse_options(argc, argv, prefix, show_ref_options,
show_ref_usage, 0);
- if (exclude_arg)
- return cmd_show_ref__exclude_existing(exclude_existing_arg);
+ if (exclude_existing_opts.enabled)
+ return cmd_show_ref__exclude_existing(&exclude_existing_opts);
else if (verify)
return cmd_show_ref__verify(argv);
else
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 06/12] builtin/show-ref: stop using global variable to count matches
From: Patrick Steinhardt @ 2023-10-24 13:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698152926.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1729 bytes --]
When passing patterns to git-show-ref(1) we're checking whether any
reference matches -- if none does, we indicate this condition via an
unsuccessful exit code.
We're using a global variable to count these matches, which is required
because the counter is getting incremented in a callback function. But
now that we have the `struct show_ref_data` in place, we can get rid of
the global variable and put the counter in there instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index e130b999c0b..4c039007dd1 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -18,7 +18,7 @@ static const char * const show_ref_usage[] = {
NULL
};
-static int deref_tags, show_head, tags_only, heads_only, found_match, verify,
+static int deref_tags, show_head, tags_only, heads_only, verify,
quiet, hash_only, abbrev;
static void show_one(const char *refname, const struct object_id *oid)
@@ -50,6 +50,7 @@ static void show_one(const char *refname, const struct object_id *oid)
struct show_ref_data {
const char **patterns;
+ int found_match;
};
static int show_ref(const char *refname, const struct object_id *oid,
@@ -78,7 +79,7 @@ static int show_ref(const char *refname, const struct object_id *oid,
}
match:
- found_match++;
+ data->found_match++;
show_one(refname, oid);
@@ -186,7 +187,7 @@ static int cmd_show_ref__patterns(const char **patterns)
} else {
for_each_ref(show_ref, &show_ref_data);
}
- if (!found_match)
+ if (!show_ref_data.found_match)
return 1;
return 0;
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 07/12] builtin/show-ref: stop using global vars for `show_one()`
From: Patrick Steinhardt @ 2023-10-24 13:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698152926.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6093 bytes --]
The `show_one()` function implicitly receives a bunch of options which
are tracked via global variables. This makes it hard to see which
subcommands of git-show-ref(1) actually make use of these options.
Introduce a `show_one_options` structure that gets passed down to this
function. This allows us to get rid of more global state and makes it
more explicit which subcommands use those options.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 59 +++++++++++++++++++++++++++++-----------------
1 file changed, 38 insertions(+), 21 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 4c039007dd1..589a91f15b9 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -18,10 +18,17 @@ static const char * const show_ref_usage[] = {
NULL
};
-static int deref_tags, show_head, tags_only, heads_only, verify,
- quiet, hash_only, abbrev;
+static int show_head, tags_only, heads_only, verify;
-static void show_one(const char *refname, const struct object_id *oid)
+struct show_one_options {
+ int quiet;
+ int hash_only;
+ int abbrev;
+ int deref_tags;
+};
+
+static void show_one(const struct show_one_options *opts,
+ const char *refname, const struct object_id *oid)
{
const char *hex;
struct object_id peeled;
@@ -30,25 +37,26 @@ static void show_one(const char *refname, const struct object_id *oid)
die("git show-ref: bad ref %s (%s)", refname,
oid_to_hex(oid));
- if (quiet)
+ if (opts->quiet)
return;
- hex = repo_find_unique_abbrev(the_repository, oid, abbrev);
- if (hash_only)
+ hex = repo_find_unique_abbrev(the_repository, oid, opts->abbrev);
+ if (opts->hash_only)
printf("%s\n", hex);
else
printf("%s %s\n", hex, refname);
- if (!deref_tags)
+ if (!opts->deref_tags)
return;
if (!peel_iterated_oid(oid, &peeled)) {
- hex = repo_find_unique_abbrev(the_repository, &peeled, abbrev);
+ hex = repo_find_unique_abbrev(the_repository, &peeled, opts->abbrev);
printf("%s %s^{}\n", hex, refname);
}
}
struct show_ref_data {
+ const struct show_one_options *show_one_opts;
const char **patterns;
int found_match;
};
@@ -81,7 +89,7 @@ static int show_ref(const char *refname, const struct object_id *oid,
match:
data->found_match++;
- show_one(refname, oid);
+ show_one(data->show_one_opts, refname, oid);
return 0;
}
@@ -149,7 +157,8 @@ static int cmd_show_ref__exclude_existing(const struct exclude_existing_options
return 0;
}
-static int cmd_show_ref__verify(const char **refs)
+static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,
+ const char **refs)
{
if (!refs || !*refs)
die("--verify requires a reference");
@@ -159,9 +168,9 @@ static int cmd_show_ref__verify(const char **refs)
if ((starts_with(*refs, "refs/") || !strcmp(*refs, "HEAD")) &&
!read_ref(*refs, &oid)) {
- show_one(*refs, &oid);
+ show_one(show_one_opts, *refs, &oid);
}
- else if (!quiet)
+ else if (!show_one_opts->quiet)
die("'%s' - not a valid ref", *refs);
else
return 1;
@@ -171,9 +180,11 @@ static int cmd_show_ref__verify(const char **refs)
return 0;
}
-static int cmd_show_ref__patterns(const char **patterns)
+static int cmd_show_ref__patterns(const struct show_one_options *show_one_opts,
+ const char **patterns)
{
struct show_ref_data show_ref_data = {
+ .show_one_opts = show_one_opts,
.patterns = (patterns && *patterns) ? patterns : NULL,
};
@@ -195,11 +206,16 @@ static int cmd_show_ref__patterns(const char **patterns)
static int hash_callback(const struct option *opt, const char *arg, int unset)
{
- hash_only = 1;
+ struct show_one_options *opts = opt->value;
+ struct option abbrev_opt = *opt;
+
+ opts->hash_only = 1;
/* Use full length SHA1 if no argument */
if (!arg)
return 0;
- return parse_opt_abbrev_cb(opt, arg, unset);
+
+ abbrev_opt.value = &opts->abbrev;
+ return parse_opt_abbrev_cb(&abbrev_opt, arg, unset);
}
static int exclude_existing_callback(const struct option *opt, const char *arg,
@@ -215,6 +231,7 @@ static int exclude_existing_callback(const struct option *opt, const char *arg,
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
struct exclude_existing_options exclude_existing_opts = {0};
+ struct show_one_options show_one_opts = {0};
const struct option show_ref_options[] = {
OPT_BOOL(0, "tags", &tags_only, N_("only show tags (can be combined with heads)")),
OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")),
@@ -224,13 +241,13 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
N_("show the HEAD reference, even if it would be filtered out")),
OPT_BOOL(0, "head", &show_head,
N_("show the HEAD reference, even if it would be filtered out")),
- OPT_BOOL('d', "dereference", &deref_tags,
+ OPT_BOOL('d', "dereference", &show_one_opts.deref_tags,
N_("dereference tags into object IDs")),
- OPT_CALLBACK_F('s', "hash", &abbrev, N_("n"),
+ OPT_CALLBACK_F('s', "hash", &show_one_opts, N_("n"),
N_("only show SHA1 hash using <n> digits"),
PARSE_OPT_OPTARG, &hash_callback),
- OPT__ABBREV(&abbrev),
- OPT__QUIET(&quiet,
+ OPT__ABBREV(&show_one_opts.abbrev),
+ OPT__QUIET(&show_one_opts.quiet,
N_("do not print results to stdout (useful with --verify)")),
OPT_CALLBACK_F(0, "exclude-existing", &exclude_existing_opts,
N_("pattern"), N_("show refs from stdin that aren't in local repository"),
@@ -246,7 +263,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
if (exclude_existing_opts.enabled)
return cmd_show_ref__exclude_existing(&exclude_existing_opts);
else if (verify)
- return cmd_show_ref__verify(argv);
+ return cmd_show_ref__verify(&show_one_opts, argv);
else
- return cmd_show_ref__patterns(argv);
+ return cmd_show_ref__patterns(&show_one_opts, argv);
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 08/12] builtin/show-ref: refactor options for patterns subcommand
From: Patrick Steinhardt @ 2023-10-24 13:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698152926.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3943 bytes --]
The patterns subcommand is the last command that still uses global
variables to track its options. Convert it to use a structure instead
with the same motivation as preceding commits.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 35 ++++++++++++++++++++++-------------
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 589a91f15b9..5d5d7d22ed1 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -18,8 +18,6 @@ static const char * const show_ref_usage[] = {
NULL
};
-static int show_head, tags_only, heads_only, verify;
-
struct show_one_options {
int quiet;
int hash_only;
@@ -59,6 +57,7 @@ struct show_ref_data {
const struct show_one_options *show_one_opts;
const char **patterns;
int found_match;
+ int show_head;
};
static int show_ref(const char *refname, const struct object_id *oid,
@@ -66,7 +65,7 @@ static int show_ref(const char *refname, const struct object_id *oid,
{
struct show_ref_data *data = cbdata;
- if (show_head && !strcmp(refname, "HEAD"))
+ if (data->show_head && !strcmp(refname, "HEAD"))
goto match;
if (data->patterns) {
@@ -180,20 +179,28 @@ static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,
return 0;
}
-static int cmd_show_ref__patterns(const struct show_one_options *show_one_opts,
+struct patterns_options {
+ int show_head;
+ int heads_only;
+ int tags_only;
+};
+
+static int cmd_show_ref__patterns(const struct patterns_options *opts,
+ const struct show_one_options *show_one_opts,
const char **patterns)
{
struct show_ref_data show_ref_data = {
.show_one_opts = show_one_opts,
+ .show_head = opts->show_head,
.patterns = (patterns && *patterns) ? patterns : NULL,
};
- if (show_head)
+ if (opts->show_head)
head_ref(show_ref, &show_ref_data);
- if (heads_only || tags_only) {
- if (heads_only)
+ if (opts->heads_only || opts->tags_only) {
+ if (opts->heads_only)
for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
- if (tags_only)
+ if (opts->tags_only)
for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
} else {
for_each_ref(show_ref, &show_ref_data);
@@ -231,15 +238,17 @@ static int exclude_existing_callback(const struct option *opt, const char *arg,
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
struct exclude_existing_options exclude_existing_opts = {0};
+ struct patterns_options patterns_opts = {0};
struct show_one_options show_one_opts = {0};
+ int verify = 0;
const struct option show_ref_options[] = {
- OPT_BOOL(0, "tags", &tags_only, N_("only show tags (can be combined with heads)")),
- OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")),
+ OPT_BOOL(0, "tags", &patterns_opts.tags_only, N_("only show tags (can be combined with heads)")),
+ OPT_BOOL(0, "heads", &patterns_opts.heads_only, N_("only show heads (can be combined with tags)")),
OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
"requires exact ref path")),
- OPT_HIDDEN_BOOL('h', NULL, &show_head,
+ OPT_HIDDEN_BOOL('h', NULL, &patterns_opts.show_head,
N_("show the HEAD reference, even if it would be filtered out")),
- OPT_BOOL(0, "head", &show_head,
+ OPT_BOOL(0, "head", &patterns_opts.show_head,
N_("show the HEAD reference, even if it would be filtered out")),
OPT_BOOL('d', "dereference", &show_one_opts.deref_tags,
N_("dereference tags into object IDs")),
@@ -265,5 +274,5 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
else if (verify)
return cmd_show_ref__verify(&show_one_opts, argv);
else
- return cmd_show_ref__patterns(&show_one_opts, argv);
+ return cmd_show_ref__patterns(&patterns_opts, &show_one_opts, argv);
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 09/12] builtin/show-ref: ensure mutual exclusiveness of subcommands
From: Patrick Steinhardt @ 2023-10-24 13:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698152926.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1777 bytes --]
The git-show-ref(1) command has three different modes, of which one is
implicit and the other two can be chosen explicitly by passing a flag.
But while these modes are standalone and cause us to execute completely
separate code paths, we gladly accept the case where a user asks for
both `--exclude-existing` and `--verify` at the same time even though it
is not obvious what will happen. Spoiler: we ignore `--verify` and
execute the `--exclude-existing` mode.
Let's explicitly detect this invalid usage and die in case both modes
were requested.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 3 +++
t/t1403-show-ref.sh | 5 +++++
2 files changed, 8 insertions(+)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 5d5d7d22ed1..10d0213e687 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -269,6 +269,9 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, show_ref_options,
show_ref_usage, 0);
+ if ((!!exclude_existing_opts.enabled + !!verify) > 1)
+ die(_("only one of --exclude-existing or --verify can be given"));
+
if (exclude_existing_opts.enabled)
return cmd_show_ref__exclude_existing(&exclude_existing_opts);
else if (verify)
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 9252a581abf..3a312c8b27c 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -196,4 +196,9 @@ test_expect_success 'show-ref --verify with dangling ref' '
)
'
+test_expect_success 'show-ref sub-modes are mutually exclusive' '
+ test_must_fail git show-ref --verify --exclude-existing 2>err &&
+ grep "only one of --exclude-existing or --verify can be given" err
+'
+
test_done
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 10/12] builtin/show-ref: explicitly spell out different modes in synopsis
From: Patrick Steinhardt @ 2023-10-24 13:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698152926.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2019 bytes --]
The synopsis treats the `--verify` and the implicit mode the same. They
are slightly different though:
- They accept different sets of flags.
- The implicit mode accepts patterns while the `--verify` mode
accepts references.
Split up the synopsis for these two modes such that we can disambiguate
those differences.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-show-ref.txt | 5 ++++-
builtin/show-ref.c | 5 ++++-
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
index 2fe274b8faa..ab23e0b62e1 100644
--- a/Documentation/git-show-ref.txt
+++ b/Documentation/git-show-ref.txt
@@ -8,9 +8,12 @@ git-show-ref - List references in a local repository
SYNOPSIS
--------
[verse]
-'git show-ref' [-q | --quiet] [--verify] [--head] [-d | --dereference]
+'git show-ref' [-q | --quiet] [--head] [-d | --dereference]
[-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]
[--heads] [--] [<pattern>...]
+'git show-ref' --verify [-q | --quiet] [-d | --dereference]
+ [-s | --hash[=<n>]] [--abbrev[=<n>]]
+ [--] [<ref>...]
'git show-ref' --exclude-existing[=<pattern>]
DESCRIPTION
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 10d0213e687..d0a32d07404 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -11,9 +11,12 @@
#include "parse-options.h"
static const char * const show_ref_usage[] = {
- N_("git show-ref [-q | --quiet] [--verify] [--head] [-d | --dereference]\n"
+ N_("git show-ref [-q | --quiet] [--head] [-d | --dereference]\n"
" [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]\n"
" [--heads] [--] [<pattern>...]"),
+ N_("git show-ref --verify [-q | --quiet] [-d | --dereference]\n"
+ " [-s | --hash[=<n>]] [--abbrev[=<n>]]\n"
+ " [--] [<ref>...]"),
N_("git show-ref --exclude-existing[=<pattern>]"),
NULL
};
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 11/12] builtin/show-ref: add new mode to check for reference existence
From: Patrick Steinhardt @ 2023-10-24 13:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698152926.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 9653 bytes --]
While we have multiple ways to show the value of a given reference, we
do not have any way to check whether a reference exists at all. While
commands like git-rev-parse(1) or git-show-ref(1) can be used to check
for reference existence in case the reference resolves to something
sane, neither of them can be used to check for existence in some other
scenarios where the reference does not resolve cleanly:
- References which have an invalid name cannot be resolved.
- References to nonexistent objects cannot be resolved.
- Dangling symrefs can be resolved via git-symbolic-ref(1), but this
requires the caller to special case existence checks depending on
whteher or not a reference is symbolic or direct.
Furthermore, git-rev-list(1) and other commands do not let the caller
distinguish easily between an actually missing reference and a generic
error.
Taken together, this gseems like sufficient motivation to introduce a
separate plumbing command to explicitly check for the existence of a
reference without trying to resolve its contents.
This new command comes in the form of `git show-ref --exists`. This
new mode will exit successfully when the reference exists, with a
specific error code of 2 when it does not exist, or with 1 when there
has been a generic error.
Note that the only way to properly implement this command is by using
the internal `refs_read_raw_ref()` function. While the public function
`refs_resolve_ref_unsafe()` can be made to behave in the same way by
passing various flags, it does not provide any way to obtain the errno
with which the reference backend failed when reading the reference. As
such, it becomes impossible for us to distinguish generic errors from
the explicit case where the reference wasn't found.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-show-ref.txt | 11 ++++++
builtin/show-ref.c | 47 ++++++++++++++++++++++--
t/t1403-show-ref.sh | 67 +++++++++++++++++++++++++++++++++-
3 files changed, 120 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
index ab23e0b62e1..a7e9374bc2b 100644
--- a/Documentation/git-show-ref.txt
+++ b/Documentation/git-show-ref.txt
@@ -15,6 +15,7 @@ SYNOPSIS
[-s | --hash[=<n>]] [--abbrev[=<n>]]
[--] [<ref>...]
'git show-ref' --exclude-existing[=<pattern>]
+'git show-ref' --exists <ref>
DESCRIPTION
-----------
@@ -30,6 +31,10 @@ The `--exclude-existing` form is a filter that does the inverse. It reads
refs from stdin, one ref per line, and shows those that don't exist in
the local repository.
+The `--exists` form can be used to check for the existence of a single
+references. This form does not verify whether the reference resolves to an
+actual object.
+
Use of this utility is encouraged in favor of directly accessing files under
the `.git` directory.
@@ -65,6 +70,12 @@ OPTIONS
Aside from returning an error code of 1, it will also print an error
message if `--quiet` was not specified.
+--exists::
+
+ Check whether the given reference exists. Returns an error code of 0 if
+ it does, 2 if it is missing, and 128 in case looking up the reference
+ failed with an error other than the reference being missing.
+
--abbrev[=<n>]::
Abbreviate the object name. When using `--hash`, you do
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index d0a32d07404..617e754bbed 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -2,7 +2,7 @@
#include "config.h"
#include "gettext.h"
#include "hex.h"
-#include "refs.h"
+#include "refs/refs-internal.h"
#include "object-name.h"
#include "object-store-ll.h"
#include "object.h"
@@ -18,6 +18,7 @@ static const char * const show_ref_usage[] = {
" [-s | --hash[=<n>]] [--abbrev[=<n>]]\n"
" [--] [<ref>...]"),
N_("git show-ref --exclude-existing[=<pattern>]"),
+ N_("git show-ref --exists <ref>"),
NULL
};
@@ -214,6 +215,41 @@ static int cmd_show_ref__patterns(const struct patterns_options *opts,
return 0;
}
+static int cmd_show_ref__exists(const char **refs)
+{
+ struct strbuf unused_referent = STRBUF_INIT;
+ struct object_id unused_oid;
+ unsigned int unused_type;
+ int failure_errno = 0;
+ const char *ref;
+ int ret = 1;
+
+ if (!refs || !*refs)
+ die("--exists requires a reference");
+ ref = *refs++;
+ if (*refs)
+ die("--exists requires exactly one reference");
+
+ if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
+ &unused_oid, &unused_referent, &unused_type,
+ &failure_errno)) {
+ if (failure_errno == ENOENT) {
+ error(_("reference does not exist"));
+ ret = 2;
+ } else {
+ error(_("failed to look up reference: %s"), strerror(failure_errno));
+ }
+
+ goto out;
+ }
+
+ ret = 0;
+
+out:
+ strbuf_release(&unused_referent);
+ return ret;
+}
+
static int hash_callback(const struct option *opt, const char *arg, int unset)
{
struct show_one_options *opts = opt->value;
@@ -243,10 +279,11 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
struct exclude_existing_options exclude_existing_opts = {0};
struct patterns_options patterns_opts = {0};
struct show_one_options show_one_opts = {0};
- int verify = 0;
+ int verify = 0, exists = 0;
const struct option show_ref_options[] = {
OPT_BOOL(0, "tags", &patterns_opts.tags_only, N_("only show tags (can be combined with heads)")),
OPT_BOOL(0, "heads", &patterns_opts.heads_only, N_("only show heads (can be combined with tags)")),
+ OPT_BOOL(0, "exists", &exists, N_("check for reference existence without resolving")),
OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
"requires exact ref path")),
OPT_HIDDEN_BOOL('h', NULL, &patterns_opts.show_head,
@@ -272,13 +309,15 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, show_ref_options,
show_ref_usage, 0);
- if ((!!exclude_existing_opts.enabled + !!verify) > 1)
- die(_("only one of --exclude-existing or --verify can be given"));
+ if ((!!exclude_existing_opts.enabled + !!verify + !!exists) > 1)
+ die(_("only one of --exclude-existing, --exists or --verify can be given"));
if (exclude_existing_opts.enabled)
return cmd_show_ref__exclude_existing(&exclude_existing_opts);
else if (verify)
return cmd_show_ref__verify(&show_one_opts, argv);
+ else if (exists)
+ return cmd_show_ref__exists(argv);
else
return cmd_show_ref__patterns(&patterns_opts, &show_one_opts, argv);
}
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 3a312c8b27c..17eba350ce5 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -197,8 +197,73 @@ test_expect_success 'show-ref --verify with dangling ref' '
'
test_expect_success 'show-ref sub-modes are mutually exclusive' '
+ cat >expect <<-EOF &&
+ fatal: only one of --exclude-existing, --exists or --verify can be given
+ EOF
+
test_must_fail git show-ref --verify --exclude-existing 2>err &&
- grep "only one of --exclude-existing or --verify can be given" err
+ test_cmp expect err &&
+
+ test_must_fail git show-ref --verify --exists 2>err &&
+ test_cmp expect err &&
+
+ test_must_fail git show-ref --exclude-existing --exists 2>err &&
+ test_cmp expect err
+'
+
+test_expect_success '--exists with existing reference' '
+ git show-ref --exists refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+'
+
+test_expect_success '--exists with missing reference' '
+ test_expect_code 2 git show-ref --exists refs/heads/does-not-exist
+'
+
+test_expect_success '--exists does not use DWIM' '
+ test_expect_code 2 git show-ref --exists $GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME 2>err &&
+ grep "reference does not exist" err
+'
+
+test_expect_success '--exists with HEAD' '
+ git show-ref --exists HEAD
+'
+
+test_expect_success '--exists with bad reference name' '
+ test_when_finished "git update-ref -d refs/heads/bad...name" &&
+ new_oid=$(git rev-parse HEAD) &&
+ test-tool ref-store main update-ref msg refs/heads/bad...name $new_oid $ZERO_OID REF_SKIP_REFNAME_VERIFICATION &&
+ git show-ref --exists refs/heads/bad...name
+'
+
+test_expect_success '--exists with arbitrary symref' '
+ test_when_finished "git symbolic-ref -d refs/symref" &&
+ git symbolic-ref refs/symref refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME &&
+ git show-ref --exists refs/symref
+'
+
+test_expect_success '--exists with dangling symref' '
+ test_when_finished "git symbolic-ref -d refs/heads/dangling" &&
+ git symbolic-ref refs/heads/dangling refs/heads/does-not-exist &&
+ git show-ref --exists refs/heads/dangling
+'
+
+test_expect_success '--exists with nonexistent object ID' '
+ test-tool ref-store main update-ref msg refs/heads/missing-oid $(test_oid 001) $ZERO_OID REF_SKIP_OID_VERIFICATION &&
+ git show-ref --exists refs/heads/missing-oid
+'
+
+test_expect_success '--exists with non-commit object' '
+ tree_oid=$(git rev-parse HEAD^{tree}) &&
+ test-tool ref-store main update-ref msg refs/heads/tree ${tree_oid} $ZERO_OID REF_SKIP_OID_VERIFICATION &&
+ git show-ref --exists refs/heads/tree
+'
+
+test_expect_success '--exists with directory fails with generic error' '
+ cat >expect <<-EOF &&
+ error: failed to look up reference: Is a directory
+ EOF
+ test_expect_code 1 git show-ref --exists refs/heads 2>err &&
+ test_cmp expect err
'
test_done
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 12/12] t: use git-show-ref(1) to check for ref existence
From: Patrick Steinhardt @ 2023-10-24 13:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698152926.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 14354 bytes --]
Convert tests that use `test_path_is_file` and `test_path_is_missing` to
instead use a set of helpers `test_ref_exists` and `test_ref_missing`.
These helpers are implemented via the newly introduced `git show-ref
--exists` command. Thus, we can avoid intimate knowledge of how the ref
backend stores references on disk.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t1430-bad-ref-name.sh | 27 +++++++++++++-------
t/t3200-branch.sh | 33 ++++++++++++++-----------
t/t5521-pull-options.sh | 4 +--
t/t5605-clone-local.sh | 2 +-
t/test-lib-functions.sh | 55 +++++++++++++++++++++++++++++++++++++++++
5 files changed, 94 insertions(+), 27 deletions(-)
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index ff1c967d550..7b7d6953c62 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -205,8 +205,9 @@ test_expect_success 'update-ref --no-deref -d can delete symref to broken name'
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref" &&
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/badname &&
git update-ref --no-deref -d refs/heads/badname >output 2>error &&
- test_path_is_missing .git/refs/heads/badname &&
+ test_ref_missing refs/heads/badname &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -216,8 +217,9 @@ test_expect_success 'branch -d can delete symref to broken name' '
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref" &&
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/badname &&
git branch -d badname >output 2>error &&
- test_path_is_missing .git/refs/heads/badname &&
+ test_ref_missing refs/heads/badname &&
test_i18ngrep "Deleted branch badname (was refs/heads/broken\.\.\.ref)" output &&
test_must_be_empty error
'
@@ -225,8 +227,9 @@ test_expect_success 'branch -d can delete symref to broken name' '
test_expect_success 'update-ref --no-deref -d can delete dangling symref to broken name' '
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/badname &&
git update-ref --no-deref -d refs/heads/badname >output 2>error &&
- test_path_is_missing .git/refs/heads/badname &&
+ test_ref_missing refs/heads/badname &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -234,8 +237,9 @@ test_expect_success 'update-ref --no-deref -d can delete dangling symref to brok
test_expect_success 'branch -d can delete dangling symref to broken name' '
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/badname &&
git branch -d badname >output 2>error &&
- test_path_is_missing .git/refs/heads/badname &&
+ test_ref_missing refs/heads/badname &&
test_i18ngrep "Deleted branch badname (was refs/heads/broken\.\.\.ref)" output &&
test_must_be_empty error
'
@@ -245,8 +249,9 @@ test_expect_success 'update-ref -d can delete broken name through symref' '
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...ref" &&
test-tool ref-store main create-symref refs/heads/badname refs/heads/broken...ref msg &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/badname" &&
+ test_ref_exists refs/heads/broken...ref &&
git update-ref -d refs/heads/badname >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...ref &&
+ test_ref_missing refs/heads/broken...ref &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -254,8 +259,9 @@ test_expect_success 'update-ref -d can delete broken name through symref' '
test_expect_success 'update-ref --no-deref -d can delete symref with broken name' '
printf "ref: refs/heads/main\n" >.git/refs/heads/broken...symref &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
+ test_ref_exists refs/heads/broken...symref &&
git update-ref --no-deref -d refs/heads/broken...symref >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...symref &&
+ test_ref_missing refs/heads/broken...symref &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -263,8 +269,9 @@ test_expect_success 'update-ref --no-deref -d can delete symref with broken name
test_expect_success 'branch -d can delete symref with broken name' '
printf "ref: refs/heads/main\n" >.git/refs/heads/broken...symref &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
+ test_ref_exists refs/heads/broken...symref &&
git branch -d broken...symref >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...symref &&
+ test_ref_missing refs/heads/broken...symref &&
test_i18ngrep "Deleted branch broken...symref (was refs/heads/main)" output &&
test_must_be_empty error
'
@@ -272,8 +279,9 @@ test_expect_success 'branch -d can delete symref with broken name' '
test_expect_success 'update-ref --no-deref -d can delete dangling symref with broken name' '
printf "ref: refs/heads/idonotexist\n" >.git/refs/heads/broken...symref &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
+ test_ref_exists refs/heads/broken...symref &&
git update-ref --no-deref -d refs/heads/broken...symref >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...symref &&
+ test_ref_missing refs/heads/broken...symref &&
test_must_be_empty output &&
test_must_be_empty error
'
@@ -281,8 +289,9 @@ test_expect_success 'update-ref --no-deref -d can delete dangling symref with br
test_expect_success 'branch -d can delete dangling symref with broken name' '
printf "ref: refs/heads/idonotexist\n" >.git/refs/heads/broken...symref &&
test_when_finished "test-tool ref-store main delete-refs REF_NO_DEREF msg refs/heads/broken...symref" &&
+ test_ref_exists refs/heads/broken...symref &&
git branch -d broken...symref >output 2>error &&
- test_path_is_missing .git/refs/heads/broken...symref &&
+ test_ref_missing refs/heads/broken...symref &&
test_i18ngrep "Deleted branch broken...symref (was refs/heads/idonotexist)" output &&
test_must_be_empty error
'
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 080e4f24a6e..bde4f1485b7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -25,7 +25,7 @@ test_expect_success 'prepare a trivial repository' '
test_expect_success 'git branch --help should not have created a bogus branch' '
test_might_fail git branch --man --help </dev/null >/dev/null 2>&1 &&
- test_path_is_missing .git/refs/heads/--help
+ test_ref_missing refs/heads/--help
'
test_expect_success 'branch -h in broken repository' '
@@ -40,7 +40,8 @@ test_expect_success 'branch -h in broken repository' '
'
test_expect_success 'git branch abc should create a branch' '
- git branch abc && test_path_is_file .git/refs/heads/abc
+ git branch abc &&
+ test_ref_exists refs/heads/abc
'
test_expect_success 'git branch abc should fail when abc exists' '
@@ -61,11 +62,13 @@ test_expect_success 'git branch --force abc should succeed when abc exists' '
'
test_expect_success 'git branch a/b/c should create a branch' '
- git branch a/b/c && test_path_is_file .git/refs/heads/a/b/c
+ git branch a/b/c &&
+ test_ref_exists refs/heads/a/b/c
'
test_expect_success 'git branch mb main... should create a branch' '
- git branch mb main... && test_path_is_file .git/refs/heads/mb
+ git branch mb main... &&
+ test_ref_exists refs/heads/mb
'
test_expect_success 'git branch HEAD should fail' '
@@ -78,14 +81,14 @@ EOF
test_expect_success 'git branch --create-reflog d/e/f should create a branch and a log' '
GIT_COMMITTER_DATE="2005-05-26 23:30" \
git -c core.logallrefupdates=false branch --create-reflog d/e/f &&
- test_path_is_file .git/refs/heads/d/e/f &&
+ test_ref_exists refs/heads/d/e/f &&
test_path_is_file .git/logs/refs/heads/d/e/f &&
test_cmp expect .git/logs/refs/heads/d/e/f
'
test_expect_success 'git branch -d d/e/f should delete a branch and a log' '
git branch -d d/e/f &&
- test_path_is_missing .git/refs/heads/d/e/f &&
+ test_ref_missing refs/heads/d/e/f &&
test_must_fail git reflog exists refs/heads/d/e/f
'
@@ -213,7 +216,7 @@ test_expect_success 'git branch -M should leave orphaned HEAD alone' '
test_commit initial &&
git checkout --orphan lonely &&
grep lonely .git/HEAD &&
- test_path_is_missing .git/refs/head/lonely &&
+ test_ref_missing refs/head/lonely &&
git branch -M main mistress &&
grep lonely .git/HEAD
)
@@ -799,8 +802,8 @@ test_expect_success 'deleting a symref' '
git symbolic-ref refs/heads/symref refs/heads/target &&
echo "Deleted branch symref (was refs/heads/target)." >expect &&
git branch -d symref >actual &&
- test_path_is_file .git/refs/heads/target &&
- test_path_is_missing .git/refs/heads/symref &&
+ test_ref_exists refs/heads/target &&
+ test_ref_missing refs/heads/symref &&
test_cmp expect actual
'
@@ -809,16 +812,16 @@ test_expect_success 'deleting a dangling symref' '
test_path_is_file .git/refs/heads/dangling-symref &&
echo "Deleted branch dangling-symref (was nowhere)." >expect &&
git branch -d dangling-symref >actual &&
- test_path_is_missing .git/refs/heads/dangling-symref &&
+ test_ref_missing refs/heads/dangling-symref &&
test_cmp expect actual
'
test_expect_success 'deleting a self-referential symref' '
git symbolic-ref refs/heads/self-reference refs/heads/self-reference &&
- test_path_is_file .git/refs/heads/self-reference &&
+ test_ref_exists refs/heads/self-reference &&
echo "Deleted branch self-reference (was refs/heads/self-reference)." >expect &&
git branch -d self-reference >actual &&
- test_path_is_missing .git/refs/heads/self-reference &&
+ test_ref_missing refs/heads/self-reference &&
test_cmp expect actual
'
@@ -826,8 +829,8 @@ test_expect_success 'renaming a symref is not allowed' '
git symbolic-ref refs/heads/topic refs/heads/main &&
test_must_fail git branch -m topic new-topic &&
git symbolic-ref refs/heads/topic &&
- test_path_is_file .git/refs/heads/main &&
- test_path_is_missing .git/refs/heads/new-topic
+ test_ref_exists refs/heads/main &&
+ test_ref_missing refs/heads/new-topic
'
test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for u is a symlink' '
@@ -1142,7 +1145,7 @@ EOF
test_expect_success 'git checkout -b g/h/i -l should create a branch and a log' '
GIT_COMMITTER_DATE="2005-05-26 23:30" \
git checkout -b g/h/i -l main &&
- test_path_is_file .git/refs/heads/g/h/i &&
+ test_ref_exists refs/heads/g/h/i &&
test_path_is_file .git/logs/refs/heads/g/h/i &&
test_cmp expect .git/logs/refs/heads/g/h/i
'
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 079b2f2536e..3681859f983 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -143,7 +143,7 @@ test_expect_success 'git pull --dry-run' '
cd clonedry &&
git pull --dry-run ../parent &&
test_path_is_missing .git/FETCH_HEAD &&
- test_path_is_missing .git/refs/heads/main &&
+ test_ref_missing refs/heads/main &&
test_path_is_missing .git/index &&
test_path_is_missing file
)
@@ -157,7 +157,7 @@ test_expect_success 'git pull --all --dry-run' '
git remote add origin ../parent &&
git pull --all --dry-run &&
test_path_is_missing .git/FETCH_HEAD &&
- test_path_is_missing .git/refs/remotes/origin/main &&
+ test_ref_missing refs/remotes/origin/main &&
test_path_is_missing .git/index &&
test_path_is_missing file
)
diff --git a/t/t5605-clone-local.sh b/t/t5605-clone-local.sh
index 1d7b1abda1a..946c5751885 100755
--- a/t/t5605-clone-local.sh
+++ b/t/t5605-clone-local.sh
@@ -69,7 +69,7 @@ test_expect_success 'local clone of repo with nonexistent ref in HEAD' '
git clone a d &&
(cd d &&
git fetch &&
- test ! -e .git/refs/remotes/origin/HEAD)
+ test_ref_missing refs/remotes/origin/HEAD)
'
test_expect_success 'bundle clone without .bundle suffix' '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2f8868caa17..56b33536ed1 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -251,6 +251,61 @@ debug () {
done
}
+# Usage: test_ref_exists [options] <ref>
+#
+# -C <dir>:
+# Run all git commands in directory <dir>
+#
+# This helper function checks whether a reference exists. Symrefs or object IDs
+# will not be resolved. Can be used to check references with bad names.
+test_ref_exists () {
+ local indir=
+
+ while test $# != 0
+ do
+ case "$1" in
+ -C)
+ indir="$2"
+ shift
+ ;;
+ *)
+ break
+ ;;
+ esac
+ shift
+ done &&
+
+ indir=${indir:+"$indir"/} &&
+
+ if test "$#" != 1
+ then
+ BUG "expected exactly one reference"
+ fi &&
+
+ git ${indir:+ -C "$indir"} show-ref --exists "$1"
+}
+
+# Behaves the same as test_ref_exists, except that it checks for the absence of
+# a reference. This is preferable to `! test_ref_exists` as this function is
+# able to distinguish actually-missing references from other, generic errors.
+test_ref_missing () {
+ test_ref_exists "$@"
+ case "$?" in
+ 2)
+ # This is the good case.
+ return 0
+ ;;
+ 0)
+ echo >&4 "test_ref_missing: reference exists"
+ return 1
+ ;;
+ *)
+ echo >&4 "test_ref_missing: generic error"
+ return 1
+ ;;
+ esac
+}
+
# Usage: test_commit [options] <message> [<file> [<contents> [<tag>]]]
# -C <dir>:
# Run all git commands in directory <dir>
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
From: Kristoffer Haugsbakk @ 2023-10-24 13:23 UTC (permalink / raw)
To: Taylor Blau, Patrick Steinhardt; +Cc: git, stolee
In-Reply-To: <ZTav2u1JWmLexEHL@nand.local>
Hi Taylor and Patrick
On Mon, Oct 23, 2023, at 19:40, Taylor Blau wrote:
>> Nit: we don't know about the intent of the caller, so they may not want
>> to write to the file but only read it.
>
> I was going to suggest that we allow the caller to pass in the flags
> that they wish for git_global_config() to pass down to access(2), but
> was surprised to see that we always use R_OK.
>
> But thinking on it for a moment longer, I realized that we don't care
> about write-level permissions for the config, since we want to instead
> open $GIT_DIR/config.lock for writing, and then rename() it into place,
> meaning we only care about whether or not we have write permissions on
> $GIT_DIR itself.
>
> I think in the existing location of this code, the "if we should write"
> portion of the comment is premature, since we don't know for sure
> whether or not we are writing. So I'd be fine with leaving it as-is, but
> changing the comment seems easy enough to do...
>
>> > + * location; error out even if XDG_CONFIG_HOME
>> > + * is set and points at a sane location.
>> > + */
>> > + die(_("$HOME not set"));
>>
>> Is it sensible to `die()` here in this new function that behaves more
>> like a library function? I imagine it would be more sensible to indicate
>> the error to the user and let them handle it accordingly.
>
> Agreed.
>
> Thanks,
> Taylor
What do you guys think the signature of `git_global_config` should be?
^ permalink raw reply
* Re: using oldest date when squashing commits
From: Phillip Wood @ 2023-10-24 14:00 UTC (permalink / raw)
To: Oswald Buddenhagen, phillip.wood; +Cc: Johannes Sixt, git
In-Reply-To: <ZTeZ3KEQLIVU/sq2@ugly>
On 24/10/2023 11:18, Oswald Buddenhagen wrote:
> On Tue, Oct 24, 2023 at 10:26:29AM +0100, Phillip Wood wrote:
>> On 20/03/2022 08:05, Johannes Sixt wrote:
>>> Am 19.03.22 um 13:48 schrieb Oswald Buddenhagen:
>>>> during interactive rebasing, i sometimes find it necessary to move a
>>>> hunk from one commit to a later one in the branch. now, if that hunk
>>>> cannot be re-ordered with the later commit due to conflicting with it,
>>>> it becomes necessary to squash the later commit onto a temporary commit
>>>> created from the extracted hunk, not the other way around (or using a
>>>> stash). unfortunately, this causes the author date of the later commit
>>>> to be reset, which can rather seriously falsify the date if the branch
>>>> is long-lived.
>>>
>>> You want `fixup -C` in the todo-list. See the hints near the end of the
>>> todo-list.
>>
>> Unfortunately "fixup -C" only copies the commit message not the
>> authorship
>
>> (that's usually a good thing
>>
> why? what would that be useful for?
> it seems rather counter-intuitive.
In the same way that you do not want to change the author date when
using a fixup to move a small hunk from one commit to another most users
do not want to update the author information when they make a small
change to a commit message using "fixup -C"
> it's also inconsistent with commit -c/-C's behavior, which seems like a
> red flag to me.
That could mean the option is mis-named instead rather than the behavior
being wrong.
>> but not it means it wont work for what Oswald wants to do).
>
>> Maybe we should add another flag for fixup/squash commands to take the
>> authorship from that commit.
>>
> that's a possibility. but given the above, it might be better to simply
> change the behavior of -c/-C to keep the UI lean and consistent with
> commit's behavior.
"fixup -c/-C" were conceived as a way to reword a commit message at the
same time as optionally fixing up the commit's content. I think changing
the behavior to automatically update the authorship would surprise
people and as I said above most of the time one does not want that behavior.
Best Wishes
Phillip
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox