* Re: Strange behaviour when pushing a commit object to remote's refs/HEAD
From: Pratyush Yadav @ 2024-01-16 11:33 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Pratyush Yadav, git
In-Reply-To: <CAOLa=ZS8YBhzaYx=9016KxErsMsazsF09rcuPs=-WpEGjV+ruw@mail.gmail.com>
On Tue, Jan 16 2024, Karthik Nayak wrote:
> Pratyush Yadav <me@yadavpratyush.com> writes:
>
>> Hi,
>>
>
> Hello,
>
>> I ran into a strange Magit bug, where when I ran magit-show-refs on a
>> particular repo it threw an error. The details of the Magit bug are not
>> very interesting, but when attempting to reproduce it, I also saw git
>> misbehaving for such repos.
>>
>> The strange behaviour happens when you push a commit object to remote's
>> refs/HEAD instead of pushing a symbolic ref. Such a repository can be
>> found at https://github.com/prati0100/magit-reproducer. I roughly used
>> the below steps to create such a repo:
>>
>> $ git init
>> $ echo 1 > foo && git add foo && git commit
>> $ echo 2 > bar && git add bar && git commit
>> $ git push
>> $ git checkout 79264c3
>> $ echo 2.1 > bar && git add bar && git commit
>> $ git push origin 707a3d5:refs/heads/HEAD
>>
>
> Just to note here that pushing to "refs/heads/HEAD" is not actually
> updating the remote repositories $GIT_DIR/HEAD file, rather it creates a
> new reference $GIT_DIR/refs/heads/HEAD.
Yes, that is what I would also expect. I checked one of the Git servers
we have and this is exactly what happens. $GIT_DIR/HEAD is a symref
pointing to refs/heads/main and $GIT_DIR/refs/heads/HEAD points to the
commit. But behaviour from client side is not consistent.
>
> With this understanding you'll see that this is not a bug, because the
> remote HEAD was never updated, but only a new branch called HEAD was
> created [0].
GitHub thinks so but try opening the branch. It won't show you the
commit (707a3d5, "2.1") but instead shows you 86e1c97 ("2"). So
something is wrong _at least_ with Github.
>
>> Now with such a repo, if you do `git log --all --oneline` it would look
>> something like:
>>
>> 707a3d5 (origin/HEAD) 2.1
>> 86e1c97 (HEAD -> main, origin/main) 2
>> 79264c3 1
>>
>> And running `git for-each-ref --format='%(symref:short),%(refname:short),%(refname),%(subject)' refs/remotes/origin` gives:
>>
>> ,origin,refs/remotes/origin/HEAD,2.1
>> ,origin/main,refs/remotes/origin/main,2
>>
>> All well and good so far. Now delete the repo and attempt to clone it.
>> This time `git log --all --oneline` gives:
>>
>> 86e1c97 (HEAD -> main, origin/main, origin/HEAD) 2
>> 79264c3 1
>>
>
> This is expected since you cloned the repository and you got the default
> branch 'main'.
No.
First, if I clone a repo with multiple branches (say
https://github.com/prati0100/git-gui) I get _all_ the remote branches.
Yet here I clearly don't get the so called "HEAD" branch. This is not
expected behaviour.
Second, git really does misunderstand refs/remotes/origin/HEAD. For
example, when running git for-each-ref command with the clone method, I
get:
origin/main,origin,refs/remotes/origin/HEAD,2
So it clearly thinks refs/remotes/origin/HEAD is at 86e1c97 ("2"). Or,
to be more specific, it thinks the ref points to origin/main which is at
86e1c97 ("2"). But we set it at (707a3d5, "2.1"). So it tells me the
wrong thing. Now if I do the git remote add && git remote update method,
git for-each-ref says:
,origin,refs/remotes/origin/HEAD,2.1
So now it thinks refs/remotes/origin/HEAD points at (707a3d5, "2.1"). I
do not see it as expected behaviour.
We can also see this when inspecting the contents of
.git/refs/remotes/origin/HEAD. With clone it says:
ref: refs/remotes/origin/main
With git remote add && git remote update it says:
707a3d587c61c089710e3924eb63a51763b5a4c8
The same ref points to different places based on how you pull the repo.
Looking deeper, if you clone a repo that does not have a branch called
"HEAD" (like git-gui), git creates a file in
.git/refs/remotes/origin/HEAD that says:
ref: refs/remotes/origin/master
So it certainly seems to use refs/remotes/origin/HEAD to point to the
remote's HEAD, and not as a regular branch.
I find this to be inconsistent behaviour on git's part and do not think
it is (or should be) expected behaviour.
>
>> And running `git for-each-ref --format='%(symref:short),%(refname:short),%(refname),%(subject)' refs/remotes/origin` gives:
>>
>> origin/main,origin,refs/remotes/origin/HEAD,2
>> ,origin/main,refs/remotes/origin/main,2
>>
>> So suddenly the remote's HEAD becomes origin/main (symbolic ref) and the
>> commit (707a3d5, "2.1") is nowhere to be found. It neither shows up in
>> `git rev-list --all` nor in `git log --all`. The files and trees
>> associated with it also do not show up in `git rev-list --all --object`.
>
>
> Because rev-list's `--all`, iterates over all refs. Since you only
> cloned, the HEAD branch is not pulled.
Why not? When you clone all branches should get pulled.
>
> Everything else is a consequence of the subtle but important difference
> between updating $GIT_DIR/HEAD vs creating $GIT_DIR/refs/heads/HEAD.
>
> [0]: https://github.com/prati0100/magit-reproducer/branches/all
--
Regards,
Pratyush Yadav
^ permalink raw reply
* Re: [Outreachy][PATCH] Port helper/test-advise.c to unit-tests/t-advise.c
From: Rubén Justo @ 2024-01-16 11:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Achu Luma, git, christian.couder, Christian Couder
In-Reply-To: <6b6b455e-26b8-442e-828e-506f9a152407@gmail.com>
On 15-ene-2024 20:24:47, Rubén Justo wrote:
> - check_str(actual.buf, expect);
> + check_str_len(actual.buf, expect, strlen(expect));
> + if (!conf_val && skip_prefix(actual.buf, expect, &hint))
> + check_str_len(hint, expect_hint_msg, strlen(expect_hint_msg));
> strbuf_release(&actual);
>
> if (!check(remove(out_file) == 0))
>
> This implies a new check_str_len() helper, which I'm not including here
> but it's a trivial copy of check_str() but using strncmp().
>
> Maybe we can turn the screw a little more.
>
> I'm still not sure of the value in the changes in this series, though.
I hope, no one has wasted time with the code above. It is not testing
correctly the conditions being probed in t-advice.c.
Take it as a way to see if this is how we want to avoid multiple
instances of similar literals that might be tempting to refactor.
^ permalink raw reply
* Re: Strange behaviour when pushing a commit object to remote's refs/HEAD
From: Karthik Nayak @ 2024-01-16 9:54 UTC (permalink / raw)
To: Pratyush Yadav, git
In-Reply-To: <mafs0fryypg82.fsf@yadavpratyush.com>
[-- Attachment #1: Type: text/plain, Size: 2720 bytes --]
Pratyush Yadav <me@yadavpratyush.com> writes:
> Hi,
>
Hello,
> I ran into a strange Magit bug, where when I ran magit-show-refs on a
> particular repo it threw an error. The details of the Magit bug are not
> very interesting, but when attempting to reproduce it, I also saw git
> misbehaving for such repos.
>
> The strange behaviour happens when you push a commit object to remote's
> refs/HEAD instead of pushing a symbolic ref. Such a repository can be
> found at https://github.com/prati0100/magit-reproducer. I roughly used
> the below steps to create such a repo:
>
> $ git init
> $ echo 1 > foo && git add foo && git commit
> $ echo 2 > bar && git add bar && git commit
> $ git push
> $ git checkout 79264c3
> $ echo 2.1 > bar && git add bar && git commit
> $ git push origin 707a3d5:refs/heads/HEAD
>
Just to note here that pushing to "refs/heads/HEAD" is not actually
updating the remote repositories $GIT_DIR/HEAD file, rather it creates a
new reference $GIT_DIR/refs/heads/HEAD.
With this understanding you'll see that this is not a bug, because the
remote HEAD was never updated, but only a new branch called HEAD was
created [0].
> Now with such a repo, if you do `git log --all --oneline` it would look
> something like:
>
> 707a3d5 (origin/HEAD) 2.1
> 86e1c97 (HEAD -> main, origin/main) 2
> 79264c3 1
>
> And running `git for-each-ref --format='%(symref:short),%(refname:short),%(refname),%(subject)' refs/remotes/origin` gives:
>
> ,origin,refs/remotes/origin/HEAD,2.1
> ,origin/main,refs/remotes/origin/main,2
>
> All well and good so far. Now delete the repo and attempt to clone it.
> This time `git log --all --oneline` gives:
>
> 86e1c97 (HEAD -> main, origin/main, origin/HEAD) 2
> 79264c3 1
>
This is expected since you cloned the repository and you got the default
branch 'main'.
> And running `git for-each-ref --format='%(symref:short),%(refname:short),%(refname),%(subject)' refs/remotes/origin` gives:
>
> origin/main,origin,refs/remotes/origin/HEAD,2
> ,origin/main,refs/remotes/origin/main,2
>
> So suddenly the remote's HEAD becomes origin/main (symbolic ref) and the
> commit (707a3d5, "2.1") is nowhere to be found. It neither shows up in
> `git rev-list --all` nor in `git log --all`. The files and trees
> associated with it also do not show up in `git rev-list --all --object`.
Because rev-list's `--all`, iterates over all refs. Since you only
cloned, the HEAD branch is not pulled.
Everything else is a consequence of the subtle but important difference
between updating $GIT_DIR/HEAD vs creating $GIT_DIR/refs/heads/HEAD.
[0]: https://github.com/prati0100/magit-reproducer/branches/all
Thanks,
Karthik
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v4 3/4] transport-helper: call do_take_over() in connect_helper
From: Jiang Xin @ 2024-01-16 9:41 UTC (permalink / raw)
To: Linus Arver; +Cc: Git List, Junio C Hamano, Jiang Xin
In-Reply-To: <owlyttnjhtmz.fsf@fine.c.googlers.com>
On Fri, Jan 12, 2024 at 3:56 PM Linus Arver <linusa@google.com> wrote:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > After successfully connecting to the smart transport by calling
> > process_connect_service() in connect_helper(), run do_take_over() to
> > replace the old vtable with a new one which has methods ready for the
> > smart transport connection.
> >
> >
> > The connect_helper() function is used as the connect method of the
> > vtable in "transport-helper.c", and it is called by transport_connect()
> > in "transport.c" to setup a connection. The only place that we call
> > transport_connect() so far is in "builtin/archive.c". Without running
> > do_take_over(), it may fail to call transport_disconnect() in
> > run_remote_archiver() of "builtin/archive.c". This is because for a
> > stateless connection or a service like "git-upload-pack-archive", the
>
> There is "git-upload-pack" and "git-upload-archive". Which one did you
> mean here? Or did you mean both?
>
Should be "git-upload-archive".
> > remote helper may receive a SIGPIPE signal and exit early. To have a
> > graceful disconnect method by calling do_take_over() will solve this
> > issue.
>
> Are you saying that this patch fixes an existing bug? That is, is this
> patch independent of the first patch (transport-helper: no connection
> restriction in connect_helper) in this series?
>
> > The subsequent commit will introduce remote archive over a stateless-rpc
> > connection.
>
> Does the next commit depend on this patch? If not, I think you can drop
> this paragraph.
One test case in next commit will break without this patch. I will
move this patch to the end of this series.
^ permalink raw reply
* Re: [PATCH v2 0/6] worktree: initialize refdb via ref backends
From: Karthik Nayak @ 2024-01-16 9:17 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Junio C Hamano, Eric Sunshine
In-Reply-To: <cover.1704705733.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 543 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> Hi,
>
> this is the second version of my patch series that refactors the
> initialization of worktree refdbs to use the refs API.
>
> Changes compared to v1:
>
> - Improved commit messages.
>
> - This series is now based on `ps/refstorage-extension`, 1b2234079b
> (t9500: write "extensions.refstorage" into config, 2023-12-29).
> While there is no functional dependency between those series,
> merging both topics causes a merge conflict.
>
This looks good to me now. Thanks Patrick!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v4 1/4] transport-helper: no connection restriction in connect_helper
From: Jiang Xin @ 2024-01-16 9:04 UTC (permalink / raw)
To: Linus Arver; +Cc: Git List, Junio C Hamano, Jiang Xin
In-Reply-To: <owlyy1cvhua5.fsf@fine.c.googlers.com>
On Fri, Jan 12, 2024 at 3:42 PM Linus Arver <linusa@google.com> wrote:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > When commit b236752a (Support remote archive from all smart transports,
> > 2009-12-09) added "remote archive" support for "smart transports", it
> > was for transport that supports the ".connect" method. The
> > "connect_helper()" function protected itself from getting called for a
> > transport without the method before calling process_connect_service(),
>
> OK.
>
> > which did not work with such a transport.
>
> How about 'which only worked with the ".connect" method.' ?
>
> >
> > Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
> > 2018-03-15) added a way for a transport without the ".connect" method
> > to establish a "stateless" connection in protocol-v2, which
>
> s/which/where
>
> > process_connect_service() was taught to handle the "stateless"
> > connection,
>
> I think using 'the ".stateless_connect" method' is more consistent with
> the rest of this text.
>
> > making the old safety valve in its caller that insisted
> > that ".connect" method must be defined too strict, and forgot to loosen
> > it.
>
> I think just "...making the old protection too strict. But edc9caf7
> forgot to adjust this protection accordingly." is simpler to read.
Thanks for the above suggestions, and will update in next reroll.
> > Remove the restriction in the "connect_helper()" function and give the
> > function "process_connect_service()" the opportunity to establish a
> > connection using ".connect" or ".stateless_connect" for protocol v2. So
> > we can connect with a stateless-rpc and do something useful. E.g., in a
> > later commit, implements remote archive for a repository over HTTP
> > protocol.
> >
> > Helped-by: Junio C Hamano <gitster@pobox.com>
> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> > ---
> > transport-helper.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/transport-helper.c b/transport-helper.c
> > index 49811ef176..2e127d24a5 100644
> > --- a/transport-helper.c
> > +++ b/transport-helper.c
> > @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
> >
> > /* Get_helper so connect is inited. */
> > get_helper(transport);
> > - if (!data->connect)
> > - die(_("operation not supported by protocol"));
>
> Should we still terminate early here if both data->connect and
> data->stateless_connect are not truthy? It would save a few CPU cycles,
> but even better, remain true to the the original intent of the code.
> Maybe there was a really good reason to terminate early here that we're
> not aware of?
>
It's not necessary to check data->connect here, because it will
terminate if fail to connect by calling the function
"process_connect_service()".
> But also, what about the case where both are enabled? Should we print an
> error message? (Maybe this concern is outside the scope of this series?)
In the function "process_connect_service()", we can see that "connect"
has a higher priority than "stateless-connect".
>
> > if (!process_connect_service(transport, name, exec))
> > die(_("can't connect to subservice %s"), name);
Regardless of whether "connect" or "stateless-connect" is used, the
function process_connect_service() will return 1 if the connection is
successful. If the connection fails, it will terminate here.
^ permalink raw reply
* Custom merge drivers: accessing the pathnames and revisions of the files being merged
From: Antonin Delpeuch @ 2024-01-16 8:44 UTC (permalink / raw)
To: git
Hello,
Custom merge drivers [1] provide a convenient way to extend Git's merge
algorithms. But as far as I can tell, there is no way to access the
pathnames and revisions of the files to merge. Those would be very
useful to generate informative conflict markers, like the built-in merge
strategies do.
For instance, when merging with the ort strategy and diff3 conflict
style, I get markers such as:
<<<<<<< HEAD:main/my_file.txt
…
||||||| 194c4190b:main/my_file.txt
…
=======
…
>>>>>>> origin/branch:main/my_renamed_file.txt
Those strings at the end of conflict markers, with the revision and the
pathname, are very useful to understand which parts are coming from where.
When implementing a custom merge driver, I don't see how to access this
information to include it in the conflict markers. Custom merge drivers
are typically invoked on temporary files generated by Git with
uninformative paths, such as ".merge_file_NgiKjJ".
Therefore, my merge driver is only able to generate conflicts which look
like this:
<<<<<<< .merge_file_NgiKjJ
…
||||||| .merge_file_D1XtCW
…
=======
…
>>>>>>> .merge_file_WbmrBA
Of course, in a given rebase/merge session, the order in which the
conflicting parts will be presented will remain consistent, so I will
generally be able to remember which revision each part corresponds to,
but it's still a mental load I would ideally try to avoid. Also, if the
rename detection heuristics have false positives, then I can get merge
conflicts which come from unrelated files: in that case it is very
useful to see the pathnames, to understand this situation better.
So, I wonder: would people be open to exposing additional parameters to
merge drivers? For instance we could add parameters "%X", "%Y" "%Z" to
expose those "revision:pathname" strings for each part. (I think colons
cannot be part of revision names, so this can be parsed unambiguously by
the custom merge driver to recover the revision and pathname
independently, if needed.)
I would be happy to submit a patch for this if you think this makes
sense. If it is already possible to access this information in another
way, I would like to work on documenting that.
Best wishes,
Antonin
[1]: https://git-scm.com/docs/gitattributes#_defining_a_custom_merge_driver
^ permalink raw reply
* Re: [PATCH] advice: allow disabling the automatic hint in advise_if_enabled()
From: Junio C Hamano @ 2024-01-16 4:56 UTC (permalink / raw)
To: Jeff King; +Cc: Rubén Justo, Git List, Taylor Blau, Dragan Simic
In-Reply-To: <20240113073828.GB657764@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> For a tri-state we often use "-1" for "not specified". That has the nice
> side effect in this case that "if (level)" shows the advice (that works
> because "unspecified" and "explicitly true" both show the advice. And
> then "if (level < 0)" is used for just the hint. But maybe that is too
> clever/fragile.
>
> Of course that means that all of the initializers have to use "-1"
> explicitly. So zero-initialization is sometimes nice, too.
;-) 100% agreed.
^ permalink raw reply
* Re: Suggested clarification for .gitattributes reference documentation
From: brian m. carlson @ 2024-01-16 2:06 UTC (permalink / raw)
To: Michael Litwak
Cc: Torsten Bögershausen, Matthias Aßhauer,
git@vger.kernel.org
In-Reply-To: <SJ0PR10MB5693A19B0B66F47B2A985739FA732@SJ0PR10MB5693.namprd10.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 6194 bytes --]
On 2024-01-16 at 00:19:20, Michael Litwak wrote:
> As for documentation clarifications for the .gitattributes manpage at
> https://git-scm.com/docs/gitattributes, I still suggest adding an
> explicit example for UTF-16LE with BOM, and/or adding a table listing
> which working-tree-encoding value to use for each of the following
> UTF-16 text encodings:
>
> ENCODING 'working-tree-encoding' VALUE
> ------------------- -----------------------------
> UTF-16LE with BOM UTF-16LE-BOM
I should point out that this encoding, while very common on Windows, is
also nonstandard. The standard says that UTF-16LE and UTF-16BE don't
include a BOM and are always the respective endianness. UTF-16 can have
a BOM or not, and if it doesn't, it's big-endian.
There is no standard-conforming way to force the use of little-endian
with a BOM. The problem is that many Windows programs insist on the
BOM, but also refuse to read big-endian data in violation of the
standard[0]. That's why this nonstandard variant exists in Git.
I'll also note that this particular nonstandard variant is essentially
impossible to encode reliably on Unix outside of Git because it's
nonstandard, so it's an extremely unportable choice. In fact, I'm not
aware of _any_ tool on my Debian system other than Git that will
guarantee a UTF-16 little-endian stream with BOM. My editor (Neovim)
certainly doesn't. (Apparently Emacs, which is not on my system, may
permit that, which does not surprise me in the least.)
> UTF-16BE with BOM UTF-16
It's a little more complicated than that. "UTF-16" would allow UTF-16
big-endian with BOM, UTF-16 little-endian with BOM, or UTF-16 big-endian
without BOM. In other words, UTF-16 is big-endian by default and
otherwise requires a BOM, which may be included even if not required.
A reader must handle every variant of this, and must honour the BOM if
set and default to big-endian if not. A writer may write whichever
variant pleases it most as long as it's consistent within the same
message.
> UTF-16LE no BOM UTF-16LE
> UTF-16BE no BOM UTF-16BE
I think the addition of this table is too much. UTF-16LE-BOM is common
on Windows, and the rest are substantially less common. It's also very
difficult to explain in a table what "UTF-16" means in an understandable
way. And I also think it's also pretty clear that users should be using
UTF-8 without BOM where possible.
We do already mention both UTF-16, UTF-16LE, and UTF-16LE-BOM as options
in the gitattributes manual page, and it's up to the user to know what
their program wants and supports if that's not UTF-8. (I would say that
the user wants a new program that _does_ support UTF-8, but perhaps I'm
being unrealistically harsh.) I agree it's difficult because the
documentation usually doesn't indicate what's supported and all the
variants are hard to understand, but that's a huge part of the reason
that we recommend UTF-8.
I'll also add that in general, when you do have Unix systems that read
or write data in UTF-16, they handle every variant correctly. Thus, the
practical choice if you steadfastly refuse to use UTF-8 is either
UTF-16LE-BOM (if your Windows program has the bug I mentioned above) or
UTF-16, both of which we mention already in the manual page.
I'm explicitly ignoring non-file contexts here, where one may use
UTF-16LE or UTF-16BE, but those are substantially less common in actual
files, which is what this feature describes.
> Why bother clarifying the documentation? Because These UTF-16
> encodings are commonly found on Windows systems. Notepad supports the
> first two, and many Visual Studio project wizards add various files
> using these encodings as well. Older versions of PowerShell saved new
> .ps1 scripts using UTF-16BE with BOM as the default encoding.
True, but Notepad also supports UTF-8 and has for quite a while.
According to the Powershell documentation[1], there is no portable
character set option for non-ASCII characters, so in general it's
impossible to know. I suspect that a simple "UTF-16" will be fine here,
though, since it clearly doesn't have the bug mentioned above.
> Also, the current .gitattributes documentation makes frequent
> reference to "UTF-16" as an encoding but fails to be clear that the
> working-tree-encoding value "UTF-16" is now only for UTF-16BE with
> BOM. It would be easy to assume that the working-tree-encoding value
> "UTF-16" meant any UTF-16 file with a BOM (either LE or BE), which was
> the original meaning of this value before UTF-16LE-BOM was added to
> Git.
As I said, your statement isn't correct. That's what libiconv does on
Windows. On Linux, glibc uses a little-endian variant with BOM on
little-endian machines. musl, if memory serves me, always uses
big-endian without a BOM. All of those are valid encodings, and a
UTF-16 reader must handle all of them.
> Finally, I am not sure how to use git add --renormalize to correct a
> UTF-16 file that was previously added incorrectly (i.e. with a missing
> or incorrect working-tree-encoding entry in .gitattributes). The git
> add documentation at https://git-scm.com/docs/git-add implies
> 'renormalize' resets only the end-of-line values; however, I suspect
> it also re-converts text encoding when a working-tree-encoding
> property is set. It would be helpful to know one way or the other.
It does indeed affect the working-tree-encoding. If you wanted to send
an inline patch created with git format-patch, it would probably be
welcome to mention that. However, because in this project we typically
scratch our own itch, if you don't send one, it's likely nobody else
will, either.
[0] https://datatracker.ietf.org/doc/html/rfc2781 § 4.1: “All
applications that process text with the "UTF-16" charset label
MUST be able to interpret both big- endian and little-endian text.”
[1] https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_character_encoding?view=powershell-7.4
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply
* Re: Suggested clarification for .gitattributes reference documentation
From: Michael Litwak @ 2024-01-16 0:19 UTC (permalink / raw)
To: Torsten Bögershausen, Matthias Aßhauer
Cc: brian m. carlson, git@vger.kernel.org
In-Reply-To: <20240113074323.GA6819@tb-raspi4>
> To my knowledge the binary iconv.exe (or just iconv under non-Windows) is never called from Git itself.
> I can't find a single instance of Git for Windows calling iconv.exe instead of using the corresponding library functions.
Thank you for your responses. I think you are both right. Git must instead call methods in libiconv-2.dll to do encoding conversions.
I have no idea why my Windows 10 PC could add a UTF-16LE with BOM file, but then fail to later successfully "decode" it, when running Git from an ordinary Command Prompt (cmd.exe). I assume this failure was a fluke, since I cannot replicate the failure on my other (Windows 11) PC. So I am withdrawing my concerns about:
1) Git for Windows failing to support UTF-16LE with BOM.
2) Git for Windows installer being misleading in its "recommended" PATH modification option.
As for documentation clarifications for the .gitattributes manpage at https://git-scm.com/docs/gitattributes, I still suggest adding an explicit example for UTF-16LE with BOM, and/or adding a table listing which working-tree-encoding value to use for each of the following UTF-16 text encodings:
ENCODING 'working-tree-encoding' VALUE
------------------- -----------------------------
UTF-16LE with BOM UTF-16LE-BOM
UTF-16BE with BOM UTF-16
UTF-16LE no BOM UTF-16LE
UTF-16BE no BOM UTF-16BE
Why bother clarifying the documentation? Because These UTF-16 encodings are commonly found on Windows systems. Notepad supports the first two, and many Visual Studio project wizards add various files using these encodings as well. Older versions of PowerShell saved new .ps1 scripts using UTF-16BE with BOM as the default encoding.
Also, the current .gitattributes documentation makes frequent reference to "UTF-16" as an encoding but fails to be clear that the working-tree-encoding value "UTF-16" is now only for UTF-16BE with BOM. It would be easy to assume that the working-tree-encoding value "UTF-16" meant any UTF-16 file with a BOM (either LE or BE), which was the original meaning of this value before UTF-16LE-BOM was added to Git.
Finally, I am not sure how to use git add --renormalize to correct a UTF-16 file that was previously added incorrectly (i.e. with a missing or incorrect working-tree-encoding entry in .gitattributes). The git add documentation at https://git-scm.com/docs/git-add implies 'renormalize' resets only the end-of-line values; however, I suspect it also re-converts text encoding when a working-tree-encoding property is set. It would be helpful to know one way or the other.
- Michael Litwak
-----Original Message-----
From: Torsten Bögershausen <tboegi@web.de>
Sent: Friday, January 12, 2024 11:43 PM
To: Michael Litwak <michael.litwak@nuix.com>
Cc: brian m. carlson <sandals@crustytoothpaste.net>; git@vger.kernel.org
Subject: [EXTERNAL]Re: Suggested clarification for .gitattributes reference documentation
[You don't often get email from tboegi@web.de. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
On Sat, Jan 13, 2024 at 02:56:27AM +0000, Michael Litwak wrote:
> I just installed Git for Windows 2.43.0 and noticed the installer offers three options for altering the PATH:
>
> 1) Run git from git bash only
>
> 2) Run git from git bash, cmd.exe and PowerShell (RECOMMENDED)
>
> 3) Run git from git bash, cmd.exe and PowerShell with optional utilities (warning: will override find, sort and other system utilities).
>
> It turns out iconv.exe is accessible from cmd.exe (Command Prompt) only when you take the third option. But iconv.exe is NOT optional. It is required for git to deal with UTF-16LE with BOM text conversions (and probably for numerous other encoding conversions).
Plese wait a second - and thanks for bringing this up.
To my knowledge the binary iconv.exe (or just iconv under non-Windows) is never called from Git itself.
Git is using iconv_open() and friends, which are all inside a library, either the C-library "libc", or "libiconv"
(not 100% sure about the naming here)
iconv.exe is not needed in everyday life, or is it ?
If yes, when ?
iconv.exe is used when you run the test-suite, to verify what Git is doing.
Could you elaborate a little bit more,
when iconv.exe is missing, and what is happening, please ?
>
> But when PATH option #2 is chosen, and iconv.exe is unreachable from a Windows Command Prompt, the git commands which call upon iconv.exe do NOT indicate the error. The call to iconv.exe fails silently. It is only later after you commit, push and clone the repo again that you see the encoding failures.
>
> And the warning about overriding find and sort must be taken with a grain of salt, since the Windows versions of those programs are accessed via a Windows folder which appears earlier in the PATH.
>
> So this Git for Windows installer screen is misleading. And perhaps iconv.exe should be relocated so it is accessible even when PATH option #2 is chosen. I intend to submit an issue on the Git for Windows issue tracker regarding this. I'll also submit an issue about the lack of an error when running 'git add' for a UTF-16LE with BOM file under PATH option #2.
>
> Thanks,
> - Michael
>
[]
CAUTION:This email originated from outside of Nuix. Do not click links or open attachments unless you recognise the sender and know the content is safe.
^ permalink raw reply
* Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper
From: Linus Arver @ 2024-01-15 22:53 UTC (permalink / raw)
To: Taylor Blau, git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <owly8r4qi5zj.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> So in summary there appear to be the following possibilities:
>
> CHUNK_MISSING
> CHUNK_TOO_SMALL
> CHUNK_OK
> CHUNK_TOO_BIG_ALIGNED
> CHUNK_TOO_BIG_MISALIGNED
On second thought, it appears that CHUNK_TOO_SMALL has three cases:
(1) chunk_size < record_size
(2) chunk_size >= record_size, but chunk_size < record_size * record_nr
and decreasing record_nr can make chunk_size "fit"
(3) chunk_size >= record_size, but chunk_size < record_size * record_nr
and decreasing record_nr cannot make chunk_size "fit"
where (2) and (3) are just like the *_(MIS)ALIGNED cases above when
chunk_size is too big.
My default position is that these additional cases might need to be
treated differently, although maybe it's overkill also.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper
From: Linus Arver @ 2024-01-15 22:31 UTC (permalink / raw)
To: Taylor Blau, git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <af5fe3b7237caeba8f970e967933db96c83a230e.1699569246.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
> +static int pair_chunk_expect_fn(const unsigned char *chunk_start,
> + size_t chunk_size,
> + void *data)
> +{
> + struct pair_chunk_data *pcd = data;
> + if (chunk_size / pcd->record_size != pcd->record_nr)
> + return -1;
> + *pcd->p = chunk_start;
> + return 0;
> +}
> +
I don't think this function should assume anything about the inputs
(chunk_size, record_size, nor record_nr). If we are asking the helper to
be the common tool for multiple locations, it should assume even less
about what these inputs could look like.
For example, if record_size is 0 this will lead to a
divide-by-zero. Likewise, if record_nr is zero it doesn't make
sense to check if chunk_size fits into record_size * record_nr.
And even if there are no (unexpected) zero-values involved, shouldn't we
also check for nonsensical comparisons, such as if chunk_size is smaller
than record_size?
I think there are several possibilities:
CHUNK_MISSING (chunk_size == 0)
CHUNK_TOO_SMALL (chunk_size < record_size)
CHUNK_OK (chunk_size == record_nr * record_size)
CHUNK_TOO_BIG (chunk_size > record_size, record_nr * record_size < chunk_size)
And for the CHUNK_TOO_BIG case there are two possibilities --- the
leftover parts of chunk_size that are not accounted by "record_nr *
record_size" are (or are not) "aligned" such that increasing the
record_size by some positive increment could exactly match the
chunk_size. For example, chunk_size = 128, record_size = 8, record_nr =
8. In this case the records account for 64 bytes so we have 128 - 64 =
64 bytes left over, and simply increasing record_nr to 16 could account
for every bytes in chunk_size. If chunk_size in this example was 120 or
130, there would be bytes left over that cannot be accounted for by
adjusting record_size. This divisibility-of-leftover-bytes check could
be done with '%' as mentioned already by others.
So in summary there appear to be the following possibilities:
CHUNK_MISSING
CHUNK_TOO_SMALL
CHUNK_OK
CHUNK_TOO_BIG_ALIGNED
CHUNK_TOO_BIG_MISALIGNED
(on top of the cases where record_* inputs are zero).
And it seems prudent to treat each of the not-OK cases differently
(including how we report error messages) once we know which category we
fall into.
^ permalink raw reply
* git-range-diff ignoring commit message changes
From: Gwyneth Morgan @ 2024-01-15 19:40 UTC (permalink / raw)
To: git
Is there a way to make git-range-diff to ignore commit messages when
considering if commits are identical? When range-diffing long series
there are cases where I would like to check at a glance whether the code
has changed, and only when the code has changed do I want to see the
change in commit message too.
It seems I can approximate what I want by tweaking range-diff's source
like this but I couldn't tell if there was an actual option:
diff --git a/range-diff.c b/range-diff.c
index c45b6d849c..fd421b7b99 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -59,7 +59,7 @@ static int read_patches(const char *range, struct string_list *list,
"--output-indicator-old=<",
"--output-indicator-context=#",
"--no-abbrev-commit",
- "--pretty=medium",
+ "--pretty=format:commit %H",
"--show-notes-by-default",
NULL);
strvec_push(&cp.args, range);
This ignores commit message changes entirely, but it would be nice to
have an option to only see commit message changes when the code diff has
changed. It would also be convenient to have a way to only consider
changes in the title of commits but ignore the message body (equivalent
to "--pretty=short" above).
You could get some of this information with git-cherry, but that is
suited for different uses (only cares about new commits on one side,
doesn't show diffs) and would take more effort than just ignoring the
commit messages in the current range-diff output.
^ permalink raw reply related
* Re: [Outreachy][PATCH] Port helper/test-advise.c to unit-tests/t-advise.c
From: Rubén Justo @ 2024-01-15 19:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Achu Luma, git, christian.couder, Christian Couder
In-Reply-To: <xmqqy1cq4ide.fsf@gitster.g>
On 15-ene-2024 09:27:25, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
>
> >> To test the effect of setting one configuration variable, and ensure
> >> it results in a slightly different advice message output to the
> >> standard error stream, "test-tool advice" needs only a single line
> >> of patch, but if we started with this version, how much work does it
> >> take to run the equivalent test in the other patch if it were to be
> >> rebased on top of this change? It won't be just the matter of
> >> adding a new TEST(check_advise_if_enabled()) call to cmd_main(),
> >> will it?
> >
> > Maybe something like this will do the trick:
> >
> > diff --git a/t/unit-tests/t-advise.c b/t/unit-tests/t-advise.c
> > index 15df29c955..ac7d2620ef 100644
> > --- a/t/unit-tests/t-advise.c
> > +++ b/t/unit-tests/t-advise.c
> > @@ -6,6 +6,7 @@
> >
> > static const char expect_advice_msg[] = "hint: This is a piece of advice\n"
> > "hint: Disable this message with \"git config advice.nestedTag false\"\n";
> > +static const char expect_advice_msg_without_disable_hint[] = "hint: This is a piece of advice\n";
> > static const char advice_msg[] = "This is a piece of advice";
> > static const char out_file[] = "./output.txt";
>
> Yup, but ...
>
> > @@ -44,7 +45,7 @@ int cmd_main(int argc, const char **argv) {
> >
> > TEST(check_advise_if_enabled(advice_msg, NULL, expect_advice_msg),
> > "advice should be printed when config variable is unset");
> > - TEST(check_advise_if_enabled(advice_msg, "true", expect_advice_msg),
> > + TEST(check_advise_if_enabled(advice_msg, "true", expect_advice_msg_without_disable_hint),
> > "advice should be printed when config variable is set to true");
> > TEST(check_advise_if_enabled(advice_msg, "false", ""),
> > "advice should not be printed when config variable is set to false");
>
> ... I cannot shake this feeling that the next person who comes to
> this code and stares at advice.c would be very tempted to "refactor"
> the messages, so that there is only one instance of the same string
> in advice.c that is passed to TEST() above. After all, you can
> change only one place to update the message and avoid triggering
> test failures that way, right?
I see. Maybe you're expecting something like:
diff --git a/t/unit-tests/t-advise.c b/t/unit-tests/t-advise.c
index 15df29c955..15e293fa82 100644
--- a/t/unit-tests/t-advise.c
+++ b/t/unit-tests/t-advise.c
@@ -4,14 +4,15 @@
#include "setup.h"
#include "strbuf.h"
-static const char expect_advice_msg[] = "hint: This is a piece of advice\n"
- "hint: Disable this message with \"git config advice.nestedTag false\"\n";
+static const char expect_advice_msg[] = "hint: This is a piece of advice\n";
+static const char expect_hint_msg[] = "hint: Disable this message with \"git config advice.nestedTag false\"\n";
static const char advice_msg[] = "This is a piece of advice";
static const char out_file[] = "./output.txt";
static void check_advise_if_enabled(const char *argv, const char *conf_val, const char *expect) {
FILE *file;
+ const char *hint;
struct strbuf actual = STRBUF_INIT;
if (conf_val)
@@ -32,7 +33,9 @@ static void check_advise_if_enabled(const char *argv, const char *conf_val, cons
return;
}
- check_str(actual.buf, expect);
+ check_str_len(actual.buf, expect, strlen(expect));
+ if (!conf_val && skip_prefix(actual.buf, expect, &hint))
+ check_str_len(hint, expect_hint_msg, strlen(expect_hint_msg));
strbuf_release(&actual);
if (!check(remove(out_file) == 0))
This implies a new check_str_len() helper, which I'm not including here
but it's a trivial copy of check_str() but using strncmp().
Maybe we can turn the screw a little more.
I'm still not sure of the value in the changes in this series, though.
^ permalink raw reply related
* Strange behaviour when pushing a commit object to remote's refs/HEAD
From: Pratyush Yadav @ 2024-01-15 19:08 UTC (permalink / raw)
To: git
Hi,
I ran into a strange Magit bug, where when I ran magit-show-refs on a
particular repo it threw an error. The details of the Magit bug are not
very interesting, but when attempting to reproduce it, I also saw git
misbehaving for such repos.
The strange behaviour happens when you push a commit object to remote's
refs/HEAD instead of pushing a symbolic ref. Such a repository can be
found at https://github.com/prati0100/magit-reproducer. I roughly used
the below steps to create such a repo:
$ git init
$ echo 1 > foo && git add foo && git commit
$ echo 2 > bar && git add bar && git commit
$ git push
$ git checkout 79264c3
$ echo 2.1 > bar && git add bar && git commit
$ git push origin 707a3d5:refs/heads/HEAD
Now with such a repo, if you do `git log --all --oneline` it would look
something like:
707a3d5 (origin/HEAD) 2.1
86e1c97 (HEAD -> main, origin/main) 2
79264c3 1
And running `git for-each-ref --format='%(symref:short),%(refname:short),%(refname),%(subject)' refs/remotes/origin` gives:
,origin,refs/remotes/origin/HEAD,2.1
,origin/main,refs/remotes/origin/main,2
All well and good so far. Now delete the repo and attempt to clone it.
This time `git log --all --oneline` gives:
86e1c97 (HEAD -> main, origin/main, origin/HEAD) 2
79264c3 1
And running `git for-each-ref --format='%(symref:short),%(refname:short),%(refname),%(subject)' refs/remotes/origin` gives:
origin/main,origin,refs/remotes/origin/HEAD,2
,origin/main,refs/remotes/origin/main,2
So suddenly the remote's HEAD becomes origin/main (symbolic ref) and the
commit (707a3d5, "2.1") is nowhere to be found. It neither shows up in
`git rev-list --all` nor in `git log --all`. The files and trees
associated with it also do not show up in `git rev-list --all --object`.
Yet if you do `git show 707a3d5` it shows up. So it does exist and did
get cloned, but git cannot properly see it.
Interestingly enough, even the GitHub UI is confused and it won't show
you the repo correctly. It will show the commit (86e1c97, "2") for both
"branches" main and HEAD. cgit's UI [0] seems to work fine with this,
though cloning from cgit still suffers from this bug.
There _is_ a way to clone the repo correctly. If you do:
$ git init magit-reproducer
$ git remote add origin https://github.com/prati0100/magit-reproducer.git
$ git remote update
Now if you do git log --all or git for-each-ref, you see the correct
result.
I don't really know how to fix this but it certainly is a bug in git
since it can't clone the repo correctly. And at least one major Git host
can't display such a repo properly (I haven't tried others).
I used Git v2.40.1 to do most of this but I did compile the latest
master d4dbce1db5 ("The seventh batch") and attempted to clone using it
and I see the same problem.
[0] https://git.kernel.org/pub/scm/linux/kernel/git/pratyush/magit-reproducer.git/
--
Regards,
Pratyush Yadav
^ permalink raw reply
* Re: [Outreachy][PATCH] Port helper/test-advise.c to unit-tests/t-advise.c
From: Junio C Hamano @ 2024-01-15 17:27 UTC (permalink / raw)
To: Rubén Justo; +Cc: Achu Luma, git, christian.couder, Christian Couder
In-Reply-To: <93468f5c-5f62-4f22-85ce-b60621852430@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
>> To test the effect of setting one configuration variable, and ensure
>> it results in a slightly different advice message output to the
>> standard error stream, "test-tool advice" needs only a single line
>> of patch, but if we started with this version, how much work does it
>> take to run the equivalent test in the other patch if it were to be
>> rebased on top of this change? It won't be just the matter of
>> adding a new TEST(check_advise_if_enabled()) call to cmd_main(),
>> will it?
>
> Maybe something like this will do the trick:
>
> diff --git a/t/unit-tests/t-advise.c b/t/unit-tests/t-advise.c
> index 15df29c955..ac7d2620ef 100644
> --- a/t/unit-tests/t-advise.c
> +++ b/t/unit-tests/t-advise.c
> @@ -6,6 +6,7 @@
>
> static const char expect_advice_msg[] = "hint: This is a piece of advice\n"
> "hint: Disable this message with \"git config advice.nestedTag false\"\n";
> +static const char expect_advice_msg_without_disable_hint[] = "hint: This is a piece of advice\n";
> static const char advice_msg[] = "This is a piece of advice";
> static const char out_file[] = "./output.txt";
Yup, but ...
> @@ -44,7 +45,7 @@ int cmd_main(int argc, const char **argv) {
>
> TEST(check_advise_if_enabled(advice_msg, NULL, expect_advice_msg),
> "advice should be printed when config variable is unset");
> - TEST(check_advise_if_enabled(advice_msg, "true", expect_advice_msg),
> + TEST(check_advise_if_enabled(advice_msg, "true", expect_advice_msg_without_disable_hint),
> "advice should be printed when config variable is set to true");
> TEST(check_advise_if_enabled(advice_msg, "false", ""),
> "advice should not be printed when config variable is set to false");
... I cannot shake this feeling that the next person who comes to
this code and stares at advice.c would be very tempted to "refactor"
the messages, so that there is only one instance of the same string
in advice.c that is passed to TEST() above. After all, you can
change only one place to update the message and avoid triggering
test failures that way, right? But that line of thinking obviously
reduces the value of having tests.
What if messages from plumbing that should not be modified are being
tested with unit tests? These messages are part of contract with
users, and it is very much worth our time to write and maintain the
tests to ensure they will not be modified by accident. Obviously
such a refactoring of test messages to reuse the production strings
would destroy the value of having such a test in the first place.
So, I dunno.
>> I doubt the issue is not about "picking the right moment" to
>> transition from the test-tool to unit testing framework in this
>> particular case. Is "check-advice-if-enabled" a bit too high level
>> a feature to be a good match to "unit" testing, I have to wonder?
>
> I don't have a formed opinion on the change, but I don't see it making
> things worse. I share your doubts, though.
>
> Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge
From: Junio C Hamano @ 2024-01-15 17:19 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Michael Lohmann, git, newren, phillip.wood123
In-Reply-To: <ZaUYyEAdr4oTImL-@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> Which would mean that we do not need a separate "other_head"
>> variable, and instead can use "MERGE_HEAD".
>
> There is no need for this, is there? We have already resolved the ref to
> an object ID, so why not use that via `add_pending_oid()`?
add_pending_oid() and its callees use the name only for human
consumption (e.g., in error messages), as they all already have the
object name.
^ permalink raw reply
* git-request-get ?
From: Matěj Cepl @ 2024-01-15 16:24 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 2166 bytes --]
Hello,
I am still haunted by my own ancient comment on
https://gitlab.com/gitlab-org/gitlab/-/issues/14116:
> Well, the minimal solution [to federated merge requests]
> would be a parser for the git-request-pull(1), which would
> check every comment and if recognized the comment as the
> pull request, it would add button for the owner of the repo
> (e.g., [Create Pull Request]). After pressing that (so it is
> constantly under the control of the owner repo), gitlab would
> setup new remote (if doesn't exist still), fetch it, and create
> a merge request.
Of course, the part with a button has to be resolved in a
particular web application, but I am still wondering whether
there isn’t any way how to make consuming git-request-pull(1)
generated emails more easily digestable and thus promote the use
of the tool.
First I created rather complicated bash script
(https://da.gd/pSgdc), but then I have read CodingGuidelines and
found that I need to keep myself to the POSIX shell script, so
I have simplified a lot. Besides, I don’t think the complicated
part (like adding remotes) is necessarily a good thing (???).
Currently I have just this:
#!/bin/sh
set -eu
STR="$(cat)"
URL="$(echo "$STR" | sed -n -e '/^are available in the Git repository at:/,+2 {
s/[[:space:]]\+//
s/\(=[[:digit:]]\{2\}\)\+$//
/^\(http\|git\)/p
}')"
END="$(echo "$STR" | awk '/^for you to fetch changes up to / { print $NF }' | sed -e 's/[=:]*$//')"
git fetch "$URL" "$END"
git checkout -B _4review FETCH_HEAD
Do you think this is a good idea at all? Should we be more
complicated or less? Should we do some fun things like parsing
and adding remotes, parsing email addresses or something to
create individualized branch names for review? Stuff like that.
Looking forward to any feedback.
Best,
Matěj
--
http://matej.ceplovi.cz/blog/, @mcepl@floss.social
GPG Finger: 3C76 A027 CA45 AD70 98B5 BC1D 7920 5802 880B C9D8
Those to whom evil is done \ Do evil in return.
-- W. H. Auden, September 1, 1939
http://www.poets.org/viewmedia.php/prmMID/15545
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 216 bytes --]
^ permalink raw reply
* [PATCH v2] advice: allow disabling the automatic hint in advise_if_enabled()
From: Rubén Justo @ 2024-01-15 14:28 UTC (permalink / raw)
To: Git List, Junio C Hamano, Taylor Blau, Jeff King, Dragan Simic
In-Reply-To: <c870a0b6-9fa8-4d00-a5a6-661ca175805f@gmail.com>
Using advise_if_enabled() to display an advice will automatically
include instructions on how to disable the advice, alongside the main
advice:
hint: use --reapply-cherry-picks to include skipped commits
hint: Disable this message with "git config advice.skippedCherryPicks false"
To do so, we provide a knob which can be used to disable the advice.
But also to tell us the opposite: to show the advice.
Let's not include the deactivation instructions for an advice if the
user explicitly sets its visibility.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
This must have been v3, but previous iteration was erroneously sent as a
v1. Sorry.
Range-diff against v1:
1: d280195c7b ! 1: 0bee5d1bba advice: allow disabling the automatic hint in advise_if_enabled()
@@ Commit message
Signed-off-by: Rubén Justo <rjusto@gmail.com>
+ ## Documentation/config/advice.txt ##
+@@
+ advice.*::
+ These variables control various optional help messages designed to
+- aid new users. All 'advice.*' variables default to 'true', and you
+- can tell Git that you do not need help by setting these to 'false':
++ aid new users. When left unconfigured, Git will give the message
++ alongside instructions on how to squelch it. You can tell Git
++ that you do not need the help message by setting these to 'false':
+ +
+ --
+ addEmbeddedRepo::
+
## advice.c ##
@@ advice.c: static const char *advise_get_color(enum color_advice ix)
return "";
}
+enum advice_level {
-+ ADVICE_LEVEL_DEFAULT = 0,
++ ADVICE_LEVEL_NONE = 0,
+ ADVICE_LEVEL_DISABLED,
+ ADVICE_LEVEL_ENABLED,
+};
@@ advice.c: void advise_if_enabled(enum advice_type type, const char *advice, ...)
va_start(params, advice);
- vadvise(advice, 1, advice_setting[type].key, params);
-+ vadvise(advice, advice_setting[type].level == ADVICE_LEVEL_DEFAULT,
-+ advice_setting[type].key, params);
++ vadvise(advice, !advice_setting[type].level, advice_setting[type].key,
++ params);
va_end(params);
}
Documentation/config/advice.txt | 5 +-
advice.c | 109 +++++++++++++++++---------------
t/t0018-advice.sh | 1 -
3 files changed, 62 insertions(+), 53 deletions(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 25c0917524..c7ea70f2e2 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -1,7 +1,8 @@
advice.*::
These variables control various optional help messages designed to
- aid new users. All 'advice.*' variables default to 'true', and you
- can tell Git that you do not need help by setting these to 'false':
+ aid new users. When left unconfigured, Git will give the message
+ alongside instructions on how to squelch it. You can tell Git
+ that you do not need the help message by setting these to 'false':
+
--
addEmbeddedRepo::
diff --git a/advice.c b/advice.c
index f6e4c2f302..6e9098ff08 100644
--- a/advice.c
+++ b/advice.c
@@ -33,50 +33,56 @@ static const char *advise_get_color(enum color_advice ix)
return "";
}
+enum advice_level {
+ ADVICE_LEVEL_NONE = 0,
+ ADVICE_LEVEL_DISABLED,
+ ADVICE_LEVEL_ENABLED,
+};
+
static struct {
const char *key;
- int enabled;
+ enum advice_level level;
} advice_setting[] = {
- [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo", 1 },
- [ADVICE_ADD_EMPTY_PATHSPEC] = { "addEmptyPathspec", 1 },
- [ADVICE_ADD_IGNORED_FILE] = { "addIgnoredFile", 1 },
- [ADVICE_AMBIGUOUS_FETCH_REFSPEC] = { "ambiguousFetchRefspec", 1 },
- [ADVICE_AM_WORK_DIR] = { "amWorkDir", 1 },
- [ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = { "checkoutAmbiguousRemoteBranchName", 1 },
- [ADVICE_COMMIT_BEFORE_MERGE] = { "commitBeforeMerge", 1 },
- [ADVICE_DETACHED_HEAD] = { "detachedHead", 1 },
- [ADVICE_DIVERGING] = { "diverging", 1 },
- [ADVICE_FETCH_SHOW_FORCED_UPDATES] = { "fetchShowForcedUpdates", 1 },
- [ADVICE_FORCE_DELETE_BRANCH] = { "forceDeleteBranch", 1 },
- [ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated", 1 },
- [ADVICE_IGNORED_HOOK] = { "ignoredHook", 1 },
- [ADVICE_IMPLICIT_IDENTITY] = { "implicitIdentity", 1 },
- [ADVICE_NESTED_TAG] = { "nestedTag", 1 },
- [ADVICE_OBJECT_NAME_WARNING] = { "objectNameWarning", 1 },
- [ADVICE_PUSH_ALREADY_EXISTS] = { "pushAlreadyExists", 1 },
- [ADVICE_PUSH_FETCH_FIRST] = { "pushFetchFirst", 1 },
- [ADVICE_PUSH_NEEDS_FORCE] = { "pushNeedsForce", 1 },
- [ADVICE_PUSH_NON_FF_CURRENT] = { "pushNonFFCurrent", 1 },
- [ADVICE_PUSH_NON_FF_MATCHING] = { "pushNonFFMatching", 1 },
- [ADVICE_PUSH_REF_NEEDS_UPDATE] = { "pushRefNeedsUpdate", 1 },
- [ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName", 1 },
- [ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected", 1 },
- [ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward", 1 }, /* backwards compatibility */
- [ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh", 1 },
- [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict", 1 },
- [ADVICE_RM_HINTS] = { "rmHints", 1 },
- [ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse", 1 },
- [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure", 1 },
- [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks", 1 },
- [ADVICE_STATUS_AHEAD_BEHIND_WARNING] = { "statusAheadBehindWarning", 1 },
- [ADVICE_STATUS_HINTS] = { "statusHints", 1 },
- [ADVICE_STATUS_U_OPTION] = { "statusUoption", 1 },
- [ADVICE_SUBMODULES_NOT_UPDATED] = { "submodulesNotUpdated", 1 },
- [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
- [ADVICE_SUGGEST_DETACHING_HEAD] = { "suggestDetachingHead", 1 },
- [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath", 1 },
- [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor", 1 },
- [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan", 1 },
+ [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo" },
+ [ADVICE_ADD_EMPTY_PATHSPEC] = { "addEmptyPathspec" },
+ [ADVICE_ADD_IGNORED_FILE] = { "addIgnoredFile" },
+ [ADVICE_AMBIGUOUS_FETCH_REFSPEC] = { "ambiguousFetchRefspec" },
+ [ADVICE_AM_WORK_DIR] = { "amWorkDir" },
+ [ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] = { "checkoutAmbiguousRemoteBranchName" },
+ [ADVICE_COMMIT_BEFORE_MERGE] = { "commitBeforeMerge" },
+ [ADVICE_DETACHED_HEAD] = { "detachedHead" },
+ [ADVICE_DIVERGING] = { "diverging" },
+ [ADVICE_FETCH_SHOW_FORCED_UPDATES] = { "fetchShowForcedUpdates" },
+ [ADVICE_FORCE_DELETE_BRANCH] = { "forceDeleteBranch" },
+ [ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated" },
+ [ADVICE_IGNORED_HOOK] = { "ignoredHook" },
+ [ADVICE_IMPLICIT_IDENTITY] = { "implicitIdentity" },
+ [ADVICE_NESTED_TAG] = { "nestedTag" },
+ [ADVICE_OBJECT_NAME_WARNING] = { "objectNameWarning" },
+ [ADVICE_PUSH_ALREADY_EXISTS] = { "pushAlreadyExists" },
+ [ADVICE_PUSH_FETCH_FIRST] = { "pushFetchFirst" },
+ [ADVICE_PUSH_NEEDS_FORCE] = { "pushNeedsForce" },
+ [ADVICE_PUSH_NON_FF_CURRENT] = { "pushNonFFCurrent" },
+ [ADVICE_PUSH_NON_FF_MATCHING] = { "pushNonFFMatching" },
+ [ADVICE_PUSH_REF_NEEDS_UPDATE] = { "pushRefNeedsUpdate" },
+ [ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName" },
+ [ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected" },
+ [ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward" }, /* backwards compatibility */
+ [ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh" },
+ [ADVICE_RESOLVE_CONFLICT] = { "resolveConflict" },
+ [ADVICE_RM_HINTS] = { "rmHints" },
+ [ADVICE_SEQUENCER_IN_USE] = { "sequencerInUse" },
+ [ADVICE_SET_UPSTREAM_FAILURE] = { "setUpstreamFailure" },
+ [ADVICE_SKIPPED_CHERRY_PICKS] = { "skippedCherryPicks" },
+ [ADVICE_STATUS_AHEAD_BEHIND_WARNING] = { "statusAheadBehindWarning" },
+ [ADVICE_STATUS_HINTS] = { "statusHints" },
+ [ADVICE_STATUS_U_OPTION] = { "statusUoption" },
+ [ADVICE_SUBMODULES_NOT_UPDATED] = { "submodulesNotUpdated" },
+ [ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie" },
+ [ADVICE_SUGGEST_DETACHING_HEAD] = { "suggestDetachingHead" },
+ [ADVICE_UPDATE_SPARSE_PATH] = { "updateSparsePath" },
+ [ADVICE_WAITING_FOR_EDITOR] = { "waitingForEditor" },
+ [ADVICE_WORKTREE_ADD_ORPHAN] = { "worktreeAddOrphan" },
};
static const char turn_off_instructions[] =
@@ -116,13 +122,13 @@ void advise(const char *advice, ...)
int advice_enabled(enum advice_type type)
{
- switch(type) {
- case ADVICE_PUSH_UPDATE_REJECTED:
- return advice_setting[ADVICE_PUSH_UPDATE_REJECTED].enabled &&
- advice_setting[ADVICE_PUSH_UPDATE_REJECTED_ALIAS].enabled;
- default:
- return advice_setting[type].enabled;
- }
+ int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
+
+ if (type == ADVICE_PUSH_UPDATE_REJECTED)
+ return enabled &&
+ advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);
+
+ return enabled;
}
void advise_if_enabled(enum advice_type type, const char *advice, ...)
@@ -133,7 +139,8 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
return;
va_start(params, advice);
- vadvise(advice, 1, advice_setting[type].key, params);
+ vadvise(advice, !advice_setting[type].level, advice_setting[type].key,
+ params);
va_end(params);
}
@@ -162,7 +169,9 @@ int git_default_advice_config(const char *var, const char *value)
for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
if (strcasecmp(k, advice_setting[i].key))
continue;
- advice_setting[i].enabled = git_config_bool(var, value);
+ advice_setting[i].level = git_config_bool(var, value)
+ ? ADVICE_LEVEL_ENABLED
+ : ADVICE_LEVEL_DISABLED;
return 0;
}
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
index c13057a4ca..0dcfb760a2 100755
--- a/t/t0018-advice.sh
+++ b/t/t0018-advice.sh
@@ -17,7 +17,6 @@ test_expect_success 'advice should be printed when config variable is unset' '
test_expect_success 'advice should be printed when config variable is set to true' '
cat >expect <<-\EOF &&
hint: This is a piece of advice
- hint: Disable this message with "git config advice.nestedTag false"
EOF
test_config advice.nestedTag true &&
test-tool advise "This is a piece of advice" 2>actual &&
base-commit: bec9bb4b3918d2b3c7b91bbb116a667d5d6d298d
--
2.42.0
^ permalink raw reply related
* [PATCH 3/3] ci: add macOS jobs to GitLab CI
From: Patrick Steinhardt @ 2024-01-15 11:45 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1705318985.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2672 bytes --]
Add two macOS-based jobs to GitLab CI, one for Clang and one for GCC.
This matches equivalent jobs we have for GitHub Workflows, except that
we use macOS 14 instead of macOS 13.
Note that one test marked as `test_must_fail` is surprisingly passing:
t7815-grep-binary.sh (Wstat: 0 Tests: 22 Failed: 0)
TODO passed: 12
This seems to boil down to an unexpected difference in how regcomp(1)
works when matching NUL bytes. Cross-checking with the respective GitHub
job shows though that this is not an issue unique to the GitLab CI job
as it passes in the same way there.
Further note that we do not include the equivalent for the "osx-gcc" job
that we use with GitHub Workflows. This is because the runner for macOS
on GitLab is running on Apple M1 machines and thus uses the "arm64"
architecture. GCC does not support this platform yet.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
.gitlab-ci.yml | 26 +++++++++++++++++++++++++-
ci/lib.sh | 9 ++++++++-
2 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 793243421c..9748970798 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -7,7 +7,7 @@ workflow:
- if: $CI_COMMIT_TAG
- if: $CI_COMMIT_REF_PROTECTED == "true"
-test:
+test:linux:
image: $image
before_script:
- ./ci/install-docker-dependencies.sh
@@ -52,6 +52,30 @@ test:
- t/failed-test-artifacts
when: on_failure
+test:osx:
+ image: $image
+ tags:
+ - saas-macos-medium-m1
+ before_script:
+ - ./ci/install-dependencies.sh
+ script:
+ - ./ci/run-build-and-tests.sh
+ after_script:
+ - |
+ if test "$CI_JOB_STATUS" != 'success'
+ then
+ ./ci/print-test-failures.sh
+ fi
+ parallel:
+ matrix:
+ - jobname: osx-clang
+ image: macos-13-xcode-14
+ CC: clang
+ artifacts:
+ paths:
+ - t/failed-test-artifacts
+ when: on_failure
+
static-analysis:
image: ubuntu:22.04
variables:
diff --git a/ci/lib.sh b/ci/lib.sh
index f631206a44..d5dd2f2697 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -252,7 +252,14 @@ then
CI_COMMIT="$CI_COMMIT_SHA"
case "$CI_JOB_IMAGE" in
macos-*)
- CI_OS_NAME=osx;;
+ # GitLab CI has Python installed via multiple package managers,
+ # most notably via asdf and Homebrew. Ensure that our builds
+ # pick up the Homebrew one by prepending it to our PATH as the
+ # asdf one breaks tests.
+ export PATH="$(brew --prefix)/bin:$PATH"
+
+ CI_OS_NAME=osx
+ ;;
alpine:*|fedora:*|ubuntu:*)
CI_OS_NAME=linux;;
*)
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/3] Makefile: detect new Homebrew location for ARM-based Macs
From: Patrick Steinhardt @ 2024-01-15 11:45 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1705318985.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1625 bytes --]
With the introduction of the ARM-based Macs the default location for
Homebrew has changed from "/usr/local" to "/opt/homebrew". We only
handle the former location though, which means that unless the user has
manually configured required search paths we won't be able to locate it.
Improve upon this by adding relevant paths to our CFLAGS and LDFLAGS as
well as detecting the location of msgfmt(1).
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
config.mak.uname | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/config.mak.uname b/config.mak.uname
index 3bb03f423a..dacc95172d 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -158,6 +158,19 @@ ifeq ($(uname_S),Darwin)
ifeq ($(shell test -x /usr/local/opt/gettext/bin/msgfmt && echo y),y)
MSGFMT = /usr/local/opt/gettext/bin/msgfmt
endif
+ # On newer ARM-based machines the default installation path has changed to
+ # /opt/homebrew. Include it in our search paths so that the user does not
+ # have to configure this manually.
+ #
+ # Note that we do not employ the same workaround as above where we manually
+ # add gettext. The issue was fixed more than three years ago by now, and at
+ # that point there haven't been any ARM-based Macs yet.
+ else ifeq ($(shell test -d /opt/homebrew/ && echo y),y)
+ BASIC_CFLAGS += -I/opt/homebrew/include
+ BASIC_LDFLAGS += -L/opt/homebrew/lib
+ ifeq ($(shell test -x /opt/homebrew/bin/msgfmt && echo y),y)
+ MSGFMT = /opt/homebrew/bin/msgfmt
+ endif
endif
# The builtin FSMonitor on MacOS builds upon Simple-IPC. Both require
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 1/3] ci: make p4 setup on macOS more robust
From: Patrick Steinhardt @ 2024-01-15 11:44 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1705318985.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1725 bytes --]
When setting up Perforce on macOS we put both `p4` and `p4d` into
"$HOME/bin". On GitHub CI this directory is indeed contained in the PATH
environment variable and thus there is no need for additional setup than
to put the binaries there. But GitLab CI does not do this, and thus our
Perforce-based tests would be skipped there even though we download the
binaries.
Refactor the setup code to become more robust by downloading binaries
into a separate directory which we then manually append to our PATH.
This matches what we do on Linux-based jobs.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/install-dependencies.sh | 10 ++++------
ci/lib.sh | 3 +++
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 4f407530d3..b4e22de3cb 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -37,15 +37,13 @@ macos-*)
test -z "$BREW_INSTALL_PACKAGES" ||
brew install $BREW_INSTALL_PACKAGES
brew link --force gettext
- mkdir -p $HOME/bin
- (
- cd $HOME/bin
+
+ mkdir -p "$P4_PATH"
+ pushd "$P4_PATH"
wget -q "$P4WHENCE/bin.macosx1015x86_64/helix-core-server.tgz" &&
tar -xf helix-core-server.tgz &&
sudo xattr -d com.apple.quarantine p4 p4d 2>/dev/null || true
- )
- PATH="$PATH:${HOME}/bin"
- export PATH
+ popd
if test -n "$CC_PACKAGE"
then
diff --git a/ci/lib.sh b/ci/lib.sh
index c749b21366..f631206a44 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -344,6 +344,9 @@ macos-*)
then
MAKEFLAGS="$MAKEFLAGS APPLE_COMMON_CRYPTO_SHA1=Yes"
fi
+
+ P4_PATH="$HOME/custom/p4"
+ export PATH="$P4_PATH:$PATH"
;;
esac
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 0/3] ci: add support for macOS to GitLab CI
From: Patrick Steinhardt @ 2024-01-15 11:44 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 989 bytes --]
Hi,
this patch series extends GitLab CI to also support macOS. Besides
extending test coverage for GitLab users, this change also has the added
benefit that the macOS runners at GitLab are based on Apple silicon,
which to the best of my knowledge is not something we're currently
testing on.
This patch series builds on top of ps/gitlab-ci-static-analysis
(currently at cd69c635a1 (ci: add job performing static analysis on
GitLab CI, 2023-12-28)) to avoid a conflict.
Patrick
Patrick Steinhardt (3):
ci: make p4 setup on macOS more robust
Makefile: detect new Homebrew location for ARM-based Macs
ci: add macOS jobs to GitLab CI
.gitlab-ci.yml | 26 +++++++++++++++++++++++++-
ci/install-dependencies.sh | 10 ++++------
ci/lib.sh | 12 +++++++++++-
config.mak.uname | 13 +++++++++++++
4 files changed, 53 insertions(+), 8 deletions(-)
base-commit: cd69c635a1a62b0c8bfdbf221778be8a512ad048
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [Outreachy][PATCH] Port helper/test-advise.c to unit-tests/t-advise.c
From: Rubén Justo @ 2024-01-15 11:37 UTC (permalink / raw)
To: Junio C Hamano, Achu Luma; +Cc: git, christian.couder, Christian Couder
In-Reply-To: <xmqqmsta6uju.fsf@gitster.g>
On 12-ene-2024 14:44:37, Junio C Hamano wrote:
> Achu Luma <ach.lumap@gmail.com> writes:
>
> > In the recent codebase update (8bf6fbd00d (Merge branch
> > 'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
> > merged, providing a standardized approach for testing C code. Prior to
> > this update, some unit tests relied on the test helper mechanism,
> > lacking a dedicated unit testing framework. It's more natural to perform
> > these unit tests using the new unit test framework.
> >
> > This commit migrates the unit tests for advise_if_enabled functionality
> > from the legacy approach using the test-tool command `test-tool advise`
> > in t/helper/test-advise.c to the new unit testing framework
> > (t/unit-tests/test-lib.h).
> >
> > The migration involves refactoring the tests to utilize the testing
> > macros provided by the framework (TEST() and check_*()).
>
> In the light of the presense of work like this one
>
> https://lore.kernel.org/git/c870a0b6-9fa8-4d00-a5a6-661ca175805f@gmail.com/
>
> I am not sure if moving this to unit-test framework is a good thing,
> at least right at this moment.
>
> To test the effect of setting one configuration variable, and ensure
> it results in a slightly different advice message output to the
> standard error stream, "test-tool advice" needs only a single line
> of patch, but if we started with this version, how much work does it
> take to run the equivalent test in the other patch if it were to be
> rebased on top of this change? It won't be just the matter of
> adding a new TEST(check_advise_if_enabled()) call to cmd_main(),
> will it?
Maybe something like this will do the trick:
diff --git a/t/unit-tests/t-advise.c b/t/unit-tests/t-advise.c
index 15df29c955..ac7d2620ef 100644
--- a/t/unit-tests/t-advise.c
+++ b/t/unit-tests/t-advise.c
@@ -6,6 +6,7 @@
static const char expect_advice_msg[] = "hint: This is a piece of advice\n"
"hint: Disable this message with \"git config advice.nestedTag false\"\n";
+static const char expect_advice_msg_without_disable_hint[] = "hint: This is a piece of advice\n";
static const char advice_msg[] = "This is a piece of advice";
static const char out_file[] = "./output.txt";
@@ -44,7 +45,7 @@ int cmd_main(int argc, const char **argv) {
TEST(check_advise_if_enabled(advice_msg, NULL, expect_advice_msg),
"advice should be printed when config variable is unset");
- TEST(check_advise_if_enabled(advice_msg, "true", expect_advice_msg),
+ TEST(check_advise_if_enabled(advice_msg, "true", expect_advice_msg_without_disable_hint),
"advice should be printed when config variable is set to true");
TEST(check_advise_if_enabled(advice_msg, "false", ""),
"advice should not be printed when config variable is set to false");
>
> I doubt the issue is not about "picking the right moment" to
> transition from the test-tool to unit testing framework in this
> particular case. Is "check-advice-if-enabled" a bit too high level
> a feature to be a good match to "unit" testing, I have to wonder?
I don't have a formed opinion on the change, but I don't see it making
things worse. I share your doubts, though.
Thanks.
^ permalink raw reply related
* Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge
From: Patrick Steinhardt @ 2024-01-15 11:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Lohmann, git, newren, phillip.wood123
In-Reply-To: <xmqqzfxa9usx.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]
On Fri, Jan 12, 2024 at 12:10:54PM -0800, Junio C Hamano wrote:
> Michael Lohmann <mi.al.lohmann@gmail.com> writes:
>
> >> but we may want to tighten the original's use of repo_get
> >> > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> >> > - die("--merge without MERGE_HEAD?");
> >> > - other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> >>
> >> ... so that we won't be confused in a repository that has a tag
> >> whose name happens to be MERGE_HEAD. We probably should be using
> >> refs.c:refs_resolve_ref_unsafe() instead to _oid() here ...
> >
> > Here I am really at the limit of my understanding of the core functions.
> > Is this roughly what you had in mind? From my testing it seems to do the
> > job, but I don't understand the details "refs_resolve_ref_unsafe"...
>
> Perhaps there are others who are more familiar with the ref API than
> I am, but I was just looking at refs.h today and wonder if something
> along the lines of this ...
>
> if (read_ref_full("MERGE_HEAD",
> RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> &oid, NULL))
> die("no MERGE_HEAD");
> if (is_null_oid(&oid))
> die("MERGE_HEAD is a symbolic ref???");
>
> ... would be simpler.
>
> With the above, we are discarding the refname read_ref_full()
> obtains from its refs_resolve_ref_unsafe(), but I think that is
> totally fine. We expect it to be "MERGE_HEAD" in the good case.
> The returned value can be different from "MERGE_HEAD" if it is
> a symbolic ref, but if the comment in refs.h on NO_RECURSE is to be
> trusted, we should catch that case with the NULL-ness check on oid.
Yeah, this should be fine.
Even though I have stared at our refs API for extended periods of time
during the last months I still have to look up this part of the API
almost every time. I find it to be hard to use because you not only have
to pay attention to the return value, but also to the flags in some edge
cases. I wouldn't be surprised if there were many callsites that get
this wrong.
> Which would mean that we do not need a separate "other_head"
> variable, and instead can use "MERGE_HEAD".
There is no need for this, is there? We have already resolved the ref to
an object ID, so why not use that via `add_pending_oid()`?
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox