Re: [PATCH] doc: filter-branch does not require re-export of vars
On Mon, May 29, 2017 at 01:27:45PM +0900, Junio C Hamano wrote: > Samuel Lijinwrites: > > >> However, I think POSIX mandates the behavior you'd expect. And the only > >> shell I know that misbehaves in this way is Solaris /bin/sh, which we > >> have already declared too broken to support. > > > > Off-topic, but where is this explicitly documented? > > One link I had readily available was > > https://public-inbox.org/git/7vei5qtnc5@alter.siamese.dyndns.org/ > > but there may be older discussions that were the actual process of > our officially having "written its /bin/sh off as broken and > unusable" if you dig further in the list archive. Ah, I had taken Samuel's question as "where is the POSIX behavior documented". :) The link above is a good explanation of the $() problem on Solaris. It also doesn't do ${parameter#word}. I couldn't find a specific moment of outlawing that /bin/sh, but there are several mentions of problems with it going back to the beginning. Here's one from 2007: http://public-inbox.org/git/7vabt9sasl@assigned-by-dhcp.cox.net/ We had already given up on it by then. I don't think there's anything in CodingGuidelines (though t/README does call it "broken"), though it explicitly endorses $(), which rules out Solaris /bin/sh. I think we found in the last year or two that there are some old versions of ksh that don't like our scripts (definitely ksh88, and maybe ksh93?). Some light reading for the curious: http://public-inbox.org/git/calr6jehvik9kzxr6r6xzkz5eao-rjwj3xyah_dosdxhejys...@mail.gmail.com/ http://public-inbox.org/git/calr6jejwjja0x2qxsxqobqc_yxrgx87lyf8cmj0mmjff6pk...@mail.gmail.com/ http://public-inbox.org/git/xmqqinxt3kwq@gitster.mtv.corp.google.com/ The problems are around backslash-escaping, signal exit codes, and doubled parentheses. There are some patches, but I think we decided not to pursue it (I couldn't find that exact decision, but it's referenced later on). -Peff
Re: [PATCH] doc: filter-branch does not require re-export of vars
Samuel Lijinwrites: >> However, I think POSIX mandates the behavior you'd expect. And the only >> shell I know that misbehaves in this way is Solaris /bin/sh, which we >> have already declared too broken to support. > > Off-topic, but where is this explicitly documented? One link I had readily available was https://public-inbox.org/git/7vei5qtnc5@alter.siamese.dyndns.org/ but there may be older discussions that were the actual process of our officially having "written its /bin/sh off as broken and unusable" if you dig further in the list archive.
Re: git push recurse.submodules behavior changed in 2.13
On Sun, May 28, 2017 at 7:44 PM, Junio C Hamanowrote: > John Shahid writes: > >> It looks like the git push recurse-submodules behavior has changed. >> Currently with 2.13 you cannot run "git push >> --recurse-submodules=on-demand" if the parent repo is on a different >> branch than the sub repos, e.g. parent repo is on "develop" and >> sub-repo on "master". I created a test that can be found here [1]. >> >> A bisect shows that the change to propagate refspec [2] to the >> submodules is the culprit. imho this is an undesired change in >> behavior. I looked at the code but couldn't see an easy way to fix >> this issue without breaking the feature mentioned above. The only >> option I can think of is to control the refspec propagation behavior >> using a flag, e.g. "--propagate-refspecs" or add another >> recurse-submodules option, e.g. "--recurse-submodules=propagate" >> >> What do you all think ? >> >> [1] https://gist.github.com/jvshahid/b778702cc3d825c6887d2707e866a9c8 >> [2] >> https://github.com/git/git/commit/06bf4ad1db92c32af38e16d9b7f928edbd647780 > > Brandon? I cannot quite tell from the report what "has changed" > refers to, what failures "you cannot run" gets, and if that is a > desirable thing to do (i.e. if letting the user run it in such a > configuration would somehow break things, actively erroring out may > be a deliberate change) or not (i.e. an unintended regression). > Before the refspec was passed down into the submodules, we'd just invoke "git push" in the submodule assuming the user setup a remote tracking branch and a push strategy such that "git push" would do the right thing. And because the submodule is configured independently, it doesn't matter which branch you're on in the superproject. Looking at the test[1], you run "git push --recurse-submodules" without any remote/branch that was called out in the commit message[2] to not have changed. Is that understanding correct? Looking at the test cases of [2] we did not test for explicit "still works with no args given", though one could have expected we'd have a test for that already. :/ Thanks, Stefan
Re: [PATCH] doc: filter-branch does not require re-export of vars
On Sun, May 28, 2017 at 09:35:30PM -0400, Samuel Lijin wrote: > > However, I think POSIX mandates the behavior you'd expect. And the only > > shell I know that misbehaves in this way is Solaris /bin/sh, which we > > have already declared too broken to support. > > Off-topic, but where is this explicitly documented? I couldn't find a place that mentioned it explicitly, but POSIX defines the concept as "the export attribute" of the variables. Which implies to me that the bit is tied to the variable itself, not its value. It also says that the flag is cleared when a variable is unset, so: foo=one export foo sh -c 'echo $foo should be one' unset foo foo=two sh -c 'echo $foo is not exported' That could potentially affect somebody writing a filter-branch snippet, but presumably if they are using "unset" they know what they're doing. -Peff
Re: 2.13.0-rc1 is broken on SPARC
"Liam R. Howlett"writes: > My SPARC build does not function and seg bus terminates on any command. Sorry, known issue in the released version 2.13 (we would have appreciated if a bug report for -rc1 came way before the decision to tag the final release). A fix exists on 'pu' (you can merge or cherry-pick a0103914c2); it was unfortunately held up with an unrelated enthusiasm around an attempt to use submodule to manage this codebase in our project. I'll try to untangle the fix proper and quickly merge down to the maintenance track. Thanks for a report.
Respond as soon as you receive this massage
#104 Lambert avenue 144/150 Adoglita Cotonou Republic Du Benin. Tel: 229. 5654.56 77 Attention:Sir/Madam Your overdue payment sum of $3,500,000.00 USD has been released today from Federal High Court Benin and we are hereby to let you know that the first payment of $5000 with the MTCN: (156-673-) have been transferred to your Name for you to pick up but you have to pay the sum of $79 usd to activate it before you can pick up the $5000 .contact now so that we can give you the Receivers name to send the activation fee and collect your $5000 immediately within 45 minutes. Again we give you 12hrs to respond and after 12hrs you did not respond we can cancel your payment file and return the total fund to Government Treasury Account kindly contact Rev BAR C.F Wilson for your payment, Contact information: E-mail: un...@fastservice.com Name: Rev BAR C.F Wilson , Tele: +229 6277 1872 Respond as soon as you receive this massage we are waiting, Regard MR.RICHARD MARK, Director Federal Ministry Of, Finance Benin Republic Federal Ministry Of Finance Benin Republic
Re: git push recurse.submodules behavior changed in 2.13
John Shahidwrites: > It looks like the git push recurse-submodules behavior has changed. > Currently with 2.13 you cannot run "git push > --recurse-submodules=on-demand" if the parent repo is on a different > branch than the sub repos, e.g. parent repo is on "develop" and > sub-repo on "master". I created a test that can be found here [1]. > > A bisect shows that the change to propagate refspec [2] to the > submodules is the culprit. imho this is an undesired change in > behavior. I looked at the code but couldn't see an easy way to fix > this issue without breaking the feature mentioned above. The only > option I can think of is to control the refspec propagation behavior > using a flag, e.g. "--propagate-refspecs" or add another > recurse-submodules option, e.g. "--recurse-submodules=propagate" > > What do you all think ? > > [1] https://gist.github.com/jvshahid/b778702cc3d825c6887d2707e866a9c8 > [2] https://github.com/git/git/commit/06bf4ad1db92c32af38e16d9b7f928edbd647780 Brandon? I cannot quite tell from the report what "has changed" refers to, what failures "you cannot run" gets, and if that is a desirable thing to do (i.e. if letting the user run it in such a configuration would somehow break things, actively erroring out may be a deliberate change) or not (i.e. an unintended regression).
2.13.0-rc1 is broken on SPARC
Hello, My SPARC build does not function and seg bus terminates on any command. I have worked out the commit is the one that switched the default SHA1 code to sha1dc - commit e6b07da2780f349c29809bd75d3eca6ad3c35d19. I managed to get it working by following the instructions on updating the library in commit 28dc98e343ca4eb370a29ceec4c19beac9b5c01e. Should I generate & send a patch for this? I would normally do just that, but with an external library, the resulting patch required conflict resolution on the merging so format-patch won't work with the given instructions. I should note that following the build instructions in commit e6b07da2780f349c29809bd75d3eca6ad3c35d19 to switch back to openssl's SHA1 implementation works fine. Thanks, Liam
Re: [PATCH] pull: ff --rebase --autostash works in dirty repo
Tyler Brazierwrites: > pull --rebase --autostash was failing on a fast-forward in a dirty repo > since we shortcut to run_merge(), which does not know how to autostash. > The shortcut is a performance optimization, and since rebase was > rewritten in C, it seemed okay to just bypass the shortcut if we > autostash. > --- Please clarify "was failing". I suspect that When we can fast-forward to the updated upstream, "git pull --rebase --autostash" in a dirty repository did not auto-stash and failed. This was due to a short-cut to avoid running rebase when we can fast-forward, but the autostash option was ignored in that codepath. or something like that was what you meant. "rebase" was not rewritten in C. Not the part that matters in making "pull --rebase" work anyway. Unlike the one in the earlier discussion, you are not removing the short-cut unconditionally; I do not think you need to justify the "bypassing" based on performance. If we need to take run_rebase() codepath when "--autostash" is in effect, we need to do so even if the result were somewhat slower for correctness (and it would not hurt to mention that actual measurement showed that it is dubious it is slower in the first place). Missing sign-off. > builtin/pull.c | 5 +++-- > t/t5520-pull.sh | 18 ++ > 2 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/builtin/pull.c b/builtin/pull.c > index dd1a4a94e41e..609e594d3f28 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -772,6 +772,7 @@ int cmd_pull(int argc, const char **argv, const char > *prefix) > struct oid_array merge_heads = OID_ARRAY_INIT; > struct object_id orig_head, curr_head; > struct object_id rebase_fork_point; > + int autostash; > > if (!getenv("GIT_REFLOG_ACTION")) > set_reflog_message(argc, argv); > @@ -800,8 +801,8 @@ int cmd_pull(int argc, const char **argv, const char > *prefix) > if (!opt_rebase && opt_autostash != -1) > die(_("--[no-]autostash option is only valid with --rebase.")); > > + autostash = config_autostash; > if (opt_rebase) { > - int autostash = config_autostash; > if (opt_autostash != -1) > autostash = opt_autostash; > > @@ -868,7 +869,7 @@ int cmd_pull(int argc, const char **argv, const char > *prefix) > head = lookup_commit_reference(orig_head.hash); > commit_list_insert(head, ); > merge_head = lookup_commit_reference(merge_heads.oid[0].hash); > - if (is_descendant_of(merge_head, list)) { > + if (!autostash && is_descendant_of(merge_head, list)) { > /* we can fast-forward this without invoking rebase */ > opt_ff = "--ff-only"; > return run_merge(); The scope of the "autostash" feels a bit unfortunate, but that is a direct consequence of having two "if (opt_rebase)" separated in the control flow, so it's not your fault. When autostsash is in effect, you _know_ you do not have to compute is-descendant-of and you do not have to prepare merge_head or list here. I do not like deeply indented code, but perhaps like this one on top of your patch? builtin/pull.c | 22 -- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index 609e594d3f..42f0560252 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -863,16 +863,18 @@ int cmd_pull(int argc, const char **argv, const char *prefix) die(_("Cannot rebase onto multiple branches.")); if (opt_rebase) { - struct commit_list *list = NULL; - struct commit *merge_head, *head; - - head = lookup_commit_reference(orig_head.hash); - commit_list_insert(head, ); - merge_head = lookup_commit_reference(merge_heads.oid[0].hash); - if (!autostash && is_descendant_of(merge_head, list)) { - /* we can fast-forward this without invoking rebase */ - opt_ff = "--ff-only"; - return run_merge(); + if (!autostash) { + struct commit_list *list = NULL; + struct commit *merge_head, *head; + + head = lookup_commit_reference(orig_head.hash); + commit_list_insert(head, ); + merge_head = lookup_commit_reference(merge_heads.oid[0].hash); + if (is_descendant_of(merge_head, list)) { + /* we can fast-forward this without invoking rebase */ + opt_ff = "--ff-only"; + return run_merge(); + } } return run_rebase(_head, merge_heads.oid, _fork_point); } else { > diff
Re: [PATCH/RFC] branch: add tests for new copy branch feature
Sahil Duawrites: > New feature - copying a branch along with its config section. That's an unusual non-sentence (without a verb) in our commit message. > Aim is to have an option -c for copying a branch just like -m option for > renaming a branch. What should it copy literally, and what should it copy with adjustment (and adjusting what), and what should it refrain from copying? For example, when you create a branch topic-2 by copying from a branch topic-1, do you copy the reflog in such a way that topic-2's reflog contains all the entries of topic-1 before the copy happens, capped with a new (and not shared with topic-1) entry that says "Copied from topic-1"? When topic-1 is set to pull from a custom upstream 'upstream' (i.e. not "origin")'s 'trunk' branch, by setting 'branch.topic-2.remote' to "upstream", i.e. the same as the value of 'branch.topic-1.remote' and 'branch.topic-2.merge' to "trunk", i.e. the same as 'branch.topic-1.merge'? Should a "git push" while topic-2 is checked out push to the same remote as a "git push" would push to when topic-1 is checked out? Should they update the same branch at the remote? Why and/or why not? [*1*] > This commit adds a few basic tests for getting any suggestions/feedback > about expected behavior for this new feature. Writing tests is a good way to prepare for an implementation, which must be done according to the design, but that happens after the design is polished and judged to be a good one. While soliciting comments on the design from others, tests are a bit too low level to express what you are aiming at. It is a bit unhelpful to those who may want to help to figure out answers to questions like the ones in the previos paragraph (the one that begins with "For example,...") by telling them to "read my intention from these tests", and when you want as wide an input as possible, that is counterproductive for your objective ;-). > Signed-off-by: Sahil Dua > --- > t/t3200-branch.sh | 53 + > 1 file changed, 53 insertions(+) > > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh > index fe62e7c775da..2c95ed6ebf3c 100755 > --- a/t/t3200-branch.sh > +++ b/t/t3200-branch.sh > @@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, > too' ' > test_must_fail git config branch.s/s/dummy > ' > > +test_expect_success 'git branch -c dumps usage' ' > + test_expect_code 128 git branch -c 2>err && > + test_i18ngrep "branch name required" err > +' OK. Do we want to support a single-name format (i.e. copy from the current)? > +git config branch.d.dummy Hello Write your tests to do as little outside test_expct_success block as possible, and do a set-up close to where it matters. > +test_expect_success 'git branch -c d e should work' ' > + git branch -l d && > + git reflog exists refs/heads/d && > + git branch -c d e && > + git reflog exists refs/heads/d && > + git reflog exists refs/heads/e > +' A reasonable design of "copy" is for d's log to be left intact, and e's log to begin with a single entry "created by copying from d". Another possible design is to copy the log (making them identical), and then add one entry to e's log "created by copying from d". The above test would succeed with either implementation and does not answer "should reflog be copied?" (see above about why "here are the tests that shows my thinking--read them and comment" is a bad approach). "move" that makes the source of the movement disappear after the operation has a stronger justification of moving the log under the new name (the only alternative is to lose the history of the source forever), it is debatable if "copy" wants to retain a copy of the history of an otherwise unrelated branch, which has its own history retained after the operation. > + > +test_expect_success 'config information was copied, too' ' > + test $(git config branch.e.dummy) = Hello && > + test $(git config branch.d.dummy) = Hello > +' The expectation is that a configuration like 'dummy' that does not have any meaning to Git itself will all blindly be copied, which is similar to what "move" does. > +git config branch.f/f.dummy Hello > + > +test_expect_success 'git branch -c f/f g/g should work' ' > + git branch -l f/f && > + git reflog exists refs/heads/f/f && > + git branch -c f/f g/g && > + git reflog exists refs/heads/f/f && > + git reflog exists refs/heads/g/g > +' Hmph. What's the reason to expect this not to work? > +test_expect_success 'config information was copied, too' ' > + test $(git config branch.f/f.dummy) = Hello && > + test $(git config branch.g/g.dummy) = Hello > +' Isn't this part of the "should work" test above? The definition of working is not just reflog is created for the new branch without the old branch losing its reflog, right? > +test_expect_success 'git branch -c m2 m2 should
Re: [PATCH] doc: filter-branch does not require re-export of vars
On Fri, May 26, 2017 at 2:37 PM, Jeff Kingwrote: > > On Fri, May 26, 2017 at 07:36:54PM +0200, Andreas Heiduk wrote: > > > The function `set_ident` in `filter-branch` exported the variables > > GIT_(AUTHOR|COMMITTER)_(NAME|EMAIL|DATE) at least since 6f6826c52b in 2007. > > Therefore the filter scripts don't need to re-eport them again. > > Some old shells keep separate values for the internal and exporter > versions of variables. I.e., this: > > foo=one > export foo > foo=two > > would continue to export $foo as "one", even though it is "two" inside > the script. > > However, I think POSIX mandates the behavior you'd expect. And the only > shell I know that misbehaves in this way is Solaris /bin/sh, which we > have already declared too broken to support. Off-topic, but where is this explicitly documented? > According to > > > https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Limitations-of-Builtins.html#export > > it sounds like there are some other antique shells which may do the > same (it doesn't cover this case explicitly, but the "coexist" cases it > mentions are likely to behave in this way). > > At this point, I'd be inclined to remove the text as you suggest and > either make a small note at the bottom of the page, or just omit it > entirely and assume that anybody on an old non-POSIX shell can fend for > themselves. > > -Peff
Re: [PATCH] branch test: fix invalid config key access
Ævar Arnfjörð Bjarmasonwrites: > On Sun, May 28, 2017 at 7:12 PM, Sahil Dua wrote: >> Fixes the test by changing "branch.s/s/dummy" to "branch.s/s.dummy" which is >> the right way of accessing config key "branch.s/s.dummy". Purpose of >> this test is to confirm that this key doesn't exist after the branch >> "s/s" has been renamed to "s". >> >> Earlier it was trying to access invalid config key and hence was getting >> an error. However, this wasn't caught because we were expecting the >> command to fail for other reason as mentioned above. > > Looks obviously correct to me. Added by Johannes in commit dc81c58cd6 > ("git-branch: rename config vars branch..*, too", 2006-12-16), > CC-ing him in case he has anything to say about it, but just looks > like a typo back in 2006. Yeah, it does look like a simple typo. Sorry for not spotting it back then. >> Signed-off-by: Sahil Dua >> --- >> t/t3200-branch.sh | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh >> index fe62e7c..10f8f02 100755 >> --- a/t/t3200-branch.sh >> +++ b/t/t3200-branch.sh >> @@ -338,7 +338,7 @@ test_expect_success 'git branch -m s/s s should work >> when s/t is deleted' ' >> >> test_expect_success 'config information was renamed, too' ' >> test $(git config branch.s.dummy) = Hello && >> - test_must_fail git config branch.s/s/dummy >> + test_must_fail git config branch.s/s.dummy >> ' >> >> test_expect_success 'deleting a symref' ' >> -- >> 2.7.4 (Apple Git-66) >>
Re: [RFC/PATCH] WIP: add deprecation & experimental process/interface
Ævar Arnfjörð Bjarmasonwrites: > diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN > index 4f94fc7574..c76bbedf86 100755 > --- a/GIT-VERSION-GEN > +++ b/GIT-VERSION-GEN > @@ -37,4 +37,5 @@ fi > test "$VN" = "$VC" || { > echo >&2 "GIT_VERSION = $VN" > echo "GIT_VERSION = $VN" >$GVF > + echo "GIT_VERSION_INT = $(echo $VN | sed -e > 's/^\([0-9]*\)\.\([0-9]*\)\..*/\1\2/')" >>$GVF > } Unlike Perl's v1.2.3.4 notation, this forces us worry when we go from v2.99.0 to v2.100.0 and eventually to v3.0, no? > + } else if (1) { > + /* > + * TODO: Instead of `if 1` we should check a > + * core.version variable here. > + * > + * I.e. if set to core.version=2.13 the user is opting > + * in to get deprecations set at dep_at right away, > + * and also perhaps experimental features from a > + * sister experimental() interface. > + */ This essentially forces us to always read _some_ configuration. Some commands are meant to work outside repositories, so those who want to affect them needs to write core.version in their global configuration. Some low-level plumbing commands may want to do absolute minimum without configurablity. I am not saying that it is absolutely a bad design decision to force us to read some configuration (yet); it's just that it is something that we have to keep in mind and always think about the ramifications of. > + die(_("Early bird deprecation error: %s"), message); > + } > +}
Re: [PATCH] change 'commands' to comments and improve wording
Adrianwrites: (nothing) Imagine a reader who finds the title of this commit in 3 weeks from now among 200 other commits. Do you think the reader can guess that this is a documentation fix? Can the reader tell this is about "stash"? Subject: stash doc: write explanation as comments, not as commands or something, perhaps. This commit is simple enough that it may not need the body but explaining why would not hurt. Perhaps like this: In the illustration of workflows in "git stash" documentation, where it shows sequence of 'git' commands, there are a few steps that are not literal commands (i.e. "here, you would use your editor"). Clarify that these are not something the user can blindly cut-and-paste by turning them into comments. would be clear enough. > --- And please sign-off (Documentation/SubmittingPatches) your patch. Thanks. > Documentation/git-stash.txt | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt > index 70191d06b69e..3899d36b2bab 100644 > --- a/Documentation/git-stash.txt > +++ b/Documentation/git-stash.txt > @@ -233,7 +233,7 @@ return to your original branch to make the emergency fix, > like this: > $ git checkout -b my_wip > $ git commit -a -m "WIP" > $ git checkout master > -$ edit emergency fix > +# ... edit emergency fix ... > $ git commit -a -m "Fix in a hurry" > $ git checkout my_wip > $ git reset --soft HEAD^ > @@ -245,7 +245,7 @@ You can use 'git stash' to simplify the above, like this: > > # ... hack hack hack ... > $ git stash > -$ edit emergency fix > +# ... edit emergency fix ... > $ git commit -a -m "Fix in a hurry" > $ git stash pop > # ... continue hacking ... > @@ -261,11 +261,11 @@ each change before committing: > # ... hack hack hack ... > $ git add --patch foo# add just first part to the index > $ git stash save --keep-index# save all other changes to the stash > -$ edit/build/test first part > +# ... edit, build and test first part ... > $ git commit -m 'First part' # commit fully tested change > $ git stash pop # prepare to work on all other changes > # ... repeat above five steps until one commit remains ... > -$ edit/build/test remaining parts > +# ... edit, build and test remaining parts ... > $ git commit foo -m 'Remaining parts' > > > > -- > https://github.com/git/git/pull/361
Re: [PATCH] wildmatch test: remove redundant duplicate test
Thanks. Did you run "sort | uniq -c" on it or something ;-)? Will apply.
Re: git-2.13.0: log --date=format:%z not working
Ævar Arnfjörð Bjarmasonwrites: >> >> Here are some links to past explorations: >> >> http://public-inbox.org/git/20160208152858.ga17...@sigill.intra.peff.net/ >> >> http://public-inbox.org/git/87vb2d37ea@web.de/ > > There's a third and possibly least shitty option that isn't covered in > those threads; We could just make a pass over the strftime format > ourselves and replace %z and %Z with the timezone (as done for > DATE_ISO8601_STRICT in date.c), then hand the rest off to strftime(). I do not know about %Z but certainly for %z that sounds the least bad. In a nearby message René seems to have come up with the same idea, too ;-)
Re: [PATCH] doc: filter-branch does not require re-export of vars
Jeff Kingwrites: > On Fri, May 26, 2017 at 07:36:54PM +0200, Andreas Heiduk wrote: > >> The function `set_ident` in `filter-branch` exported the variables >> GIT_(AUTHOR|COMMITTER)_(NAME|EMAIL|DATE) at least since 6f6826c52b in 2007. >> Therefore the filter scripts don't need to re-eport them again. > > Some old shells keep separate values for the internal and exporter > versions of variables. I.e., this: > > foo=one > export foo > foo=two > > would continue to export $foo as "one", even though it is "two" inside > the script. Yup, that sounds like a grandfather's war story at this point, though ;-) > However, I think POSIX mandates the behavior you'd expect. ... > ... > At this point, I'd be inclined to remove the text as you suggest and > either make a small note at the bottom of the page, or just omit it > entirely and assume that anybody on an old non-POSIX shell can fend for > themselves. Sounds good to me. Thanks.
[PATCH] pull: ff --rebase --autostash works in dirty repo
pull --rebase --autostash was failing on a fast-forward in a dirty repo since we shortcut to run_merge(), which does not know how to autostash. The shortcut is a performance optimization, and since rebase was rewritten in C, it seemed okay to just bypass the shortcut if we autostash. --- builtin/pull.c | 5 +++-- t/t5520-pull.sh | 18 ++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index dd1a4a94e41e..609e594d3f28 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -772,6 +772,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) struct oid_array merge_heads = OID_ARRAY_INIT; struct object_id orig_head, curr_head; struct object_id rebase_fork_point; + int autostash; if (!getenv("GIT_REFLOG_ACTION")) set_reflog_message(argc, argv); @@ -800,8 +801,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (!opt_rebase && opt_autostash != -1) die(_("--[no-]autostash option is only valid with --rebase.")); + autostash = config_autostash; if (opt_rebase) { - int autostash = config_autostash; if (opt_autostash != -1) autostash = opt_autostash; @@ -868,7 +869,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) head = lookup_commit_reference(orig_head.hash); commit_list_insert(head, ); merge_head = lookup_commit_reference(merge_heads.oid[0].hash); - if (is_descendant_of(merge_head, list)) { + if (!autostash && is_descendant_of(merge_head, list)) { /* we can fast-forward this without invoking rebase */ opt_ff = "--ff-only"; return run_merge(); diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 17f4d0fe4e72..4c85be0417cf 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -272,6 +272,24 @@ test_expect_success '--rebase fast forward' ' test_cmp reflog.expected reflog.fuzzy ' +test_expect_success '--rebase --autostash fast forward' ' + test_when_finished " + git reset --hard; + git checkout to-rebase; + git branch -D to-rebase-ff; + git branch -D behind" && + git branch behind && + git checkout -b to-rebase-ff && + echo another modification >>file && + git add file && + git commit -m mod && + + git checkout behind && + echo dirty >file && + git pull --rebase --autostash . to-rebase-ff && + test "$(git rev-parse HEAD)" = "$(git rev-parse to-rebase-ff)" +' + test_expect_success '--rebase with conflicts shows advice' ' test_when_finished "git rebase --abort; git checkout -f to-rebase" && git checkout -b seq && -- https://github.com/git/git/pull/365
Re: [PATCH/RFC] branch: add tests for new copy branch feature
On Mon, May 29, 2017 at 12:56 AM, Sahil Duawrote: > New feature - copying a branch along with its config section. > > Aim is to have an option -c for copying a branch just like -m option for > renaming a branch. > > This commit adds a few basic tests for getting any suggestions/feedback > about expected behavior for this new feature. > > Signed-off-by: Sahil Dua > --- > t/t3200-branch.sh | 53 + > 1 file changed, 53 insertions(+) > > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh > index fe62e7c775da..2c95ed6ebf3c 100755 > --- a/t/t3200-branch.sh > +++ b/t/t3200-branch.sh > @@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, > too' ' > test_must_fail git config branch.s/s/dummy > ' > > +test_expect_success 'git branch -c dumps usage' ' > + test_expect_code 128 git branch -c 2>err && > + test_i18ngrep "branch name required" err > +' > + > +git config branch.d.dummy Hello > + > +test_expect_success 'git branch -c d e should work' ' > + git branch -l d && > + git reflog exists refs/heads/d && > + git branch -c d e && > + git reflog exists refs/heads/d && > + git reflog exists refs/heads/e > +' > + > +test_expect_success 'config information was copied, too' ' > + test $(git config branch.e.dummy) = Hello && > + test $(git config branch.d.dummy) = Hello > +' > + > +git config branch.f/f.dummy Hello > + > +test_expect_success 'git branch -c f/f g/g should work' ' > + git branch -l f/f && > + git reflog exists refs/heads/f/f && > + git branch -c f/f g/g && > + git reflog exists refs/heads/f/f && > + git reflog exists refs/heads/g/g > +' > + > +test_expect_success 'config information was copied, too' ' > + test $(git config branch.f/f.dummy) = Hello && > + test $(git config branch.g/g.dummy) = Hello > +' > + > +test_expect_success 'git branch -c m2 m2 should work' ' > + git branch -l m2 && > + git reflog exists refs/heads/m2 && > + git branch -c m2 m2 && > + git reflog exists refs/heads/m2 > +' > + > +test_expect_success 'git branch -c a a/a should fail' ' > + git branch -l a && > + git reflog exists refs/heads/a && > + test_must_fail git branch -c a a/a > +' > + > +test_expect_success 'git branch -c b/b b should fail' ' > + git branch -l b/b && > + test_must_fail git branch -c b/b b > +' > + > test_expect_success 'deleting a symref' ' > git branch target && > git symbolic-ref refs/heads/symref refs/heads/target && > This looks good to me. Comments, in no particular order: * There should be a test for `git branch -c `, i.e. that should implicitly copy from HEAD just like `git branch -m ` does. However, when looking at this I can see there's actually no test for one-argument `git branch -m`, i.e. if you apply this: --- a/builtin/branch.c +++ b/builtin/branch.c @@ -699,8 +699,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix) } else if (rename) { if (!argc) die(_("branch name required")); - else if (argc == 1) - rename_branch(head, argv[0], rename > 1); else if (argc == 2) rename_branch(argv[0], argv[1], rename > 1); else The only test that fails is a `git branch -M master`, i.e. one-argument -M is tested for, but not -m, same codepath though, but while you're at it a patch/series like this could start by adding a one-arg -m test, then follow-up by copying that for the new `-c`. * We should have a -C to force -c just like -M forces -m, i.e. a test where one-arg `git branch -C alreadyexists` clobbers alreadyexists, and `git branch -C source alreadyexists` does the same for two-arg. * I know this is just something you're copying, but this `git branch -l ` use gets me every time "wait how does listing it help isn't that always succeeding ... damnit it's --create-reflog not --list, got me again" :) Just noting it in case it confuses other reviewers who are skimming this. Might be worth it to just use --create-reflog for new tests instead of the non-obvious -l whatever the existing tests in the file do, or maybe I'm the only one confused by this :) * When you use `git branch -m` it adds a note to the reflog, your patch should have a test like the existing "git branch -M baz bam should add entries to .git/logs/HEAD" test in this file except "Branch: copied ..." instead of "Branch: renamed...". * Should there be tests for `git branch -c master master` like we have for `git branch -m master master` (see 3f59481e33 ("branch: allow a no-op "branch -M HEAD"", 2011-11-25)). Allowing this for -m smells like a bend-over-backwards workaround for some script Jonathan had, should we be doing this for -c too? I don't know.
[PATCH/RFC] branch: add tests for new copy branch feature
New feature - copying a branch along with its config section. Aim is to have an option -c for copying a branch just like -m option for renaming a branch. This commit adds a few basic tests for getting any suggestions/feedback about expected behavior for this new feature. Signed-off-by: Sahil Dua--- t/t3200-branch.sh | 53 + 1 file changed, 53 insertions(+) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index fe62e7c775da..2c95ed6ebf3c 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, too' ' test_must_fail git config branch.s/s/dummy ' +test_expect_success 'git branch -c dumps usage' ' + test_expect_code 128 git branch -c 2>err && + test_i18ngrep "branch name required" err +' + +git config branch.d.dummy Hello + +test_expect_success 'git branch -c d e should work' ' + git branch -l d && + git reflog exists refs/heads/d && + git branch -c d e && + git reflog exists refs/heads/d && + git reflog exists refs/heads/e +' + +test_expect_success 'config information was copied, too' ' + test $(git config branch.e.dummy) = Hello && + test $(git config branch.d.dummy) = Hello +' + +git config branch.f/f.dummy Hello + +test_expect_success 'git branch -c f/f g/g should work' ' + git branch -l f/f && + git reflog exists refs/heads/f/f && + git branch -c f/f g/g && + git reflog exists refs/heads/f/f && + git reflog exists refs/heads/g/g +' + +test_expect_success 'config information was copied, too' ' + test $(git config branch.f/f.dummy) = Hello && + test $(git config branch.g/g.dummy) = Hello +' + +test_expect_success 'git branch -c m2 m2 should work' ' + git branch -l m2 && + git reflog exists refs/heads/m2 && + git branch -c m2 m2 && + git reflog exists refs/heads/m2 +' + +test_expect_success 'git branch -c a a/a should fail' ' + git branch -l a && + git reflog exists refs/heads/a && + test_must_fail git branch -c a a/a +' + +test_expect_success 'git branch -c b/b b should fail' ' + git branch -l b/b && + test_must_fail git branch -c b/b b +' + test_expect_success 'deleting a symref' ' git branch target && git symbolic-ref refs/heads/symref refs/heads/target && -- https://github.com/git/git/pull/363
Re: [PATCH v3 4/4] stash: implement builtin stash
On Sun, May 28, 2017 at 11:31:48AM -0700, Joel Teichroeb wrote: > >> + /* TODO: Improve this logic */ > >> + strbuf_addf(, "%s", REV); > >> + str = strstr(symbolic.buf, "@"); > > > > Could you elaborate on how this should be improved? > > I just figured there would be a builtin function that could help here, > but hadn't had the chance to look into it. It's something easy to do > in bash, but more complicated in C. There's no strbuf function for "truncate at this character". But: - you can use strchr for a single-character match, which is more efficient; i.e.: str = strchr(symbolic.buf, '@'); - instead of inserting a '\0' into the strbuf, use strbuf_setlen(), which also updates the symbolic.len; i.e.: strbuf_setlen(, str - symbolic.buf); - it looks like you copy into the strbuf just to truncate, so you could actually get the final size before inserting into the strbuf using strchrnul. Like: end_of_rev = strchrnul(REV, '@'); strbuf_add(, REV, end_of_rev - REV); -Peff
Re: [PATCH v3 4/4] stash: implement builtin stash
On Sun, May 28, 2017 at 08:51:07PM +0200, Ævar Arnfjörð Bjarmason wrote: > On Sun, May 28, 2017 at 6:56 PM, Joel Teichroebwrote: > > Implement all git stash functionality as a builtin command > > > > Signed-off-by: Joel Teichroeb > > --- > > General note on this that I missed in my first E-Mail, you have ~20 > calls to argv_array_init() but none to argv_array_clear(). So you're > leaking memory, and it obscures potential other issues with valgrind. I haven't looked carefully at the patches, but note that the argv array in a child_process is handled automatically by start/finish_command. So: > @@ -1107,9 +,9 @@ static int list_stash(int argc, const char > **argv, const char *prefix) > argv_array_pushv(, argv); > argv_array_push(, ref_stash); > if (cmd_log(args.argc, args.argv, prefix)) > - return 1; > - > - return 0; > + ret = 1; > + argv_array_clear(); > + return ret; > } This one does need a clear, because it's calling an internal function. But... > @@ -741,13 +740,18 @@ static int do_push_stash(const char *prefix, > const char *message, > argv_array_push(, "--"); > argv_array_pushv(, argv); > pipe_command(, NULL, 0, , 0, NULL, 0); > + argv_array_clear(); This one does not, because the array will have been cleared by calling pipe_command (because it does the start/finish block itself; and yes, before you go checking, start_command() clears it even when it returns error). > + child_process_clear(); This should not be necessary for the same reason. > - cp2.git_cmd = 1; > - argv_array_push(, "checkout-index"); > - argv_array_push(, "-z"); > - argv_array_push(, "--force"); > - argv_array_push(, "--stdin"); > - pipe_command(, out.buf, out.len, NULL, 0, NULL, > 0); > + child_process_init(); > + cp.git_cmd = 1; > + argv_array_push(, "checkout-index"); > + argv_array_push(, "-z"); > + argv_array_push(, "--force"); > + argv_array_push(, "--stdin"); > + pipe_command(, out.buf, out.len, NULL, 0, NULL, 0); > + argv_array_clear(); > + child_process_clear(); And ditto here. I'd also encourage using CHILD_PROCESS_INIT and ARGV_ARRAY_INIT constant initializers instead of their function-call counterparts, as that matches our usual style. -Peff
Re: [PATCH v3 0/4] Implement git stash as a builtin command
On Sun, May 28, 2017 at 6:56 PM, Joel Teichroebwrote: > I've rewritten git stash as a builtin c command. All tests pass, > and I've added two new tests. Test coverage is around 95% with the > only things missing coverage being error handlers. Worth noting, with your patches the best of 3 run of all the stash tests: $ for i in {1..3}; do (time prove -j1 t*-stash*.sh) 2>&1 | grep ^real; done |awk '{print $2}' | sort -n | head -n 1 0m3.293s Without: $ for i in {1..3}; do (time prove -j1 t*-stash*.sh) 2>&1 | grep ^real; done |awk '{print $2}' | sort -n | head -n 1 0m7.619s Of course some individual invocations are much faster than that, this includes all the shell overhead of the tests themselves.
Re: [PATCH v3 4/4] stash: implement builtin stash
On Sun, May 28, 2017 at 6:56 PM, Joel Teichroebwrote: > Implement all git stash functionality as a builtin command > > Signed-off-by: Joel Teichroeb > --- General note on this that I missed in my first E-Mail, you have ~20 calls to argv_array_init() but none to argv_array_clear(). So you're leaking memory, and it obscures potential other issues with valgrind. A lot of that's easy to solve, but sometimes requires a temporary variable since the code is now returning directly, e.g: @@ -1091,6 +1094,7 @@ static int list_stash(int argc, const char **argv, const char *prefix) struct object_id obj; struct object_context unused; struct argv_array args; + int ret = 0; argc = parse_options(argc, argv, prefix, options, git_stash_list_usage, PARSE_OPT_KEEP_UNKNOWN); @@ -1107,9 +,9 @@ static int list_stash(int argc, const char **argv, const char *prefix) argv_array_pushv(, argv); argv_array_push(, ref_stash); if (cmd_log(args.argc, args.argv, prefix)) - return 1; - - return 0; + ret = 1; + argv_array_clear(); + return ret; } But more generally this goes a long way to resolving the issue where you have variables like out1, out2 or cp1, cp2 etc. which Christian pointed out. I.e. you're not freeing/clearing strbufs either, instead just creating new ones that also aren't freed, or not clearing child_process structs, e.g. this on top allows you to re-use the same variable and stops leaking memory: diff --git a/builtin/stash.c b/builtin/stash.c index bf36ff8f9b..4e7344501a 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -729,7 +729,6 @@ static int do_push_stash(const char *prefix, const char *message, if (keep_index) { struct child_process cp = CHILD_PROCESS_INIT; - struct child_process cp2 = CHILD_PROCESS_INIT; struct strbuf out = STRBUF_INIT; reset_tree(info.i_tree, 0, 1); @@ -741,13 +740,18 @@ static int do_push_stash(const char *prefix, const char *message, argv_array_push(, "--"); argv_array_pushv(, argv); pipe_command(, NULL, 0, , 0, NULL, 0); + argv_array_clear(); + child_process_clear(); - cp2.git_cmd = 1; - argv_array_push(, "checkout-index"); - argv_array_push(, "-z"); - argv_array_push(, "--force"); - argv_array_push(, "--stdin"); - pipe_command(, out.buf, out.len, NULL, 0, NULL, 0); + child_process_init(); + cp.git_cmd = 1; + argv_array_push(, "checkout-index"); + argv_array_push(, "-z"); + argv_array_push(, "--force"); + argv_array_push(, "--stdin"); + pipe_command(, out.buf, out.len, NULL, 0, NULL, 0); + argv_array_clear(); + child_process_clear(); } } else { struct child_process cp2 = CHILD_PROCESS_INIT;
Re: [PATCH v3 4/4] stash: implement builtin stash
On Sun, May 28, 2017 at 11:26 AM, Ævar Arnfjörð Bjarmasonwrote: > On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb wrote: >> Implement all git stash functionality as a builtin command > > First thanks for working on this, it's great. Applied it locally, > passes all tests for me. A couple of comments Christian didn't cover > >> + info->has_u = get_sha1_with_context(u_commit_rev.buf, 0, >> info->u_commit.hash, ) == 0 && >> + get_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, >> ) == 0; >> + >> + >> + /* TODO: Improve this logic */ >> + strbuf_addf(, "%s", REV); >> + str = strstr(symbolic.buf, "@"); > > Could you elaborate on how this should be improved? > I just figured there would be a builtin function that could help here, but hadn't had the chance to look into it. It's something easy to do in bash, but more complicated in C. > >> +static int patch_working_tree(struct stash_info *info, const char *prefix, >> + const char **argv) >> +{ >> + const char *stash_index_path = ".git/foocache2"; > > This foocache path isn't created by the shell code, if it's a new > thing that's needed (and I haven't followed this code in detail, don'n > know what it's for) shouldn't we give it a more descriptive name so > that if git crashes it's obvious what it is? > Opps, I had cleaned that part up locally, but I forgot to push it when switching computers. It'll be better in the next patch set. >> + const char *message = NULL; >> + const char *commit = NULL; >> + struct object_id obj; >> + struct option options[] = { >> + OPT_STRING('m', "message", , N_("message"), >> +N_("stash commit message")), >> + OPT__QUIET(, N_("be quiet, only report errors")), >> + OPT_END() >> + }; >> + argc = parse_options(argc, argv, prefix, options, >> +git_stash_store_usage, 0); > > Nit: In general in this patch the 2nd line of parse_options doesn't > align with a tabwidth of 8. Ditto for indenting function arguments > (e.g. for untracked_files). I'll fix my tab width. Forgot that long lines would change, haha.
Re: [PATCH v3 4/4] stash: implement builtin stash
On Sun, May 28, 2017 at 6:56 PM, Joel Teichroebwrote: > Implement all git stash functionality as a builtin command First thanks for working on this, it's great. Applied it locally, passes all tests for me. A couple of comments Christian didn't cover > + info->has_u = get_sha1_with_context(u_commit_rev.buf, 0, > info->u_commit.hash, ) == 0 && > + get_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, > ) == 0; > + > + > + /* TODO: Improve this logic */ > + strbuf_addf(, "%s", REV); > + str = strstr(symbolic.buf, "@"); Could you elaborate on how this should be improved? > +static int patch_working_tree(struct stash_info *info, const char *prefix, > + const char **argv) > +{ > + const char *stash_index_path = ".git/foocache2"; This foocache path isn't created by the shell code, if it's a new thing that's needed (and I haven't followed this code in detail, don'n know what it's for) shouldn't we give it a more descriptive name so that if git crashes it's obvious what it is? > + const char *message = NULL; > + const char *commit = NULL; > + struct object_id obj; > + struct option options[] = { > + OPT_STRING('m', "message", , N_("message"), > +N_("stash commit message")), > + OPT__QUIET(, N_("be quiet, only report errors")), > + OPT_END() > + }; > + argc = parse_options(argc, argv, prefix, options, > +git_stash_store_usage, 0); Nit: In general in this patch the 2nd line of parse_options doesn't align with a tabwidth of 8. Ditto for indenting function arguments (e.g. for untracked_files).
Re: [PATCH v3 2/4] stash: add test for stashing in a detached state
On Sun, May 28, 2017 at 6:56 PM, Joel Teichroebwrote: > Signed-off-by: Joel Teichroeb > --- > t/t3903-stash.sh | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index aaae221304..b86851ef46 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -808,6 +808,17 @@ test_expect_success 'create with multiple arguments for > the message' ' > test_cmp expect actual > ' > > +test_expect_success 'create in a detached state' ' > + test_when_finished "git checkout master" && > + git checkout HEAD~1 && > + >foo && > + git add foo && > + STASH_ID=$(git stash create) && > + echo "WIP on (no branch): 47d5e0e initial" >expect && > + git show --pretty=%s -s ${STASH_ID} >actual && > + test_cmp expect actual I thought this needed test_i18ncmp, turns out it didn't, neither the original stash code nor your replacement translates this. That's fine, just a note to other reviewers... > +' > + > test_expect_success 'stash -- stashes and restores the file' ' > >foo && > >bar && > -- > 2.13.0 >
Re: [PATCH v3 4/4] stash: implement builtin stash
On Sun, May 28, 2017 at 6:56 PM, Joel Teichroebwrote: [...] > +int untracked_files(struct strbuf *out, int include_untracked, > + const char **argv) > +{ > + struct child_process cp = CHILD_PROCESS_INIT; > + cp.git_cmd = 1; > + argv_array_push(, "ls-files"); > + argv_array_push(, "-o"); > + argv_array_push(, "-z"); You might want to use argv_array_pushl(), for example: argv_array_pushl(, "ls-files", "-o", "-z", NULL); > + if (include_untracked != 2) > + argv_array_push(, "--exclude-standard"); > + argv_array_push(, "--"); > + if (argv) > + argv_array_pushv(, argv); > + return pipe_command(, NULL, 0, out, 0, NULL, 0); > +} > + > +static int check_no_changes(const char *prefix, int include_untracked, > + const char **argv) > +{ > + struct argv_array args1; > + struct argv_array args2; > + struct strbuf out = STRBUF_INIT; > + > + argv_array_init(); > + argv_array_push(, "diff-index"); > + argv_array_push(, "--quiet"); > + argv_array_push(, "--cached"); > + argv_array_push(, "HEAD"); > + argv_array_push(, "--ignore-submodules"); > + argv_array_push(, "--"); Here and in other places also you could use argv_array_pushl(). > + if (argv) > + argv_array_pushv(, argv); > + argv_array_init(); > + argv_array_push(, "diff-files"); > + argv_array_push(, "--quiet"); > + argv_array_push(, "--ignore-submodules"); > + argv_array_push(, "--"); > + if (argv) > + argv_array_pushv(, argv); > + if (include_untracked) > + untracked_files(, include_untracked, argv); > + return cmd_diff_index(args1.argc, args1.argv, prefix) == 0 && > + cmd_diff_files(args2.argc, args2.argv, prefix) == 0 && > + (!include_untracked || out.len == 0); > +} > + > +static int get_stash_info(struct stash_info *info, const char *commit) > +{ > + struct strbuf w_commit_rev = STRBUF_INIT; > + struct strbuf b_commit_rev = STRBUF_INIT; > + struct strbuf i_commit_rev = STRBUF_INIT; > + struct strbuf u_commit_rev = STRBUF_INIT; > + struct strbuf w_tree_rev = STRBUF_INIT; > + struct strbuf b_tree_rev = STRBUF_INIT; > + struct strbuf i_tree_rev = STRBUF_INIT; > + struct strbuf u_tree_rev = STRBUF_INIT; > + struct strbuf commit_buf = STRBUF_INIT; > + struct strbuf symbolic = STRBUF_INIT; > + struct strbuf out = STRBUF_INIT; > + struct object_context unused; > + char *str; > + int ret; > + const char *REV = commit; We use lower case variable names. > + struct child_process cp = CHILD_PROCESS_INIT; > + info->is_stash_ref = 0; > + > + if (commit == NULL) { > + strbuf_addf(_buf, "%s@{0}", ref_stash); > + REV = commit_buf.buf; > + } else if (strlen(commit) < 3) { > + strbuf_addf(_buf, "%s@{%s}", ref_stash, commit); > + REV = commit_buf.buf; > + } > + info->REV = REV; Also the "REV" member of struct stash_info could be lower cased. > + strbuf_addf(_commit_rev, "%s", REV); > + strbuf_addf(_commit_rev, "%s^1", REV); > + strbuf_addf(_commit_rev, "%s^2", REV); > + strbuf_addf(_commit_rev, "%s^3", REV); > + strbuf_addf(_tree_rev, "%s:", REV); > + strbuf_addf(_tree_rev, "%s^1:", REV); > + strbuf_addf(_tree_rev, "%s^2:", REV); > + strbuf_addf(_tree_rev, "%s^3:", REV); > + > + Spurious new line above. > + ret = ( > + get_sha1_with_context(w_commit_rev.buf, 0, > info->w_commit.hash, ) == 0 && > + get_sha1_with_context(b_commit_rev.buf, 0, > info->b_commit.hash, ) == 0 && > + get_sha1_with_context(i_commit_rev.buf, 0, > info->i_commit.hash, ) == 0 && > + get_sha1_with_context(w_tree_rev.buf, 0, info->w_tree.hash, > ) == 0 && > + get_sha1_with_context(b_tree_rev.buf, 0, info->b_tree.hash, > ) == 0 && > + get_sha1_with_context(i_tree_rev.buf, 0, info->i_tree.hash, > ) == 0); > + > + if (!ret) { > + fprintf_ln(stderr, _("%s is not a valid reference"), REV); > + return 1; Maybe use "return error(_("%s is not a valid reference"), REV);" > + } > + > + Spurious new lines above. > + > +static void stash_create_callback(struct diff_queue_struct *q, > + struct diff_options *opt, void *cbdata) > +{ > + int i; > + > + for (i = 0; i < q->nr; i++) { > + struct diff_filepair *p = q->queue[i]; > + const char *path = p->one->path; > + struct stat st; > + remove_file_from_index(_index, path); > + if (!lstat(path, )) > + add_to_index(_index, path, , 0); > +
Re: [PATCH v3 3/4] close the index lock when not writing the new index
On Sun, May 28, 2017 at 6:56 PM, Joel Teichroebwrote: > Signed-off-by: Joel Teichroeb > --- > builtin/add.c | 3 ++- > builtin/mv.c | 8 +--- > builtin/rm.c | 3 ++- > merge-recursive.c | 8 +--- > 4 files changed, 14 insertions(+), 8 deletions(-) > > diff --git a/builtin/add.c b/builtin/add.c > index 9f53f020d0..6b04eb2c71 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -461,7 +461,8 @@ int cmd_add(int argc, const char **argv, const char > *prefix) > if (active_cache_changed) { > if (write_locked_index(_index, _file, COMMIT_LOCK)) > die(_("Unable to write new index file")); > - } > + } else > + rollback_lock_file(_file); >From Documentation/CodingGuidelines: "When there are multiple arms to a conditional and some of them[...]". > > return exit_status; > } > diff --git a/builtin/mv.c b/builtin/mv.c > index 61d20037ad..ccf21de17f 100644 > --- a/builtin/mv.c > +++ b/builtin/mv.c > @@ -293,9 +293,11 @@ int cmd_mv(int argc, const char **argv, const char > *prefix) > if (gitmodules_modified) > stage_updated_gitmodules(); > > - if (active_cache_changed && > - write_locked_index(_index, _file, COMMIT_LOCK)) > - die(_("Unable to write new index file")); > + if (active_cache_changed) { > + if (write_locked_index(_index, _file, COMMIT_LOCK)) > + die(_("Unable to write new index file")); > + } else > + rollback_lock_file(_file); > > return 0; > } > diff --git a/builtin/rm.c b/builtin/rm.c > index fb79dcab18..4c7a91888b 100644 > --- a/builtin/rm.c > +++ b/builtin/rm.c > @@ -389,7 +389,8 @@ int cmd_rm(int argc, const char **argv, const char > *prefix) > if (active_cache_changed) { > if (write_locked_index(_index, _file, COMMIT_LOCK)) > die(_("Unable to write new index file")); > - } > + } else > + rollback_lock_file(_file); > > return 0; > } > diff --git a/merge-recursive.c b/merge-recursive.c > index 62decd51cc..db841c0d38 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -2145,9 +2145,11 @@ int merge_recursive_generic(struct merge_options *o, > if (clean < 0) > return clean; > > - if (active_cache_changed && > - write_locked_index(_index, lock, COMMIT_LOCK)) > - return err(o, _("Unable to write index.")); > + if (active_cache_changed) { > + if (write_locked_index(_index, lock, COMMIT_LOCK)) > + return err(o, _("Unable to write index.")); > + } else > + rollback_lock_file(lock); > > return clean ? 0 : 1; > } > -- > 2.13.0 >
Re: [PATCH v3 1/4] stash: add test for stash create with no files
On Sun, May 28, 2017 at 6:56 PM, Joel Teichroebwrote: > Ensure the command gives the correct return code > > Signed-off-by: Joel Teichroeb > --- > t/t3903-stash.sh | 8 > 1 file changed, 8 insertions(+) > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index 3b4bed5c9a..aaae221304 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -444,6 +444,14 @@ test_expect_failure 'stash file to directory' ' > test foo = "$(cat file/file)" > ' > > +test_expect_success 'stash create - no changes' ' > + git stash clear && > + test_when_finished "git reset --hard HEAD" && > + git reset --hard && > + git stash create > actual && > + test $(cat actual | wc -l) -eq 0 Looks fine like this, I don't think there's any actual portability concern (as with some wrappers), but FWIW you can do this more briefly with the test framework as: test_line_count = 0 actual Although I wonder in this case whether you don't actually mean: [...]>actual && ! test -s actual I.e. isn't the test that there should be no output, not that there shouldn't be a \n there? Or alternatively: test $(cat actual | wc -c) -eq 0 > +' > + > test_expect_success 'stash branch - no stashes on stack, stash-like > argument' ' > git stash clear && > test_when_finished "git reset --hard HEAD" && > -- > 2.13.0 >
Re: [PATCH] branch test: fix invalid config key access
On Sun, May 28, 2017 at 7:12 PM, Sahil Duawrote: > Fixes the test by changing "branch.s/s/dummy" to "branch.s/s.dummy" which is > the right way of accessing config key "branch.s/s.dummy". Purpose of > this test is to confirm that this key doesn't exist after the branch > "s/s" has been renamed to "s". > > Earlier it was trying to access invalid config key and hence was getting > an error. However, this wasn't caught because we were expecting the > command to fail for other reason as mentioned above. Looks obviously correct to me. Added by Johannes in commit dc81c58cd6 ("git-branch: rename config vars branch..*, too", 2006-12-16), CC-ing him in case he has anything to say about it, but just looks like a typo back in 2006. > Signed-off-by: Sahil Dua > --- > t/t3200-branch.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh > index fe62e7c..10f8f02 100755 > --- a/t/t3200-branch.sh > +++ b/t/t3200-branch.sh > @@ -338,7 +338,7 @@ test_expect_success 'git branch -m s/s s should work when > s/t is deleted' ' > > test_expect_success 'config information was renamed, too' ' > test $(git config branch.s.dummy) = Hello && > - test_must_fail git config branch.s/s/dummy > + test_must_fail git config branch.s/s.dummy > ' > > test_expect_success 'deleting a symref' ' > -- > 2.7.4 (Apple Git-66) >
[PATCH] branch test: fix invalid config key access
Fixes the test by changing "branch.s/s/dummy" to "branch.s/s.dummy" which is the right way of accessing config key "branch.s/s.dummy". Purpose of this test is to confirm that this key doesn't exist after the branch "s/s" has been renamed to "s". Earlier it was trying to access invalid config key and hence was getting an error. However, this wasn't caught because we were expecting the command to fail for other reason as mentioned above. Signed-off-by: Sahil Dua--- t/t3200-branch.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index fe62e7c..10f8f02 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -338,7 +338,7 @@ test_expect_success 'git branch -m s/s s should work when s/t is deleted' ' test_expect_success 'config information was renamed, too' ' test $(git config branch.s.dummy) = Hello && - test_must_fail git config branch.s/s/dummy + test_must_fail git config branch.s/s.dummy ' test_expect_success 'deleting a symref' ' -- 2.7.4 (Apple Git-66)
[PATCH v3 3/4] close the index lock when not writing the new index
Signed-off-by: Joel Teichroeb--- builtin/add.c | 3 ++- builtin/mv.c | 8 +--- builtin/rm.c | 3 ++- merge-recursive.c | 8 +--- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index 9f53f020d0..6b04eb2c71 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -461,7 +461,8 @@ int cmd_add(int argc, const char **argv, const char *prefix) if (active_cache_changed) { if (write_locked_index(_index, _file, COMMIT_LOCK)) die(_("Unable to write new index file")); - } + } else + rollback_lock_file(_file); return exit_status; } diff --git a/builtin/mv.c b/builtin/mv.c index 61d20037ad..ccf21de17f 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -293,9 +293,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix) if (gitmodules_modified) stage_updated_gitmodules(); - if (active_cache_changed && - write_locked_index(_index, _file, COMMIT_LOCK)) - die(_("Unable to write new index file")); + if (active_cache_changed) { + if (write_locked_index(_index, _file, COMMIT_LOCK)) + die(_("Unable to write new index file")); + } else + rollback_lock_file(_file); return 0; } diff --git a/builtin/rm.c b/builtin/rm.c index fb79dcab18..4c7a91888b 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -389,7 +389,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix) if (active_cache_changed) { if (write_locked_index(_index, _file, COMMIT_LOCK)) die(_("Unable to write new index file")); - } + } else + rollback_lock_file(_file); return 0; } diff --git a/merge-recursive.c b/merge-recursive.c index 62decd51cc..db841c0d38 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2145,9 +2145,11 @@ int merge_recursive_generic(struct merge_options *o, if (clean < 0) return clean; - if (active_cache_changed && - write_locked_index(_index, lock, COMMIT_LOCK)) - return err(o, _("Unable to write index.")); + if (active_cache_changed) { + if (write_locked_index(_index, lock, COMMIT_LOCK)) + return err(o, _("Unable to write index.")); + } else + rollback_lock_file(lock); return clean ? 0 : 1; } -- 2.13.0
[PATCH v3 2/4] stash: add test for stashing in a detached state
Signed-off-by: Joel Teichroeb--- t/t3903-stash.sh | 11 +++ 1 file changed, 11 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index aaae221304..b86851ef46 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -808,6 +808,17 @@ test_expect_success 'create with multiple arguments for the message' ' test_cmp expect actual ' +test_expect_success 'create in a detached state' ' + test_when_finished "git checkout master" && + git checkout HEAD~1 && + >foo && + git add foo && + STASH_ID=$(git stash create) && + echo "WIP on (no branch): 47d5e0e initial" >expect && + git show --pretty=%s -s ${STASH_ID} >actual && + test_cmp expect actual +' + test_expect_success 'stash -- stashes and restores the file' ' >foo && >bar && -- 2.13.0
[PATCH v3 4/4] stash: implement builtin stash
Implement all git stash functionality as a builtin command Signed-off-by: Joel Teichroeb--- Makefile |2 +- builtin.h |1 + builtin/stash.c | 1280 + git-stash.sh => contrib/examples/git-stash.sh |0 git.c |1 + 5 files changed, 1283 insertions(+), 1 deletion(-) create mode 100644 builtin/stash.c rename git-stash.sh => contrib/examples/git-stash.sh (100%) diff --git a/Makefile b/Makefile index e35542e631..4af4ac41c7 100644 --- a/Makefile +++ b/Makefile @@ -523,7 +523,6 @@ SCRIPT_SH += git-quiltimport.sh SCRIPT_SH += git-rebase.sh SCRIPT_SH += git-remote-testgit.sh SCRIPT_SH += git-request-pull.sh -SCRIPT_SH += git-stash.sh SCRIPT_SH += git-submodule.sh SCRIPT_SH += git-web--browse.sh @@ -961,6 +960,7 @@ BUILTIN_OBJS += builtin/send-pack.o BUILTIN_OBJS += builtin/shortlog.o BUILTIN_OBJS += builtin/show-branch.o BUILTIN_OBJS += builtin/show-ref.o +BUILTIN_OBJS += builtin/stash.o BUILTIN_OBJS += builtin/stripspace.o BUILTIN_OBJS += builtin/submodule--helper.o BUILTIN_OBJS += builtin/symbolic-ref.o diff --git a/builtin.h b/builtin.h index 9e4a89816d..16e8a437d4 100644 --- a/builtin.h +++ b/builtin.h @@ -121,6 +121,7 @@ extern int cmd_shortlog(int argc, const char **argv, const char *prefix); extern int cmd_show(int argc, const char **argv, const char *prefix); extern int cmd_show_branch(int argc, const char **argv, const char *prefix); extern int cmd_status(int argc, const char **argv, const char *prefix); +extern int cmd_stash(int argc, const char **argv, const char *prefix); extern int cmd_stripspace(int argc, const char **argv, const char *prefix); extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix); extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix); diff --git a/builtin/stash.c b/builtin/stash.c new file mode 100644 index 00..bf36ff8f9b --- /dev/null +++ b/builtin/stash.c @@ -0,0 +1,1280 @@ +#include "builtin.h" +#include "parse-options.h" +#include "refs.h" +#include "tree.h" +#include "lockfile.h" +#include "object.h" +#include "tree-walk.h" +#include "cache-tree.h" +#include "unpack-trees.h" +#include "diff.h" +#include "revision.h" +#include "commit.h" +#include "diffcore.h" +#include "merge-recursive.h" +#include "argv-array.h" +#include "run-command.h" + +static const char * const git_stash_usage[] = { + N_("git stash list []"), + N_("git stash show []"), + N_("git stash drop [-q|--quiet] []"), + N_("git stash ( pop | apply ) [--index] [-q|--quiet] []"), + N_("git stash branch []"), + N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"), + N_("[-u|--include-untracked] [-a|--all] []]"), + N_("git stash clear"), + N_("git stash create []"), + N_("git stash store [-m|--message ] [-q|--quiet] "), + NULL +}; + +static const char * const git_stash_list_usage[] = { + N_("git stash list []"), + NULL +}; + +static const char * const git_stash_show_usage[] = { + N_("git stash show []"), + NULL +}; + +static const char * const git_stash_drop_usage[] = { + N_("git stash drop [-q|--quiet] []"), + NULL +}; + +static const char * const git_stash_pop_usage[] = { + N_("git stash pop [--index] [-q|--quiet] []"), + NULL +}; + +static const char * const git_stash_apply_usage[] = { + N_("git stash apply [--index] [-q|--quiet] []"), + NULL +}; + +static const char * const git_stash_branch_usage[] = { + N_("git stash branch []"), + NULL +}; + +static const char * const git_stash_save_usage[] = { + N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"), + N_("[-u|--include-untracked] [-a|--all] []]"), + NULL +}; + +static const char * const git_stash_clear_usage[] = { + N_("git stash clear"), + NULL +}; + +static const char * const git_stash_create_usage[] = { + N_("git stash create []"), + NULL +}; + +static const char * const git_stash_store_usage[] = { + N_("git stash store [-m|--message ] [-q|--quiet] "), + NULL +}; + +static const char *ref_stash = "refs/stash"; +static int quiet = 0; +static struct lock_file lock_file; +static char stash_index_path[64]; + +struct stash_info { + struct object_id w_commit; + struct object_id b_commit; + struct object_id i_commit; + struct object_id u_commit; + struct object_id w_tree; + struct object_id b_tree; + struct object_id i_tree; + struct object_id u_tree; + const char *message; + const char *REV; + int is_stash_ref; + int has_u; + const char *patch; +}; + +int untracked_files(struct strbuf *out, int include_untracked, + const char **argv) +{ +
[PATCH v3 1/4] stash: add test for stash create with no files
Ensure the command gives the correct return code Signed-off-by: Joel Teichroeb--- t/t3903-stash.sh | 8 1 file changed, 8 insertions(+) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 3b4bed5c9a..aaae221304 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -444,6 +444,14 @@ test_expect_failure 'stash file to directory' ' test foo = "$(cat file/file)" ' +test_expect_success 'stash create - no changes' ' + git stash clear && + test_when_finished "git reset --hard HEAD" && + git reset --hard && + git stash create > actual && + test $(cat actual | wc -l) -eq 0 +' + test_expect_success 'stash branch - no stashes on stack, stash-like argument' ' git stash clear && test_when_finished "git reset --hard HEAD" && -- 2.13.0
[PATCH v3 0/4] Implement git stash as a builtin command
I've rewritten git stash as a builtin c command. All tests pass, and I've added two new tests. Test coverage is around 95% with the only things missing coverage being error handlers. Joel Teichroeb (4): stash: add test for stash create with no files stash: add test for stashing in a detached state close the index lock when not writing the new index stash: implement builtin stash Makefile |2 +- builtin.h |1 + builtin/add.c |3 +- builtin/mv.c |8 +- builtin/rm.c |3 +- builtin/stash.c | 1280 + git-stash.sh => contrib/examples/git-stash.sh |0 git.c |1 + merge-recursive.c |8 +- t/t3903-stash.sh | 19 + 10 files changed, 1316 insertions(+), 9 deletions(-) create mode 100644 builtin/stash.c rename git-stash.sh => contrib/examples/git-stash.sh (100%) -- 2.13.0
[PATCH] doc: Improve description for rev-parse --short
First: `git rev-parse --short` without a number does use a fixed default but `core.abbrev` which in turn uses `find_unique_abbrev` internally. Second: `--short` implies `--verify` since the beginning (d50125085a), so it cannot be used for bulk-shortening ids unfortunately. Signed-off-by: Andreas Heiduk--- Documentation/config.txt| 1 + Documentation/git-rev-parse.txt | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index e0b9fd0bc..158cb588b 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -862,6 +862,7 @@ core.abbrev:: computed based on the approximate number of packed objects in your repository, which hopefully is enough for abbreviated object names to stay unique for some time. + The minimum length is 4. add.ignoreErrors:: add.ignore-errors (deprecated):: diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt index c40c47044..7a7421c8e 100644 --- a/Documentation/git-rev-parse.txt +++ b/Documentation/git-rev-parse.txt @@ -140,7 +140,9 @@ can be used. --short=number:: Instead of outputting the full SHA-1 values of object names try to abbreviate them to a shorter unique name. When no length is specified - 7 is used. The minimum length is 4. + the effective value of the configuration variable `core.abbrev` (see + linkgit:git-config[1]) is used. The minimum length is 4. The length + may be exceeded to ensure unique object names. Implies `--verify`. --symbolic:: Usually the object names are output in SHA-1 form (with -- 2.13.0
Re: Missing: Consistency of clean state output of "git add -i"
On Sun, 2017-05-28 at 11:39 +0100, Philip Oakley wrote: > I wouldn't let that stop you. We were all ignorant once. > I know little of tcl (gitk/git-gui), but I've still managed to fix a > couple > of issues, with the help of others on the list (and the search > engines and > their results;-) > Thanks for the motivative reply. > Perl is *that* hard, is it? I don't have any idea about Perl. I'll see if I could do something when I find time. That said, I wouldn't like to prevent anyone who like to take up this task. If anyone's interested, please take it up. -- Regards, Kaartic
Re: mergetool: what to do about deleting precious files?
"Philip Oakley"writes: >> So I do not think this is not limited to "new file". Anything that >> a tree-level three-way merge would resolve cleanly without having to >> consult the content-level three-way merge will complete without >> consulting the merge.ours.driver; per-file content-level three-way >> merge driver (which is what merge= mechanism lets you >> specify via the attributes mechanism) is not something you would >> want to use for this kind of thing. It is purely for resolving the >> actual content-level conflicts. >> > That (that Git knows best) sounds just wrong. Don't twist my words. I never said Git knows best. The user-level merge driver is a mechanism to affect conflict level three-way merges. The interface to the content level three-way merge driver feeds three versions of blobs and the driver is expected to give a merged result. The interface as designed is incapable of passing "here is the common ancestor", "our side is missing" and "their side is this content". So if we want a mechanism that can affect the outcome of tree-level three-way merge, we need a _new_ mechanism. The existing merge drivers that are written by end users (at least the ones written correctly to the spec, anyway) are not expecting to be called with "in our tree, there is no blob here", and trying to piggyback on it will break existing users.
Re: [Non-Bug] cloning a repository with a default MASTER branch tries to check out the master branch
"Philip Oakley"writes: > From: "Junio C Hamano" >> "Philip Oakley" writes: >> >>> However given the discussion about an unborn HEAD, the option here >>> would be to also pass the NULL sha for the symref and then add the >>> annotation 'HEAD' after an extra \0, in the same way that an active >>> symref could be annotated with the '\0HEAD'. This would kill two birds >>> with one stone! >> >> Are you aware of the symref capability that is already advertised in >> the initial upload-pack response? Right now, we do so only when >> HEAD actually points at something, and the earlier suggestion by >> Peff is to do so unconditionally, even when HEAD is dangling. > > The suggestion is the otherway around. I would argue (as a viewpoint) > that what we advertise are object IDs and their associated refs, > sorted by ref name. (I'm thinking of the > git/Documentation/technical/pack-protocol.txt here). My suggestion was That's not the part of the protocol I explained Peff's suggestion to you about. That's ref advertisement proper, and its first line has a trailing NUL followed by "protocol capability" list. There is one "capability" that tells the receiver specifically about HEAD symref (if and only if HEAD is a symref). There are two reasons why the current code does not help even though that necessary protocol bits are *already* there (i.e. you do not need any protocol extension). One is that existing servers do not use the symref capability for HEAD if HEAD is pointing at an unborn branch (i.e. dangling). The other is that the existing code sitting on the receiving end is not prepared to handle one, even if the server end sent one.
[no subject]
Hello, $850,000 has been donate to you, kindly get back to us via: maureendav...@outlook.com ü Mielőtt kinyomtatja ezt az e-mailt, gondoljon a környezetre. P Please consider the environment before printing this email. *** Ezt az emailt a Websense ESG ellenőrizte a BKV Zrt. biztonsági szabályzata alapján. Nem található benne vírus.
[PATCH] completion: Add completions for git config commit
Add missing completions for git config: * commit.cleanup * commit.gpgSign * commit.verbose Signed-off-by: Rikard Falkeborn--- contrib/completion/git-completion.bash | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index 1ed0a09fe..90dfd7681 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -2395,8 +2395,11 @@ _git_config () color.status.untracked color.status.updated color.ui + commit.cleanup + commit.gpgSign commit.status commit.template + commit.verbose core.abbrev core.askpass core.attributesfile -- 2.13.0
Re: git-2.13.0: log --date=format:%z not working
Am 27.05.2017 um 23:46 schrieb Jeff King: > On Sat, May 27, 2017 at 06:57:08PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> There's another test which breaks if we just s/gmtime/localtime/g. As >> far as I can tell to make the non-local case work we'd need to do a >> whole dance where we set the TZ variable to e.g. UTC$offset, then call >> strftime(), then call it again. Maybe there's some way to just specify >> the tz offset, but I didn't find any in a quick skimming of time.h. > > There isn't. Right. We could handle %z internally, though. %Z would be harder (left as an exercise for readers..). First we'd have to undo 0a0416a3 (strbuf_expand: convert "%%" to "%"), though, in order to give full control back to strbuf_expand callbacks. 2-pack patch: --- >8 --- builtin/cat-file.c | 5 + convert.c | 1 + daemon.c | 3 +++ date.c | 2 +- ll-merge.c | 5 +++-- pretty.c | 3 +++ strbuf.c | 39 ++- strbuf.h | 11 +-- t/t6006-rev-list-format.sh | 12 9 files changed, 63 insertions(+), 18 deletions(-) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 1890d7a639..9cf2e4291d 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -252,6 +252,11 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *data) { const char *end; + if (*start == '%') { + strbuf_addch(sb, '%'); + return 1; + } + if (*start != '(') return 0; end = strchr(start + 1, ')'); diff --git a/convert.c b/convert.c index 8d652bf27c..8336fff694 100644 --- a/convert.c +++ b/convert.c @@ -399,6 +399,7 @@ static int filter_buffer_or_fd(int in, int out, void *data) struct strbuf path = STRBUF_INIT; struct strbuf_expand_dict_entry dict[] = { { "f", NULL, }, + { "%", "%" }, { NULL, NULL, }, }; diff --git a/daemon.c b/daemon.c index ac7181a483..191fac2716 100644 --- a/daemon.c +++ b/daemon.c @@ -148,6 +148,9 @@ static size_t expand_path(struct strbuf *sb, const char *placeholder, void *ctx) case 'D': strbuf_addstr(sb, context->directory); return 1; + case '%': + strbuf_addch(sb, '%'); + return 1; } return 0; } diff --git a/date.c b/date.c index 63fa99685e..d16a88eea5 100644 --- a/date.c +++ b/date.c @@ -246,7 +246,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode) month_names[tm->tm_mon], tm->tm_year + 1900, tm->tm_hour, tm->tm_min, tm->tm_sec, tz); else if (mode->type == DATE_STRFTIME) - strbuf_addftime(, mode->strftime_fmt, tm); + strbuf_addftime(, mode->strftime_fmt, tm, tz); else strbuf_addf(, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d", weekday_names[tm->tm_wday], diff --git a/ll-merge.c b/ll-merge.c index ac0d4a5d78..e6202c7397 100644 --- a/ll-merge.c +++ b/ll-merge.c @@ -172,7 +172,7 @@ static int ll_ext_merge(const struct ll_merge_driver *fn, { char temp[4][50]; struct strbuf cmd = STRBUF_INIT; - struct strbuf_expand_dict_entry dict[6]; + struct strbuf_expand_dict_entry dict[7]; struct strbuf path_sq = STRBUF_INIT; const char *args[] = { NULL, NULL }; int status, fd, i; @@ -185,7 +185,8 @@ static int ll_ext_merge(const struct ll_merge_driver *fn, dict[2].placeholder = "B"; dict[2].value = temp[2]; dict[3].placeholder = "L"; dict[3].value = temp[3]; dict[4].placeholder = "P"; dict[4].value = path_sq.buf; - dict[5].placeholder = NULL; dict[5].value = NULL; + dict[5].placeholder = "%"; dict[5].value = "%"; + dict[6].placeholder = NULL; dict[6].value = NULL; if (fn->cmdline == NULL) die("custom merge driver %s lacks command line.", fn->name); diff --git a/pretty.c b/pretty.c index 587d48371b..56872bfa25 100644 --- a/pretty.c +++ b/pretty.c @@ -1428,6 +1428,9 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */ } magic = NO_MAGIC; switch (placeholder[0]) { + case '%': + strbuf_addch(sb, '%'); + return 1; case '-': magic = DEL_LF_BEFORE_EMPTY; break; diff --git a/strbuf.c b/strbuf.c index 00457940cf..206dff6037 100644 --- a/strbuf.c +++ b/strbuf.c @@ -309,12 +309,6 @@ void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, break; format = percent + 1; - if (*format == '%') { - strbuf_addch(sb, '%'); - format++; - continue; - } -
Re: [Non-Bug] cloning a repository with a default MASTER branch tries to check out the master branch
From: "Junio C Hamano""Philip Oakley" writes: However given the discussion about an unborn HEAD, the option here would be to also pass the NULL sha for the symref and then add the annotation 'HEAD' after an extra \0, in the same way that an active symref could be annotated with the '\0HEAD'. This would kill two birds with one stone! Are you aware of the symref capability that is already advertised in the initial upload-pack response? Right now, we do so only when HEAD actually points at something, and the earlier suggestion by Peff is to do so unconditionally, even when HEAD is dangling. The suggestion is the otherway around. I would argue (as a viewpoint) that what we advertise are object IDs and their associated refs, sorted by ref name. (I'm thinking of the git/Documentation/technical/pack-protocol.txt here). My suggestion was that when we get to the sorted ref that HEAD points to (including the unborn oid) that we annotate that ref. I didn't quite follow Peff's suggestion as to where the list change went and if that was a protocol change. There are two current fault scenarios. a. The currently reported case where HEAD has an unborn symref b. The multiple ref HEAD case, where the HEAD oid matches multiple advertised refs, and the correct symref is not the first listed (which is the case I had looked at a few years ago, a prompted me to respond). With the above discussions, we would have both a NULL oid for the unborn (sym)ref sent (if needed), and the (sym)ref for HEAD would have the extra annotation. That would at least not break the protocol rule that "If HEAD is not a valid ref, HEAD MUST NOT appear in the advertisement list at all" (it is now an annotation to another valid ref [or the unborn symref]). Existing clients that are symref aware do not do anything (good or bad) when a HEAD that is dangling [*1*] is advertised, so such a change will not hurt (but it would not help by itself either). Ancient clients that are not even aware of the symref are not affected. Then new clients _could_ start paying attention to the advertised HEAD that is dangling. My main step was that for case b, so that we don't need to guess_remote_head(). The annotation would have already told us. The bundle transport is a different beast. I do not think it advertises where HEAD is pointing at, whether it is dangling or not. My understanding was that the Bundle was a tiny wrapper and that it has that same protocol header, which was then decoded using the same code. Hence the hope that it could fix both the bundle and remote clone problems. I'd just avoided stepping into remote clone arena because of other implementations. [Footnote] *1* A HEAD symref that is advertised in the upload-pack response is dangling when its pointee does not appear in the set of refs that are advertised. Félix's case would have shown HEAD pointing at refs/heads/master in the symref capability extension, but the list of refs and their values would not have included that ref (there was only refs/heads/MASTER "for joke reasons"). I hope I haven't confused the different parts of the negotiation and transfer, which can be confusing. Philip
Re: Missing: Consistency of clean state output of "git add -i"
From: "Kaartic Sivaraam"I guess I'll take back my note in the previous email that says, I could help. I saw the implementation and found that I couldn't help as I don't have experience with PERL. My bad. I wouldn't let that stop you. We were all ignorant once. I know little of tcl (gitk/git-gui), but I've still managed to fix a couple of issues, with the help of others on the list (and the search engines and their results;-) Perl is *that* hard, is it? regards Philip
Re: git-2.13.0: log --date=format:%z not working
On Sat, May 27, 2017 at 11:46 PM, Jeff Kingwrote: > On Sat, May 27, 2017 at 06:57:08PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> There's another test which breaks if we just s/gmtime/localtime/g. As >> far as I can tell to make the non-local case work we'd need to do a >> whole dance where we set the TZ variable to e.g. UTC$offset, then call >> strftime(), then call it again. Maybe there's some way to just specify >> the tz offset, but I didn't find any in a quick skimming of time.h. > > There isn't. At least on _some_ platforms, the zone information is > embedded in "struct tm" and stored by gmtime() and localtime(), but the > fields aren't publicly accessible. Which is why your patch worked for > format-local (it swaps out gmtime() for localtime() which sets those > fields behind the scenes). But: > > - I'm not sure that's guaranteed by the standard; strftime() might get > its zone information elsewhere (if it needs to reliably distinguish > between gmtime() and localtime() results it has to at least set a > bit in the "struct tm", but that bit may not be the full zone info). > > - Even if it does work, you're stuck with only the local timezone. In > theory you could temporarily tweak the process's timezone, call > localtime(), and then tweak it back. I was never able to get that to > work (links below). > > On glibc, at least, you can access the zone fields in "struct tm" by > compiling with _DEFAULT_SOURCE. > > So I think the best we could do is probably to have a feature macro like > TM_HAS_GMTOFF, and set tm->tm_gmtoff and tm->tm_zone on platforms that > support it. I'm not sure what we'd put in the latter, though; we don't > actually have the timezone name at all (we just have "+0200" or whatever > we parsed from the git object, but that would be better than nothing). > > That leaves other platforms still broken, but like I said, I don't think > there's a portable solution. > > Here are some links to past explorations: > > http://public-inbox.org/git/20160208152858.ga17...@sigill.intra.peff.net/ > > http://public-inbox.org/git/87vb2d37ea@web.de/ There's a third and possibly least shitty option that isn't covered in those threads; We could just make a pass over the strftime format ourselves and replace %z and %Z with the timezone (as done for DATE_ISO8601_STRICT in date.c), then hand the rest off to strftime(). I'm not going to pursue it though, Ulrich, are you maybe interested in hacking on that?
Re: mergetool: what to do about deleting precious files?
From: "Junio C Hamano""Philip Oakley" writes: The git book [1] and a few blog posts [2] show how to preserve files which are in the current branch against changes that are on the branch being merged in. e.g. (from [2]) echo ' merge=ours' >> .gitattributes && # commit git config --global merge.ours.driver true (test) $ git checkout demo (demo) $ git merge - # contents are not merged and the original retained. However what is not covered (at least in the documentation ) is the case where the file to be ignored is not present on the current branch, but is present on the branch to be merged in. Hmph. Per-path 'ours' and 'theirs' kick in only after we decide to perform the content level three-way merge. I wonder what would (not "should", but "would with the current code") happen, with the same attribute setting, if the file being merged were not changed by ours but modified by the side branch? I suspect that we'd take the change made by the side branch. Here the 'ours' strategy is defined by the user's config file merge driver list. I'd understood it that once it was decided there was a merge to be performed (the repective blob oid's in the repo/index are different) that the problem of merging is then handed off to the declared merge driver. Normal expectations would be that in such a case the new file from the second parent branch would be added to the current branch. The git-scm and blog posts suggest that the original is left in place at the %P path, the merge driver run, and its return values used to decide if the user has to go and resolve conflicts. By setting the driver to 'true', the result is then said to be that the current 'blob' (i.e. file) is accepted unchanged (in %P), so anything from the second parent blob was completely ignored. However if we have the addition of a new file, I can't tell from the docs what should happen? Is this still a merge such that the merge driver is called, or is the added file accepted without recourse to its .gitattributes setting (surely that would be a bug). Then assuming we have reached an external driver, and it wants to not add that very file that was added in the second parent branch, what does the %P path point to (/dev/null?) - in particular, shouldn't the docs say? (I've not tested, and one test is not proof) It maybe that the user wants a merge driver that says "If I ever see a secret key or password, then remove the whole file", which (removing the file from the merge) is a currently undocumented process (if even possible). So I do not think this is not limited to "new file". Anything that a tree-level three-way merge would resolve cleanly without having to consult the content-level three-way merge will complete without consulting the merge.ours.driver; per-file content-level three-way merge driver (which is what merge= mechanism lets you specify via the attributes mechanism) is not something you would want to use for this kind of thing. It is purely for resolving the actual content-level conflicts. That (that Git knows best) sounds just wrong. If the user has set a file attribute strategy, why would we ignore it? We already have different internal strategies anyway, so how do we even know that the potential merge was conflict free if we have haven't checked its attribute type. Maybe I'm missing something. -- Philip
Re: Missing: Consistency of clean state output of "git add -i"
I guess I'll take back my note in the previous email that says, I could help. I saw the implementation and found that I couldn't help as I don't have experience with PERL. My bad.
Missing: Consistency of clean state output of "git add -i"
Hello all, When the "git add -i" command is triggered with a clean working directory and index, the outputs of the various options don't seem to be giving consistent results. A few of the distinct outputs are, 1. No output, the options are displayed 2. A single blank line and the options are displayed 3. The "add untracked" option, prints "No untracked files." and adds a empty line and prints the options 4. The "patch" option, prints "No changes" and prints the options It seems that the clean state output should be improved for the sake of a consistent user interface. Note: I could possibly help if I were pointed to the implementation. Quote: "We hate most in others that which we fail to see in ourselves." - Anil Dash -- Regards, Kaartic