New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 885315 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner: ----
Closed: Sep 18
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 785137



Sign in to add a comment

shill: git blame not working as expected after directory move

Project Member Reported by akhouderchah@chromium.org, Sep 18

Issue description

After moving shill from aosp/ back to platform2/, running git blame on shill source files leads most lines to reference the CL 'shill: move back from AOSP' rather than the original CL.

Sample run on eap_listener.cc:
dc2d795176 (Peter Qiu      2015-09-03 11:25:46 -0700   1) //
dc2d795176 (Peter Qiu      2015-09-03 11:25:46 -0700   2) // Copyright (C) 2013 The Android Open Source Project
dc2d795176 (Peter Qiu      2015-09-03 11:25:46 -0700   3) //
dc2d795176 (Peter Qiu      2015-09-03 11:25:46 -0700   4) // Licensed under the Apache License, Version 2.0 (the "License");
dc2d795176 (Peter Qiu      2015-09-03 11:25:46 -0700   5) // you may not use this file except in compliance with the License.
dc2d795176 (Peter Qiu      2015-09-03 11:25:46 -0700   6) // You may obtain a copy of the License at
dc2d795176 (Peter Qiu      2015-09-03 11:25:46 -0700   7) //
dc2d795176 (Peter Qiu      2015-09-03 11:25:46 -0700   8) //      http://www.apache.org/licenses/LICENSE-2.0
dc2d795176 (Peter Qiu      2015-09-03 11:25:46 -0700   9) //
dc2d795176 (Peter Qiu      2015-09-03 11:25:46 -0700  10) // Unless required by applicable law or agreed to in writing, software
dc2d795176 (Peter Qiu      2015-09-03 11:25:46 -0700  11) // distributed under the License is distributed on an "AS IS" BASIS,
dc2d795176 (Peter Qiu      2015-09-03 11:25:46 -0700  12) // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
dc2d795176 (Peter Qiu      2015-09-03 11:25:46 -0700  13) // See the License for the specific language governing permissions and
dc2d795176 (Peter Qiu      2015-09-03 11:25:46 -0700  14) // limitations under the License.
dc2d795176 (Peter Qiu      2015-09-03 11:25:46 -0700  15) //
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  16) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  17) #include "shill/eap_listener.h"
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  18) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  19) #include <linux/if_ether.h>
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  20) #include <linux/if_packet.h>
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  21) #include <netinet/in.h>
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  22) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  23) #include <base/bind.h>
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  24) #include <base/compiler_specific.h>
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  25) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  26) #include "shill/eap_protocol.h"
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  27) #include "shill/event_dispatcher.h"
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  28) #include "shill/logging.h"
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  29) #include "shill/net/sockets.h"
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  30) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  31) namespace shill {
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  32) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  33) const size_t EapListener::kMaxEapPacketLength =
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  34)     sizeof(eap_protocol::Ieee8021xHdr) + sizeof(eap_protocol::EapHeader);
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  35) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  36) EapListener::EapListener(EventDispatcher* event_dispatcher,
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  37)                          int interface_index)
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  38)     : dispatcher_(event_dispatcher),
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  39)       interface_index_(interface_index),
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  40)       sockets_(new Sockets()),
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  41)       socket_(-1) {}
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  42) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  43) EapListener::~EapListener() {}
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  44) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  45) bool EapListener::Start() {
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  46)   if (!CreateSocket()) {
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  47)     LOG(ERROR) << "Could not open EAP listener socket.";
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  48)     Stop();
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  49)     return false;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  50)   }
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  51) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  52)   receive_request_handler_.reset(
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  53)     dispatcher_->CreateReadyHandler(
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  54)         socket_,
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  55)         IOHandler::kModeInput,
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  56)         base::Bind(&EapListener::ReceiveRequest, base::Unretained(this))));
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  57) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  58)   return true;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  59) }
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  60) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  61) void EapListener::Stop() {
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  62)   receive_request_handler_.reset();
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  63)   socket_closer_.reset();
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  64)   socket_ = -1;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  65) }
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  66) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  67) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  68) bool EapListener::CreateSocket() {
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  69)   int socket = sockets_->Socket(PF_PACKET, SOCK_DGRAM, htons(ETH_P_PAE));
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  70)   if (socket == -1) {
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  71)     PLOG(ERROR) << "Could not create EAP listener socket";
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  72)     return false;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  73)   }
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  74)   socket_ = socket;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  75)   socket_closer_.reset(new ScopedSocketCloser(sockets_.get(), socket_));
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  76) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  77)   if (sockets_->SetNonBlocking(socket_) != 0) {
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  78)     PLOG(ERROR) << "Could not set socket to be non-blocking";
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  79)     return false;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  80)   }
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  81) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  82)   sockaddr_ll socket_address;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  83)   memset(&socket_address, 0, sizeof(socket_address));
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  84)   socket_address.sll_family = AF_PACKET;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  85)   socket_address.sll_protocol = htons(ETH_P_PAE);
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  86)   socket_address.sll_ifindex = interface_index_;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  87) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  88)   if (sockets_->Bind(socket_,
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  89)                      reinterpret_cast<struct sockaddr*>(&socket_address),
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  90)                      sizeof(socket_address)) != 0) {
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  91)     PLOG(ERROR) << "Could not bind socket to interface";
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  92)     return false;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  93)   }
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  94) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  95)   return true;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  96) }
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  97) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  98) void EapListener::ReceiveRequest(int fd) {
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400  99)   struct ALIGNAS(1) {
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 100)     eap_protocol::Ieee8021xHdr onex_header;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 101)     eap_protocol::EapHeader eap_header;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 102)   } payload;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 103)   sockaddr_ll remote_address;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 104)   memset(&remote_address, 0, sizeof(remote_address));
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 105)   socklen_t socklen = sizeof(remote_address);
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 106)   int result = sockets_->RecvFrom(
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 107)       socket_, &payload, sizeof(payload), 0,
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 108)       reinterpret_cast<struct sockaddr*>(&remote_address),
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 109)       &socklen);
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 110)   if (result < 0) {
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 111)     PLOG(ERROR) << "Socket recvfrom failed";
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 112)     Stop();
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 113)     return;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 114)   }
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 115) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 116)   if (result != sizeof(payload)) {
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 117)     LOG(INFO) << "Short EAP packet received";
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 118)     return;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 119)   }
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 120) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 121)   if (payload.onex_header.version < eap_protocol::kIeee8021xEapolVersion1 ||
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 122)       payload.onex_header.type != eap_protocol::kIIeee8021xTypeEapPacket ||
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 123)       payload.eap_header.code != eap_protocol::kEapCodeRequest) {
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 124)     LOG(INFO) << "Packet is not a valid EAP request";
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 125)     return;
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 126)   }
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 127) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 128)   request_received_callback_.Run();
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 129) }
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 130) 
db66d0fab3 (Mike Frysinger 2018-07-23 22:14:39 -0400 131) }  // namespace shill

 
Status: WontFix (was: Untriaged)
i don't think there's much we can do about it at this point in the git history itself.  db66d0fab3 is the revert of the deletion of shill so we only had to pull in newer commits to the file.

