threads / patch / 17988

patch, 23 partsbuiltin-remote improvments

Subject: [PATCH 00/23] builtin-remote improvments

## tl;dr

6 messages between Feb 24, 2009 and Feb 24, 2009. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Jay Soffian· Feb 24, 2009, 09:50 UTC · lore
Junio, this is a big series, so let me explain.

Patches 1-5 are js/remote-set-head, but rebased on master (2d602e9). I did this because patch 18 in this series depends on commit cfa1ee6 which is now in master, but was not when you originally spawned js/remote-set-head. I also corrected two failing tests in #3.

Patches 6-18 are the 13 patch series I sent out earlier, directly on-top of 1-5, instead of on pu. I incorporated the feedback I received, and re-ordered the patches to put all the refactoring, as well as non-builtin-remote changes, first among the 13.

Patches 19-23 are jk/head-lookup, now based at the end of my changes. Previously they were effectively between 1-5 and 6-18.

You might consider applying #6 (make match_refs() copy src...) independently to master if it is obvious enough, but in any case it has to come before #18 in this series.

I wasn't sure what to do about the SOB on the rebased patches, so I kept Jeff's, removed yours, and added my own. Please correct me if this was the wrong thing to do.

Finally, I've and compiled and run t5505-remote.sh after each individual commit, as well as the entire default test suite (but w/NO_SVN_TESTS=YesPlease) after #10 and #23.

Thank you for your patience with this topic.
j.
Jay Soffian (18):
  builtin-clone: move locate_head() to remote.c so it can be re-used
  builtin-remote: move duplicated cleanup code its own function
  builtin-remote: teach show to display remote HEAD
  builtin-remote: add set-head subcommand
  builtin-remote: better handling of multiple remote HEADs
  remote.c: make match_refs() copy src ref before assigning to peer_ref
  remote.c: don't short-circuit match_refs() when error in match_explicit_refs()
  refactor duplicated get_local_heads() to remote.c
  refactor duplicated ref_newer() to remote.c
  string-list: new for_each_string_list() function
  builtin-remote: rename variable and eliminate redundant function call
  builtin-remote: name remote_refs consistently
  builtin-remote: remove unused code in get_ref_states
  builtin-remote: fix two inconsistencies in the output of "show <remote>"
  builtin-remote: make get_remote_ref_states() always populate states.tracked
  builtin-remote: refactor get_remote_ref_states()
  builtin-remote: new show output style
  builtin-remote: new show output style for push refspecs
Jeff King (5):
  test scripts: refactor start_httpd helper
  add basic http clone/fetch tests
  refactor find_refs_by_name to accept const list
  remote: refactor guess_remote_head
  remote: use exact HEAD lookup if it is available
 Documentation/git-remote.txt           |   28 ++-
 Makefile                               |    1 +
 builtin-clone.c                        |   42 +---
 builtin-remote.c                       |  562 ++++++++++++++++++++++++++------
 builtin-send-pack.c                    |   79 +-----
 cache.h                                |    2 +-
 contrib/completion/git-completion.bash |    2 +-
 http-push.c                            |   72 +----
 refs.c                                 |    4 +-
 remote.c                               |  137 ++++++++-
 remote.h                               |   12 +
 string-list.c                          |   10 +
 string-list.h                          |    5 +
 t/lib-httpd.sh                         |    9 +-
 t/t5505-remote.sh                      |  122 +++++--
 t/t5540-http-push.sh                   |    9 +-
 t/t5550-http-fetch.sh                  |   57 ++++
 17 files changed, 817 insertions(+), 336 deletions(-)
 create mode 100755 t/t5550-http-fetch.sh
Jay Soffian· Feb 24, 2009, 10:53 UTC · re: Jay Soffian · lore

Re: [PATCH 00/23] builtin-remote improvments

