* Re: [PATCH v2 1/3] t: fix quotes tests for --pathspec-from-file
From: Eric Sunshine @ 2019-12-30 21:55 UTC (permalink / raw)
To: Alexandr Miloslavskiy via GitGitGadget
Cc: Git List, Alexandr Miloslavskiy, Junio C Hamano
In-Reply-To: <6193dc7396b9cc6cb78f382c1b1679d6bb455fe4.1577733329.git.gitgitgadget@gmail.com>
On Mon, Dec 30, 2019 at 2:15 PM Alexandr Miloslavskiy via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> While working on the next patch, I also noticed that quotes testing via
> `"\"file\\101.t\""` was somewhat incorrect: I escaped `\` one time while
> I had to escape it two times! Tests still worked due to `"` being
> preserved which in turn prevented pathspec from matching files.
>
> Fix this by properly escaping one more time.
>
> Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
> ---
> diff --git a/t/t2026-checkout-pathspec-file.sh b/t/t2026-checkout-pathspec-file.sh
> @@ -109,7 +109,10 @@ test_expect_success 'CRLF delimiters' '
> - printf "\"file\\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
> + # shell takes \\\\101 and spits \\101
> + # printf takes \\101 and spits \101
> + # git takes \101 and spits A
> + printf "\"file\\\\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
So, you want git-checkout to receive the following, quotes, backslash,
and no newline, on its standard input?
"file\101.t"
If so, another way to achieve the same without taxing the brain of the
reader or the next person who works on this code would be:
tr -d "\012" | git checkout --pathspec-from-file=- HEAD^1 <<-\EOF &&
"file\101.t"
EOF
Although it's three lines long, the body of the here-doc is the
literal text you want sent to the Git command, so no counting
backslashes, and no need for a lengthy in-code comment.
But is the "no newline" bit indeed intentional? If not, then a simple
echo would be even easier (though with a bit more escaping):
echo "\"file\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
^ permalink raw reply
* Re: [PATCH v2 00/17] merge: learn --autostash
From: Junio C Hamano @ 2019-12-30 21:49 UTC (permalink / raw)
To: Denton Liu
Cc: Git Mailing List, Alban Gruin, Johannes Schindelin, Phillip Wood
In-Reply-To: <cover.1577185374.git.liu.denton@gmail.com>
Denton Liu <liu.denton@gmail.com> writes:
> Alban reported[1] that he expected merge to have an --autostash option,
> just like rebase. Since there's not really any reason why merge can't
> have it, let's implement it in this patchset.
Sigh.
I would actually have preferred to remove the autostash from rebase
if the goal is to have a consistent behaviour between the two,
though ;-)
But it is OK as long as it does not degrade the codepath with
changes that wouldn't have been necessary if this weren't added.
Let's see how it goes. Queued.
^ permalink raw reply
* Re: [PATCH] sha1-file: remove OBJECT_INFO_SKIP_CACHED
From: Junio C Hamano @ 2019-12-30 21:43 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <20191230211027.37002-1-jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> In a partial clone, if a user provides the hash of the empty tree ("git
> mktree </dev/null" - for SHA-1, this is 4b825d...) to a command which
> requires that that object be parsed, for example:
>
> git diff-tree 4b825d <a non-empty tree>
>
> then Git will lazily fetch the empty tree.
That sounds like a bug. Shouldn't some objects like empty tree and
empty blob whose names are hardcoded come internally without being
fetched from anywhere?
> There are 2 functions: repo_has_object_file() which does not consult
> find_cached_object() (which, among other things, knows about the empty
> tree); and repo_read_object_file() which does. This issue occurs
> because, as an optimization to avoid reading blobs into memory,
> parse_object() calls repo_has_object_file() before
> repo_read_object_file(). In the case of a regular repository (that is,
> not a partial clone), repo_has_object_file() will return false for the
> empty tree (thus bypassing the optimization) and...
OK, that bypassing already sounds wrong.
> ... it will no longer be the case that
> repo_has_object_file() doesn't know about empty trees, but
> repo_read_object_file() does.
Yup, that sounds like the right solution to the bug, with or without
lazy cloning.
^ permalink raw reply
* Re: [PATCH v2 01/17] Makefile: alphabetically sort += lists
From: Junio C Hamano @ 2019-12-30 21:38 UTC (permalink / raw)
To: Denton Liu
Cc: Git Mailing List, Alban Gruin, Johannes Schindelin, Phillip Wood
In-Reply-To: <31055cdac4a5bfc48cf6f18179f4e9bce9a04c8c.1577185374.git.liu.denton@gmail.com>
Denton Liu <liu.denton@gmail.com> writes:
> Note that if we omit file prefixes, the lists aren't sorted in strictly
> alphabetical order (e.g. archive.o comes after archive-zip.o instead of
> before archive-tar.o). This is intentional ...
That's fine, but then we shouldn't label it as "alphabetically sort".
> because the purpose of
> maintaining the sorted list is to ensure line insertions are
> deterministic.
Truly alphabetic sorting is deterministic, too, so that is *not* the
reason why you are not being strictly alphabetic. The true reason
is because it is easier to apply mechanical sorting in ascii order
than truly sort alphabetically. Both are equally deterministic and
reproducible. Alphabetic might be easier for humans to see and
understand, but you value mechanical ease of sorting and that is why
you chose ascii order over alphabetical sorting.
^ permalink raw reply
* [PATCH] sha1-file: remove OBJECT_INFO_SKIP_CACHED
From: Jonathan Tan @ 2019-12-30 21:10 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
In a partial clone, if a user provides the hash of the empty tree ("git
mktree </dev/null" - for SHA-1, this is 4b825d...) to a command which
requires that that object be parsed, for example:
git diff-tree 4b825d <a non-empty tree>
then Git will lazily fetch the empty tree. This fetch would merely be
inconvenient if the promisor remote could supply that tree, but at
$DAYJOB we discovered that some repositories do not (e.g. [1]).
There are 2 functions: repo_has_object_file() which does not consult
find_cached_object() (which, among other things, knows about the empty
tree); and repo_read_object_file() which does. This issue occurs
because, as an optimization to avoid reading blobs into memory,
parse_object() calls repo_has_object_file() before
repo_read_object_file(). In the case of a regular repository (that is,
not a partial clone), repo_has_object_file() will return false for the
empty tree (thus bypassing the optimization) and repo_read_object_file()
will nevertheless succeed, thus things coincidentally work. But in a
partial clone, repo_has_object_file() triggers a lazy fetch of the
missing empty tree. This optimization was introduced by 090ea12671
("parse_object: avoid putting whole blob in core", 2012-03-07), and the
empty-tree special-case dichotomy between repo_has_object_file() (then,
has_sha1_file()) and repo_read_object_file() (then, sha1_object_info())
has existed since then.
(The flag OBJECT_INFO_SKIP_CACHED, introduced later in dfdd4afcf9
("sha1_file: teach sha1_object_info_extended more flags", 2017-06-26)
and used in e83e71c5e1 ("sha1_file: refactor has_sha1_file_with_flags",
2017-06-26), was introduced to preserve the empty-tree special-case
dichotomy.)
The best solution to the problem introduced at the start of this commit
message seems to be to eliminate this dichotomy. Not only will this fix
the problem, but it reduces a potential avenue of surprise for future
developers of Git - it will no longer be the case that
repo_has_object_file() doesn't know about empty trees, but
repo_read_object_file() does. A cost is that repo_has_object_file() will
now need to oideq upon each invocation, but that is trivial compared to
the filesystem lookup or the pack index search required anyway. (And if
find_cached_object() needs to do more because of previous invocations to
pretend_object_file(), all the more reason to be consistent in whether
we present cached objects.) Therefore, remove OBJECT_INFO_SKIP_CACHED.
[1] https://dart.googlesource.com/json_rpc_2
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
I put a lot of historical references which makes the commit message
rather long - let me know if you think that some can be omitted.
---
object-store.h | 2 --
sha1-file.c | 38 ++++++++++++++++++--------------------
2 files changed, 18 insertions(+), 22 deletions(-)
diff --git a/object-store.h b/object-store.h
index 55ee639350..61b8b13e3b 100644
--- a/object-store.h
+++ b/object-store.h
@@ -292,8 +292,6 @@ struct object_info {
#define OBJECT_INFO_LOOKUP_REPLACE 1
/* Allow reading from a loose object file of unknown/bogus type */
#define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2
-/* Do not check cached storage */
-#define OBJECT_INFO_SKIP_CACHED 4
/* Do not retry packed storage after checking packed and loose storage */
#define OBJECT_INFO_QUICK 8
/* Do not check loose object */
diff --git a/sha1-file.c b/sha1-file.c
index 188de57634..03ae9ae93a 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -1417,6 +1417,7 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,
struct object_info *oi, unsigned flags)
{
static struct object_info blank_oi = OBJECT_INFO_INIT;
+ struct cached_object *co;
struct pack_entry e;
int rtype;
const struct object_id *real = oid;
@@ -1431,24 +1432,22 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,
if (!oi)
oi = &blank_oi;
- if (!(flags & OBJECT_INFO_SKIP_CACHED)) {
- struct cached_object *co = find_cached_object(real);
- if (co) {
- if (oi->typep)
- *(oi->typep) = co->type;
- if (oi->sizep)
- *(oi->sizep) = co->size;
- if (oi->disk_sizep)
- *(oi->disk_sizep) = 0;
- if (oi->delta_base_sha1)
- hashclr(oi->delta_base_sha1);
- if (oi->type_name)
- strbuf_addstr(oi->type_name, type_name(co->type));
- if (oi->contentp)
- *oi->contentp = xmemdupz(co->buf, co->size);
- oi->whence = OI_CACHED;
- return 0;
- }
+ co = find_cached_object(real);
+ if (co) {
+ if (oi->typep)
+ *(oi->typep) = co->type;
+ if (oi->sizep)
+ *(oi->sizep) = co->size;
+ if (oi->disk_sizep)
+ *(oi->disk_sizep) = 0;
+ if (oi->delta_base_sha1)
+ hashclr(oi->delta_base_sha1);
+ if (oi->type_name)
+ strbuf_addstr(oi->type_name, type_name(co->type));
+ if (oi->contentp)
+ *oi->contentp = xmemdupz(co->buf, co->size);
+ oi->whence = OI_CACHED;
+ return 0;
}
while (1) {
@@ -1932,8 +1931,7 @@ int repo_has_object_file_with_flags(struct repository *r,
{
if (!startup_info->have_repository)
return 0;
- return oid_object_info_extended(r, oid, NULL,
- flags | OBJECT_INFO_SKIP_CACHED) >= 0;
+ return oid_object_info_extended(r, oid, NULL, flags) >= 0;
}
int repo_has_object_file(struct repository *r,
--
2.24.1.735.g03f4e72817-goog
^ permalink raw reply related
* Re: Updating the commit message for reverts
From: Oswald Buddenhagen @ 2019-12-30 20:33 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Gal Paikin, git, ajp, Nasser Grainawi
In-Reply-To: <20191230195915.GE57251@google.com>
On Mon, Dec 30, 2019 at 11:59:15AM -0800, Jonathan Nieder wrote:
>Gal Paikin wrote:
>> The suggestion is to change the behavior to "Revert^N" instead of
>> multiple Reverts one after another.
>
>This would be replacing one kind of jargon with another, so it's not
>clear to me that it would improve matters.
>
>With Revert / Reland, we can forget the N altogether: [...]
>
the irony here is that "reland" is of course yet more jargon. :-D
i'd strongly suggest to use something that actually appears in standard
dictionaries, preferentially "reapply".
> 1. 'Do some great thing'
> 2. 'Revert "Do some great thing"'
> 3. 'Reland "Do some great thing"'
> 4. 'Revert "Do some great thing"'
> 5. (etc)
>
>For the reader of the shortlog, it's not too important how long the
>edit war has gone. The single word makes it clear what the commit
>is going to do.
>
in principle i agree, but it irks me somewhat that the summaries become
non-unique, as that always somewhat impacts history browsing.
^ permalink raw reply
* Re: [PATCH 08/16] t3310: extract common no_notes_merge_left()
From: Jakub Narebski @ 2019-12-30 20:38 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Denton Liu, Git Mailing List
In-Reply-To: <CAPig+cTaph9Mc1cdL6eNPnTF006YCZ14oLX+xKN9VTVYs7X2_A@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Fri, Dec 27, 2019 at 8:48 AM Denton Liu <liu.denton@gmail.com> wrote:
>> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
>> @@ -32,6 +32,11 @@ verify_notes () {
>> +no_notes_merge_left () {
>> + { ls .git/NOTES_MERGE_* >output || :; } &&
>> + test_must_be_empty output
>> +}
>
> This function name leaves me thinking that it's talking about
> directionality (left vs. right) and gives insufficient clue that it's
> talking about a .git/NOTES_MERGE_* file. A name such as
> assert_no_notes_merge_files() or notes_merge_files_gone() would make
> the intention more obvious.
>
>> - # No .git/NOTES_MERGE_* files left
>> - test_might_fail ls .git/NOTES_MERGE_* >output 2>/dev/null &&
>> - test_must_be_empty output &&
>
> On the other hand, the original in-code comment was not confusing,
> probably because it was obvious it was talking about an actual file,
> due to spelling out .git/NOTES_MERGE_* explicitly and due to actually
> using the literal word "file", plus the code following the comment
> made it very obvious what was happening.
>
> These observations may not be actionable since someone actually
> working on this script will know that it's dealing with
> .git/NOTES_MERGES_*, but as a reviewer not familiar with this
> particular script, reading the patch from top to bottom, I found the
> function name confusing.
The problem with clarity of meaning is enhanced by the fact that during
refactoring the "# No .git/NOTES_MERGE_* files left" comment got lost.
It could have been added in the new function, as first line; or
rephrased as this new function description.
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH] revision: allow missing promisor objects on CLI
From: Junio C Hamano @ 2019-12-30 20:33 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, matvore
In-Reply-To: <20191230183801.28538-1-jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
>> Jonathan Tan <jonathantanmy@google.com> writes:
>>
>> > object = get_reference(revs, arg, &oid, flags ^ local_flags);
>> > if (!object)
>> > - return revs->ignore_missing ? 0 : -1;
>> > + /*
>> > + * Either this object is missing and ignore_missing is true, or
>> > + * this object is a (missing) promisor object and
>> > + * exclude_promisor_objects is true.
>>
>> I had to guess and dig where these assertions are coming from; we
>> should not force future readers of the code to.
>>
>> At least this comment must say why these assertions hold. Say
>> something like "get_reference() yields NULL on only such and such
>> cases" before concluding with "and in any of these cases, we can
>> safely ignore it because ...".
>
> OK, will do.
>
>> I think the two cases the comment covers are safe for this caller to
>> silently return 0. Another case get_reference() yields NULL is when
>> oid_object_info() says it is a commit but it turns out that the
>> object is found by repo_parse_commit() to be a non-commit, isn't it?
>> I am not sure if it is safe for this caller to just return 0. There
>> may be some other "unusual-but-not-fatal" cases where get_reference()
>> does not hit a die() but returns NULL.
>
> I don't think there is any other case where get_reference() yields NULL,
> at least where I based my patch (99c33bed56 ("Git 2.25-rc0",
> 2019-12-25)). Quoting the entire get_reference():
>
>> static struct object *get_reference(struct rev_info *revs, const char *name,
>> const struct object_id *oid,
>> unsigned int flags)
>> {
>> struct object *object;
>>
>> /*
>> * If the repository has commit graphs, repo_parse_commit() avoids
>> * reading the object buffer, so use it whenever possible.
>> */
>> if (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT) {
>> struct commit *c = lookup_commit(revs->repo, oid);
>> if (!repo_parse_commit(revs->repo, c))
>> object = (struct object *) c;
>> else
>> object = NULL;
This is the case where oid must be COMMIT from oid_object_info()'s
point of view, but repo_parse_commit() finds it as a non-commit, and
object becomes NULL. This is quite different from the normal lazy
clone case where exclude-promisor-objects etc. wants to cover, that
the object whose name is oid is truly missing because it can be
fetched later from elsewhere. Instead, we have found that there is
an inconsistency in the data we have about the object, iow, a
possible corruption.
>> if (!object) {
>> if (revs->ignore_missing)
>> return object;
>
> Return NULL (the value of object).
>
>> if (revs->exclude_promisor_objects && is_promisor_object(oid))
>> return NULL;
>
> Return NULL.
>
>> die("bad object %s", name);
>
> Die (so this function invocation never returns). In conclusion, if
> object is NULL at this point in time, get_reference() either returns
> NULL or dies.
And when !object, this does not die if
- ignore-missing is in effect, or
- exclude-promisor is in effect and this is a promisor object that
is missing from the local repository.
and instead return NULL.
It just felt funny that the "we found something fishy about the
asked-for object" case is treated the same way for the purpose of
ignore-missing and exclude-promisor. The asked-for objet is
certainly not missing (i.e. we know more than we want to know about
the object---some part of our database says it is a commit and some
other part says it is not).
^ permalink raw reply
* Re: Re: [PATCH v3 1/1] git-gui: allow opening currently selected file in default app
From: Zoli Szabó @ 2019-12-30 20:31 UTC (permalink / raw)
To: Pratyush Yadav; +Cc: git
In-Reply-To: <20191230194129.kjmp6r5xuwmq4wum@yadavpratyush.com>
On 2019.12.30 21:41, Pratyush Yadav wrote:
> Hi Zoli,
>
> On 30/12/19 03:56PM, Zoli Szabó via GitGitGadget wrote:
>> From: =?UTF-8?q?Zoli=20Szab=C3=B3?= <zoli.szabo@gmail.com>
>>
>> Many times there's the need to quickly open a source file (the one you're
>> looking at in Git GUI) in the predefined text editor / IDE. Of course,
>> the file can be searched for in your preferred file manager or directly
>> in the text editor, but having the option to directly open the current
>> file from Git GUI would be just faster. This change enables just that by:
>> - clicking the diff header path (which is now highlighted as a hyperlink)
>> - or diff header path context menu -> Open;
>
> Semi-colon left in by mistake?
>
Yes, that should have been a simple period (.), signaling the end of the
sentence.
>> + eval exec $explorer [list [file nativename $full_file_path]] &
>
> IIUC, this line can be simplified to:
>
> exec $explorer [file nativename $full_file_path] &
>
> It works fine for me including on files with spaces in their names, but
> a test on Windows would be appreciated just to rule out any hidden
> surprises.
You're right. It can be simplified like you have proposed. Tested it and
works correctly also on Windows. (Since I have copied that line from the
existing `do_explore` proc, the same simplification can be done there
too. (I did not realize this myself, since I am just learning some TCL
in order to have this feature in Git GUI...))
>
> No need to send a re-roll just for these two small things. I have
> updated the commit locally before pushing the new version out [0]. The
> rest of the patch looks good. Will merge. Thanks.
>
> [0] https://github.com/prati0100/git-gui/tree/zs/open-current-file
>
Thank you very much for your support!
All the best,
Zoli
^ permalink raw reply
* Re: [PATCH 03/16] t2018: use test_must_fail for failing git commands
From: Jakub Narebski @ 2019-12-30 20:30 UTC (permalink / raw)
To: Denton Liu; +Cc: Git Mailing List
In-Reply-To: <c584c9a52b492db2128846e86afb0aadddd6f2de.1577454401.git.liu.denton@gmail.com>
Denton Liu <liu.denton@gmail.com> writes:
> While we're at it, remove redirections to /dev/null since output is
> silenced when running without `-v` and debugging output is useful with
> `-v`, remove redirections to /dev/null as it is not useful.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Very minor nit: The underlined part (after last comma) duplicates what
already has been said.
--
Jakub Narębski
^ permalink raw reply
* Re: Updating the commit message for reverts
From: Jonathan Nieder @ 2019-12-30 19:59 UTC (permalink / raw)
To: Gal Paikin; +Cc: git, ajp, Oswald Buddenhagen, Nasser Grainawi
In-Reply-To: <CAEsQYpNtgMgwjVSOYB9vn-YPvKyKPZ2yZ3NigAVe3PztTN4v8w@mail.gmail.com>
(not cc-ing the Gerrit list after all, since only members can post)
Hi Gal,
Gal Paikin wrote:
> I work on the Gerrit team and I would like to change the default
> behavior for suggested commit messages for Reverts.
Thanks for writing. I like it when tools behave consistently with one
another, so I'm glad you brought it up here.
> Currently, if the user is trying a change called '"Revert "change X"',
> the suggested commit message would be 'Revert "Revert "Change X""'
> which is silly, since sometimes users want to revert the same change
> many times.
For context, [1] is where this was discussed previously on the Gerrit
mailing list. Do you have more context? For example, have there been
reports from users requesting this kind of change?
For a bit of precedent, the Chromium project has some custom logic[2]
that rewrites a revert of a revert to "Reland". I kind of like that,
since it makes the intention behind the revert-of-revert a little
clearer.
If both reverts are clean, then the typical use case I can think of is
that a prerequisite was missing, hence the revert; after all the
prerequisites were in place, it was then safe to roll forward, hence
the reland. I would hope that the full commit messages for these
revert commits would provide this context since without that kind of
communication from the people involved, a reader is lost.
> The suggestion is to change the behavior to "Revert^N" instead of
> multiple Reverts one after another.
This would be replacing one kind of jargon with another, so it's not
clear to me that it would improve matters.
With Revert / Reland, we can forget the N altogether:
1. 'Do some great thing'
2. 'Revert "Do some great thing"'
3. 'Reland "Do some great thing"'
4. 'Revert "Do some great thing"'
5. (etc)
For the reader of the shortlog, it's not too important how long the
edit war has gone. The single word makes it clear what the commit
is going to do.
In the full commit message, I would hope that people put in some
explanation of the story so far to explain why they have gone
back-and-forth all these times.
Thanks and hope that helps,
Jonathan
[1] https://groups.google.com/d/msg/repo-discuss/edl43LcyEKE/f9Q65kOcBgAJ
[2] https://chromium-review.googlesource.com/c/infra/gerrit-plugins/chromium-behavior/+/757684
^ permalink raw reply
* Re: Updating the commit message for reverts
From: Jonathan Nieder @ 2019-12-30 19:55 UTC (permalink / raw)
To: Gal Paikin; +Cc: git, repo-discuss, ajp
In-Reply-To: <CAEsQYpNtgMgwjVSOYB9vn-YPvKyKPZ2yZ3NigAVe3PztTN4v8w@mail.gmail.com>
(also cc-ing the Gerrit list. The git mailing list requires plain-text
email and prefers bottom-posting; there shouldn't be any other
gotchas if you reply-all)
Hi Gal,
Gal Paikin wrote:
> I work on the Gerrit team and I would like to change the default
> behavior for suggested commit messages for Reverts.
Thanks for writing. I like it when tools behave consistently with one
another, so I'm glad you brought it up here.
> Currently, if the user is trying a change called '"Revert "change X"',
> the suggested commit message would be 'Revert "Revert "Change X""'
> which is silly, since sometimes users want to revert the same change
> many times.
For context, [1] is where this was discussed previously on the Gerrit
mailing list. Do you have more context? For example, have there been
reports from users requesting this kind of change?
For a bit of precedent, the Chromium project has some custom logic[2]
that rewrites a revert of a revert to "Reland". I kind of like that,
since it makes the intention behind the revert-of-revert a little
clearer.
If both reverts are clean, then the typical use case I can think of is
that a prerequisite was missing, hence the revert; after all the
prerequisites were in place, it was then safe to roll forward, hence
the reland. I would hope that the full commit messages for these
revert commits would provide this context since without that kind of
communication from the people involved, a reader is lost.
> The suggestion is to change the behavior to "Revert^N" instead of
> multiple Reverts one after another.
This would be replacing one kind of jargon with another, so it's not
clear to me that it would improve matters.
With Revert / Reland, we can forget the N altogether:
1. 'Do some great thing'
2. 'Revert "Do some great thing"'
3. 'Reland "Do some great thing"'
4. 'Revert "Do some great thing"'
5. (etc)
For the reader of the shortlog, it's not too important how long the
edit war has gone. The single word makes it clear what the commit
is going to do.
In the full commit message, I would hope that people put in some
explanation of the story so far to explain why they have gone
back-and-forth all these times.
Thanks and hope that helps,
Jonathan
[1] https://groups.google.com/d/msg/repo-discuss/edl43LcyEKE/f9Q65kOcBgAJ
[2] https://chromium-review.googlesource.com/c/infra/gerrit-plugins/chromium-behavior/+/757684
^ permalink raw reply
* Re: [PATCH v3 1/1] git-gui: allow opening currently selected file in default app
From: Pratyush Yadav @ 2019-12-30 19:41 UTC (permalink / raw)
To: Zoli Szabó via GitGitGadget; +Cc: git, Zoli Szabó
In-Reply-To: <1b2363be726c6d70746aec9fae62edaf857cd665.1577721419.git.gitgitgadget@gmail.com>
Hi Zoli,
On 30/12/19 03:56PM, Zoli Szabó via GitGitGadget wrote:
> From: =?UTF-8?q?Zoli=20Szab=C3=B3?= <zoli.szabo@gmail.com>
>
> Many times there's the need to quickly open a source file (the one you're
> looking at in Git GUI) in the predefined text editor / IDE. Of course,
> the file can be searched for in your preferred file manager or directly
> in the text editor, but having the option to directly open the current
> file from Git GUI would be just faster. This change enables just that by:
> - clicking the diff header path (which is now highlighted as a hyperlink)
> - or diff header path context menu -> Open;
Semi-colon left in by mistake?
>
> Note: executable files will be run and not opened for editing.
>
> Signed-off-by: Zoli Szabó <zoli.szabo@gmail.com>
> ---
> git-gui.sh | 29 ++++++++++++++++++++++++-----
> 1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index c1be733e3e..8920e4ddb0 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -2259,9 +2258,23 @@ proc do_explore {} {
> +# Open file relative to the working tree by the default associated
> app.
> +proc do_file_open {file} {
> + global _gitworktree
> + set explorer [get_explorer]
> + set full_file_path [file join $_gitworktree $file]
> + eval exec $explorer [list [file nativename $full_file_path]] &
IIUC, this line can be simplified to:
exec $explorer [file nativename $full_file_path] &
It works fine for me including on files with spaces in their names, but
a test on Windows would be appreciated just to rule out any hidden
surprises.
No need to send a re-roll just for these two small things. I have
updated the commit locally before pushing the new version out [0]. The
rest of the patch looks good. Will merge. Thanks.
> +}
> +
> set is_quitting 0
> set ret_code 1
>
[0] https://github.com/prati0100/git-gui/tree/zs/open-current-file
--
Regards,
Pratyush Yadav
^ permalink raw reply
* Re: [PATCH 2/3] t: directly test parse_pathspec_file()
From: Alexandr Miloslavskiy @ 2019-12-30 19:16 UTC (permalink / raw)
To: Junio C Hamano, Alexandr Miloslavskiy via GitGitGadget; +Cc: git
In-Reply-To: <xmqq8smthcib.fsf@gitster-ct.c.googlers.com>
Thanks for having a look! I think I have fixed all mentioned issues in V2.
^ permalink raw reply
* [PATCH v2 3/3] t: drop copy&pasted tests for --pathspec-from-file
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-12-30 19:15 UTC (permalink / raw)
To: git; +Cc: Alexandr Miloslavskiy, Junio C Hamano, Alexandr Miloslavskiy
In-Reply-To: <pull.503.v2.git.1577733329.gitgitgadget@gmail.com>
From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
With direct tests for `parse_pathspec_file()` already in place, it is
not very reasonable to copy&paste 6 tests for `parse_pathspec_file()`
for every git command that uses it (I counted 13 commands that could use
it eventually).
I believe that indirect tests are redundant because I don't expect
direct tests to ever disagree with indirect tests.
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
t/t2026-checkout-pathspec-file.sh | 75 +--------------------------
t/t2072-restore-pathspec-file.sh | 75 +--------------------------
t/t3704-add-pathspec-file.sh | 75 +--------------------------
t/t7107-reset-pathspec-file.sh | 84 +++----------------------------
t/t7526-commit-pathspec-file.sh | 75 +--------------------------
5 files changed, 16 insertions(+), 368 deletions(-)
diff --git a/t/t2026-checkout-pathspec-file.sh b/t/t2026-checkout-pathspec-file.sh
index 2dc8901bca..559b4528d7 100755
--- a/t/t2026-checkout-pathspec-file.sh
+++ b/t/t2026-checkout-pathspec-file.sh
@@ -35,7 +35,7 @@ verify_expect () {
test_cmp expect actual
}
-test_expect_success '--pathspec-from-file from stdin' '
+test_expect_success 'simplest' '
restore_checkpoint &&
echo fileA.t | git checkout --pathspec-from-file=- HEAD^1 &&
@@ -46,19 +46,7 @@ test_expect_success '--pathspec-from-file from stdin' '
verify_expect
'
-test_expect_success '--pathspec-from-file from file' '
- restore_checkpoint &&
-
- echo fileA.t >list &&
- git checkout --pathspec-from-file=list HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'NUL delimiters' '
+test_expect_success '--pathspec-file-nul' '
restore_checkpoint &&
printf "fileA.t\0fileB.t\0" | git checkout --pathspec-from-file=- --pathspec-file-nul HEAD^1 &&
@@ -70,65 +58,6 @@ test_expect_success 'NUL delimiters' '
verify_expect
'
-test_expect_success 'LF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t\n" | git checkout --pathspec-from-file=- HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- M fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'no trailing delimiter' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t" | git checkout --pathspec-from-file=- HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- M fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\r\nfileB.t\r\n" | git checkout --pathspec-from-file=- HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- M fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes' '
- restore_checkpoint &&
-
- # shell takes \\\\101 and spits \\101
- # printf takes \\101 and spits \101
- # git takes \101 and spits A
- printf "\"file\\\\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
- restore_checkpoint &&
-
- # shell takes \\\\101 and spits \\101
- # printf takes \\101 and spits \101
- printf "\"file\\\\101.t\"" >list &&
- test_must_fail git checkout --pathspec-from-file=list --pathspec-file-nul HEAD^1
-'
-
test_expect_success 'only touches what was listed' '
restore_checkpoint &&
diff --git a/t/t2072-restore-pathspec-file.sh b/t/t2072-restore-pathspec-file.sh
index 70e95ef3b6..9b3125d582 100755
--- a/t/t2072-restore-pathspec-file.sh
+++ b/t/t2072-restore-pathspec-file.sh
@@ -35,7 +35,7 @@ verify_expect () {
test_cmp expect actual
}
-test_expect_success '--pathspec-from-file from stdin' '
+test_expect_success 'simplest' '
restore_checkpoint &&
echo fileA.t | git restore --pathspec-from-file=- --source=HEAD^1 &&
@@ -46,19 +46,7 @@ test_expect_success '--pathspec-from-file from stdin' '
verify_expect
'
-test_expect_success '--pathspec-from-file from file' '
- restore_checkpoint &&
-
- echo fileA.t >list &&
- git restore --pathspec-from-file=list --source=HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'NUL delimiters' '
+test_expect_success '--pathspec-file-nul' '
restore_checkpoint &&
printf "fileA.t\0fileB.t\0" | git restore --pathspec-from-file=- --pathspec-file-nul --source=HEAD^1 &&
@@ -70,65 +58,6 @@ test_expect_success 'NUL delimiters' '
verify_expect
'
-test_expect_success 'LF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t\n" | git restore --pathspec-from-file=- --source=HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- M fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'no trailing delimiter' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t" | git restore --pathspec-from-file=- --source=HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- M fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\r\nfileB.t\r\n" | git restore --pathspec-from-file=- --source=HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- M fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes' '
- restore_checkpoint &&
-
- # shell takes \\\\101 and spits \\101
- # printf takes \\101 and spits \101
- # git takes \101 and spits A
- printf "\"file\\\\101.t\"" | git restore --pathspec-from-file=- --source=HEAD^1 &&
-
- cat >expect <<-\EOF &&
- M fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
- restore_checkpoint &&
-
- # shell takes \\\\101 and spits \\101
- # printf takes \\101 and spits \101
- printf "\"file\\\\101.t\"" >list &&
- test_must_fail git restore --pathspec-from-file=list --pathspec-file-nul --source=HEAD^1
-'
-
test_expect_success 'only touches what was listed' '
restore_checkpoint &&
diff --git a/t/t3704-add-pathspec-file.sh b/t/t3704-add-pathspec-file.sh
index 2e0141fcce..9009f8a9ac 100755
--- a/t/t3704-add-pathspec-file.sh
+++ b/t/t3704-add-pathspec-file.sh
@@ -23,7 +23,7 @@ verify_expect () {
test_cmp expect actual
}
-test_expect_success '--pathspec-from-file from stdin' '
+test_expect_success 'simplest' '
restore_checkpoint &&
echo fileA.t | git add --pathspec-from-file=- &&
@@ -34,19 +34,7 @@ test_expect_success '--pathspec-from-file from stdin' '
verify_expect
'
-test_expect_success '--pathspec-from-file from file' '
- restore_checkpoint &&
-
- echo fileA.t >list &&
- git add --pathspec-from-file=list &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'NUL delimiters' '
+test_expect_success '--pathspec-file-nul' '
restore_checkpoint &&
printf "fileA.t\0fileB.t\0" | git add --pathspec-from-file=- --pathspec-file-nul &&
@@ -58,65 +46,6 @@ test_expect_success 'NUL delimiters' '
verify_expect
'
-test_expect_success 'LF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t\n" | git add --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'no trailing delimiter' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t" | git add --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\r\nfileB.t\r\n" | git add --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes' '
- restore_checkpoint &&
-
- # shell takes \\\\101 and spits \\101
- # printf takes \\101 and spits \101
- # git takes \101 and spits A
- printf "\"file\\\\101.t\"" | git add --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
- restore_checkpoint &&
-
- # shell takes \\\\101 and spits \\101
- # printf takes \\101 and spits \101
- printf "\"file\\\\101.t\"" >list &&
- test_must_fail git add --pathspec-from-file=list --pathspec-file-nul
-'
-
test_expect_success 'only touches what was listed' '
restore_checkpoint &&
diff --git a/t/t7107-reset-pathspec-file.sh b/t/t7107-reset-pathspec-file.sh
index 52a44f033d..5b845f4f7c 100755
--- a/t/t7107-reset-pathspec-file.sh
+++ b/t/t7107-reset-pathspec-file.sh
@@ -25,7 +25,7 @@ verify_expect () {
test_cmp expect actual
}
-test_expect_success '--pathspec-from-file from stdin' '
+test_expect_success 'simplest' '
restore_checkpoint &&
git rm fileA.t &&
@@ -37,20 +37,7 @@ test_expect_success '--pathspec-from-file from stdin' '
verify_expect
'
-test_expect_success '--pathspec-from-file from file' '
- restore_checkpoint &&
-
- git rm fileA.t &&
- echo fileA.t >list &&
- git reset --pathspec-from-file=list &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'NUL delimiters' '
+test_expect_success '--pathspec-file-nul' '
restore_checkpoint &&
git rm fileA.t fileB.t &&
@@ -63,76 +50,21 @@ test_expect_success 'NUL delimiters' '
verify_expect
'
-test_expect_success 'LF delimiters' '
- restore_checkpoint &&
-
- git rm fileA.t fileB.t &&
- printf "fileA.t\nfileB.t\n" | git reset --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- D fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'no trailing delimiter' '
- restore_checkpoint &&
-
- git rm fileA.t fileB.t &&
- printf "fileA.t\nfileB.t" | git reset --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- D fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
+test_expect_success 'only touches what was listed' '
restore_checkpoint &&
- git rm fileA.t fileB.t &&
- printf "fileA.t\r\nfileB.t\r\n" | git reset --pathspec-from-file=- &&
+ git rm fileA.t fileB.t fileC.t fileD.t &&
+ printf "fileB.t\nfileC.t\n" | git reset --pathspec-from-file=- &&
cat >expect <<-\EOF &&
- D fileA.t
+ D fileA.t
D fileB.t
+ D fileC.t
+ D fileD.t
EOF
verify_expect
'
-test_expect_success 'quotes' '
- restore_checkpoint &&
-
- # shell takes \\\\101 and spits \\101
- # printf takes \\101 and spits \101
- # git takes \101 and spits A
- git rm fileA.t &&
- printf "\"file\\\\101.t\"" | git reset --pathspec-from-file=- &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
- restore_checkpoint &&
-
- # shell takes \\\\101 and spits \\101
- # printf takes \\101 and spits \101
- git rm fileA.t &&
- printf "\"file\\\\101.t\"" >list &&
- # Note: "git reset" has not yet learned to fail on wrong pathspecs
- git reset --pathspec-from-file=list --pathspec-file-nul &&
-
- cat >expect <<-\EOF &&
- D fileA.t
- EOF
- test_must_fail verify_expect
-'
-
test_expect_success '--pathspec-from-file is not compatible with --soft or --hard' '
restore_checkpoint &&
diff --git a/t/t7526-commit-pathspec-file.sh b/t/t7526-commit-pathspec-file.sh
index e7dc2ff8b1..8d6c652690 100755
--- a/t/t7526-commit-pathspec-file.sh
+++ b/t/t7526-commit-pathspec-file.sh
@@ -26,7 +26,7 @@ verify_expect () {
test_cmp expect actual
}
-test_expect_success '--pathspec-from-file from stdin' '
+test_expect_success 'simplest' '
restore_checkpoint &&
echo fileA.t | git commit --pathspec-from-file=- -m "Commit" &&
@@ -37,19 +37,7 @@ test_expect_success '--pathspec-from-file from stdin' '
verify_expect
'
-test_expect_success '--pathspec-from-file from file' '
- restore_checkpoint &&
-
- echo fileA.t >list &&
- git commit --pathspec-from-file=list -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- EOF
- verify_expect
-'
-
-test_expect_success 'NUL delimiters' '
+test_expect_success '--pathspec-file-nul' '
restore_checkpoint &&
printf "fileA.t\0fileB.t\0" | git commit --pathspec-from-file=- --pathspec-file-nul -m "Commit" &&
@@ -61,65 +49,6 @@ test_expect_success 'NUL delimiters' '
verify_expect
'
-test_expect_success 'LF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t\n" | git commit --pathspec-from-file=- -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'no trailing delimiter' '
- restore_checkpoint &&
-
- printf "fileA.t\nfileB.t" | git commit --pathspec-from-file=- -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'CRLF delimiters' '
- restore_checkpoint &&
-
- printf "fileA.t\r\nfileB.t\r\n" | git commit --pathspec-from-file=- -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- A fileB.t
- EOF
- verify_expect
-'
-
-test_expect_success 'quotes' '
- restore_checkpoint &&
-
- # shell takes \\\\101 and spits \\101
- # printf takes \\101 and spits \101
- # git takes \101 and spits A
- printf "\"file\\\\101.t\"" | git commit --pathspec-from-file=- -m "Commit" &&
-
- cat >expect <<-\EOF &&
- A fileA.t
- EOF
- verify_expect expect
-'
-
-test_expect_success 'quotes not compatible with --pathspec-file-nul' '
- restore_checkpoint &&
-
- # shell takes \\\\101 and spits \\101
- # printf takes \\101 and spits \101
- printf "\"file\\\\101.t\"" >list &&
- test_must_fail git commit --pathspec-from-file=list --pathspec-file-nul -m "Commit"
-'
-
test_expect_success 'only touches what was listed' '
restore_checkpoint &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 1/3] t: fix quotes tests for --pathspec-from-file
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-12-30 19:15 UTC (permalink / raw)
To: git; +Cc: Alexandr Miloslavskiy, Junio C Hamano, Alexandr Miloslavskiy
In-Reply-To: <pull.503.v2.git.1577733329.gitgitgadget@gmail.com>
From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
While working on the next patch, I also noticed that quotes testing via
`"\"file\\101.t\""` was somewhat incorrect: I escaped `\` one time while
I had to escape it two times! Tests still worked due to `"` being
preserved which in turn prevented pathspec from matching files.
Fix this by properly escaping one more time.
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
t/t2026-checkout-pathspec-file.sh | 9 +++++++--
t/t2072-restore-pathspec-file.sh | 9 +++++++--
t/t3704-add-pathspec-file.sh | 9 +++++++--
t/t7107-reset-pathspec-file.sh | 9 +++++++--
t/t7526-commit-pathspec-file.sh | 9 +++++++--
5 files changed, 35 insertions(+), 10 deletions(-)
diff --git a/t/t2026-checkout-pathspec-file.sh b/t/t2026-checkout-pathspec-file.sh
index f62fd27440..2dc8901bca 100755
--- a/t/t2026-checkout-pathspec-file.sh
+++ b/t/t2026-checkout-pathspec-file.sh
@@ -109,7 +109,10 @@ test_expect_success 'CRLF delimiters' '
test_expect_success 'quotes' '
restore_checkpoint &&
- printf "\"file\\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
+ # shell takes \\\\101 and spits \\101
+ # printf takes \\101 and spits \101
+ # git takes \101 and spits A
+ printf "\"file\\\\101.t\"" | git checkout --pathspec-from-file=- HEAD^1 &&
cat >expect <<-\EOF &&
M fileA.t
@@ -120,7 +123,9 @@ test_expect_success 'quotes' '
test_expect_success 'quotes not compatible with --pathspec-file-nul' '
restore_checkpoint &&
- printf "\"file\\101.t\"" >list &&
+ # shell takes \\\\101 and spits \\101
+ # printf takes \\101 and spits \101
+ printf "\"file\\\\101.t\"" >list &&
test_must_fail git checkout --pathspec-from-file=list --pathspec-file-nul HEAD^1
'
diff --git a/t/t2072-restore-pathspec-file.sh b/t/t2072-restore-pathspec-file.sh
index db58e83735..70e95ef3b6 100755
--- a/t/t2072-restore-pathspec-file.sh
+++ b/t/t2072-restore-pathspec-file.sh
@@ -109,7 +109,10 @@ test_expect_success 'CRLF delimiters' '
test_expect_success 'quotes' '
restore_checkpoint &&
- printf "\"file\\101.t\"" | git restore --pathspec-from-file=- --source=HEAD^1 &&
+ # shell takes \\\\101 and spits \\101
+ # printf takes \\101 and spits \101
+ # git takes \101 and spits A
+ printf "\"file\\\\101.t\"" | git restore --pathspec-from-file=- --source=HEAD^1 &&
cat >expect <<-\EOF &&
M fileA.t
@@ -120,7 +123,9 @@ test_expect_success 'quotes' '
test_expect_success 'quotes not compatible with --pathspec-file-nul' '
restore_checkpoint &&
- printf "\"file\\101.t\"" >list &&
+ # shell takes \\\\101 and spits \\101
+ # printf takes \\101 and spits \101
+ printf "\"file\\\\101.t\"" >list &&
test_must_fail git restore --pathspec-from-file=list --pathspec-file-nul --source=HEAD^1
'
diff --git a/t/t3704-add-pathspec-file.sh b/t/t3704-add-pathspec-file.sh
index 3cfdb669b7..2e0141fcce 100755
--- a/t/t3704-add-pathspec-file.sh
+++ b/t/t3704-add-pathspec-file.sh
@@ -97,7 +97,10 @@ test_expect_success 'CRLF delimiters' '
test_expect_success 'quotes' '
restore_checkpoint &&
- printf "\"file\\101.t\"" | git add --pathspec-from-file=- &&
+ # shell takes \\\\101 and spits \\101
+ # printf takes \\101 and spits \101
+ # git takes \101 and spits A
+ printf "\"file\\\\101.t\"" | git add --pathspec-from-file=- &&
cat >expect <<-\EOF &&
A fileA.t
@@ -108,7 +111,9 @@ test_expect_success 'quotes' '
test_expect_success 'quotes not compatible with --pathspec-file-nul' '
restore_checkpoint &&
- printf "\"file\\101.t\"" >list &&
+ # shell takes \\\\101 and spits \\101
+ # printf takes \\101 and spits \101
+ printf "\"file\\\\101.t\"" >list &&
test_must_fail git add --pathspec-from-file=list --pathspec-file-nul
'
diff --git a/t/t7107-reset-pathspec-file.sh b/t/t7107-reset-pathspec-file.sh
index 6b1a731fff..52a44f033d 100755
--- a/t/t7107-reset-pathspec-file.sh
+++ b/t/t7107-reset-pathspec-file.sh
@@ -105,8 +105,11 @@ test_expect_success 'CRLF delimiters' '
test_expect_success 'quotes' '
restore_checkpoint &&
+ # shell takes \\\\101 and spits \\101
+ # printf takes \\101 and spits \101
+ # git takes \101 and spits A
git rm fileA.t &&
- printf "\"file\\101.t\"" | git reset --pathspec-from-file=- &&
+ printf "\"file\\\\101.t\"" | git reset --pathspec-from-file=- &&
cat >expect <<-\EOF &&
D fileA.t
@@ -117,8 +120,10 @@ test_expect_success 'quotes' '
test_expect_success 'quotes not compatible with --pathspec-file-nul' '
restore_checkpoint &&
+ # shell takes \\\\101 and spits \\101
+ # printf takes \\101 and spits \101
git rm fileA.t &&
- printf "\"file\\101.t\"" >list &&
+ printf "\"file\\\\101.t\"" >list &&
# Note: "git reset" has not yet learned to fail on wrong pathspecs
git reset --pathspec-from-file=list --pathspec-file-nul &&
diff --git a/t/t7526-commit-pathspec-file.sh b/t/t7526-commit-pathspec-file.sh
index 4b58901ed6..e7dc2ff8b1 100755
--- a/t/t7526-commit-pathspec-file.sh
+++ b/t/t7526-commit-pathspec-file.sh
@@ -100,7 +100,10 @@ test_expect_success 'CRLF delimiters' '
test_expect_success 'quotes' '
restore_checkpoint &&
- printf "\"file\\101.t\"" | git commit --pathspec-from-file=- -m "Commit" &&
+ # shell takes \\\\101 and spits \\101
+ # printf takes \\101 and spits \101
+ # git takes \101 and spits A
+ printf "\"file\\\\101.t\"" | git commit --pathspec-from-file=- -m "Commit" &&
cat >expect <<-\EOF &&
A fileA.t
@@ -111,7 +114,9 @@ test_expect_success 'quotes' '
test_expect_success 'quotes not compatible with --pathspec-file-nul' '
restore_checkpoint &&
- printf "\"file\\101.t\"" >list &&
+ # shell takes \\\\101 and spits \\101
+ # printf takes \\101 and spits \101
+ printf "\"file\\\\101.t\"" >list &&
test_must_fail git commit --pathspec-from-file=list --pathspec-file-nul -m "Commit"
'
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 2/3] t: directly test parse_pathspec_file()
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-12-30 19:15 UTC (permalink / raw)
To: git; +Cc: Alexandr Miloslavskiy, Junio C Hamano, Alexandr Miloslavskiy
In-Reply-To: <pull.503.v2.git.1577733329.gitgitgadget@gmail.com>
From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Previously, `parse_pathspec_file()` was tested indirectly by invoking
git commands with properly crafted inputs. As demonstrated by the
previous bugfix, testing complicated black boxes indirectly can lead to
tests that silently test the wrong thing.
Introduce direct tests for `parse_pathspec_file()`.
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
Makefile | 1 +
t/helper/test-parse-pathspec-file.c | 33 +++++++++
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t0067-parse_pathspec_file.sh | 104 ++++++++++++++++++++++++++++
5 files changed, 140 insertions(+)
create mode 100644 t/helper/test-parse-pathspec-file.c
create mode 100755 t/t0067-parse_pathspec_file.sh
diff --git a/Makefile b/Makefile
index 09f98b777c..0061f96e8a 100644
--- a/Makefile
+++ b/Makefile
@@ -721,6 +721,7 @@ TEST_BUILTINS_OBJS += test-mktemp.o
TEST_BUILTINS_OBJS += test-oidmap.o
TEST_BUILTINS_OBJS += test-online-cpus.o
TEST_BUILTINS_OBJS += test-parse-options.o
+TEST_BUILTINS_OBJS += test-parse-pathspec-file.o
TEST_BUILTINS_OBJS += test-path-utils.o
TEST_BUILTINS_OBJS += test-pkt-line.o
TEST_BUILTINS_OBJS += test-prio-queue.o
diff --git a/t/helper/test-parse-pathspec-file.c b/t/helper/test-parse-pathspec-file.c
new file mode 100644
index 0000000000..02f4ccfd2a
--- /dev/null
+++ b/t/helper/test-parse-pathspec-file.c
@@ -0,0 +1,33 @@
+#include "test-tool.h"
+#include "parse-options.h"
+#include "pathspec.h"
+#include "gettext.h"
+
+int cmd__parse_pathspec_file(int argc, const char **argv)
+{
+ struct pathspec pathspec;
+ const char *pathspec_from_file = 0;
+ int pathspec_file_nul = 0, i;
+
+ static const char *const usage[] = {
+ "test-tool parse-pathspec-file --pathspec-from-file [--pathspec-file-nul]",
+ NULL
+ };
+
+ struct option options[] = {
+ OPT_PATHSPEC_FROM_FILE(&pathspec_from_file),
+ OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),
+ OPT_END()
+ };
+
+ parse_options(argc, argv, 0, options, usage, 0);
+
+ parse_pathspec_file(&pathspec, 0, 0, 0, pathspec_from_file,
+ pathspec_file_nul);
+
+ for (i = 0; i < pathspec.nr; i++)
+ printf("%s\n", pathspec.items[i].original);
+
+ clear_pathspec(&pathspec);
+ return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index f20989d449..c9a232d238 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -39,6 +39,7 @@ static struct test_cmd cmds[] = {
{ "oidmap", cmd__oidmap },
{ "online-cpus", cmd__online_cpus },
{ "parse-options", cmd__parse_options },
+ { "parse-pathspec-file", cmd__parse_pathspec_file },
{ "path-utils", cmd__path_utils },
{ "pkt-line", cmd__pkt_line },
{ "prio-queue", cmd__prio_queue },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 8ed2af71d1..c8549fd87f 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -29,6 +29,7 @@ int cmd__mktemp(int argc, const char **argv);
int cmd__oidmap(int argc, const char **argv);
int cmd__online_cpus(int argc, const char **argv);
int cmd__parse_options(int argc, const char **argv);
+int cmd__parse_pathspec_file(int argc, const char** argv);
int cmd__path_utils(int argc, const char **argv);
int cmd__pkt_line(int argc, const char **argv);
int cmd__prio_queue(int argc, const char **argv);
diff --git a/t/t0067-parse_pathspec_file.sh b/t/t0067-parse_pathspec_file.sh
new file mode 100755
index 0000000000..77b44f6702
--- /dev/null
+++ b/t/t0067-parse_pathspec_file.sh
@@ -0,0 +1,104 @@
+#!/bin/sh
+
+test_description='Test parse_pathspec_file()'
+
+. ./test-lib.sh
+
+test_expect_success 'one item from stdin' '
+ cat >expect <<-\EOF &&
+ fileA.t
+ EOF
+
+ echo fileA.t |
+ test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
+
+ test_cmp expect actual
+'
+
+test_expect_success 'one item from file' '
+ cat >expect <<-\EOF &&
+ fileA.t
+ EOF
+
+ echo fileA.t >list &&
+ test-tool parse-pathspec-file --pathspec-from-file=list >actual &&
+
+ test_cmp expect actual
+'
+
+test_expect_success 'NUL delimiters' '
+ cat >expect <<-\EOF &&
+ fileA.t
+ fileB.t
+ EOF
+
+ printf "fileA.t\0fileB.t\0" |
+ test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&
+
+ test_cmp expect actual
+'
+
+test_expect_success 'LF delimiters' '
+ cat >expect <<-\EOF &&
+ fileA.t
+ fileB.t
+ EOF
+
+ printf "fileA.t\nfileB.t\n" |
+ test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
+
+ test_cmp expect actual
+'
+
+test_expect_success 'no trailing delimiter' '
+ cat >expect <<-\EOF &&
+ fileA.t
+ fileB.t
+ EOF
+
+ printf "fileA.t\nfileB.t" |
+ test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
+
+ test_cmp expect actual
+'
+
+test_expect_success 'CRLF delimiters' '
+ cat >expect <<-\EOF &&
+ fileA.t
+ fileB.t
+ EOF
+
+ printf "fileA.t\r\nfileB.t\r\n" |
+ test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
+
+ test_cmp expect actual
+'
+
+test_expect_success 'quotes' '
+ cat >expect <<-\EOF &&
+ fileA.t
+ EOF
+
+ # shell takes \\\\101 and spits \\101
+ # printf takes \\101 and spits \101
+ # git takes \101 and spits A
+ printf "\"file\\\\101.t\"" |
+ test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
+
+ test_cmp expect actual
+'
+
+test_expect_success '--pathspec-file-nul takes quotes literally' '
+ cat >expect <<-\EOF &&
+ "file\101.t"
+ EOF
+
+ # shell takes \\\\101 and spits \\101
+ # printf takes \\101 and spits \101
+ printf "\"file\\\\101.t\"" |
+ test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&
+
+ test_cmp expect actual
+'
+
+test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 0/3] t: rework tests for --pathspec-from-file
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-12-30 19:15 UTC (permalink / raw)
To: git; +Cc: Alexandr Miloslavskiy, Junio C Hamano
In-Reply-To: <pull.503.git.1577727747.gitgitgadget@gmail.com>
Please refer to commit messages for rationale.
This branch is a follow-up for [1] where part of branch was merged into `master` via [2].
Previously in [3] there were some concerns on whether removing
copy&pasted tests is good. I still think that yes, it 's a good thing,
mostly because of high volume of potential 13*6=78 duplicate tests.
Still, I separated this change as last patch, so that the remaining
part of the branch can be taken without it.
[1] https://lore.kernel.org/git/pull.490.git.1576161385.gitgitgadget@gmail.com/
[2] https://public-inbox.org/git/pull.445.v4.git.1575381738.gitgitgadget@gmail.com/
[3] https://lore.kernel.org/git/xmqqwoatcn5u.fsf@gitster-ct.c.googlers.com/
Changes since V1
----------------
Small code formatting changes suggested in V1.
Alexandr Miloslavskiy (3):
t: fix quotes tests for --pathspec-from-file
t: directly test parse_pathspec_file()
t: drop copy&pasted tests for --pathspec-from-file
Makefile | 1 +
t/helper/test-parse-pathspec-file.c | 33 +++++++++
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t0067-parse_pathspec_file.sh | 104 ++++++++++++++++++++++++++++
t/t2026-checkout-pathspec-file.sh | 70 +------------------
t/t2072-restore-pathspec-file.sh | 70 +------------------
t/t3704-add-pathspec-file.sh | 70 +------------------
t/t7107-reset-pathspec-file.sh | 79 +++------------------
t/t7526-commit-pathspec-file.sh | 70 +------------------
10 files changed, 156 insertions(+), 343 deletions(-)
create mode 100644 t/helper/test-parse-pathspec-file.c
create mode 100755 t/t0067-parse_pathspec_file.sh
base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-503%2FSyntevoAlex%2F%230207(git)_2b_test_parse_directly-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-503/SyntevoAlex/#0207(git)_2b_test_parse_directly-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/503
Range-diff vs v1:
1: ab9519298d = 1: 6193dc7396 t: fix quotes tests for --pathspec-from-file
2: 27383a5b08 ! 2: ab449ac15a t: directly test parse_pathspec_file()
@@ -55,9 +55,8 @@
+ parse_pathspec_file(&pathspec, 0, 0, 0, pathspec_from_file,
+ pathspec_file_nul);
+
-+ for (i = 0; i < pathspec.nr; i++) {
++ for (i = 0; i < pathspec.nr; i++)
+ printf("%s\n", pathspec.items[i].original);
-+ }
+
+ clear_pathspec(&pathspec);
+ return 0;
@@ -99,84 +98,99 @@
+. ./test-lib.sh
+
+test_expect_success 'one item from stdin' '
-+ echo fileA.t | test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
-+
+ cat >expect <<-\EOF &&
+ fileA.t
+ EOF
++
++ echo fileA.t |
++ test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
++
+ test_cmp expect actual
+'
+
+test_expect_success 'one item from file' '
-+ echo fileA.t >list &&
-+ test-tool parse-pathspec-file --pathspec-from-file=list >actual &&
-+
+ cat >expect <<-\EOF &&
+ fileA.t
+ EOF
++
++ echo fileA.t >list &&
++ test-tool parse-pathspec-file --pathspec-from-file=list >actual &&
++
+ test_cmp expect actual
+'
+
+test_expect_success 'NUL delimiters' '
-+ printf "fileA.t\0fileB.t\0" | test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&
-+
+ cat >expect <<-\EOF &&
+ fileA.t
+ fileB.t
+ EOF
++
++ printf "fileA.t\0fileB.t\0" |
++ test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&
++
+ test_cmp expect actual
+'
+
+test_expect_success 'LF delimiters' '
-+ printf "fileA.t\nfileB.t\n" | test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
-+
+ cat >expect <<-\EOF &&
+ fileA.t
+ fileB.t
+ EOF
++
++ printf "fileA.t\nfileB.t\n" |
++ test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
++
+ test_cmp expect actual
+'
+
+test_expect_success 'no trailing delimiter' '
-+ printf "fileA.t\nfileB.t" | test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
-+
+ cat >expect <<-\EOF &&
+ fileA.t
+ fileB.t
+ EOF
++
++ printf "fileA.t\nfileB.t" |
++ test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
++
+ test_cmp expect actual
+'
+
+test_expect_success 'CRLF delimiters' '
-+ printf "fileA.t\r\nfileB.t\r\n" | test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
-+
+ cat >expect <<-\EOF &&
+ fileA.t
+ fileB.t
+ EOF
++
++ printf "fileA.t\r\nfileB.t\r\n" |
++ test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
++
+ test_cmp expect actual
+'
+
+test_expect_success 'quotes' '
++ cat >expect <<-\EOF &&
++ fileA.t
++ EOF
++
+ # shell takes \\\\101 and spits \\101
+ # printf takes \\101 and spits \101
+ # git takes \101 and spits A
-+ printf "\"file\\\\101.t\"" | test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
++ printf "\"file\\\\101.t\"" |
++ test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
+
-+ cat >expect <<-\EOF &&
-+ fileA.t
-+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success '--pathspec-file-nul takes quotes literally' '
-+ # shell takes \\\\101 and spits \\101
-+ # printf takes \\101 and spits \101
-+ printf "\"file\\\\101.t\"" | test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&
-+
+ cat >expect <<-\EOF &&
+ "file\101.t"
+ EOF
++
++ # shell takes \\\\101 and spits \\101
++ # printf takes \\101 and spits \101
++ printf "\"file\\\\101.t\"" |
++ test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&
++
+ test_cmp expect actual
+'
+
3: daef256db3 = 3: 88086cebce t: drop copy&pasted tests for --pathspec-from-file
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH 2/3] t: directly test parse_pathspec_file()
From: Junio C Hamano @ 2019-12-30 18:52 UTC (permalink / raw)
To: Alexandr Miloslavskiy via GitGitGadget; +Cc: git, Alexandr Miloslavskiy
In-Reply-To: <27383a5b084b5e68152b08eb96fb4ddaf6d87f82.1577727747.git.gitgitgadget@gmail.com>
"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> diff --git a/t/helper/test-parse-pathspec-file.c b/t/helper/test-parse-pathspec-file.c
> new file mode 100644
> index 0000000000..e7f525feb9
> --- /dev/null
> +++ b/t/helper/test-parse-pathspec-file.c
> @@ -0,0 +1,34 @@
> +#include "test-tool.h"
> +#include "parse-options.h"
> +#include "pathspec.h"
> +#include "gettext.h"
> + ...
> + parse_pathspec_file(&pathspec, 0, 0, 0, pathspec_from_file,
> + pathspec_file_nul);
> +
> + for (i = 0; i < pathspec.nr; i++) {
> + printf("%s\n", pathspec.items[i].original);
> + }
No need for {} around a single statement block.
> diff --git a/t/t0067-parse_pathspec_file.sh b/t/t0067-parse_pathspec_file.sh
> new file mode 100755
> index 0000000000..df7b319713
> --- /dev/null
> +++ b/t/t0067-parse_pathspec_file.sh
> @@ -0,0 +1,89 @@
> +#!/bin/sh
> +
> +test_description='Test parse_pathspec_file()'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'one item from stdin' '
> + echo fileA.t | test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
> +
> + cat >expect <<-\EOF &&
> + fileA.t
> + EOF
> + test_cmp expect actual
> +'
The use of the blank lines are somewhat inconsistent here.
> + ...
> +test_expect_success 'NUL delimiters' '
> + printf "fileA.t\0fileB.t\0" | test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&
Fold line immediately after the pipe (same for the earlier and later ones).
> + cat >expect <<-\EOF &&
> + fileA.t
> + fileB.t
> + EOF
> + test_cmp expect actual
> +'
If you want to have a gap between the steps, i.e. "capturing the
actual output", "creating the ideal output", and "seeing how they
differ", using blank like this is OK:
printf "fileA.t\0fileB.t\0" |
test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&
cat >expect <<-\EOF &&
fileA.t
fileB.t
EOF
test_cmp expect actual
I thought we typically prepare the ideal output sample before
capturing the actual output, so if we follow that convention, the
above becomes
cat >expect <<-\EOF &&
fileA.t
fileB.t
EOF
printf "fileA.t\0fileB.t\0" |
test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&
test_cmp expect actual
> +test_expect_success 'quotes' '
> + # shell takes \\\\101 and spits \\101
> + # printf takes \\101 and spits \101
> + # git takes \101 and spits A
> + printf "\"file\\\\101.t\"" | test-tool parse-pathspec-file --pathspec-from-file=- >actual &&
> +
> + cat >expect <<-\EOF &&
> + fileA.t
> + EOF
> + test_cmp expect actual
> +'
> +
> +test_expect_success '--pathspec-file-nul takes quotes literally' '
> + # shell takes \\\\101 and spits \\101
> + # printf takes \\101 and spits \101
> + printf "\"file\\\\101.t\"" | test-tool parse-pathspec-file --pathspec-from-file=- --pathspec-file-nul >actual &&
> +
> + cat >expect <<-\EOF &&
> + "file\101.t"
> + EOF
> + test_cmp expect actual
> +'
Testing low level machinery like this is of course a good idea, in
addition to the end-to-end tests that make sure that the machinery
is called correctly from the higher layer.
Thanks.
^ permalink raw reply
* Re: ERANGE strikes again on my Windows build; RFH
From: Jonathan Nieder @ 2019-12-30 18:49 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Git Mailing List
In-Reply-To: <54522fee-0796-df46-a3cf-4331537ecf59@kdbg.org>
Johannes Sixt wrote:
> Am 30.12.19 um 19:06 schrieb Jonathan Nieder:
>> when
>> errno is meaningful for a function for a given return value, the usual
>> convention is
>>
>> (1) it *always* sets errno for errors, not conditionally
>
> You seem to understand that errno isn't set somewhere where it should be
> set.
On the contrary: this caller is using errno as an error *indicator*
instead of a way of *distinguishing* between errors (or to put it
another way, this caller is treating `errno == 0` as a meaningful
condition). This means the calling code is buggy.
[...]
>> Do you have more details about the case where read_object is expected
>> to produce errno == 0? I'm wondering whether we forgot to set 'errno
>> = ENOENT' explicitly somewhere.
>
> I don't think that forgetting to set ENOENT is the problem.
>
> It happens reproducibly in test 5 of t0410-partial-clone:
Thanks, will try it out.
^ permalink raw reply
* Re: ERANGE strikes again on my Windows build; RFH
From: Johannes Sixt @ 2019-12-30 18:46 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git Mailing List
In-Reply-To: <20191230180653.GA57251@google.com>
Am 30.12.19 um 19:06 schrieb Jonathan Nieder:
> Johannes Sixt wrote:
>
>> In sha1-file.c:read_object_file_extended() we have the following pattern:
>>
>> errno = 0;
>> data = read_object(r, repl, type, size);
>> if (data)
>> return data;
>>
>> if (errno && errno != ENOENT)
>> die_errno(_("failed to read object %s"), oid_to_hex(oid));
>>
>> That is, it is expected that read_object() does not change the value of
>> errno in the non-error case. I find it intriguing that we expect a quite
>> large call graph that is behind read_object() to behave this way.
>
> Yes, this seems dubious.
>
> In fact this is only inspecting errno in the returned-NULL case. If I
> look only at the code above and not at the implementation of
> read_object, then I would say that the bug is the 'errno &&' part: when
> errno is meaningful for a function for a given return value, the usual
> convention is
>
> (1) it *always* sets errno for errors, not conditionally
You seem to understand that errno isn't set somewhere where it should be
set. But the problem is not absence of setting errno, but abundance of
setting errno; in particular, when functions receive errors from
lower-level functions, but then indicate to the higher levels that
everything's fine; then the higher levels observe errno when they shouldn't.
> (2) it never sets errno to 0
>
> There are some exceptions (like strtoul) but they are few and
> unfortunate, not something to be imitated.
>
> Do you have more details about the case where read_object is expected
> to produce errno == 0? I'm wondering whether we forgot to set 'errno
> = ENOENT' explicitly somewhere.
I don't think that forgetting to set ENOENT is the problem.
It happens reproducibly in test 5 of t0410-partial-clone:
++ git -C repo fsck
Checking object directories: 100% (256/256), done.
Checking objects: 100% (1/1), done.
fatal: failed to read object
383670739c2f993999f3c10911ac5cb5c6591523: Result too large
when it should be
Checking object directories: 100% (256/256), done.
Checking objects: 100% (1/1), done.
dangling tag e5f4cb9fd329c512b08fb81a8e6b1f5e27658263
-- Hannes
^ permalink raw reply
* [PATCH 2/2] checkout: don't revert file on ambiguous tracking branches
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-12-30 18:38 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð Bjarmason, Alexandr Miloslavskiy,
Junio C Hamano, Alexandr Miloslavskiy
In-Reply-To: <pull.504.git.1577731093.gitgitgadget@gmail.com>
From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
For easier understanding, here are the existing good scenarios:
1) Have *no* file 'foo', *no* local branch 'foo' and a *single*
remote branch 'foo'
2) `git checkout foo` will create local branch foo, see [1]
and
1) Have *a* file 'foo', *no* local branch 'foo' and a *single*
remote branch 'foo'
2) `git checkout foo` will complain, see [3]
This patch prevents the following scenario:
1) Have *a* file 'foo', *no* local branch 'foo' and *multiple*
remote branches 'foo'
2) `git checkout foo` will successfully... revert contents of
file `foo`!
That is, adding another remote suddenly changes behavior significantly,
which is a surprise at best and could go unnoticed by user at worst.
Please see [3] which gives some real world complaints.
To my understanding, fix in [3] overlooked the case of multiple remotes,
and the whole behavior of falling back to reverting file was never
intended:
[1] introduces the unexpected behavior. Before, there was fallback
from not-a-ref to pathspec. This is reasonable fallback. After, there
is another fallback from ambiguous-remote to pathspec. I understand
that it was a copy&paste oversight.
[2] noticed the unexpected behavior but chose to semi-document it
instead of forbidding, because the goal of the patch series was
focused on something else.
[3] adds `die()` when there is ambiguity between branch and file. The
case of multiple tracking branches is seemingly overlooked.
The new behavior: if there is no local branch and multiple remote
candidates, just die() and don't try reverting file whether it
exists (prevents surprise) or not (improves error message).
[1] Commit 70c9ac2f ("DWIM "git checkout frotz" to "git checkout -b frotz origin/frotz"" 2009-10-18)
https://public-inbox.org/git/7vaazpxha4.fsf_-_@alter.siamese.dyndns.org/
[2] Commit ad8d5104 ("checkout: add advice for ambiguous "checkout <branch>"", 2018-06-05)
https://public-inbox.org/git/20180502105452.17583-1-avarab@gmail.com/
[3] Commit be4908f1 ("checkout: disambiguate dwim tracking branches and local files", 2018-11-13)
https://public-inbox.org/git/20181110120707.25846-1-pclouds@gmail.com/
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
builtin/checkout.c | 56 ++++++++++++++++++----------------------
t/t2024-checkout-dwim.sh | 28 ++++++++++++++++++--
2 files changed, 51 insertions(+), 33 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f832040e94..5c41645c7d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1117,10 +1117,10 @@ static void setup_new_branch_info_and_source_tree(
static const char *parse_remote_branch(const char *arg,
struct object_id *rev,
- int could_be_checkout_paths,
- int *dwim_remotes_matched)
+ int could_be_checkout_paths)
{
- const char *remote = unique_tracking_name(arg, rev, dwim_remotes_matched);
+ int num_matches = 0;
+ const char *remote = unique_tracking_name(arg, rev, &num_matches);
if (remote && could_be_checkout_paths) {
die(_("'%s' could be both a local file and a tracking branch.\n"
@@ -1128,6 +1128,22 @@ static const char *parse_remote_branch(const char *arg,
arg);
}
+ if (!remote && num_matches > 1) {
+ if (advice_checkout_ambiguous_remote_branch_name) {
+ advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
+ "you can do so by fully qualifying the name with the --track option:\n"
+ "\n"
+ " git checkout --track origin/<name>\n"
+ "\n"
+ "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+ "one remote, e.g. the 'origin' remote, consider setting\n"
+ "checkout.defaultRemote=origin in your config."));
+ }
+
+ die(_("'%s' matched multiple (%d) remote tracking branches"),
+ arg, num_matches);
+ }
+
return remote;
}
@@ -1135,8 +1151,7 @@ static int parse_branchname_arg(int argc, const char **argv,
int dwim_new_local_branch_ok,
struct branch_info *new_branch_info,
struct checkout_opts *opts,
- struct object_id *rev,
- int *dwim_remotes_matched)
+ struct object_id *rev)
{
const char **new_branch = &opts->new_branch;
int argcount = 0;
@@ -1242,8 +1257,7 @@ static int parse_branchname_arg(int argc, const char **argv,
if (recover_with_dwim) {
const char *remote = parse_remote_branch(arg, rev,
- could_be_checkout_paths,
- dwim_remotes_matched);
+ could_be_checkout_paths);
if (remote) {
*new_branch = arg;
arg = remote;
@@ -1509,7 +1523,6 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
const char * const usagestr[])
{
struct branch_info new_branch_info;
- int dwim_remotes_matched = 0;
int parseopt_flags = 0;
memset(&new_branch_info, 0, sizeof(new_branch_info));
@@ -1617,8 +1630,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
opts->track == BRANCH_TRACK_UNSPECIFIED &&
!opts->new_branch;
int n = parse_branchname_arg(argc, argv, dwim_ok,
- &new_branch_info, opts, &rev,
- &dwim_remotes_matched);
+ &new_branch_info, opts, &rev);
argv += n;
argc -= n;
} else if (!opts->accept_ref && opts->from_treeish) {
@@ -1695,28 +1707,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
}
UNLEAK(opts);
- if (opts->patch_mode || opts->pathspec.nr) {
- int ret = checkout_paths(opts, new_branch_info.name);
- if (ret && dwim_remotes_matched > 1 &&
- advice_checkout_ambiguous_remote_branch_name)
- advise(_("'%s' matched more than one remote tracking branch.\n"
- "We found %d remotes with a reference that matched. So we fell back\n"
- "on trying to resolve the argument as a path, but failed there too!\n"
- "\n"
- "If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
- "you can do so by fully qualifying the name with the --track option:\n"
- "\n"
- " git checkout --track origin/<name>\n"
- "\n"
- "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
- "one remote, e.g. the 'origin' remote, consider setting\n"
- "checkout.defaultRemote=origin in your config."),
- argv[0],
- dwim_remotes_matched);
- return ret;
- } else {
+ if (opts->patch_mode || opts->pathspec.nr)
+ return checkout_paths(opts, new_branch_info.name);
+ else
return checkout_branch(opts, &new_branch_info);
- }
}
int cmd_checkout(int argc, const char **argv, const char *prefix)
diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
index fa0718c730..accfa9aa4b 100755
--- a/t/t2024-checkout-dwim.sh
+++ b/t/t2024-checkout-dwim.sh
@@ -37,7 +37,9 @@ test_expect_success 'setup' '
git checkout -b foo &&
test_commit a_foo &&
git checkout -b bar &&
- test_commit a_bar
+ test_commit a_bar &&
+ git checkout -b ambiguous_branch_and_file &&
+ test_commit a_ambiguous_branch_and_file
) &&
git init repo_b &&
(
@@ -46,7 +48,9 @@ test_expect_success 'setup' '
git checkout -b foo &&
test_commit b_foo &&
git checkout -b baz &&
- test_commit b_baz
+ test_commit b_baz &&
+ git checkout -b ambiguous_branch_and_file &&
+ test_commit b_ambiguous_branch_and_file
) &&
git remote add repo_a repo_a &&
git remote add repo_b repo_b &&
@@ -75,6 +79,26 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' '
test_branch master
'
+test_expect_success 'when arg matches multiple remotes, do not fallback to interpreting as pathspec' '
+ # create a file with name matching remote branch name
+ git checkout -b t_ambiguous_branch_and_file &&
+ >ambiguous_branch_and_file &&
+ git add ambiguous_branch_and_file &&
+ git commit -m "ambiguous_branch_and_file" &&
+
+ # modify file to verify that it will not be touched by checkout
+ test_when_finished "git checkout -- ambiguous_branch_and_file" &&
+ echo "file contents" >ambiguous_branch_and_file &&
+ cp ambiguous_branch_and_file expect &&
+
+ test_must_fail git checkout ambiguous_branch_and_file 2>err &&
+
+ test_i18ngrep "matched multiple (2) remote tracking branches" err &&
+
+ # file must not be altered
+ test_cmp expect ambiguous_branch_and_file
+'
+
test_expect_success 'checkout of branch from multiple remotes fails with advice' '
git checkout -B master &&
test_might_fail git branch -D foo &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/2] parse_branchname_arg(): extract part as new function
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-12-30 18:38 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð Bjarmason, Alexandr Miloslavskiy,
Junio C Hamano, Alexandr Miloslavskiy
In-Reply-To: <pull.504.git.1577731093.gitgitgadget@gmail.com>
From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
This is done for the next commit to avoid crazy 7x tab code padding.
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
builtin/checkout.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b52c490c8f..f832040e94 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1115,6 +1115,22 @@ static void setup_new_branch_info_and_source_tree(
}
}
+static const char *parse_remote_branch(const char *arg,
+ struct object_id *rev,
+ int could_be_checkout_paths,
+ int *dwim_remotes_matched)
+{
+ const char *remote = unique_tracking_name(arg, rev, dwim_remotes_matched);
+
+ if (remote && could_be_checkout_paths) {
+ die(_("'%s' could be both a local file and a tracking branch.\n"
+ "Please use -- (and optionally --no-guess) to disambiguate"),
+ arg);
+ }
+
+ return remote;
+}
+
static int parse_branchname_arg(int argc, const char **argv,
int dwim_new_local_branch_ok,
struct branch_info *new_branch_info,
@@ -1225,13 +1241,10 @@ static int parse_branchname_arg(int argc, const char **argv,
recover_with_dwim = 0;
if (recover_with_dwim) {
- const char *remote = unique_tracking_name(arg, rev,
- dwim_remotes_matched);
+ const char *remote = parse_remote_branch(arg, rev,
+ could_be_checkout_paths,
+ dwim_remotes_matched);
if (remote) {
- if (could_be_checkout_paths)
- die(_("'%s' could be both a local file and a tracking branch.\n"
- "Please use -- (and optionally --no-guess) to disambiguate"),
- arg);
*new_branch = arg;
arg = remote;
/* DWIMmed to create local branch, case (3).(b) */
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/2] checkout: don't revert file on ambiguous tracking branches
From: Alexandr Miloslavskiy via GitGitGadget @ 2019-12-30 18:38 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy,
Ævar Arnfjörð Bjarmason, Alexandr Miloslavskiy,
Junio C Hamano
This is an improved version of [1]; I tried to clarify the commit message.
CC'ing authors of previous commits mentioned in my commit message.
[1] https://public-inbox.org/git/pull.477.git.1574848137.gitgitgadget@gmail.com/T/#u
Alexandr Miloslavskiy (2):
parse_branchname_arg(): extract part as new function
checkout: don't revert file on ambiguous tracking branches
builtin/checkout.c | 71 ++++++++++++++++++++++------------------
t/t2024-checkout-dwim.sh | 28 ++++++++++++++--
2 files changed, 65 insertions(+), 34 deletions(-)
base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-504%2FSyntevoAlex%2F%230207(git)_2c_prevent_ambiguous_checkout-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-504/SyntevoAlex/#0207(git)_2c_prevent_ambiguous_checkout-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/504
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH] revision: allow missing promisor objects on CLI
From: Jonathan Tan @ 2019-12-30 18:38 UTC (permalink / raw)
To: gitster; +Cc: jonathantanmy, git, matvore
In-Reply-To: <xmqqlfqxhzvu.fsf@gitster-ct.c.googlers.com>
> Jonathan Tan <jonathantanmy@google.com> writes:
>
> > object = get_reference(revs, arg, &oid, flags ^ local_flags);
> > if (!object)
> > - return revs->ignore_missing ? 0 : -1;
> > + /*
> > + * Either this object is missing and ignore_missing is true, or
> > + * this object is a (missing) promisor object and
> > + * exclude_promisor_objects is true.
>
> I had to guess and dig where these assertions are coming from; we
> should not force future readers of the code to.
>
> At least this comment must say why these assertions hold. Say
> something like "get_reference() yields NULL on only such and such
> cases" before concluding with "and in any of these cases, we can
> safely ignore it because ...".
OK, will do.
> I think the two cases the comment covers are safe for this caller to
> silently return 0. Another case get_reference() yields NULL is when
> oid_object_info() says it is a commit but it turns out that the
> object is found by repo_parse_commit() to be a non-commit, isn't it?
> I am not sure if it is safe for this caller to just return 0. There
> may be some other "unusual-but-not-fatal" cases where get_reference()
> does not hit a die() but returns NULL.
I don't think there is any other case where get_reference() yields NULL,
at least where I based my patch (99c33bed56 ("Git 2.25-rc0",
2019-12-25)). Quoting the entire get_reference():
> static struct object *get_reference(struct rev_info *revs, const char *name,
> const struct object_id *oid,
> unsigned int flags)
> {
> struct object *object;
>
> /*
> * If the repository has commit graphs, repo_parse_commit() avoids
> * reading the object buffer, so use it whenever possible.
> */
> if (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT) {
> struct commit *c = lookup_commit(revs->repo, oid);
> if (!repo_parse_commit(revs->repo, c))
> object = (struct object *) c;
> else
> object = NULL;
> } else {
> object = parse_object(revs->repo, oid);
> }
No return statements at all prior to this line.
> if (!object) {
> if (revs->ignore_missing)
> return object;
Return NULL (the value of object).
> if (revs->exclude_promisor_objects && is_promisor_object(oid))
> return NULL;
Return NULL.
> die("bad object %s", name);
Die (so this function invocation never returns). In conclusion, if
object is NULL at this point in time, get_reference() either returns
NULL or dies.
> }
Since get_reference() did not return NULL or die, object is non-NULL
here.
> object->flags |= flags;
> return object;
Nothing has overridden object since, so we're returning non-NULL here.
> }
So I think get_reference() only returns NULL in those two safe cases.
(Or did I miss something?)
^ 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