the only way in the history to address this would have been to re-import all of the shill history from scratch which would be pretty wasteful imo and not accurately reflect what happened.

seems like `git hyper-blame` might help, but i don't think that handles deletions+additions correctly.  so you'll have to do it manually:
git blame e6bebd36a8cb0db343d0895a3f6508aa2638f343^ <file>
Blocking: 785137
Cc: briannorris@chromium.org benchan@chromium.org
Mike,
This is going to slow down the networking team heavily as a few of us at least rely on having the git history to follow unknown areas. 
This is sad because the migration only bought us cosmetic benefits. 
I put this line in my .{bash,zsh}rc:
alias shame='git blame e6bebd36a8cb0db343d0895a3f6508aa2638f343^'
i disagree that having shill in platform2-vs-aosp is only cosmetic benefits

but unless someone has some tips for how to run git blame in a way that ignores specific commits, this is just how it is.  we can't rewrite the history again, and considering the AOSP shill git history is about ~30megs, adding another 30megs to platform2 just to make git blame fully accurate doesn't seem worth it imo.
Okay, git blame e6bebd36a8cb0db343d0895a3f6508aa2638f343^ <file> misses out on too many relevant changes.

I attached a script that creates a shill_history branch in platform2, which pulls in all the history from the previous shill repo. After running the script, `git blame shill_history <file>` will work as before. Shill commits that go in after the move back to platform2 will not be included in those git blame runs, but it's better than nothing.
shill_history.sh
1005 bytes View Download
Alex, what exactly is your problem with 'git blame e6bebd36a8cb0db343d0895a3f6508aa2638f343^'? Do note that we *do* have all the AOSP history (history from ~Sept 2016 to 2018) in platform2 already, and that should show up if you just do an unadorned 'git blame'. IIUC, they problem you are having is with seeing pre-approx-2015 history, cause there's a big inflection point in history -- dropping all our code in commit e6bebd36a8cb0db343d0895a3f6508aa2638f343.

But that still doesn't mean you lost any pre-2015 history either. You just have to look past commit e6bebd36a8cb0db343d0895a3f6508aa2638f343.

So I think things work fine with:

~2015 onward (AOSP move): just use git blame
pre-Sept-2015 (pre-AOSP move): use git blame e6bebd36a8cb0db343d0895a3f6508aa2638f343^

Or maybe I'm missing something. For one:

> misses out on too many relevant changes.

what relevant change(s), specifically, do you miss?
> what relevant change(s), specifically, do you miss?
You just answered your own question. You miss the ~2015 onward changes by using git blame e6bebd36a8cb0db343d0895a3f6508aa2638f343^.

This is clearly not a discussion that is going anywhere. It just seemed to me that 30 megs is not so great a price to for consistent git blame behavior.
Have a nice day.
you might see it, but i've had to fight over far less than 30MB when it comes to repo sizes

Sign in to add a comment