Re: Changing encoding of a file : What should happen to CRLF in file ?
> If you commit the file, it will be stored with LF in the index, This is what i believe is not happening. Lets do this with a public repository and steps which are reproducible. I have created a repo : https://github.com/ashishnegi/git_encoding If you clone this repo in linux and run `git status`, you will find that file is modified. About repo : Repo have 2 commits, done on windows machine. First one check in a utf-16le encoded file which has crlf. crlf will not be converted to lf in index as git treats it as binary file. 2nd commit changes encoding to utf-8 and commits. This commit does not change crlf to lf in index, even though new format is utf-8 which is text based for git. This is the crux of problem. I have attached all commands i ran on windows while creating the repo. I tried to capture all information that i could give. Please have a look. It might be useful. Finally, thank you Torsten for giving your time to the problem. Really appreciate it. On Tue, Nov 14, 2017 at 10:39 PM, Torsten Bögershausen wrote: > (Back to the beginning) > > You have a file ApplicationManifest.xml > It is encoded in UTF-16 (and has CRLF) > > You convert it into UTF-8 > The file has still CRLF (in the worktree) > > Now you add it and make a commit. > Under both Linux and Windows you have "text=auto". > > I assume that you have efficiently core.eol=lf under Linux > and core.eol=crlf on Windows. > > (That is the default, when you don't change anything) > > Now, what happens to the CRLF? > If you commit the file, it will be stored with LF in the index, > on both systems. > On checkout, Windows will convert them into CRLF, but Linux will not. > > That why you see >>On linux, during committing i get warning : warning: CRLF will be >>replaced by LF in …file_name.. > > All in all there is nothing wrong, at least as I see it. > > The question remains: > Do you need CRLF in Linux ? > Probably not, but if yes, plase add a line > > *.xml text eol=crlf > > to your > .gitattributes > > Otherwise your .gitconfig looks good to me. > > > > > > git_encoding_repo>git config --global core.safecrlf true git_encoding_repo>git config core.autocrlf false git_encoding_repo>git config --get core.autocrlf false git_encoding_repo>cat .gitattributes # Set the default behavior, in case people don't have core.autocrlf set. * text=auto *.vcxproj eol=crlf *.sh eol=lf # Denote all files that are truly binary and should not be modified. *.exe binary *.dll binary *.pdb binary *.ico binary *.png binary *.jpg binary git_encoding_repo>git status On branch master No commits yet Untracked files: (use "git add ..." to include in what will be committed) .gitattributes file_name.txt nothing added to commit but untracked files present (use "git add" to track) git_encoding_repo>git ls-files --eol file_name.txt git_encoding_repo>git add . git_encoding_repo>git status On branch master No commits yet Changes to be committed: (use "git rm --cached ..." to unstage) new file: .gitattributes new file: file_name.txt git_encoding_repo>git ls-files --eol file_name.txt i/-text w/-text attr/text=auto file_name.txt git_encoding_repo>git commit -m "Commit utf-16le encoded file which has crlf." [master (root-commit) 91fe3bd] Commit utf-16le encoded file which has crlf. 2 files changed, 13 insertions(+) create mode 100644 .gitattributes create mode 100644 file_name.txt ## At this time, i changed file encoding to utf-8. git_encoding_repo>git status On branch master nothing to commit, working tree clean git_encoding_repo>git add -p Only binary files changed. git_encoding_repo>git status On branch master Changes not staged for commit: (use "git add ..." to update what will be committed) (use "git checkout -- ..." to discard changes in working directory) modified: file_name.txt no changes added to commit (use "git add" and/or "git commit -a") git_encoding_repo>git add file_name.txt git_encoding_repo>git status On branch master Changes to be committed: (use "git reset HEAD ..." to unstage) modified: file_name.txt git_encoding_repo>git commit -m "Change encoding of file to utf-8" [master 179c27b] Change encoding of file to utf-8 1 file changed, 0 insertions(+), 0 deletions(-) rewrite file_name.txt (100%) git_encoding_repo>git remote add origin https://github.com/ashishnegi/git_encoding.git git_encoding_repo>git push -u origin master Counting objects: 7, done. Delta compression using up to 12 threads. Compressing objects: 100% (7/7), done. Writing objects: 100% (7/7), 837 bytes | 837.00 KiB/s, done. Total 7 (delta 0), reused 0 (delta 0) To https://github.com/ashishnegi/git_encoding.git * [new branch] master -> master Branch master set up to track remote branch master from origin. git_encoding_repo>git ls-files --eol file_name.txt i/crlf w/crlf attr/text=auto file_name.txt git_encoding_repo>
Re: [PATCH v1 2/2] worktree: make add dwim
On 11/14, Eric Sunshine wrote:
> On Tue, Nov 14, 2017 at 3:45 AM, Thomas Gummerer wrote:
> > On 11/13, Junio C Hamano wrote:
> >> If so, as long as the new DWIM kicks in ONLY when "topic" does not
> >> exist, I suspect that there is no backward compatibility worries.
> >> The command line
> >>
> >> $ git worktree add ../a-new-worktree topic
> >>
> >> would just have failed because three was no 'topic' branch yet, no?
> >
> > Indeed, with this there would not be any backwards compatility
> > worries.
> >
> > Ideally I'd still like to make
> >
> > $ git worktree add ../topic
> >
> > work as described above, to avoid having to type 'topic' twice (the
> > directory name matches the branch name for the vast majority of my
> > worktrees) but that should then come behind a flag/config option?
> > Following your reasoning above it should probably be called something
> > other than --guess.
> >
> > Maybe --guess-remote and worktree.guessRemote would do? I'm quite bad
> > at naming though, so other suggestions are very welcome.
>
> For my own edification...
>
> git worktree add ../dir branch
>
> * Checks out branch into ../dir if branch exists.
>
> * Errors out if branch does not exist. However, if we consider the
> history of the implementation of worktrees[*1*], then this really
> ought to try the "origin/branch -> branch" DWIM before erroring-out.
> Implementing this DWIM could be considered a regression fix according
> to [*1*] and, as Junio points out, should not pose backward
> compatibility worries.
Agreed, I think it is not very controversial that this would be an
improvement.
> git worktree add ../topic
>
> * Correctly errors out, refusing to create a new branch named "topic",
> if "topic" is already a branch.
>
> * Creates a new branch named "topic" if no such local branch exists.
>
> The desired new DWIMing would change the second bullet point to:
>
> * If no local branch named "topic" exists, DWIM it from "origin/topic"
> if possible, else create a new local branch named "topic".
>
> But that's a behavior change since, with the existing implementation,
> a newly created local "topic" has no relation to, and does not track,
> any origin/topic branch. The proposed --guess option is to avoid users
> being hit by this backward incompatibility, however, one could also
> view the proposed DWIMing as some sort of bug fix since, at least
> some, users would expect the DWIMing since it already takes place
> elsewhere.
I'm not sure we can call it a bug fix anymore, since as Junio pointed
out the current behaviour of creating a new branch at HEAD is
documented in the man page.
However git-worktree is also still marked as experimental in the man
page, so we could allow ourselves to be a little bit more lax when it
comes to backwards compatibility, especially because it is easy to
take corrective action after the new DWIMing happened.
> So, at least two options exist:
>
> * Just make the new DWIMing the default behavior, accepting that it
> might bite a few people. Fallout can be mitigated somewhat by
> prominently announcing that the DWIMing took place, in which case the
> user can take corrective action (say, by choosing a different worktree
> name); nothing is lost and no damage done since this is happening only
> at worktree creation time.
>
> * Add a new option to enable DWIMing; perhaps name it -t/--track,
> which is familiar terminology and still gives you a relatively short
> and sweet "git worktree add -t ../topic".
>
> Given the mentioned mitigation factor and that some/many users likely
> would expect it to DWIM "origin/topic -> topic" anyhow, I'd lean in
> favor of the first option (but perhaps I'm too daring with other
> people's workflows).
Yeah, I'm leaning towards the first option as well, but I'm clearly
biased as that's how I'd like it to behave, and others might want the
other behaviour. Unfortunately I don't know many worktree users, so I
can't tell what the general consensus on this would be.
I guess the second option would be the safer one, and we can still
switch that to be the default at some point if we wish to do so
later.
tl;dr I have no idea which of the options would be better :)
> FOOTNOTES
>
> [*1*]: When Duy first implemented worktree support, he incorporated it
> directly into the git-checkout command ("git checkout --to worktree
> ..."), which means that he got all the git-checkout features for free,
> including the "origin/branch -> branch" DWIM. When worktree support
> was later moved to git-worktree, it lost most of the features
> inherited implicitly from git-checkout, such as -b, -B, --detach, so
> those were added back to git-worktree explicitly. However, at that
> early stage, git-worktree was still piggy-backing atop git-checkout,
> thus likely was still getting the "origin/branch -> branch" DWIM for
> free. A final iteration converted git-worktree away from heavyweight
> git-checkout to lightweight git-reset, at which point he DWIMing was
> l
Re: [PATCH v1 2/2] worktree: make add dwim
On 11/14, Eric Sunshine wrote: > On Tue, Nov 14, 2017 at 3:14 PM, Eric Sunshine > wrote: > > For my own edification... > > [...] > > git worktree add ../topic > > > > * Correctly errors out, refusing to create a new branch named "topic", > > if "topic" is already a branch. > > By the way, there's an additional DWIM that could be done here instead > of erroring out. Specifically, for "git worktree add ../topic": > > * If branch "topic" exists, check it out (rather than refusing to > create a new branch named "topic"). I think this would be a good improvement either way as I suspect this is what users would hope for, and as it currently just dies there are less backwards compatibility worries. > * If origin/topic exists, DWIM local "topic" branch into existence. > > * Otherwise, create new local branch "topic". > > > * Creates a new branch named "topic" if no such local branch exists. > > > > The desired new DWIMing would change the second bullet point to: > > > > * If no local branch named "topic" exists, DWIM it from "origin/topic" > > if possible, else create a new local branch named "topic".
Re: [PATCH v1 2/2] worktree: make add dwim
On Wed, Nov 15, 2017 at 3:50 AM, Thomas Gummerer wrote: > On 11/14, Eric Sunshine wrote: >> git worktree add ../topic >> [...] >> The desired new DWIMing would change the second bullet point to: >> >> * If no local branch named "topic" exists, DWIM it from "origin/topic" >> if possible, else create a new local branch named "topic". >> >> But that's a behavior change since, with the existing implementation, >> a newly created local "topic" has no relation to, and does not track, >> any origin/topic branch. The proposed --guess option is to avoid users >> being hit by this backward incompatibility, however, one could also >> view the proposed DWIMing as some sort of bug fix since, at least >> some, users would expect the DWIMing since it already takes place >> elsewhere. > > I'm not sure we can call it a bug fix anymore, since as Junio pointed > out the current behaviour of creating a new branch at HEAD is > documented in the man page. > > However git-worktree is also still marked as experimental in the man > page, so we could allow ourselves to be a little bit more lax when it > comes to backwards compatibility, especially because it is easy to > take corrective action after the new DWIMing happened. Yep, my leaning toward making this new DWIMing default (without a --guess or --track option) also is based in part on git-worktree still being marked "experimental". >> So, at least two options exist: >> >> * Just make the new DWIMing the default behavior, accepting that it >> might bite a few people. Fallout can be mitigated somewhat by >> prominently announcing that the DWIMing took place, in which case the >> user can take corrective action (say, by choosing a different worktree >> name); nothing is lost and no damage done since this is happening only >> at worktree creation time. >> >> * Add a new option to enable DWIMing; perhaps name it -t/--track, >> which is familiar terminology and still gives you a relatively short >> and sweet "git worktree add -t ../topic". >> >> Given the mentioned mitigation factor and that some/many users likely >> would expect it to DWIM "origin/topic -> topic" anyhow, I'd lean in >> favor of the first option (but perhaps I'm too daring with other >> people's workflows). > > Yeah, I'm leaning towards the first option as well, but I'm clearly > biased as that's how I'd like it to behave, and others might want the > other behaviour. Unfortunately I don't know many worktree users, so I > can't tell what the general consensus on this would be. Aside from the mentioned mitigation factor, which somewhat eases my worries about backward incompatibility, one genuine concern is breaking existing scripts. At the very least, if the new DWIM becomes default, there might need to be some escape hatch for scripts to opt out of it. > I guess the second option would be the safer one, and we can still > switch that to be the default at some point if we wish to do so > later. The longer you wait, the less likely you'll have the chance since git-worktree will (presumably) gain more users and be used in more scripts as time passes. So, if the new DWIMing is to become the default, better to do so earlier rather than later. > tl;dr I have no idea which of the options would be better :) I'm probably too cavalier and shortsighted (at least on this topic) to make a well-informed decision about it. Junio probably has a better feeling about whether such a change of behavior makes sense at this late date, and, of course, it's his decision whether to accept such a change into his tree.
(From: Mr.James Tan (Urgent & Confidential)
-- (From: Mr.James Tan (Urgent & Confidential) Good Day, Please Read. My name is Mr.James Tan , I am the Bill and Exchange manager here in Bank of Africa (BOA) Lome-Togo.West-Africa. I have a business proposal in the tune of $9.7m, (Nine Million Seven Hundred Thousand Dollars Only) after the successful transfer, we shall share in ratio of 40% for you and 60% for me. Should you be interested, please contact me through my private email ( tan84...@gmail.com ) so we can commence all arrangements and i will give you more information on how we would handle this project. Please treat this business with utmost confidentiality and send me the Following: 1. Your Full Name: 2. Your Phone Number:.. 3. Your Age: 4. Your Sex:... 5. Your Occupations: 6. Your Country and City: Kind Regards, Mr.James Tan . Bill & Exchange manager (BOA) Bank of Africa. Lome-Togo. Email: tan84...@gmail.com
[PATCH] sequencer: reschedule pick if index can't be locked
From: Phillip Wood Return an error instead of dying if the index cannot be locked in do_recursive_merge() as if the commit cannot be picked it needs to be rescheduled when performing an interactive rebase. If the pick is not rescheduled and the user runs 'git rebase --continue' rather than 'git rebase --abort' then the commit gets silently dropped. Signed-off-by: Phillip Wood --- sequencer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 6d027b06c8d8dc69b14d05752637a65aa121ab24..8c10442b84068d3fb7ec809ef1faa0203cb83e60 100644 --- a/sequencer.c +++ b/sequencer.c @@ -438,7 +438,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next, char **xopt; static struct lock_file index_lock; - hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR); + if (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR)) + return -1; read_cache(); -- 2.15.0
Dear Friend,
Dear Friend, My Seasons of greetings to you and your family. First, I am Mr. Dao ALPHA, a banker working with Bank of Africa here in my country Burkina Faso West Africa, and I have a business transaction that will benefit both of us, and want to know if you can handle and claim the fund to your country's account. The amount is ($10.5Million). After the transfer, we have to share it, 50% for me, and 50% for you. I am a staff working in the same bank here in my country. Please let me know if you can assist me, for more details information on how to proceed, and then claim this money to your country successfully. I hope you will work with me honestly? This business is legal, risk free and will be 100% successful, because I arranged it properly before contacting you. If yes, update me with your personal information such as; 1. Your name in Full: 2. Your House Address: 3. Your Occupation: 4. Your Age: 5. Your direct phone number; Regards. Mr. Dao ALPHA.
[PATCH] config: avoid "write_in_full(fd, buf, len) != len" pattern
From: Phillip Wood
As explained in commit 06f46f237 (avoid "write_in_full(fd, buf, len)
!= len" pattern, 2017–09–13) the return value of write_in_full() is
either -1 or the requested number of bytes. As such comparing the
return value to an unsigned value such as strbuf.len will fail to
catch errors. Change the code to use the preferred '< 0' check.
Signed-off-by: Phillip Wood
---
config.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index
903abf9533b188fd472c213c29a9f968eb90eb8b..d377161113009f394f118d81d27fa6117cde8e9f
100644
--- a/config.c
+++ b/config.c
@@ -2810,7 +2810,7 @@ static int
git_config_copy_or_rename_section_in_file(const char *config_filename
* multiple [branch "$name"] sections.
*/
if (copystr.len > 0) {
- if (write_in_full(out_fd, copystr.buf,
copystr.len) != copystr.len) {
+ if (write_in_full(out_fd, copystr.buf,
copystr.len) < 0) {
ret =
write_error(get_lock_file_path(&lock));
goto out;
}
@@ -2872,7 +2872,7 @@ static int
git_config_copy_or_rename_section_in_file(const char *config_filename
* logic in the loop above.
*/
if (copystr.len > 0) {
- if (write_in_full(out_fd, copystr.buf, copystr.len) !=
copystr.len) {
+ if (write_in_full(out_fd, copystr.buf, copystr.len) < 0) {
ret = write_error(get_lock_file_path(&lock));
goto out;
}
--
2.15.0
[PATCH] Makefile: check that tcl/tk is installed
By default running `make install` in the root directory of the
project will set TCLTK_PATH to `wish` and then go into the "git-gui"
and "gitk-git" sub-directories to build and install these 2
sub-projects.
When Tcl/Tk is not installed, the above will succeed if gettext
is installed, as Tcl/Tk is only required as a substitute for msgfmt
when msgfmt is not installed. But then running the installed gitk
and git-gui will fail.
If neither Tcl/Tk nor gettext are installed, then processing po
files will fail in the git-gui directory. The error message when
this happens is very confusing to new comers as it is difficult
to understand that we tried to use Tcl/Tk as a substitute for
msgfmt, and that the solution is to either install gettext or
Tcl/Tk, or to set both NO_GETTEXT and NO_TCLTK.
To improve the current behavior when Tcl/Tk is not installed,
let's just check that TCLTK_PATH points to something and error
out right away if this is not the case.
This should benefit people who actually want to install and use
git-gui or gitk as they will have to install Tcl/Tk anyway, and
it is better for them if they are told about installing it soon
in the build process, rather than if they have to debug it after
installing.
For people who don't want to use git-gui or gitk, this forces
them to specify NO_TCLTK. If they don't have gettext, this will
make it much easier to fix the problems they would have had to
fix anyway. If they have gettext, setting NO_TCLTK is a small
additional step they will have to make, but it might be a good
thing as it will not install what they don't want and it will
build a bit faster.
Signed-off-by: Christian Couder
---
Makefile | 7 +++
1 file changed, 7 insertions(+)
diff --git a/Makefile b/Makefile
index ee9d5eb11e..ada6164e15 100644
--- a/Makefile
+++ b/Makefile
@@ -1636,6 +1636,13 @@ ifeq ($(TCLTK_PATH),)
NO_TCLTK = NoThanks
endif
+ifndef NO_TCLTK
+ has_tcltk := $(shell type $(TCLTK_PATH) 2>/dev/null)
+ ifndef has_tcltk
+$(error "Tcl/Tk is not installed ('$(TCLTK_PATH)' not found). Consider setting
NO_TCLTK or installing Tcl/Tk")
+ endif
+endif
+
ifeq ($(PERL_PATH),)
NO_PERL = NoThanks
endif
--
2.15.0.165.g42c5887
Re: [PATCH] notes: send "Automatic notes merge failed" messages to stderr
On Tue, Nov 14, 2017 at 5:17 PM, Todd Zullinger wrote: > All other error messages from notes use stderr. Do the same when > alerting users of an unresolved notes merge. > > Fix the output redirection in t3310 and t3320 as well. Previously, the > tests directed output to a file, but stderr was either not captured or > not sent to the file due to the order of the redirection operators. > > Signed-off-by: Todd Zullinger Looks good to me. ...Johan
[RFC] Indicate that Git waits for user input via editor
Hi, Git gathers user input via text editor in certain commands (e.g. "git commit"). If you configure a text based editor, such as vi, then this works great as the editor is opened in your terminal window in the foreground and in focus. However, if you configure an editor that runs outside your terminal window then you might run into the following problem: Git opens the editor but the editor is the background or on another screen and consequently you don't see the editor. You only see the Git command line interface which appears to hang. I wonder if would make sense to print "Opening editor for user input..." or something to the screen to make the user aware of the action. Does this sound sensible to you? Am I missing an existing solution to this problem? Thanks, Lars
[Feature- Request] Option to commit after checking out branch command is made
Hey guys Sometimes I tend to forget to commit changes before I checkout another branch and the following scenario happens (via cli on windows [with git bash]): 1. I checkout a branch, without having commited first > git checkout dev 2. I get this error message: > error: Your local changes to the following files would be overwritten by checkout: > // List of files > // .. > // > Please commit your changes or stash them before you switch branches. But I would rather prefer a scenario like this: 1. I checkout a branch, without having commited first > git checkout dev 2. I get a message like this: > Your local changes to the following files would be overwritten by checkout: > // List of files > // .. > // > Would you want to commit first? (y/n)) IF y --> prompt for commit message and commit automatically IF n --> display error message: "Please commit your changes or stash them before you switch branches" This would be a faster/ more productive way to handle this error. I think it wont be a big challenge, you just should put a message in the cli-tool when this error occurs and call the commit functionality if "y" is pressed. If you'd like I'll even consider doing it myself. If you have some documentation or some tipps on where to look for it. Cheers Ninivaggi Mattia
Re: [PATCH v2 1/2] branch: forbid refs/heads/HEAD
On Tuesday 14 November 2017 08:38 PM, Junio C Hamano wrote: Kaartic Sivaraam writes: I should have been a little more clear with the numbering, sorry. The correct prefix should have been as follows, * [PATCH v2 1/2] --> [PATCH v2 3/3] * [PATCH v2 1/2] --> [PATCH v2 4/3] Sorry for the inconvenience. I assume that the second one above actually talks about what was sent as "v2 2/2" (not "v2 1/2") being "4/3"? Yeah. Copy paste error, sorry. Are these two patches follow-up fixes (replacement of 3/3 plus an extra patch) to jc/branch-name-sanity topic? Yes, that's right. Thanks for working on these. You're welcome. Please do be sure I haven't broken anything in v2. These patches should cleanly apply on 'next', if they don't let me know. Thanks, Kaartic
Re: Changing encoding of a file : What should happen to CRLF in file ?
On Wed, Nov 15, 2017 at 01:41:42PM +0530, Ashish Negi wrote: > > If you commit the file, it will be stored with LF in the index, > This is what i believe is not happening. > > Lets do this with a public repository and steps which are reproducible. > I have created a repo : https://github.com/ashishnegi/git_encoding > > If you clone this repo in linux and run `git status`, you will find > that file is modified. This is what what I get: git ls-files --eol i/lfw/lfattr/text=auto .gitattributes i/crlf w/crlf attr/text=auto file_name.txt (And you get the same, I think) Newer versions of Git (>=2.10) will -not- treat file_name.txt as changed, older versions do. What does "git --version" say on your Linux machine ? However, if you want to fix it, you want to either end up with A) git ls-files --eol i/lfw/lfattr/text=auto .gitattributes i/lfw/crlf attr/text=auto file_name.txt or B) git ls-files --eol i/lfw/lfattr/text=auto .gitattributes i/crlf w/crlf attr/-text file_name.txt (The "attr/-text" means "don't change the line endings") Both A) or B) will work, typically A) is preferred. You should be able to achive A) : git rm --cached file_name.txt && git add file_name.txt rm 'file_name.txt' warning: CRLF will be replaced by LF in file_name.txt. The file will have its original line endings in your working directory. git ls-files --eol i/lfw/lfattr/text=auto .gitattributes i/lfw/crlf attr/text=auto file_name.txt [snip the rest]
Re: submodules and merging (Was: Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue)
On Tue, Nov 14, 2017 at 10:13 AM, Stefan Beller wrote:
> Thanks for your reply!
>
> On Tue, Nov 14, 2017 at 9:17 AM, Elijah Newren wrote:
>> On Mon, Nov 13, 2017 at 3:46 PM, Stefan Beller wrote:
>>> On Mon, Nov 13, 2017 at 3:39 PM, Elijah Newren wrote:
On Mon, Nov 13, 2017 at 2:12 PM, Stefan Beller wrote:
> I wanted to debug a very similar issue today just after reviewing this
> series, see
> https://public-inbox.org/git/743acc29-85bb-3773-b6a0-68d4a0b8f...@ispras.ru/
Oh, bleh. That's not a D/F conflict at all, it's the code assuming
there's a D/F conflict because the entry it is processing ("sub") is a
submodule rather than a file, and it panics when it sees "a directory
in the way" -- a directory that just so happens to be named "sub" and
which is in fact the desired submodule, meaning that the working
directory is already good and needs no changes.
>>>
>>> yup, I came to find the same snippet of code to be the offender,
>>> I just haven't figured out how to fix this bug.
>>>
>>> Thanks for taking a look!
>>>
>>> As you have a lot of fresh knowledge in the merge-recursive case
>>> currently, how would we approach the fix here?
>>
>> submodules and merging looks pretty broken, to me. Backing off from
>> the current bug and just looking at merging with submodules in
>> general, makes me a little uneasy with what I see. I mean, just look
>> at update_file_flags, when it wants the working directory updated, the
>> code for a submodule is the following:
>>
>> if (update_wd) {
>>
>>
>> if (S_ISGITLINK(mode)) {
>> /*
>> * We may later decide to recursively descend into
>> * the submodule directory and update its index
>> * and/or work tree, but we do not do that now.
>> */
>> update_wd = 0;
>> goto update_index;
>> }
>>
>> So, it just doesn't put anything there, so the working directory is
>> made out-of-date immediately. Users are happy with that? Maybe it is
>> what makes sense, but it surprised me.
>
> Submodules are traditionally not touched by git commands and we are slowly
> trying to get that changed. Some commands have a --recurse-submodules
> flag now, including checkout, reset; merge is missing this flag as the
> semantics
> are hard to define sensibly, yet.
>
Yea, figuring out how to represent a conflict with submodules is
pretty challenging, especially in cases where one side has a submodule
and the other side does not.
> Yes, introducing submodules into the mix is a big mess, because
> in the tree it is recorded as if it is a file (only the top level
> entry at path/)
> but on the file system a submodule is represented as a directory
> with contents, so the conflict detection is harder, too.
>
> I had the idea of introducing a command that can "internalize"
> a submodule. This would take the tree recorded in the submodule
> commit and put it where the submodule was mounted,
> The opposite is "externalizing" a submodule, which would turn
> a tree into a submodule. (One of the many question is if it will take
> the whole history or start with a new fresh initial commit).
>
> But this line of though might be distracting from your original point,
> which was that we have so much to keep in mind when doing tree
> operations (flags, D/F conflicts, now submodules too). I wonder how
> a sensible refactoring would look like to detangle all these aspects,
> but still keeping Git fast and not overengineered.
I think given how complex a lot of these code paths are, that an
attempt to refactor it a bit to detangle some of the mess would be
well worth the time. I'd suspect it might make handling the more
complex task of actually resolving conflicts to be easier, so the
effort to clean up the code here should be worth it.
Thanks,
Jake
>
> Thanks,
> Stefan
Re: [RFC] Indicate that Git waits for user input via editor
On Wed, Nov 15, 2017 at 7:08 AM, Lars Schneider wrote: > Hi, > > Git gathers user input via text editor in certain commands (e.g. "git > commit"). > If you configure a text based editor, such as vi, then this works great as the > editor is opened in your terminal window in the foreground and in focus. > > However, if you configure an editor that runs outside your terminal window > then > you might run into the following problem: > Git opens the editor but the editor is the background or on another screen and > consequently you don't see the editor. You only see the Git command line > interface which appears to hang. > > I wonder if would make sense to print "Opening editor for user input..." or > something to the screen to make the user aware of the action. Does this sound > sensible to you? Am I missing an existing solution to this problem? Can this be put in a wrapper that opens the text editor? The wrapper would print these lines and then open the text editor; depending on the operating system, there might even be a command to focus on that editor window. Regarding Git, maybe improve the documentation for how to set the editor variable? > > Thanks, > Lars
Re: [RFC] Indicate that Git waits for user input via editor
> On 15 Nov 2017, at 18:51, Stefan Beller wrote: > > On Wed, Nov 15, 2017 at 7:08 AM, Lars Schneider > wrote: >> Hi, >> >> Git gathers user input via text editor in certain commands (e.g. "git >> commit"). >> If you configure a text based editor, such as vi, then this works great as >> the >> editor is opened in your terminal window in the foreground and in focus. >> >> However, if you configure an editor that runs outside your terminal window >> then >> you might run into the following problem: >> Git opens the editor but the editor is the background or on another screen >> and >> consequently you don't see the editor. You only see the Git command line >> interface which appears to hang. >> >> I wonder if would make sense to print "Opening editor for user input..." or >> something to the screen to make the user aware of the action. Does this sound >> sensible to you? Am I missing an existing solution to this problem? > > Can this be put in a wrapper that opens the text editor? > The wrapper would print these lines and then open the text editor; > depending on the operating system, there might even be a command to > focus on that editor window. Yeah, that would be a workaround. However, in order to take these steps (write the wrapper, enable the focus command) the users needs to understand the problem. That's quite a leap especially for new Git users. They just get the feeling "Git is broken because it hangs". Can you imagine any downside for an "Opening editor for user input..." message? > Regarding Git, maybe improve the documentation for how to set the editor > variable? The sad truth is that 98% of the users do not read the documentation (made up number but close to my observed reality). - Lars
Re: [RFC] Indicate that Git waits for user input via editor
On Wed, Nov 15, 2017 at 10:07 AM, Lars Schneider wrote: >> Can this be put in a wrapper that opens the text editor? >> The wrapper would print these lines and then open the text editor; >> depending on the operating system, there might even be a command to >> focus on that editor window. > > Yeah, that would be a workaround. However, in order to take these steps > (write the > wrapper, enable the focus command) the users needs to understand the problem. > That's quite a leap especially for new Git users. They just get the feeling > "Git > is broken because it hangs". > > Can you imagine any downside for an "Opening editor for user input..." > message? "Wasting precious screen real estate for power users" (tm) So maybe if one can turn it off, this would be ok? Or even for known inline editors? >> Regarding Git, maybe improve the documentation for how to set the editor >> variable? > > The sad truth is that 98% of the users do not read the documentation > (made up number but close to my observed reality). Yeah that is an excellent point, but you forgot the people who look at stackoverflow which of course links back to the docs in the first excellent reply. In a way this suggestion of bettering the docs was a cheap way out, sorry.
Re: [PATCH] sequencer: reschedule pick if index can't be locked
On 15 November 2017 at 11:41, Phillip Wood wrote: > From: Phillip Wood > > Return an error instead of dying if the index cannot be locked in > do_recursive_merge() as if the commit cannot be picked it needs to be > rescheduled when performing an interactive rebase. If the pick is not > rescheduled and the user runs 'git rebase --continue' rather than 'git > rebase --abort' then the commit gets silently dropped. Makes sense. (Your analysis, not the current behavior. ;-) ) > Signed-off-by: Phillip Wood > --- > sequencer.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sequencer.c b/sequencer.c > index > 6d027b06c8d8dc69b14d05752637a65aa121ab24..8c10442b84068d3fb7ec809ef1faa0203cb83e60 > 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -438,7 +438,8 @@ static int do_recursive_merge(struct commit *base, struct > commit *next, > char **xopt; > static struct lock_file index_lock; > > - hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR); > + if (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR)) > + return -1; > > read_cache(); >From the commit message, I would have expected the flags to be zero. This patch does not only turn off the die-ing, it also tells the lockfile-API to print an error message before returning. I don't have an opinion on whether that extra verboseness is good or bad, but if it's wanted, I think the commit message should mention this change. Also, don't you want to check "< 0" rather than "!= 0"? If all goes well, the return value will be a file descriptor. I think that it will always be non-zero, so I think you'll always return -1 here. Martin
Re: Changing encoding of a file : What should happen to CRLF in file ?
On windows : > git --version git version 2.14.2.windows.2 On linux : > git --version git version 2.7.4 I would like to understand the solution : If i understood it correctly : it removes file_name.txt from index, so git forgets about it. we then add the file again after changing encoding. This time, git takes it as utf-8 file and converts crlf to lf when storing it internally. Right ? Thank you for the support. On Wed, Nov 15, 2017 at 10:42 PM, Torsten Bögershausen wrote: > On Wed, Nov 15, 2017 at 01:41:42PM +0530, Ashish Negi wrote: >> > If you commit the file, it will be stored with LF in the index, >> This is what i believe is not happening. >> >> Lets do this with a public repository and steps which are reproducible. >> I have created a repo : https://github.com/ashishnegi/git_encoding >> >> If you clone this repo in linux and run `git status`, you will find >> that file is modified. > > This is what what I get: > > git ls-files --eol > i/lfw/lfattr/text=auto .gitattributes > i/crlf w/crlf attr/text=auto file_name.txt > > (And you get the same, I think) > Newer versions of Git (>=2.10) will -not- treat file_name.txt as changed, > older versions do. > What does "git --version" say on your Linux machine ? > > However, if you want to fix it, you want to either end up with > > A) > git ls-files --eol > i/lfw/lfattr/text=auto .gitattributes > i/lfw/crlf attr/text=auto file_name.txt > > or > B) > git ls-files --eol > i/lfw/lfattr/text=auto .gitattributes > i/crlf w/crlf attr/-text file_name.txt > > (The "attr/-text" means "don't change the line endings") > > Both A) or B) will work, typically A) is preferred. > > You should be able to achive A) : > git rm --cached file_name.txt && git add file_name.txt > rm 'file_name.txt' > warning: CRLF will be replaced by LF in file_name.txt. > The file will have its original line endings in your working directory. > > git ls-files --eol > i/lfw/lfattr/text=auto .gitattributes > i/lfw/crlf attr/text=auto file_name.txt > > [snip the rest]
Re: [PATCH] Reduce performance penalty for turned off traces
On Sun, Nov 12, 2017 at 6:17 AM, Jeff King wrote: > On Sat, Nov 11, 2017 at 07:28:58PM +, gennady.kup...@gmail.com wrote: > >> From: Gennady Kupava >> >> Signed-off-by: Gennady Kupava > > Thanks, and welcome to the list. Welcome to the list! > I did manually disable HAVE_VARIADIC_MACROS and confirmed that the > result builds and passes the test suite (though I suspect that GIT_TRACE > is not well exercised by the suite). GIT_TRACE is exercised in the test suite (though I am not sure if it counts as well-exercised) in t7406-submodule-update.sh for example, which uses GIT_TRACE to obtain information about thread parallelism used by Git, as that is not observable otherwise, if we assume that performance tests in the standard test suite are not feasible. > I tried timing a simple loop like: > > Without your patch, the times for GIT_TRACE=1 and GIT_TRACE=0 are about > 500ms and 9ms respectively. > > After your patch, the GIT_TRACE=1 time remains the same but GIT_TRACE=0 > drops to 1ms. So does that mean we can use a lot more tracing now? Thanks, Stefan
Re: [PATCH 12/30] directory rename detection: miscellaneous testcases to complete coverage
On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren wrote:
> +###
> +# SECTION 9: Other testcases
> +#
> +# I came up with the testcases in the first eight sections before coding up
> +# the implementation. The testcases in this section were mostly ones I
> +# thought of while coding/debugging, and which I was too lazy to insert
> +# into the previous sections because I didn't want to re-label with all the
> +# testcase references. :-)
This might also be commit message material, as it describes the workflow,
not the 'misc' aspect of these test cases.
> +###
> +
> +# Testcase 9a, Inner renamed directory within outer renamed directory
> +# (Related to testcase 1f)
> +# Commit A: z/{b,c,d/{e,f,g}}
> +# Commit B: y/{b,c}, x/w/{e,f,g}
> +# Commit C: z/{b,c,d/{e,f,g,h},i}
> +# Expected: y/{b,c,i}, x/w/{e,f,g,h}
> +# NOTE: The only reason this one is interesting is because when a directory
> +# is split into multiple other directories, we determine by the
> weight
> +# of which one had the most paths going to it. A naive
> implementation
> +# of that could take the new file in commit C at z/i to x/w/i or x/i.
Makes sense.
> +# Testcase 9b, Transitive rename with content merge
> +# (Related to testcase 1c)
> +# Commit A: z/{b,c}, x/d_1
> +# Commit B: y/{b,c}, x/d_2
> +# Commit C: z/{b,c,d_3}
> +# Expected: y/{b,c,d_merged}
Makes sense.
> +# Testcase 9c, Doubly transitive rename?
> +# (Related to testcase 1c, 7e, and 9d)
> +# Commit A: z/{b,c}, x/{d,e},w/f
> +# Commit B: y/{b,c}, x/{d,e,f,g}
> +# Commit C: z/{b,c,d,e}, w/f
> +# Expected: y/{b,c,d,e}, x/{f,g}
> +#
> +# NOTE: x/f and x/g may be slightly confusing here. The rename from w/f to
> +# x/f is clear. Let's look beyond that. Here's the logic:
> +#Commit C renamed x/ -> z/
> +#Commit B renamed z/ -> y/
> +# So, we could possibly further rename x/f to z/f to y/f, a doubly
> +# transient rename. However, where does it end? We can chain these
> +# indefinitely (see testcase 9d). What if there is a D/F conflict
> +# at z/f/ or y/f/? Or just another file conflict at one of those
> +# paths? In the case of an N-long chain of transient renamings,
> +# where do we "abort" the rename at? Can the user make sense of
> +# the resulting conflict and resolve it?
> +#
> +# To avoid this confusion I use the simple rule that if the other
> side
> +# of history did a directory rename to a path that your side renamed
> +# away, then ignore that particular rename from the other side of
> +# history for any implicit directory renames.
This is repeated in the rule of section 9 below.
Makes sense.
> +# Testcase 9d, N-fold transitive rename?
> +# (Related to testcase 9c...and 1c and 7e)
> +# Commit A: z/a, y/b, x/c, w/d, v/e, u/f
> +# Commit B: y/{a,b}, w/{c,d}, u/{e,f}
> +# Commit C: z/{a,t}, x/{b,c}, v/{d,e}, u/f
> +# Expected:
> +#
> +# NOTE: z/ -> y/ (in commit B)
> +# y/ -> x/ (in commit C)
> +# x/ -> w/ (in commit B)
> +# w/ -> v/ (in commit C)
> +# v/ -> u/ (in commit B)
> +# So, if we add a file to z, say z/t, where should it end up? In u?
> +# What if there's another file or directory named 't' in one of the
> +# intervening directories and/or in u itself? Also, shouldn't the
> +# same logic that places 't' in u/ also move ALL other files to u/?
> +# What if there are file or directory conflicts in any of them? If
> +# we attempted to do N-way (N-fold? N-ary? N-uple?) transitive
> renames
> +# like this, would the user have any hope of understanding any
> +# conflicts or how their working tree ended up? I think not, so I'm
> +# ruling out N-ary transitive renames for N>1.
> +#
> +# Therefore our expected result is:
> +# z/t, y/a, x/b, w/c, u/d, u/e, u/f
> +# The reason that v/d DOES get transitively renamed to u/d is that u/ isn't
> +# renamed somewhere. A slightly sub-optimal result, but it uses fairly
> +# simple rules that are consistent with what we need for all the other
> +# testcases and simplifies things for the user.
Does the merge order matter here?
If B and C were swapped, applying the same logic presented in the NOTE,
one could argue that we expect:
z/t y/a x/b w/c v/d v/e u/f
I can make a strong point for y/a here, but the v/{d,e} also seem to deviate.
> +# Testcase 9e, N-to-1 whammo
> +# (Related to testcase 9c...and 1c and 7e)
> +# Commit A: dir1/{a,b}, dir2/{d,e}, dir3/{g,h}, dirN/{j,k}
> +# Commit B: dir1/{a,b,c,yo}, dir2/{d,e,f,yo}, dir3/{g,h,i,yo},
> dirN/{j,k,l,yo}
> +# Commit C: combined/{a,b,d,e,g,h,j,k}
> +# Expected: combined/{a,b
Re: [PATCH 27/30] merge-recursive: Apply necessary modifications for directory renames
> + if (!strcmp(pair->one->path, pair->two->path)) {
> + /*
> +* Paths should only match if this was initially a
> +* non-rename that is being turned into one by
> +* directory rename detection.
> +*/
> + assert(pair->status != 'R');
> + } else {
> + assert(pair->status == 'R');
assert() is compiled conditionally depending on whether
NDEBUG is set, which makes potential error reports more interesting and
head-scratching. But we'd rather prefer easy bug reports, therefore
we'd want to (a) either have the condition checked always, when
you know this could occur, e.g. via
if ()
BUG("Git is broken, because...");
or (b) you could omit the asserts if they are more of a developer guideline?
I wonder if we want to introduce a BUG_ON(, ) macro
that contains (a).
> + re->dst_entry->processed = 1;
> + //string_list_remove(entries, pair->two->path, 0);
commented code?
Re: [PATCH 1/5] connect: split git:// setup into a separate function
Hi,
On Oct 24, 2017, Junio C Hamano wrote:
> Jonathan Nieder writes:
>> +static struct child_process *git_connect_git(int fd[2], char *hostandport,
>> + const char *path, const char *prog,
>> + int flags)
>> +{
>> +struct child_process *conn = &no_fork;
>> +struct strbuf request = STRBUF_INIT;
>
> As this one decides what "conn" to return, including the fallback
> &no_fork instance,...
>
>> +...
>> +return conn;
>> +}
>> +
>> /*
>> * This returns a dummy child_process if the transport protocol does not
>> * need fork(2), or a struct child_process object if it does. Once done,
>> @@ -881,50 +939,7 @@ struct child_process *git_connect(int fd[2], const char
>> *url,
>
> Each of the if/elseif/ cascade, one of which calls the new helper,
> now makes an explicit assignment to "conn" declared in
> git_connect().
>
> Which means the defaulting of git_connect::conn to &no_fork is now
> unneeded. One of the things that made the original cascade a bit
> harder to follow than necessary, aside from the physical length of
> the PROTO_GIT part, was that the case where conn remains to point at
> no_fork looked very special and it was buried in that long PROTO_GIT
> part.
Good idea. Here's what I'll include in the reroll.
-- >8 --
Subject: connect: move no_fork fallback to git_tcp_connect
git_connect has the structure
struct child_process *conn = &no_fork;
...
switch (protocol) {
case PROTO_GIT:
if (git_use_proxy(hostandport))
conn = git_proxy_connect(fd, hostandport);
else
git_tcp_connect(fd, hostandport, flags);
...
break;
case PROTO_SSH:
conn = xmalloc(sizeof(*conn));
child_process_init(conn);
argv_array_push(&conn->args, ssh);
...
break;
...
return conn;
In all cases except the git_tcp_connect case, conn is explicitly
assigned a value. Make the code clearer by explicitly assigning
'conn = &no_fork' in the tcp case and eliminating the default so the
compiler can ensure conn is always correctly assigned.
Noticed-by: Junio C Hamano
Signed-off-by: Jonathan Nieder
---
connect.c | 22 --
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/connect.c b/connect.c
index 7fbd396b35..b6accf71cb 100644
--- a/connect.c
+++ b/connect.c
@@ -582,12 +582,21 @@ static int git_tcp_connect_sock(char *host, int flags)
#endif /* NO_IPV6 */
-static void git_tcp_connect(int fd[2], char *host, int flags)
+static struct child_process no_fork = CHILD_PROCESS_INIT;
+
+int git_connection_is_socket(struct child_process *conn)
+{
+ return conn == &no_fork;
+}
+
+static struct child_process *git_tcp_connect(int fd[2], char *host, int flags)
{
int sockfd = git_tcp_connect_sock(host, flags);
fd[0] = sockfd;
fd[1] = dup(sockfd);
+
+ return &no_fork;
}
@@ -761,8 +770,6 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
return protocol;
}
-static struct child_process no_fork = CHILD_PROCESS_INIT;
-
static const char *get_ssh_command(void)
{
const char *ssh;
@@ -865,7 +872,7 @@ struct child_process *git_connect(int fd[2], const char
*url,
const char *prog, int flags)
{
char *hostandport, *path;
- struct child_process *conn = &no_fork;
+ struct child_process *conn;
enum protocol protocol;
/* Without this we cannot rely on waitpid() to tell
@@ -901,7 +908,7 @@ struct child_process *git_connect(int fd[2], const char
*url,
if (git_use_proxy(hostandport))
conn = git_proxy_connect(fd, hostandport);
else
- git_tcp_connect(fd, hostandport, flags);
+ conn = git_tcp_connect(fd, hostandport, flags);
/*
* Separate original protocol components prog and path
* from extended host header with a NUL byte.
@@ -1041,11 +1048,6 @@ struct child_process *git_connect(int fd[2], const char
*url,
return conn;
}
-int git_connection_is_socket(struct child_process *conn)
-{
- return conn == &no_fork;
-}
-
int finish_connect(struct child_process *conn)
{
int code;
--
2.15.0.448.gf294e3d99a
Re: [PATCH] notes: send "Automatic notes merge failed" messages to stderr
Johan Herland wrote: On Tue, Nov 14, 2017 at 5:17 PM, Todd Zullinger wrote: All other error messages from notes use stderr. Do the same when alerting users of an unresolved notes merge. Fix the output redirection in t3310 and t3320 as well. Previously, the tests directed output to a file, but stderr was either not captured or not sent to the file due to the order of the redirection operators. Signed-off-by: Todd Zullinger Looks good to me. Thanks Johan. -- Todd ~~ Deliberation, n. The act of examining one's bread to determine which side it is buttered on. -- Ambrose Bierce, "The Devil's Dictionary"
Re: Bug in "revision.c: --all adds HEAD from all worktrees" ?
+Jeff King On 13 November 2017 at 22:15, Stefan Beller wrote: > On Mon, Nov 13, 2017 at 2:03 PM, Luke Diamand wrote: >> On 13 November 2017 at 19:51, Luke Diamand wrote: >>> Hi! >>> >>> I think there may be a regression caused by this change which means >>> that "git fetch origin" doesn't work: >>> >>> commit d0c39a49ccb5dfe7feba4325c3374d99ab123c59 (refs/bisect/bad) >>> Author: Nguyễn Thái Ngọc Duy >>> Date: Wed Aug 23 19:36:59 2017 +0700 >>> >>> revision.c: --all adds HEAD from all worktrees >>> >>> $ git fetch origin >>> fatal: bad object HEAD >>> error: ssh://my_remote_host/reponame did not send all necessary objects >>> >>> I used git bisect to find the problem, and it seems pretty consistent. >>> "git fetch" with the previous revision works fine. >>> >>> FWIW, I've got a lot of git worktrees associated with this repo, so >>> that may be why it's failing. The remote repo is actually a git-p4 >>> clone, so HEAD there actually ends up pointing at >>> refs/remote/p4/master. >>> >>> Thanks, >>> Luke >> >> Quite a few of the worktrees have expired - their head revision has >> been GC'd and no longer points to anything sensible >> (gc.worktreePruneExpire). The function other_head_refs() in worktree.c >> bails out if there's an error, which I think is the problem. I wonder >> if it should instead just report something and then keep going. > > Also see > https://public-inbox.org/git/cagz79kyp0z1g_h3nwfmshrawhmbocik5lepuxkj0nveebri...@mail.gmail.com/ So is this a bug or user error on my part? Surely at the very least "git fetch" shouldn't give a cryptic error message just because one of my git worktrees has expired!
Re: [PATCH] sequencer: reschedule pick if index can't be locked
Hi Phillip, On Wed, 15 Nov 2017, Phillip Wood wrote: > diff --git a/sequencer.c b/sequencer.c > index > 6d027b06c8d8dc69b14d05752637a65aa121ab24..8c10442b84068d3fb7ec809ef1faa0203cb83e60 > 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -438,7 +438,8 @@ static int do_recursive_merge(struct commit *base, struct > commit *next, > char **xopt; > static struct lock_file index_lock; > > - hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR); > + if (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR)) If you test the return value for *negative* values, I am fully on board with the change. As far as I understand the code, hold_locked_index() returns -1 on error, but *a file descriptor* (which is usually not 0) upon success... Ciao, Dscho
Re: Bug in "revision.c: --all adds HEAD from all worktrees" ?
Luke Diamand writes: > Quite a few of the worktrees have expired - their head revision has > been GC'd and no longer points to anything sensible > (gc.worktreePruneExpire). The function other_head_refs() in worktree.c > bails out if there's an error, which I think is the problem. I wonder > if it should instead just report something and then keep going. Am I correct to understand that your "git fsck" would fail because these HEAD refs used by other stale worktrees are pointing at missing objects? What do you mean by "expired"? "Even though I want to keep using them, Git for some reason decided to destroy them." or "I no longer use them but kept them lying around."? If the latter, I wonder "worktree prune" to remove the admininstrative information for them would unblock you?
Re: [PATCH V2] config: add --expiry-date
On 2017-11-14 06:38, Junio C Hamano wrote:
h...@unimetic.com writes:
From: Haaris
Description:
This patch adds a new option to the config command.
...
Motivation:
A parse_expiry_date() function already existed for api calls,
this patch simply allows the function to be used from the command
line.
Signed-off-by: Haaris
---
Please drop all these section headers; they are irritating. Learn
from "git log --no-merges" how the log messages in this project is
written and imitate them. Documentation/SubmittingPatches would be
helpful.
Add --expiry-date as a new type 'git config --get' takes,
similar to existing --int, --bool, etc. types, so that
scripts can learn values of configuration variables like
gc.reflogexpire (e.g. "2.weeks") in a more useful way
(e.g. the timesamp as of two weeks ago, expressed in number
of seconds since epoch).
As a helper function necessary to do this already exists in
the implementation of builtin/reflog.c, the implementation
is just the matter of moving it to config.c and using it
from bultin/config.c, but shuffle the order of the parameter
so that the pointer to the output variable comes first.
This is to match the convention used by git_config_pathname()
and other helper functions.
or something like that?
Hi,
I am sorry for not following the format properly. I will change this for
next patch update.
+ } else if (types == TYPE_EXPIRY_DATE) {
+ timestamp_t t;
+ if(git_config_expiry_date(&t, key_, value_) < 0)
Style.
Sure.
if (git_config_expiry_date(&t, key_, value_) < 0)
+ return -1;
+ strbuf_addf(buf, "%"PRItime, t);
...
Thanks.
Kind Regards,
Haaris
[PATCH 3/4] branch: correctly reject refs/heads/{-dash,HEAD}
strbuf_check_branch_ref() is the central place where many codepaths
see if a proposed name is suitable for the name of a branch. It was
designed to allow us to get stricter than the check_refname_format()
check used for refnames in general, and we already use it to reject
a branch whose name begins with a '-'. The function gets a strbuf
and a string "name", and returns non-zero if the name is not
appropriate as the name for a branch. When the name is good, it
places the full refname for the branch with the proposed name in the
strbuf before it returns.
However, it turns out that one caller looks at what is in the strbuf
even when the function returns an error. Make the function populate
the strbuf even when it returns an error. That way, when "-dash" is
given as name, "refs/heads/-dash" is placed in the strbuf when
returning an error to copy_or_rename_branch(), which notices that
the user is trying to recover with "git branch -m -- -dash dash" to
rename "-dash" to "dash".
While at it, use the same mechanism to also reject "HEAD" as a
branch name.
Helped-by: Jeff King
Helped-by: Kaartic Sivaraam
Signed-off-by: Junio C Hamano
---
Kaartic Sivaraam writes:
>> Are these two patches follow-up fixes (replacement of 3/3 plus an
>> extra patch) to jc/branch-name-sanity topic?
>
> Yes, that's right.
>
>> Thanks for working on these.
>
> You're welcome. Please do be sure I haven't broken anything in
> v2. These patches should cleanly apply on 'next', if they don't let me
> know.
OK, so here is a replacement for your replacement, based on an
additional analysis I did while I was reviewing your changes.
The final 4/4 is what you sent as [v2 2/2] (which was meant to
be [v2 4/3]). I think with these updates, the resulting 4-patch
series is good for 'next'.
Thanks again.
sha1_name.c | 14 --
t/t1430-bad-ref-name.sh | 43 +++
2 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index c7c5ab376c..67961d6e47 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1332,9 +1332,19 @@ void strbuf_branchname(struct strbuf *sb, const char
*name, unsigned allowed)
int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
{
strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
- if (name[0] == '-')
- return -1;
+
+ /*
+* This splice must be done even if we end up rejecting the
+* name; builtin/branch.c::copy_or_rename_branch() still wants
+* to see what the name expanded to so that "branch -m" can be
+* used as a tool to correct earlier mistakes.
+*/
strbuf_splice(sb, 0, 0, "refs/heads/", 11);
+
+ if (*name == '-' ||
+ !strcmp(sb->buf, "refs/heads/HEAD"))
+ return -1;
+
return check_refname_format(sb->buf, 0);
}
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index e88349c8a0..c7878a60ed 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -331,4 +331,47 @@ test_expect_success 'update-ref --stdin -z fails delete
with bad ref name' '
grep "fatal: invalid ref format: ~a" err
'
+test_expect_success 'branch rejects HEAD as a branch name' '
+ test_must_fail git branch HEAD HEAD^ &&
+ test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'checkout -b rejects HEAD as a branch name' '
+ test_must_fail git checkout -B HEAD HEAD^ &&
+ test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'update-ref can operate on refs/heads/HEAD' '
+ git update-ref refs/heads/HEAD HEAD^ &&
+ git show-ref refs/heads/HEAD &&
+ git update-ref -d refs/heads/HEAD &&
+ test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'branch -d can remove refs/heads/HEAD' '
+ git update-ref refs/heads/HEAD HEAD^ &&
+ git branch -d HEAD &&
+ test_must_fail git show-ref refs/heads/HEAD
+'
+
+test_expect_success 'branch -m can rename refs/heads/HEAD' '
+ git update-ref refs/heads/HEAD HEAD^ &&
+ git branch -m HEAD tail &&
+ test_must_fail git show-ref refs/heads/HEAD &&
+ git show-ref refs/heads/tail
+'
+
+test_expect_success 'branch -d can remove refs/heads/-dash' '
+ git update-ref refs/heads/-dash HEAD^ &&
+ git branch -d -- -dash &&
+ test_must_fail git show-ref refs/heads/-dash
+'
+
+test_expect_success 'branch -m can rename refs/heads/-dash' '
+ git update-ref refs/heads/-dash HEAD^ &&
+ git branch -m -- -dash dash &&
+ test_must_fail git show-ref refs/heads/-dash &&
+ git show-ref refs/heads/dash
+'
+
test_done
--
2.15.0-358-g6c105002b3
Re: [RFC] Indicate that Git waits for user input via editor
Lars Schneider writes:
> However, if you configure an editor that runs outside your terminal window
> then
> you might run into the following problem:
> Git opens the editor but the editor is the background or on another screen
> and
> consequently you don't see the editor. You only see the Git command line
> interface which appears to hang.
>
> I wonder if would make sense to print "Opening editor for user input..." or
> something to the screen to make the user aware of the action. Does this sound
> sensible to you? Am I missing an existing solution to this problem?
My knee-jerk reaction was: for such a user who has EDITOR set to a
program that pops under, wouldn't any program, not just Git, that
uses the editor to open a file for editing/viewing look broken?
Would we care if we are called "broken" by such a clueless user who
cannot tell a (non-)broken caller of an editor and a broken editor?
But that is true only when the user does realize/expect that the
program s/he is running _will_ open an editor at the point of the
workflow. If s/he types "git merge" or "git rebase -i @{u}", for
example, it is true that the world would be a better place if s/he
knows that would ask a file to be edited with an editor, but it is
unrealisic to expect that everybody knows how to operate these
commands. Everybody is a newbie at least once.
I wonder if we can do something like
git_spawn_editor()
{
const char *EL = "\033[K"; /* Erase in Line */
/* notice the lack of terminating LF */
fprintf(stderr, "Launching your editor...");
fflush(stderr);
if (!run_command(... spawn the editor ...)) {
/* Success! - go back and erase the whole line */
fprintf(stderr, "\r%s", EL);
} else {
fprintf(stderr, "failed (%s)\n", strerror(errno));
}
fflush(stderr);
}
to tentatively give a message without permanently wasting the
vertical space.
[PATCH V3] config: add --expiry-date
From: Haaris Mehmood
Add --expiry-date as a data-type for config files when
'git config --get' is used. This will return any relative
or fixed dates from config files as a timestamp value.
This is useful for scripts (e.g. gc.reflogexpire) that work
with timestamps so that '2.weeks' can be converted to a format
acceptable by those scripts/functions.
Following the convention of git_config_pathname(), move
the helper function required for this feature from
builtin/reflog.c to builtin/config.c where other similar
functions exist (e.g. for --bool or --path), and match
the order of parameters with other functions (i.e. output
pointer as first parameter).
Signed-off-by: Haaris Mehmood
---
Documentation/git-config.txt | 5 +
builtin/config.c | 10 +-
builtin/reflog.c | 14 ++
config.c | 9 +
config.h | 1 +
t/helper/test-date.c | 12
t/t1300-repo-config.sh | 30 ++
7 files changed, 68 insertions(+), 13 deletions(-)
update v3: fix style issue
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 4edd09fc6..14da5fc15 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -180,6 +180,11 @@ See also <>.
value (but you can use `git config section.variable ~/`
from the command line to let your shell do the expansion).
+--expiry-date::
+ `git config` will ensure that the output is converted from
+ a fixed or relative date-string to a timestamp. This option
+ has no effect when setting the value.
+
-z::
--null::
For all options that output values and/or keys, always
diff --git a/builtin/config.c b/builtin/config.c
index d13daeeb5..ab5f95476 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -52,6 +52,7 @@ static int show_origin;
#define TYPE_INT (1<<1)
#define TYPE_BOOL_OR_INT (1<<2)
#define TYPE_PATH (1<<3)
+#define TYPE_EXPIRY_DATE (1<<4)
static struct option builtin_config_options[] = {
OPT_GROUP(N_("Config file location")),
@@ -80,6 +81,7 @@ static struct option builtin_config_options[] = {
OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"),
TYPE_BOOL_OR_INT),
OPT_BIT(0, "path", &types, N_("value is a path (file or directory
name)"), TYPE_PATH),
+ OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"),
TYPE_EXPIRY_DATE),
OPT_GROUP(N_("Other")),
OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
@@ -159,6 +161,11 @@ static int format_config(struct strbuf *buf, const char
*key_, const char *value
return -1;
strbuf_addstr(buf, v);
free((char *)v);
+ } else if (types == TYPE_EXPIRY_DATE) {
+ timestamp_t t;
+ if (git_config_expiry_date(&t, key_, value_) < 0)
+ return -1;
+ strbuf_addf(buf, "%"PRItime, t);
} else if (value_) {
strbuf_addstr(buf, value_);
} else {
@@ -273,12 +280,13 @@ static char *normalize_value(const char *key, const char
*value)
if (!value)
return NULL;
- if (types == 0 || types == TYPE_PATH)
+ if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)
/*
* We don't do normalization for TYPE_PATH here: If
* the path is like ~/foobar/, we prefer to store
* "~/foobar/" in the config file, and to expand the ~
* when retrieving the value.
+* Also don't do normalization for expiry dates.
*/
return xstrdup(value);
if (types == TYPE_INT)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index ab31a3b6a..223372531 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -416,16 +416,6 @@ static struct reflog_expire_cfg *find_cfg_ent(const char
*pattern, size_t len)
return ent;
}
-static int parse_expire_cfg_value(const char *var, const char *value,
timestamp_t *expire)
-{
- if (!value)
- return config_error_nonbool(var);
- if (parse_expiry_date(value, expire))
- return error(_("'%s' for '%s' is not a valid timestamp"),
-value, var);
- return 0;
-}
-
/* expiry timer slot */
#define EXPIRE_TOTAL 01
#define EXPIRE_UNREACH 02
@@ -443,11 +433,11 @@ static int reflog_expire_config(const char *var, const
char *value, void *cb)
if (!strcmp(key, "reflogexpire")) {
slot = EXPIRE_TOTAL;
- if (parse_expire_cfg_value(var, value, &expire))
+ if (
Re: [RFC] Indicate that Git waits for user input via editor
On Wed, Nov 15, 2017 at 2:32 PM, Junio C Hamano wrote:
> Lars Schneider writes:
>
>> However, if you configure an editor that runs outside your terminal window
>> then
>> you might run into the following problem:
>> Git opens the editor but the editor is the background or on another screen
>> and
>> consequently you don't see the editor. You only see the Git command line
>> interface which appears to hang.
>>
>> I wonder if would make sense to print "Opening editor for user input..." or
>> something to the screen to make the user aware of the action. Does this sound
>> sensible to you? Am I missing an existing solution to this problem?
>
> My knee-jerk reaction was: for such a user who has EDITOR set to a
> program that pops under, wouldn't any program, not just Git, that
> uses the editor to open a file for editing/viewing look broken?
> Would we care if we are called "broken" by such a clueless user who
> cannot tell a (non-)broken caller of an editor and a broken editor?
>
> But that is true only when the user does realize/expect that the
> program s/he is running _will_ open an editor at the point of the
> workflow. If s/he types "git merge" or "git rebase -i @{u}", for
> example, it is true that the world would be a better place if s/he
> knows that would ask a file to be edited with an editor, but it is
> unrealisic to expect that everybody knows how to operate these
> commands. Everybody is a newbie at least once.
>
> I wonder if we can do something like
>
> git_spawn_editor()
> {
> const char *EL = "\033[K"; /* Erase in Line */
>
> /* notice the lack of terminating LF */
> fprintf(stderr, "Launching your editor...");
"It takes quite some time to launch this special Git Editor"
As Lars pointed out, the editor may be launched in the background,
that the user would not know, but they might expect a thing to
pop up as a modal dialog as is always with UIs.
So despite it being technically wrong at this point in time,
I would phrase it in past tense or in a way that indicates that the
user needs to take action already.
The "Launching..." sounds as if I need to wait for an event to occur.
> fflush(stderr);
>
> if (!run_command(... spawn the editor ...)) {
> /* Success! - go back and erase the whole line */
> fprintf(stderr, "\r%s", EL);
> } else {
> fprintf(stderr, "failed (%s)\n", strerror(errno));
> }
> fflush(stderr);
> }
>
> to tentatively give a message without permanently wasting the
> vertical space.
Re: [Feature- Request] Option to commit after checking out branch command is made
Ninivaggi Mattia writes: > 1. I checkout a branch, without having commited first > > git checkout dev > 2. I get this error message: > > error: Your local changes to the following files would be overwritten > by checkout: > > // List of files > > // .. > > // > > Please commit your changes or stash them before you switch branches. > > But I would rather prefer a scenario like this: > ... > 1. I checkout a branch, without having commited first > > git checkout dev > 2. I get a message like this: > > Your local changes to the following files would be overwritten by > checkout: > > // List of files > > // .. > > // > > Would you want to commit first? (y/n)) > > IF y --> prompt for commit message and commit automatically I do not think you want to do this for a few reasons. * The "please commit or stash" is merely a suggestion whose primary purpose is to silence clueless newbies who would have complained "Git said 'error: ... overwritten by checkout' and I do not know what to do next; the error message is so unhelpful" otherwise. Majority of the time when I see this message, it is because I forgot that I was in the middle of doing something (meaning: I am far from finished with the changes I was working on), and I would not be ready to commit. I'd rather keep working to get the work into a more reasonable shape before committing, or stash the changes first if the task I wanted to do on that "dev" branch is more urgent and what I was in the middle of doing is lower priority. Because of this, I would expect many users (including the ones who are right now newbies but will have gained experience to become experts in the future) to appreciate "stash before switch" a lot more than "commit first before switch". * People write scripts that use "git checkout" to switch branches, and they rely on the command to fail in this situation, instead of going interactive and gets stuck waiting for an input (which may never come). Because of this, the updated behaviour you propose must never be the default, and at least must be protected behind a flag, something like "git checkout --autostash dev" (or "--autocommit", if you insist). With that, you could do [alias] co = checkout --autostash and train your fingers to say "git co dev". Of course, you can have a "git-co" script on your $PATH, which runs "git checkout $1", lets it fail just like it does now, and then does "git commit", if you really want the behaviour. Again, you can then use "git co dev" and you do not have to worry about breaking people's scripts that depends on "git checkout" to fail without going interactive.
Git on Mac - Segmentation fault:11
I am using Git on a Macbook pro with MacOS High Sierra version 10.13.1 (17B48). I have been using it in a virtualenv with python 3. I have begun to get "Segmentation fault: 11" with every git command. I have been searching for a reason why this is occurring but have not been able to find a solution. I have reinstalled the operating system, uninstalled and reinstalled Git, and a variety of other attempts at finding a solution. Is this a know issue? Any suggestions would be appreciated.
Re: [RFC] Indicate that Git waits for user input via editor
Stefan Beller writes:
>> I wonder if we can do something like
>>
>> git_spawn_editor()
>> {
>> const char *EL = "\033[K"; /* Erase in Line */
>>
>> /* notice the lack of terminating LF */
>> fprintf(stderr, "Launching your editor...");
>
> "It takes quite some time to launch this special Git Editor"
>
> As Lars pointed out, the editor may be launched in the background,
> that the user would not know, but they might expect a thing to
> pop up as a modal dialog as is always with UIs.
>
> So despite it being technically wrong at this point in time,
> I would phrase it in past tense or in a way that indicates that the
> user needs to take action already.
>
> The "Launching..." sounds as if I need to wait for an event to occur.
Heh, I wasn't expecting phrasing nitpicks when I was trying to help
the thread by trying to come up with a way to avoid vertical space
wastage.
Re: [RFC] Indicate that Git waits for user input via editor
On Wed, Nov 15, 2017 at 4:33 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>>> I wonder if we can do something like
>>>
>>> git_spawn_editor()
>>> {
>>> const char *EL = "\033[K"; /* Erase in Line */
>>>
>>> /* notice the lack of terminating LF */
>>> fprintf(stderr, "Launching your editor...");
>>
>> "It takes quite some time to launch this special Git Editor"
>>
>> As Lars pointed out, the editor may be launched in the background,
>> that the user would not know, but they might expect a thing to
>> pop up as a modal dialog as is always with UIs.
>>
>> So despite it being technically wrong at this point in time,
>> I would phrase it in past tense or in a way that indicates that the
>> user needs to take action already.
>>
>> The "Launching..." sounds as if I need to wait for an event to occur.
>
> Heh, I wasn't expecting phrasing nitpicks when I was trying to help
> the thread by trying to come up with a way to avoid vertical space
> wastage.
I know you weren't, but maybe it is helpful for the author of the patch
(I presume you may not be the author, after all).
But you are right, I should have started with a more fundamental
answer stating this is a good idea, and I cannot think of a negative
side effect currently.
Re: [PATCH V3] config: add --expiry-date
h...@unimetic.com writes:
> From: Haaris Mehmood
>
> Add --expiry-date as a data-type for config files when
> 'git config --get' is used. This will return any relative
> or fixed dates from config files as a timestamp value.
>
> This is useful for scripts (e.g. gc.reflogexpire) that work
> with timestamps so that '2.weeks' can be converted to a format
> acceptable by those scripts/functions.
>
> Following the convention of git_config_pathname(), move
> the helper function required for this feature from
> builtin/reflog.c to builtin/config.c where other similar
> functions exist (e.g. for --bool or --path), and match
> the order of parameters with other functions (i.e. output
> pointer as first parameter).
>
> Signed-off-by: Haaris Mehmood
Very nicely explained. I often feel irritated when people further
rewrite what I wrote for them as an example and make it much worse,
but this one definitely is a lot more readable than the "something
like this perhaps?" in my response to the previous round.
> @@ -273,12 +280,13 @@ static char *normalize_value(const char *key, const
> char *value)
> if (!value)
> return NULL;
>
> - if (types == 0 || types == TYPE_PATH)
> + if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)
> /*
>* We don't do normalization for TYPE_PATH here: If
>* the path is like ~/foobar/, we prefer to store
>* "~/foobar/" in the config file, and to expand the ~
>* when retrieving the value.
> + * Also don't do normalization for expiry dates.
>*/
> return xstrdup(value);
Sensible. Just like we want to save "~u/path" as-is without
expanding the "~u"/ part, we want to keep "2 weeks ago" as-is.
> - if (parse_expiry_date(value, expire))
> - return error(_("'%s' for '%s' is not a valid timestamp"),
> - value, var);
> ...
> + if (parse_expiry_date(value, timestamp))
> + die(_("failed to parse date_string in: '%s'"), value);
This is an unintended change in behaviour (or at least undocumented
in the log message) for the "git reflog" command, no?
Not just the error message is different, but the original gave the
calling code a chance to react to the failure by returning -1 from
the function, but this makes the command fail outright here.
Would it break anything if you did "return error()" just like the
original used to? Are your callers of this new function not
prepared to see an error return?
Re: Bug in "revision.c: --all adds HEAD from all worktrees" ?
On 15 November 2017 at 22:08, Junio C Hamano wrote: > Luke Diamand writes: > >> Quite a few of the worktrees have expired - their head revision has >> been GC'd and no longer points to anything sensible >> (gc.worktreePruneExpire). The function other_head_refs() in worktree.c >> bails out if there's an error, which I think is the problem. I wonder >> if it should instead just report something and then keep going. > > Am I correct to understand that your "git fsck" would fail because > these HEAD refs used by other stale worktrees are pointing at > missing objects? git fsck says: Checking object directories: 100% (256/256), done. Checking objects: 100% (1434634/1434634), done. error: HEAD: invalid reflog entry 7fa2b7ee4bc0d11529f659db8b13ed1f537d2a98 error: HEAD: invalid reflog entry 7fa2b7ee4bc0d11529f659db8b13ed1f537d2a98 error: HEAD: invalid reflog entry 7e79e09e8a7382f91610f7255a1b99ea59f68c0b error: refs/stash: invalid reflog entry feeb35e7b045d28943c706e761d0a2ac8206af2f error: refs/remotes/origin/master: invalid reflog entry 7fa2b7ee4bc0d11529f659db8b13ed1f537d2a98 Checking connectivity: 1419477, done. missing tree 1480c0a7ed2ad59ae701667292399c38d294658e missing tree ca2a01116bfbbd1fcbcf9812b95d8dc6c39e69d5 missing tree 5b7c41e547fc5c4c840e5b496da13d3daebc5fbe ... ... > > What do you mean by "expired"? "Even though I want to keep using > them, Git for some reason decided to destroy them." or "I no longer > use them but kept them lying around."? git worktree automatically prunes work trees: "The working tree’s administrative files in the repository (see "DETAILS" below) will eventually be removed automatically (see gc.worktreePruneExpire in git-config(1))," In my case I didn't actually want them removed, but fortunately there's nothing important in them (there certainly isn't anymore...). > > If the latter, I wonder "worktree prune" to remove the > admininstrative information for them would unblock you? It doesn't seem to help. $ git worktree prune -n $ git worktree prune $ git fetch remote: Counting objects: 35, done. remote: Compressing objects: 100% (20/20), done. remote: Total 21 (delta 17), reused 5 (delta 1) Unpacking objects: 100% (21/21), done. fatal: bad object HEAD error: ssh://whatever/myrepol did not send all necessary objects $ /usr/bin/git-2.7.3 fetch
Re: [PATCHv4 7/7] builtin/describe.c: describe a blob
On Tue, Nov 14, 2017 at 5:52 PM, Jonathan Tan wrote:
> On Tue, 14 Nov 2017 16:30:43 -0800
> Stefan Beller wrote:
>
>> The walking is performed in reverse order to show the introduction of a
>> blob rather than its last occurrence.
>
> The code as implemented here does not do this - it instead shows the last
> occurrence.
fixed to show the first occurrence.
>
>> NAME
>>
>> -git-describe - Describe a commit using the most recent tag reachable from it
>> +git-describe - Describe a commit or blob using the graph relations
>
> I would write "Describe a commit or blob using a tag reachable from it".
using a ref, as we also can use refs.
I think 'the graph' is technically correct here, but may be too confusing.
>
>> +If the given object refers to a blob, it will be described
>> +as `:`, such that the blob can be found
>> +at `` in the ``. Note, that the commit is likely
>> +not the commit that introduced the blob, but the one that was found
>> +first; to find the commit that introduced the blob, you need to find
>> +the commit that last touched the path, e.g.
>> +`git log -- `.
>> +As blobs do not point at the commits they are contained in,
>> +describing blobs is slow as we have to walk the whole graph.
>
> I think some of this needs to be updated?
fixed.
>
>> +static void process_object(struct object *obj, const char *path, void *data)
>> +{
>> + struct process_commit_data *pcd = data;
>> +
>> + if (!oidcmp(&pcd->looking_for, &obj->oid) && !pcd->dst->len) {
>> + reset_revision_walk();
>> + describe_commit(&pcd->current_commit, pcd->dst);
>> + strbuf_addf(pcd->dst, ":%s", path);
>> + pcd->revs->max_count = 0;
>> + }
>> +}
>
> Setting max_count to 0 does not work when reverse is used, because the
> commits are first buffered into revs->commits (see get_revision() in
> revision.c). There doesn't seem to be a convenient way to terminate the
> traversal immediately - I think setting revs->commits to NULL should
> work (but I didn't check). Remember to free revs->commits (using
> free_commit_list) first.
This does work indeed.
>
>> +test_expect_success 'describe a blob at a tag' '
>> + echo "make it a unique blob" >file &&
>> + git add file && git commit -m "content in file" &&
>> + git tag -a -m "latest annotated tag" unique-file &&
>> + git describe HEAD:file >actual &&
>> + echo "unique-file:file" >expect &&
>> + test_cmp expect actual
>> +'
>
> This is probably better named "describe a blob at a directly tagged
> commit".
ok
> (Should we also test the case where a blob is directly
> tagged?)
We do a bad job at describing tags that point at a blob currently:
git tag test-blob HEAD:Makefile
git describe test-blob
error: object cd75985991f4535c45e2589222a9e6a38fb1d613 is a blob, not a commit
fatal: test-blob is not a valid 'commit' object
This series changes this to
git describe test-blob
v2.15.0-rc0-43-g54bd705a95:Makefile
which might not be expected (you'd expect "test-blob"),
so I think I can write a test telling that this is suboptimal
behavior?
>
>> +test_expect_success 'describe a blob with its last introduction' '
>> + git commit --allow-empty -m "empty commit" &&
>> + git rm file &&
>> + git commit -m "delete blob" &&
>> + git revert HEAD &&
>> + git commit --allow-empty -m "empty commit" &&
>> + git describe HEAD:file >actual &&
>> + grep unique-file-3-g actual
>> +'
>
> The description is not true: firstly, this shows the last occurrence,
> not the last introduction (you can verify this by adding another commit
> and noticing that the contents of "actual" changes), and what we want is
> not the last introduction anyway, but the first one.
fixed.
Re: [PATCH] Makefile: check that tcl/tk is installed
Christian Couder writes:
> To improve the current behavior when Tcl/Tk is not installed,
> let's just check that TCLTK_PATH points to something and error
> out right away if this is not the case.
>
> This should benefit people who actually want to install and use
> git-gui or gitk as they will have to install Tcl/Tk anyway, and
> it is better for them if they are told about installing it soon
> in the build process, rather than if they have to debug it after
> installing.
Not objecting, but thinking aloud if this change makes sense.
If you are building Git for your own use on the same box, which is
presumably the majority of "build failed and I have no clue how to
fix" case that needs help, if you want gui tools, you need to have
tcl/tk installed anyway, whether you have msgfmt installed. This
seems to be the _only_ class of users this patch wants to cater to.
I wonder if we are hurting people who are not in that category.
- To run gui tools, tcl/tk must be available at runtime, but tcl/tk
is not necessary in the packager's environment to produce a
package of Git that contains working git-gui and gitk that will
be used on an end-user box with tcl/tk installed, as long as the
packager's environment has a working msgfmt.
- To process .po files for the gui tools in the packager's
environment without msgfmt, tcl/tk is required.
I suspect that this change will hurt those who package Git for other
people.
It used to be that, as long as they have msgfmt installed, they only
needed to _know_ what the path on the users' box to "wish" is, and
set it to TCLTK_PATH, and if they are distro packagers, most likely
they already have such an automated set-up working. Now with this
change, they are forced to install tcl/tk on their possibly headless
box where tcl/tk is useless, and worse yet, an attempt to install it
may bring in tons of unwanted stuff related to X that is irrelevant
on such a headless development environment.
I doubt that this is quite a good trade-off; it feels that this
burdens packagers a bit too much, and we may need a way to override
this new check further. I think "If I cannot run either wish or
msgfmt, then barf and give an error message" might at least be
needed. Am I misinterpreting the motivation of the patch?
> diff --git a/Makefile b/Makefile
> index ee9d5eb11e..ada6164e15 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1636,6 +1636,13 @@ ifeq ($(TCLTK_PATH),)
> NO_TCLTK = NoThanks
> endif
>
> +ifndef NO_TCLTK
> + has_tcltk := $(shell type $(TCLTK_PATH) 2>/dev/null)
> + ifndef has_tcltk
> +$(error "Tcl/Tk is not installed ('$(TCLTK_PATH)' not found). Consider
> setting NO_TCLTK or installing Tcl/Tk")
> + endif
> +endif
> +
> ifeq ($(PERL_PATH),)
> NO_PERL = NoThanks
> endif
Re: [PATCHv4 7/7] builtin/describe.c: describe a blob
Stefan Beller writes: >>> -git-describe - Describe a commit using the most recent tag reachable from >>> it >>> +git-describe - Describe a commit or blob using the graph relations >> >> I would write "Describe a commit or blob using a tag reachable from it". > > using a ref, as we also can use refs. > I think 'the graph' is technically correct here, but may be too confusing. Without saying graph over what, "graph relations" is not just confusing but an insufficient explanation for a technically correct explanation. Even though we have "--contains", we say "reachable from" and nobody has complained---so perhaps we can keep the white lie to keep the synopsis simpler? If I were writing this sentence from scratch, perhaps I wouldn't even use the word "describe". How about Give an object a human readable name based on an available ref or something like that? >> (Should we also test the case where a blob is directly >> tagged?) > > We do a bad job at describing tags that point at a blob currently: > > git tag test-blob HEAD:Makefile > git describe test-blob > error: object cd75985991f4535c45e2589222a9e6a38fb1d613 is a blob, not a commit > fatal: test-blob is not a valid 'commit' object > > This series changes this to > > git describe test-blob > v2.15.0-rc0-43-g54bd705a95:Makefile > > which might not be expected (you'd expect "test-blob"), > so I think I can write a test telling that this is suboptimal > behavior? Or a sentence in BUGS section. A case (or two) I find more interesting is to see how the code behaves against these: git tag -a -m "annotated blob" a-blob HEAD:Makefile git tag -a -m "annotated tree" a-tree HEAD:t git describe a-blob a-tree Thanks.
Re: [PATCHv4 7/7] builtin/describe.c: describe a blob
> > Give an object a human readable name based on an available ref > > or something like that? will use > Or a sentence in BUGS section. will add. > A case (or two) I find more interesting is to see how the code > behaves against these: > > git tag -a -m "annotated blob" a-blob HEAD:Makefile > git tag -a -m "annotated tree" a-tree HEAD:t > git describe a-blob a-tree Glad I added a test for exactly this (well only the a-blob case, but a-tree will be the same): test_expect_success 'describe tag object' ' git tag test-blob-1 -a -m msg unique-file:file && test_must_fail git describe test-blob-1 2>actual && grep "fatal: test-blob-1 is neither a commit nor blob" actual '
[PATCHv5 5/7] builtin/describe.c: print debug statements earlier
When debugging, print the received argument at the start of the
function instead of in the middle. This ensures that the received
argument is printed in all code paths, and also allows a subsequent
refactoring to not need to move the "arg" parameter.
Signed-off-by: Stefan Beller
---
builtin/describe.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/builtin/describe.c b/builtin/describe.c
index fd61f463cf..3136efde31 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -293,6 +293,9 @@ static void describe(const char *arg, int last_one)
unsigned long seen_commits = 0;
unsigned int unannotated_cnt = 0;
+ if (debug)
+ fprintf(stderr, _("describe %s\n"), arg);
+
if (get_oid(arg, &oid))
die(_("Not a valid object name %s"), arg);
cmit = lookup_commit_reference(&oid);
@@ -316,7 +319,7 @@ static void describe(const char *arg, int last_one)
if (!max_candidates)
die(_("no tag exactly matches '%s'"),
oid_to_hex(&cmit->object.oid));
if (debug)
- fprintf(stderr, _("searching to describe %s\n"), arg);
+ fprintf(stderr, _("No exact match on refs or tags, searching to
describe\n"));
if (!have_util) {
struct hashmap_iter iter;
--
2.15.0.128.gcadd42da22
[PATCHv5 1/7] t6120: fix typo in test name
Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- t/t6120-describe.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index 1c0e8659d9..c8b7ed82d9 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -304,7 +304,7 @@ test_expect_success 'describe chokes on severely broken submodules' ' mv .git/modules/sub1/ .git/modules/sub_moved && test_must_fail git describe --dirty ' -test_expect_success 'describe ignoring a borken submodule' ' +test_expect_success 'describe ignoring a broken submodule' ' git describe --broken >out && test_when_finished "mv .git/modules/sub_moved .git/modules/sub1" && grep broken out -- 2.15.0.128.gcadd42da22
[PATCHv5 7/7] builtin/describe.c: describe a blob
Sometimes users are given a hash of an object and they want to
identify it further (ex.: Use verify-pack to find the largest blobs,
but what are these? or [1])
When describing commits, we try to anchor them to tags or refs, as these
are conceptually on a higher level than the commit. And if there is no ref
or tag that matches exactly, we're out of luck. So we employ a heuristic
to make up a name for the commit. These names are ambiguous, there might
be different tags or refs to anchor to, and there might be different
path in the DAG to travel to arrive at the commit precisely.
When describing a blob, we want to describe the blob from a higher layer
as well, which is a tuple of (commit, deep/path) as the tree objects
involved are rather uninteresting. The same blob can be referenced by
multiple commits, so how we decide which commit to use? This patch
implements a rather naive approach on this: As there are no back pointers
from blobs to commits in which the blob occurs, we'll start walking from
any tips available, listing the blobs in-order of the commit and once we
found the blob, we'll take the first commit that listed the blob. For
example
git describe --tags v0.99:Makefile
conversion-901-g7672db20c2:Makefile
tells us the Makefile as it was in v0.99 was introduced in commit 7672db20.
The walking is performed in reverse order to show the introduction of a
blob rather than its last occurrence.
[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob
Signed-off-by: Stefan Beller
---
Documentation/git-describe.txt | 18 ++--
builtin/describe.c | 62 ++
t/t6120-describe.sh| 34 +++
3 files changed, 107 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index c924c945ba..e027fb8c4b 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -3,14 +3,14 @@ git-describe(1)
NAME
-git-describe - Describe a commit using the most recent tag reachable from it
-
+git-describe - Give an object a human readable name based on an available ref
SYNOPSIS
[verse]
'git describe' [--all] [--tags] [--contains] [--abbrev=] [...]
'git describe' [--all] [--tags] [--contains] [--abbrev=] --dirty[=]
+'git describe'
DESCRIPTION
---
@@ -24,6 +24,12 @@ By default (without --all or --tags) `git describe` only
shows
annotated tags. For more information about creating annotated tags
see the -a and -s options to linkgit:git-tag[1].
+If the given object refers to a blob, it will be described
+as `:`, such that the blob can be found
+at `` in the ``, which itself describes the
+first commit in which this blob occurs in a reverse revision walk
+from HEAD.
+
OPTIONS
---
...::
@@ -186,6 +192,14 @@ selected and output. Here fewest commits different is
defined as
the number of commits which would be shown by `git log tag..input`
will be the smallest number of commits possible.
+BUGS
+
+
+Tree objects as well as tag objects not pointing at commits, cannot be
described.
+When describing blobs, the lightweight tags pointing at blobs are ignored,
+but the blob is still described as : despite the lightweight
+tag being favorable.
+
GIT
---
Part of the linkgit:git[1] suite
diff --git a/builtin/describe.c b/builtin/describe.c
index 9e9a5ed5d4..5b4bfaba3f 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -3,6 +3,7 @@
#include "lockfile.h"
#include "commit.h"
#include "tag.h"
+#include "blob.h"
#include "refs.h"
#include "builtin.h"
#include "exec_cmd.h"
@@ -11,8 +12,9 @@
#include "hashmap.h"
#include "argv-array.h"
#include "run-command.h"
+#include "revision.h"
+#include "list-objects.h"
-#define SEEN (1u << 0)
#define MAX_TAGS (FLAG_BITS - 1)
static const char * const describe_usage[] = {
@@ -434,6 +436,53 @@ static void describe_commit(struct object_id *oid, struct
strbuf *dst)
strbuf_addstr(dst, suffix);
}
+struct process_commit_data {
+ struct object_id current_commit;
+ struct object_id looking_for;
+ struct strbuf *dst;
+ struct rev_info *revs;
+};
+
+static void process_commit(struct commit *commit, void *data)
+{
+ struct process_commit_data *pcd = data;
+ pcd->current_commit = commit->object.oid;
+}
+
+static void process_object(struct object *obj, const char *path, void *data)
+{
+ struct process_commit_data *pcd = data;
+
+ if (!oidcmp(&pcd->looking_for, &obj->oid) && !pcd->dst->len) {
+ reset_revision_walk();
+ describe_commit(&pcd->current_commit, pcd->dst);
+ strbuf_addf(pcd->dst, ":%s", path);
+ free_commit_list(pcd->revs->commits);
+ pcd->revs->commits = NULL;
+ }
+}
+
+static void describe_blob(struct object_id oid, struct strbuf *dst)
+{
+ struct rev_info revs;
+ struct argv_
[PATCHv5 4/7] builtin/describe.c: rename `oid` to avoid variable shadowing
The function `describe` has already a variable named `oid` declared at
the beginning of the function for an object id. Do not shadow that
variable with a pointer to an object id.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
builtin/describe.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/describe.c b/builtin/describe.c
index 29075dbd0f..fd61f463cf 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -381,9 +381,9 @@ static void describe(const char *arg, int last_one)
}
if (!match_cnt) {
- struct object_id *oid = &cmit->object.oid;
+ struct object_id *cmit_oid = &cmit->object.oid;
if (always) {
- printf("%s", find_unique_abbrev(oid->hash, abbrev));
+ printf("%s", find_unique_abbrev(cmit_oid->hash,
abbrev));
if (suffix)
printf("%s", suffix);
printf("\n");
@@ -392,11 +392,11 @@ static void describe(const char *arg, int last_one)
if (unannotated_cnt)
die(_("No annotated tags can describe '%s'.\n"
"However, there were unannotated tags: try
--tags."),
- oid_to_hex(oid));
+ oid_to_hex(cmit_oid));
else
die(_("No tags can describe '%s'.\n"
"Try --always, or create some tags."),
- oid_to_hex(oid));
+ oid_to_hex(cmit_oid));
}
QSORT(all_matches, match_cnt, compare_pt);
--
2.15.0.128.gcadd42da22
[PATCHv5 2/7] list-objects.c: factor out traverse_trees_and_blobs
With traverse_trees_and_blobs factored out of the main traverse function,
the next patch can introduce an in-order revision walking with ease.
In the next patch we'll call `traverse_trees_and_blobs` from within the
loop walking the commits, such that we'll have one invocation of that
function per commit. That is why we do not want to have memory allocations
in that function, such as we'd have if we were to use a strbuf locally.
Pass a strbuf from traverse_commit_list into the blob and tree traversing
function as a scratch pad that only needs to be allocated once.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
list-objects.c | 50 +++---
1 file changed, 31 insertions(+), 19 deletions(-)
diff --git a/list-objects.c b/list-objects.c
index b3931fa434..7c2ce9c4bd 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -183,25 +183,15 @@ static void add_pending_tree(struct rev_info *revs,
struct tree *tree)
add_pending_object(revs, &tree->object, "");
}
-void traverse_commit_list(struct rev_info *revs,
- show_commit_fn show_commit,
- show_object_fn show_object,
- void *data)
+static void traverse_trees_and_blobs(struct rev_info *revs,
+struct strbuf *base,
+show_object_fn show_object,
+void *data)
{
int i;
- struct commit *commit;
- struct strbuf base;
- strbuf_init(&base, PATH_MAX);
- while ((commit = get_revision(revs)) != NULL) {
- /*
-* an uninteresting boundary commit may not have its tree
-* parsed yet, but we are not going to show them anyway
-*/
- if (commit->tree)
- add_pending_tree(revs, commit->tree);
- show_commit(commit, data);
- }
+ assert(base->len == 0);
+
for (i = 0; i < revs->pending.nr; i++) {
struct object_array_entry *pending = revs->pending.objects + i;
struct object *obj = pending->item;
@@ -218,17 +208,39 @@ void traverse_commit_list(struct rev_info *revs,
path = "";
if (obj->type == OBJ_TREE) {
process_tree(revs, (struct tree *)obj, show_object,
-&base, path, data);
+base, path, data);
continue;
}
if (obj->type == OBJ_BLOB) {
process_blob(revs, (struct blob *)obj, show_object,
-&base, path, data);
+base, path, data);
continue;
}
die("unknown pending object %s (%s)",
oid_to_hex(&obj->oid), name);
}
object_array_clear(&revs->pending);
- strbuf_release(&base);
+}
+
+void traverse_commit_list(struct rev_info *revs,
+ show_commit_fn show_commit,
+ show_object_fn show_object,
+ void *data)
+{
+ struct commit *commit;
+ struct strbuf csp; /* callee's scratch pad */
+ strbuf_init(&csp, PATH_MAX);
+
+ while ((commit = get_revision(revs)) != NULL) {
+ /*
+* an uninteresting boundary commit may not have its tree
+* parsed yet, but we are not going to show them anyway
+*/
+ if (commit->tree)
+ add_pending_tree(revs, commit->tree);
+ show_commit(commit, data);
+ }
+ traverse_trees_and_blobs(revs, &csp, show_object, data);
+
+ strbuf_release(&csp);
}
--
2.15.0.128.gcadd42da22
[PATCHv5 6/7] builtin/describe.c: factor out describe_commit
Factor out describing commits into its own function `describe_commit`,
which will put any output to stdout into a strbuf, to be printed
afterwards.
As the next patch will teach Git to describe blobs using a commit and path,
this refactor will make it easy to reuse the code describing commits.
Signed-off-by: Stefan Beller
---
builtin/describe.c | 63 --
1 file changed, 37 insertions(+), 26 deletions(-)
diff --git a/builtin/describe.c b/builtin/describe.c
index 3136efde31..9e9a5ed5d4 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -256,7 +256,7 @@ static unsigned long finish_depth_computation(
return seen_commits;
}
-static void display_name(struct commit_name *n)
+static void append_name(struct commit_name *n, struct strbuf *dst)
{
if (n->prio == 2 && !n->tag) {
n->tag = lookup_tag(&n->oid);
@@ -272,19 +272,18 @@ static void display_name(struct commit_name *n)
}
if (n->tag)
- printf("%s", n->tag->tag);
+ strbuf_addstr(dst, n->tag->tag);
else
- printf("%s", n->path);
+ strbuf_addstr(dst, n->path);
}
-static void show_suffix(int depth, const struct object_id *oid)
+static void append_suffix(int depth, const struct object_id *oid, struct
strbuf *dst)
{
- printf("-%d-g%s", depth, find_unique_abbrev(oid->hash, abbrev));
+ strbuf_addf(dst, "-%d-g%s", depth, find_unique_abbrev(oid->hash,
abbrev));
}
-static void describe(const char *arg, int last_one)
+static void describe_commit(struct object_id *oid, struct strbuf *dst)
{
- struct object_id oid;
struct commit *cmit, *gave_up_on = NULL;
struct commit_list *list;
struct commit_name *n;
@@ -293,26 +292,18 @@ static void describe(const char *arg, int last_one)
unsigned long seen_commits = 0;
unsigned int unannotated_cnt = 0;
- if (debug)
- fprintf(stderr, _("describe %s\n"), arg);
-
- if (get_oid(arg, &oid))
- die(_("Not a valid object name %s"), arg);
- cmit = lookup_commit_reference(&oid);
- if (!cmit)
- die(_("%s is not a valid '%s' object"), arg, commit_type);
+ cmit = lookup_commit_reference(oid);
n = find_commit_name(&cmit->object.oid);
if (n && (tags || all || n->prio == 2)) {
/*
* Exact match to an existing ref.
*/
- display_name(n);
+ append_name(n, dst);
if (longformat)
- show_suffix(0, n->tag ? &n->tag->tagged->oid : &oid);
+ append_suffix(0, n->tag ? &n->tag->tagged->oid : oid,
dst);
if (suffix)
- printf("%s", suffix);
- printf("\n");
+ strbuf_addstr(dst, suffix);
return;
}
@@ -386,10 +377,9 @@ static void describe(const char *arg, int last_one)
if (!match_cnt) {
struct object_id *cmit_oid = &cmit->object.oid;
if (always) {
- printf("%s", find_unique_abbrev(cmit_oid->hash,
abbrev));
+ strbuf_addstr(dst, find_unique_abbrev(cmit_oid->hash,
abbrev));
if (suffix)
- printf("%s", suffix);
- printf("\n");
+ strbuf_addstr(dst, suffix);
return;
}
if (unannotated_cnt)
@@ -437,15 +427,36 @@ static void describe(const char *arg, int last_one)
}
}
- display_name(all_matches[0].name);
+ append_name(all_matches[0].name, dst);
if (abbrev)
- show_suffix(all_matches[0].depth, &cmit->object.oid);
+ append_suffix(all_matches[0].depth, &cmit->object.oid, dst);
if (suffix)
- printf("%s", suffix);
- printf("\n");
+ strbuf_addstr(dst, suffix);
+}
+
+static void describe(const char *arg, int last_one)
+{
+ struct object_id oid;
+ struct commit *cmit;
+ struct strbuf sb = STRBUF_INIT;
+
+ if (debug)
+ fprintf(stderr, _("describe %s\n"), arg);
+
+ if (get_oid(arg, &oid))
+ die(_("Not a valid object name %s"), arg);
+ cmit = lookup_commit_reference(&oid);
+ if (!cmit)
+ die(_("%s is not a valid '%s' object"), arg, commit_type);
+
+ describe_commit(&oid, &sb);
+
+ puts(sb.buf);
if (!last_one)
clear_commit_marks(cmit, -1);
+
+ strbuf_release(&sb);
}
int cmd_describe(int argc, const char **argv, const char *prefix)
--
2.15.0.128.gcadd42da22
[PATCHv5 0/7] describe blob
Thanks Jonathan and Junio for the patient review!
* fixed issues brought up in the last patch, see interdiff below.
(I found that walking from so many refs as starting points was
the source of confusion, hence we only want to walk from HEAD
* reworded commit messages from earlier patches
* added a BUGS section to the man page
* took Junios suggestion for the NAME section.
Stefan Beller (7):
t6120: fix typo in test name
list-objects.c: factor out traverse_trees_and_blobs
revision.h: introduce blob/tree walking in order of the commits
builtin/describe.c: rename `oid` to avoid variable shadowing
builtin/describe.c: print debug statements earlier
builtin/describe.c: factor out describe_commit
builtin/describe.c: describe a blob
Documentation/git-describe.txt | 18 +-
Documentation/rev-list-options.txt | 5 ++
builtin/describe.c | 122 -
list-objects.c | 58 --
revision.c | 2 +
revision.h | 3 +-
t/t6100-rev-list-in-order.sh | 77 +++
t/t6120-describe.sh| 36 ++-
8 files changed, 270 insertions(+), 51 deletions(-)
create mode 100755 t/t6100-rev-list-in-order.sh
--
2.15.0.128.gcadd42da22
diff --git c/Documentation/git-describe.txt w/Documentation/git-describe.txt
index a25443ca91..e027fb8c4b 100644
--- c/Documentation/git-describe.txt
+++ w/Documentation/git-describe.txt
@@ -3,8 +3,7 @@ git-describe(1)
NAME
-git-describe - Describe a commit or blob using the graph relations
-
+git-describe - Give an object a human readable name based on an available ref
SYNOPSIS
@@ -27,13 +26,9 @@ see the -a and -s options to linkgit:git-tag[1].
If the given object refers to a blob, it will be described
as `:`, such that the blob can be found
-at `` in the ``. Note, that the commit is likely
-not the commit that introduced the blob, but the one that was found
-first; to find the commit that introduced the blob, you need to find
-the commit that last touched the path, e.g.
-`git log -- `.
-As blobs do not point at the commits they are contained in,
-describing blobs is slow as we have to walk the whole graph.
+at `` in the ``, which itself describes the
+first commit in which this blob occurs in a reverse revision walk
+from HEAD.
OPTIONS
---
@@ -197,6 +192,14 @@ selected and output. Here fewest commits different is
defined as
the number of commits which would be shown by `git log tag..input`
will be the smallest number of commits possible.
+BUGS
+
+
+Tree objects as well as tag objects not pointing at commits, cannot be
described.
+When describing blobs, the lightweight tags pointing at blobs are ignored,
+but the blob is still described as : despite the lightweight
+tag being favorable.
+
GIT
---
Part of the linkgit:git[1] suite
diff --git c/builtin/describe.c w/builtin/describe.c
index a2a5fdc48d..5b4bfaba3f 100644
--- c/builtin/describe.c
+++ w/builtin/describe.c
@@ -457,7 +457,8 @@ static void process_object(struct object *obj, const char
*path, void *data)
reset_revision_walk();
describe_commit(&pcd->current_commit, pcd->dst);
strbuf_addf(pcd->dst, ":%s", path);
- pcd->revs->max_count = 0;
+ free_commit_list(pcd->revs->commits);
+ pcd->revs->commits = NULL;
}
}
@@ -468,12 +469,7 @@ static void describe_blob(struct object_id oid, struct
strbuf *dst)
struct process_commit_data pcd = { null_oid, oid, dst, &revs};
argv_array_pushl(&args, "internal: The first arg is not parsed",
- "--all", "--reflog", /* as many starting points as possible */
- /* NEEDSWORK: --all is incompatible with worktrees for now: */
- "--single-worktree",
- "--objects",
- "--in-commit-order",
- "--reverse",
+ "--objects", "--in-commit-order", "--reverse", "HEAD",
NULL);
init_revisions(&revs, NULL);
diff --git c/list-objects.c w/list-objects.c
index 07a92f35fe..4caa6fcb77 100644
--- c/list-objects.c
+++ w/list-objects.c
@@ -239,7 +239,13 @@ void traverse_commit_list(struct rev_info *revs,
if (commit->tree)
add_pending_tree(revs, commit->tree);
show_commit(commit, data);
+
if (revs->tree_blobs_in_commit_order)
+ /*
+* NEEDSWORK: Adding the tree and then flushing it here
+* needs a reallocation for each commit. Can we pass the
+* tree directory without allocation churn?
+*/
traverse_trees_and_blobs(revs, &csp, show_object, data);
}
traverse_trees_and_blobs(revs, &csp, show_object, data);
diff --git c/t/t6120-describe
[PATCHv5 3/7] revision.h: introduce blob/tree walking in order of the commits
The functionality to list tree objects in the order they were seen
while traversing the commits will be used in one of the next commits,
where we teach `git describe` to describe not only commits, but blobs, too.
The change in list-objects.c is rather minimal as we'll be re-using
the infrastructure put in place of the revision walking machinery. For
example one could expect that add_pending_tree is not called, but rather
commit->tree is directly passed to the tree traversal function. This
however requires a lot more code than just emptying the queue containing
trees after each commit.
Signed-off-by: Stefan Beller
---
Documentation/rev-list-options.txt | 5 +++
list-objects.c | 8
revision.c | 2 +
revision.h | 3 +-
t/t6100-rev-list-in-order.sh | 77 ++
5 files changed, 94 insertions(+), 1 deletion(-)
create mode 100755 t/t6100-rev-list-in-order.sh
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index 13501e1556..9066e0c777 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -686,6 +686,11 @@ ifdef::git-rev-list[]
all object IDs which I need to download if I have the commit
object _bar_ but not _foo_''.
+--in-commit-order::
+ Print tree and blob ids in order of the commits. The tree
+ and blob ids are printed after they are first referenced
+ by a commit.
+
--objects-edge::
Similar to `--objects`, but also print the IDs of excluded
commits prefixed with a ``-'' character. This is used by
diff --git a/list-objects.c b/list-objects.c
index 7c2ce9c4bd..4caa6fcb77 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -239,6 +239,14 @@ void traverse_commit_list(struct rev_info *revs,
if (commit->tree)
add_pending_tree(revs, commit->tree);
show_commit(commit, data);
+
+ if (revs->tree_blobs_in_commit_order)
+ /*
+* NEEDSWORK: Adding the tree and then flushing it here
+* needs a reallocation for each commit. Can we pass the
+* tree directory without allocation churn?
+*/
+ traverse_trees_and_blobs(revs, &csp, show_object, data);
}
traverse_trees_and_blobs(revs, &csp, show_object, data);
diff --git a/revision.c b/revision.c
index d167223e69..9329d4ebbf 100644
--- a/revision.c
+++ b/revision.c
@@ -1845,6 +1845,8 @@ static int handle_revision_opt(struct rev_info *revs, int
argc, const char **arg
revs->dense = 0;
} else if (!strcmp(arg, "--show-all")) {
revs->show_all = 1;
+ } else if (!strcmp(arg, "--in-commit-order")) {
+ revs->tree_blobs_in_commit_order = 1;
} else if (!strcmp(arg, "--remove-empty")) {
revs->remove_empty_trees = 1;
} else if (!strcmp(arg, "--merges")) {
diff --git a/revision.h b/revision.h
index 54761200ad..86985d68aa 100644
--- a/revision.h
+++ b/revision.h
@@ -121,7 +121,8 @@ struct rev_info {
bisect:1,
ancestry_path:1,
first_parent_only:1,
- line_level_traverse:1;
+ line_level_traverse:1,
+ tree_blobs_in_commit_order:1;
/* Diff flags */
unsigned intdiff:1,
diff --git a/t/t6100-rev-list-in-order.sh b/t/t6100-rev-list-in-order.sh
new file mode 100755
index 00..b2bb0a7f61
--- /dev/null
+++ b/t/t6100-rev-list-in-order.sh
@@ -0,0 +1,77 @@
+#!/bin/sh
+
+test_description='rev-list testing in-commit-order'
+
+. ./test-lib.sh
+
+test_expect_success 'setup a commit history with trees, blobs' '
+ for x in one two three four
+ do
+ echo $x >$x &&
+ git add $x &&
+ git commit -m "add file $x" ||
+ return 1
+ done &&
+ for x in four three
+ do
+ git rm $x &&
+ git commit -m "remove $x" ||
+ return 1
+ done
+'
+
+test_expect_success 'rev-list --in-commit-order' '
+ git rev-list --in-commit-order --objects HEAD >actual.raw &&
+ cut -c 1-40 >actual expect.raw <<-\EOF &&
+ HEAD^{commit}
+ HEAD^{tree}
+ HEAD^{tree}:one
+ HEAD^{tree}:two
+ HEAD~1^{commit}
+ HEAD~1^{tree}
+ HEAD~1^{tree}:three
+ HEAD~2^{commit}
+ HEAD~2^{tree}
+ HEAD~2^{tree}:four
+ HEAD~3^{commit}
+ # HEAD~3^{tree} skipped, same as HEAD~1^{tree}
+ HEAD~4^{commit}
+ # HEAD~4^{tree} skipped, same as HEAD^{tree}
+ HEAD~5^{commit}
+ HEAD~5^{tree}
+ EOF
Re: [PATCHv4 7/7] builtin/describe.c: describe a blob
Stefan Beller writes: > grep "fatal: test-blob-1 is neither a commit nor blob" actual OK, that might be somewhat unsatisfying from end-user's point of view (logically "test-blob-1" is already a name based on the 'graph relations' that is satisfactory). [side note] should that grep become test_i18ngrep? If this machinery is to find an object that is *bad* (in the sense in one of your earlier messages), I suspect it may be quite easy to use the new mechanism added by the series to also locate a tree object and report "that tree appears in this commit at this path" and it would be equally useful as the feature to find a blob. Thanks.
Re: [PATCHv5 7/7] builtin/describe.c: describe a blob
Stefan Beller writes: > When describing commits, we try to anchor them to tags or refs, as these > are conceptually on a higher level than the commit. And if there is no ref > or tag that matches exactly, we're out of luck. So we employ a heuristic > to make up a name for the commit. These names are ambiguous, there might > be different tags or refs to anchor to, and there might be different > path in the DAG to travel to arrive at the commit precisely. I am not sure if "And if there is ..." is adding much value here (I do not think it is even technically correct for that matter). If there are more than one tag that point at the commit the user is interested in, we use one of the tags, as tags conceptually sit at a higher level. And we use a heuristic to use one or the other tag to make up a name for the commit, so the same commit can have two valid names. ---So what? Neither of these two valid names is "ambigous"; the commit object the user wanted to name _is_ correctly identified (I would assume that we are not discussing a hash collision). Lucikly, if we remove "And if...precisely", the logic still flows nicely, if not more, to the next paragraph. > When describing a blob, we want to describe the blob from a higher layer > as well, which is a tuple of (commit, deep/path) as the tree objects > involved are rather uninteresting. The same blob can be referenced by > multiple commits, so how we decide which commit to use? This patch > implements a rather naive approach on this: As there are no back pointers > from blobs to commits in which the blob occurs, we'll start walking from > any tips available, listing the blobs in-order of the commit and once we Is "any tips" still the case? I was wondering why you start your traversal at HEAD and nothing else in this iteration. There seems to be no mention of this design decision in the documentation and no justification in the log. > found the blob, we'll take the first commit that listed the blob. For > example > > git describe --tags v0.99:Makefile > conversion-901-g7672db20c2:Makefile > > tells us the Makefile as it was in v0.99 was introduced in commit 7672db20. > > The walking is performed in reverse order to show the introduction of a > blob rather than its last occurrence. The reversing may improve the chance of an older commit to be chosen rather than the newer one, but it does not even guarantee to show the "introduction". What this guarantees is that a long history will be traversed fully before we start considering which commit has the blob of interest, I am afraid. Is this a sensible trade-off? > + argv_array_pushl(&args, "internal: The first arg is not parsed", > + "--objects", "--in-commit-order", "--reverse", "HEAD", > + NULL);
Re: [PATCH 27/30] merge-recursive: Apply necessary modifications for directory renames
On Wed, Nov 15, 2017 at 12:23 PM, Stefan Beller wrote:
>> + if (!strcmp(pair->one->path, pair->two->path)) {
>> + /*
>> +* Paths should only match if this was initially a
>> +* non-rename that is being turned into one by
>> +* directory rename detection.
>> +*/
>> + assert(pair->status != 'R');
>> + } else {
>> + assert(pair->status == 'R');
>
> assert() is compiled conditionally depending on whether
> NDEBUG is set, which makes potential error reports more interesting and
> head-scratching. But we'd rather prefer easy bug reports, therefore
> we'd want to (a) either have the condition checked always, when
> you know this could occur, e.g. via
>
> if ()
> BUG("Git is broken, because...");
>
> or (b) you could omit the asserts if they are more of a developer guideline?
>
> I wonder if we want to introduce a BUG_ON(, ) macro
> that contains (a).
Yeah, I added a few other asserts in other commits too. None of these
were written with the expectation that they should or could ever occur
for a user; it was just a developer guideline to make sure I (and
future others) didn't break certain invariants during the
implementation or while making modifications to it.
So that makes it more like (b), but I feel that there is something to
be said for having a convenient syntax for expressing pre-conditions
that others shouldn't violate when changing the code, and which will
be given more weight than a comment. For that, something that is
compiled out on many users systems seemed just fine.
But, I have certainly seen abuses of asserts in my time as well (e.g.
function calls with important side-effects being placed inside
asserts), so if folks have decided it's against git's style, then I
understand. I'll remove some, and switch the cheaper checks over to
BUG().
>> + re->dst_entry->processed = 1;
>> + //string_list_remove(entries, pair->two->path, 0);
>
> commented code?
Ugh, that's embarrassing. I'll clean that out.
Usability suggestion: Catch `git commit -amend`
Despite being a native English speaker, I've often typo'd when trying to invoke `git commit --amend`. Recently I wrote `git commit -ammend` which of course added everything to the commit and attached the commit message "mend". This doesn't seem to be an uncommon error, there are 64,000 commits like this on GitHub: https://github.com/search?utf8=✓&q=mend&type=Commits The search for simply "end" (as in `-amend`) returns millions of hits but it's hard to tell how many of those are false positives. It might be reasonable to try to catch `-amend` (at least) and prevent an errant commit. Trying to roll back a commit is a place where people often get into trouble and get confused about the state of their repository. (Of course, if they just cursor-up and fix the typo, now they're editing the "end" commit and not the one they were trying to amend in the first place.) Git could suggest the user instead pass the commit message as a separate argument, like `-am end` if they really want to do it. I'm OK with getting an error if I type `--ammend`, but it is nice that Git does do spell check on subcommand names. That would make it easier to figure out which argument I mistyped. (In this case, there's usually only one.) Ryan
[PATCH v2] sequencer: reschedule pick if index can't be locked
From: Phillip Wood Date: Wed, 15 Nov 2017 10:41:25 + If the index cannot be locked in do_recursive_merge(), issue an error message and go on to the error recovery codepath, instead of dying. When the commit cannot be picked, it needs to be rescheduled when performing an interactive rebase, but just dying there won't allow that to happen, and when the user runs 'git rebase --continue' rather than 'git rebase --abort', the commit gets silently dropped. Signed-off-by: Phillip Wood --- * I've queue this, taking input from responses by Dscho and Martin. sequencer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 332a383b03..10924ffd49 100644 --- a/sequencer.c +++ b/sequencer.c @@ -438,7 +438,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next, char **xopt; static struct lock_file index_lock; - hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR); + if (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR) < 0) + return -1; read_cache(); -- 2.15.0-360-gf2497ca0e5
Re: [PATCH] config: avoid "write_in_full(fd, buf, len) != len" pattern
Phillip Wood writes:
> From: Phillip Wood
>
> As explained in commit 06f46f237 (avoid "write_in_full(fd, buf, len)
> != len" pattern, 2017–09–13) the return value of write_in_full() is
> either -1 or the requested number of bytes. As such comparing the
> return value to an unsigned value such as strbuf.len will fail to
> catch errors. Change the code to use the preferred '< 0' check.
Thanks, queued. This seems to have come from 9a5abfc7 ("After
renaming a section, print any trailing variable definitions",
2009-07-24), which is rather ancient, but was made worse by getting
duplicated by 52d59cc6 ("branch: add a --copy (-c) option to go with
--move (-m)", 2017-06-18) recently.
Re: [RFC] Indicate that Git waits for user input via editor
Junio C Hamano writes:
> I wonder if we can do something like
> ...
> to tentatively give a message without permanently wasting the
> vertical space.
Learning from 13e4760a ("recv_sideband: Do not use ANSI escape
sequence on dumb terminals.", 2008-01-08), I think the above should
be more like this:
git_spawn_editor()
{
static const char *finish;
if (!finish) {
const char ANSI_FINISH[] = "\r\033[K";
const char DUMB_FINISH[] = "done.\n";
char *term = getenv("TERM");
if (term && strcmp(term, "dumb"))
finish = ANSI_FINISH;
else
finish = DUMB_FINISH;
}
/* notice the lack of terminating LF */
fprintf(stderr, "Launched your editor, waiting...");
fflush(stderr);
if (!run_command(... spawn the editor ...))
fputs(finish, stderr);
else
fprintf(stderr, "failed (%s)\n", strerror(errno));
fflush(stderr);
}
[PATCH] launch_editor(): indicate that Git waits for user input
When a graphical GIT_EDITOR is spawned by a Git command that opens
and waits for it for the user input (e.g. "git rebase -i") pops its
window elsewhere that is obscure, the user may be left staring the
original terminal window s/he invoked the Git command from, without
even realizing that now s/he needs to interact with another window
the editor is waiting in, before Git can proceed.
Show a message to the original terminal, and get rid of it when the
editor returns.
Signed-off-by: Junio C Hamano
---
* I tried this with "emacsclient" but it turns out that Emacs folks
have already thought about this issue, and the program says
"Waiting for Emacs..." while it does its thing, so from that
point of view, perhaps as Stefan said originally, the editor Lars
had trouble with is what is at fault and needs fixing? I dunno.
Also, emacsclient seems to conclude its message, once the editing
is done, by emitting LF _before_ we have a chance to do the "go
back to the beginning and clear" dance, so it probably is not a
very good example to emulate the situation Lars had trouble with.
You cannot observe the nuking of the "launched, waiting..." with
it.
editor.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/editor.c b/editor.c
index 7519edecdc..1321944716 100644
--- a/editor.c
+++ b/editor.c
@@ -40,6 +40,28 @@ int launch_editor(const char *path, struct strbuf *buffer,
const char *const *en
const char *args[] = { editor, real_path(path), NULL };
struct child_process p = CHILD_PROCESS_INIT;
int ret, sig;
+ static const char *close_notice = NULL;
+
+ if (isatty(2) && !close_notice) {
+ char *term = getenv("TERM");
+
+ if (term && strcmp(term, "dumb"))
+ /*
+* go back to the beginning and erase the
+* entire line if the terminal is capable
+* to do so, to avoid wasting the vertical
+* space.
+*/
+ close_notice = "\r\033[K";
+ else
+ /* otherwise, complete and waste the line */
+ close_notice = "done.\n";
+ }
+
+ if (close_notice) {
+ fprintf(stderr, "Launched your editor, waiting...");
+ fflush(stderr);
+ }
p.argv = args;
p.env = env;
@@ -53,11 +75,14 @@ int launch_editor(const char *path, struct strbuf *buffer,
const char *const *en
sig = ret - 128;
sigchain_pop(SIGINT);
sigchain_pop(SIGQUIT);
+
if (sig == SIGINT || sig == SIGQUIT)
raise(sig);
if (ret)
return error("There was a problem with the editor
'%s'.",
editor);
+ if (close_notice)
+ fputs(close_notice, stderr);
}
if (!buffer)
--
2.15.0-360-gf2497ca0e5
[PATCH] branch doc: remove --set-upstream from synopsis
Support for the --set-upstream option was removed in 52668846ea (builtin/branch: stop supporting the "--set-upstream" option, 2017-08-17), after a long deprecation period. Remove the option from the command synopsis for consistency. Replace another reference to it in the description of `--delete` with `--set-upstream-to`. Signed-off-by: Todd Zullinger --- I noticed that --set-upstream was still in the synopsis for git branch. I don't think it was left there intentionally. I looked through the thread where support for the option was removed and didn't notice any comments suggesting otherwise[1]. With luck, I didn't miss the obvious while reading the thread. [1] https://public-inbox.org/git/20170807143938.5127-1-kaarticsivaraam91...@gmail.com/ Documentation/git-branch.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt index d6587c5e96..159ca388f1 100644 --- a/Documentation/git-branch.txt +++ b/Documentation/git-branch.txt @@ -14,7 +14,7 @@ SYNOPSIS [(--merged | --no-merged) []] [--contains []] [--points-at ] [--format=] [...] -'git branch' [--set-upstream | --track | --no-track] [-l] [-f] [] +'git branch' [--track | --no-track] [-l] [-f] [] 'git branch' (--set-upstream-to= | -u ) [] 'git branch' --unset-upstream [] 'git branch' (-m | -M) [] @@ -86,7 +86,7 @@ OPTIONS --delete:: Delete a branch. The branch must be fully merged in its upstream branch, or in `HEAD` if no upstream was set with - `--track` or `--set-upstream`. + `--track` or `--set-upstream-to`. -D:: Shortcut for `--delete --force`. -- 2.15.0