On Tue, Feb 24, 2009 at 4:50 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
Show 27 quoted lines
> Jay Soffian (18):
>  builtin-clone: move locate_head() to remote.c so it can be re-used
>  builtin-remote: move duplicated cleanup code its own function
>  builtin-remote: teach show to display remote HEAD
>  builtin-remote: add set-head subcommand
>  builtin-remote: better handling of multiple remote HEADs
>
>  remote.c: make match_refs() copy src ref before assigning to peer_ref
>  remote.c: don't short-circuit match_refs() when error in match_explicit_refs()
>  refactor duplicated get_local_heads() to remote.c
>  refactor duplicated ref_newer() to remote.c
>  string-list: new for_each_string_list() function
>  builtin-remote: rename variable and eliminate redundant function call
>  builtin-remote: name remote_refs consistently
>  builtin-remote: remove unused code in get_ref_states
>  builtin-remote: fix two inconsistencies in the output of "show <remote>"
>  builtin-remote: make get_remote_ref_states() always populate states.tracked
>  builtin-remote: refactor get_remote_ref_states()
>  builtin-remote: new show output style
>  builtin-remote: new show output style for push refspecs
>
> Jeff King (5):
>  test scripts: refactor start_httpd helper
>  add basic http clone/fetch tests
>  refactor find_refs_by_name to accept const list
>  remote: refactor guess_remote_head
>  remote: use exact HEAD lookup if it is available
Argh. Looking at this again, it seems clear to me now that I should've put Jeff's series at the beginning, followed by my refactoring (6-10), followed by the rest of my patches, so that all the refactoring is early.
And I forgot that I'd done patch #5 (better handling of multiple remote HEADs) a few days after 1-4. As part of this re-roll I should've redone 1-4 to include #5's functionality.
I also noticed I'm passing an argument to get_head_names() that it never uses. But fixing that touches at least 3 of the patches.
All of this is my way of saying ignore what I've emailed tonight till I can clean it up a bit more.
I apologize to the list for the spam.
When I have the series really ready, would you prefer I setup a repo for you to pull from? I'm loath to spam the list with another 24 messages.
j.
Marius Storm-Olsen· Feb 24, 2009, 11:01 UTC · re: Jay Soffian · lore

Re: [PATCH 00/23] builtin-remote improvments

Show 8 quoted lines
> All of this is my way of saying ignore what I've emailed tonight
> till I can clean it up a bit more.
> 
> I apologize to the list for the spam.
> 
> When I have the series really ready, would you prefer I setup a
> repo for you to pull from? I'm loath to spam the list with another
> 24 messages.

It would at least be great if you could include --thread in your options to git format-patch, so your patches are threaded underneath your cover letter ;-)

-- 
.marius [@trolltech.com]
'if you know what you're doing, it's not research'
Jay Soffian· Feb 24, 2009, 17:36 UTC · re: Marius Storm-Olsen · lore

Re: [PATCH 00/23] builtin-remote improvments

On Tue, Feb 24, 2009 at 6:01 AM, Marius Storm-Olsen <marius@trolltech.com> wrote:

Show 12 quoted lines
>> All of this is my way of saying ignore what I've emailed tonight
>> till I can clean it up a bit more.
>>
>> I apologize to the list for the spam.
>>
>> When I have the series really ready, would you prefer I setup a
>> repo for you to pull from? I'm loath to spam the list with another
>> 24 messages.
>
> It would at least be great if you could include --thread in your options to
> git format-patch, so your patches are threaded underneath your cover letter
> ;-)

I did, but I had run format-patch twice because I changed a patch. Unfortunately I kept the cover-letter from the first run but forgot to update its message-id. So all the patches reply to a non-existent message.

:-(
j.
Junio C Hamano· Feb 24, 2009, 17:39 UTC · re: Jay Soffian · lore

Re: [PATCH 00/23] builtin-remote improvments

Jay Soffian <jaysoffian@gmail.com> writes:
> When I have the series really ready, would you prefer I setup a repo for
> you to pull from? I'm loath to spam the list with another 24 messages.

I don't mind pulling, as it is less work for me, but it would make harder for others to see the changes in flight and discuss, so...

Even though I'd need to re-read them before merging them to 'next', I also think the earlier remote-set-head series and Jeff's changes on top of it looked more or less reasonable. Perhaps I should rebase them on top of 'master' before moving them to 'next' and then you can build on top, which would reduce your new series by about 10 commits.

Jay Soffian· Feb 24, 2009, 17:49 UTC · re: Junio C Hamano · lore

Re: [PATCH 00/23] builtin-remote improvments

On Tue, Feb 24, 2009 at 12:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> I don't mind pulling, as it is less work for me, but it would make
> harder for others to see the changes in flight and discuss, so...
>
> Even though I'd need to re-read them before merging them to 'next', I also
> think the earlier remote-set-head series and Jeff's changes on top of it
> looked more or less reasonable.  Perhaps I should rebase them on top of
> 'master' before moving them to 'next' and then you can build on top, which
> would reduce your new series by about 10 commits.

Putting Jeff's changes first reduces churn a small amount, as we don't introduce new code just to change it a few commits later in the same topic. So it keeps the history marginally cleaner. That was my thinking anyway. Otherwise you may as well just use what I sent last night and let me add one more patch on-top to clean-up that unused variable that gets passed to get_head_names().

Your call. :-)
j.

← back to recent threads